Code Review

Blocking Issues

None.

Suggestions

1. Edge case documentation: When workflowRunId is set on the context, data items that lack a workflowRunId tag (e.g., written outside any workflow) are silently filtered out by findBySpec. This is clearly the intended behavior per the PR description and design docs, but it might be worth a brief inline comment noting this implication for future readers — something like "data without a workflowRunId tag is also excluded when scoped to a run."

Overall this is a clean, well-scoped fix. The filtering logic is minimal and correctly placed in the resolver. Tests cover both scoped and unscoped paths, including run switching. Design docs are clear about the intentional asymmetry between findBySpec (run-scoped) and findByTag (global). DDD principles are respected — the orchestration layer sets the scope, the resolver applies it, and no new abstractions are introduced beyond what's needed.

Adversarial Review

Critical / High

None found.

Medium

1. Missing test for data without workflowRunId tag when context has workflowRunId set (model_resolver_test.ts): The second test ("returns all data when workflowRunId is not set") creates data items both with and without a workflowRunId tag, but never sets ctx.workflowRunId on the context. There is no test that verifies what happens when ctx.workflowRunId is set but a data item lacks the workflowRunId tag entirely. In the current implementation (model_resolver.ts:632), versionData.tags["workflowRunId"] would be undefined, so undefined !== "run-1" is true, and the item gets filtered out. This is likely the desired behavior (data written outside the current run is excluded), but there's no test asserting it explicitly. If someone later changed the filter to e.g. only skip when the tag exists AND mismatches, they wouldn't break any test. Consider adding a case where workflowRunId is set on context and one data item has no workflowRunId tag — assert that item is excluded.

Low

1. findBySpec reads context.workflowRunId via closure over a mutable object (model_resolver.ts:614): The findBySpec closure reads context.workflowRunId from the shared context object reference. This works correctly because buildContext() returns the same object, and execution_service.ts:1146 mutates it before execution begins. However, if buildContext() were ever refactored to return a shallow copy or frozen object, this mutation pattern would silently break (the context would have workflowRunId set but the closure would read from the original). This is a coupling that's fine today but worth noting.

Verdict

PASS — The implementation is clean and correct. The findBySpec run-scoping logic is simple, the mutation of expressionContext.workflowRunId correctly propagates into the closure, findByTag is intentionally left globally scoped, and the workflowRunId tag is consistently written via StepExecutionContext. The only suggestion is a minor test gap (Medium #1) to lock in the filtering behavior for data items that lack the workflowRunId tag.