Skip to content

fix(agent): track streamed activity from event sessions#611

Merged
marcusrbrown merged 13 commits into
mainfrom
fix/scheduled-agent-watchdog
May 12, 2026
Merged

fix(agent): track streamed activity from event sessions#611
marcusrbrown merged 13 commits into
mainfrom
fix/scheduled-agent-watchdog

Conversation

@marcusrbrown
Copy link
Copy Markdown
Collaborator

Summary

  • Reads streamed event session IDs from the event envelope before falling back to nested message part/message metadata.
  • Keeps activity and token tracking compatible with current OpenCode SDK event shapes.
  • Adds regression coverage for envelope-level session IDs on part and message updates.

Why

Scheduled Fro Bot runs can receive active stream events where the session ID is carried on the event envelope instead of the nested payload. Treating those events as unrelated can trip the activity watchdog even while work is still progressing.

Verification

  • pnpm vitest run src/features/agent/opencode.test.ts -- --runInBand
  • pnpm check-types
  • pnpm lint
  • pnpm build

@marcusrbrown marcusrbrown requested a review from fro-bot as a code owner May 11, 2026 19:49
@marcusrbrown marcusrbrown self-assigned this May 11, 2026
Copy link
Copy Markdown
Owner

@fro-bot fro-bot left a comment

Choose a reason for hiding this comment

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

Verdict: CONDITIONAL

The core intent is correct — reading the session ID from the event envelope before falling back to the nested payload prevents false watchdog trips on scheduled runs. The logic and tests are sound overall, with one non-blocking concern worth discussing.

Blocking issues

None.

Non-blocking concerns

** uses where a simple property access would do**

// streaming.ts:58-63
function getSessionID(value: unknown): string | null {
  if (value == null || typeof value !== 'object') return null
  const descriptor = Object.getOwnPropertyDescriptor(value, 'sessionID')
  return typeof descriptor?.value === 'string' ? descriptor.value : null
}

This deliberately avoids inherited or accessor properties. That's a valid defensive move when value is an opaque SDK payload, but it silently misses a sessionID that is:

  • defined as a getter (accessor descriptor) rather than a data descriptor, or
  • inherited from a prototype.

If the SDK ever ships a class-based event where sessionID is a getter, this returns null instead of the correct value, causing a silent session-ID mismatch. A simple typeof (value as Record<string,unknown>).sessionID === 'string' check would be more robust, matches the pattern used for session.error / session.idle (line 127, 136), and is equally type-safe. If the strictness is intentional (e.g., guarding against prototype-pollution attacks from malformed payloads), a comment explaining the rationale would clarify this for future readers.

Asymmetry between event types

session.error (line 127) and session.idle (line 136) still read directly from event.properties.sessionID without going through getSessionID. That's fine for now because those event types already carry a top-level sessionID in the typed SDK shape, but it means the two code paths are inconsistent. If a future SDK version moves sessionID to the envelope for those events too, only the message.* handlers would pick it up. Not a bug today, just an inconsistency worth noting.

Missing tests

The new tests cover the two primary cases (envelope-level sessionID for message.part.updated and message.updated). A test for mismatched envelope session IDs being filtered out would complete the regression suite — e.g., verify that an event with properties.sessionID = 'ses_other' and no part.sessionID is skipped (i.e., firstMeaningfulEventReceived stays false). This mirrors the existing "sets sessionIdle only for matching session" pattern on line 1930.

Risk assessment (LOW):

The fix is narrowly scoped to reading an additional field before the existing fallback. The failure mode if getSessionID returns null (e.g., due to the accessor-descriptor edge case) degrades gracefully to the previous behavior — the activity tracker simply doesn't advance, which is the pre-fix status quo. No data is mutated, no external calls are made, and the watchdog logic in session-poll.ts is unchanged. Blast radius is limited to scheduled-agent liveness detection.


Run Summary
Field Value
Event pull_request
Repository fro-bot/agent
Run ID 25693461325
Cache hit
Session ses_1e76768d7ffelY0uYGJ4V1mBQE

Copy link
Copy Markdown
Owner

@fro-bot fro-bot left a comment

Choose a reason for hiding this comment

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

Verdict: CONDITIONAL

The core intent is correct — reading the session ID from the event envelope before falling back to the nested payload prevents false watchdog trips on scheduled runs. The logic and tests are sound overall, with one non-blocking concern worth discussing.

