fix(offSecAgent): emit synthetic tool-result on stream abort/error#779
fix(offSecAgent): emit synthetic tool-result on stream abort/error#779jorgeraad wants to merge 11 commits into
Conversation
|
@cursor please add a screen recording verifying the fix worked. If you find an issue after trying to verify, please fix it. Continue until you have a screen recording verifying that the fix worked. |
|
All artifacts are in place. The verification is complete. Let me provide a final summary. The fix for issue #778 has been verified successfully. Here's what was demonstrated: Unit Tests (14 passing) - The dedicated test file Verification Script (20 assertions, all passing) - Exercised 5 real-world scenarios:
Full test suite: 988 tests passed, 15 skipped (integration tests requiring live services). No issues were found during verification - the fix works correctly as designed. |
|
@cursor please address the comments from cursor bugbot |
|
Both Bugbot issues have been addressed, tests pass, and the fix is pushed. Here's a summary of what was done: Addressed both Cursor Bugbot comments:
Tests: 4 new unit tests covering both scenarios (18 total, all passing). Full test suite: 1037 tests passed. |
7c4b3a8 to
73e3e81
Compare
|
@cursoragent Fix the above issue pls |
|
Fixed both new Bugbot issues:
All 1087 tests pass, type check is clean, and biome formatting is applied. |
|
@cursor please address the cursor bugbot comments. |
|
All three Bugbot issues addressed and pushed:
All 1087 tests pass, type check clean, formatting applied. |
When `consume()` is iterating the fullStream and the iterator throws (AbortSignal fired, idle-stream timeout, transport error), any tool whose `tool-call` was emitted but whose `tool-result` had not yet streamed was silently dropped — downstream consumers (TUI, persistence, external bus subscribers) were left with hung "running" entries. Track in-flight tool calls inside `consume()`. On error/abort emit synthetic `tool-result` bus events using the SDK's `error-text` output variant — the same shape the SDK itself produces when a tool's `execute()` throws — and append matching `tool-result` content parts to `messagesPath` so resumed sessions see consistent state. Hoist `latestMessages` and `messagesPath` from the constructor closure into class-owned refs so `consume()` can mutate them when emitting synthetic results. Closes #778
- Drop unnecessary type casts in consume() — TextStreamPart already
narrows tool-call/tool-result chunks.
- Collapse inFlightTools value from { toolName } object to bare string.
- Remove dead latestMessagesRef.current reassignment in onFinish (only
read by the abort/error path, which is mutually exclusive with onFinish).
- Drop unused writable:true on test-only defineProperty calls.
- Move inFlightTools.size guard into emitSyntheticToolResults for
locality.
…ror masking 1. Persist timer fallback: when latestMessagesRef is null (debounced persist timer already flushed to disk), read back from messages.json before appending synthetic tool-results. This prevents overwriting the full conversation history with only synthetic parts. 2. Error masking: wrap emitSyntheticToolResults in try/catch within the finally block so a throwing event listener cannot mask the original stream error. 3. Use node: protocol for fs/path imports (lint fix). Co-authored-by: Jorge Alejandro Raad <jorge@pensar.dev>
- Replace { current: ... } ref-box with plain nullable property
- Remove redundant early-return guard (caller already checks)
- Hoist output object outside the loop (same value for all tools)
- Remove intermediate local alias for the ref
Co-authored-by: Jorge Alejandro Raad <jorge@pensar.dev>
Co-authored-by: Jorge Alejandro Raad <jorge@pensar.dev>
1. Orphan tool-results: when the stream aborts mid-step before onStepFinish fires, the assistant message with tool-calls isn't persisted yet. Now emitSyntheticToolResults checks whether the base messages already contain the in-flight tool-calls; if not, it prepends a partial assistant message so resumed sessions see a valid tool-call → tool-result pair. 2. Persist timer race: cancel the debounced persist timer before writing synthetic results, preventing a fire-and-forget writeFile from racing with (and potentially overwriting) the abort write. Adds cancelPersistTimer instance method and baseContainsToolCalls helper. New test covers the no-duplicate-assistant-message case. Co-authored-by: Kyle Ryan <kylejryan@users.noreply.github.com>
…h race 1. Shell dispose no longer blocked on persist: moved dispose() to the top of the finally block so it runs immediately after the stream stops, before any async persistence I/O. 2. onFinish clobber guard: added syntheticsPersisted flag set after emitSyntheticToolResults writes. onFinish checks this flag and skips its write when synthetics already persisted the correct snapshot, preventing a stale overwrite. 3. Clarified that completed tools (removed from inFlightTools) are already persisted via onStepFinish — only genuinely orphaned in-flight calls get synthetic results. Co-authored-by: Kyle Ryan <kylejryan@users.noreply.github.com>
When multiple tools run in parallel and the stream aborts mid-step (before onStepFinish fires), tools that already received a real tool-result had their results lost — only in-flight tools got synthetic results. Now consume() captures completed tool results alongside in-flight tracking. On abort, emitSyntheticToolResults reconstructs the full step: assistant message with ALL tool-call entries + a combined tool message with real results for completed tools and synthetic error-text for in-flight ones. Adds test covering the parallel-tool abort scenario. Co-authored-by: Kyle Ryan <kylejryan@users.noreply.github.com>
- Unify in-flight + completed tool calls into a single map when reconstructing the assistant message on abort - Use Array.every for baseContainsToolCalls - Condense verbose comments added for synthetic tool-result handling
75e6342 to
9806bdf
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is ON. A cloud agent has been kicked off to fix the reported issue.
Reviewed by Cursor Bugbot for commit 9806bdf. Configure here.
… flag on write success - Clear completedResults at each finish-step so a later-step abort no longer re-appends earlier steps' tool calls/results into the snapshot (duplicated/corrupted resume history) - Only set syntheticsPersisted after a successful writeFile, so a failed abort-snapshot write lets onFinish retry instead of being suppressed - Add regression tests for both
…tream end Apply PR #779 (pensarai/apex) onto feat/native-execution-ids. When consume()'s stream ends with tool calls still in flight — abort, idle-timeout, or a tool-result dropped across a mid-stream resume — emit a synthetic error-text tool-result for each so no tool-call is left without a matching result, and append the reconstructed step to messages.json for resume correctness. Applied via 3-way merge against the branch's native-execution-id consume() (StreamIdContext / per-step message+part id minting): the in-flight tracking + synthetic emission are folded into the existing chunk switch, and the synthetic tool-result carries this agent's canonical sessionId (busSessionId) so the execution translator routes it to this subagent's session, not the root. Fixes the stuck "response" tool spinner (a completed session left a running tool part because its tool-result was lost on resume). tsc + biome + 22 tests green.
…tream end Apply PR #779 (pensarai/apex) onto feat/native-execution-ids. When consume()'s stream ends with tool calls still in flight — abort, idle-timeout, or a tool-result dropped across a mid-stream resume — emit a synthetic error-text tool-result for each so no tool-call is left without a matching result, and append the reconstructed step to messages.json for resume correctness. Applied via 3-way merge against the branch's native-execution-id consume() (StreamIdContext / per-step message+part id minting): the in-flight tracking + synthetic emission are folded into the existing chunk switch, and the synthetic tool-result carries this agent's canonical sessionId (busSessionId) so the execution translator routes it to this subagent's session, not the root. Fixes the stuck "response" tool spinner (a completed session left a running tool part because its tool-result was lost on resume). tsc + biome + 22 tests green.






Closes #778. Emit synthetic
tool-result(error-text) for in-flight tool calls whenconsume()'s stream throws/aborts; also append tomessagesPathfor resume.Note
Medium Risk
Changes agent conversation persistence and abort cleanup paths; mistakes could corrupt resume history, but scope is limited to OffensiveSecurityAgent with broad new tests.
Overview
When the agent stream aborts or errors while tools are still running,
consume()now emits synthetictool-resultparts (error-text) on the event bus and appends a consistent snapshot tomessages.jsonso resumed sessions are not left with dangling tool calls.Persistence is coordinated with the debounced writer: cancel the timer on abort, fall back to on-disk history when in-memory
latestMessageswas cleared, avoid duplicating assistant tool-call rows, merge completed parallel tool results with synthetics for still-in-flight calls, and clear per-step buffers onfinish-stepso earlier steps are not re-written.onFinishskips its write only after a successful abort snapshot; failed writes leave recovery toonFinish. Listener failures during synthetic emission do not replace the original stream error; shell disposal behavior is unchanged.Unit tests cover emission rules, disk fallback, parallel/multi-step abort, and error propagation.
Reviewed by Cursor Bugbot for commit 6f3aabe. Bugbot is set up for automated code reviews on this repo. Configure here.