fix(workflows): resolve runtime expressions in driverConfig (swamp-club#291)

[johnrwatson]

Summary

Workflow-level \${{ env.* }}, \${{ vault.* }}, and \${{ self.* }} (forEach) expressions in driverConfig fields were never resolved before being handed to the driver. Docker (and any out-of-process driver) received literal \${{ ... }} strings on argv and rejected them.

This PR adds a new ExpressionEvaluationService.resolveAllExpressionsInData operation that composes the existing CEL pass (evaluateData) and runtime pass (resolveRuntimeExpressionsInData), and wires it at two seams:

• Workflow-step path: after DriverPlan.withDefinition in DefaultStepExecutor.invokeMethod. Covers workflow/job/step driverConfig.
• Direct model-method path: after resolveDriverConfig in libswamp/models/run.ts. Load-bearing for repo-tier defaultDriverConfig from .swamp.yaml, which has never been runtime-resolved at any earlier point.

Vault values resolved in driverConfig are registered with the workflow's SecretRedactor for log scrubbing. They land as raw strings on the driver subprocess argv, which is documented in the manual reference (companion PR systeminit/swamp-club#527) — argv exposure to local-machine ps is a documented trade-off.

Test plan

• Unit tests: 6 new tests on resolveAllExpressionsInData covering literal no-op, env resolution, self.* via supplied context, nested arrays/objects walking, no-expression short-circuit, and env-only-no-secret-registration.
• Existing test mock for evaluationService updated to satisfy the new dependency.
• Full test suite: 5831 passed, 1 pre-existing failure (extension_quality_test.ts: missing README — exists on main without this PR).
• deno check, deno lint, deno fmt all clean.
• deno run compile succeeds.
• Manual repro against /tmp/swamp-repro-issue-291: before fix, docker rejected with "invalid characters for a local volume name"; after fix, docker accepts the resolved \${HOME}:/host-home:ro volume string and only fails on an unrelated docker daemon issue on the test host.
• End-to-end UAT coverage tracked in systeminit/swamp-uat#210 (deferred — no in-repo precedent for docker-conditional integration tests).

Companion changes

• Manual reference: systeminit/swamp-club#527 (adds driverConfig to supported expression sites + argv-visibility security caveat)
• UAT: systeminit/swamp-uat#210 (end-to-end coverage: env in volumes, self.* in env on forEach, vault in env with log redaction)

Closes swamp-club#291

🤖 Generated with Claude Code

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

None.

Verdict

PASS — This PR is a pure internal bug fix with no user-facing interface changes. No new flags, help text, error messages, or output formatting are added or modified. The UX improvement is behavioral: ${{ env.* }}, ${{ vault.* }}, and ${{ self.* }} expressions in driverConfig now resolve to their actual values instead of passing literal expression strings to drivers (e.g., Docker). This makes the existing documented behavior work as users expect.

[github-actions]

Adversarial Review

Critical / High

No critical or high severity findings.

Medium

1. src/domain/workflows/execution_service.ts:766 — Guard skips driverConfig resolution when expressionContext is undefined, even if driverConfig contains env.* / vault.* expressions.

   The guard is ctx.expressionContext && resolvedDriver.driverConfig. If expressionContext is undefined (which shouldn't happen in practice for the workflow path — it's always set in runStep at line 1803), the runtime expressions in driverConfig would pass through unresolved. This is defensive and matches the pre-existing pattern (the CEL evaluation block at line 489 also guards on ctx.expressionContext), and in practice expressionContext is always populated for workflow steps. However, a future caller constructing a StepExecutionContext manually (e.g. a test harness) without setting expressionContext would silently get unresolved ${{ env.* }} strings in driverConfig.

   Impact: Low in practice — the only callers go through runStep which always sets expressionContext. This is a documentation/contract gap, not a live bug.

2. src/domain/expressions/expression_evaluation_service_test.ts:280,325 — Tests mutate Deno.env without test isolation, creating potential flaky parallel test interactions.

   The tests set/delete environment variables (SWAMP_TEST_HOME, SWAMP_TEST_A, SWAMP_TEST_B, SWAMP_TEST_PUB) inside try/finally blocks. If Deno's test runner executes tests in parallel within the same process, two tests that both touch Deno.env could interfere. The SWAMP_TEST_ prefix and distinct names mitigate this, and this pattern is consistent with how other tests in this file handle env vars (e.g. the pre-existing tests). This follows established convention.

   Impact: Theoretical — the env var names are unique per test, and try/finally ensures cleanup even on failure.

Low

1. src/domain/expressions/expression_evaluation_service.ts:526 — resolveAllExpressionsInData does not short-circuit when data contains no expressions at all. Both evaluateData and resolveRuntimeExpressionsInData independently extract and check for expressions, so the double-walk is not semantically incorrect — each pass short-circuits internally when there are no relevant expressions. The overhead is two extractExpressions calls on expression-free data, which returns immediately on non-string leaf nodes. Not a performance concern in practice.

2. src/libswamp/models/run_test.ts:131 — The fake resolveRuntimeExpressionsInData mock is a passthrough. This correctly models the "no expressions" case for existing tests, but means the test suite doesn't verify that the direct model-method path actually resolves repo-tier driverConfig expressions. The PR notes this is covered by manual testing and deferred UAT — acceptable given the difficulty of mocking the full vault/env resolution chain in unit tests.

Verdict

PASS — The changes are well-scoped and correct. The new resolveAllExpressionsInData method is a clean two-pass composition that preserves the existing separation between CEL and runtime expression resolution. The guard conditions in both seams (execution_service.ts and run.ts) are consistent with existing patterns and correctly handle the "no driverConfig" case by skipping resolution entirely. The test coverage is solid for the new method itself. No logic errors, no security regressions beyond the documented argv-visibility trade-off, and no resource leaks.

[github-actions]

Code Review

Clean, well-scoped bug fix. The two-seam approach (workflow path via resolveAllExpressionsInData, direct path via resolveRuntimeExpressionsInData) correctly handles the different expression resolution needs of each path. The conditional guard ctx.expressionContext && resolvedDriver.driverConfig is the right fallback for legacy/test contexts.

Blocking Issues

None.

Suggestions

1. Verbose JSDoc on resolveAllExpressionsInData (expression_evaluation_service.ts:505-519): The 15-line JSDoc on a 2-line method body is heavier than needed — the method name, parameter types, and the two delegated method names already communicate the two-pass composition. That said, the surrounding file has 20 JSDoc blocks and similar density, so this is consistent with the existing style. A shorter comment would be cleaner but isn't blocking.

2. Inline comments in execution_service.ts:758-765 and run.ts:589-595: Both 7-8 line comment blocks explain the WHY well (idempotency reasoning, tier coverage, argv visibility caveat), which is valuable context for a subtle multi-tier resolution system. No change needed, but future readers would benefit from these being trimmed to ~3 lines each.

3. Test monkey-patching in expression_evaluation_service_test.ts:367-371: The redactor.addSecret = ... override works but is fragile if SecretRedactor internals change. A spy/stub from a test utility would be more robust, but the approach is fine for now and matches the test's intent (verifying no secret registration happens for env-only expressions).

All checks pass (Linux + Windows CI green), test coverage is solid (6 new unit tests covering literals, env, self.*, nested structures, short-circuit, and env-only-no-redactor), and the DDD structure is sound — composing existing domain service operations at the right architectural seams.