CLI UX Review

Blocking

None.

Suggestions

1. JSON mode silently drops env_var_warning — JsonWorkflowRunRenderer in workflow_run.ts:247 is a no-op for env_var_warning, while the equivalent JsonModelMethodRunRenderer in model_method_run.ts:177 emits the warning as a JSON object. Users running workflow run --json in scripts won't see the warning at all. Consider emitting a JSON warning object here for parity with direct model execution.

2. Minor log-mode inconsistency — workflow_run.ts:118 uses logger.warn(e.message) while model_method_run.ts:72 uses logger.warn("{message}", { message: e.message }). The message contains no placeholders so this is functionally identical, but keeping the template form would be consistent.

3. Tree renderer silently drops warning — workflow_run_tree/state.ts:472 handles env_var_warning by returning unchanged state (same as method_executing), so the interactive TUI shows no indication of the warning. This may be intentional since the tree focuses on step status, but it means the warning is only visible in log mode, not the default TTY experience.

Verdict

PASS — The core feature works correctly: env var warnings now appear during workflow step execution in log mode, matching the behavior of direct CLI execution. The JSON omission is a suggestion rather than a blocker since the warning is already present in log mode.

Adversarial Review

Critical / High

None.

Medium

None.

Low

1. src/presentation/renderers/workflow_run.ts:247 — JSON renderer silently drops env_var_warning: The JSON renderer handles env_var_warning as a no-op (() => {}). This means users consuming --json output will never see these warnings. This is consistent with how model_resolved and other informational events are treated in the JSON renderer (they're all no-ops), so it follows the existing pattern — but it's worth noting that a workflow consumer relying solely on JSON output won't get env var warnings. If this is intentional (warnings are presentation-only, not data), this is fine.

2. src/presentation/renderers/workflow_run_tree/state.ts:472-473 — tree UI silently ignores env_var_warning: The tree renderer's state reducer returns state unchanged for env_var_warning, meaning the TUI dashboard won't display any visual indication of env var usage. The warning only appears in the log renderer. This is a reasonable choice since the tree view has limited space, but users who only use the TUI won't see the warning.

3. src/domain/workflows/execution_service.ts:236 — ctx.emitEvent?.() optional chain: If emitEvent is undefined (i.e., the step executor is called without an event sink), the warning is silently skipped. This is consistent with all other ctx.emitEvent?.() calls in the same function (e.g., line 225), so the pattern is correct. Just noting that env var detection still runs even when nobody is listening.

Verdict

PASS — This is a clean, minimal change that adds env_var_warning plumbing through the event pipeline. The new event variant is handled exhaustively in all three renderers (enforced by the EventHandlers<E> mapped type), the mapEvent switch has a never exhaustiveness guard, the domain/client protocol types are structurally compatible, and the detectEnvVarUsageInDefinition function was already reviewed in #964. No logic errors, no security concerns, no resource leaks.

Code Review

Clean, well-scoped PR that extends the existing env_var_warning pattern from direct CLI execution to workflow step execution. The implementation is consistent with the established pattern in src/libswamp/models/run.ts and src/presentation/renderers/model_method_run.ts.

Blocking Issues

None.

Suggestions

1. Test coverage for the new event path: The env_var_warning emission in execution_service.ts:234-246 and its handling across the three workflow renderers have no new tests. The existing execution_service_test.ts could be extended with a test that uses a definition containing ${{ env.VAR }} expressions and asserts the env_var_warning event is emitted. This is non-blocking since the underlying detectEnvVarUsageInDefinition function is already tested, and the wiring is straightforward event passthrough.

DDD Review

• The env_var_warning event is correctly modeled as a domain execution event in the workflow bounded context, consistent with sibling events (model_resolved, method_executing, etc.).
• The event is properly propagated through the layers: domain → libswamp (mapped via mapEvent) → presentation renderers — maintaining clean layer separation.
• The EnvVarUsageDetail value object is appropriately reused from the validation service where it's already defined.

Other Notes

• All three renderers (log, JSON, tree) handle the new event correctly: log renders warnings, JSON silently ignores it (consistent with how model_resolved is handled), and tree forwards it to the state reducer which returns state unchanged.
• The mapEvent passthrough in run.ts:351 is correct since the event shape matches between domain and libswamp layers.
• The client protocol type in packages/client/protocol.ts correctly inlines the envVars shape rather than importing from domain internals.
• Import boundaries are respected: libswamp-internal code imports from domain (allowed), and presentation/CLI code doesn't directly import from new internal paths.