Skip to content

fix: forward driver onLog output as method_output events#900

Merged
stack72 merged 1 commit intomainfrom
fix/driver-onlog-events
Mar 27, 2026
Merged

fix: forward driver onLog output as method_output events#900
stack72 merged 1 commit intomainfrom
fix/driver-onlog-events

Conversation

@johnrwatson
Copy link
Copy Markdown
Contributor

Summary

Out-of-process execution drivers (Tensorlake, Docker, etc.) emit log lines via callbacks.onLog, but these were only forwarded to the logger — not through the onEvent stream. This meant WebSocket clients using swamp serve never received method_output events from these drivers.

Now onLog also emits MethodExecutionEvent { type: "output" } so the output appears in the event stream alongside in-process output.

Test plan

  • Tensorlake driver output appears in WebSocket event stream
  • Existing forEach tests pass (verified locally)

🤖 Generated with Claude Code

Out-of-process execution drivers emit log lines via callbacks.onLog,
but these were only sent to the logger — not through the onEvent
stream. This meant WebSocket clients (swamp serve) never received
method_output events from drivers like Tensorlake.

Now onLog also emits MethodExecutionEvent { type: "output" } so
the output appears in the event stream alongside in-process output.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Blocking Issues

None.

Suggestions

  1. Test coverage for onEvent in driver path: The test file method_execution_service_test.ts has no coverage for onEvent callbacks (pre-existing gap, not introduced by this PR). A test verifying that onLog callbacks forward events via context.onEvent would be valuable as a follow-up — e.g., a mock driver that calls onLog and asserts the event appears on the onEvent callback.

Notes

  • Event shape { type: "output", stream: "stdout", line } correctly matches the MethodExecutionEvent discriminated union in method_events.ts
  • Safe optional chaining (context.onEvent?.()) is consistent with other call sites (data_writer.ts, shell_model.ts)
  • Hardcoding stream: "stdout" is appropriate since the onLog callback doesn't carry stream information
  • No libswamp import boundary violations — this is internal domain code
  • DDD: proper use of domain events within a domain service; no concerns

Copy link
Copy Markdown

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial Review

Critical / High

None.

Medium

  1. src/domain/models/method_execution_service.ts:657 — hardcoded stream: "stdout" is incorrect for Docker driver output

    The Docker driver (docker_execution_driver.ts:157-159, 328-330) calls onLog exclusively from its stderr stream processing. However, the new code hardcodes stream: "stdout" for all onLog output events. This means WebSocket clients will see Docker driver log lines labeled as stdout when they actually originate from stderr.

    The root cause is that the ExecutionCallbacks.onLog interface (execution_driver.ts:60) is stream-agnostic — it only passes (line: string) with no stream indicator. So the method_execution_service layer has no way to know which stream the line came from.

    Impact: Consumers relying on stream to distinguish stdout vs stderr output will misclassify Docker driver logs. This is medium because onLog is used for informational logging (e.g., "starting container", "loading model") rather than model output, so the misclassification is unlikely to cause functional breakage — but it is semantically wrong.

    Suggested fix (not blocking): Either extend ExecutionCallbacks.onLog to (line: string, stream: "stdout" | "stderr") => void and pass the stream through, or use a neutral label like "stderr" since the Docker driver (the only current caller) only emits from stderr. Alternatively, document that stream on driver-forwarded events is best-effort.

Low

None.

Verdict

PASS — The change correctly addresses the missing event forwarding gap. The event shape matches the MethodExecutionEvent type contract exactly. The use of optional chaining (context.onEvent?.()) is safe. The only concern is the hardcoded stream label, which is a semantic inaccuracy rather than a functional bug.

@stack72 stack72 merged commit 0c993a6 into main Mar 27, 2026
10 checks passed
@stack72 stack72 deleted the fix/driver-onlog-events branch March 27, 2026 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants