refactor(session): move run-events to session/auto-reply/ with sink interface by tesdal · Pull Request #14 · tesdal/opencode

tesdal · 2026-04-29T13:49:02Z

Summary

Move src/cli/cmd/run-events.ts → src/session/auto-reply/auto-reply.ts so ACP, future TUI headless/daemon, and CLI run mode can share the auto-permission/question-reply core.
Introduce src/session/auto-reply/sink.ts with a Sink type alias and silentSink constant. Sink decouples emission policy (stdout JSON, log, no-op, future ACP transport) from the bus-subscription core.
Drop jsonMode from the core Config. CLI's run.ts now constructs the JSON-emitting sink via a new exported makeRunSink() helper; non-CLI consumers default to silentSink.
Add onLivelockWarn sink callback alongside the existing log.warn so non-CLI sinks can surface the warning without grepping logs. CLI sink intentionally no-ops it (preserves pre-F11 stdout contract).
Pin the operator-facing JSON shape with a new dedicated test file (test/cli/run-make-sink.test.ts) so renaming any field is caught — the prior shape pinning lived in stdout-monkey-patching tests now reorganized for the sink-callback contract.
ACP unification intentionally deferred (TODO(auto-reply-acp) comment in place).

Diamond review

Both reviewers APPROVE after one rework round.

codex-5.3 (spec): initially REQUEST CHANGES — wanted the JSON shape pinned independently after the stdout monkey-patch tests were retired. Fixed by adding test/cli/run-make-sink.test.ts.
general / Opus (quality): APPROVE with nits. Top non-blocker was "makeRunSink reinvents silentSink". Fixed (now returns `silentAutoReplySink` directly).

Verification

bun typecheck clean
bun test test/session/auto-reply.test.ts — 14/14 pass
bun test test/cli/run-make-sink.test.ts — 5/5 pass
bun test test/session/subagent-hang-regression.test.ts test/cli/run-attach-permission.test.ts — 11/11 pass

Out of scope (deferred follow-ups)

Renaming `runEventsHandle` / `hasRunEventsHandle` in run.ts and dispatchPermissionAsked — would ripple into 9 exported unit tests in run-attach-permission.test.ts. F10 doc-trail narrative paragraphs were updated to refer to SessionAutoReply.
ACP auto-reply unification — separate PR, tracked by inline TODO.
F12 (regression test reuse SessionAutoReply.make / silentSink) — next branch.
F13 (extract Session.isDescendantOf) — branch after F12.

Addresses audit finding F11 (Opus diamond review, 2026-04-22) — biggest upstream blocker per the audit-remediation plan.

…nterface RunEvents was never CLI-specific — ACP and TUI headless/daemon will need equivalent auto-permission-reply logic. Moves to src/session/auto-reply/ with a Sink interface that decouples the event-emission concern from the auto-reply contract. jsonMode becomes a CLI-side concern in run.ts's makeRunSink() implementation. The new SessionAutoReply.make is behaviorally equivalent to the old RunEvents.make except for one extension: a new onLivelockWarn sink callback fires alongside the existing log.warn at the same gate so non-CLI sinks (ACP transport, TUI status bar, daemon telemetry) can surface the warning without grepping logs. The CLI sink in run.ts intentionally no-ops onLivelockWarn to preserve the pre-F11 stdout JSON contract (operators only saw auto-reject/auto-approve events). JSON shape stability is now pinned by test/cli/run-make-sink.test.ts, which exercises makeRunSink in isolation and asserts exact field names (autoRejectSessionID/totalAutoRejects/autoApproveSessionID/ totalAutoApproves/sessionID/timestamp/kind/type). Operator pipelines parsing this output keep working. The variable runEventsHandle and field hasRunEventsHandle in run.ts / dispatchPermissionAsked are intentionally left unrenamed: the names are now slightly misleading but renaming would ripple into 9 exported unit tests in run-attach-permission.test.ts. The F10 doc trail narrative paragraphs were updated to refer to SessionAutoReply. ACP unification deferred to a follow-up PR (see TODO(auto-reply-acp) in src/session/auto-reply/auto-reply.ts). Addresses audit finding F11 (Opus diamond review, 2026-04-22) — biggest upstream blocker. Diamond review: codex-5.3 spec + general (Opus) quality, both APPROVE after rework that added test/cli/run-make-sink.test.ts to pin the JSON shape contract.