Blocking issues

None.

Non-blocking concerns

getSessionID uses Object.getOwnPropertyDescriptor where a simple property access would do

// streaming.ts:58-63
function getSessionID(value: unknown): string | null {
  if (value == null || typeof value !== 'object') return null
  const descriptor = Object.getOwnPropertyDescriptor(value, 'sessionID')
  return typeof descriptor?.value === 'string' ? descriptor.value : null
}

This deliberately avoids inherited or accessor properties. That is a valid defensive move when value is an opaque SDK payload, but it silently misses a sessionID that is defined as a getter (accessor descriptor) rather than a data descriptor, or inherited from a prototype. If the SDK ever ships a class-based event where sessionID is a getter, this returns null instead of the correct value, causing a silent session-ID mismatch. A simple typeof (value as Record<string,unknown>).sessionID === 'string' check would be more robust, matches the pattern used for session.error / session.idle (line 127, 136), and is equally type-safe. If the strictness is intentional (e.g., guarding against prototype-pollution from malformed payloads), a comment explaining the rationale would help future readers.

Asymmetry between event types

session.error (line 127) and session.idle (line 136) still read directly from event.properties.sessionID without going through getSessionID. That is fine for now because those event types already carry a top-level sessionID in the typed SDK shape, but it means the two code paths are inconsistent. If a future SDK version moves sessionID to the envelope for those events too, only the message.* handlers would pick it up. Not a bug today, just worth noting.

Missing tests

The new tests cover the two primary cases (envelope-level sessionID for message.part.updated and message.updated). A test for mismatched envelope session IDs being filtered out would complete the regression suite — e.g., verify that an event with properties.sessionID = 'ses_other' and no part.sessionID is skipped (i.e., firstMeaningfulEventReceived stays false). This mirrors the existing "sets sessionIdle only for matching session" pattern at line 1930.

Risk assessment (LOW)

The fix is narrowly scoped to reading an additional field before the existing fallback. The failure mode if getSessionID returns null (e.g., due to the accessor-descriptor edge case) degrades gracefully to the previous behavior — the activity tracker simply does not advance, which is the pre-fix status quo. No data is mutated, no external calls are made, and the watchdog logic in session-poll.ts is unchanged. Blast radius is limited to scheduled-agent liveness detection.


Run Summary
Field Value
Event pull_request
Repository fro-bot/agent
Run ID 25693461325
Cache hit
Session ses_1e76768d7ffelY0uYGJ4V1mBQE

fro-bot
fro-bot previously approved these changes May 11, 2026
Copy link
Copy Markdown
Owner

@fro-bot fro-bot left a comment

Choose a reason for hiding this comment

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

Verdict: PASS

The second commit addresses the spirit of the previous feedback and adds meaningful new coverage. The PR is ready to merge with the minor observations below noted for awareness.

Blocking issues

None.

Non-blocking concerns

getStringProperty(event, 'type') treats a typed field as opaque

event.type is always a string in the SDK's Event type, so getStringProperty(event, 'type') will work in practice. But using the descriptor-based accessor for a typed property leaks the "defensive unknown-object" pattern into typed call sites, and it silently returns null if type were ever an accessor property (same caveat as the original getSessionID concern). A direct event.type dereference would be cleaner and match how event.type is accessed everywhere else in the function. This is cosmetic — no actual risk here.

isStreamActivityEvent only checks message.part.delta and session.next.*

