Skip to content

feat(runtime): stream persistent sandbox events#618

Merged
drewstone merged 3 commits into
mainfrom
feat/sandbox-event-observer
Jul 25, 2026
Merged

feat(runtime): stream persistent sandbox events#618
drewstone merged 3 commits into
mainfrom
feat/sandbox-event-observer

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Summary

  • add live event observation to openSandboxRun() for both start and resumed turns
  • reuse one defensive, non-blocking observer implementation across both runtime execution paths
  • prove metadata, mutation isolation, synchronous failures, rejected promises, and non-blocking behavior

Checks

  • pnpm exec vitest run tests/runtime/sandbox-run.test.ts src/runtime/run-loop.test.ts (32/32)
  • mutation check: observer tests fail when event forwarding is removed (2/3 red)
  • pnpm typecheck
  • pnpm exec biome check src/runtime/sandbox-events.ts src/runtime/run-loop.ts src/runtime/sandbox-run.ts tests/runtime/sandbox-run.test.ts
  • git diff --check
  • git merge-tree --write-tree origin/main HEAD

Closes #387

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 183faccb

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-25T00:26:24Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Value Audit — sound

Verdict sound
Concerns 0 (none)
Heuristic 0.0s
Duplication 0.0s
Interrogation 78.6s (2 bridge agents)
Total 78.6s

💰 Value — sound

Extends live per-event observation to the persistent-resume path (openSandboxRun) and extracts the shared defensive-copy + error-isolation helper that both run paths now reuse — a clean refactor-plus-feature in the codebase's grain.

  • What it does: Two things. (1) Refactor: the ~70 lines of inline observer logic in run-loop.ts (structuredClone-with-fallback deep copy, sync-throw swallowing, rejected-promise non-await) move to a single notifySandboxEventObserver(event, observer, meta) in sandbox-events.ts (sandbox-events.ts:23-65), and run-loop's executeIteration now calls it (run-loop.ts:752). (2) Feature: openSandboxRun gains an `onSandbo
  • Goals it achieves: (1) Let a caller stream an agent's live output even when running over a persistent sandbox session across resume turns — the capability the run-loop path already had, now available on the resume path too. (2) Collapse two copies of the defensive-copy + isolation contract into one helper so the invariant (observer can never mutate run-consumed events, never break the run) lives in one place. Both a
  • Assessment: Good change, built in the grain of the codebase. The extraction target is exactly right — sandbox-events.ts is already the single home for sandbox-event→runtime mapping (extractLlmCallEvent, mapSandboxEvent, mapSandboxToolEvent all live there, per its header docstring). The generic <Meta> type param cleanly accommodates the genuinely-different metadata at each call site (run-loop: {iterationInde
  • Better / existing approach: none — this is the right approach. Checked for an existing equivalent before answering: (1) notifyRuntimeHookEvent (runtime-hooks.ts:158) is the lifecycle-hook broadcaster but emits coarse turn/run-level snapshots (before/after/error payloads), not per-token streaming — wrong surface for live event tee, and correctly left alone. (2) extractLlmCallEvent/mapSandboxEvent in sandbox-events.ts pr
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

Extracts the existing run-loop event-observer pattern into a shared helper and brings openSandboxRun to parity with runAgentRounds for live event streaming — consolidation, not new surface.

  • Integration: Reachable from both run primitives. notifySandboxEventObserver is called at run-loop.ts:752 (the existing runAgentRounds path) and sandbox-run.ts:257 (newly added to openSandboxRun's settle loop, covering both start at :344 and resume at :384). openSandboxRun itself has ~10 real call sites — bench/src/worker.ts:109, bench/src/run-benchmarks.ts:230 (the default shot runner), bench/src/cloud-loop.mt
  • Fit with existing patterns: Excellent — it REMOVES duplication rather than introducing a competing pattern. The pre-existing run-loop.ts carried ~60 lines of cloneEventForObserver + copyPlainSpine + try/catch inline; this PR moves that exact logic into notifySandboxEventObserver (sandbox-events.ts:23-65) and run-loop.ts now calls it verbatim (run-loop.ts:752). sandbox-run.ts adopts the identical isolation contract. The two m
  • Real-world viability: Robust on the hot path. The isolation guarantees are proven by the new tests (tests/runtime/sandbox-run.test.ts:159-256): mutation isolation (observer mutates data/usage → turn.events and turn.out untouched), sync throw swallowed (call 1), promise rejection swallowed (call 2), and a never-settling promise (call 3) that does not block the turn resolving to {out:{text:'done'}}. structuredClone with
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

No concerns — sound change, no better or existing approach found. ✅


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260725T002932Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 183faccb

Review health 100/100 · Reviewer score 89/100 · Confidence 70/100 · 2 findings (2 low)

glm: Correctness 89 · Security 89 · Testing 89 · Architecture 89

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 2/2 planned shots over 4 changed files. Global verifier still owns final merge decision.

🟡 LOW agentRunName recomputed per event inside settle loop — src/runtime/sandbox-run.ts

options.agentRun.name ?? options.agentRun.profile.name ?? 'agent' is evaluated on every streamed event inside the for-await loop. It is constant for the entire run and already computed in runPayload(). Hoisting it once before the loop (or reusing runPayload's agentName) avoids redundant nullish-coalescing on the hot path. Pure micro-optimization — no correctness impact, observer is opt-in only.

🟡 LOW No test for observer interaction with mid-stream abort — tests/runtime/sandbox-run.test.ts

The new observer tests cover defensive-copy, error-tolerance, and non-blocking semantics in isolation, and the file separately covers abort mid-stream (lines 460-556) without an observer set. The composition — observer registered AND signal aborts mid-drain — is not directly asserted. Evidence: in src/runtime/sandbox-run.ts:254-266 the observer call sits inside the same try block that catches isAbortError, so an observer that throws on the event preceding an abort could in principle mask the abort path (it does not today because notifySandboxEventObserver catches internally). Impact today: none — the helper's own catch makes the composition safe. Sugg


tangletools · 2026-07-25T00:33:41Z · trace

tangletools
tangletools previously approved these changes Jul 25, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 2 non-blocking findings — 183faccb

Full multi-shot audit completed 2/2 planned shots over 4 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-07-25T00:33:41Z · immutable trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — dc59c715

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-25T00:47:58Z

@drewstone
drewstone merged commit a82732b into main Jul 25, 2026
3 checks passed
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.

feat(runtime): expose raw sandbox event tee on openSandboxRun

2 participants