Adversarial Review

Critical / High

1. HIGH — driver field never reaches consumers via the libswamp layer (src/libswamp/workflows/run.ts:82-90 and src/libswamp/workflows/run.ts:324-334)

   The PR adds driver?: string to the domain event type (execution_events.ts) and the client protocol type (packages/client/protocol.ts), and sets it in execution_service.ts. However, the intermediate libswamp WorkflowRunEvent type (line 86-89) and the mapEvent function (lines 324-334) that translates domain events into libswamp events were not updated.

   The mapEvent function explicitly constructs a new object with only kind, runId, workflowName, and jobs — the driver field is silently dropped:

   // src/libswamp/workflows/run.ts:324-334
   case "started":
     return {
       kind: "started",
       runId: event.runId,
       workflowName: event.workflowName,
       jobs: event.jobs.map(…),
       // ← driver is not forwarded
     };

   How it breaks: Run any workflow with driver: "@john/tensorlake". The domain event will contain driver: "@john/tensorlake", but after mapEvent processes it, the libswamp event sent to the server (and serialized to the WebSocket client) will have driver: undefined. The stated goal of the PR — "Allows consumers to know the executor immediately at workflow start" — does not work.

   Fix: Add driver?: string to the started variant of WorkflowRunEvent in src/libswamp/workflows/run.ts (around line 86-89), and forward driver: event.driver in the mapEvent function (around line 326).

Medium

2. MEDIUM — onLog→onEvent forwarding only applies to out-of-process drivers (src/domain/models/method_execution_service.ts:666-673)

   The new onEvent call inside the onLog callback is only added for out-of-process (non-raw) drivers. The raw driver path (line 605-608) calls driver.execute(executionRequest) with no callbacks at all. This is an inconsistency — output from raw driver execution won't emit output events through onEvent. This may be intentional (raw driver may emit events through a different mechanism), but it's worth verifying that the asymmetry is deliberate and that consumers aren't missing output from raw-driver runs.

Low

3. LOW — Protocol test doesn't exercise driver field (src/serve/protocol_test.ts:149-152)

   The existing ServerMessage test at line 152 constructs a started event without driver, and no new test was added for the driver field. Given finding #1, this is academic until the field actually propagates, but once fixed, tests should cover both the driver: "some-driver" and driver: undefined cases.

Verdict

FAIL — The driver field is added to the domain event and client protocol but is silently dropped by the libswamp mapping layer (mapEvent in src/libswamp/workflows/run.ts), so the feature does not work end-to-end. The libswamp WorkflowRunEvent type and its mapEvent function must be updated for the driver to reach consumers.

Code Review

Blocking Issues

1. driver field missing from libswamp layer — data silently dropped before reaching clients
   src/libswamp/workflows/run.ts:82-90 — The WorkflowRunEvent type's started variant does not include driver?: string. The mapEvent function at line 324-334 explicitly constructs the started event with only runId, workflowName, and jobs — dropping driver. Since the server serializer operates on libswamp events (not domain events), the driver field added to packages/client/protocol.ts will never actually be populated over the wire. This needs:

   • Add driver?: string to the started variant of WorkflowRunEvent in src/libswamp/workflows/run.ts
   • Forward driver: event.driver in the mapEvent function's "started" case

2. Undocumented behavioral change: registered drivers now emit output events
   src/domain/models/method_execution_service.ts:666-673 — The onLog callback for registered (non-raw) drivers now calls context.onEvent?.({ type: "output", ... }) in addition to logging. Previously only the raw driver emitted MethodExecutionEvent output events. This means workflows using registered drivers will now produce method_output events in the event stream, which is a user-visible behavioral change not mentioned in the PR title, description, or test plan. If this is intentional, please document it; if not, it should be a separate PR.

3. No tests for either change
   Per CLAUDE.md, new code needs unit test coverage. Neither the driver field on the started event nor the new onEvent output forwarding for registered drivers has test coverage. At minimum:

   • A test in src/libswamp/workflows/run_test.ts verifying driver is present on the mapped started event when provided, and undefined when not
   • A test for the onEvent output forwarding in the registered driver path

Suggestions

1. The protocol_test.ts started event at line 152 ({ kind: "started", runId: "r1" }) is missing workflowName — while it type-checks because the type was loosened, updating it to include the full shape would make it a better regression guard.

Adversarial Review

Critical / High

None.

Medium

None.

Low

1. src/domain/models/method_execution_service.ts:668-672 — onEvent hardcodes stream: "stdout": The onLog callback receives all driver log lines and forwards them as stream: "stdout". If a driver ever emits stderr-originated lines through onLog, they will be misclassified. This is consistent with the existing onLog contract (which doesn't distinguish streams), and the in-process raw driver path doesn't hit this code, so this is purely theoretical. No action needed now, but worth noting if driver log fidelity becomes important.

2. src/domain/models/method_execution_service.ts:668 — onEvent failure in onLog callback: If context.onEvent throws, it will propagate out of the onLog callback and abort driver execution. The original onLog only called context.logger?.info(line) which is unlikely to throw. Adding context.onEvent?.() introduces a second call that could throw if the event consumer has a bug. The blast radius is limited (it would fail the step), but it's a change in failure characteristics. Theoretical risk.

Verdict

PASS — Clean, minimal change that threads an optional driver field through three type layers (domain events → libswamp → client protocol) and adds stdout event forwarding for out-of-process drivers. All types align correctly (workflow.driver: string | undefined → driver?: string on all event types). The serializer uses generic jsonSafeClone so no serialization changes are needed. No logic errors, no security concerns, no breaking API changes.

Code Review

Blocking Issues

None.

Suggestions

1. Undocumented behavioral change in method_execution_service.ts: The PR description mentions only adding driver to the workflow started event, but there's also a change to forward driver onLog lines as onEvent({ type: "output", ... }) events. This is actually a useful addition (mirrors the pattern in shell_model.ts:125), but worth calling out in the PR description for reviewers.

2. No new tests for the onEvent forwarding: There are no existing tests covering the onLog callback in the driver execution path (method_execution_service_test.ts), so this PR doesn't regress test coverage. However, a unit test verifying that driver log lines are forwarded as output events would be a nice addition. Not blocking since the existing test file doesn't cover this code path.

3. Type-only changes are consistent across all three layers (domain events → libswamp → client protocol) — nicely done. The mapEvent function correctly passes through the new driver field.

Overall: Small, focused change. The driver field addition is clean and consistent with the existing domain model. The onEvent forwarding for driver logs is a correct pattern match with shell_model.ts.

🤖 Generated with Claude Code

CLI UX Review

Blocking

None.

Suggestions

1. Log-mode: driver not shown on start — src/presentation/renderers/workflow_run.ts:65-68: The started handler now receives driver but only logs "Starting workflow". A user who passes --driver @john/tensorlake gets no visual confirmation that the override took effect. Consider appending it: "Starting workflow (driver: {driver})" when e.driver is set.

2. JSON-mode: driver absent from completed output — WorkflowRunView (in src/libswamp/workflows/workflow_run_view.ts) has no driver field, so the JSON output emitted at completion doesn't include which driver was used. Scripts that parse swamp workflow run --json have no way to confirm the driver. Consider adding driver?: string to WorkflowRunView and populating it in toRunData.

Verdict

PASS — no blocking issues. The type additions are clean and additive; existing output is unaffected.