CLI UX Review

Blocking

None.

Suggestions

1. Malformed .swamp.yaml error lacks file context (src/infrastructure/persistence/repo_marker_repository.ts, src/infrastructure/persistence/repo_marker_loader.ts): When .swamp.yaml has a YAML parse error, the user sees Method execution failed: <raw yaml parse error> (e.g. Method execution failed: unexpected end of the stream within a single-quoted scalar at line 1, column 19). The word "yaml" does appear, satisfying the integration test, but the user isn't told which file to fix. Wrapping the thrown error in repo_marker_repository.ts#read() or repo_marker_loader.ts with a message like Failed to parse .swamp.yaml: <original message> would make this immediately actionable. Not blocking because the run aborts correctly with non-zero exit and "yaml" surfaces in the message.

2. Behavioral change is silent at runtime (src/cli/commands/model_method_run.ts): swamp model method run now reads .swamp.yaml on every invocation and silently applies defaultDriver. Users who previously relied on the implicit raw fallback (because the direct path never read the marker) won't know their run switched drivers. A --verbose log line like using driver: docker (from .swamp.yaml defaultDriver) would surface this. Not blocking — the design docs cover it — but could save a confused support question.

Verdict

PASS — No UX-visible regressions. Changes are internal driver-resolution plumbing. Malformed-YAML abort is new but intentional and tested; the existing Method execution failed: wrapper propagates "yaml" in the message so users can diagnose the problem.

Adversarial Review

Critical / High

None found.

Medium

1. src/serve/open/http.ts:1819-1824 — buildRunDeps creates the repo-marker loader once per handleRun call, but the comment on WorkflowExecutionService.loadRepoMarker says "instance-scoped so a long-running serve process creates a fresh loader per request." Both serve paths (WS at connection.ts:262 and HTTP SSE at http.ts:1768) do construct deps per request, so this is currently correct. However, if buildRunDeps were ever lifted to the route-initialization scope (creating deps once for the server), the loader would cache the .swamp.yaml state from the first request forever. The current code is fine — this is a future-maintenance concern only.

2. src/domain/workflows/execution_service.ts:1869 — CLI tier omits driverConfig. The workflow path constructs the CLI tier as { driver: options.driver } with no driverConfig. This means a CLI user who specifies --driver docker cannot pass --driver-config at the workflow level. This is pre-existing behavior unchanged by this PR (the old code also only passed { driver: options.driver } to resolveDriverConfig), not a regression. Flagging for awareness.

Low

1. src/infrastructure/persistence/repo_marker_loader.ts:43 — rejected promises are cached. If markerRepo.read() rejects (e.g., malformed YAML), the rejected promise is permanently cached by the ??= operator. The PR documents this as intentional ("marker parse failures are typically permanent"). This is correct for the current use — malformed YAML won't self-heal — but if a transient I/O error (disk hiccup, NFS timeout) triggers a rejection, all subsequent calls will also reject without retry. In practice, transient I/O failures during a single CLI invocation are extremely unlikely, and the loader is scoped per-request, so this is theoretical.

2. src/domain/workflows/execution_service.ts:1168 — markerRepo field only used in constructor. The private readonly markerRepo = new RepoMarkerRepository() field is only referenced at line 1199 to pass into createRepoMarkerLoader. It could be a local variable inside the constructor. Not a bug, just a minor cleanup opportunity.

3. integration/driver_resolution_test.ts:106 — crypto.randomUUID() for model ID. The test generates a random UUID for each model definition. If the test ever needs to assert on the model ID, this would be non-deterministic. In the current tests, only modelName is asserted, so this is fine.

Verdict

PASS. The PR is well-structured and correct. The core change — deferring driver resolution to the step executor where the evaluated definition is in scope — is the right fix for the dead-code ctx.driver ?? evaluatedDefinition.driver pattern. The promise-memoization primitive is clean and well-tested. The six-tier resolveDriverConfig call in the step executor correctly mirrors the documented priority chain. No logic errors, no regressions, no security issues found.

Code Review

Clean, well-structured PR that correctly fixes the three issues (direct-path driver resolution, concurrent-read race, dead definition.driver tier). The approach of deferring resolution into DefaultStepExecutor.executeModelMethod (where evaluatedDefinition is in scope) is the right design — it eliminates the dead-code ctx.driver ?? evaluatedDefinition.driver pair and makes the priority chain honest.

Blocking Issues

None.

Suggestions

1. DriverTiers interface placement (execution_service.ts:113): This interface is semantically about driver resolution, not workflow execution. Co-locating it with DriverSource and ResolvedDriverConfig in src/domain/drivers/driver_resolution.ts would keep all resolution-related types together. Minor — works fine where it is.

2. as any in run_test.ts:425-426: The (deps as any).createExecutionService = ... pattern adds a new no-explicit-any lint override. The existing test file already has 9 such overrides so this follows the established pattern, but a typed partial-mock helper could reduce any usage across the file in a future cleanup.

DDD Assessment

• createRepoMarkerLoader correctly lives in src/infrastructure/persistence/ — it's a persistence-layer concern (promise-memoized file read).
• DriverTiers is a well-formed value object interface capturing unresolved tiers.
• modelMethodRun in src/libswamp/models/run.ts acts as an application service orchestrating domain objects — the driver resolution call sits at the right layer.
• Dependency direction is clean: infrastructure → domain, application → domain + infrastructure. No inversion.

Other Observations

• All new files include AGPLv3 license headers.
• The libswamp import boundary is respected — CLI and serve paths import from infrastructure, not from libswamp internals.
• Test coverage is thorough: unit tests for the concurrency primitive, propagation audit, four direct-path resolution scenarios, updated workflow-path assertions, and an integration smoke test.
• The buildRunDeps call in http.ts is per-request (inside handleRun), confirming the per-request loader scoping claim.
• The promise-memoize correctly caches both resolved values and null (absent file), with rejected reads staying cached since malformed YAML is permanent.