diff --git a/.changeset/oauth-popup-reserve-on-click.md b/.changeset/oauth-popup-reserve-on-click.md new file mode 100644 index 0000000000..cf8d9430a5 --- /dev/null +++ b/.changeset/oauth-popup-reserve-on-click.md @@ -0,0 +1,9 @@ +--- +"@executor-js/react": patch +--- + +**A slow OAuth discovery no longer kills the connect with no popup and no error** + +The transparent connect flows opened the sign-in window only after their setup round trips had answered: DCR after probe and dynamic registration, CIMD after minting the client, reconnect after starting the session. `window.open` needs transient user activation, which browsers expire a few seconds after the click, so once the API was slow enough the browser refused the window and the connect ended with nothing on screen but the button returning to "Connect". Every MCP integration takes that path. + +The window is now claimed on the click itself and navigated when the authorization URL arrives, however long that takes, and it is closed again on the paths that end without signing in (failed probe, no registration endpoint, rejected registration, failed client mint) as well as on cancel and unmount. A window the browser does refuse is now reported instead of swallowed: the flows stop before their round trips, and the sign-in error renders above the dialog footer, where the automatic flows can actually show it, rather than inside a method tab panel they never render. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 53065b9d50..49b9c446db 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -10,6 +10,18 @@ concurrency: group: ci-${{ github.ref }} cancel-in-progress: true +# CI is not a user. Without this, executor's own product telemetry reports to the +# production PostHog project from every e2e shard: the analytics layer +# (integration_added, execution_completed, artifact_*) and the +# integrations-registry fetch, which lands as a `hit` event. Each shard is a +# fresh container, so it mints a fresh anonymous id and arrives on a fresh +# runner IP — inflating machine and user counts by roughly an order of +# magnitude. Both opt-outs are the packages' documented CI/test hooks. +env: + DO_NOT_TRACK: "1" + EXECUTOR_DISABLE_ANALYTICS: "1" + EXECUTOR_DISABLE_INTEGRATIONS_FETCH: "1" + jobs: changes: name: Changed paths diff --git a/.github/workflows/publish-desktop.yml b/.github/workflows/publish-desktop.yml index 090e0d6d40..54f22c7ce7 100644 --- a/.github/workflows/publish-desktop.yml +++ b/.github/workflows/publish-desktop.yml @@ -21,6 +21,18 @@ concurrency: group: publish-desktop-${{ github.ref }} cancel-in-progress: false +# CI is not a user. Without this, executor's own product telemetry reports to the +# production PostHog project from every e2e shard: the analytics layer +# (integration_added, execution_completed, artifact_*) and the +# integrations-registry fetch, which lands as a `hit` event. Each shard is a +# fresh container, so it mints a fresh anonymous id and arrives on a fresh +# runner IP — inflating machine and user counts by roughly an order of +# magnitude. Both opt-outs are the packages' documented CI/test hooks. +env: + DO_NOT_TRACK: "1" + EXECUTOR_DISABLE_ANALYTICS: "1" + EXECUTOR_DISABLE_INTEGRATIONS_FETCH: "1" + jobs: build: permissions: diff --git a/.github/workflows/publish-selfhost-docker.yml b/.github/workflows/publish-selfhost-docker.yml index 1da6470320..a70192b13b 100644 --- a/.github/workflows/publish-selfhost-docker.yml +++ b/.github/workflows/publish-selfhost-docker.yml @@ -16,6 +16,18 @@ concurrency: group: publish-selfhost-docker-${{ inputs.tag }} cancel-in-progress: false +# CI is not a user. Without this, executor's own product telemetry reports to the +# production PostHog project from every e2e shard: the analytics layer +# (integration_added, execution_completed, artifact_*) and the +# integrations-registry fetch, which lands as a `hit` event. Each shard is a +# fresh container, so it mints a fresh anonymous id and arrives on a fresh +# runner IP — inflating machine and user counts by roughly an order of +# magnitude. Both opt-outs are the packages' documented CI/test hooks. +env: + DO_NOT_TRACK: "1" + EXECUTOR_DISABLE_ANALYTICS: "1" + EXECUTOR_DISABLE_INTEGRATIONS_FETCH: "1" + jobs: metadata: runs-on: blacksmith-4vcpu-ubuntu-2404 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 241d4757f8..171b42a14a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -13,6 +13,18 @@ concurrency: group: release-${{ github.ref }} cancel-in-progress: false +# CI is not a user. Without this, executor's own product telemetry reports to the +# production PostHog project from every e2e shard: the analytics layer +# (integration_added, execution_completed, artifact_*) and the +# integrations-registry fetch, which lands as a `hit` event. Each shard is a +# fresh container, so it mints a fresh anonymous id and arrives on a fresh +# runner IP — inflating machine and user counts by roughly an order of +# magnitude. Both opt-outs are the packages' documented CI/test hooks. +env: + DO_NOT_TRACK: "1" + EXECUTOR_DISABLE_ANALYTICS: "1" + EXECUTOR_DISABLE_INTEGRATIONS_FETCH: "1" + jobs: release: runs-on: ubuntu-latest diff --git a/apps/cloud/scripts/start-closure.mjs b/apps/cloud/scripts/start-closure.mjs index 0b45c22f16..a841cb5977 100644 --- a/apps/cloud/scripts/start-closure.mjs +++ b/apps/cloud/scripts/start-closure.mjs @@ -12,7 +12,10 @@ // cold isolate before any request is served. // start - the TanStack Start server graph, reached through the lazy // `loadEntries` dynamic imports. Evaluated on the first request -// that enters the app. +// that enters the app - i.e. a page request. +// app - the Effect app plane. `/api/*` dispatches at the Worker entry and +// skips Start entirely, so an API request evaluates this instead of +// `start`; reported separately because the two planes now diverge. // // Anything reachable only through a dynamic import is not counted: making a // heavy dependency lazy is exactly the outcome this rewards. @@ -81,6 +84,9 @@ const startRoots = [...graph.get(ENTRY).dynamic].filter((f) => /(start|router|tanstack)/.test(name(f)), ); const start = closure(startRoots); +const appRoots = [...graph.get(ENTRY).dynamic].filter((f) => /\/app-[A-Za-z0-9_-]+\.js$/.test(f)); +const app = closure([ENTRY, ...appRoots]); +// The budget tracks the worst plane: whichever costs a cold isolate more. const evaluated = new Set([...startup, ...start]); const report = (label, files) => { @@ -93,7 +99,10 @@ const report = (label, files) => { report("startup", startup); report("start", start); -console.log(`\ntotal evaluated on a warm-path request: ${mb(bytes(evaluated))}`); +console.log(`\npage request (startup + start): ${mb(bytes(evaluated))}`); +console.log( + `API request (startup + app): ${mb(bytes(app))}${appRoots.length ? "" : " [no app chunk - /api still routes through Start]"}`, +); const lazyOnly = [...graph.keys()].filter((f) => !evaluated.has(f)); console.log( `deferred behind dynamic import: ${mb(bytes(lazyOnly))} (${lazyOnly.length} chunks)`, diff --git a/apps/cloud/src/app-paths.test.ts b/apps/cloud/src/app-paths.test.ts index 98aeff3bdc..dd9ff1af3c 100644 --- a/apps/cloud/src/app-paths.test.ts +++ b/apps/cloud/src/app-paths.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "@effect/vitest"; -import { isAppOwnedPath } from "./app-paths"; +import { isAppOwnedPath, servedByAppPlane } from "./app-paths"; // Guards the start.ts dispatch decision: every surface the unified app handler // serves must be classified app-owned (forwarded to `app.handler`), and Start's @@ -62,3 +62,39 @@ describe("isAppOwnedPath", () => { }); } }); + +describe("app-plane dispatch", () => { + // These two are the whole risk of dispatching `/api` before Start: both still + // return a response if routed early, just the wrong one, so nothing else would + // catch a regression here. + it("leaves the Sentry tunnel POST to Start's middleware", () => { + expect(servedByAppPlane("/api/sentry-tunnel", "POST")).toBe(false); + // Only the POST is claimed; anything else under that path is ordinary API. + expect(servedByAppPlane("/api/sentry-tunnel", "GET")).toBe(true); + }); + + it("leaves the OAuth callback to Start, for the signed-out redirect", () => { + expect(servedByAppPlane("/api/oauth/callback", "GET")).toBe(false); + expect(servedByAppPlane("/api/oauth/callback", "POST")).toBe(false); + }); + + const appPlane = [ + "/api/connections", + "/api/tools", + "/api/integrations", + "/api/account/members", + "/api/docs", + "/api/billing/checkout", + ]; + for (const pathname of appPlane) { + it(`serves ${pathname} without entering Start`, () => { + expect(servedByAppPlane(pathname, "GET")).toBe(true); + }); + } + + it("never claims a non-API path, however app-owned", () => { + expect(servedByAppPlane("/mcp", "POST")).toBe(false); + expect(servedByAppPlane("/", "GET")).toBe(false); + expect(servedByAppPlane("/.well-known/oauth-authorization-server", "GET")).toBe(false); + }); +}); diff --git a/apps/cloud/src/app-paths.ts b/apps/cloud/src/app-paths.ts index 6dd6fc56f5..48e70a8c3a 100644 --- a/apps/cloud/src/app-paths.ts +++ b/apps/cloud/src/app-paths.ts @@ -17,3 +17,31 @@ export const isApiPath = (pathname: string) => pathname === "/api" || pathname.s export const isAppOwnedPath = (pathname: string) => isApiPath(pathname) || classifyMcpPath(pathname) !== null; + +// --------------------------------------------------------------------------- +// Which plane serves an app-owned path: the Effect app directly, or TanStack +// Start's middleware chain. +// +// Everything under `/api` is pure Effect and touches no part of the router, +// React, or SSR — so `server.ts` dispatches it at the Worker entry and skips +// Start's lazy `loadEntries` import entirely. Two paths must NOT take that +// shortcut, because Start's request middleware claims them BEFORE the app +// handler would ever see them: +// +// POST /api/sentry-tunnel - `sentryTunnelMiddleware` forwards the envelope +// to Sentry; the app has no such route. +// /api/oauth/callback - `oauthCallbackSignInMiddleware` redirects a +// signed-out visitor to /login, and start.ts +// rewrites the org-scoped `state` before handing +// off. Routing it early would drop both. +// +// Getting this wrong is silent: the request still gets a response, just the +// wrong one, which is why it is classified here and tested rather than being +// an inline condition at the dispatch site. +// --------------------------------------------------------------------------- + +export const isStartOwnedApiPath = (pathname: string, method: string): boolean => + (pathname === "/api/sentry-tunnel" && method === "POST") || pathname === "/api/oauth/callback"; + +export const servedByAppPlane = (pathname: string, method: string): boolean => + isApiPath(pathname) && !isStartOwnedApiPath(pathname, method); diff --git a/apps/cloud/src/engine/execution-rate-limit.node.test.ts b/apps/cloud/src/engine/execution-rate-limit.node.test.ts new file mode 100644 index 0000000000..22d570bcec --- /dev/null +++ b/apps/cloud/src/engine/execution-rate-limit.node.test.ts @@ -0,0 +1,171 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Data, Effect } from "effect"; + +import type { ExecutionEngine } from "@executor-js/execution"; + +import { makeExecutionRateLimiter } from "./execution-rate-limit"; +import { RATE_LIMIT_BLOCKED_MESSAGE } from "./execution-limit-messages"; + +const ORG = "org_test"; + +/** Stands in for whatever the real lookups fail with (Autumn down, DO down). */ +class UpstreamDownError extends Data.TaggedError("UpstreamDownError")<{ + readonly which: string; +}> {} + +// A stand-in engine: `execute` resolves to a marker, so a test tells an allowed +// execution (marker) from a blocked one (the gate's error result) by which of +// the two came back. A blocked decision never reaches the engine at all. +const engineStub: ExecutionEngine = { + execute: () => Effect.succeed({ result: "ran" }), + executeWithPause: () => Effect.succeed({ status: "completed", result: { result: "ran" } }), + resume: () => Effect.succeed(null), + getPausedExecution: () => Effect.succeed(null), + pausedExecutionCount: () => Effect.succeed(0), + hasPausedExecutions: () => Effect.succeed(false), + getDescription: Effect.succeed("stub"), +}; + +/** Counter that hands out a caller-controlled sequence of counts. */ +const countingIncrement = (counts: ReadonlyArray) => { + let calls = 0; + return () => Effect.succeed(counts[Math.min(calls++, counts.length - 1)] ?? 0); +}; + +const runExecute = (limiter: ReturnType) => + Effect.runPromise( + limiter.decorate(ORG, engineStub).execute("code", { + // Never invoked: the stub ignores it, and a blocked execution never runs. + onElicitation: () => Effect.die("elicitation is not exercised here"), + }), + ); + +describe("execution rate limiter — paid exemption", () => { + it("allows executions under the cap without consulting the exemption", async () => { + let exemptionCalls = 0; + const limiter = makeExecutionRateLimiter(countingIncrement([1]), { + limit: 10, + isExempt: () => { + exemptionCalls += 1; + return Effect.succeed(false); + }, + }); + + expect(await runExecute(limiter)).toMatchObject({ result: "ran" }); + // The whole point of resolving lazily: the common path costs no lookup. + expect(exemptionCalls).toBe(0); + }); + + it("blocks a non-exempt org over the cap", async () => { + const limiter = makeExecutionRateLimiter(countingIncrement([11]), { + limit: 10, + isExempt: () => Effect.succeed(false), + }); + + expect(await runExecute(limiter)).toMatchObject({ + result: null, + error: RATE_LIMIT_BLOCKED_MESSAGE, + }); + }); + + it("allows an exempt org over the cap", async () => { + const limiter = makeExecutionRateLimiter(countingIncrement([11]), { + limit: 10, + isExempt: () => Effect.succeed(true), + }); + + expect(await runExecute(limiter)).toMatchObject({ result: "ran" }); + }); + + it("caches the exemption so a paid org past the cap looks it up once", async () => { + let exemptionCalls = 0; + const limiter = makeExecutionRateLimiter(countingIncrement([11, 12, 13]), { + limit: 10, + exemptionTtlMs: 60_000, + now: () => 1_000, + isExempt: () => { + exemptionCalls += 1; + return Effect.succeed(true); + }, + }); + + for (let i = 0; i < 3; i += 1) + expect(await runExecute(limiter)).toMatchObject({ result: "ran" }); + expect(exemptionCalls).toBe(1); + }); + + it("re-resolves once the cached exemption expires", async () => { + let exemptionCalls = 0; + let clock = 1_000; + const limiter = makeExecutionRateLimiter(countingIncrement([11, 12]), { + limit: 10, + exemptionTtlMs: 1_000, + now: () => clock, + isExempt: () => { + exemptionCalls += 1; + return Effect.succeed(true); + }, + }); + + await runExecute(limiter); + clock += 5_000; + await runExecute(limiter); + expect(exemptionCalls).toBe(2); + }); + + it("blocks when the exemption cannot be resolved and nothing is cached", async () => { + // Deliberately NOT fail-open: an unresolvable exemption during an Autumn + // outage must not switch the backstop off, which is the one scenario it + // exists to cover. + const limiter = makeExecutionRateLimiter(countingIncrement([11]), { + limit: 10, + isExempt: () => Effect.fail(new UpstreamDownError({ which: "autumn" })), + }); + + expect(await runExecute(limiter)).toMatchObject({ + result: null, + error: RATE_LIMIT_BLOCKED_MESSAGE, + }); + }); + + it("honours a stale exemption when a later lookup fails", async () => { + let clock = 1_000; + let shouldFail = false; + const limiter = makeExecutionRateLimiter(countingIncrement([11, 12]), { + limit: 10, + exemptionTtlMs: 1_000, + now: () => clock, + isExempt: () => + shouldFail ? Effect.fail(new UpstreamDownError({ which: "autumn" })) : Effect.succeed(true), + }); + + expect(await runExecute(limiter)).toMatchObject({ result: "ran" }); + + // Cache expires and Autumn is now unreachable: the known-paid org keeps + // running rather than getting blocked mid-workload by a blip. + clock += 5_000; + shouldFail = true; + expect(await runExecute(limiter)).toMatchObject({ result: "ran" }); + }); + + it("fails open when the counter itself is unreachable", async () => { + const limiter = makeExecutionRateLimiter( + () => Effect.fail(new UpstreamDownError({ which: "counter DO" })), + { + limit: 10, + isExempt: () => Effect.succeed(false), + }, + ); + + expect(await runExecute(limiter)).toMatchObject({ result: "ran" }); + }); + + it("applies the cap when no exemption predicate is wired", async () => { + const limiter = makeExecutionRateLimiter(countingIncrement([11]), { limit: 10 }); + + expect(await runExecute(limiter)).toMatchObject({ + result: null, + error: RATE_LIMIT_BLOCKED_MESSAGE, + }); + }); +}); diff --git a/apps/cloud/src/engine/execution-rate-limit.ts b/apps/cloud/src/engine/execution-rate-limit.ts index b68d4c044e..3b5ea8194c 100644 --- a/apps/cloud/src/engine/execution-rate-limit.ts +++ b/apps/cloud/src/engine/execution-rate-limit.ts @@ -1,5 +1,5 @@ // --------------------------------------------------------------------------- -// Per-org execution rate limit — an abuse backstop independent of billing. +// Per-org execution rate limit — a free-tier abuse backstop. // // The balance gate (execution-gate.ts) depends on Autumn and fails open, so a // billing outage plus runaway automation could still run unbounded executions. @@ -8,9 +8,22 @@ // each MCP session lives in its own DO instance, so an in-memory counter // would be per-session and trivially bypassed by opening more sessions). // -// Like the balance gate it FAILS OPEN: an unreachable counter DO, a missing +// Paid organizations are EXEMPT. The cap was sized for free-tier abuse but +// applied to everyone, and on 2026-08-18 it blocked a paying customer +// mid-workload — their agent gave up on Executor and routed around it. Paid +// usage is what the balance gate and metered overage are for; this backstop +// has no business capping it. +// +// The exemption is resolved ONLY once the counter reports an org over the cap, +// so the common path (under the cap) costs the counter increment and nothing +// else. `isExempt` is an opaque predicate: this module still names no billing +// concept, and the Autumn coupling lives in `execution-stack-metered.ts`, +// which already owns that dependency. +// +// FAIL OPEN applies to the COUNTER: an unreachable counter DO, a missing // binding, or a slow call allows the execution (warn + Sentry). The backstop -// must never take executions down with it. +// must never take executions down with it. An unresolved EXEMPTION is the one +// thing that does not fail open — see `resolveExemption`. // --------------------------------------------------------------------------- import { DurableObject, env } from "cloudflare:workers"; @@ -25,14 +38,26 @@ import { RATE_LIMIT_BLOCKED_MESSAGE } from "./execution-limit-messages"; // Fixed window: all executions in the same clock hour share one counter. export const RATE_LIMIT_WINDOW_MS = 3_600_000; -// Calibration: the heaviest legitimate org runs ~1.1k executions per MONTH, -// so 1000 per HOUR is far above any human-driven usage and only trips on -// runaway automation (the incident this backstops: ~18k in 30 days would -// still pass, which is fine — that class of overrun is the balance gate's -// job; this catches tight loops). +// The cap for organizations WITHOUT a paid subscription. +// +// The original calibration ("the heaviest legitimate org runs ~1.1k executions +// per MONTH, so 1000 per HOUR is far above any human-driven usage") went stale +// inside six weeks: by 2026-08-18 a paying org was sustaining ~5.3k executions +// per DAY and crossed this cap in a single hour. Sizing a shared number +// against the largest customer is a losing game, so the number no longer tries +// to describe them — paid orgs are exempt below, and this now has only +// free-tier abuse to cover, which is what it was picked for. export const EXECUTIONS_PER_ORG_PER_HOUR = 1000; // Counter DO slower than this => fail open rather than stall executions. const RATE_LIMIT_CHECK_TIMEOUT_MS = 2_000; +// Exemption lookup slower than this => treat as unresolved. +const EXEMPTION_CHECK_TIMEOUT_MS = 2_000; +// An org over the cap is checked at most once per TTL rather than once per +// execution, so a paid org running far past it doesn't hammer the lookup. +const EXEMPTION_CACHE_TTL_MS = 60_000; +// Sweep guard, mirroring the balance gate's cache: one long-lived isolate can +// serve many orgs. +const EXEMPTION_CACHE_MAX_ENTRIES = 10_000; // The DO purges its storage this long after the last increment, so idle orgs // cost nothing. Two windows: long enough that an active window never purges. const COUNTER_PURGE_AFTER_MS = 2 * RATE_LIMIT_WINDOW_MS; @@ -51,6 +76,20 @@ class RateLimitCheckTimeoutError extends Data.TaggedError("RateLimitCheckTimeout readonly timeoutMs: number; }> {} +/** Internal sentinel for an exemption lookup that exceeded its time budget. */ +class ExemptionCheckTimeoutError extends Data.TaggedError("ExemptionCheckTimeoutError")<{ + readonly timeoutMs: number; +}> {} + +/** + * Whether an organization is exempt from the cap. Production passes a paid- + * subscription check; keeping it an opaque predicate is what lets this module + * stay free of any billing import. + */ +export type ExecutionRateLimitExemption = ( + organizationId: string, +) => Effect.Effect; + // --------------------------------------------------------------------------- // Counter Durable Object — one instance per organization (idFromName(orgId)). // Stores a single { windowId, count } record: an increment in a new window @@ -119,12 +158,76 @@ export const makeExecutionRateLimiter = ( readonly windowMs?: number; readonly timeoutMs?: number; readonly now?: () => number; + readonly isExempt?: ExecutionRateLimitExemption; + readonly exemptionTtlMs?: number; }, ): ExecutionRateLimiter => { const limit = options?.limit ?? EXECUTIONS_PER_ORG_PER_HOUR; const windowMs = options?.windowMs ?? RATE_LIMIT_WINDOW_MS; const timeoutMs = options?.timeoutMs ?? RATE_LIMIT_CHECK_TIMEOUT_MS; const now = options?.now ?? Date.now; + const isExempt = options?.isExempt; + const exemptionTtlMs = options?.exemptionTtlMs ?? EXEMPTION_CACHE_TTL_MS; + + const exemptionCache = new Map< + string, + { readonly exempt: boolean; readonly expiresAtMs: number } + >([]); + + const writeExemptionCache = (organizationId: string, exempt: boolean, nowMs: number): void => { + if (exemptionCache.size >= EXEMPTION_CACHE_MAX_ENTRIES) { + for (const [key, entry] of exemptionCache) { + if (entry.expiresAtMs <= nowMs) exemptionCache.delete(key); + } + // Still saturated after dropping expired entries: reset rather than grow. + if (exemptionCache.size >= EXEMPTION_CACHE_MAX_ENTRIES) exemptionCache.clear(); + } + exemptionCache.set(organizationId, { exempt, expiresAtMs: nowMs + exemptionTtlMs }); + }; + + /** + * Resolved only for orgs already over the cap, so the lookup never touches + * the common path. + * + * This is the one place that does NOT fail open. The balance gate already + * allows executions when Autumn is unreachable; if the exemption did too, + * an Autumn outage would switch this backstop off entirely — precisely the + * "billing outage plus runaway automation" case it exists to cover. A stale + * positive is honoured ahead of that fallback, so a blip can't flip a + * known-paid org into a block mid-workload. + */ + const resolveExemption = (organizationId: string): Effect.Effect => + Effect.suspend(() => { + if (!isExempt) return Effect.succeed(false); + const nowMs = now(); + const cached = exemptionCache.get(organizationId); + if (cached && cached.expiresAtMs > nowMs) return Effect.succeed(cached.exempt); + return isExempt(organizationId).pipe( + Effect.timeoutOrElse({ + duration: `${EXEMPTION_CHECK_TIMEOUT_MS} millis`, + orElse: () => + Effect.fail(new ExemptionCheckTimeoutError({ timeoutMs: EXEMPTION_CHECK_TIMEOUT_MS })), + }), + Effect.map((exempt) => { + writeExemptionCache(organizationId, exempt, nowMs); + return exempt; + }), + Effect.catch((error: unknown) => + Effect.gen(function* () { + yield* Effect.sync(() => { + console.warn( + `[rate-limit] exemption lookup failed for ${organizationId}; treating as ${ + cached ? "last known" : "not exempt" + }:`, + error, + ); + }); + yield* captureCauseEffect(error); + return cached?.exempt ?? false; + }), + ), + ); + }); const decide = (organizationId: string): Effect.Effect => Effect.suspend(() => { @@ -134,18 +237,34 @@ export const makeExecutionRateLimiter = ( duration: `${timeoutMs} millis`, orElse: () => Effect.fail(new RateLimitCheckTimeoutError({ timeoutMs })), }), - Effect.map( - (count): GateDecision => - count > limit - ? { - blocked: true, - error: new ExecutionRateLimitExceededError({ - organizationId, - message: RATE_LIMIT_BLOCKED_MESSAGE, - }), - } - : { blocked: false }, - ), + Effect.flatMap((count): Effect.Effect => { + // Under the cap: no exemption lookup, no extra I/O. + if (count <= limit) return Effect.succeed({ blocked: false }); + return Effect.gen(function* () { + if (yield* resolveExemption(organizationId)) { + return { blocked: false } as const satisfies GateDecision; + } + // The only record that the backstop fired. A blocked execution is + // never usage-tracked (the gate short-circuits before the tracker) + // and deliberately not sent to Sentry — a backstop stopping + // runaway automation is expected, not exceptional — so without + // this line a blocked org is invisible outside a bug report, which + // is how the 2026-08-18 block went unnoticed until a customer + // sent a screenshot. + yield* Effect.sync(() => { + console.warn( + `[rate-limit] blocked execution for ${organizationId}: ${count} > ${limit} in window ${windowId}`, + ); + }); + return { + blocked: true, + error: new ExecutionRateLimitExceededError({ + organizationId, + message: RATE_LIMIT_BLOCKED_MESSAGE, + }), + } as const satisfies GateDecision; + }); + }), // FAIL OPEN: the backstop must never block executions because its // counter is unreachable or slow. Effect.catch((error: unknown) => @@ -185,8 +304,13 @@ type RateLimiterNamespace = { * Production rate limiter backed by the `EXECUTION_RATE_LIMITER` counter DO. * When the binding is absent (unit-test workers, older local setups) the * limiter is disabled: every check passes, logged once at construction. + * + * `isExempt` decides which orgs the cap skips; production passes a paid- + * subscription check from `execution-stack-metered.ts`. */ -export const makeCloudExecutionRateLimiter = (): ExecutionRateLimiter => { +export const makeCloudExecutionRateLimiter = ( + isExempt: ExecutionRateLimitExemption, +): ExecutionRateLimiter => { const limit = resolveRateLimit(); const namespace = (env as { EXECUTION_RATE_LIMITER?: RateLimiterNamespace }) .EXECUTION_RATE_LIMITER; @@ -204,7 +328,7 @@ export const makeCloudExecutionRateLimiter = (): ExecutionRateLimiter => { ) as ExecutionRateLimiterStub; return stub.increment(windowId); }), - { limit }, + { limit, isExempt }, ); }; diff --git a/apps/cloud/src/engine/execution-stack-metered.ts b/apps/cloud/src/engine/execution-stack-metered.ts index 4ea70b6d43..6063e1456c 100644 --- a/apps/cloud/src/engine/execution-stack-metered.ts +++ b/apps/cloud/src/engine/execution-stack-metered.ts @@ -26,6 +26,7 @@ import { } from "@executor-js/api/server"; import { AutumnService } from "../extensions/billing/service"; +import { hasPaidOrganizationSubscription } from "../extensions/billing/plans"; import type { DbService } from "../db/db"; import { CloudExecutionSeamsLayer } from "../engine/execution-stack"; import { makeExecutionLimitGate } from "./execution-gate"; @@ -34,7 +35,8 @@ import { withExecutionUsageTracking } from "./execution-usage"; // Usage-metering decorator bound to the billing service, plus the two // pre-execution guards this layer owns, ordered cheapest first: -// 1. rate-limit backstop (counter DO, independent of billing) +// 1. rate-limit backstop (counter DO; free-tier abuse only — paid orgs are +// exempt via the subscription lookup below) // 2. execution balance gate (Autumn check, cached 60s, fails open) // 3. usage tracking — fire-and-forget (`Effect.runFork`) so the billing // call can't stall a user-facing execution. @@ -48,7 +50,18 @@ export const CloudMeteringEngineDecorator: Layer.Layer autumn.checkExecutionBalance(organizationId), ); - const rateLimiter = makeCloudExecutionRateLimiter(); + // The limiter's paid-org exemption. This is the billing coupling the + // limiter module deliberately avoids owning, and it reads the same + // `PAID_AUTUMN_PLAN_IDS` config as the org-creation and seat gates so + // "paid" means one thing across the app. The limiter calls this only + // for orgs already over the cap and caches the answer, so the extra + // Autumn round trip stays off the hot path. + const rateLimiter = makeCloudExecutionRateLimiter((organizationId) => + Effect.map( + autumn.use((client) => client.customers.getOrCreate({ customerId: organizationId })), + (customer) => hasPaidOrganizationSubscription(customer.subscriptions), + ), + ); return { decorate: (engine, identity: EngineStackIdentity) => rateLimiter.decorate( diff --git a/apps/cloud/src/server.ts b/apps/cloud/src/server.ts index 30624f3b44..ec4771e962 100644 --- a/apps/cloud/src/server.ts +++ b/apps/cloud/src/server.ts @@ -11,7 +11,7 @@ import { import * as Sentry from "@sentry/cloudflare"; import handler from "@tanstack/react-start/server-entry"; -import { isAppOwnedPath } from "./app-paths"; +import { isAppOwnedPath, servedByAppPlane } from "./app-paths"; import { marketingProxyRequest } from "./edge/marketing"; import { passthroughResponse } from "./edge/passthrough"; import { makeCloudMcpAgentHandler } from "./mcp/agent-handler"; @@ -244,6 +244,42 @@ const markStartGraphEntered = (): void => { startGraphEntered = true; }; +// --------------------------------------------------------------------------- +// Serving `/api/*` without entering TanStack Start. +// --------------------------------------------------------------------------- +// +// Everything under `/api` is the Effect app (`ExecutorApp.make`'s web handler) +// and uses no part of the router, React, or SSR. But it was dispatched from a +// Start *request middleware*, so reaching it meant paying Start's lazy +// `loadEntries` import of the whole server graph first. Measured on production +// 2026-08-19, splitting `/api/*` by whether the isolate had already loaded that +// graph: warm p50 **186ms**, cold p50 **2129ms**, with 28% of API requests cold. +// The dashboard fires many `/api/*` calls in parallel and waits for the slowest, +// so that cold tail is what the app actually feels like. +// +// So `/api` joins marketing, `/docs`, the PostHog proxy and `/mcp` at the Worker +// entry: classify and dispatch before anything touches Start. The evaluated +// closure for an API request drops from the full Start graph to the Worker's own +// (see `scripts/start-closure.mjs`). +// +// `servedByAppPlane` (./app-paths) decides which paths qualify — two under +// `/api` are claimed by Start's middleware first and must keep their old route. + +// Instantiated on the first request that needs it and memoized per isolate, +// mirroring `start.ts`'s `getApp`. The import stays dynamic so an isolate that +// only serves pages or proxies never evaluates the app graph at all. +let appPlane: ReturnType | undefined; +let appGraphEntered = false; + +const getAppPlane = async (): Promise> => { + if (appPlane === undefined) { + const { cloudApiHandler } = await import("./app"); + appPlane = cloudApiHandler(); + appGraphEntered = true; + } + return appPlane; +}; + const cloudflareHandler: ExportedHandler = { fetch: async (request, env, ctx) => { isolateRequestSeq += 1; @@ -330,13 +366,19 @@ const cloudflareHandler: ExportedHandler = { span.setAttribute("executor.start_graph.entered", startGraphEntered); span.setAttribute("executor.isolate.id", isolate.id); span.setAttribute("executor.isolate.age_ms", isolate.ageMs); + // Which plane served this: "app" skipped the Start graph entirely, so + // `start_graph.entered` says nothing about it. `app_graph.entered` + // is the app-plane analogue - false means this request paid for the + // Effect graph's first evaluation in this isolate. + const appPlaneRequest = servedByAppPlane(url.pathname, request.method); + span.setAttribute("executor.dispatch.plane", appPlaneRequest ? "app" : "start"); + if (appPlaneRequest) span.setAttribute("executor.app_graph.entered", appGraphEntered); // oxlint-disable-next-line executor/no-try-catch-or-throw -- adapter boundary; observe response/error for span status, keep the flush alive past the response try { - const response = await fetchHandler( - withTraceparent(request, span.spanContext()), - env, - ctx, - ); + const traced = withTraceparent(request, span.spanContext()); + const response = appPlaneRequest + ? await (await getAppPlane()).handler(prepareMcpOrgScope(traced)) + : await fetchHandler(traced, env, ctx); span.setAttribute(ATTR_HTTP_RESPONSE_STATUS_CODE, response.status); return response; } catch (err) { diff --git a/apps/cloud/src/start.ts b/apps/cloud/src/start.ts index be55277636..ebdf8256c6 100644 --- a/apps/cloud/src/start.ts +++ b/apps/cloud/src/start.ts @@ -1,7 +1,6 @@ import { createMiddleware, createStart } from "@tanstack/react-start"; import { decodeOAuthCallbackState } from "@executor-js/sdk/shared"; -import { cloudApiHandler } from "./app"; import { isAppOwnedPath } from "./app-paths"; import { authGateMiddleware } from "./auth/ssr-gate"; import { parseCookie } from "./auth/cookies"; @@ -35,8 +34,18 @@ import { // (a workerd-only virtual module) into the browser build, breaking it. Keeping // the call inside the server callback mirrors how every other server concern // here stays server-only. -let app: ReturnType | undefined; -const getApp = () => (app ??= cloudApiHandler()); +// +// The IMPORT is dynamic for a second, server-side reason: `server.ts` now +// dispatches `/api/*` at the Worker entry, so the only paths that still reach +// this middleware are the two Start's own chain claims first (the Sentry tunnel +// and the OAuth callback's signed-out redirect) plus `/mcp`. A static import +// would still put the entire Effect app in the graph every SSR page load +// evaluates — 3.06 MB of the 12.94 MB page closure, for code a page never runs. +// Deferring it takes the page closure to 9.88 MB; `scripts/start-closure.mjs` +// reports both planes and will show it coming back if this becomes static. +let app: ReturnType | undefined; +const getApp = async (): Promise> => + (app ??= (await import("./app")).cloudApiHandler()); const SESSION_COOKIE = "wos-session"; const OAUTH_CALLBACK_PATH = "/api/oauth/callback"; @@ -78,11 +87,11 @@ const oauthCallbackSignInMiddleware = createMiddleware({ type: "request" }).serv // envelope routes, pinning the org in an internal header (a no-op for everything // else, including `/api/*`). const appRequestMiddleware = createMiddleware({ type: "request" }).server( - ({ pathname, request, next }) => { + async ({ pathname, request, next }) => { if (isAppOwnedPath(pathname)) { const scopedRequest = pathname === OAUTH_CALLBACK_PATH ? oauthCallbackOrgScopedRequest(request) : request; - return getApp().handler(prepareMcpOrgScope(scopedRequest)); + return (await getApp()).handler(prepareMcpOrgScope(scopedRequest)); } return next(); }, diff --git a/apps/cloud/wrangler.jsonc b/apps/cloud/wrangler.jsonc index 3fa69306e0..3578de6d25 100644 --- a/apps/cloud/wrangler.jsonc +++ b/apps/cloud/wrangler.jsonc @@ -94,6 +94,14 @@ "bucket_name": "executor-cloud-blobs", }, ], + // Pins the Worker to the Hyperdrive/Postgres region. Added in #420 to fix + // MCP Hyperdrive connection pressure; dropped in #1693 as a placement + // experiment, which took `user_store.*` from ~12ms to ~300ms because every + // query started crossing regions. Restored to measure that against the + // JWKS verify time the experiment also moved. + "placement": { + "region": "aws:us-east-1", + }, "worker_loaders": [ { "binding": "LOADER", diff --git a/e2e/cloud/admin-users-console.test.ts b/e2e/cloud/admin-users-console.test.ts index b30fcb8d83..d5eaa0521f 100644 --- a/e2e/cloud/admin-users-console.test.ts +++ b/e2e/cloud/admin-users-console.test.ts @@ -24,6 +24,7 @@ import { AuthTemplateSlug, ConnectionName, IntegrationSlug } from "@executor-js/ import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; import { accountIdOf, forBrowser, joinOrg } from "./support/session"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); type Client = HttpApiClient.ForApi; @@ -126,7 +127,7 @@ scenario( let slug = ""; await step("Land in the workspace and canonicalize onto the org slug", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page.waitForURL((url) => /^\/[a-z0-9-]+\/?$/.test(url.pathname), { timeout: 30_000, }); @@ -323,7 +324,7 @@ scenario( let slug = ""; await step("Sign in as a plain member of the same workspace", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page.waitForURL((url) => /^\/[a-z0-9-]+\/?$/.test(url.pathname), { timeout: 30_000, }); @@ -345,7 +346,7 @@ scenario( await step( "Reaching the URL directly says no access, not an empty workspace", async () => { - await page.goto(`/${slug}/users`, { waitUntil: "networkidle" }); + await visit(page, `/${slug}/users`); // The denial is the assertion: an empty table here would misreport // a populated workspace as having no users. await page diff --git a/e2e/cloud/auth-routing-flow.test.ts b/e2e/cloud/auth-routing-flow.test.ts index 63210d8933..30efb17cdd 100644 --- a/e2e/cloud/auth-routing-flow.test.ts +++ b/e2e/cloud/auth-routing-flow.test.ts @@ -23,6 +23,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { settle } from "../src/surfaces/browser"; scenario( "Auth routing · signed out → login → onboarding → dashboard, one browser session", @@ -58,7 +59,7 @@ scenario( await step("Create the first org → canonical dashboard at /", async () => { const orgName = "Flow Test Org"; const orgNameInput = page.getByPlaceholder("Northwind Labs"); - await page.waitForLoadState("networkidle"); + await settle(page); await orgNameInput.fill(orgName); await page.waitForTimeout(250); if ((await orgNameInput.inputValue()) !== orgName) { diff --git a/e2e/cloud/billing-trial-checkout-stale.test.ts b/e2e/cloud/billing-trial-checkout-stale.test.ts index ba64ae461d..453a7bfd1f 100644 --- a/e2e/cloud/billing-trial-checkout-stale.test.ts +++ b/e2e/cloud/billing-trial-checkout-stale.test.ts @@ -20,6 +20,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Autumn, Billing, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; scenario( "Billing · completing the trial checkout shows the new plan without a reload", @@ -51,12 +52,12 @@ scenario( // "/undefined/billing/plans" — the billing fetches fire under the bogus // slug, fail, and are never refetched (autumn-js staleTime 60s), leaving // the plans grid empty forever. - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page.waitForURL((url) => /^\/[a-z0-9-]+\/?$/.test(url.pathname), { timeout: 30_000, }); const slug = new URL(page.url()).pathname.split("/").filter(Boolean)[0]; - await page.goto(`/${slug}/billing/plans`, { waitUntil: "networkidle" }); + await visit(page, `/${slug}/billing/plans`); await page.getByRole("heading", { name: "Choose a plan" }).waitFor(); await startTrial.waitFor(); }); diff --git a/e2e/cloud/connect-panel.test.ts b/e2e/cloud/connect-panel.test.ts index 0c5e906c79..12202df779 100644 --- a/e2e/cloud/connect-panel.test.ts +++ b/e2e/cloud/connect-panel.test.ts @@ -6,6 +6,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { visit, settle } from "../src/surfaces/browser"; scenario( "Connect · the agent-connect panel gives working copy for both transports", @@ -17,7 +18,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the Integrations page", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page.getByText("Connect an agent").first().waitFor(); }); @@ -35,7 +36,7 @@ scenario( await step("Switch to Standard I/O", async () => { await page.getByRole("tab", { name: "Standard I/O" }).click(); - await page.waitForLoadState("networkidle"); + await settle(page); }); const stdioCommand = await command(); expect(stdioCommand, "the command changed for stdio").not.toBe(httpCommand); @@ -45,7 +46,7 @@ scenario( await step("Switch back to Remote HTTP", async () => { await page.getByRole("tab", { name: "Remote HTTP" }).click(); - await page.waitForLoadState("networkidle"); + await settle(page); }); expect(await command(), "the HTTP command is restored").toContain("--transport http"); }); diff --git a/e2e/cloud/connection-modal-oauth-abandon.test.ts b/e2e/cloud/connection-modal-oauth-abandon.test.ts index 2b33bb82a9..8e816ced65 100644 --- a/e2e/cloud/connection-modal-oauth-abandon.test.ts +++ b/e2e/cloud/connection-modal-oauth-abandon.test.ts @@ -21,6 +21,7 @@ import { serveOAuthTestServer } from "@executor-js/sdk/testing"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit, settle } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); @@ -98,7 +99,7 @@ scenario( const connecting = dialog.getByRole("button", { name: "Connecting…" }); await step("Open the integration and start a new connection", async () => { - await page.goto(`/integrations/${integration}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${integration}`); await addConnection.click(); // The registered app is auto-selected, so the OAuth connect button is // present and enabled. @@ -133,7 +134,7 @@ scenario( async () => { await addConnection.click(); await dialog.waitFor({ state: "visible", timeout: 15_000 }); - await page.waitForLoadState("networkidle"); + await settle(page); // The guarantee: the reopened modal is reset. Before the fix it stays // wedged on "Connecting…" (the abandoned flow's busy state survived the diff --git a/e2e/cloud/logout-stale-session.test.ts b/e2e/cloud/logout-stale-session.test.ts index 8093cc24d2..8701865e46 100644 --- a/e2e/cloud/logout-stale-session.test.ts +++ b/e2e/cloud/logout-stale-session.test.ts @@ -19,6 +19,7 @@ import type { Page } from "playwright"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { settle } from "../src/surfaces/browser"; /** The display-only identity cookie the SSR gate mints (non-HttpOnly). */ const HINT_COOKIE = "executor-auth-hint"; @@ -145,7 +146,7 @@ scenario( await page.waitForURL((url) => url.toString() !== before, { timeout: 15_000, }); - await page.waitForLoadState("networkidle"); + await settle(page); }); const shown = (await page.locator("body").innerText()).trim(); diff --git a/e2e/cloud/mcp-browser-approval-org-scope.test.ts b/e2e/cloud/mcp-browser-approval-org-scope.test.ts index 1c931e3bfa..7c2e158f15 100644 --- a/e2e/cloud/mcp-browser-approval-org-scope.test.ts +++ b/e2e/cloud/mcp-browser-approval-org-scope.test.ts @@ -14,6 +14,7 @@ import { composePluginApi } from "@executor-js/api/server"; import { scenario } from "../src/scenario"; import { Api, Browser, Mcp, Target } from "../src/services"; import { type McpBrowserApproval, parseBrowserApproval } from "../src/surfaces/mcp"; +import { visit } from "../src/surfaces/browser"; const coreApi = composePluginApi([] as const); @@ -168,13 +169,13 @@ scenario( mcpSession.awaitResume(approval.executionId), browser.session(identity, async ({ page, step }) => { await step("Land in the original organization", async () => { - await page.goto(`/${orgA.slug}`, { waitUntil: "networkidle" }); + await visit(page, `/${orgA.slug}`); await expectOrgShell(page, orgA); }); await step("The browser session is switched to another organization", async () => { await setWorkosSessionCookie(page, target.baseUrl, sessionB); - await page.goto(`/${orgB.slug}`, { waitUntil: "networkidle" }); + await visit(page, `/${orgB.slug}`); await expectOrgShell(page, orgB); }); @@ -182,7 +183,7 @@ scenario( const loadRequest = page.waitForRequest(approvalApiRequest(approval, "GET"), { timeout: 30_000, }); - await page.goto(approval.approvalUrl, { waitUntil: "networkidle" }); + await visit(page, approval.approvalUrl); expect( (await loadRequest).headers()["x-executor-organization"], "loading the approval page scopes the paused execution lookup to org A", diff --git a/e2e/cloud/mcp-browser-resume-page.test.ts b/e2e/cloud/mcp-browser-resume-page.test.ts index 02eb39e4cd..26be5910a4 100644 --- a/e2e/cloud/mcp-browser-resume-page.test.ts +++ b/e2e/cloud/mcp-browser-resume-page.test.ts @@ -18,6 +18,7 @@ import { scenario } from "../src/scenario"; import { Api, Browser, Mcp, Target } from "../src/services"; import { parseBrowserApproval } from "../src/surfaces/mcp"; import type { Identity } from "../src/target"; +import { visit } from "../src/surfaces/browser"; const coreApi = composePluginApi([] as const); @@ -224,7 +225,7 @@ scenario( ), browser.session(identity, async ({ page, step }) => { await step("Open the paused execution approval page", async () => { - await page.goto(pathWithSearch(approval.approvalUrl), { waitUntil: "networkidle" }); + await visit(page, pathWithSearch(approval.approvalUrl)); await page.getByText("User approval required").waitFor(); }); diff --git a/e2e/cloud/mcp-execution-limits.test.ts b/e2e/cloud/mcp-execution-limits.test.ts index f80dcaac23..4c3272267c 100644 --- a/e2e/cloud/mcp-execution-limits.test.ts +++ b/e2e/cloud/mcp-execution-limits.test.ts @@ -198,3 +198,52 @@ scenario( ); }), ); + +scenario( + "Billing · a paid org runs past the rate-limit backstop instead of being blocked", + { timeout: 180_000 }, + Effect.gen(function* () { + // The backstop is sized for free-tier abuse, and it used to apply to every + // org regardless of plan: on 2026-08-18 it blocked a paying customer + // mid-workload. This pins the exemption end to end — the same run that + // blocks a free org (the scenario above) must sail past the cap here. + yield* Billing; + const autumn = yield* Autumn; + const target = yield* Target; + const mcp = yield* Mcp; + + const identity = yield* target.newIdentity(); + const bearer = yield* mcp.mintBearer(emailOf(identity)); + const customerId = orgIdOf(bearer); + + // Enterprise, not Team: it carries no price and no card-required trial, so + // the emulator activates it inline rather than answering with a checkout + // URL. Its executions are unlimited, so the BALANCE gate can never be what + // allows or blocks here — the rate-limit backstop is isolated as the only + // guard under test. + yield* autumn.attachPlan(customerId, "enterprise"); + + const session = mcp.session(identity); + + // Deliberately past E2E_EXECUTION_RATE_LIMIT: the count that blocks a free + // org. Every one must run. + const overCap = RATE_LIMIT + 3; + for (let i = 1; i <= overCap; i++) { + const ok = yield* session.call("execute", { code: `return ${i};` }); + expect(ok.ok, `execution ${i} succeeds for a paid org (cap is ${RATE_LIMIT})`).toBe(true); + expect( + ok.text, + `execution ${i} returns its value rather than the backstop message`, + ).not.toContain(RATE_LIMIT_BLOCKED_MESSAGE); + } + + // Nothing was withheld from the meter either: a paid org past the cap is + // billed for every execution it ran. + const metered = yield* autumn.expectUsage({ + customerId, + featureId: "executions", + count: overCap, + }); + expect(metered.length, "every execution past the cap is still metered").toBe(overCap); + }), +); diff --git a/e2e/cloud/member-invite-seat-limit.test.ts b/e2e/cloud/member-invite-seat-limit.test.ts index 470bb9a019..e465c1396e 100644 --- a/e2e/cloud/member-invite-seat-limit.test.ts +++ b/e2e/cloud/member-invite-seat-limit.test.ts @@ -23,6 +23,7 @@ import { AccountHttpApi } from "@executor-js/api"; import { scenario } from "../src/scenario"; import { Api, Billing, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; // apps/cloud/src/extensions/billing/plans.ts → MEMBER_LIMITS.free const FREE_MEMBER_SEATS = 3; @@ -45,7 +46,7 @@ scenario( let slug = ""; await step("Land in the app and canonicalize onto the org slug", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page.waitForURL((url) => /^\/[a-z0-9-]+\/?$/.test(url.pathname), { timeout: 30_000, }); @@ -54,7 +55,7 @@ scenario( }); await step("Open the organization members page", async () => { - await page.goto(`/${slug}/org`, { waitUntil: "networkidle" }); + await visit(page, `/${slug}/org`); await page.getByRole("button", { name: "Invite member" }).waitFor(); }); diff --git a/e2e/cloud/oauth-callback-org-scope.test.ts b/e2e/cloud/oauth-callback-org-scope.test.ts index afe0214743..66f1f50415 100644 --- a/e2e/cloud/oauth-callback-org-scope.test.ts +++ b/e2e/cloud/oauth-callback-org-scope.test.ts @@ -17,6 +17,7 @@ import { serveOAuthTestServer } from "@executor-js/sdk/testing"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; import type { Identity } from "../src/target"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); @@ -212,13 +213,13 @@ scenario( await installSameWindowOAuthPopup(page); await step("Land in the original organization", async () => { - await page.goto(`/${orgA.slug}`, { waitUntil: "networkidle" }); + await visit(page, `/${orgA.slug}`); await expectOrgShell(page, orgA); }); await step("The browser session is switched to another organization", async () => { await setWorkosSessionCookie(page, target.baseUrl, sessionB); - await page.goto(`/${orgB.slug}`, { waitUntil: "networkidle" }); + await visit(page, `/${orgB.slug}`); await expectOrgShell(page, orgB); }); @@ -232,9 +233,7 @@ scenario( // refetch, and leave "Connect with OAuth" disabled forever (the flake // this step used to have). Waiting for org A's shell before opening // the modal makes every modal fetch run under the settled org A scope. - await page.goto(`/${orgA.slug}/integrations/${String(integration)}`, { - waitUntil: "networkidle", - }); + await visit(page, `/${orgA.slug}/integrations/${String(integration)}`); await page.getByRole("button", { name: new RegExp(escapeRegExp(orgA.name)) }).waitFor({ timeout: 30_000, }); @@ -286,7 +285,7 @@ scenario( await step("The provider returns to the OAuth callback", async () => { const callbackUrl = await submitProviderLoginFromPage(page); callback = new URL(callbackUrl); - const response = await page.goto(callbackUrl, { waitUntil: "networkidle" }); + const response = await visit(page, callbackUrl); expect(response?.status(), "the callback renders its popup result page").toBe(200); }); diff --git a/e2e/cloud/onboarding-mcp-url.test.ts b/e2e/cloud/onboarding-mcp-url.test.ts index b1998668f2..84de5e0a70 100644 --- a/e2e/cloud/onboarding-mcp-url.test.ts +++ b/e2e/cloud/onboarding-mcp-url.test.ts @@ -6,6 +6,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; scenario( "Onboarding · the MCP setup step hands the user their org-scoped MCP server URL", @@ -19,7 +20,7 @@ scenario( await step( "A fresh user without an org lands on the create-org onboarding page", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); // Step 1 of 2 — the org-name input is the landmark that proves we're on onboarding. await page.getByPlaceholder("Northwind Labs").waitFor(); }, diff --git a/e2e/cloud/org-api-keys-console.test.ts b/e2e/cloud/org-api-keys-console.test.ts index fae9f61e63..828dcb9afb 100644 --- a/e2e/cloud/org-api-keys-console.test.ts +++ b/e2e/cloud/org-api-keys-console.test.ts @@ -20,6 +20,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; import { forBrowser, joinOrg } from "./support/session"; +import { visit } from "../src/surfaces/browser"; declare global { interface Window { @@ -45,7 +46,7 @@ scenario( let slug = ""; await step("Land in the workspace and open the API keys page", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page.waitForURL((url) => /^\/[a-z0-9-]+\/?$/.test(url.pathname), { timeout: 30_000, }); @@ -120,12 +121,12 @@ scenario( // ── The plain member's view ─────────────────────────────────────────── yield* browser.session(forBrowser(member), async ({ page, step }) => { await step("A plain member is not shown the Organization keys section", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page.waitForURL((url) => /^\/[a-z0-9-]+\/?$/.test(url.pathname), { timeout: 30_000, }); const slug = new URL(page.url()).pathname.split("/").filter(Boolean)[0] ?? ""; - await page.goto(`/${slug}/api-keys`, { waitUntil: "networkidle" }); + await visit(page, `/${slug}/api-keys`); // The personal half renders for everyone… await page .getByRole("heading", { name: "Personal keys" }) diff --git a/e2e/cloud/org-delete.test.ts b/e2e/cloud/org-delete.test.ts index f18a6fc8c2..bb3f9f187c 100644 --- a/e2e/cloud/org-delete.test.ts +++ b/e2e/cloud/org-delete.test.ts @@ -9,6 +9,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { visit, settle } from "../src/surfaces/browser"; scenario( "Organizations · an admin deletes the organization from settings", @@ -22,7 +23,7 @@ scenario( const ORG = "Doomed Org"; await step("Fresh user creates an org via onboarding", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page.getByPlaceholder("Northwind Labs").fill(ORG); await page.getByRole("button", { name: "Create organization" }).click(); await page.getByText("Connect your MCP client").waitFor(); @@ -32,13 +33,13 @@ scenario( await page.waitForURL((url) => /^\/[a-z0-9-]+\/?$/.test(url.pathname), { timeout: 30_000, }); - await page.waitForLoadState("networkidle"); + await settle(page); }); const slug = new URL(page.url()).pathname.split("/").filter(Boolean)[0]!; await step("Open Organization settings and find the danger zone", async () => { - await page.goto(`/${slug}/org`, { waitUntil: "networkidle" }); + await visit(page, `/${slug}/org`); // The admin-only danger zone renders (a member would not see it). await page.getByText("Permanently delete this organization").waitFor(); }); diff --git a/e2e/cloud/org-last-visited.test.ts b/e2e/cloud/org-last-visited.test.ts index 65a351ff22..6912fce1cc 100644 --- a/e2e/cloud/org-last-visited.test.ts +++ b/e2e/cloud/org-last-visited.test.ts @@ -13,6 +13,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const CLOUD_ORIGIN_HEADERS = (baseUrl: string) => ({ origin: new URL(baseUrl).origin }); @@ -64,7 +65,7 @@ scenario( yield* browser.session(inB, async ({ page, step }) => { await step("Work in org A by its slug URL (the session still pins org B)", async () => { - await page.goto(`/${slugA}`, { waitUntil: "networkidle" }); + await visit(page, `/${slugA}`); await page.getByText("Integrations").first().waitFor({ timeout: 30_000 }); // The client records the viewed org once /account/me confirms it. await page.waitForFunction( @@ -75,7 +76,7 @@ scenario( }); await step("A bare entry (`/`) returns to org A, not the session's org B", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page.waitForURL( (url) => url.pathname === `/${slugA}` || url.pathname === `/${slugA}/`, { @@ -86,7 +87,7 @@ scenario( }); await step("A bare deep link keeps its path while landing in org A", async () => { - await page.goto("/policies", { waitUntil: "networkidle" }); + await visit(page, "/policies"); await page.waitForURL((url) => url.pathname === `/${slugA}/policies`, { timeout: 30_000 }); await page.getByText("Policies").first().waitFor({ timeout: 30_000 }); }); diff --git a/e2e/cloud/org-limit.test.ts b/e2e/cloud/org-limit.test.ts index 3bd8b2b256..dcdb570936 100644 --- a/e2e/cloud/org-limit.test.ts +++ b/e2e/cloud/org-limit.test.ts @@ -8,6 +8,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Billing, Browser, Target } from "../src/services"; +import { visit, settle } from "../src/surfaces/browser"; const FREE_LIMIT = 3; @@ -23,7 +24,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("A fresh user lands on onboarding (no organization yet)", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page.getByPlaceholder("Northwind Labs").waitFor(); }); @@ -43,7 +44,7 @@ scenario( await page.waitForURL((url) => /^\/[a-z0-9-]+\/?$/.test(url.pathname), { timeout: 30_000, }); - await page.waitForLoadState("networkidle"); + await settle(page); }); const openCreateOrgModal = async (currentOrg: string) => { diff --git a/e2e/cloud/org-multitab-cookie.test.ts b/e2e/cloud/org-multitab-cookie.test.ts index 27c8a967a5..75125828ba 100644 --- a/e2e/cloud/org-multitab-cookie.test.ts +++ b/e2e/cloud/org-multitab-cookie.test.ts @@ -21,6 +21,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; scenario( "Org tabs · two tabs on different orgs stay independent (URL-scoped, no cookie steal)", @@ -53,7 +54,7 @@ scenario( let slugB = ""; await step("Onboard org A in tab 1", async () => { - await tab1.goto("/", { waitUntil: "networkidle" }); + await visit(tab1, "/"); await tab1.getByPlaceholder("Northwind Labs").fill("Multitab A"); await tab1.getByRole("button", { name: "Create organization" }).click(); await tab1.getByText("Connect your MCP client").waitFor({ timeout: 30_000 }); @@ -83,7 +84,7 @@ scenario( const tab2 = await tab1.context().newPage(); await step("Tab 2 opens org A's URL and stays in A — no switch, no reload loop", async () => { - await tab2.goto(`/${slugA}/policies`, { waitUntil: "networkidle" }); + await visit(tab2, `/${slugA}/policies`); await tab2.getByText("Policies").first().waitFor({ timeout: 30_000 }); expect(new URL(tab2.url()).pathname, "tab 2 stays on org A's URL").toBe( `/${slugA}/policies`, @@ -134,7 +135,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the OpenAPI add form", async () => { - await page.goto("/integrations/add/openapi", { waitUntil: "networkidle" }); + await visit(page, "/integrations/add/openapi"); await page.getByPlaceholder("https://api.example.com/openapi.json").waitFor(); }); diff --git a/e2e/cloud/org-slug-foreign.test.ts b/e2e/cloud/org-slug-foreign.test.ts index 04aee48d35..c50e024d2b 100644 --- a/e2e/cloud/org-slug-foreign.test.ts +++ b/e2e/cloud/org-slug-foreign.test.ts @@ -13,6 +13,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const CLOUD_ORIGIN_HEADERS = (baseUrl: string) => ({ origin: new URL(baseUrl).origin }); @@ -67,10 +68,10 @@ scenario( yield* browser.session(inB, async ({ page, step }) => { await step("Land in org B, then open org A's slug URL directly", async () => { - await page.goto(`/${orgB.slug}`, { waitUntil: "networkidle" }); + await visit(page, `/${orgB.slug}`); await page.getByText("Integrations").first().waitFor({ timeout: 30_000 }); // Open org A by its slug while the cookie is still pinned to B. - await page.goto(`/${slugA}/policies`, { waitUntil: "networkidle" }); + await visit(page, `/${slugA}/policies`); }); await step("Org A's page renders at its slugged URL — the URL is the scope", async () => { diff --git a/e2e/cloud/org-switcher.test.ts b/e2e/cloud/org-switcher.test.ts index eaf1aa9980..62f77bd4ed 100644 --- a/e2e/cloud/org-switcher.test.ts +++ b/e2e/cloud/org-switcher.test.ts @@ -8,6 +8,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { visit, settle } from "../src/surfaces/browser"; scenario( "Organizations · switching organizations switches the workspace", @@ -20,7 +21,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { // ── Step 1: onboarding, create the first org ───────────────────── await step("Fresh user lands on onboarding (no organization yet)", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page.getByPlaceholder("Northwind Labs").waitFor(); }); @@ -43,7 +44,7 @@ scenario( await page.waitForURL((url) => /^\/[a-z0-9-]+\/?$/.test(url.pathname), { timeout: 30_000, }); - await page.waitForLoadState("networkidle"); + await settle(page); }); // ── Step 2: create the second org via the account-menu switcher ── @@ -89,7 +90,7 @@ scenario( // rendered with data-disabled="" (Radix convention). The only item without // data-disabled that isn't "Create organization" is ORG_1. await step(`Open the org switcher and switch back to "${ORG_1}"`, async () => { - await page.waitForLoadState("networkidle"); + await settle(page); await page.getByRole("button", { name: /Test User/ }).click(); // Click the SubTrigger (shows current org name = ORG_2) to expand the list. await page.getByRole("menuitem", { name: ORG_2 }).click(); diff --git a/e2e/scenarios/artifact-loading-surface.test.ts b/e2e/scenarios/artifact-loading-surface.test.ts index 993228eb13..141d61dda1 100644 --- a/e2e/scenarios/artifact-loading-surface.test.ts +++ b/e2e/scenarios/artifact-loading-surface.test.ts @@ -43,6 +43,7 @@ import type { ArtifactId } from "@executor-js/sdk/shared"; import { scenario } from "../src/scenario"; import { Api, Browser, Mcp, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([] as const); @@ -230,7 +231,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the Artifacts gallery", async () => { await startSampling(page); - await page.goto(`${target.baseUrl}/artifacts`, { waitUntil: "networkidle" }); + await visit(page, `${target.baseUrl}/artifacts`); await page .getByRole("link", { name: `Open artifact ${title}` }) .waitFor({ timeout: 30_000 }); diff --git a/e2e/scenarios/artifact-preview-gallery.test.ts b/e2e/scenarios/artifact-preview-gallery.test.ts index f3e08643e5..a468f5897b 100644 --- a/e2e/scenarios/artifact-preview-gallery.test.ts +++ b/e2e/scenarios/artifact-preview-gallery.test.ts @@ -17,6 +17,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Mcp, Target } from "../src/services"; +import { revisit, visit } from "../src/surfaces/browser"; const uniqueSuffix = () => randomBytes(3).toString("hex"); @@ -167,7 +168,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the Artifacts gallery", async () => { - await page.goto(`${target.baseUrl}/artifacts`, { waitUntil: "networkidle" }); + await visit(page, `${target.baseUrl}/artifacts`); await page.getByRole("heading", { name: "Artifacts", level: 1 }).waitFor(); // Every seeded artifact is present before anything is measured. for (const [name] of seeded) { @@ -257,7 +258,7 @@ scenario( // The stored markup is token-based, so the SAME markup has to read // correctly against the console's dark surface without being restyled. await page.emulateMedia({ colorScheme: "dark" }); - await page.reload({ waitUntil: "networkidle" }); + await revisit(page); await page.getByRole("heading", { name: "Artifacts", level: 1 }).waitFor(); await page.locator('[data-slot="artifact-preview"]').first().waitFor({ timeout: 20_000 }); @@ -272,7 +273,7 @@ scenario( await step("Gallery in light mode", async () => { await page.emulateMedia({ colorScheme: "light" }); - await page.reload({ waitUntil: "networkidle" }); + await revisit(page); await page.getByRole("heading", { name: "Artifacts", level: 1 }).waitFor(); await page.locator('[data-slot="artifact-preview"]').first().waitFor({ timeout: 20_000 }); }); diff --git a/e2e/scenarios/artifacts.test.ts b/e2e/scenarios/artifacts.test.ts index 29f4be3e90..fa9a82d03b 100644 --- a/e2e/scenarios/artifacts.test.ts +++ b/e2e/scenarios/artifacts.test.ts @@ -24,6 +24,7 @@ import type { ArtifactId } from "@executor-js/sdk/shared"; import { scenario } from "../src/scenario"; import { Api, Browser, Mcp, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([] as const); @@ -263,7 +264,7 @@ scenario( await step("Open the artifact link the agent handed over", async () => { await recordHandshakeOrdering(page); - await page.goto(url, { waitUntil: "networkidle" }); + await visit(page, url); consoleStyleBefore = await readConsoleStyle(page); }); @@ -596,7 +597,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the Artifacts tab", async () => { - await page.goto("/artifacts", { waitUntil: "networkidle" }); + await visit(page, "/artifacts"); await page.getByRole("link", { name: `Open artifact ${originalTitle}` }).waitFor({ timeout: 20_000, }); @@ -634,7 +635,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Delete the artifact from the list", async () => { - await page.goto("/artifacts", { waitUntil: "networkidle" }); + await visit(page, "/artifacts"); const card = page.locator('[data-slot="artifact-card"]').filter({ hasText: renamedTitle, }); @@ -738,7 +739,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("The artifact's own URL shows the updated component", async () => { - await page.goto(`/artifacts/${artifactId}`, { waitUntil: "networkidle" }); + await visit(page, `/artifacts/${artifactId}`); const content = artifactContent(page); await content .locator('[data-testid="artifact-marker"]') diff --git a/e2e/scenarios/auth-methods-ui.test.ts b/e2e/scenarios/auth-methods-ui.test.ts index 058fbd77f1..224dd11f27 100644 --- a/e2e/scenarios/auth-methods-ui.test.ts +++ b/e2e/scenarios/auth-methods-ui.test.ts @@ -15,6 +15,7 @@ import { makeGreetingMcpServer, serveMcpServer } from "@executor-js/plugin-mcp/t import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; scenario( "Auth methods · the add flow declares an API key alongside the detected method", @@ -34,9 +35,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the add-MCP flow pointed at the server", async () => { - await page.goto(`/integrations/add/mcp?url=${encodeURIComponent(server.endpoint)}`, { - waitUntil: "networkidle", - }); + await visit(page, `/integrations/add/mcp?url=${encodeURIComponent(server.endpoint)}`); // The URL auto-probes (debounced); the method list appears once // the probe lands. Generous timeout: the probe request can queue // behind a busy dev server under CI load, and there is no clean diff --git a/e2e/scenarios/browser-approval.test.ts b/e2e/scenarios/browser-approval.test.ts index 3736316650..63d0d020ac 100644 --- a/e2e/scenarios/browser-approval.test.ts +++ b/e2e/scenarios/browser-approval.test.ts @@ -25,6 +25,7 @@ import { Api, Browser, Mcp, Target } from "../src/services"; import { type McpBrowserApproval, parseBrowserApproval } from "../src/surfaces/mcp"; import type { BrowserSurface } from "../src/surfaces/browser"; import type { Identity } from "../src/target"; +import { visit } from "../src/surfaces/browser"; const coreApi = composePluginApi([] as const); @@ -70,7 +71,7 @@ const decideInBrowser = ( await step( `Open the approval page and ${decision.toLowerCase()} the paused action`, async () => { - await page.goto(approval.approvalUrl, { waitUntil: "networkidle" }); + await visit(page, approval.approvalUrl); await page.getByRole("button", { name: decision }).click(); // The page confirms the decision was recorded ("Approve sent" / "Decline sent"). await page.getByText(`${decision} sent`).waitFor(); diff --git a/e2e/scenarios/connect-deep-link.test.ts b/e2e/scenarios/connect-deep-link.test.ts index 4961392198..6e296a8f9b 100644 --- a/e2e/scenarios/connect-deep-link.test.ts +++ b/e2e/scenarios/connect-deep-link.test.ts @@ -30,6 +30,7 @@ import { import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); @@ -95,7 +96,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the connect deep link for the seeded integration", async () => { - await page.goto(`/connect/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/connect/${slug}`); // It forwards into the integration's OWN detail route rather than // rendering a parallel connect UI, carrying the add-account handoff. await page.waitForURL((url) => url.pathname === detailPath, { timeout: 30_000 }); @@ -115,7 +116,7 @@ scenario( }); await step("An unknown slug is a clear not-found, not a blank page", async () => { - await page.goto("/connect/zz-no-such-integration", { waitUntil: "networkidle" }); + await visit(page, "/connect/zz-no-such-integration"); await page .getByText("Unknown integration: zz-no-such-integration") .waitFor({ timeout: 30_000 }); @@ -139,7 +140,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("The deep link surfaces the existing connection", async () => { - await page.goto(`/connect/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/connect/${slug}`); await page.waitForURL((url) => url.pathname === detailPath, { timeout: 30_000 }); // The connect flow still opens (this link means "connect me"), and // the already-saved account is visible behind it. diff --git a/e2e/scenarios/connect-handoff-session.test.ts b/e2e/scenarios/connect-handoff-session.test.ts index e91feeca37..055baf44ed 100644 --- a/e2e/scenarios/connect-handoff-session.test.ts +++ b/e2e/scenarios/connect-handoff-session.test.ts @@ -20,6 +20,7 @@ import { scenario } from "../src/scenario"; import { Browser, Cli, Mcp, RunDir, Target } from "../src/services"; import { withChatTheater } from "../src/clients/chat-theater"; import type { McpSession } from "../src/surfaces/mcp"; +import { visit } from "../src/surfaces/browser"; const unique = (prefix: string) => `${prefix}_${randomBytes(4).toString("hex")}`; @@ -148,7 +149,7 @@ scenario( yield* chat.status("you, in the browser: opening the link and pasting the API key…"); yield* browser.session(identity, async ({ page, step }) => { await step("Open the connect link from the chat", async () => { - await page.goto(handoffUrl, { waitUntil: "networkidle" }); + await visit(page, handoffUrl); await page .getByRole("heading", { name: /Add connection/ }) .waitFor({ timeout: 15_000 }); diff --git a/e2e/scenarios/connect-handoff.test.ts b/e2e/scenarios/connect-handoff.test.ts index d4cc80692e..e9f7696cd4 100644 --- a/e2e/scenarios/connect-handoff.test.ts +++ b/e2e/scenarios/connect-handoff.test.ts @@ -32,6 +32,7 @@ import { Api, Browser, Mcp, Target } from "../src/services"; import type { Identity, Target as TargetShape } from "../src/target"; import type { BrowserSurface } from "../src/surfaces/browser"; import type { McpSession } from "../src/surfaces/mcp"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); @@ -214,7 +215,7 @@ const runScenario = (input: { // 4. The user opens the handoff URL and pastes the credential. yield* browser.session(identity, async ({ page, step }) => { await step("Open the handoff URL from the agent", async () => { - await page.goto(handoffUrl, { waitUntil: "networkidle" }); + await visit(page, handoffUrl); }); await step("The Add connection modal is open", async () => { diff --git a/e2e/scenarios/connection-remove-confirm.test.ts b/e2e/scenarios/connection-remove-confirm.test.ts index 52877548f1..b0c8ab8146 100644 --- a/e2e/scenarios/connection-remove-confirm.test.ts +++ b/e2e/scenarios/connection-remove-confirm.test.ts @@ -14,6 +14,7 @@ import { AuthTemplateSlug, ConnectionName, IntegrationSlug } from "@executor-js/ import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); @@ -77,7 +78,7 @@ scenario( const confirm = page.getByRole("alertdialog"); await step("Open the integration's connections", async () => { - await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}`); await row.waitFor(); }); diff --git a/e2e/scenarios/docs-link.test.ts b/e2e/scenarios/docs-link.test.ts index 5017f23d39..8445f421b5 100644 --- a/e2e/scenarios/docs-link.test.ts +++ b/e2e/scenarios/docs-link.test.ts @@ -3,6 +3,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; // A user inside the console should be able to reach the documentation without // leaving the app for the marketing site (the original friction: docs were @@ -18,7 +19,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the console", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page.getByText("Integrations").first().waitFor(); }); diff --git a/e2e/scenarios/first-party-oauth.test.ts b/e2e/scenarios/first-party-oauth.test.ts index f0473784ab..f162163a1a 100644 --- a/e2e/scenarios/first-party-oauth.test.ts +++ b/e2e/scenarios/first-party-oauth.test.ts @@ -26,7 +26,7 @@ import { import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; -import { clickToReveal } from "../src/surfaces/browser"; +import { clickToReveal, visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); @@ -182,7 +182,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the Google integration catalog", async () => { - await page.goto("/integrations", { waitUntil: "networkidle" }); + await visit(page, "/integrations"); const dialog = page.getByRole("dialog", { name: "Connect an integration" }); await clickToReveal(page.getByRole("button", { name: /Connect/ }).first(), dialog); }); diff --git a/e2e/scenarios/google-health-checks.test.ts b/e2e/scenarios/google-health-checks.test.ts index 6dc532ac5b..8cdb602a1e 100644 --- a/e2e/scenarios/google-health-checks.test.ts +++ b/e2e/scenarios/google-health-checks.test.ts @@ -13,10 +13,11 @@ import { OAuthClientSlug, } from "@executor-js/sdk/shared"; +import { createEmulatorInstance } from "../src/emulator-instance"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; import type { Identity, Target as TargetShape } from "../src/target"; -import { clickToReveal, type BrowserSurface } from "../src/surfaces/browser"; +import { type BrowserSurface, clickToReveal, visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); type Client = HttpApiClient.ForApi; @@ -69,14 +70,13 @@ const completeGoogleConsent = (authorizationUrl: string) => return code; }); -const createGoogleEmulator = Effect.promise(async () => { - const response = await fetch("https://google.emulators.dev/_emulate/instances", { - method: "POST", - }); - if (!response.ok) throw new Error(`Google emulator instance failed: ${response.status}`); - const instance = (await response.json()) as { readonly providerBaseUrl: string }; - const client = await connectEmulator({ baseUrl: instance.providerBaseUrl }); - return { client, baseUrl: instance.providerBaseUrl }; +const createGoogleEmulator = Effect.gen(function* () { + // Through the shared helper: it bounds and retries the one request in this + // scenario that leaves the runner, so a blip on the way to the edge doesn't + // read as a Google health-check failure. + const baseUrl = yield* createEmulatorInstance("google", "google-health"); + const client = yield* Effect.promise(() => connectEmulator({ baseUrl })); + return { client, baseUrl }; }); const addGooglePresetFromCatalog = ( @@ -87,7 +87,7 @@ const addGooglePresetFromCatalog = ( ) => browser.session(identity, async ({ page, step }) => { await step(`Open ${presetName} from the connect catalog`, async () => { - await page.goto("/integrations", { waitUntil: "networkidle" }); + await visit(page, "/integrations"); const dialog = page.getByRole("dialog", { name: "Connect an integration" }); await clickToReveal(page.getByRole("button", { name: /Connect/ }).first(), dialog); await dialog.getByPlaceholder(/Search or paste a URL/).fill(presetName); @@ -266,29 +266,31 @@ scenario( health.status, `${row.presetName} health check is healthy: ${JSON.stringify(health)}`, ).toBe("healthy"); - } - // The hosted emulator acknowledges a request before its ledger entry - // is readable, so a single list right after the probe races the write - // (the health checks above already came back healthy, which only the - // emulator can answer). Poll until every expected operation is - // visible; on timeout the assertion names what never arrived. - const missing = yield* Effect.gen(function* () { - const ledger = yield* Effect.promise(() => emulator.client.ledger.list(100)); - return rows.filter( - (row) => !ledger.some((entry) => entry.operationId === row.expectedLedgerOperation), + // Check this row's ledger entry HERE, not after both rows have run. + // `ledger.list(n)` is the last n entries, and connecting the second + // account is easily a hundred emulator requests, so the first row's + // health check can be evicted from the window before a combined + // assertion at the end ever looks for it — which reads as "Calendar's + // health check never reached the emulator" when it plainly did (the + // probe above came back healthy, and only the emulator can answer + // that). The hosted emulator also acknowledges a request before its + // ledger entry is readable, so poll rather than read once. + const reached = yield* Effect.promise(() => emulator.client.ledger.list(50)).pipe( + Effect.map((ledger) => + ledger.some((entry) => entry.operationId === row.expectedLedgerOperation), + ), + Effect.repeat({ + schedule: Schedule.spaced("500 millis"), + until: (seen) => seen, + times: 19, + }), ); - }).pipe( - Effect.repeat({ - schedule: Schedule.spaced("500 millis"), - until: (unseen) => unseen.length === 0, - times: 19, - }), - ); - expect( - missing.map((row) => row.presetName), - "every health check reached the Google emulator", - ).toEqual([]); + expect( + reached, + `${row.presetName}'s health check reached the Google emulator as ${row.expectedLedgerOperation}`, + ).toBe(true); + } }), Effect.gen(function* () { for (const row of rows) { diff --git a/e2e/scenarios/google-photos-preset-ui.test.ts b/e2e/scenarios/google-photos-preset-ui.test.ts index 8b5788493b..463f4e8bd3 100644 --- a/e2e/scenarios/google-photos-preset-ui.test.ts +++ b/e2e/scenarios/google-photos-preset-ui.test.ts @@ -3,7 +3,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; -import { clickToReveal } from "../src/surfaces/browser"; +import { clickToReveal, visit } from "../src/surfaces/browser"; scenario( "Google Photos: separated catalog presets open a Photos service add flow", @@ -17,7 +17,7 @@ scenario( await step( "Find the separated Google Photos presets from the integrations picker", async () => { - await page.goto("/integrations", { waitUntil: "networkidle" }); + await visit(page, "/integrations"); const dialog = page.getByRole("dialog", { name: "Connect an integration" }); await clickToReveal(page.getByRole("button", { name: "Connect" }), dialog); await dialog.getByPlaceholder(/Search or paste a URL/).fill("google photos"); diff --git a/e2e/scenarios/graphql-introspection-health.test.ts b/e2e/scenarios/graphql-introspection-health.test.ts index 185ec36653..b0ca487fb9 100644 --- a/e2e/scenarios/graphql-introspection-health.test.ts +++ b/e2e/scenarios/graphql-introspection-health.test.ts @@ -11,6 +11,7 @@ import { variable } from "@executor-js/sdk/http-auth"; import { createEmulatorInstance } from "../src/emulator-instance"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([graphqlHttpPlugin()] as const); const unique = (prefix: string): string => `${prefix}_${randomBytes(4).toString("hex")}`; @@ -30,11 +31,18 @@ scenario( connectEmulator({ baseUrl: emulatorBaseUrl, service: "github" }), ); + // A budget, not a count: nothing in this scenario depends on how many + // times the connect flow introspects, and the emulator's answer when the + // budget runs out is not a neutral pass-through — an unauthenticated + // GraphQL POST to the real handler is GitHub-shaped, so it comes back 403 + // "API rate limit exceeded". The UI then honestly reports HTTP 403 and the + // assertion below fails on a message that has nothing to do with the + // product. Arm enough that one connect attempt cannot exhaust it. yield* Effect.promise(() => emulator.faults.arm({ match: { method: "POST", pathPattern: "/graphql" }, response: { status: 401, body: { message: "Bad credentials" } }, - times: 10, + times: 100, }), ); @@ -56,9 +64,7 @@ scenario( yield* Effect.gen(function* () { yield* browser.session(identity, async ({ page, step }) => { await step("Open the connection flow", async () => { - await page.goto(`/integrations/${slug}?addAccount=1&owner=org&template=header`, { - waitUntil: "networkidle", - }); + await visit(page, `/integrations/${slug}?addAccount=1&owner=org&template=header`); await page.getByRole("heading", { name: /Add connection · GraphQL health/ }).waitFor(); }); @@ -97,7 +103,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("A failed existing connection explains the empty tool catalogue", async () => { - await page.goto(`/integrations/${slug}?tab=tools`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}?tab=tools`); await page.getByText("Connection rejected", { exact: true }).first().waitFor(); await page .getByText("The endpoint rejected the credential with HTTP 401.", { diff --git a/e2e/scenarios/health-checks-ui.test.ts b/e2e/scenarios/health-checks-ui.test.ts index 10ae01dcaf..ce74cd9d6b 100644 --- a/e2e/scenarios/health-checks-ui.test.ts +++ b/e2e/scenarios/health-checks-ui.test.ts @@ -34,6 +34,7 @@ import { AuthTemplateSlug, ConnectionName, IntegrationSlug } from "@executor-js/ import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); type Client = HttpApiClient.ForApi; @@ -281,7 +282,7 @@ scenario( const dialog = page.getByRole("dialog"); await step("Open the Add Connection modal", async () => { - await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}`); await page.getByRole("button", { name: "Add connection", exact: true }).click(); await page.getByRole("heading", { name: /Add connection/ }).waitFor(); }); @@ -395,7 +396,7 @@ scenario( const options = page.getByRole("option"); await step("Open the health-check editor over the large spec", async () => { - await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}`); await page.getByRole("heading", { level: 3, name: "Health check" }).waitFor(); await page.getByRole("button", { name: "Set up" }).click(); await input.waitFor(); @@ -479,7 +480,7 @@ scenario( const options = page.getByRole("option"); await step("Open the Add form and paste the large spec", async () => { - await page.goto("/integrations/add/openapi", { waitUntil: "networkidle" }); + await visit(page, "/integrations/add/openapi"); await page.getByPlaceholder("https://api.example.com/openapi.json").fill(spec); await page .getByRole("heading", { name: "Health check (optional)" }) @@ -558,7 +559,7 @@ scenario( const operationInput = page.locator("#health-check-operation"); await step("Open the health-check editor", async () => { - await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}`); await page.getByRole("heading", { level: 3, name: "Health check" }).waitFor(); await page.getByRole("button", { name: "Set up" }).click(); await operationInput.waitFor(); @@ -629,7 +630,7 @@ scenario( const list = page.locator("[data-slot='combobox-list']").first(); await step("Open the operation combobox in the sheet", async () => { - await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}`); await page.getByRole("heading", { level: 3, name: "Health check" }).waitFor(); await page.getByRole("button", { name: "Set up" }).click(); await operationInput.waitFor(); @@ -705,7 +706,7 @@ scenario( const menuTrigger = connections.locator('button[aria-haspopup="menu"]'); await step("Open the integration's connections", async () => { - await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}`); await connections.getByText("main", { exact: true }).waitFor(); await page.getByRole("heading", { level: 3, name: "Health check" }).waitFor(); }); @@ -795,7 +796,7 @@ scenario( const dialog = page.getByRole("dialog"); await step("Open the Add Connection modal", async () => { - await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}`); await page.getByRole("button", { name: "Add connection", exact: true }).click(); await page.getByRole("heading", { name: /Add connection/ }).waitFor(); }); @@ -888,7 +889,7 @@ scenario( await step( "A fresh page load shows the expired connection with NO clicking", async () => { - await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}`); // The persisted verdict drives the row: red dot + Expired badge // are already there on first paint of the list. await connections.getByLabel("Status: Expired").waitFor({ timeout: 30_000 }); @@ -907,7 +908,7 @@ scenario( // not after the freshness window). Restore the key, reload, // and the dot flips back with no clicks. server.restore(); - await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}`); // The row mounts with the stale expired verdict, then the // background revalidation flips it to healthy in place. await connections.getByLabel("Status: Healthy").waitFor({ timeout: 30_000 }); @@ -993,7 +994,7 @@ scenario( // while the operation combobox still renders (a healthy probe // saves the check and turns the request line static). await page.setViewportSize({ width: 1280, height: 420 }); - await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}`); await page.getByRole("button", { name: "Add connection", exact: true }).click(); await page.getByRole("heading", { name: /Add connection/ }).waitFor(); await page.keyboard.type(goodToken); @@ -1136,7 +1137,7 @@ scenario( await step( "Load the integrations list: the dead MCP row reads Expired with no clicks", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); // The row itself is the assertion surface: the list-page // summary probes in the background and paints the worst-of // verdict onto the row, scoped so a verdict from another row diff --git a/e2e/scenarios/mcp-catalog-sync-ui.test.ts b/e2e/scenarios/mcp-catalog-sync-ui.test.ts index 780ed83ebe..bda26e2cd7 100644 --- a/e2e/scenarios/mcp-catalog-sync-ui.test.ts +++ b/e2e/scenarios/mcp-catalog-sync-ui.test.ts @@ -14,6 +14,7 @@ import { makeMutableCatalogMcpServer, serveMcpServer } from "@executor-js/plugin import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { revisit, visit } from "../src/surfaces/browser"; scenario( "MCP catalog · the Tools tab follows a server-side rename after a list_changed notification", @@ -35,9 +36,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the add-MCP flow pointed at the live server", async () => { - await page.goto(`/integrations/add/mcp?url=${encodeURIComponent(server.endpoint)}`, { - waitUntil: "networkidle", - }); + await visit(page, `/integrations/add/mcp?url=${encodeURIComponent(server.endpoint)}`); // The URL auto-probes (debounced); the method list appears once the // probe lands — an open server seeds the detected no-auth method. await page.getByText("How does this server authenticate?").waitFor(); @@ -95,7 +94,7 @@ scenario( // Re-enter the page: a fresh tools read. The list_changed the server // sent during the call marked the catalog stale, so THIS read // re-lists — the renamed tool appears with no Refresh click. - await page.reload({ waitUntil: "networkidle" }); + await revisit(page); await page.getByRole("tab", { name: "Tools" }).click(); await filterTools("greet"); await toolRow(mutable.renamedToolName).waitFor({ timeout: 30_000 }); diff --git a/e2e/scenarios/oauth-callback-url.test.ts b/e2e/scenarios/oauth-callback-url.test.ts index f2012b7e31..ff78f6fcff 100644 --- a/e2e/scenarios/oauth-callback-url.test.ts +++ b/e2e/scenarios/oauth-callback-url.test.ts @@ -31,6 +31,7 @@ import { serveOAuthTestServer } from "@executor-js/sdk/testing"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); @@ -153,9 +154,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the connect modal for an OAuth integration", async () => { - await page.goto(`/integrations/${String(integration)}?addAccount=1`, { - waitUntil: "networkidle", - }); + await visit(page, `/integrations/${String(integration)}?addAccount=1`); await page.getByRole("button", { name: "Register app", exact: true }).click(); }); diff --git a/e2e/scenarios/oauth-client-handoff.test.ts b/e2e/scenarios/oauth-client-handoff.test.ts index 258ce3276c..a9575c2e7f 100644 --- a/e2e/scenarios/oauth-client-handoff.test.ts +++ b/e2e/scenarios/oauth-client-handoff.test.ts @@ -37,6 +37,7 @@ import { createEmulatorInstance } from "../src/emulator-instance"; import { scenario } from "../src/scenario"; import { Api, Browser, Mcp, Target } from "../src/services"; import type { McpSession } from "../src/surfaces/mcp"; +import { visit } from "../src/surfaces/browser"; const microsoftApi = composePluginApi([ openApiHttpPlugin({ presets: microsoftCatalog, specFormats: [microsoftGraphAdapter] }), @@ -400,7 +401,7 @@ scenario( // pre-filled from the handoff. They type ONLY the secret. yield* browser.session(identity, async ({ page, step }) => { await step("Open the agent's handoff URL", async () => { - await page.goto(handoffUrl, { waitUntil: "networkidle" }); + await visit(page, handoffUrl); }); await step("The Register-OAuth-app form auto-opens, pre-filled", async () => { diff --git a/e2e/scenarios/openapi-add-integration-action-bar.test.ts b/e2e/scenarios/openapi-add-integration-action-bar.test.ts index 3f963a9da3..839c483856 100644 --- a/e2e/scenarios/openapi-add-integration-action-bar.test.ts +++ b/e2e/scenarios/openapi-add-integration-action-bar.test.ts @@ -9,6 +9,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; // One server so "Add integration" is enabled straight from the preview (no // base-URL picker to resolve first). @@ -32,7 +33,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the Add OpenAPI integration form", async () => { - await page.goto("/integrations/add/openapi", { waitUntil: "networkidle" }); + await visit(page, "/integrations/add/openapi"); await page.getByPlaceholder("https://api.example.com/openapi.json").waitFor(); }); diff --git a/e2e/scenarios/openapi-multi-key-auth-ui.test.ts b/e2e/scenarios/openapi-multi-key-auth-ui.test.ts index 42fbd9047b..dd38630d08 100644 --- a/e2e/scenarios/openapi-multi-key-auth-ui.test.ts +++ b/e2e/scenarios/openapi-multi-key-auth-ui.test.ts @@ -13,6 +13,7 @@ import { IntegrationSlug } from "@executor-js/sdk/shared"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); @@ -61,7 +62,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the Cloudflare-style integration", async () => { - await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}`); await page.getByText("Connections").first().waitFor(); }); diff --git a/e2e/scenarios/openapi-server-selection-ui.test.ts b/e2e/scenarios/openapi-server-selection-ui.test.ts index 0451f7e892..256434ad8d 100644 --- a/e2e/scenarios/openapi-server-selection-ui.test.ts +++ b/e2e/scenarios/openapi-server-selection-ui.test.ts @@ -9,6 +9,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; // Two servers — production and staging — so the form offers a base-URL picker // instead of the single locked input a one-server spec gets. @@ -35,7 +36,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the Add OpenAPI integration form", async () => { - await page.goto("/integrations/add/openapi", { waitUntil: "networkidle" }); + await visit(page, "/integrations/add/openapi"); await page.getByPlaceholder("https://api.example.com/openapi.json").waitFor(); }); diff --git a/e2e/scenarios/org-slug-routing.test.ts b/e2e/scenarios/org-slug-routing.test.ts index a39b021f4b..3ba129bee6 100644 --- a/e2e/scenarios/org-slug-routing.test.ts +++ b/e2e/scenarios/org-slug-routing.test.ts @@ -18,6 +18,7 @@ import { AccountHttpApi, isValidOrgSlug } from "@executor-js/api"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; scenario( "Org URLs · console paths carry the organization slug", @@ -38,7 +39,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("A bare deep link canonicalizes onto the org slug", async () => { - await page.goto("/policies", { waitUntil: "networkidle" }); + await visit(page, "/policies"); await page.waitForURL((url) => url.pathname === `/${slug}/policies`, { timeout: 30_000, }); @@ -52,13 +53,13 @@ scenario( // legitimately does not. if (!target.name.startsWith("selfhost")) { await step("An unknown org slug is a wrong address, not a redirect", async () => { - await page.goto("/zz-no-such-org/policies", { waitUntil: "networkidle" }); + await visit(page, "/zz-no-such-org/policies"); await page.getByText("Page not found").waitFor({ timeout: 30_000 }); }); } await step("In-shell navigation keeps the slug prefix", async () => { - await page.goto(`/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/${slug}`); await page.getByRole("link", { name: "Policies" }).first().click(); await page.waitForURL((url) => url.pathname === `/${slug}/policies`, { timeout: 30_000, diff --git a/e2e/scenarios/policies-ui.test.ts b/e2e/scenarios/policies-ui.test.ts index cc400cd0ab..6e42865ea2 100644 --- a/e2e/scenarios/policies-ui.test.ts +++ b/e2e/scenarios/policies-ui.test.ts @@ -27,6 +27,7 @@ import { AuthTemplateSlug, ConnectionName, IntegrationSlug } from "@executor-js/ import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); @@ -161,7 +162,7 @@ scenario( const internalError = JSON.stringify({ _tag: "InternalError", traceId: "policy-write" }); await step("Open the integration's Tools tab", async () => { - await page.goto(`/integrations/${integration}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${integration}`); await page.getByRole("tab", { name: "Tools" }).click(); await sectionFor(alpha).waitFor(); await sectionFor(beta).waitFor(); @@ -288,7 +289,7 @@ scenario( }); await step("Both rules are manageable rows on the Policies page", async () => { - await page.goto("/policies", { waitUntil: "networkidle" }); + await visit(page, "/policies"); await page.getByText(leafPattern, { exact: true }).waitFor(); await page.getByText(categoryPattern, { exact: true }).waitFor(); }); diff --git a/e2e/scenarios/provider-plugins-ui.test.ts b/e2e/scenarios/provider-plugins-ui.test.ts index dd59d5f4b4..add10ccd15 100644 --- a/e2e/scenarios/provider-plugins-ui.test.ts +++ b/e2e/scenarios/provider-plugins-ui.test.ts @@ -3,7 +3,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; -import { clickToReveal } from "../src/surfaces/browser"; +import { clickToReveal, visit } from "../src/surfaces/browser"; scenario( "Provider catalog · Google and Microsoft services are OpenAPI presets", @@ -15,7 +15,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the integrations picker", async () => { - await page.goto("/integrations", { waitUntil: "networkidle" }); + await visit(page, "/integrations"); await clickToReveal( page.getByRole("button", { name: "Connect" }), page.getByRole("dialog", { name: "Connect an integration" }), diff --git a/e2e/scripts/record-connect-card.ts b/e2e/scripts/record-connect-card.ts index 8266da9229..0085740077 100644 --- a/e2e/scripts/record-connect-card.ts +++ b/e2e/scripts/record-connect-card.ts @@ -15,6 +15,7 @@ import { randomUUID } from "node:crypto"; import { chromium } from "playwright"; +import { visit } from "../src/surfaces/browser"; const [baseUrl, outPrefix] = process.argv.slice(2); if (!baseUrl || !outPrefix) { @@ -86,7 +87,7 @@ const shoot = async (label: string, javaScriptEnabled: boolean, outPath: string) // the light palette — an artifact no user sees. JS stays disabled for the // SSR shot only to freeze the pre-hydration origin (no client correction), // not to skip CSS. - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); // The install command is the thing that flashes; screenshot just its block. const command = page.locator('pre:has-text("add-mcp")').first(); await command.waitFor({ state: "visible", timeout: 15_000 }); diff --git a/e2e/selfhost/admin-users-console.test.ts b/e2e/selfhost/admin-users-console.test.ts index 23959c8d8b..fccdef9251 100644 --- a/e2e/selfhost/admin-users-console.test.ts +++ b/e2e/selfhost/admin-users-console.test.ts @@ -20,6 +20,7 @@ import { AuthTemplateSlug, ConnectionName, IntegrationSlug } from "@executor-js/ import { scenario } from "../src/scenario"; import { createInvitedIdentity } from "../targets/selfhost"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); type Client = HttpApiClient.ForApi; @@ -107,7 +108,7 @@ scenario( yield* browser.session(owner, async ({ page, step }) => { await step("Open Users from the sidebar as the instance owner", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page .locator("nav") .getByRole("link", { name: "Users" }) @@ -289,7 +290,7 @@ scenario( yield* browser.session(member, async ({ page, step }) => { await step("A plain member is not offered the section", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page .locator("nav") .getByRole("link", { name: "Integrations" }) @@ -302,7 +303,7 @@ scenario( }); await step("And reaching the URL directly is refused, not silently empty", async () => { - await page.goto("/users", { waitUntil: "networkidle" }); + await visit(page, "/users"); await page .getByText("You don't have access to this workspace's users") .waitFor({ state: "visible", timeout: 30_000 }); diff --git a/e2e/selfhost/api-keys-feedback.test.ts b/e2e/selfhost/api-keys-feedback.test.ts index eee5fde723..0f7cecce2d 100644 --- a/e2e/selfhost/api-keys-feedback.test.ts +++ b/e2e/selfhost/api-keys-feedback.test.ts @@ -20,6 +20,7 @@ import { AccountHttpApi } from "@executor-js/api"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; declare global { interface Window { @@ -39,7 +40,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Land on the dashboard", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); // The shared shell's main nav lists the workspace sections. await page.locator("nav").getByRole("link", { name: "Integrations" }).first().waitFor(); }); @@ -105,7 +106,7 @@ scenario( }); await step("Open the API keys page", async () => { - await page.goto("/api-keys", { waitUntil: "networkidle" }); + await visit(page, "/api-keys"); await page.getByRole("heading", { name: "API keys", exact: true }).waitFor(); }); @@ -174,7 +175,7 @@ scenario( }); await step("Create a new key", async () => { - await page.goto("/api-keys", { waitUntil: "networkidle" }); + await visit(page, "/api-keys"); await page.getByRole("button", { name: "New key" }).click(); const dialog = page.getByRole("dialog"); await dialog.getByLabel("Name").fill(keyName); diff --git a/e2e/selfhost/auth-methods-ui.test.ts b/e2e/selfhost/auth-methods-ui.test.ts index 6860eeafb0..fd06b0c7c6 100644 --- a/e2e/selfhost/auth-methods-ui.test.ts +++ b/e2e/selfhost/auth-methods-ui.test.ts @@ -23,6 +23,7 @@ import { IntegrationSlug } from "@executor-js/sdk/shared"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([mcpHttpPlugin()] as const); @@ -160,9 +161,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the add-MCP flow pointed at the server", async () => { - await page.goto(`/integrations/add/mcp?url=${encodeURIComponent(server.endpoint)}`, { - waitUntil: "networkidle", - }); + await visit(page, `/integrations/add/mcp?url=${encodeURIComponent(server.endpoint)}`); // Generous timeout: the debounced probe request can queue behind a // busy dev server under CI load, and there is no clean client-side // re-trigger to poke it mid-flight. @@ -216,9 +215,7 @@ scenario( yield* Effect.gen(function* () { yield* browser.session(identity, async ({ page, step }) => { await step("Open the add-MCP flow pointed at the Slack-shaped server", async () => { - await page.goto(`/integrations/add/mcp?url=${encodeURIComponent(server.endpoint)}`, { - waitUntil: "networkidle", - }); + await visit(page, `/integrations/add/mcp?url=${encodeURIComponent(server.endpoint)}`); await page.getByText("How does this server authenticate?").waitFor({ timeout: 90_000 }); await page.getByText("OAuth metadata is discovered from this server").waitFor(); }); @@ -324,9 +321,7 @@ scenario( yield* Effect.gen(function* () { yield* browser.session(identity, async ({ page, step }) => { await step("Open the integration's connect modal", async () => { - await page.goto(`/integrations/${slug}`, { - waitUntil: "networkidle", - }); + await visit(page, `/integrations/${slug}`); await page.getByRole("button", { name: "Add connection" }).first().click(); await page.getByRole("tab", { name: "OAuth" }).waitFor(); }); @@ -426,9 +421,7 @@ scenario( yield* Effect.gen(function* () { yield* browser.session(identity, async ({ page, step }) => { await step("Open the integration's connect modal", async () => { - await page.goto(`/integrations/${slug}`, { - waitUntil: "networkidle", - }); + await visit(page, `/integrations/${slug}`); await page.getByRole("button", { name: "Add connection" }).first().click(); await page.getByRole("tab", { name: "API key (Authorization)" }).waitFor(); }); diff --git a/e2e/selfhost/connect-modal-credential-ux.test.ts b/e2e/selfhost/connect-modal-credential-ux.test.ts index 1c994cfcee..f1350ee8fb 100644 --- a/e2e/selfhost/connect-modal-credential-ux.test.ts +++ b/e2e/selfhost/connect-modal-credential-ux.test.ts @@ -18,6 +18,7 @@ import { IntegrationSlug } from "@executor-js/sdk/shared"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); @@ -63,7 +64,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the connect modal", async () => { - await page.goto(`/integrations/${slug}?addAccount=1`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}?addAccount=1`); await page.getByRole("heading", { name: /Add connection/ }).waitFor(); }); diff --git a/e2e/selfhost/detected-auth-immutable-ui.test.ts b/e2e/selfhost/detected-auth-immutable-ui.test.ts index 2b9bfa795f..4fa7c8ff1e 100644 --- a/e2e/selfhost/detected-auth-immutable-ui.test.ts +++ b/e2e/selfhost/detected-auth-immutable-ui.test.ts @@ -16,6 +16,7 @@ import { OAuthTestServer } from "@executor-js/sdk/testing"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const REMOVE_HINT = "Pulled from spec. Remove to override."; @@ -36,9 +37,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Open the add-MCP flow pointed at the OAuth server", async () => { - await page.goto(`/integrations/add/mcp?url=${encodeURIComponent(server.endpoint)}`, { - waitUntil: "networkidle", - }); + await visit(page, `/integrations/add/mcp?url=${encodeURIComponent(server.endpoint)}`); await page.getByText("How does this server authenticate?").waitFor(); await page.getByText("Method 1 · Detected").waitFor(); }); @@ -145,7 +144,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("Analyze a spec that declares both API key and OAuth", async () => { - await page.goto(`/integrations/add/openapi`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/add/openapi`); await page .getByPlaceholder(/openapi\.json/i) .first() diff --git a/e2e/selfhost/mcp-auth-required-add.test.ts b/e2e/selfhost/mcp-auth-required-add.test.ts index 836175aa42..dfd6c7dd35 100644 --- a/e2e/selfhost/mcp-auth-required-add.test.ts +++ b/e2e/selfhost/mcp-auth-required-add.test.ts @@ -19,6 +19,7 @@ import { serveTestHttpApp } from "@executor-js/sdk/testing"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([mcpHttpPlugin()] as const); @@ -53,9 +54,7 @@ scenario( yield* Effect.gen(function* () { yield* browser.session(identity, async ({ page, step }) => { await step("Open the add-MCP flow pointed at the auth-gated server", async () => { - await page.goto(`/integrations/add/mcp?url=${encodeURIComponent(endpoint)}`, { - waitUntil: "networkidle", - }); + await visit(page, `/integrations/add/mcp?url=${encodeURIComponent(endpoint)}`); // Before the fix this dead-ended on a red "add credentials below" // error with no editor. Now the auth-method editor renders. await page.getByText("How does this server authenticate?").waitFor(); diff --git a/e2e/selfhost/mcp-connect-card-url.test.ts b/e2e/selfhost/mcp-connect-card-url.test.ts index 69c7b22341..99fb59f022 100644 --- a/e2e/selfhost/mcp-connect-card-url.test.ts +++ b/e2e/selfhost/mcp-connect-card-url.test.ts @@ -15,6 +15,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; scenario( "MCP · the URL printed by the Connect card is actually served", @@ -28,7 +29,7 @@ scenario( let cardUrl = ""; yield* browser.session(identity, async ({ page, step }) => { await step("Read the MCP URL from the Connect-an-agent card", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page.waitForTimeout(2000); // let the card resolve the server origin // The card renders `npx add-mcp --transport http --name executor`. // The CodeBlock tokenizes it across spans; read the concatenated text. diff --git a/e2e/selfhost/mcp-oauth-consent.test.ts b/e2e/selfhost/mcp-oauth-consent.test.ts index e62a9d281c..1e6023be82 100644 --- a/e2e/selfhost/mcp-oauth-consent.test.ts +++ b/e2e/selfhost/mcp-oauth-consent.test.ts @@ -19,6 +19,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; interface AuthServerMetadata { readonly authorization_endpoint: string; @@ -78,7 +79,7 @@ scenario( yield* browser.session(identity, async ({ page, step }) => { await step("A signed-in user is using their Executor instance", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); // Confirm we're in the app, not bounced to sign-in. expect(new URL(page.url()).pathname, "the session is active (not on /login)").not.toBe( "/login", @@ -91,7 +92,7 @@ scenario( // A connecting MCP client opens this authorize URL. The server forces // prompt=consent, so it stops here on the approval screen instead of // auto-issuing a code. - await page.goto(authorize, { waitUntil: "networkidle" }); + await visit(page, authorize); await page.locator("#mcp-consent-allow").waitFor(); expect( new URL(page.url()).pathname, diff --git a/e2e/selfhost/mcp-oauth-reconnect-health.test.ts b/e2e/selfhost/mcp-oauth-reconnect-health.test.ts index cbb6ff5090..1be7de3909 100644 --- a/e2e/selfhost/mcp-oauth-reconnect-health.test.ts +++ b/e2e/selfhost/mcp-oauth-reconnect-health.test.ts @@ -21,6 +21,7 @@ import { serveOAuthTestServer, type OAuthTestServerShape } from "@executor-js/sd import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([mcpHttpPlugin()] as const); type Client = HttpApiClient.ForApi; @@ -300,7 +301,7 @@ scenario( const menuTrigger = connections.locator('button[aria-haspopup="menu"]').first(); await step("Open the MCP integration with its expired OAuth connection", async () => { - await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}`); await connections.getByText("main", { exact: true }).waitFor({ timeout: 30_000 }); }); @@ -380,7 +381,7 @@ scenario( const menuTrigger = connections.locator('button[aria-haspopup="menu"]').first(); await step("Open the integration: the connection reads Expired", async () => { - await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}`); await connections.getByText("main", { exact: true }).waitFor({ timeout: 30_000 }); await connections.getByLabel("Status: Expired").waitFor({ timeout: 30_000 }); }); @@ -451,7 +452,7 @@ scenario( }); await step("Open the MCP integration with its DCR OAuth connection", async () => { - await page.goto(`/integrations/${slug}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${slug}`); await connections.getByText("main", { exact: true }).waitFor({ timeout: 30_000 }); }); diff --git a/e2e/selfhost/mcp-oauth-slow-connect.test.ts b/e2e/selfhost/mcp-oauth-slow-connect.test.ts new file mode 100644 index 0000000000..56422ba598 --- /dev/null +++ b/e2e/selfhost/mcp-oauth-slow-connect.test.ts @@ -0,0 +1,133 @@ +// Selfhost browser coverage for the transparent DCR connect when OAuth +// discovery is slow, the shape behind the report: "When I hit connect, it loads +// for a second, but then theres no user feedback and nothing happens." +// +// One click runs probe -> register -> start. The first two are network round +// trips, and `window.open` needs transient user activation, which a real +// browser expires a few seconds after the click. The shipped code opened the +// sign-in window AFTER both round trips, so once the API was slow the browser +// refused it and the connect died silently. The window is now claimed on the +// click and navigated later, so discovery latency no longer decides whether the +// user can connect. +// +// What this scenario guards: the whole slow path still works end to end, with +// the reservation threaded from the click through registration to a window that +// really lands on the discovered authorization server. +// +// What it CANNOT guard, and why: Playwright drives Chromium in automation mode, +// which never enforces the activation rule for `window.open` (verified headed +// and headless, and with `--disable-popup-blocking` removed via +// ignoreDefaultArgs). So a browser here opens the window no matter how stale +// the click is, and this scenario passes against the pre-fix ordering too. The +// ordering itself is pinned by unit tests over `runDcrConnect` in +// packages/react/src/components/add-account-modal.test.ts, which do fail when +// the reservation moves back after the round trips. See LEARNINGS.md. +import { randomBytes } from "node:crypto"; + +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; +import { composePluginApi } from "@executor-js/api/server"; +import { deriveMcpNamespace } from "@executor-js/plugin-mcp"; +import { mcpHttpPlugin } from "@executor-js/plugin-mcp/api"; +import { makeGreetingMcpServer, serveMcpServerWithOAuth } from "@executor-js/plugin-mcp/testing"; +import { IntegrationSlug } from "@executor-js/sdk/shared"; +import { OAuthTestServer } from "@executor-js/sdk/testing"; + +import { scenario } from "../src/scenario"; +import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; + +const api = composePluginApi([mcpHttpPlugin()] as const); + +// Comfortably past Chromium's ~5s transient user activation once both land on +// the same click, and well under the step timeouts below. +const STALL_MS = 3_500; + +const isDiscoveryCall = (url: string): boolean => + url.includes("/api/oauth/probe") || url.includes("/api/oauth/clients/register-dynamic"); + +scenario( + "MCP OAuth · a slow discovery round trip still opens the sign-in window", + { timeout: 240_000 }, + Effect.scoped( + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const { client: makeApiClient } = yield* Api; + const oauth = yield* OAuthTestServer; + const server = yield* serveMcpServerWithOAuth( + () => makeGreetingMcpServer({ name: "slow-connect-mcp" }), + { path: "/mcp" }, + ); + const identity = yield* target.newIdentity(); + const client = yield* makeApiClient(api, identity); + const displayName = `Slow MCP ${randomBytes(3).toString("hex")}`; + const slug = IntegrationSlug.make(deriveMcpNamespace({ name: displayName })); + + yield* Effect.gen(function* () { + yield* browser.session(identity, async ({ page, step }) => { + await step("Add an OAuth-protected MCP integration", async () => { + const addUrl = new URL("/integrations/add/mcp", target.baseUrl); + addUrl.searchParams.set("url", server.endpoint); + await visit(page, addUrl.toString()); + await page.getByText("How does this server authenticate?").waitFor({ timeout: 30_000 }); + await page.getByPlaceholder("e.g. Linear").fill(displayName); + await page.getByRole("button", { name: "Add integration" }).click(); + await page.waitForURL(/\/integrations\/(?!add\b)[^/?]+$/, { timeout: 30_000 }); + await page.getByText("Connections").first().waitFor(); + }); + + await step("Make OAuth discovery slow, the way a degraded API is", async () => { + // Delay the responses rather than the requests, so the app sees a + // genuinely slow API and not a stalled network stack. + await page.route( + (url) => isDiscoveryCall(url.href), + async (route) => { + await new Promise((resolve) => setTimeout(resolve, STALL_MS)); + await route.continue(); + }, + ); + }); + + await step("Connect, and wait out the slow discovery", async () => { + await page.getByRole("button", { name: "Add connection" }).first().click(); + await page.getByRole("heading", { name: /Add connection/ }).waitFor(); + await page.getByRole("tab", { name: "OAuth" }).waitFor(); + + const popupPromise = page.waitForEvent("popup", { timeout: 60_000 }); + await page.getByRole("button", { name: "Connect", exact: true }).click(); + + // In a real browser the two stalls outlast the click's user + // activation, so this popup only exists because it was reserved on + // the click. Automation-mode Chromium would open it either way; + // the ordering is pinned by the unit tests named above. + const popup = await popupPromise; + await popup.waitForURL((url) => url.origin === new URL(oauth.issuerUrl).origin, { + timeout: 60_000, + }); + await popup.waitForLoadState("domcontentloaded", { timeout: 30_000 }); + expect( + new URL(popup.url()).origin, + "the reserved window reached the discovered authorization host", + ).toBe(new URL(oauth.authorizationEndpoint).origin); + await popup.close(); + }); + }); + + const oauthRequests = yield* oauth.requests; + expect( + oauthRequests.some( + (request) => request.method === "POST" && request.path === "/register", + ), + "the slow connect still dynamically registered its OAuth client", + ).toBe(true); + expect( + oauthRequests.some( + (request) => request.method === "GET" && request.path === "/authorize", + ), + "the slow connect still reached the authorize endpoint", + ).toBe(true); + }).pipe(Effect.ensuring(client.mcp.removeServer({ params: { slug } }).pipe(Effect.ignore))); + }), + ).pipe(Effect.provide(OAuthTestServer.layer())), +); diff --git a/e2e/selfhost/mcp-opencode-consent.test.ts b/e2e/selfhost/mcp-opencode-consent.test.ts index ceda537798..9461189d84 100644 --- a/e2e/selfhost/mcp-opencode-consent.test.ts +++ b/e2e/selfhost/mcp-opencode-consent.test.ts @@ -22,6 +22,7 @@ import { Effect } from "effect"; import { scenario } from "../src/scenario"; import { enterFocus, markRecordingStart } from "../src/timeline"; import { Browser, Cli, Mcp, OpenCode, RunDir, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const SERVER_NAME = "executor"; @@ -119,7 +120,7 @@ scenario( await step("OpenCode asks to connect — the approval screen", async () => { // Authenticated (owner cookies) → authorize forces prompt=consent → // the approval screen. - await page.goto(authorizeUrl, { waitUntil: "networkidle" }); + await visit(page, authorizeUrl); await page.locator("#mcp-consent-allow").waitFor({ timeout: 30_000 }); expect(new URL(page.url()).pathname, "lands on the approval screen").toBe( "/mcp-consent", diff --git a/e2e/selfhost/oauth-app-modal.test.ts b/e2e/selfhost/oauth-app-modal.test.ts index d699d6c501..a4ec6b7371 100644 --- a/e2e/selfhost/oauth-app-modal.test.ts +++ b/e2e/selfhost/oauth-app-modal.test.ts @@ -16,6 +16,7 @@ import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); @@ -81,7 +82,7 @@ scenario( const actions = page.getByRole("button", { name: `Actions for ${appName}` }); await step("Open the integration and start a new connection", async () => { - await page.goto(`/integrations/${integration}`, { waitUntil: "networkidle" }); + await visit(page, `/integrations/${integration}`); await page.getByRole("button", { name: "Add connection" }).click(); // OAuth2 is the integration's only method, so the modal opens on // the OAuth app step with nothing registered yet. (`exact` avoids diff --git a/e2e/selfhost/oauth-callback-unauthenticated.test.ts b/e2e/selfhost/oauth-callback-unauthenticated.test.ts index ae43ffb837..72126a33fc 100644 --- a/e2e/selfhost/oauth-callback-unauthenticated.test.ts +++ b/e2e/selfhost/oauth-callback-unauthenticated.test.ts @@ -14,6 +14,7 @@ import { serveOAuthTestServer } from "@executor-js/sdk/testing"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([openApiHttpPlugin()] as const); @@ -111,7 +112,7 @@ scenario( yield* browser.session({ label: "anonymous" }, async ({ page, step }) => { await step("Provider sends a signed-out browser to the OAuth callback", async () => { - const response = await page.goto(callbackPath, { waitUntil: "networkidle" }); + const response = await visit(page, callbackPath); expect(response?.status(), "the callback redirects into the login flow").toBe(200); await page.getByRole("heading", { name: "Sign in" }).waitFor(); }); diff --git a/e2e/selfhost/posthog-mcp-oauth.test.ts b/e2e/selfhost/posthog-mcp-oauth.test.ts index 49d20349a9..e6c3efe854 100644 --- a/e2e/selfhost/posthog-mcp-oauth.test.ts +++ b/e2e/selfhost/posthog-mcp-oauth.test.ts @@ -22,6 +22,7 @@ import { OAuthTestServer } from "@executor-js/sdk/testing"; import { scenario } from "../src/scenario"; import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; const api = composePluginApi([mcpHttpPlugin()] as const); @@ -48,7 +49,7 @@ scenario( await step("Open the add-MCP flow pointed at the OAuth server", async () => { const addUrl = new URL("/integrations/add/mcp", target.baseUrl); addUrl.searchParams.set("url", server.endpoint); - await page.goto(addUrl.toString(), { waitUntil: "networkidle" }); + await visit(page, addUrl.toString()); await page.getByText("How does this server authenticate?").waitFor({ timeout: 30_000 }); await page.getByText("Method 1 · Detected").waitFor(); await page.getByText("OAuth metadata is discovered from this server").waitFor(); diff --git a/e2e/src/emulator-instance.ts b/e2e/src/emulator-instance.ts index 6d5174b206..d17ebc7117 100644 --- a/e2e/src/emulator-instance.ts +++ b/e2e/src/emulator-instance.ts @@ -1,20 +1,71 @@ -import { Effect } from "effect"; +import { Effect, Schedule } from "effect"; + +/** The suite could not get an instance out of the hosted control plane. */ +export class EmulatorInstanceError extends Error { + readonly _tag = "EmulatorInstanceError"; + + constructor( + readonly service: string, + readonly reason: string, + ) { + super(`${service} emulator instance creation failed: ${reason}`); + this.name = "EmulatorInstanceError"; + } +} + +// Bound each attempt: a hung connection to the edge must not eat the +// scenario's whole timeout before the first retry. +const ATTEMPT_TIMEOUT = "10 seconds"; +const RETRIES = 3; + +const requestInstance = (service: string, label: string) => + Effect.tryPromise({ + try: async (): Promise => { + const response = await fetch(`https://${service}.emulators.dev/_emulate/instances`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ instance: label }), + }); + if (!response.ok) { + throw new EmulatorInstanceError(service, `HTTP ${response.status}`); + } + const instance = (await response.json()) as { readonly providerBaseUrl: string }; + return instance.providerBaseUrl; + }, + catch: (cause) => + cause instanceof EmulatorInstanceError + ? cause + : new EmulatorInstanceError(service, String(cause)), + }); // Hosted service hosts (e.g. resend.emulators.dev) are control plane only — // there is no shared default instance behind them. Every scenario creates its // own isolated instance and works against the returned providerBaseUrl, which // also keeps ledger assertions free of cross-run pollution. The server // generates an unguessable instance name; the label is a readable prefix. -export const createEmulatorInstance = (service: string, label = "e2e") => - Effect.promise(async () => { - const response = await fetch(`https://${service}.emulators.dev/_emulate/instances`, { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify({ instance: label }), - }); - if (!response.ok) { - throw new Error(`${service} emulator instance creation failed: ${response.status}`); - } - const instance = (await response.json()) as { readonly providerBaseUrl: string }; - return instance.providerBaseUrl; - }); +// +// This is also the one request in a scenario that leaves the runner, so it is +// the one place where a CI runner's transient network trouble fails a scenario +// that has nothing to do with the network: `connect ETIMEDOUT` reaching the +// edge, plus the occasional bare 502, accounted for 17 shard failures in the +// two weeks to 2026-08-20. Asking for an instance is idempotent (a spare +// instance is nobody's business but the control plane's), so bound the attempt +// and retry with backoff. Nothing below this line retries anything the +// scenario is actually asserting on. +export const createEmulatorInstance = (service: string, label = "e2e"): Effect.Effect => + requestInstance(service, label).pipe( + Effect.timeoutOrElse({ + duration: ATTEMPT_TIMEOUT, + orElse: () => + Effect.fail(new EmulatorInstanceError(service, `no response in ${ATTEMPT_TIMEOUT}`)), + }), + Effect.retry( + Schedule.both( + Schedule.exponential("500 millis").pipe(Schedule.jittered), + Schedule.recurs(RETRIES), + ), + ), + // An emulator the suite cannot reach at all is a defect in the run, not a + // product failure the scenario should be asked to model. + Effect.orDie, + ); diff --git a/e2e/src/surfaces/autumn.ts b/e2e/src/surfaces/autumn.ts index a831491178..a401cf5486 100644 --- a/e2e/src/surfaces/autumn.ts +++ b/e2e/src/surfaces/autumn.ts @@ -76,6 +76,14 @@ export interface AutumnSurface { * which drives `remaining` to zero. This is the setup a "blocked at cap" * scenario runs before opening the session it expects to be gated. */ readonly exhaustExecutions: (customerId: string) => Effect.Effect; + /** Put an org on a plan directly (`billing.attach`), skipping the hosted + * checkout. Only valid for a plan that needs no payment — Enterprise carries + * no price and no card-required trial, so the emulator activates its + * subscription inline. Team does have both, so it must still go through the + * browser checkout + `settleCheckout`. This is the setup a scenario runs when + * it needs a PAID org and the upgrade journey itself is not what's under + * test (e.g. the rate-limit backstop's paid exemption). */ + readonly attachPlan: (customerId: string, planId: string) => Effect.Effect; /** Arm a fault against a matching Autumn request (`POST /_emulate/faults`). */ readonly armFault: (input: FaultInput) => Effect.Effect; /** Clear every armed fault (`DELETE /_emulate/faults`) — the finalizer that @@ -155,6 +163,33 @@ export const makeAutumnSurface = (autumnUrl: string): AutumnSurface => { } }); + const attachPlan = (customerId: string, planId: string) => + Effect.gen(function* () { + const response = yield* Effect.promise(() => + fetch(`${autumnUrl}/v1/billing.attach`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ customer_id: customerId, plan_id: planId }), + }), + ); + if (!response.ok) { + return yield* Effect.fail( + `autumn billing.attach responded ${response.status}: ${yield* Effect.promise(() => response.text())}`, + ); + } + // A plan that needs payment answers with a checkout URL instead of + // activating; that silently leaves the org on Free, and a scenario that + // asked for a paid org would then assert against the wrong plan. + const body = (yield* Effect.promise(() => response.json())) as { + readonly payment_url?: string | null; + }; + if (body.payment_url) { + return yield* Effect.fail( + `autumn billing.attach returned a checkout URL for '${planId}': it requires payment, so it cannot be attached directly`, + ); + } + }); + const armFault = (input: FaultInput) => Effect.gen(function* () { const response = yield* Effect.promise(() => @@ -213,6 +248,7 @@ export const makeAutumnSurface = (autumnUrl: string): AutumnSurface => { usageEvents, settleCheckout, exhaustExecutions, + attachPlan, armFault, clearFaults, ledgerFor, diff --git a/e2e/src/surfaces/browser.ts b/e2e/src/surfaces/browser.ts index 1ac33e305c..6510f77d9a 100644 --- a/e2e/src/surfaces/browser.ts +++ b/e2e/src/surfaces/browser.ts @@ -9,7 +9,7 @@ import { join } from "node:path"; import { promisify } from "node:util"; import { Effect } from "effect"; -import { chromium, type Locator, type Page } from "playwright"; +import { chromium, type Locator, type Page, type Response } from "playwright"; import { beat, enterFocus, markNavigation, markRecordingStart } from "../timeline"; import { appendTraces, type TraceEntry } from "../trace-harvest"; @@ -36,10 +36,103 @@ const slug = (text: string): string => .replace(/^-+|-+$/g, "") .slice(0, 60); +// How long a navigation may wait for the network to go quiet before giving up +// on quiet and letting the scenario's own assertion decide. Long enough to +// cover a cold vite compile's request burst, short enough that a page which +// never goes quiet costs seconds instead of the 30s navigation timeout. +const SETTLE_TIMEOUT_MS = 5_000; + +/** + * Wait for the network to go quiet, but only for `SETTLE_TIMEOUT_MS`. + * + * Playwright's `networkidle` — 500ms with zero in-flight requests — cannot be + * a hard gate in this suite, because the suite itself keeps the network busy: + * every browser session exports OTel spans to the run's motel through + * packages/react's `OtlpTracer` on a one-second interval (wired by + * `setup/motel.ts` via VITE_PUBLIC_OTLP_TRACES_URL). A page that keeps + * producing spans — a console retrying an org-scoped query that 403s, say — + * feeds that exporter indefinitely, so the 500ms window never opens and the + * wait burns its full timeout on a page that is, visibly, completely loaded. + * + * The quiet is worth waiting for when it comes (it usually arrives in + * milliseconds, and it lets a step's screenshot catch a settled page), so keep + * it — bounded. Readiness is asserted by whatever the scenario does next: + * Playwright's locators auto-wait, so `getByRole(...).waitFor()` is a real, + * page-specific readiness signal where `networkidle` was only ever a proxy. + */ +export const settle = async (page: Page): Promise => { + await page + .waitForLoadState("networkidle", { timeout: SETTLE_TIMEOUT_MS }) + // oxlint-disable-next-line executor/no-promise-catch -- boundary: quiet is best-effort, the scenario's next assertion is the gate + .catch(() => {}); +}; + +/** + * Wait until React owns the document — the readiness `networkidle` was standing + * in for, asked directly. + * + * React DOM stamps `__reactContainer$` on the element it roots at the + * moment `hydrateRoot` runs, so its presence says the SSR markup is under a + * live root rather than inert HTML. That matters because it is the line either + * side of which an interaction has a different meaning: before it, a click or + * a `fill` goes to markup nobody is listening to and is lost without a trace; + * after it, React records the event and replays it as hydration reaches that + * subtree. Locators can't see the difference — the element is visible and + * enabled either way — which is why a swallowed interaction surfaces 20 + * seconds later as "the thing it should have opened never appeared". + * + * Best-effort, like `settle`: plenty of pages the suite visits are not this + * app at all (a provider's authorize screen, an OAuth callback result), and + * they are none the worse for having no React root. + */ +const HYDRATION_TIMEOUT_MS = 10_000; + +export const hydrated = async (page: Page): Promise => { + await page + .waitForFunction( + () => + [...document.body.children].some((element) => + Object.keys(element).some((key) => key.startsWith("__reactContainer$")), + ), + undefined, + { timeout: HYDRATION_TIMEOUT_MS }, + ) + // oxlint-disable-next-line executor/no-promise-catch -- boundary: a page with no React root is a legitimate destination + .catch(() => {}); +}; + +/** + * Navigate, then wait for the page to be interactive. The drop-in for a `goto` + * that waited on `networkidle`, and a better one: it waits for React to take + * the document (`hydrated`) and for the network to go quiet (`settle`), both + * bounded, instead of one unbounded wait that proved neither. `timeout` bounds + * the navigation itself, exactly as `goto`'s does. + */ +export const visit = async ( + page: Page, + url: string, + options: { readonly timeout?: number } = {}, +): Promise => { + const response = await page.goto(url, { waitUntil: "load", ...options }); + await hydrated(page); + await settle(page); + return response; +}; + +/** `visit` for a reload — the drop-in for `reload({ waitUntil: "networkidle" })`. */ +export const revisit = async ( + page: Page, + options: { readonly timeout?: number } = {}, +): Promise => { + await page.reload({ waitUntil: "load", ...options }); + await hydrated(page); + await settle(page); +}; + /** * Click `trigger` until `revealed` is visible. * - * `waitUntil: "networkidle"` does not mean the console has hydrated: a click + * A settled network does not mean the console has hydrated: a click * that lands between the SSR paint and React attaching the handler is * swallowed without a trace, and whatever the click was meant to open never * appears (the "Connect an integration" dialog no-show flake). Re-clicking a diff --git a/e2e/src/update-card-render.ts b/e2e/src/update-card-render.ts index 4046ab5353..66c4ef3fd0 100644 --- a/e2e/src/update-card-render.ts +++ b/e2e/src/update-card-render.ts @@ -10,6 +10,7 @@ import { Effect } from "effect"; import { scenario } from "./scenario"; import { Browser, Target } from "./services"; +import { visit } from "./surfaces/browser"; export const FORCED_LATEST = "99.0.0"; @@ -82,7 +83,7 @@ export const registerUpdateCardCurrentScenario = (name: string): void => ); await step("Open the console", async () => { - await page.goto("/", { waitUntil: "networkidle" }); + await visit(page, "/"); await page.getByRole("heading", { name: "Integrations" }).waitFor({ timeout: 60_000 }); }); diff --git a/packages/core/integrations-registry/src/registry.test.ts b/packages/core/integrations-registry/src/registry.test.ts index 20da742fbd..589cac482a 100644 --- a/packages/core/integrations-registry/src/registry.test.ts +++ b/packages/core/integrations-registry/src/registry.test.ts @@ -131,6 +131,10 @@ describe("IntegrationsRegistry", () => { Effect.provide( integrationsRegistryLayer({ userAgent: TEST_USER_AGENT, + // Pinned: this case asserts the fetch happens, so it must not + // inherit an ambient DO_NOT_TRACK / EXECUTOR_DISABLE_INTEGRATIONS_FETCH + // (CI sets both). Mirrors the analytics package's test config. + disabled: false, cacheDir, url: "https://integrations.test/api.json", }).pipe(Layer.provide(httpLayer), Layer.provide(NodeFileSystem.layer)), @@ -164,6 +168,10 @@ describe("IntegrationsRegistry", () => { Effect.provide( integrationsRegistryLayer({ userAgent: TEST_USER_AGENT, + // Pinned: this case asserts the fetch happens, so it must not + // inherit an ambient DO_NOT_TRACK / EXECUTOR_DISABLE_INTEGRATIONS_FETCH + // (CI sets both). Mirrors the analytics package's test config. + disabled: false, cacheDir, url: "https://integrations.test/api.json", }).pipe(Layer.provide(httpLayer), Layer.provide(NodeFileSystem.layer)), diff --git a/packages/kernel/core/package.json b/packages/kernel/core/package.json index be8580478e..657dacd8b2 100644 --- a/packages/kernel/core/package.json +++ b/packages/kernel/core/package.json @@ -15,6 +15,7 @@ "dist" ], "type": "module", + "sideEffects": false, "exports": { ".": "./src/index.ts" }, diff --git a/packages/kernel/core/src/json-schema.ts b/packages/kernel/core/src/json-schema.ts index 3cfdefd156..f7c976284d 100644 --- a/packages/kernel/core/src/json-schema.ts +++ b/packages/kernel/core/src/json-schema.ts @@ -5,14 +5,33 @@ import addFormats from "ajv-formats"; import type { StandardSchema } from "./types"; import { unknownInputSchema } from "./types"; -const ajv = new Ajv2020({ - allErrors: true, - strict: false, - validateSchema: false, - allowUnionTypes: true, -}); +// Built on first use, not at import. Two reasons, and the second is the one +// that matters beyond this file: +// +// 1. Constructing an Ajv instance and registering every format is real work, +// and it ran on each cold isolate that so much as imported this package — +// including the many that never validate a schema. +// 2. `new Ajv2020(...)` + `addFormats(...)` at module scope are import-time +// side effects, so a bundler must keep this module (and therefore ajv) +// whenever anything touches the package barrel. `packages/core/execution` +// imports exactly one runtime value from it — `CodeExecutionError` — and +// the rest as erased types, yet still dragged ajv AND sucrase into the +// server graph. With no module-scope effects left here, `sideEffects: +// false` in package.json is honest and the unused modules tree-shake out. +let ajvInstance: Ajv2020 | undefined; -addFormats(ajv); +const getAjv = (): Ajv2020 => { + if (ajvInstance === undefined) { + ajvInstance = new Ajv2020({ + allErrors: true, + strict: false, + validateSchema: false, + allowUnionTypes: true, + }); + addFormats(ajvInstance); + } + return ajvInstance; +}; const decodePointerSegment = (segment: string): PropertyKey => { const decoded = segment.replaceAll("~1", "/").replaceAll("~0", "~"); @@ -47,7 +66,7 @@ export const standardSchemaFromJsonSchema = ( ): StandardSchema => { // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: AJV compile throws for invalid schemas and this adapter preserves fallback behavior try { - const validate = ajv.compile(schema as Record); + const validate = getAjv().compile(schema as Record); return { "~standard": { diff --git a/packages/plugins/mcp/src/sdk/plugin.ts b/packages/plugins/mcp/src/sdk/plugin.ts index e3b7a6857e..01f5ad7bc6 100644 --- a/packages/plugins/mcp/src/sdk/plugin.ts +++ b/packages/plugins/mcp/src/sdk/plugin.ts @@ -52,6 +52,7 @@ import { invokeMcpTool, isUnknownToolMessage } from "./invoke"; import { deriveMcpNamespace, type McpToolManifestEntry } from "./manifest"; import { mcpPresets } from "./presets"; import { probeMcpEndpointShape, type McpShapeProbeResult } from "./probe-shape"; +import { recoverSlackConnectFile } from "./slack-connect-file"; import { McpAuthMethodInput, McpAuthShorthand, @@ -1296,12 +1297,13 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { } } + const invokeHttpClientLayer = options?.httpClientLayer ?? ctx.httpClientLayer; const connectorInput = yield* buildConnectorInput( parsed, credential.values, String(credential.template), allowStdio, - options?.httpClientLayer ?? ctx.httpClientLayer, + invokeHttpClientLayer, ); const connector: McpConnector = createMcpConnector(connectorInput); const poolKey = @@ -1353,6 +1355,18 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { .markToolsStale(connectionRef) .pipe(Effect.ignore, Effect.as(unknownToolFailure(String(toolRow.name), credential))); } + if (parsed.transport === "remote") { + const recoveredSlackConnectFile = yield* recoverSlackConnectFile({ + endpoint: parsed.endpoint, + toolName: stamp.toolName, + args, + accessToken: credential.values[TOKEN_VARIABLE], + upstreamErrorMessage: errorMessage, + }).pipe(Effect.provide(invokeHttpClientLayer)); + if (Option.isSome(recoveredSlackConnectFile)) { + return ToolResult.ok(recoveredSlackConnectFile.value); + } + } return ToolResult.fail({ code: "mcp_tool_error", message: errorMessage, diff --git a/packages/plugins/mcp/src/sdk/slack-connect-file.integration.test.ts b/packages/plugins/mcp/src/sdk/slack-connect-file.integration.test.ts new file mode 100644 index 0000000000..696fb12f5c --- /dev/null +++ b/packages/plugins/mcp/src/sdk/slack-connect-file.integration.test.ts @@ -0,0 +1,182 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Effect, Layer, Option, Schema } from "effect"; +import { HttpClient, HttpClientRequest, HttpClientResponse } from "effect/unstable/http"; + +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + ToolAddress, + createExecutor, +} from "@executor-js/sdk"; +import { makeTestConfig, memoryCredentialsPlugin } from "@executor-js/sdk/testing"; + +import { mcpPlugin } from "./plugin"; + +const FILE_ID = "F012ABC3456"; +const IMAGE_BYTES = new Uint8Array([137, 80, 78, 71, 13, 10, 26, 10]); +const seenRpcMethods: string[] = []; + +const JsonRpcRequest = Schema.Struct({ + id: Schema.optional(Schema.Union([Schema.String, Schema.Number, Schema.Null])), + method: Schema.String, +}); +const decodeJsonRpcRequest = Schema.decodeUnknownOption(Schema.fromJsonString(JsonRpcRequest)); + +const jsonRpcResponse = (request: typeof JsonRpcRequest.Type, result: unknown): Response => + Response.json({ jsonrpc: "2.0", id: request.id ?? null, result }); + +const slackFallbackHttpClientLayer = Layer.succeed(HttpClient.HttpClient)( + HttpClient.make((request: HttpClientRequest.HttpClientRequest) => + Effect.gen(function* () { + const webRequest = yield* HttpClientRequest.toWeb(request).pipe(Effect.orDie); + const url = new URL(webRequest.url); + + if (url.hostname === "slack.com" && url.pathname === "/api/files.info") { + return HttpClientResponse.fromWeb( + request, + Response.json({ + ok: true, + file: { + id: FILE_ID, + name: "external-screenshot.png", + mimetype: "image/png", + size: IMAGE_BYTES.byteLength, + url_private_download: `https://files.slack.com/files-pri/T000-${FILE_ID}/download/external-screenshot.png`, + }, + }), + ); + } + + if (url.hostname === "files.slack.com") { + return HttpClientResponse.fromWeb( + request, + new Response(IMAGE_BYTES, { status: 200, headers: { "content-type": "image/png" } }), + ); + } + + if (url.hostname !== "mcp.slack.com") { + return HttpClientResponse.fromWeb( + request, + new Response("unexpected host", { status: 500 }), + ); + } + if (webRequest.method === "GET") { + return HttpClientResponse.fromWeb(request, new Response("SSE disabled", { status: 405 })); + } + + const rpc = Option.getOrUndefined( + decodeJsonRpcRequest(yield* Effect.promise(() => webRequest.text())), + ); + if (rpc === undefined) { + return HttpClientResponse.fromWeb( + request, + new Response("invalid JSON-RPC", { status: 400 }), + ); + } + seenRpcMethods.push(rpc.method); + if (rpc.method === "initialize") { + return HttpClientResponse.fromWeb( + request, + jsonRpcResponse(rpc, { + protocolVersion: "2025-06-18", + capabilities: { tools: {} }, + serverInfo: { name: "Slack", version: "1.0.0" }, + }), + ); + } + if (rpc.method === "notifications/initialized") { + return HttpClientResponse.fromWeb(request, new Response("", { status: 202 })); + } + if (rpc.method === "tools/list") { + return HttpClientResponse.fromWeb( + request, + jsonRpcResponse(rpc, { + tools: [ + { + name: "slack_read_file", + inputSchema: { + type: "object", + properties: { file_id: { type: "string" } }, + required: ["file_id"], + }, + }, + ], + }), + ); + } + if (rpc.method === "tools/call") { + return HttpClientResponse.fromWeb( + request, + jsonRpcResponse(rpc, { + isError: true, + content: [{ type: "text", text: "execution_failed: file_not_found" }], + }), + ); + } + return HttpClientResponse.fromWeb( + request, + new Response("unexpected method", { status: 400 }), + ); + }), + ), +); + +describe("Slack Connect file fallback", () => { + it.effect("recovers the image through the caller-visible MCP tool", () => + Effect.scoped( + Effect.gen(function* () { + const config = { + ...makeTestConfig({ + plugins: [ + memoryCredentialsPlugin(), + mcpPlugin({ httpClientLayer: slackFallbackHttpClientLayer }), + ] as const, + }), + httpClientLayer: slackFallbackHttpClientLayer, + }; + const executor = yield* Effect.acquireRelease(createExecutor(config), (executor) => + Effect.gen(function* () { + yield* executor.close().pipe(Effect.ignore); + yield* Effect.promise(() => config.testDb.close()).pipe(Effect.ignore); + }), + ); + + yield* executor.mcp.addServer({ + name: "Slack", + endpoint: "https://mcp.slack.com/mcp", + slug: "slack_connect_fixture", + remoteTransport: "streamable-http", + auth: { kind: "oauth2" }, + }); + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("main"), + integration: IntegrationSlug.make("slack_connect_fixture"), + template: AuthTemplateSlug.make("oauth2"), + value: "xoxp-test-token", + }); + + const toolAddresses = (yield* executor.tools.list()).map((tool) => String(tool.address)); + expect(seenRpcMethods).toContain("tools/list"); + expect(toolAddresses).toContain("tools.slack_connect_fixture.org.main.slack_read_file"); + + const result = yield* executor.execute( + ToolAddress.make("tools.slack_connect_fixture.org.main.slack_read_file"), + { file_id: FILE_ID }, + { onElicitation: "accept-all" }, + ); + + expect(result).toMatchObject({ + ok: true, + data: { + content: [ + { type: "text", text: expect.stringContaining(FILE_ID) }, + { type: "image", mimeType: "image/png" }, + ], + }, + }); + }), + ), + ); +}); diff --git a/packages/plugins/mcp/src/sdk/slack-connect-file.test.ts b/packages/plugins/mcp/src/sdk/slack-connect-file.test.ts new file mode 100644 index 0000000000..5eab9dc116 --- /dev/null +++ b/packages/plugins/mcp/src/sdk/slack-connect-file.test.ts @@ -0,0 +1,171 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Effect, Encoding, Layer, Option } from "effect"; +import { HttpClient, HttpClientRequest, HttpClientResponse } from "effect/unstable/http"; + +import { recoverSlackConnectFile } from "./slack-connect-file"; + +const ACCESS_TOKEN = "xoxp-test-token"; +const FILE_ID = "F012ABC3456"; +const IMAGE_BYTES = new Uint8Array([137, 80, 78, 71, 13, 10, 26, 10]); + +const httpClientLayer = ( + respond: (request: Request) => Response, +): Layer.Layer => + Layer.succeed(HttpClient.HttpClient)( + HttpClient.make((request: HttpClientRequest.HttpClientRequest) => { + const url = new URL(request.url); + for (const [name, value] of request.urlParams) url.searchParams.append(name, value); + return Effect.succeed( + HttpClientResponse.fromWeb( + request, + respond(new Request(url, { method: request.method, headers: request.headers })), + ), + ); + }), + ); + +const recover = ( + layer: Layer.Layer, + overrides: Partial[0]> = {}, +) => + recoverSlackConnectFile({ + endpoint: "https://mcp.slack.com/mcp", + toolName: "slack_read_file", + args: { file_id: FILE_ID }, + accessToken: ACCESS_TOKEN, + upstreamErrorMessage: "execution_failed: file_not_found", + ...overrides, + }).pipe(Effect.provide(layer)); + +describe("recoverSlackConnectFile", () => { + it.effect("resolves and downloads a Slack Connect image with the existing OAuth token", () => + Effect.gen(function* () { + const requests: Request[] = []; + const layer = httpClientLayer((request) => { + requests.push(request); + const url = new URL(request.url); + if (url.hostname === "slack.com") { + return Response.json({ + ok: true, + file: { + id: FILE_ID, + name: "screenshot.png", + title: "screenshot.png", + mimetype: "image/png", + size: IMAGE_BYTES.byteLength, + url_private_download: `https://files.slack.com/files-pri/T000-${FILE_ID}/download/screenshot.png`, + }, + }); + } + return new Response(IMAGE_BYTES, { + status: 200, + headers: { "content-type": "image/png" }, + }); + }); + + const result = yield* recover(layer); + + expect(Option.isSome(result)).toBe(true); + const recovered = Option.getOrThrow(result); + expect(recovered.content).toEqual([ + { + type: "text", + text: `File ID: ${FILE_ID}\nTitle: screenshot.png\nMIME Type: image/png\nSize: 8 bytes\n`, + }, + { + type: "image", + data: Encoding.encodeBase64(IMAGE_BYTES), + mimeType: "image/png", + }, + ]); + expect(requests).toHaveLength(2); + expect(requests.map((request) => new URL(request.url).searchParams.get("file"))).toEqual([ + FILE_ID, + null, + ]); + expect(requests.map((request) => request.headers.get("authorization"))).toEqual([ + `Bearer ${ACCESS_TOKEN}`, + `Bearer ${ACCESS_TOKEN}`, + ]); + }), + ); + + it.effect("does not call Slack for unrelated MCP failures", () => + Effect.gen(function* () { + let requestCount = 0; + const layer = httpClientLayer(() => { + requestCount += 1; + return new Response("unexpected", { status: 500 }); + }); + + const results = yield* Effect.all([ + recover(layer, { endpoint: "https://example.com/mcp" }), + recover(layer, { toolName: "another_tool" }), + recover(layer, { upstreamErrorMessage: "execution_failed: permission_denied" }), + recover(layer, { accessToken: null }), + recover(layer, { args: { file_id: "../not-a-file-id" } }), + ]); + + expect(results.every(Option.isNone)).toBe(true); + expect(requestCount).toBe(0); + }), + ); + + it.effect("rejects non-image and untrusted download responses", () => + Effect.gen(function* () { + const nonImage = yield* recover( + httpClientLayer(() => + Response.json({ + ok: true, + file: { + id: FILE_ID, + name: "notes.txt", + mimetype: "text/plain", + size: 10, + url_private_download: "https://files.slack.com/files-pri/file", + }, + }), + ), + ); + const untrusted = yield* recover( + httpClientLayer(() => + Response.json({ + ok: true, + file: { + id: FILE_ID, + name: "screenshot.png", + mimetype: "image/png", + size: 10, + url_private_download: "https://example.com/screenshot.png", + }, + }), + ), + ); + let requestCount = 0; + const wrongResponseType = yield* recover( + httpClientLayer(() => { + requestCount += 1; + return requestCount === 1 + ? Response.json({ + ok: true, + file: { + id: FILE_ID, + name: "screenshot.png", + mimetype: "image/png", + size: 10, + url_private_download: "https://files.slack.com/files-pri/file", + }, + }) + : new Response("not an image", { + status: 200, + headers: { "content-type": "text/html" }, + }); + }), + ); + + expect(Option.isNone(nonImage)).toBe(true); + expect(Option.isNone(untrusted)).toBe(true); + expect(Option.isNone(wrongResponseType)).toBe(true); + }), + ); +}); diff --git a/packages/plugins/mcp/src/sdk/slack-connect-file.ts b/packages/plugins/mcp/src/sdk/slack-connect-file.ts new file mode 100644 index 0000000000..f7159569e9 --- /dev/null +++ b/packages/plugins/mcp/src/sdk/slack-connect-file.ts @@ -0,0 +1,153 @@ +import { Effect, Encoding, Option, Schema } from "effect"; +import { HttpClient, HttpClientRequest } from "effect/unstable/http"; + +const SLACK_MCP_HOST = "mcp.slack.com"; +const SLACK_MCP_PATH = "/mcp"; +const SLACK_FILE_INFO_URL = "https://slack.com/api/files.info"; +const SLACK_FILE_HOST = "files.slack.com"; +const MAX_SLACK_FILE_BYTES = 10 * 1024 * 1024; + +const SlackReadFileArgs = Schema.Struct({ file_id: Schema.String }); +const decodeSlackReadFileArgs = Schema.decodeUnknownOption(SlackReadFileArgs); + +const SlackFileInfo = Schema.Struct({ + id: Schema.String, + name: Schema.optional(Schema.String), + title: Schema.optional(Schema.String), + mimetype: Schema.String, + size: Schema.Number, + url_private: Schema.optional(Schema.String), + url_private_download: Schema.optional(Schema.String), +}); + +const SlackFileInfoSuccess = Schema.Struct({ + ok: Schema.Literal(true), + file: SlackFileInfo, +}); +const decodeSlackFileInfoSuccess = Schema.decodeUnknownOption(SlackFileInfoSuccess); + +const parseUrl = Option.liftThrowable((value: string) => new URL(value)); + +const isSlackMcpEndpoint = (value: string): boolean => + Option.match(parseUrl(value), { + onNone: () => false, + onSome: (url) => + url.protocol === "https:" && + url.hostname === SLACK_MCP_HOST && + url.pathname.replace(/\/$/, "") === SLACK_MCP_PATH, + }); + +const trustedSlackFileUrl = (value: string): URL | null => + Option.match(parseUrl(value), { + onNone: () => null, + onSome: (url) => (url.protocol === "https:" && url.hostname === SLACK_FILE_HOST ? url : null), + }); + +const bearerHeaders = (accessToken: string): Readonly> => ({ + Authorization: `Bearer ${accessToken}`, +}); + +const safeTitle = (value: string): string => value.replace(/[\r\n]/g, " "); + +type SlackImageToolResult = { + readonly content: readonly [ + { readonly type: "text"; readonly text: string }, + { readonly type: "image"; readonly data: string; readonly mimeType: string }, + ]; +}; + +interface RecoverSlackConnectFileInput { + readonly endpoint: string; + readonly toolName: string; + readonly args: unknown; + readonly accessToken: string | null; + readonly upstreamErrorMessage: string; +} + +/** + * Recovers Slack Connect images that Slack's hosted MCP server reports as + * `file_not_found` by resolving the file through Slack's Web API with the same + * user OAuth grant. Returns `None` for non-Slack calls and for any fallback + * failure so the caller can preserve the original upstream error. + */ +export const recoverSlackConnectFile = ( + input: RecoverSlackConnectFileInput, +): Effect.Effect, never, HttpClient.HttpClient> => { + if ( + input.toolName !== "slack_read_file" || + !input.upstreamErrorMessage.includes("file_not_found") || + !isSlackMcpEndpoint(input.endpoint) || + input.accessToken === null || + input.accessToken.length === 0 + ) { + return Effect.succeed(Option.none()); + } + const accessToken = input.accessToken; + + const args = Option.getOrUndefined(decodeSlackReadFileArgs(input.args)); + const fileId = args?.file_id.trim(); + if (fileId === undefined || !/^F[A-Z0-9]+$/.test(fileId)) { + return Effect.succeed(Option.none()); + } + + return Effect.gen(function* () { + const client = yield* HttpClient.HttpClient; + const infoUrl = new URL(SLACK_FILE_INFO_URL); + infoUrl.searchParams.set("file", fileId); + + const infoResponse = yield* client.execute( + HttpClientRequest.get(infoUrl, { headers: bearerHeaders(accessToken) }), + ); + if (infoResponse.status < 200 || infoResponse.status >= 300) return Option.none(); + + const info = Option.getOrUndefined(decodeSlackFileInfoSuccess(yield* infoResponse.json)); + if ( + info === undefined || + !info.file.mimetype.startsWith("image/") || + !Number.isSafeInteger(info.file.size) || + info.file.size < 0 || + info.file.size > MAX_SLACK_FILE_BYTES + ) { + return Option.none(); + } + + const downloadUrl = trustedSlackFileUrl( + info.file.url_private_download ?? info.file.url_private ?? "", + ); + if (downloadUrl === null) return Option.none(); + + const downloadResponse = yield* client.execute( + HttpClientRequest.get(downloadUrl, { headers: bearerHeaders(accessToken) }), + ); + if (downloadResponse.status < 200 || downloadResponse.status >= 300) return Option.none(); + const responseMimeType = downloadResponse.headers["content-type"] + ?.split(";", 1)[0] + ?.trim() + .toLowerCase(); + if (responseMimeType !== undefined && !responseMimeType.startsWith("image/")) { + return Option.none(); + } + + const bytes = new Uint8Array(yield* downloadResponse.arrayBuffer); + if (bytes.byteLength > MAX_SLACK_FILE_BYTES) return Option.none(); + + const title = safeTitle(info.file.title ?? info.file.name ?? info.file.id); + const recovered: SlackImageToolResult = { + content: [ + { + type: "text", + text: `File ID: ${info.file.id}\nTitle: ${title}\nMIME Type: ${info.file.mimetype}\nSize: ${info.file.size} bytes\n`, + }, + { + type: "image", + data: Encoding.encodeBase64(bytes), + mimeType: info.file.mimetype, + }, + ], + }; + return Option.some(recovered); + }).pipe( + Effect.catch(() => Effect.succeed(Option.none())), + Effect.withSpan("plugin.mcp.slack_connect_file_fallback"), + ); +}; diff --git a/packages/react/src/components/accounts-section.tsx b/packages/react/src/components/accounts-section.tsx index dddbfbe805..f0ec361dab 100644 --- a/packages/react/src/components/accounts-section.tsx +++ b/packages/react/src/components/accounts-section.tsx @@ -279,6 +279,11 @@ function OwnerAccounts(props: { } const payload = oauthReconnectPayload(connection); if (payload === null) return; + // Claim the sign-in window on the click: `oauth.start` below is a network + // round trip, and the browser's user activation can expire before it + // answers, which would leave Reconnect silently doing nothing. + const reservation = oauthPopup.reserve(); + if (reservation.kind === "blocked") return; // `oauth.start` discriminates the grant: client_credentials mints inline // (`status: "connected"`, no authorization URL) while authorization_code // returns a redirect the popup must complete. The popup hook only handles @@ -291,6 +296,7 @@ function OwnerAccounts(props: { reactivityKeys: connectionWriteKeys, }); if (Exit.isFailure(startExit)) { + oauthPopup.releaseReservation(); toast.error(messageFromExit(startExit, "Failed to reconnect")); trackEvent("connection_reconnected", { integration_slug: String(connection.integration), @@ -301,6 +307,7 @@ function OwnerAccounts(props: { } const started = startExit.value; if (started.status === "connected") { + oauthPopup.releaseReservation(); toast.success("Reconnected"); trackEvent("connection_reconnected", { integration_slug: String(connection.integration), @@ -311,6 +318,7 @@ function OwnerAccounts(props: { } void oauthPopup.openAuthorization({ owner: payload.owner, + reservation, run: () => Promise.resolve({ state: started.state, diff --git a/packages/react/src/components/add-account-modal.test.ts b/packages/react/src/components/add-account-modal.test.ts index acc5952691..b95a3b2aa4 100644 --- a/packages/react/src/components/add-account-modal.test.ts +++ b/packages/react/src/components/add-account-modal.test.ts @@ -9,6 +9,7 @@ import { } from "@executor-js/sdk/shared"; import type { AuthMethod } from "../lib/auth-placements"; +import type { OAuthPopupReservation } from "../plugins/oauth-sign-in"; import { connectionNameFrom, connectionLabel, @@ -57,7 +58,35 @@ type RegisterArgs = { readonly originIntegration?: IntegrationSlug; }; -type StartArgs = { readonly client: OAuthClientSlug; readonly owner: Owner }; +type StartArgs = { + readonly client: OAuthClientSlug; + readonly owner: Owner; + readonly reservation: OAuthPopupReservation; +}; + +// A reservation the browser honoured. The desktop shape is the one that carries +// no `Window`, so the orchestrators' ordering is testable without a DOM. +const RESERVED: OAuthPopupReservation = { + kind: "desktop", + bridge: { openExternal: (): Promise => Promise.resolve() }, +}; + +/** Records the reserve/release calls alongside the network steps, so a test can + * assert the window is claimed BEFORE the first round trip and closed again + * whenever the flow ends without signing in. */ +const popupSpy = (reservation: OAuthPopupReservation = RESERVED) => { + const calls: string[] = []; + return { + calls, + reserve: (): OAuthPopupReservation => { + calls.push("reserve"); + return reservation; + }, + release: (): void => { + calls.push("release"); + }, + }; +}; type CimdCreateArgs = { readonly owner: Owner; @@ -70,7 +99,11 @@ type CimdCreateArgs = { readonly clientSecret: ""; }; -type CimdStartArgs = { readonly client: OAuthClientSlug; readonly owner: Owner }; +type CimdStartArgs = { + readonly client: OAuthClientSlug; + readonly owner: Owner; + readonly reservation: OAuthPopupReservation; +}; const TEST_INTEGRATION = IntegrationSlug.make("linear_mcp"); @@ -310,6 +343,8 @@ describe("runCimdConnect", () => { const outcome = await runCimdConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, createClient: (args: CimdCreateArgs): Promise => { createArgs = args; return Promise.resolve(args.slug); @@ -352,6 +387,8 @@ describe("runCimdConnect", () => { const outcome = await runCimdConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, createClient: (): Promise => { created = true; return Promise.resolve(OAuthClientSlug.make("new-client")); @@ -383,7 +420,221 @@ describe("runCimdConnect", () => { expect(outcome).toEqual({ kind: "started", client: existingSlug, reused: true }); expect(created).toBe(false); - expect(startArgs).toEqual({ client: existingSlug, owner: "user" }); + expect(startArgs).toEqual({ client: existingSlug, owner: "user", reservation: RESERVED }); + }); +}); + +describe("runDcrConnect popup reservation", () => { + const probeOk = (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + registrationEndpoint: "https://auth.example.com/register", + }); + const dcrInput = { + discoveryUrl: "https://mcp.example.com/mcp", + owner: "user" as Owner, + integration: TEST_INTEGRATION, + }; + + // The regression: probe and register are two round trips, and a browser stops + // honouring `window.open` about five seconds after the click. Opening the + // window after them is refused, so the connect ends with no popup, no error, + // and the button back on "Connect". The window has to be claimed up front. + it("claims the sign-in window BEFORE the first network call", async () => { + const popup = popupSpy(); + const outcome = await runDcrConnect( + { + ...popup, + probe: (): Promise => { + popup.calls.push("probe"); + return probeOk(); + }, + register: (): Promise => { + popup.calls.push("register"); + return Promise.resolve(OAuthClientSlug.make("mcp-app")); + }, + start: (): void => { + popup.calls.push("start"); + }, + }, + dcrInput, + ); + + expect(outcome).toEqual({ kind: "started" }); + expect(popup.calls).toEqual(["reserve", "probe", "register", "start"]); + }); + + it("hands the reserved window to start, so the flow navigates it rather than opening a new one", async () => { + let startArgs: StartArgs | null = null; + const outcome = await runDcrConnect( + { + ...popupSpy(), + probe: probeOk, + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (args: StartArgs): void => { + startArgs = args; + }, + }, + dcrInput, + ); + + expect(outcome).toEqual({ kind: "started" }); + expect(startArgs!.reservation).toBe(RESERVED); + }); + + it("gives up before any network call when the browser refuses the window", async () => { + const popup = popupSpy({ kind: "blocked" }); + let probed = false; + const outcome = await runDcrConnect( + { + ...popup, + probe: (): Promise => { + probed = true; + return probeOk(); + }, + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (): void => {}, + }, + dcrInput, + ); + + // No BYO fallback: registering an app by hand cannot open a window either. + expect(outcome).toEqual({ kind: "popup-blocked" }); + expect(probed).toBe(false); + expect(popup.calls).toEqual(["reserve"]); + }); + + // Reserving early means a flow that ends without signing in is holding an + // about:blank window the user never asked for. + it("closes the reserved window when the probe fails", async () => { + const popup = popupSpy(); + const outcome = await runDcrConnect( + { + ...popup, + probe: (): Promise => Promise.resolve(null), + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (): void => {}, + }, + dcrInput, + ); + + expect(outcome).toEqual({ kind: "fallback", reason: "probe-failed" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + it("closes the reserved window when the server advertises no registration endpoint", async () => { + const popup = popupSpy(); + const outcome = await runDcrConnect( + { + ...popup, + probe: (): Promise => + Promise.resolve({ + authorizationUrl: "https://auth.example.com/authorize", + tokenUrl: "https://auth.example.com/token", + }), + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (): void => {}, + }, + dcrInput, + ); + + expect(outcome.kind).toBe("fallback"); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + it("closes the reserved window when registration is rejected", async () => { + const popup = popupSpy(); + const outcome = await runDcrConnect( + { + ...popup, + probe: probeOk, + register: (): Promise<{ readonly error: string }> => + Promise.resolve({ error: "redirect_uri not allowed" }), + start: (): void => {}, + }, + dcrInput, + ); + + expect(outcome).toMatchObject({ reason: "registration-failed" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + it("keeps the reserved window open on the path that signs in", async () => { + const popup = popupSpy(); + await runDcrConnect( + { + ...popup, + probe: probeOk, + register: (): Promise => Promise.resolve(OAuthClientSlug.make("mcp-app")), + start: (): void => {}, + }, + dcrInput, + ); + + expect(popup.calls).not.toContain("release"); + }); +}); + +describe("runCimdConnect popup reservation", () => { + const cimdInput = { + owner: "user" as Owner, + integrationName: "PostHog API", + authorizationUrl: "https://us.posthog.com/oauth/authorize/", + tokenUrl: "https://us.posthog.com/oauth/token/", + resource: null, + clientIdMetadataDocumentUrl: "https://executor.example/api/oauth/client-id-metadata.json", + existingClients: [], + }; + + it("claims the sign-in window before minting the client", async () => { + const popup = popupSpy(); + const outcome = await runCimdConnect( + { + ...popup, + createClient: (args: CimdCreateArgs): Promise => { + popup.calls.push("createClient"); + return Promise.resolve(args.slug); + }, + start: (): void => { + popup.calls.push("start"); + }, + }, + cimdInput, + ); + + expect(outcome.kind).toBe("started"); + expect(popup.calls).toEqual(["reserve", "createClient", "start"]); + }); + + it("closes the reserved window when minting the client fails", async () => { + const popup = popupSpy(); + const outcome = await runCimdConnect( + { + ...popup, + createClient: (): Promise => Promise.resolve(null), + start: (): void => {}, + }, + cimdInput, + ); + + expect(outcome).toEqual({ kind: "failed", reason: "create-failed" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + + it("never claims a window when the method is missing its endpoints", async () => { + const popup = popupSpy(); + const outcome = await runCimdConnect( + { + ...popup, + createClient: (): Promise => Promise.resolve(null), + start: (): void => {}, + }, + { ...cimdInput, tokenUrl: " " }, + ); + + expect(outcome).toEqual({ kind: "failed", reason: "missing-endpoints" }); + expect(popup.calls).toEqual([]); }); }); @@ -416,7 +667,13 @@ describe("runDcrConnect", () => { }; const outcome = await runDcrConnect( - { probe, register, start }, + { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, + probe, + register, + start, + }, { discoveryUrl: "https://mcp.example.com/mcp", owner: "user", @@ -454,6 +711,8 @@ describe("runDcrConnect", () => { let registerArgs: RegisterArgs | null = null; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ authorizationUrl: "https://auth.example.com/authorize", @@ -490,6 +749,8 @@ describe("runDcrConnect", () => { let registerArgs: RegisterArgs | null = null; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ authorizationUrl: "https://auth.example.com/authorize", @@ -517,6 +778,8 @@ describe("runDcrConnect", () => { const calls: string[] = []; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => { calls.push("probe"); return Promise.resolve({ @@ -554,6 +817,8 @@ describe("runDcrConnect", () => { const calls: string[] = []; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => { calls.push("probe"); return Promise.resolve(null); @@ -580,6 +845,8 @@ describe("runDcrConnect", () => { const calls: string[] = []; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ authorizationUrl: "https://auth.example.com/authorize", @@ -620,6 +887,8 @@ describe("runDcrConnect", () => { let started = false; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ authorizationUrl: "https://auth.example.com/authorize", @@ -659,6 +928,8 @@ describe("runDcrConnect", () => { let registerArgs: RegisterArgs | null = null; const outcome = await runDcrConnect( { + reserve: (): OAuthPopupReservation => RESERVED, + release: (): void => {}, probe: (): Promise => Promise.resolve({ issuer: "https://auth.example.com", diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index 7376a39890..c2a7a5ac98 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -62,6 +62,7 @@ import { oauthClientIdMetadataDocumentUrl, useOAuthPopupFlow, type OAuthCompletionPayload, + type OAuthPopupReservation, } from "../plugins/oauth-sign-in"; import { clientDisplayName, @@ -638,7 +639,17 @@ type CimdCreateClientArgs = { type RunCimdConnectDeps = { readonly createClient: (args: CimdCreateClientArgs) => Promise; - readonly start: (args: { readonly client: OAuthClientSlug; readonly owner: Owner }) => void; + readonly start: (args: CimdStartArgs) => void; + /** Claim the sign-in window before any await. See `useOAuthPopupFlow.reserve`. */ + readonly reserve: () => OAuthPopupReservation; + /** Close a claimed window this flow turned out not to need. */ + readonly release: () => void; +}; + +type CimdStartArgs = { + readonly client: OAuthClientSlug; + readonly owner: Owner; + readonly reservation: OAuthPopupReservation; }; type RunCimdConnectInput = { @@ -653,6 +664,7 @@ type RunCimdConnectInput = { type CimdOutcome = | { readonly kind: "started"; readonly client: OAuthClientSlug; readonly reused: boolean } + | { readonly kind: "popup-blocked" } | { readonly kind: "failed"; readonly reason: "missing-endpoints" | "create-failed" }; export async function runCimdConnect( @@ -663,6 +675,12 @@ export async function runCimdConnect( return { kind: "failed", reason: "missing-endpoints" }; } + // Claim the window while the click still counts. Minting the client is a + // round trip, and the browser stops honouring `window.open` once the + // activation from the click expires. + const reservation = deps.reserve(); + if (reservation.kind === "blocked") return { kind: "popup-blocked" }; + const resource = input.resource ?? null; const existing = input.existingClients.find( (client) => @@ -675,7 +693,7 @@ export async function runCimdConnect( ); if (existing) { - deps.start({ client: existing.slug, owner: input.owner }); + deps.start({ client: existing.slug, owner: input.owner, reservation }); return { kind: "started", client: existing.slug, reused: true }; } @@ -693,8 +711,11 @@ export async function runCimdConnect( clientId: input.clientIdMetadataDocumentUrl, clientSecret: "", }); - if (created === null) return { kind: "failed", reason: "create-failed" }; - deps.start({ client: created, owner: input.owner }); + if (created === null) { + deps.release(); + return { kind: "failed", reason: "create-failed" }; + } + deps.start({ client: created, owner: input.owner, reservation }); return { kind: "started", client: created, reused: false }; } @@ -741,16 +762,20 @@ type DcrRegisterArgs = { type DcrStartArgs = { readonly client: OAuthClientSlug; readonly owner: Owner; + readonly reservation: OAuthPopupReservation; }; /** Outcome of the DCR orchestration. `"started"` means the OAuth flow handed - * off (the popup/inline start ran); `"fallback"` means we could not auto-set-up - * — a failed probe, no registration endpoint, or a failed registration — and - * the caller should fall back to the bring-your-own-app picker. A failed probe + * off (the popup/inline start ran); `"popup-blocked"` means the browser refused + * the sign-in window, which no app picker can fix, so the caller reports it + * rather than falling back; `"fallback"` means we could not auto-set-up (a + * failed probe, no registration endpoint, or a failed registration) and the + * caller should fall back to the bring-your-own-app picker. A failed probe * carries no probe result; the other two reasons always carry the probe that * seeds the picker. */ type DcrOutcome = | { readonly kind: "started" } + | { readonly kind: "popup-blocked" } | { readonly kind: "fallback"; readonly reason: "probe-failed" } | { readonly kind: "fallback"; @@ -774,6 +799,10 @@ type RunDcrConnectDeps = { ) => Promise; /** Start the OAuth flow with the minted client (popup / inline). */ readonly start: (args: DcrStartArgs) => void; + /** Claim the sign-in window before any await. See `useOAuthPopupFlow.reserve`. */ + readonly reserve: () => OAuthPopupReservation; + /** Close a claimed window this flow turned out not to need. */ + readonly release: () => void; }; type RunDcrConnectInput = { @@ -804,8 +833,9 @@ type RunDcrConnectInput = { const DCR_CLIENT_NAME = "Executor"; /** - * Run the transparent DCR connect sequence: probe → register → start. + * Run the transparent DCR connect sequence: reserve → probe → register → start. * + * - Popup refused → `{ kind: "popup-blocked" }` before any network call. * - Probe failure → `{ kind: "fallback", reason: "probe-failed" }` (caller shows BYO). * - No registration endpoint → `{ kind: "fallback", reason: "no-registration-endpoint", probe }`. * - Register rejected with a message → `{ kind: "fallback", reason: "registration-failed", probe, message }` @@ -817,10 +847,23 @@ export async function runDcrConnect( deps: RunDcrConnectDeps, input: RunDcrConnectInput, ): Promise { + // Claim the sign-in window FIRST, on the click that got us here. Probe and + // register are two network round trips; the browser's user activation expires + // well before they finish, so a window opened after them is refused and the + // connect dies with nothing on screen. + const reservation = deps.reserve(); + if (reservation.kind === "blocked") return { kind: "popup-blocked" }; + const probe = await deps.probe(input.discoveryUrl); - if (probe === null) return { kind: "fallback", reason: "probe-failed" }; + if (probe === null) { + deps.release(); + return { kind: "fallback", reason: "probe-failed" }; + } const registrationEndpoint = probe.registrationEndpoint; - if (!registrationEndpoint) return { kind: "fallback", reason: "no-registration-endpoint", probe }; + if (!registrationEndpoint) { + deps.release(); + return { kind: "fallback", reason: "no-registration-endpoint", probe }; + } const slug = optimisticDcrClientSlug(probe.issuer ?? registrationEndpoint); const scopes = registrationScopes(input.declaredScopes ?? [], probe.scopesSupported ?? []); @@ -838,13 +881,17 @@ export async function runDcrConnect( redirectUri: input.redirectUri, originIntegration: input.integration, }); - if (minted === null) return { kind: "fallback", reason: "registration-failed", probe }; + if (minted === null) { + deps.release(); + return { kind: "fallback", reason: "registration-failed", probe }; + } // OAuthClientSlug is a branded string; an object return carries the failure // message (e.g. the server rejected the redirect URI) for the BYO fallback. if (typeof minted === "object") { + deps.release(); return { kind: "fallback", reason: "registration-failed", probe, message: minted.error }; } - deps.start({ client: minted, owner: input.owner }); + deps.start({ client: minted, owner: input.owner, reservation }); return { kind: "started" }; } @@ -2083,6 +2130,8 @@ function AddAccountModalView(props: AddAccountModalProps) { setCimdBusy(true); const outcome = await runCimdConnect( { + reserve: oauthPopup.reserve, + release: oauthPopup.releaseReservation, createClient: async (args: CimdCreateClientArgs): Promise => { const exit = await doCreateOAuthClient({ payload: { @@ -2100,8 +2149,9 @@ function AddAccountModalView(props: AddAccountModalProps) { if (Exit.isFailure(exit)) return null; return exit.value.client; }, - start: (args: { readonly client: OAuthClientSlug; readonly owner: Owner }): void => { + start: (args: CimdStartArgs): void => { void oauthPopup.start({ + reservation: args.reservation, payload: { client: args.client, // CIMD creates the public client under the connection owner, so @@ -2160,6 +2210,8 @@ function AddAccountModalView(props: AddAccountModalProps) { setDcrBusy(true); const outcome = await runDcrConnect( { + reserve: oauthPopup.reserve, + release: oauthPopup.releaseReservation, probe: async (url: string): Promise => { const exit = await doProbe({ payload: { url }, reactivityKeys: [] }); if (Exit.isFailure(exit)) return null; @@ -2194,6 +2246,7 @@ function AddAccountModalView(props: AddAccountModalProps) { }, start: (args: DcrStartArgs): void => { void oauthPopup.start({ + reservation: args.reservation, payload: { client: args.client, // DCR registers the client under the connection owner, so the app @@ -2236,6 +2289,10 @@ function AddAccountModalView(props: AddAccountModalProps) { success: outcome.kind === "started", ...(outcome.kind === "fallback" ? { dcr_fallback: true } : {}), }); + // Deliberately absent: a "popup-blocked" branch. Registering an app by hand + // does not make the browser open a window, so dropping to the BYO picker + // would send the user down a path that cannot succeed either. `reserve` + // already put the reason in `oauthPopup.error`, which the footer renders. if (outcome.kind === "fallback") { setOAuthFallbackProbe("probe" in outcome ? outcome.probe : null); setDcrFailed(true); @@ -2497,17 +2554,6 @@ function AddAccountModalView(props: AddAccountModalProps) { : `${integrationName} supports Client ID Metadata Document OAuth. We'll use this Executor host's public client metadata document and sign you in.`}

- ) : dcrActive ? ( - // Transparent DCR: no picker. We register an app for you and run - // the OAuth flow with a single Connect click. -
-

No app to choose

-

- {dcrConnecting - ? `Connecting to ${integrationName}…` - : `${integrationName} supports automatic setup. We register an app for you and sign you in — no client ID or app to pick.`} -

-
) : oauthLoading ? (

Loading OAuth apps…

) : ( @@ -2739,9 +2785,6 @@ function AddAccountModalView(props: AddAccountModalProps) { /> )} - {isOAuth && oauthPopup.error ? ( -

{oauthPopup.error}

- ) : null} )} @@ -2909,6 +2952,15 @@ function AddAccountModalView(props: AddAccountModalProps) { {continueError}

) : null} + {/* Above the footer, not inside the method tab: the automatic + (CIMD/DCR) flows render no tab panel at all, and putting the + sign-in error in there left a blocked popup with nothing on + screen but the button returning to "Connect". */} + {isOAuth && oauthPopup.error ? ( +

+ {oauthPopup.error} +

+ ) : null}