Copilot

Pull request overview

Refactors the CLI “run-events” auto-reply bus subscriber into a reusable session/auto-reply module with an injected sink interface so non-CLI runtimes (future TUI/daemon/ACP) can reuse the core while controlling how telemetry is emitted.

Changes:

Introduces AutoReplySink (Sink + silentSink) and moves auto-reply logic into SessionAutoReply.make(config, sink) (removing jsonMode from core config).
Updates CLI run.ts to build a JSON-emitting sink via makeRunSink() (or silentSink when not in json output mode).
Reworks/pins tests: core behavior is asserted via captured sink events; CLI JSON envelope shape is pinned in a dedicated test file.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
packages/opencode/src/session/auto-reply/auto-reply.ts	New reusable auto-reply core using an injected sink for telemetry emission.
packages/opencode/src/session/auto-reply/sink.ts	Adds sink contract (`Sink`) plus `silentSink` no-op implementation.
packages/opencode/src/cli/cmd/run.ts	Builds CLI-specific sink (`makeRunSink`) and wires it into `SessionAutoReply.make`.
packages/opencode/test/cli/run-make-sink.test.ts	New tests pinning the CLI JSON envelope field names and newline-delimited output behavior.
packages/opencode/test/session/auto-reply.test.ts	Updates core tests to assert sink callback contracts instead of stdout monkey-patching.
packages/opencode/test/session/subagent-hang-regression.test.ts	Updates narrative/comments to reference `SessionAutoReply` semantics.

Comments suppressed due to low confidence (3)

packages/opencode/src/session/auto-reply/auto-reply.ts:86

sink.onAutoReject / sink.onLivelockWarn are invoked before the actual question.reject / permission.reply side effects. If a sink implementation throws (e.g. stdout stream closed), the fiber fails and the auto-reply action will never run. Wrap sink callbacks in a try/catch (or Effect.sync + catchAll) so telemetry failures can’t break the core auto-reply behavior; log the sink error at debug/error level and continue.
packages/opencode/src/session/auto-reply/auto-reply.ts:97
sink.onAutoApprove is also called before permission.reply. For the same reason as auto-reject, this callback should be insulated from throwing so a telemetry sink can’t prevent the permission reply from being sent.
packages/opencode/test/session/auto-reply.test.ts:267
The @ts-expect-error only applies to the next line (attach: true). Since jsonMode: true is also intentionally invalid for SessionAutoReply.Config, this test currently relies on TypeScript only reporting the excess-property error on attach. Add a second @ts-expect-error (or restructure the assertion) so the test doesn’t depend on compiler error-location behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ts-expect-error

3 suppressed comments evaluated: 1. auto-reply.ts:86 sink.onAutoReject before question.reject side effect could break auto-reply if sink throws. Acknowledged as a real concern but pre-F11 RunEvents.emit() had identical exposure (process.stdout.write is the only realistic throw site, e.g. EPIPE) — not an F11 regression. Hardened the Sink contract documentation in sink.ts to explicitly require 'callbacks must not throw': implementations targeting fallible transports (closed pipe, network, full disk) must catch internally. Did NOT add try/catch in the core (AGENTS.md: avoid try/catch where possible; core stays small). 2. auto-reply.ts:97 onAutoApprove same concern. Same resolution: covered by the now-explicit Sink contract. 3. auto-reply.test.ts:267 @ts-expect-error covered only the line below (attach: true), so jsonMode: true was unsuppressed but coincidentally not flagged because TS only reports the first excess-property error per literal. Real risk: if attach is later legitimately added, the suppression goes silent on jsonMode. Fixed by splitting into two separate config literals each with its own ts-expect-error. Verification: typecheck clean, 19/19 tests pass (auto-reply 14 + run-make-sink 5).

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-29T14:33:48Z