The prefix match eventType?.startsWith('session.next.') is intentionally broad, which is the right call for forward-compat. Just worth confirming the SDK won't emit unrelated session.next.* events that happen to carry a different session's ID (they'd still be filtered by the getEventSessionID === sessionId guard on line 108, so worst case is a spurious firstMeaningfulEventReceived = true for the correct session — not harmful).

getEventSessionID checks properties then data but not the event root

The existing getSessionID(event.properties) ?? getSessionID(part) pattern on line 113 checks the envelope first, then a nested field. getEventSessionID checks properties then data, which covers the new event shapes. If a future event shape ever puts sessionID directly on the event root object, neither path catches it — but that's speculative and the existing coverage is consistent.

Missing tests

The suggested negative test from the prior review (mismatched envelope sessionID for message.part.updated being filtered) is still absent, but the new isStreamActivityEvent path has its own mismatch filtering tested via the session-next-on-data test. Acceptable gap given the graceful degradation path.

Risk assessment (LOW)

Scope is still narrowly contained to processEventStream. The new early-exit block at line 106-109 runs before any state-mutating code and only sets firstMeaningfulEventReceived = true — it cannot corrupt tokens, model, cost, or the artifact lists. session.error / session.idle handlers are unchanged. The getEventSessionID guard on line 108 ensures cross-session pollution cannot occur.


Run Summary
Field Value
Event pull_request
Repository fro-bot/agent
Run ID 25694424519
Cache hit
Session ses_1e76768d7ffelY0uYGJ4V1mBQE

fro-bot
fro-bot previously approved these changes May 11, 2026
Copy link
Copy Markdown
Owner

@fro-bot fro-bot left a comment

Choose a reason for hiding this comment

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

Verdict: PASS

The third commit adds the missing negative test ('ignores stream activity events for other sessions') that was called out in the prior reviews, completing the regression suite. No source changes — this is a test-only commit.

All concerns from prior reviews have been addressed:

  • Envelope-level session ID fallback: covered
  • message.part.delta and session.next.* delta events recognized as activity: covered
  • Cross-session filtering for stream activity events: now covered by the new test

Blocking issues

None.

Non-blocking concerns

The previously noted observations (descriptor-based accessor for event.type, session.error/session.idle asymmetry, getEventSessionID not checking the event root) remain as low-friction items. None warrant blocking merge.

Missing tests

None.

Risk assessment (LOW)

No logic changes in this commit. Test-only. The full suite of processEventStream tests now exercises all the envelope/nested/mismatch combinations introduced across the three commits.


Run Summary
Field Value
Event pull_request
Repository fro-bot/agent
Run ID 25694584241
Cache hit
Session ses_1e76768d7ffelY0uYGJ4V1mBQE

fro-bot
fro-bot previously approved these changes May 11, 2026
Copy link
Copy Markdown
Owner

@fro-bot fro-bot left a comment

Choose a reason for hiding this comment

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

Verdict: PASS

Commit 4 (69ff3f9) is a substantive expansion that addresses the sync-type event shape used by some SDK transports, fixes a subscribe/prompt ordering race in prompt-sender.ts, and completes the refactor by routing session.error and session.idle through the same getEventPayload/getSessionID accessors as the rest of the handlers — closing the asymmetry noted in earlier reviews.

Blocking issues

None.

Non-blocking concerns

getEventKind strips trailing .N from sync event names

The regex /\.\d+$/ correctly normalizes session.next.text.delta.1session.next.text.delta and session.idle.1session.idle. Edge case: a sync event whose name ends in a numeric segment that is semantically meaningful (e.g., a hypothetical session.v2) would be silently stripped. That seems unlikely given the naming convention, but is worth being aware of. Graceful fallback to the raw sync type keeps this safe.

outputToolExecution now receives String(getObjectProperty(toolState, 'title') ?? '') instead of the typed toolState.title

The original code passed toolState.title which was typed. The new code coerces getObjectProperty(toolState, 'title') ?? '' to string. This is functionally identical for plain-string titles but would coerce non-string values (e.g., objects) to "[object Object]" rather than failing type-safe. Given the SDK emits well-typed payloads in practice this is fine, but the double-coercion through getObjectProperty then String() is more defensive than necessary here.

getNumberProperty added but isStreamActivityEvent / activity path still use getStringProperty

All paths are now consistently using the descriptor helpers. getNumberProperty is used for timestamps, token counts, and cost — sensible.

Missing tests

Two new tests ('marks activity tracker when sync session-next deltas arrive' and 'sets sessionIdle when sync session idle arrives') cover the sync event normalization. The subscribe-before-prompt race fix in prompt-sender.ts is not directly tested in isolation, but the existing integration-style tests exercise the full sendPromptToSessionrunPromptAttempt path and would catch regressions.

Risk assessment (LOW)

The subscribe-before-prompt reordering in prompt-sender.ts is the highest-impact change: it eliminates a real race condition where the first streaming delta could be missed if the SDK emitted it between promptAsync resolving and event.subscribe() completing. The change is safe — event.subscribe() is moved ahead of promptAsync, so the stream is open before any events can be emitted. The rest of the streaming.ts refactor replaces direct typed accesses with descriptor-based helpers throughout; the descriptor pattern's accessor/inherited-property blind spot applies uniformly now but has no practical impact on JSON-deserialized SDK payloads. session-poll.ts change (marking activity on any non-null status response) is correct — if the server responds at all, the agent is clearly alive.


Run Summary
Field Value
Event pull_request
Repository fro-bot/agent
Run ID 25695542023
Cache hit
Session ses_1e76768d7ffelY0uYGJ4V1mBQE

fro-bot
fro-bot previously approved these changes May 11, 2026
Copy link
Copy Markdown
Owner

@fro-bot fro-bot left a comment

Choose a reason for hiding this comment

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

Verdict: PASS

Commits 4 (69ff3f9) and 5 (8557db1) address the remaining structural gaps: commit 4 unifies all event shape variations under getEventKind/getEventPayload (closing the asymmetry noted in earlier reviews), and commit 5 introduces v2.session.wait() as the authoritative session-completion signal — a meaningful reliability improvement for SDK 1.14.39+ that eliminates over-reliance on polling heuristics.

Blocking issues

None.

Non-blocking concerns

Double waitForEventProcessorShutdown call on happy path (retry.ts:164 vs retry.ts:188)

collectEventResults() at line 164 calls waitForEventProcessorShutdown(eventProcessor), and the finally block at line 188 calls it again unconditionally. On a settled promise the second race resolves instantly (no observable harm), but it is a latent maintenance hazard — if eventProcessor is ever replaced with something that is not idempotent to drain twice, this will silently double-consume it. A simple let eventResultsCollected = false guard, or moving collectEventResults() exclusively into the finally block, would remove the ambiguity.

startV2SessionWait sets activityTracker.firstMeaningfulEventReceived = true on wait() success

This is correct in intent — if wait() resolves, the agent clearly did work. But it means the no-activity watchdog in pollForSessionCompletion (line 81-93 of session-poll.ts) can be bypassed before the poll ever runs the activity check, since activityTracker.firstMeaningfulEventReceived is set by a concurrent goroutine. In practice this is fine (wait resolving means the session completed), but it is worth noting that the activity timeout is no longer the authoritative liveness check when v2.session.wait() is present.

getEventKind regex strips only trailing .N from sync event names

/\.\d+$/ correctly handles session.idle.1session.idle and session.next.text.delta.1session.next.text.delta. If the SDK ever emits a segment number in a non-terminal position (e.g., session.1.idle), the normalization would not apply and the event would be treated as type session.1.idle. Unlikely but worth tracking as SDK versioning evolves.

Missing tests

collectEventResults being called twice is not explicitly tested. The test 'never-resolving wait() does not block the no-activity watchdog' covers the important failure mode and the abort signal propagation. Coverage is otherwise thorough across all v2.session.wait() scenarios.

Risk assessment (LOW/MED)

The v2.session.wait() path changes how session completion is determined for SDK 1.14.39+. The duck-type guard (typeof waitFn !== 'function') correctly short-circuits for older SDKs, and all three failure modes (unavailable, error response, thrown exception) fall back to the existing polling path. The race construction Promise.race([waitPromise.then(...), pollPromise]) is correct — when waitFn fails .then() returns pollPromise, which is the same reference as the other race arm; the first one to resolve wins without duplication.

The prompt-sender.ts subscribe-before-prompt reordering is the highest correctness gain: it eliminates a real window where the first streaming event could be lost between promptAsync returning and event.subscribe() completing. The change is safe — subscribing before prompting is the correct ordering per SSE semantics.

Overall blast radius is limited to session completion detection. The polling watchdog remains an independent fallback for all new paths.


Run Summary
Field Value
Event pull_request
Repository fro-bot/agent
Run ID 25698278382
Cache hit
Session ses_1e76768d7ffelY0uYGJ4V1mBQE

fro-bot
fro-bot previously approved these changes May 11, 2026
Copy link
Copy Markdown
Owner

@fro-bot fro-bot left a comment

Choose a reason for hiding this comment

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

Verdict: PASS

Commits 7 (c7119e7) and 8 (b7defbc) address the most subtle correctness gap identified in the prior review: v2.session.wait() resolving before the current-turn event processor has had a chance to observe any activity. The 500ms/10ms grace loop in startV2SessionWait gives the async event processor time to catch up before accepting wait() as proof that the current turn completed. session-poll.ts gets a matching guard — a polling-detected idle now also requires firstMeaningfulEventReceived before it counts as completion.

Blocking issues

None.

Non-blocking concerns

Double waitForEventProcessorShutdown on the happy path is still present

collectEventResults() at line 174 drains the processor, then the finally block at line 198 drains it again. As noted in review 006, a settled Promise.race resolves instantly so there is no observable harm today. Still worth tracking — if eventProcessor is ever replaced with a non-idempotent drainable, this silently double-consumes it.

500ms grace window is a hard-coded constant with no test for the boundary

ACTIVITY_GRACE_MS = 500 (retry.ts:64) is the deadline for the event processor to observe activity after wait() resolves. The value is defensible but untested at the boundary — there is no test that verifies the grace loop exhaustion path falls back correctly (i.e., wait() resolves but activity never arrives within 500ms → returns false → poll is the authority). The test 'wait() resolves before current-turn activity is observed falls through to poll' at line ~2390 covers the case where poll eventually times out, but not the mid-grace-window state. Low urgency given the fallback is always the poll watchdog.

session-poll.ts idle guard skips returning completed: true but doesn't log the skip until the next iteration

At line 73-74, when idle is seen before activity the poller logs and continues. If the session subsequently times out, the caller sees Poll timeout rather than a more specific message about idle-before-activity. Minor — does not affect behavior.

Missing tests

A test verifying that when wait() resolves but the 500ms grace period expires with no activity, startV2SessionWait returns false (forcing poll to be the authority) would close the remaining gap. The current suite tests "wait before activity → poll timeout" but not the exact boundary condition where the grace loop runs to completion.

Risk assessment (LOW)

The activity guard is conservative in the right direction — wait() can only accelerate success, never mask a real problem. If the grace loop times out, the system falls back to the existing poll watchdog unchanged. The new session-poll.ts guard (requiring activity before accepting a polling-detected idle) is additive and does not affect runs where firstMeaningfulEventReceived is already set before the poll detects idle, which is the common case.


Run Summary
Field Value
Event pull_request
Repository fro-bot/agent
Run ID 25702408320
Cache hit
Session ses_1e76768d7ffelY0uYGJ4V1mBQE

fro-bot
fro-bot previously approved these changes May 11, 2026
Copy link
Copy Markdown
Owner

@fro-bot fro-bot left a comment

Choose a reason for hiding this comment

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

Verdict: PASS

Commit 9 (0dc58e8) addresses the race between stream subscription and prompt submission with a clean inversion of control: the caller now passes a startPrompt callback that runPromptAttempt invokes only after the event stream iterator has started consuming. The currentTurnArmed flag gates the event processor so pre-arm events (which can arrive before the current prompt fires, e.g., stale idle events from a previous turn) are silently dropped rather than miscounted as current-turn activity.

The key invariant is sound: Promise.resolve() at retry.ts:137 yields to the microtask queue, allowing the async generator inside processEventStream to reach its first await (the for await loop head), which begins pulling from the SSE stream. startPrompt is then called with the stream already consuming, and currentTurnArmed is set to true immediately before promptAsync sends the request. Any events that arrive between event.subscribe() and the arm point are correctly discarded.

Blocking issues

None.

Non-blocking concerns

currentTurnArmed is optional on ActivityTracker but treated as boolean-false when undefined

streaming.ts:124 checks activityTracker?.currentTurnArmed === false, which correctly passes through when the field is undefined (i.e., callers that construct ActivityTracker without setting the field). However, the existing tests for processEventStream construct the tracker as {firstMeaningfulEventReceived: false, sessionIdle: false, sessionError: null} — without currentTurnArmed. Those tests continue to work because undefined === false is false. This is intentional and safe, but the optionality is a mild footgun: a caller who forgets to set currentTurnArmed: true will find their events silently dropped. The field and the skip condition are well-commented in the source, so this is acceptable.

Double waitForEventProcessorShutdown drain still present

Lines 192 and 216 of retry.ts still call waitForEventProcessorShutdown(eventProcessor) twice on the success path (once inside collectEventResults(), once in finally). No observable harm; tracking note remains the same as prior reviews.

await Promise.resolve() is a micro-ordering assumption

The single microtask tick at retry.ts:137 is enough to let the for await loop inside processEventStream reach its first iteration in the current V8/Node.js async scheduler. This is a valid and idiomatic pattern, but it is an internal timing assumption rather than a protocol guarantee. A brief comment explaining the intent (matching the one in prompt-sender.ts:134-136) would make this clear for future maintainers.

Missing tests

The new test 'starts consuming the lazy event stream before prompt submission and ignores pre-arm stale events' (line 2335) covers the primary scenario. The 500ms grace boundary case for startV2SessionWait (noted in review 007) remains untested but is still non-blocking.

Risk assessment (LOW)

The currentTurnArmed guard only suppresses activity recording — it never suppresses polling or completion detection. If a bug caused currentTurnArmed to remain false indefinitely, the worst outcome is that firstMeaningfulEventReceived is never set, which triggers the existing no-activity watchdog (90s timeout) — the same safe fallback that existed before this PR.


Run Summary
Field Value
Event pull_request
Repository fro-bot/agent
Run ID 25703263669
Cache hit
Session ses_1e76768d7ffelY0uYGJ4V1mBQE

fro-bot
fro-bot previously approved these changes May 12, 2026
Copy link
Copy Markdown
Owner

@fro-bot fro-bot left a comment

Choose a reason for hiding this comment

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

Verdict: PASS

Commit 12 (e5bd8f1) addresses the false-positive completion scenario identified by the prior c025372 commit: OpenCode sometimes completes one assistant message and immediately starts the next turn's loop. The stability window (COMPLETED_ASSISTANT_MESSAGE_STABILITY_MS = 1000ms) prevents declaring success on a time.completed message until it has been stable (the same message is the "latest" assistant message) for at least a full second. If a newer in-progress assistant message appears within the window, the stability timer resets.

The startV2SessionWait grace loop has been correctly upgraded from checking firstMeaningfulEventReceived to currentTurnTerminalSignalReceived — a strictly stronger signal that requires either a session.idle event or a stably-completed assistant message before accepting wait() as proof that the current turn fully finished.

Blocking issues

None.

Non-blocking concerns

Stability window is a wall-clock poll, not event-driven

The 1000ms stability check runs by re-querying session.messages() every 500ms (the POLL_INTERVAL_MS tick). In practice this means completion detection can take up to 1500ms after the message is first observed — 500ms for the first poll to find it, 500ms for the next poll, then pass at the 1000ms stability threshold. This is fine for correctness, but callers sensitive to latency should be aware. No action needed.

detectMessageActivity iterates all messages but only keeps the last assistant message

The current loop overwrites latestAssistantMessageInfo on each new assistant message (line 87), so only the latest is checked. This is correct by design, but if the SDK ever returns messages in non-chronological order, the last-in-list message might not be the most recent. Given the SDK presumably returns messages in insertion order, this is safe. A sort by time.created would be defensive but is not necessary today.

Double waitForEventProcessorShutdown drain is still present

retry.ts lines 249 and 273 still call it twice. Same note as prior reviews — idempotent today, maintenance hazard long-term.

Missing tests

'does not complete when a completed assistant message is followed by a newer in-progress assistant message' (the multi-step scenario) is now tested. The 500ms grace boundary case for startV2SessionWait (noted in review 007, now upgraded to check currentTurnTerminalSignalReceived) remains untested at the exact boundary condition but is still non-blocking given the poll watchdog fallback.

Risk assessment (LOW)

The stability window only delays success — it cannot cause a false positive. If the stability check runs past the overall maxPollTimeMs, the poller returns Poll timeout rather than a success, which is the correct safe fallback. New fields on ActivityTracker (completedAssistantMessageId, completedAssistantMessageObservedAt) are non-optional in structure but only populated by detectMessageActivity; all poll paths that do not use baselineMessageIds skip detectMessageActivity entirely (baselineMessageIds == null guard at line 65), so existing paths are unaffected.


Run Summary
Field Value
Event pull_request
Repository fro-bot/agent
Run ID 25706014891
Cache hit
Session ses_1e76768d7ffelY0uYGJ4V1mBQE

Render assistant text and tool execution lines from current OpenCode stream event shapes while preserving the terminal completion gate.
Copy link
Copy Markdown
Owner

@fro-bot fro-bot left a comment

Choose a reason for hiding this comment

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

Verdict: PASS

Commit 13 (47e65ad) restores stream output that was lost when the getObjectProperty-based refactor removed direct typed access to message.part.updated fields — it adds four new event handlers for the V2 sync protocol's streaming output shape:

  • message.part.delta — accumulates text delta fragments into lastText (flushed on session.idle)
  • session.next.text.delta — handles the sync-envelope shape with both plain-string and object-shaped deltas
  • session.next.tool.called — caches tool call metadata by callID in pendingToolCalls
  • session.next.tool.success — correlates with the cached call, renders tool output, and runs artifact detection

The logServerEvent function is also fixed for sync events, which previously logged properties: undefined (sync events carry data on the data field, not properties).

Blocking issues

None.

Non-blocking concerns

pendingToolCalls is unbounded and never pruned for uncorrelated calls

If a session.next.tool.called event arrives but its matching session.next.tool.success never comes (tool error, abort, session crash), the callID entry stays in the map for the life of the stream. In practice each session processes a bounded number of tool calls, so this is not a memory leak of concern — just worth noting. A corresponding session.next.tool.error handler could clean up entries, if such an event exists in the SDK.

Title fallback chain for session.next.tool.success is bash-specific

The title resolution at lines 201-206 correctly falls back from structured.titleinput.title → bash command string → tool name. For non-bash tools with no structured.title and no input.title, the fallback is the tool name itself — a sensible default. No issue here, just documenting the logic.

Double waitForEventProcessorShutdown drain still present

retry.ts lines 249 and 273. Same longstanding tracking note.

Missing tests

The new handlers are well-covered:

  • 'renders visible stdout text from message.part.delta with string delta when field is text'
  • 'renders visible stdout text from message.part.delta events flushed on session.idle'
  • 'renders visible stdout text from sync session.next.text.delta events flushed on session.idle'
  • 'renders visible stdout text from sync session.next.text.delta with object-shaped delta'
  • 'renders visible stdout tool execution from V2 sync session.next.tool.called + success events'
  • 'detects PR artifacts from V2 sync session.next.tool.called + success for gh pr create'

No missing tests.

Risk assessment (LOW)

The new handlers are additive — they add output rendering paths that were silently missing. The existing message.part.updated handler continues to serve the non-V2 SDK path unchanged. The pendingToolCalls map is local to each processEventStream invocation and is garbage collected when the stream ends. No state is shared across invocations.


Run Summary
Field Value
Event pull_request
Repository fro-bot/agent
Run ID 25735101670
Cache hit
Session ses_1e76768d7ffelY0uYGJ4V1mBQE

@marcusrbrown marcusrbrown merged commit 4354593 into main May 12, 2026
10 checks passed
@marcusrbrown marcusrbrown deleted the fix/scheduled-agent-watchdog branch May 12, 2026 12:56
@fro-bot fro-bot mentioned this pull request May 12, 2026
48 tasks
marcusrbrown pushed a commit that referenced this pull request May 12, 2026
* build(deps): update dependency anomalyco/opencode to v1.14.48 (#601)

Co-authored-by: fro-bot[bot] <109017866+fro-bot[bot]@users.noreply.github.com>

* docs(wiki): update project wiki (#605)

* build(deps): update dependency oh-my-openagent to v3.17.15 (#604)

Co-authored-by: fro-bot[bot] <109017866+fro-bot[bot]@users.noreply.github.com>

* build(deps): update dependency @fro.bot/systematic to v2.12.2 (#597)

Co-authored-by: fro-bot[bot] <109017866+fro-bot[bot]@users.noreply.github.com>

* fix(deps): update dependency @aws-sdk/client-s3 to v3.1045.0 (#600)

Co-authored-by: fro-bot[bot] <109017866+fro-bot[bot]@users.noreply.github.com>

* fix(agent): track streamed activity from event sessions (#611)

* fix(agent): track streamed activity from event session envelope

* fix(agent): recognize streaming deltas as activity

* test(agent): cover stream activity session filtering

* fix(agent): handle sync stream events for watchdog

* fix(agent): wait for opencode session completion

* fix(agent): attach v2 wait to opencode server

* fix(agent): require activity before session completion

* fix(agent): require stream activity before completion

* fix(agent): consume event stream before prompting

* fix(agent): consume opencode activity before completion

* fix(agent): require terminal opencode completion signal

* fix(agent): stabilize opencode message completion

* fix(agent): restore opencode stream output

Render assistant text and tool execution lines from current OpenCode stream event shapes while preserving the terminal completion gate.

---------

Co-authored-by: fro-bot[bot] <109017866+fro-bot[bot]@users.noreply.github.com>
Co-authored-by: Fro Bot <80104189+fro-bot@users.noreply.github.com>
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