fix: correct CLI --driver flag priority in workflow execution

[stack72]

Summary

• Bug: The CLI --driver flag was resolved as lowest priority (fallback), but the design doc (design/execution-drivers.md) specifies it should be highest priority
• Fix: Flipped options.driver to first position in the ?? chain in execution_service.ts so CLI overrides step/job/workflow driver settings
• Contract: Added cli parameter to resolveDriverConfig() in driver_resolution.ts to keep the resolution function's contract in sync with the design doc

What was wrong

In src/domain/workflows/execution_service.ts (line 1463), the driver resolution was:

driver: step.driver ?? job.driver ?? workflow.driver ?? options.driver,

This made the CLI --driver flag a fallback — it only applied when step, job, and workflow didn't specify a driver.

What it should be (per design doc)

1. CLI --driver flag        ← highest priority
2. Workflow step driver:
3. Workflow job driver:
4. Workflow-level driver:
5. Model definition driver:
6. Default: raw             ← lowest priority

The fix

driver: options.driver ?? step.driver ?? job.driver ?? workflow.driver,

Also updated resolveDriverConfig() to accept a cli parameter as first in the resolution chain (cli > step > job > workflow > definition > raw), and added tests for CLI override behavior.

Impact

Users passing --driver on the CLI will now correctly override any driver configuration set in workflow YAML files. Previously, a workflow step with driver: docker would silently ignore --driver raw from the CLI.

Files changed

• src/domain/workflows/execution_service.ts — flip ?? chain priority
• src/domain/drivers/driver_resolution.ts — add cli parameter to resolution function
• src/domain/drivers/driver_resolution_test.ts — update existing tests + add CLI override tests

Test plan

• All 11 driver resolution tests pass (including 3 new CLI override tests)
• Full test suite passes (3560 tests, 0 failures)
• deno check — type checking clean
• deno lint — no warnings
• deno fmt — properly formatted

🤖 Generated with Claude Code

[github-actions]

Adversarial Review

Critical / High

1. HIGH — driverConfig not updated to match new driver priority in execution_service.ts:1463-1466

   The driver resolution was correctly reordered to options.driver ?? step.driver ?? job.driver ?? workflow.driver, but the driverConfig on the very next line was not updated:

   driver: options.driver ?? step.driver ?? job.driver ?? workflow.driver,
   driverConfig: step.driverConfig ?? job.driverConfig ?? workflow.driverConfig,

   Breaking scenario: A workflow step defines driver: docker with driverConfig: { image: "node:18" }. User runs with --driver raw. Result: driver: "raw" but driverConfig: { image: "node:18" } — a docker config applied to the raw driver. Before this PR, the mismatch was nearly impossible because options.driver was last priority and only won when no other level set a driver (and therefore likely no driverConfig either). Now that options.driver is first priority, this mismatch becomes the common case whenever CLI override is used.

   Suggested fix: When options.driver wins, driverConfig should be undefined (or from the same source). The cleanest fix is to use resolveDriverConfig() from driver_resolution.ts — which already exists and correctly pairs driver+config from the winning level — instead of the manual ?? chains. Alternatively, add driverConfig to StepOptions and thread CLI driver config through, then apply the same priority order.

2. HIGH — resolveDriverConfig() is never used in production code

   The PR adds a cli parameter to resolveDriverConfig() in driver_resolution.ts and adds tests for it, but execution_service.ts (the actual production path) does NOT call resolveDriverConfig() at all — it uses a hand-rolled ?? chain. The function is exported from mod.ts but has zero production callers. This means:

   • The well-tested resolution function with correct driver+config pairing is dead code
   • The actual production path at execution_service.ts:1463-1466 has the driver/config mismatch bug described above
   • The new tests give false confidence — they test a function that isn't used where it matters

   Suggested fix: Refactor execution_service.ts to call resolveDriverConfig() instead of the manual ?? chains. This would fix finding #1 and eliminate the dead code problem.

Medium

1. MEDIUM — execution_service.ts:464-465 has the same split-resolution pattern

   Line 464-465 also resolves driver and driverConfig independently:

   driver: ctx.driver ?? evaluatedDefinition.driver,
   driverConfig: ctx.driverConfig ?? evaluatedDefinition.driverConfig,

   This is a pre-existing issue (not introduced by this PR) but is the same anti-pattern. If ctx.driver is set but ctx.driverConfig is not, the config from evaluatedDefinition leaks through for a different driver. Worth noting since this PR is fixing driver resolution.

Low

None.

Verdict

FAIL — The driverConfig resolution in execution_service.ts was not updated to match the new driver priority, creating a driver/config mismatch whenever --driver CLI flag overrides a workflow-level driver that has config. The resolveDriverConfig() function (which handles this correctly) is dead code.

[github-actions]

Code Review

Blocking Issues