+  // the right object — reuse it instead of duplicating the shape.
+  if (!jsonMode) return silentAutoReplySink
+  const emit = (type: string, data: Record<string, unknown>) => {
+    process.stdout.write(JSON.stringify({ type, timestamp: Date.now(), sessionID: rootSessionID, ...data }) + "\n")


makeRunSink's Sink callbacks call process.stdout.write directly. That can throw (e.g., broken pipe / EPIPE when piping JSON output), which would violate the Sink contract (“callbacks must not throw”) and can break SessionAutoReply’s guarantee by failing the auto-reply fiber before it performs question.reject/permission.reply. Wrap the write/JSON serialization in a try/catch (or otherwise ensure errors are swallowed) so sink callbacks are non-throwing and auto-reply side effects still run.

Suggested change

process.stdout.write(JSON.stringify({ type, timestamp: Date.now(), sessionID: rootSessionID, ...data }) + "\n")

try {

process.stdout.write(JSON.stringify({ type, timestamp: Date.now(), sessionID: rootSessionID, ...data }) + "\n")

} catch {

// Sink callbacks must not throw. Swallow serialization/stdout failures

// (for example broken pipe / EPIPE) so auto-reply side effects still run.

}

Applied. makeRunSink now wraps the stdout.write in a try/catch (commit 9374fb0). New test test/cli/run-make-sink.test.ts pins the contract: an EPIPE-throwing stdout.write must not propagate out of any of the three sink callbacks. The Sink contract documented in sink.ts ('callbacks must not throw') is now honored by the only caller-supplied sink in the codebase. Good catch — without this fix the contract would have been aspirational only.

R1 documented 'Sink callbacks must not throw' (sink.ts JSDoc) but the only caller-supplied sink in the codebase (makeRunSink) violated it: process.stdout.write throws EPIPE when a downstream consumer closes the pipe (e.g. `opencode run --output-format=json | head -1`). That would fail the auto-reply fiber before its question.reject / permission.reply side effect runs, breaking the contract that earned the documentation in the first place. Wraps the JSON.stringify+stdout.write in a try/catch with an intentionally empty catch block. AGENTS.md's 'avoid try/catch where possible' is overridden here because this is exactly where contract enforcement belongs — concentrated in the sink implementation rather than spread through the core. The core stays try/catch-free. New test pins the contract: a stdout.write that throws EPIPE must not propagate out of any of the three sink callbacks.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

tesdal · 2026-04-29T18:42:17Z

Review-only PR (fork-local). Diamond + 3 rounds of Copilot review complete; merging via --no-ff to local/integration-v2 in the worktree.

tesdal requested a review from Copilot April 29, 2026 13:49

Copilot started reviewing on behalf of tesdal April 29, 2026 13:49 View session

Copilot AI reviewed Apr 29, 2026

View reviewed changes

tesdal requested a review from Copilot April 29, 2026 14:28

Copilot started reviewing on behalf of tesdal April 29, 2026 14:29 View session

Copilot AI reviewed Apr 29, 2026

View reviewed changes

tesdal requested a review from Copilot April 29, 2026 14:38

Copilot started reviewing on behalf of tesdal April 29, 2026 14:39 View session

Copilot AI reviewed Apr 29, 2026

View reviewed changes

tesdal closed this Apr 29, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(session): move run-events to session/auto-reply/ with sink interface#14

refactor(session): move run-events to session/auto-reply/ with sink interface#14
tesdal wants to merge 3 commits intophase-ab-basefrom
audit-f11-runevents-to-core

tesdal commented Apr 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 29, 2026

Uh oh!

tesdal Apr 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

tesdal commented Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-    process.stdout.write(JSON.stringify({ type, timestamp: Date.now(), sessionID: rootSessionID, ...data }) + "\n")
+    try {
+      process.stdout.write(JSON.stringify({ type, timestamp: Date.now(), sessionID: rootSessionID, ...data }) + "\n")
+    } catch {
+      // Sink callbacks must not throw. Swallow serialization/stdout failures
+      // (for example broken pipe / EPIPE) so auto-reply side effects still run.
+    }

Conversation

tesdal commented Apr 29, 2026

Summary

Diamond review

Verification

Out of scope (deferred follow-ups)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

tesdal Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

tesdal commented Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants