feat: preserve partial assistant reply on Ctrl+C / Esc abort (opencode parity) - #65
Merged
Merged
Conversation
Match opencode's `finalizeInterruptedAssistant` pattern. When the user aborts mid-stream: - Previously: `task.abort()` cancelled the tokio task. Tokens that had already streamed were visible on screen but were NOT in the session — next turn's LLM context omitted everything the agent had been saying. Frustrating for "wait, I just need to clarify" cases. - Now: before calling `h.abort()`, the in-flight `response_buf` is trimmed, appended with `\n\n[interrupted by user (Ctrl+C)]` (or `(Esc)`), and saved to the session as the assistant's reply via `session.add_message(Assistant, ...)`. Next turn's LLM context includes the partial, so the agent can pick up where it left off rather than starting from scratch. The "partial reply preserved" suffix is also added to the "interrupted" chat banner so the user knows their typing wasn't wasted. This matches opencode's behavior in `packages/opencode/src/session/prompt.ts` where the `Effect.onInterrupt(() => finalizeInterruptedAssistant)` handler marks the streaming message as `aborted: true` and updates the session. opencode uses `MessageV2.fromError(..., aborted: true)` to annotate the message structurally; dirge's `SessionMessage` is content-only, so the marker lives in the text trailer. ## Implementation New `capture_partial_on_abort(&mut buf, &mut session, why) -> bool` helper in `src/ui/mod.rs`: - Returns `true` if a partial was stashed, `false` if nothing meaningful had streamed yet. - Whitespace-only partials are treated as no-op (no empty "[interrupted]" messages from misclicks). - Clears `response_buf` regardless so the next turn starts fresh. Both abort sites use it: - `Ctrl+C`/`Ctrl+D` while running (mod.rs ~717) - `Esc` while running (mod.rs ~878) ## Tests Three new tests in `ui::tests`: - `capture_partial_on_abort_stashes_partial_with_trailer`: real partial → saved with trailer + buf cleared. - `capture_partial_on_abort_noop_on_empty_buf`: empty buf → no session change. - `capture_partial_on_abort_noop_on_whitespace_only`: whitespace- only buf → no session change. ## Test plan - [x] `cargo test --features plugin` -> 616 pass (3 new, 0 fail). - [x] `cargo build --all-features` -> compiles.
yogthos
added a commit
that referenced
this pull request
May 21, 2026
…t tool count + token parity (#66) TDD: tests first, all 6 new tests failed initially, then implemented. ## Round A — hook error sanitize + dedup Two bugs in PR #64's hook-error notification path: 1. Multi-line / tab-containing Janet errors broke the `level\tmsg\n` wire format. A `(error "trace\n at file:42")` produced multiple malformed notification entries (one per source line), the first with truncated content and the rest filtered out as malformed. drain_notifications splits raw on `\n` per entry and on first `\t` per level/msg — both control chars now sanitized in Janet before push. 2. A buggy `on-message-update` hook (fires ~every 16 streamed tokens) flooded the chat with thousands of identical "[plugin] hook X.Y errored: ..." banners during a single long response. Now deduped: two new Janet vars track the most-recent sanitized error msg + a consecutive-repeat count; identical errors just bump the count instead of pushing. On drain, any outstanding count is flushed as a "(repeated N times)" summary entry. Implementation: - `harness/sanitize-hook-err` (new) normalizes `\t` → space and `\n`/`\r\n` → ` | `. Distinct hook errors stay separate; only consecutive identical ones collapse. Wrote with explicit nested `string/replace-all` calls — Janet's `->` threading macro would pass the string in the wrong arg position (string/replace-all expects `(patt subst str)`). - `harness/push-hook-err` (new) does the dedup check using `harness-last-hook-err-msg` + `harness-last-hook-err-count` module-level vars. - `drain_notifications` flushes pending dedup count before reading the notif list so a 50× repeat shows up as a single "(repeated 50 times)" entry in the next drain. - The catch arm in `dispatch` calls these instead of appending directly. Wrapped in explicit `(do ...)` for Janet's single-form catch-body semantics. ## Round B — partial-on-abort trailer notes tool calls PR #65 saved the streamed assistant text on abort but didn't indicate that tool calls had also run in the same turn (whose results aren't in `response_buf` — only Token events accumulate there). The LLM on next turn would see the partial as a definitive "this was my reply" and could re-run side-effecting tools. `capture_partial_on_abort` now takes a `tool_calls_in_turn: u32` parameter. When non-zero, the trailer reads: [interrupted by user (Ctrl+C); 2 tool calls ran in this turn — results not preserved] Singular case ("1 tool call ran") uses the right noun. UI loop tracks `tool_calls_this_run: u32`, incremented on every `AgentEvent::ToolCall`, reset on `Done`/`Interjected`/both abort sites (since each marks the end of one agent run). ## Round C — token-accumulator parity on abort `Done` and `Interjected` branches both update `session.total_tokens` alongside the message add. The abort path didn't — made aborted turns look like zero-token contributions in the placeholder field. Fixed with an explicit `session.total_tokens.saturating_add(Session::estimate_tokens(&stashed))` inside `capture_partial_on_abort`. Both fields stay under the `TODO(cost-tracking)` comment but at least they're now internally consistent. ## Test plan - [x] 6 new tests (3 plugin dispatch + 3 capture_partial_on_abort + 2 updated existing tests with new signature). - [x] `cargo test --features plugin` -> 622 pass, 0 fail. - [x] `cargo build --all-features` -> compiles. ## Skipped (observational, not bugs) - #3 print/loop mode notifications never drained: print mode is non-interactive; tracing::warn (via `--verbose`) is the right channel. - #4 Janet `err` non-string-coerced: `(string ...)` calls Janet's `tostring` which handles any value type. Documented behavior. - #7 markdown rendering of `[interrupted by user (Ctrl+C)]`: not a link by pulldown-cmark's rules; visually acceptable inline. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…ode#65) Match opencode's `finalizeInterruptedAssistant` pattern. When the user aborts mid-stream: - Previously: `task.abort()` cancelled the tokio task. Tokens that had already streamed were visible on screen but were NOT in the session — next turn's LLM context omitted everything the agent had been saying. Frustrating for "wait, I just need to clarify" cases. - Now: before calling `h.abort()`, the in-flight `response_buf` is trimmed, appended with `\n\n[interrupted by user (Ctrl+C)]` (or `(Esc)`), and saved to the session as the assistant's reply via `session.add_message(Assistant, ...)`. Next turn's LLM context includes the partial, so the agent can pick up where it left off rather than starting from scratch. The "partial reply preserved" suffix is also added to the "interrupted" chat banner so the user knows their typing wasn't wasted. This matches opencode's behavior in `packages/opencode/src/session/prompt.ts` where the `Effect.onInterrupt(() => finalizeInterruptedAssistant)` handler marks the streaming message as `aborted: true` and updates the session. opencode uses `MessageV2.fromError(..., aborted: true)` to annotate the message structurally; dirge's `SessionMessage` is content-only, so the marker lives in the text trailer. ## Implementation New `capture_partial_on_abort(&mut buf, &mut session, why) -> bool` helper in `src/ui/mod.rs`: - Returns `true` if a partial was stashed, `false` if nothing meaningful had streamed yet. - Whitespace-only partials are treated as no-op (no empty "[interrupted]" messages from misclicks). - Clears `response_buf` regardless so the next turn starts fresh. Both abort sites use it: - `Ctrl+C`/`Ctrl+D` while running (mod.rs ~717) - `Esc` while running (mod.rs ~878) ## Tests Three new tests in `ui::tests`: - `capture_partial_on_abort_stashes_partial_with_trailer`: real partial → saved with trailer + buf cleared. - `capture_partial_on_abort_noop_on_empty_buf`: empty buf → no session change. - `capture_partial_on_abort_noop_on_whitespace_only`: whitespace- only buf → no session change. ## Test plan - [x] `cargo test --features plugin` -> 616 pass (3 new, 0 fail). - [x] `cargo build --all-features` -> compiles. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…t tool count + token parity (dirge-code#66) TDD: tests first, all 6 new tests failed initially, then implemented. ## Round A — hook error sanitize + dedup Two bugs in PR dirge-code#64's hook-error notification path: 1. Multi-line / tab-containing Janet errors broke the `level\tmsg\n` wire format. A `(error "trace\n at file:42")` produced multiple malformed notification entries (one per source line), the first with truncated content and the rest filtered out as malformed. drain_notifications splits raw on `\n` per entry and on first `\t` per level/msg — both control chars now sanitized in Janet before push. 2. A buggy `on-message-update` hook (fires ~every 16 streamed tokens) flooded the chat with thousands of identical "[plugin] hook X.Y errored: ..." banners during a single long response. Now deduped: two new Janet vars track the most-recent sanitized error msg + a consecutive-repeat count; identical errors just bump the count instead of pushing. On drain, any outstanding count is flushed as a "(repeated N times)" summary entry. Implementation: - `harness/sanitize-hook-err` (new) normalizes `\t` → space and `\n`/`\r\n` → ` | `. Distinct hook errors stay separate; only consecutive identical ones collapse. Wrote with explicit nested `string/replace-all` calls — Janet's `->` threading macro would pass the string in the wrong arg position (string/replace-all expects `(patt subst str)`). - `harness/push-hook-err` (new) does the dedup check using `harness-last-hook-err-msg` + `harness-last-hook-err-count` module-level vars. - `drain_notifications` flushes pending dedup count before reading the notif list so a 50× repeat shows up as a single "(repeated 50 times)" entry in the next drain. - The catch arm in `dispatch` calls these instead of appending directly. Wrapped in explicit `(do ...)` for Janet's single-form catch-body semantics. ## Round B — partial-on-abort trailer notes tool calls PR dirge-code#65 saved the streamed assistant text on abort but didn't indicate that tool calls had also run in the same turn (whose results aren't in `response_buf` — only Token events accumulate there). The LLM on next turn would see the partial as a definitive "this was my reply" and could re-run side-effecting tools. `capture_partial_on_abort` now takes a `tool_calls_in_turn: u32` parameter. When non-zero, the trailer reads: [interrupted by user (Ctrl+C); 2 tool calls ran in this turn — results not preserved] Singular case ("1 tool call ran") uses the right noun. UI loop tracks `tool_calls_this_run: u32`, incremented on every `AgentEvent::ToolCall`, reset on `Done`/`Interjected`/both abort sites (since each marks the end of one agent run). ## Round C — token-accumulator parity on abort `Done` and `Interjected` branches both update `session.total_tokens` alongside the message add. The abort path didn't — made aborted turns look like zero-token contributions in the placeholder field. Fixed with an explicit `session.total_tokens.saturating_add(Session::estimate_tokens(&stashed))` inside `capture_partial_on_abort`. Both fields stay under the `TODO(cost-tracking)` comment but at least they're now internally consistent. ## Test plan - [x] 6 new tests (3 plugin dispatch + 3 capture_partial_on_abort + 2 updated existing tests with new signature). - [x] `cargo test --features plugin` -> 622 pass, 0 fail. - [x] `cargo build --all-features` -> compiles. ## Skipped (observational, not bugs) - #3 print/loop mode notifications never drained: print mode is non-interactive; tracing::warn (via `--verbose`) is the right channel. - #4 Janet `err` non-string-coerced: `(string ...)` calls Janet's `tostring` which handles any value type. Documented behavior. - dirge-code#7 markdown rendering of `[interrupted by user (Ctrl+C)]`: not a link by pulldown-cmark's rules; visually acceptable inline. Co-authored-by: Yogthos <yogthos@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Matches opencode's
finalizeInterruptedAssistantpattern (packages/opencode/src/session/prompt.ts). Beforetask.abort()fires, the in-flight response buffer is stashed into the session with a[interrupted by user]trailer so the next turn's LLM context picks up where the agent left off. Three new regression tests, 616 pass.