From 95f6ffe89020f793f829b5b363850b384cb50359 Mon Sep 17 00:00:00 2001 From: stack72 Date: Thu, 26 Mar 2026 12:31:51 +0100 Subject: [PATCH 1/2] fix: correct CLI --driver flag priority in workflow execution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The design doc specifies driver resolution priority as: CLI > step > job > workflow > definition > raw But the implementation had CLI as lowest priority — it only applied when step, job, and workflow didn't specify a driver. This flips options.driver to first position in the ?? chain so the CLI flag overrides all other sources, matching the documented behavior. Also adds a cli parameter to resolveDriverConfig() to keep the resolution contract in sync with the design doc, and adds tests for CLI override behavior. --- src/domain/drivers/driver_resolution.ts | 4 +- src/domain/drivers/driver_resolution_test.ts | 59 +++++++++++++++++--- src/domain/workflows/execution_service.ts | 4 +- 3 files changed, 56 insertions(+), 11 deletions(-) diff --git a/src/domain/drivers/driver_resolution.ts b/src/domain/drivers/driver_resolution.ts index 0fbf17cb..808809c1 100644 --- a/src/domain/drivers/driver_resolution.ts +++ b/src/domain/drivers/driver_resolution.ts @@ -35,18 +35,20 @@ export interface ResolvedDriverConfig { /** * Resolves the effective driver and config using precedence: - * step > job > workflow > definition > "raw" + * cli > step > job > workflow > definition > "raw" * * The first non-undefined `driver` value wins. Its corresponding * `driverConfig` is used as-is (no merging across levels). */ export function resolveDriverConfig( + cli?: DriverSource, step?: DriverSource, job?: DriverSource, workflow?: DriverSource, definition?: DriverSource, ): ResolvedDriverConfig { const sources: (DriverSource | undefined)[] = [ + cli, step, job, workflow, diff --git a/src/domain/drivers/driver_resolution_test.ts b/src/domain/drivers/driver_resolution_test.ts index 0aa9ae49..1ea9c44b 100644 --- a/src/domain/drivers/driver_resolution_test.ts +++ b/src/domain/drivers/driver_resolution_test.ts @@ -20,35 +20,38 @@ import { assertEquals } from "@std/assert"; import { resolveDriverConfig } from "./driver_resolution.ts"; -Deno.test("resolveDriverConfig - defaults to raw when no sources", () => { +Deno.test("resolveDriverConfig: defaults to raw when no sources", () => { const result = resolveDriverConfig(); assertEquals(result.driver, "raw"); assertEquals(result.driverConfig, undefined); }); -Deno.test("resolveDriverConfig - defaults to raw when all sources undefined", () => { +Deno.test("resolveDriverConfig: defaults to raw when all sources undefined", () => { const result = resolveDriverConfig( undefined, undefined, undefined, undefined, + undefined, ); assertEquals(result.driver, "raw"); }); -Deno.test("resolveDriverConfig - definition driver wins over default", () => { +Deno.test("resolveDriverConfig: definition driver wins over default", () => { const result = resolveDriverConfig( undefined, undefined, undefined, + undefined, { driver: "docker", driverConfig: { image: "node:18" } }, ); assertEquals(result.driver, "docker"); assertEquals(result.driverConfig, { image: "node:18" }); }); -Deno.test("resolveDriverConfig - workflow driver wins over definition", () => { +Deno.test("resolveDriverConfig: workflow driver wins over definition", () => { const result = resolveDriverConfig( + undefined, undefined, undefined, { driver: "docker", driverConfig: { image: "deno:latest" } }, @@ -58,8 +61,9 @@ Deno.test("resolveDriverConfig - workflow driver wins over definition", () => { assertEquals(result.driverConfig, { image: "deno:latest" }); }); -Deno.test("resolveDriverConfig - job driver wins over workflow", () => { +Deno.test("resolveDriverConfig: job driver wins over workflow", () => { const result = resolveDriverConfig( + undefined, undefined, { driver: "raw" }, { driver: "docker" }, @@ -68,8 +72,9 @@ Deno.test("resolveDriverConfig - job driver wins over workflow", () => { assertEquals(result.driver, "raw"); }); -Deno.test("resolveDriverConfig - step driver wins over all", () => { +Deno.test("resolveDriverConfig: step driver wins over job", () => { const result = resolveDriverConfig( + undefined, { driver: "docker", driverConfig: { image: "step-image" } }, { driver: "raw" }, { driver: "raw" }, @@ -79,8 +84,45 @@ Deno.test("resolveDriverConfig - step driver wins over all", () => { assertEquals(result.driverConfig, { image: "step-image" }); }); -Deno.test("resolveDriverConfig - skips sources without driver field", () => { +Deno.test("resolveDriverConfig: cli driver wins over all others", () => { + const result = resolveDriverConfig( + { driver: "raw" }, + { driver: "docker", driverConfig: { image: "step-image" } }, + { driver: "docker", driverConfig: { image: "job-image" } }, + { driver: "docker", driverConfig: { image: "wf-image" } }, + { driver: "docker", driverConfig: { image: "def-image" } }, + ); + assertEquals(result.driver, "raw"); + assertEquals(result.driverConfig, undefined); +}); + +Deno.test("resolveDriverConfig: cli driver config is used when cli wins", () => { const result = resolveDriverConfig( + { driver: "docker", driverConfig: { image: "cli-image" } }, + { driver: "raw" }, + { driver: "raw" }, + { driver: "raw" }, + { driver: "raw" }, + ); + assertEquals(result.driver, "docker"); + assertEquals(result.driverConfig, { image: "cli-image" }); +}); + +Deno.test("resolveDriverConfig: step wins when cli is undefined", () => { + const result = resolveDriverConfig( + undefined, + { driver: "docker", driverConfig: { image: "step-image" } }, + { driver: "raw" }, + { driver: "raw" }, + { driver: "raw" }, + ); + assertEquals(result.driver, "docker"); + assertEquals(result.driverConfig, { image: "step-image" }); +}); + +Deno.test("resolveDriverConfig: skips sources without driver field", () => { + const result = resolveDriverConfig( + undefined, { driverConfig: { ignore: true } }, undefined, { driver: "docker" }, @@ -89,8 +131,9 @@ Deno.test("resolveDriverConfig - skips sources without driver field", () => { assertEquals(result.driver, "docker"); }); -Deno.test("resolveDriverConfig - uses driverConfig from winning level only", () => { +Deno.test("resolveDriverConfig: uses driverConfig from winning level only", () => { const result = resolveDriverConfig( + undefined, undefined, { driver: "docker", driverConfig: { timeout: 30 } }, { driver: "raw", driverConfig: { verbose: true } }, diff --git a/src/domain/workflows/execution_service.ts b/src/domain/workflows/execution_service.ts index d67b1c68..d7ec7bdd 100644 --- a/src/domain/workflows/execution_service.ts +++ b/src/domain/workflows/execution_service.ts @@ -1460,8 +1460,8 @@ export class WorkflowExecutionService { workflowTags: options.workflowTags, runtimeTags: options.runtimeTags, secretRedactor: options.secretRedactor, - driver: step.driver ?? job.driver ?? workflow.driver ?? - options.driver, + driver: options.driver ?? step.driver ?? job.driver ?? + workflow.driver, driverConfig: step.driverConfig ?? job.driverConfig ?? workflow.driverConfig, emitEvent: push, From be22eb939c6d1d0d33d0ca6b656711bdcba0cd33 Mon Sep 17 00:00:00 2001 From: stack72 Date: Thu, 26 Mar 2026 12:46:37 +0100 Subject: [PATCH 2/2] fix: pair driver+config from winning level via resolveDriverConfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback: when CLI --driver wins, driverConfig must come from the same source (undefined for CLI) rather than leaking from a lower-priority level. Replaces the inline ?? chains in execution_service.ts with resolveDriverConfig() which correctly pairs driver and driverConfig from the winning resolution level. This also eliminates resolveDriverConfig as dead code — it is now used in the production execution path. --- src/domain/workflows/execution_service.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/domain/workflows/execution_service.ts b/src/domain/workflows/execution_service.ts index d7ec7bdd..7bdcd8d3 100644 --- a/src/domain/workflows/execution_service.ts +++ b/src/domain/workflows/execution_service.ts @@ -90,6 +90,7 @@ import { reportRegistry } from "../reports/report_registry.ts"; import type { MethodReportContext } from "../reports/report_context.ts"; import { modelRegistry } from "../models/model.ts"; import { getTracer, SpanStatusCode } from "../../infrastructure/tracing/mod.ts"; +import { resolveDriverConfig } from "../drivers/driver_resolution.ts"; /** * Context for step execution. */ @@ -1460,10 +1461,12 @@ export class WorkflowExecutionService { workflowTags: options.workflowTags, runtimeTags: options.runtimeTags, secretRedactor: options.secretRedactor, - driver: options.driver ?? step.driver ?? job.driver ?? - workflow.driver, - driverConfig: step.driverConfig ?? job.driverConfig ?? - workflow.driverConfig, + ...resolveDriverConfig( + { driver: options.driver }, + { driver: step.driver, driverConfig: step.driverConfig }, + { driver: job.driver, driverConfig: job.driverConfig }, + { driver: workflow.driver, driverConfig: workflow.driverConfig }, + ), emitEvent: push, reportFilterOptions: options.reportFilterOptions, };