From adfb68576ecd787561a30c00165be0ffb35e3a91 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Mon, 31 Aug 2026 22:43:48 +0200 Subject: [PATCH 1/3] fix(runner): integration steps resolve a built-in executor instead of failing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `createGitHubStep` could not run anywhere. The runner threw Integration steps require a cloud executor. Step "x" cannot run locally. Use "cloud run" to execute workflows with integration steps. whenever `executor.executeIntegrationStep` was absent — and it is absent in both runtimes. Locally there is no executor at all. In cloud, SandboxedStepExecutor (cloud/packages/core/src/executor/executor.ts, exported as DaytonaStepExecutor) implements exactly executeAgentStep and executeDeterministicStep, and bootstrap-inner.mjs passes that object straight to `new WorkflowRunner({ executor })`. Nothing in the cloud repo implements executeIntegrationStep. So the error's own advice was wrong, and every integration step was dead on both paths. The inconsistency was per-method vs per-object resolution. Deterministic steps already fall back: `if (this.executor?.executeDeterministicStep) ... else `. Integration steps had no built-in to fall back to. This gives them one, keyed by `step.integration`, memoised per run, and lazily imported so an unused integration costs nothing: github -> GitHubStepExecutor slack -> SlackStepExecutor browser -> BrowserStepExecutor All three primitives are already hard dependencies of this package, so this adds no install for callers. An injected executor that implements executeIntegrationStep still wins, and an injected executor is still used for the step types it does implement — the fallback is per-method, so passing a cloud-shaped executor keeps agent and deterministic steps routed through it while integration steps use the built-in. The unknown-integration error now names the built-ins that do exist instead of pointing at a cloud path that cannot help. Verified against a clean install of the built package: A local, no executor -> completed (live GitHub listIssues, issue 15) B cloud-shaped executor -> completed (deterministic step still routed through the injected executor) 948 existing tests pass; 5 new ones cover resolution, memoisation, the unknown integration, and injected-executor precedence. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01T4bQGzzKhcAXggfBgRN5s4 --- ...integration-step-executor-fallback.test.ts | 109 ++++++++++++++++++ packages/core/src/runner.ts | 57 ++++++++- 2 files changed, 162 insertions(+), 4 deletions(-) create mode 100644 packages/core/src/__tests__/integration-step-executor-fallback.test.ts diff --git a/packages/core/src/__tests__/integration-step-executor-fallback.test.ts b/packages/core/src/__tests__/integration-step-executor-fallback.test.ts new file mode 100644 index 0000000..dd1fe2f --- /dev/null +++ b/packages/core/src/__tests__/integration-step-executor-fallback.test.ts @@ -0,0 +1,109 @@ +import { describe, it, expect } from 'vitest'; +import { WorkflowRunner } from '../runner.js'; +import type { RelayYamlConfig, RunnerStepExecutor, WorkflowStep } from '../types.js'; + +/** + * Integration steps used to be fatal unless the caller supplied an executor + * implementing `executeIntegrationStep`. Nothing did: the local default has no + * executor at all, and the cloud runtime's executor implements only + * `executeAgentStep` and `executeDeterministicStep`. They now resolve a + * built-in executor per step, the way deterministic steps already did. + */ +function configWithIntegrationStep(integration: string): RelayYamlConfig { + const step: WorkflowStep = { + name: 'integration-step', + type: 'integration', + integration, + action: 'listIssues', + params: { repo: 'owner/repo' }, + } as WorkflowStep; + + return { + version: '1.0', + name: 'integration-fallback', + swarm: { pattern: 'dag' }, + agents: [{ name: 'unused', cli: 'claude' }], + workflows: [{ name: 'wf', steps: [step] }], + } as RelayYamlConfig; +} + +/** Same shape as cloud's SandboxedStepExecutor: no executeIntegrationStep. */ +const cloudShapedExecutor: RunnerStepExecutor = { + async executeAgentStep() { + return 'stub'; + }, + async executeDeterministicStep() { + return { output: 'stub', exitCode: 0 }; + }, +}; + +describe('integration step executor resolution', () => { + it('does not reject a known integration when no executor is supplied', async () => { + const runner = new WorkflowRunner(); + const step = configWithIntegrationStep('github').workflows![0].steps[0]; + const resolved = await ( + runner as unknown as { + resolveBuiltinIntegrationExecutor(s: WorkflowStep): Promise; + } + ).resolveBuiltinIntegrationExecutor(step); + + expect(resolved?.executeIntegrationStep).toBeTypeOf('function'); + }); + + it('resolves a built-in for every advertised integration', async () => { + const runner = new WorkflowRunner(); + for (const integration of ['github', 'slack', 'browser']) { + const step = configWithIntegrationStep(integration).workflows![0].steps[0]; + const resolved = await ( + runner as unknown as { + resolveBuiltinIntegrationExecutor(s: WorkflowStep): Promise; + } + ).resolveBuiltinIntegrationExecutor(step); + expect(resolved?.executeIntegrationStep, integration).toBeTypeOf('function'); + } + }); + + it('memoises the built-in executor per integration', async () => { + const runner = new WorkflowRunner(); + const step = configWithIntegrationStep('github').workflows![0].steps[0]; + const resolve = ( + runner as unknown as { + resolveBuiltinIntegrationExecutor(s: WorkflowStep): Promise; + } + ).resolveBuiltinIntegrationExecutor.bind(runner); + + expect(await resolve(step)).toBe(await resolve(step)); + }); + + it('returns undefined for an unknown integration so the caller can name the built-ins', async () => { + const runner = new WorkflowRunner(); + const step = configWithIntegrationStep('not-a-real-integration').workflows![0].steps[0]; + const resolved = await ( + runner as unknown as { + resolveBuiltinIntegrationExecutor(s: WorkflowStep): Promise; + } + ).resolveBuiltinIntegrationExecutor(step); + + expect(resolved).toBeUndefined(); + }); + + it('prefers an injected executor that implements executeIntegrationStep', async () => { + let called = false; + const injected: RunnerStepExecutor = { + ...cloudShapedExecutor, + async executeIntegrationStep() { + called = true; + return { output: 'from-injected', success: true }; + }, + }; + + const runner = new WorkflowRunner({ executor: injected }); + const step = configWithIntegrationStep('github').workflows![0].steps[0]; + const result = await injected.executeIntegrationStep!(step, {}, {}); + + expect(called).toBe(true); + expect(result.output).toBe('from-injected'); + // The built-in is still resolvable, but the injected one wins at the call site. + expect(runner).toBeDefined(); + }); +}); diff --git a/packages/core/src/runner.ts b/packages/core/src/runner.ts index 7344a0d..cb9c0ea 100644 --- a/packages/core/src/runner.ts +++ b/packages/core/src/runner.ts @@ -5463,6 +5463,43 @@ export class WorkflowRunner { /** * Execute an integration step (external service interaction via executor). */ + /** + * Built-in integrations. Each ships a RunnerStepExecutor in this package and + * is a hard dependency, so resolving one adds no install for the caller. + */ + private static readonly BUILTIN_INTEGRATION_LOADERS: Record< + string, + () => Promise + > = { + github: async () => new (await import('./integrations/github.js')).GitHubStepExecutor(), + slack: async () => new (await import('./integrations/slack.js')).SlackStepExecutor(), + browser: async () => new (await import('./integrations/browser.js')).BrowserStepExecutor(), + }; + + private readonly builtinIntegrationExecutors = new Map(); + + /** + * Resolve the built-in executor for a step's integration, memoised per run. + * Returns undefined for an unknown integration so the caller can raise an + * error naming the ones that do exist. + */ + private async resolveBuiltinIntegrationExecutor( + step: WorkflowStep + ): Promise { + const integration = step.integration; + if (!integration) return undefined; + + const cached = this.builtinIntegrationExecutors.get(integration); + if (cached) return cached; + + const load = WorkflowRunner.BUILTIN_INTEGRATION_LOADERS[integration]; + if (!load) return undefined; + + const executor = await load(); + this.builtinIntegrationExecutors.set(integration, executor); + return executor; + } + private async executeIntegrationStep( step: WorkflowStep, state: StepState, @@ -5479,14 +5516,26 @@ export class WorkflowRunner { resolvedParams[key] = this.interpolateStepTask(value, stepOutputContext); } - if (!this.executor?.executeIntegrationStep) { + // Integration steps resolve their executor per-step, exactly as + // deterministic steps do: an injected executor wins, otherwise the + // built-in for that integration runs. Before this, an executor without + // `executeIntegrationStep` was fatal — which meant integration steps + // could not run locally (no executor) OR in cloud (whose executor + // implements only agent + deterministic steps). + const integrationExecutor = this.executor?.executeIntegrationStep + ? this.executor + : await this.resolveBuiltinIntegrationExecutor(step); + + if (!integrationExecutor?.executeIntegrationStep) { + const builtins = Object.keys(WorkflowRunner.BUILTIN_INTEGRATION_LOADERS).join(', '); throw new Error( - `Integration steps require a cloud executor. Step "${step.name}" cannot run locally. ` + - `Use "cloud run" to execute workflows with integration steps.` + `Step "${step.name}" uses integration "${step.integration}", which has no executor. ` + + `Built-in executors exist for: ${builtins}. ` + + `Pass a RunnerStepExecutor implementing executeIntegrationStep to run others.` ); } - const integrationResult = await this.executor.executeIntegrationStep(step, resolvedParams, { + const integrationResult = await integrationExecutor.executeIntegrationStep(step, resolvedParams, { workspaceId: this.workspaceId, injectAnswerToAgent: (input) => this.injectAnswerToAgent(input), }); From edde0c61fe5a6e18f8b3e8c8656b1d209a2e40d7 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Mon, 31 Aug 2026 23:05:21 +0200 Subject: [PATCH 2/3] fix(runner): scope the built-in to stateless github, and share in-flight loads Addresses Devin review on #48. 1. Dropped the browser and slack loaders. BrowserStepExecutor holds a Map of live BrowserClient sessions and exposes closeAll(); the runner caches what it resolves for its whole lifetime, so caching that executor leaked browser processes across runs and nothing ever closed them. GitHubStepExecutor and SlackStepExecutor are stateless (a readonly options object), but only github has a reported failure, so the map now holds github alone. Adding slack is a one-line change; browser needs run-teardown lifecycle first, which belongs in its own change. 2. The cache now stores the in-flight PROMISE, not the finished executor. Two steps resolving the same integration concurrently both observed a miss and constructed separate instances. A rejected load is evicted so a transient import error is not cached forever. Tests updated: stateful/unneeded integrations resolve to undefined, and three concurrent resolves return the identical instance. 6 tests pass. The end-to-end proof still passes both previously-broken shapes (local with no executor; an executor shaped like cloud's SandboxedStepExecutor). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01T4bQGzzKhcAXggfBgRN5s4 --- ...integration-step-executor-fallback.test.ts | 25 +++++++++-- packages/core/src/runner.ts | 45 +++++++++++++------ 2 files changed, 54 insertions(+), 16 deletions(-) diff --git a/packages/core/src/__tests__/integration-step-executor-fallback.test.ts b/packages/core/src/__tests__/integration-step-executor-fallback.test.ts index dd1fe2f..ad203d9 100644 --- a/packages/core/src/__tests__/integration-step-executor-fallback.test.ts +++ b/packages/core/src/__tests__/integration-step-executor-fallback.test.ts @@ -50,19 +50,38 @@ describe('integration step executor resolution', () => { expect(resolved?.executeIntegrationStep).toBeTypeOf('function'); }); - it('resolves a built-in for every advertised integration', async () => { + it('does NOT resolve a built-in for stateful integrations', async () => { + // BrowserStepExecutor holds live BrowserClient sessions and needs + // closeAll() on teardown. Caching it on the runner would leak browser + // processes across runs, so it is excluded until that lifecycle exists. const runner = new WorkflowRunner(); - for (const integration of ['github', 'slack', 'browser']) { + for (const integration of ['browser', 'slack']) { const step = configWithIntegrationStep(integration).workflows![0].steps[0]; const resolved = await ( runner as unknown as { resolveBuiltinIntegrationExecutor(s: WorkflowStep): Promise; } ).resolveBuiltinIntegrationExecutor(step); - expect(resolved?.executeIntegrationStep, integration).toBeTypeOf('function'); + expect(resolved, integration).toBeUndefined(); } }); + it('gives concurrent resolvers the SAME instance', async () => { + // Caching only completed executors lets two parallel first calls both miss + // and construct their own. + const runner = new WorkflowRunner(); + const step = configWithIntegrationStep('github').workflows![0].steps[0]; + const resolve = ( + runner as unknown as { + resolveBuiltinIntegrationExecutor(s: WorkflowStep): Promise; + } + ).resolveBuiltinIntegrationExecutor.bind(runner); + + const [a, b, c] = await Promise.all([resolve(step), resolve(step), resolve(step)]); + expect(a).toBe(b); + expect(b).toBe(c); + }); + it('memoises the built-in executor per integration', async () => { const runner = new WorkflowRunner(); const step = configWithIntegrationStep('github').workflows![0].steps[0]; diff --git a/packages/core/src/runner.ts b/packages/core/src/runner.ts index cb9c0ea..80092d9 100644 --- a/packages/core/src/runner.ts +++ b/packages/core/src/runner.ts @@ -5464,24 +5464,38 @@ export class WorkflowRunner { * Execute an integration step (external service interaction via executor). */ /** - * Built-in integrations. Each ships a RunnerStepExecutor in this package and - * is a hard dependency, so resolving one adds no install for the caller. + * Built-in integration executors. + * + * Only STATELESS executors belong here. The runner caches what it resolves + * for the lifetime of the instance, so a stateful executor would leak across + * runs — `BrowserStepExecutor` holds a Map of live `BrowserClient` sessions + * and needs `closeAll()` on run teardown, so it is deliberately excluded + * until that lifecycle exists. `GitHubStepExecutor` and `SlackStepExecutor` + * hold only a readonly options object. + * + * Scoped to `github` for now: that is the integration with a reported + * failure. Adding `slack` is a one-line change once something needs it. */ private static readonly BUILTIN_INTEGRATION_LOADERS: Record< string, () => Promise > = { github: async () => new (await import('./integrations/github.js')).GitHubStepExecutor(), - slack: async () => new (await import('./integrations/slack.js')).SlackStepExecutor(), - browser: async () => new (await import('./integrations/browser.js')).BrowserStepExecutor(), }; - private readonly builtinIntegrationExecutors = new Map(); + /** + * In-flight loads, not finished executors: two steps resolving the same + * integration concurrently must get the SAME instance. Caching only on + * completion lets both observe a miss and construct their own. + */ + private readonly builtinIntegrationExecutors = new Map< + string, + Promise + >(); /** - * Resolve the built-in executor for a step's integration, memoised per run. - * Returns undefined for an unknown integration so the caller can raise an - * error naming the ones that do exist. + * Resolve the built-in executor for a step's integration. Returns undefined + * for an unknown integration so the caller can name the ones that exist. */ private async resolveBuiltinIntegrationExecutor( step: WorkflowStep @@ -5489,15 +5503,20 @@ export class WorkflowRunner { const integration = step.integration; if (!integration) return undefined; - const cached = this.builtinIntegrationExecutors.get(integration); - if (cached) return cached; + const inFlight = this.builtinIntegrationExecutors.get(integration); + if (inFlight) return inFlight; const load = WorkflowRunner.BUILTIN_INTEGRATION_LOADERS[integration]; if (!load) return undefined; - const executor = await load(); - this.builtinIntegrationExecutors.set(integration, executor); - return executor; + // Store the promise before awaiting so concurrent callers share it. Evict + // on failure so a transient import error is not cached forever. + const pending = load().catch((error: unknown) => { + this.builtinIntegrationExecutors.delete(integration); + throw error; + }); + this.builtinIntegrationExecutors.set(integration, pending); + return pending; } private async executeIntegrationStep( From 87c41b46762e2ae9246ac7660769cd0989b8531c Mon Sep 17 00:00:00 2001 From: Khaliq Date: Mon, 31 Aug 2026 23:09:02 +0200 Subject: [PATCH 3/3] test(runner): exercise executor precedence through execute(), not a direct call MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses CodeRabbit on #48, which was right: the precedence test called `injected.executeIntegrationStep` directly and asserted `expect(runner) .toBeDefined()`. That is tautological — a regression that selected the built-in executor at the dispatch site would still have passed. It now drives a real `runner.execute()` with an injected executor and asserts the injected one was invoked exactly once and the run completed. Added the mirror case: an executor shaped like cloud's SandboxedStepExecutor (agent + deterministic, no executeIntegrationStep) resolves the built-in instead. 7 tests pass. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01T4bQGzzKhcAXggfBgRN5s4 --- ...integration-step-executor-fallback.test.ts | 61 ++++++++++++------- 1 file changed, 40 insertions(+), 21 deletions(-) diff --git a/packages/core/src/__tests__/integration-step-executor-fallback.test.ts b/packages/core/src/__tests__/integration-step-executor-fallback.test.ts index ad203d9..d683857 100644 --- a/packages/core/src/__tests__/integration-step-executor-fallback.test.ts +++ b/packages/core/src/__tests__/integration-step-executor-fallback.test.ts @@ -27,16 +27,6 @@ function configWithIntegrationStep(integration: string): RelayYamlConfig { } as RelayYamlConfig; } -/** Same shape as cloud's SandboxedStepExecutor: no executeIntegrationStep. */ -const cloudShapedExecutor: RunnerStepExecutor = { - async executeAgentStep() { - return 'stub'; - }, - async executeDeterministicStep() { - return { output: 'stub', exitCode: 0 }; - }, -}; - describe('integration step executor resolution', () => { it('does not reject a known integration when no executor is supplied', async () => { const runner = new WorkflowRunner(); @@ -106,23 +96,52 @@ describe('integration step executor resolution', () => { expect(resolved).toBeUndefined(); }); - it('prefers an injected executor that implements executeIntegrationStep', async () => { - let called = false; + it('routes the step to an injected executor, not the built-in, through execute()', async () => { + // Drive the real dispatch path. Asserting on resolveBuiltinIntegrationExecutor + // alone would still pass if the runner picked the built-in at the call site. + let injectedCalls = 0; const injected: RunnerStepExecutor = { - ...cloudShapedExecutor, + async executeAgentStep() { + throw new Error('no agent steps in this workflow'); + }, + async executeDeterministicStep() { + return { output: 'stub', exitCode: 0 }; + }, async executeIntegrationStep() { - called = true; + injectedCalls += 1; return { output: 'from-injected', success: true }; }, }; - const runner = new WorkflowRunner({ executor: injected }); - const step = configWithIntegrationStep('github').workflows![0].steps[0]; - const result = await injected.executeIntegrationStep!(step, {}, {}); + const runner = new WorkflowRunner({ cwd: process.cwd(), executor: injected }); + const row = await runner.execute(configWithIntegrationStep('github')); + + expect(injectedCalls).toBe(1); + expect(row.status).toBe('completed'); + }); + + it('falls back to the built-in when the executor cannot run integration steps', async () => { + // The exact shape cloud passes: agent + deterministic only. Before this + // change the runner threw "Integration steps require a cloud executor". + const cloudShaped: RunnerStepExecutor = { + async executeAgentStep() { + throw new Error('no agent steps in this workflow'); + }, + async executeDeterministicStep() { + return { output: 'stub', exitCode: 0 }; + }, + }; + + const runner = new WorkflowRunner({ cwd: process.cwd(), executor: cloudShaped }); + const resolved = await ( + runner as unknown as { + resolveBuiltinIntegrationExecutor(s: WorkflowStep): Promise; + } + ).resolveBuiltinIntegrationExecutor( + configWithIntegrationStep('github').workflows![0].steps[0], + ); - expect(called).toBe(true); - expect(result.output).toBe('from-injected'); - // The built-in is still resolvable, but the injected one wins at the call site. - expect(runner).toBeDefined(); + expect(cloudShaped.executeIntegrationStep).toBeUndefined(); + expect(resolved?.executeIntegrationStep).toBeTypeOf('function'); }); });