feat(drivers): repo-level defaultDriver in .swamp.yaml (swamp-club#159)#1219
feat(drivers): repo-level defaultDriver in .swamp.yaml (swamp-club#159)#1219
Conversation
…b#159) Adds optional defaultDriver and defaultDriverConfig fields to RepoMarkerData and wires them into resolveDriverConfig as a new 'repo' tier below the definition slot. New chain: cli > step > job > workflow > definition > repo > "raw" The repo tier lets repos declare a baseline driver once in .swamp.yaml without setting driver: on every workflow or passing --driver on every CLI invocation. Mirrors the existing RepoMarkerData.datastore pattern. WorkflowExecutionService loads the marker once per run() (memoized on the instance so nested workflow calls don't re-read) and passes the repo tier into resolveDriverConfig at the step-context build site. Scope: this covers workflow execution. Direct 'swamp model method run' invocations go through a separate code path that doesn't call resolveDriverConfig today and will still default to 'raw' when no CLI override is supplied. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
Code Review
Well-crafted PR. The new repo-level defaultDriver tier is cleanly integrated into the existing resolution chain, with good test coverage at every layer.
Blocking Issues
None.
Suggestions
-
Positional parameter growth on
resolveDriverConfig(src/domain/drivers/driver_resolution.ts:44-51): The function now has 6 optionalDriverSourceparameters, all the same type. Call sites likeexecution_service.ts:1843-1853are fragile — passingundefinedfordefinitionto get torepois easy to misread. A named-fields object ({ cli?, step?, job?, workflow?, definition?, repo? }) would make call sites self-documenting. Not for this PR, but worth considering as a follow-up since the parameter list is only going to grow with custom tiers. -
execute()doesn't forwarddriver(execution_service.ts:1389-1405): The convenience wrapper only forwards a subset ofrun()options. This is pre-existing, but the new tests highlight the gap — the "CLI driver overrides" test must userun()directly while the other two useexecute(). If more options get added over time, it might be simpler to haveexecute()accept the fullrun()options type.
What looks good
- Memoization pattern for the repo marker is solid — lazy load with
undefined/nullsentinel, loaded once per service instance, shared across nested workflows. - Test coverage is thorough: 5 unit cases for resolution precedence, 1 persistence roundtrip, and 3 service-level integration tests exercising the default-applied, CLI-override, and no-default-fallback paths.
- Design doc updated with the new tier and example
.swamp.yaml. - The
undefinedfordefinitionat the call site correctly preserves pre-existing behavior where that tier wasn't populated.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None found. The changes are minimal, well-scoped, and follow established patterns.
Medium
-
Race in
loadRepoMarker()memoization —src/domain/workflows/execution_service.ts:1169-1175If two concurrent
run()calls hitloadRepoMarker()before either completes, both seethis.repoMarker === undefined, both issue a file read, and both write the result. Since both reads return the same data this is functionally harmless — but it means the "read at most once" contract in the comment is aspirational, not guaranteed. In practice,WorkflowExecutionServiceinstances are typically short-lived and sequential, so this is unlikely to matter.Breaking scenario: Two concurrent
run()calls → two file reads instead of one. No wrong result, just wasted I/O.Fix (if desired): Store the promise itself:
this.markerPromise ??= this.markerRepo.read(RepoPath.create(this.repoDir))and await the stored promise on subsequent calls. -
definitiontier is alwaysundefinedat the call site —src/domain/workflows/execution_service.ts:1848The function signature documents a
definitiontier betweenworkflowandrepo, but the call site passesundefinedfor it. This is pre-existing (before this PR, only 4 args were passed, leavingdefinitionasundefinedby default), but the PR makes it more visible by passingundefinedexplicitly. Model-level driver settings from definitions are not respected during workflow execution — therepotier can't compensate for that gap.Not introduced by this PR; just surfaced.
Low
-
No runtime validation of
defaultDriver/defaultDriverConfigtypes —src/infrastructure/persistence/repo_marker_repository.ts:105parseYaml(content) as RepoMarkerDatais a type assertion without runtime validation. A.swamp.yamlwithdefaultDriver: 123(number instead of string) would survive parsing and reachresolveDriverConfig, wheresource.driver !== undefinedwould betruefor the number123, and it would be returned as the driver value — a string-typed field carrying a number. This is the pre-existing pattern for allRepoMarkerDatafields and not introduced by this PR.
Verdict
PASS — Clean, well-tested addition that follows existing patterns. The new repo tier slots into the resolution chain correctly, memoization is sound for practical usage, and test coverage is thorough (unit precedence tests + integration tests with real .swamp.yaml on disk). The medium findings are either pre-existing or cosmetic concurrency — nothing blocks merge.
Summary
Closes swamp-club#159. Adds a repo-level
defaultDriver(anddefaultDriverConfig) to.swamp.yamlso a repo can declare a baseline execution driver once, without touching every workflow or passing--driveron every CLI invocation.New resolution chain for workflow steps:
RepoMarkerDatagains two optional fields (defaultDriver,defaultDriverConfig) — same shape as the existingdatastorefield.resolveDriverConfiggains an optional 6threpotier, positioned belowdefinitionand above the"raw"fallback.WorkflowExecutionServiceloads.swamp.yamlonce perrun()(memoized on the instance so nested workflow calls don't re-read) and passes the repo tier intoresolveDriverConfigat the step-context build site.design/execution-drivers.mdupdated to document the new tier with an example.Scope: workflow execution only. Direct
swamp model method runinvocations go through a separate code path that doesn't callresolveDriverConfigtoday and will still default to"raw"when no CLI override is supplied. Flagged in the adversarial review as ADV-1; can be a follow-up if desired.Test Plan
deno check— passesdeno lint— passesdeno fmt --check— passesdeno run test— 4732 passed / 0 failed / 1 ignoreddeno run compile— produces a working binaryNew tests added:
driver_resolution_test.tscovering repo-tier precedence (wins over default, loses to workflow/job/step/cli, skipped whendriverundefined butdriverConfigset)repo_marker_repository_test.tswithdefaultDriver+defaultDriverConfigexecution_service_test.tswith a real.swamp.yamlon disk: default applied, CLI overrides, no default → raw