fix(workflows): wire markDirty hook through workflow execution path

[stack72]

Summary

• Wire markDirty through WorkflowExecutionService and DefaultStepExecutor: Both classes created their own repository instances (FileSystemUnifiedDataRepository, YamlOutputRepository, YamlEvaluatedDefinitionRepository) without the markDirty hook from repoContext. Workflow-executed model methods never notified the datastore sync service about dirty writes, while the same methods run via model method run (which uses repoContext's hooked repos) worked correctly.
• Fix buildMarkDirtyHook path computation for out-of-cache paths: When a repo writes to <repoDir>/.swamp/outputs/ but cacheRoot is ~/.swamp/repos/<id>/, relative(cacheRoot, absPath) produces ../... traversal. The hook now falls back to relative(repoSwampDir, absPath) when the result starts with .., since both trees share the same internal layout.
• Expose markDirty on RepositoryContext so all callers (workflow_run.ts, serve/deps.ts, serve/open/http.ts) can forward it when constructing WorkflowExecutionService. Nested child workflows also propagate the hook.

Test plan

• All 49 execution_service_test.ts tests pass
• All 35 repo_context_test.ts tests pass
• Full deno check passes with no type errors
• deno lint and deno fmt clean
• Binary compiles successfully
• Manual verification with a custom datastore (S3) to confirm workflow runs now trigger markDirty for data, outputs, and evaluated definitions

🤖 Generated with Claude Code

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

None.

Verdict

PASS — This PR makes no user-facing changes. All diffs are internal plumbing: wiring the markDirty hook through WorkflowExecutionService, DefaultStepExecutor, and the path-computation fallback in buildMarkDirtyHook. No help text, flags, error messages, log-mode output, or JSON output is affected. The only user-observable effect is that custom datastores now sync correctly after workflow runs, which is the intended behavior fix.

[github-actions]

Code Review

Clean, well-scoped bug fix that threads the markDirty hook through the workflow execution path so datastore sync notifications fire for workflow-executed model methods — not just direct model method run invocations.

Blocking Issues

None.

Suggestions

1. Missing test for the buildMarkDirtyHook .. fallback path (src/cli/repo_context.ts:158). The existing relPath wiring test uses a temp dir where cacheRoot and repoSwampDir coincide, so the rel.startsWith("..") branch is never exercised. A targeted unit test that constructs the hook with divergent cacheRoot and repoDir values, passes an absPath under the repo .swamp/ dir, and asserts the fallback produces a correct relative path would pin this logic. Not blocking because the fix is straightforward and the branch is a strict improvement over the prior behavior (which produced a ../... traversal path).

2. Growing positional parameter list on WorkflowExecutionService — the constructor now has 8 positional parameters, several of which are undefined at most callsites. An options-object pattern (e.g. opts: { executor?, dataBaseDir?, directTypeResolver?, markDirty? }) would make callsites self-documenting and reduce the risk of positional mix-ups. This is pre-existing and not introduced by this PR, so purely a future-improvement note.

DDD Notes

• MarkDirtyHook lives in the domain layer (src/domain/datastore/datastore_sync_service.ts) and is correctly imported as a type-only dependency — the domain defines the contract, infrastructure implements it. Good boundary placement.
• The DefaultStepExecutor and WorkflowExecutionService in the domain layer already had direct infrastructure imports (e.g. YamlOutputRepository, FileSystemUnifiedDataRepository) before this PR. Threading markDirty through the same path is consistent with the existing pattern, even if the broader coupling is something to revisit later.
• The buildMarkDirtyHook function correctly stays in the CLI/application layer (repo_context.ts), bridging the infrastructure sync service into the domain hook — proper layering.

Overall: focused fix, correct wiring at all callsites (including child workflows), no boundary violations introduced. LGTM.

[github-actions]

Adversarial Review

Critical / High

None found.

Medium

1. DefaultStepExecutor.fromRepoDir does not forward markDirty to the instance field (src/domain/workflows/execution_service.ts:275): fromRepoDir calls new DefaultStepExecutor(await buildDeps(repoDir, opts)) — the repos inside buildDeps correctly receive opts.markDirty, but the executor's this.markDirty private field is never set (only injectedDeps occupies the first constructor param). Today this is safe because resolveDeps short-circuits on this.injectedDeps and never reads this.markDirty. However, if a future change adds a code path that reads this.markDirty on an executor built via fromRepoDir, the hook will silently be absent. Suggested fix: forward opts.markDirty to the third constructor parameter:

   return new DefaultStepExecutor(
     await DefaultStepExecutor.buildDeps(repoDir, opts),
     undefined,
     opts.markDirty,
   );

   (fromRepoDir is not called anywhere today, so this is defensive — not a current bug.)

Low

1. buildMarkDirtyHook fallback doesn't guard double-traversal (src/cli/repo_context.ts:158-159): If absPath is outside both cacheRoot and repoSwampDir, relative(repoSwampDir, absPath) also produces a ..-prefixed string. The resulting relPath forwarded to syncService.markDirty({ relPath }) would be a traversal path like ../../tmp/foo, violating the contract that relPath is cache-relative (rule 5 in the DatastoreSyncService docs). In practice repos only write inside one of the two known trees, so the condition can't fire in normal operation. If you want belt-and-suspenders, a guard like if (rel.startsWith("..")) { return syncService.markDirty(); } (bulk fallback) on the second result would be fully safe.

2. 8 positional constructor parameters on WorkflowExecutionService (src/domain/workflows/execution_service.ts:1247-1255): Call sites like deps.ts:81 and http.ts:1599 now pass (wfRepo, rnRepo, dir, undefined, undefined, catalogStore, undefined, markDirty) — four undefined holes. A positional swap would silently compile if the swapped types happen to overlap (e.g., DirectTypeResolver and MarkDirtyHook are both ((…) => Promise<void>) | undefined). Not a bug today, but an options-object refactor would make future additions less fragile.

Verdict

PASS — The fix correctly wires markDirty through the workflow execution path. All four call sites (workflow_run.ts, deps.ts, http.ts, child-workflow in execution_service.ts) pass the hook in the right position. The buildMarkDirtyHook fallback for out-of-cache paths is sound for the supported write locations. The RepositoryContext interface and createRepositoryContext factory are updated consistently. No critical or high severity issues found.