1. driverConfig not updated to match driver priority (execution_service.ts:1465): The driver chain was correctly reordered to options.driver ?? step.driver ?? job.driver ?? workflow.driver, but the driverConfig chain on the very next line is still step.driverConfig ?? job.driverConfig ?? workflow.driverConfig. This means when CLI --driver raw wins, the driverConfig will still come from a lower-priority source (e.g., a Docker step's { image: "node:18" }), producing a driver/config mismatch. The design doc states: "The first non-undefined driver value wins. Its corresponding driverConfig is used as-is — configs are not merged across levels."

   The cleanest fix would be to use the already-updated resolveDriverConfig() function here instead of inline ?? chains. It correctly pairs driver with its matching config from the winning source, and it's already exported from src/domain/drivers/mod.ts but currently unused in production code. Something like:

   const resolved = resolveDriverConfig(
     { driver: options.driver },
     { driver: step.driver, driverConfig: step.driverConfig },
     { driver: job.driver, driverConfig: job.driverConfig },
     { driver: workflow.driver, driverConfig: workflow.driverConfig },
   );
   // then use resolved.driver and resolved.driverConfig

Suggestions

1. resolveDriverConfig() is dead code in production: The function is exported from mod.ts and well-tested, but no production code calls it. If the blocking issue above is fixed by using it in execution_service.ts, this is resolved automatically.

2. Test name change on line 75: The rename from "step driver wins over all" to "step driver wins over job" is more precise — nice improvement. Just noting it accurately reflects the new semantics where CLI is now the highest priority.

Overall this is a well-motivated bug fix with good test coverage for the resolveDriverConfig function itself. The remaining gap is in wiring it into the actual execution path.

[github-actions]

Code Review

This is a clean, well-scoped bug fix that correctly aligns the CLI --driver flag priority with the design doc (design/execution-drivers.md). The change is minimal and focused.

Blocking Issues

None.

Suggestions

1. Missing blank line before doc comment (line 93-94 in execution_service.ts): The new import { resolveDriverConfig } line is immediately followed by a /** doc comment with no separating blank line. Not a formatting error per se (deno fmt passed), but slightly inconsistent with the surrounding style.

2. definition source not passed at call site: The resolveDriverConfig call in execution_service.ts:1464 passes 4 sources (cli, step, job, workflow) but omits the definition source. The old code also didn't include it — it was resolved elsewhere at line 466 (ctx.driverConfig ?? evaluatedDefinition.driverConfig). This is a pre-existing gap, not a regression. Worth a follow-up to unify definition-level resolution through resolveDriverConfig so all 6 levels from the design doc go through one path.

What looks good

• Fix correctly matches the design doc priority order
• resolveDriverConfig is a proper stateless domain service — correct DDD building block
• Test coverage is thorough: 11 tests covering every precedence level including 3 new CLI override tests
• Test naming follows project conventions (functionName: describes behavior)
• License headers present, import paths are domain-internal (no libswamp boundary violation)
• No security concerns

[github-actions]

Adversarial Review

Critical / High

None.

Medium

1. Undocumented behavioral change in driverConfig resolution (execution_service.ts:1464-1469)

   The old code resolved driver and driverConfig independently:

   driver: step.driver ?? job.driver ?? workflow.driver ?? options.driver,
   driverConfig: step.driverConfig ?? job.driverConfig ?? workflow.driverConfig,

   The new code couples them via resolveDriverConfig — driverConfig always comes from the same level as the winning driver.

   Breaking scenario: A workflow where a step sets driver: docker but no driverConfig, and the job sets driverConfig: { image: "node:18" }. Old behavior: step's driver + job's driverConfig. New behavior: step's driver + undefined driverConfig (falling through to evaluatedDefinition.driverConfig at line 466).

   This is arguably the correct behavior per the design doc ("no merging across levels"), but it's a silent behavioral change beyond the stated priority fix that could affect existing workflows. Worth calling out in the PR description.

2. Missing definition in call site (execution_service.ts:1464-1469)

   resolveDriverConfig accepts a definition parameter but the call site doesn't pass it — the definition-level driver/driverConfig is handled separately downstream at line 465-466 with independent ?? chains:

   driver: ctx.driver ?? evaluatedDefinition.driver,
   driverConfig: ctx.driverConfig ?? evaluatedDefinition.driverConfig,

   This means the "no merging across levels" guarantee of resolveDriverConfig is circumvented for the definition level: if CLI wins with driver: docker (no driverConfig), ctx.driverConfig is undefined, so evaluatedDefinition.driverConfig is used — mixing CLI driver with definition-level config. Not necessarily wrong, but it's inconsistent with the resolution function's stated contract.

Low

1. Missing blank line between import and JSDoc (execution_service.ts:93-94) — The new import at line 93 butts directly against the JSDoc comment at line 94. Style-only, not functional.

Verdict

PASS — The core fix is correct: CLI --driver now takes highest priority as specified in the design doc. The resolveDriverConfig function is clean and well-tested. The medium findings are about secondary behavioral changes that align with the design intent but should be documented.