feat(phase 3): structured tool-call persistence with interrupted-state pairing - #71
Merged
Merged
Conversation
…e pairing
Phase 3 of the 6-phase plan. Reference pattern: opencode's
`packages/opencode/src/session/message-v2.ts:310-320, 630-899`
where `ToolPart` carries a `state: pending|running|completed|error`
and is materialized into provider-format tool_use/tool_result
blocks on resume. Anthropic + OpenAI reject orphan tool_use
blocks, so opencode always emits a paired result — even
"[Tool execution was interrupted]" for unfinished calls. dirge
adopts the same shape.
## Problem
Before Phase 3, dirge's `SessionMessage` was (role + text). Tool
calls and results streamed to the UI but were never persisted.
On session resume, `convert_history` emitted assistants as
text-only — the LLM lost all structured knowledge of prior tool
work. It could re-attempt the same bash command, re-read the
same file, or hallucinate that "the file said X" without seeing
the actual prior tool_result.
## Fix
New `ToolCallEntry` + `ToolCallState` types in `src/session/mod.rs`:
```rust
pub struct ToolCallEntry {
pub id: String, // rig's ToolCall.id for correlation
pub name: String, // tool name
pub args: serde_json::Value, // unparsed args
pub state: ToolCallState,
}
pub enum ToolCallState {
Completed { result: String },
Interrupted,
Failed { error: String },
}
```
`SessionMessage` gains `#[serde(default)] tool_calls: Vec<ToolCallEntry>`
— back-compat with old session files (missing field → empty Vec).
`Session::add_message_with_tool_calls(role, content, tool_calls)`
is the new constructor; the existing `add_message` becomes a
thin wrapper that passes an empty Vec.
### Event shape
`AgentEvent::ToolCall` + `AgentEvent::ToolResult` now both carry
an `id: CompactString` (rig's `ToolCall.id` / `ToolResult.id`).
Empty when the provider didn't emit one — the UI falls back to
positional pairing in that case.
### UI capture (src/ui/mod.rs)
- New `tool_calls_buf: Vec<ToolCallEntry>` lives alongside
`response_buf` for the duration of an agent run.
- `AgentEvent::ToolCall` pushes a new entry with
`state: Interrupted` (defensive default — if the user aborts
before the result arrives, the saved state is already correct).
- `AgentEvent::ToolResult` finds the matching entry (by id, or
last-pending if id is empty) and flips state to
`Completed { result }`.
- `AgentEvent::Done` and `AgentEvent::Interjected` call
`add_message_with_tool_calls(Assistant, response,
std::mem::take(&mut tool_calls_buf))` — the run's tool calls
attach to the final assistant message.
- `capture_partial_on_abort` (Ctrl+C, Esc) also drains the
buffer onto the stashed message; any still-Interrupted entries
stay that way, completed ones keep their state. Empty buf
alone no longer counts as a no-op when there are pending tool
calls — the abort still stashes a message with just the
trailer + the tool_calls so the LLM sees the interrupted state.
### convert_history (src/agent/runner.rs)
When an assistant message has `tool_calls`, emit:
1. `Message::Assistant` with content = [text (if any), tool_call(...)...]
built via `OneOrMany::many(...)` / `OneOrMany::one(...)`.
2. `Message::tool_result(id, body)` per call, where:
- `Completed { result }` → body = result text verbatim
- `Interrupted` → "[Tool execution was interrupted]"
- `Failed { error }` → "[Tool error: <msg>]"
Bare assistant messages (no tool_calls) keep the prior simple
`Message::assistant(text)` shape — full backward compatibility
with existing session files.
## Tests
4 new session tests, written failing first:
- `session_message_tool_calls_default_when_field_missing`:
old session JSON without `tool_calls` field deserializes
with empty Vec — back-compat guard.
- `session_message_tool_calls_roundtrip`: write a message with
tool_calls, read back via serde, fields intact.
- `convert_history_emits_tool_use_and_tool_result_blocks`:
builds a session with one bash call → completed; asserts
history has [User, Assistant(text+tool_use), User(tool_result)]
with the id correlation intact.
- `convert_history_pairs_interrupted_tool_calls_with_error_marker`:
Interrupted entry → result body contains "interrupted".
1 new UI test:
- `capture_partial_on_abort_preserves_pending_tool_calls_as_interrupted`:
mix of Interrupted + Completed entries in the buffer →
saved assistant message has both with their states intact.
5 new tests total. 644 pass (was 639), 0 fail, all build
profiles clean.
## Test plan
- [x] `cargo test --features plugin` -> 644 pass.
- [x] `cargo build --all-features` -> compiles.
- [x] `cargo build --no-default-features` -> compiles.
## Up next: Phase 4 (optional, pi-style branch summaries)
Phase 2 drops sibling branches with a notification. If users
report this feels too lossy, Phase 4 would add pi's
`BranchSummaryMessage` pattern — generate per-branch LLM
summaries and persist them as a new message variant so forks
are preserved across compactions instead of discarded.
yogthos
added a commit
that referenced
this pull request
May 21, 2026
) Phase 5 of the plan. No clean equivalent in opencode or pi — both manage allowlist entries only via the interactive permission prompt's "allow always" answer. dirge adds explicit CRUD so users can inspect, manually add, drop one, or clear all entries without editing the session JSON. ## Commands - `/allow` or `/allow list` — show numbered entries - `/allow add <tool> <pattern>` — add a (tool, pattern) entry, e.g. `/allow add bash 'cargo *'` - `/allow remove <idx>` — drop one entry by 0-based index from the `list` output - `/allow clear` — drop all entries The `add` form preserves pattern strings containing spaces by re-deriving the args from the raw `text` slice (rather than relying on the SmallVec `parts[2]` which would get truncated at the first whitespace). Same approach `/cd` uses for paths. ## State sync Both surfaces stay aligned: - `PermissionChecker::session_allowlist` (in-memory, used by `check` / `check_path`) — updated via the existing `add_session_allowlist` and the two new `remove_session_allowlist_at(idx)` + `clear_session_allowlist`. - `Session::permission_allowlist` (persisted to JSON, restored on resume via `load_session_allowlist`) — mirrored manually in the slash handler so save/load round-trips show the user-edited list. Without the session-side mirror, a `/allow add foo bar` would stay in memory for this run but vanish on `-c` resume. Dedup at the session level too so `save()` writes a clean list. ## Tests 3 new permission tests, written failing first: - `remove_session_allowlist_at_returns_removed_entry`: remove(1) on [bash:cargo*, bash:git*, read:/tmp/*] yields Some(("bash","git *")) and the surviving entries shift down. - `remove_session_allowlist_at_out_of_range_returns_none`: remove(99) returns None without panicking; existing entries intact. - `clear_session_allowlist_empties_the_list`: clear() drops all entries. The slash handler itself is not unit-tested (requires the full UI loop fixtures); manual smoke test: ``` $ dirge --restrictive > bash 'echo hi' > (a) allow always > /allow list → [0] bash echo * > /allow add read /tmp/* > /allow list → [0] bash echo * [1] read /tmp/* > /allow remove 0 > /allow list → [0] read /tmp/* > /allow clear > /allow list → empty ``` ## Docs `/help` text gets four new lines covering each subcommand. ## Test plan - [x] `cargo test --features plugin` -> 649 pass (was 646). - [x] `cargo build --all-features` -> compiles. - [x] `cargo build --no-default-features` -> compiles. ## Plan status | Phase | PR | Done | |---|---|---| | 1 | #69 | first-wins block + docs | | 2 | #70 | sibling-branch pruning + notification | | 3 | #71 | structured tool-call persistence | | 4 | #72 | branch summary metadata in /tree | | 5 | this | /allow CRUD | | 6 | — | skipped per user request (cost tracking) | 5 of 6 phases complete; Phase 6 (cost tracking) skipped. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…e pairing (dirge-code#71) Phase 3 of the 6-phase plan. Reference pattern: opencode's `packages/opencode/src/session/message-v2.ts:310-320, 630-899` where `ToolPart` carries a `state: pending|running|completed|error` and is materialized into provider-format tool_use/tool_result blocks on resume. Anthropic + OpenAI reject orphan tool_use blocks, so opencode always emits a paired result — even "[Tool execution was interrupted]" for unfinished calls. dirge adopts the same shape. ## Problem Before Phase 3, dirge's `SessionMessage` was (role + text). Tool calls and results streamed to the UI but were never persisted. On session resume, `convert_history` emitted assistants as text-only — the LLM lost all structured knowledge of prior tool work. It could re-attempt the same bash command, re-read the same file, or hallucinate that "the file said X" without seeing the actual prior tool_result. ## Fix New `ToolCallEntry` + `ToolCallState` types in `src/session/mod.rs`: ```rust pub struct ToolCallEntry { pub id: String, // rig's ToolCall.id for correlation pub name: String, // tool name pub args: serde_json::Value, // unparsed args pub state: ToolCallState, } pub enum ToolCallState { Completed { result: String }, Interrupted, Failed { error: String }, } ``` `SessionMessage` gains `#[serde(default)] tool_calls: Vec<ToolCallEntry>` — back-compat with old session files (missing field → empty Vec). `Session::add_message_with_tool_calls(role, content, tool_calls)` is the new constructor; the existing `add_message` becomes a thin wrapper that passes an empty Vec. ### Event shape `AgentEvent::ToolCall` + `AgentEvent::ToolResult` now both carry an `id: CompactString` (rig's `ToolCall.id` / `ToolResult.id`). Empty when the provider didn't emit one — the UI falls back to positional pairing in that case. ### UI capture (src/ui/mod.rs) - New `tool_calls_buf: Vec<ToolCallEntry>` lives alongside `response_buf` for the duration of an agent run. - `AgentEvent::ToolCall` pushes a new entry with `state: Interrupted` (defensive default — if the user aborts before the result arrives, the saved state is already correct). - `AgentEvent::ToolResult` finds the matching entry (by id, or last-pending if id is empty) and flips state to `Completed { result }`. - `AgentEvent::Done` and `AgentEvent::Interjected` call `add_message_with_tool_calls(Assistant, response, std::mem::take(&mut tool_calls_buf))` — the run's tool calls attach to the final assistant message. - `capture_partial_on_abort` (Ctrl+C, Esc) also drains the buffer onto the stashed message; any still-Interrupted entries stay that way, completed ones keep their state. Empty buf alone no longer counts as a no-op when there are pending tool calls — the abort still stashes a message with just the trailer + the tool_calls so the LLM sees the interrupted state. ### convert_history (src/agent/runner.rs) When an assistant message has `tool_calls`, emit: 1. `Message::Assistant` with content = [text (if any), tool_call(...)...] built via `OneOrMany::many(...)` / `OneOrMany::one(...)`. 2. `Message::tool_result(id, body)` per call, where: - `Completed { result }` → body = result text verbatim - `Interrupted` → "[Tool execution was interrupted]" - `Failed { error }` → "[Tool error: <msg>]" Bare assistant messages (no tool_calls) keep the prior simple `Message::assistant(text)` shape — full backward compatibility with existing session files. ## Tests 4 new session tests, written failing first: - `session_message_tool_calls_default_when_field_missing`: old session JSON without `tool_calls` field deserializes with empty Vec — back-compat guard. - `session_message_tool_calls_roundtrip`: write a message with tool_calls, read back via serde, fields intact. - `convert_history_emits_tool_use_and_tool_result_blocks`: builds a session with one bash call → completed; asserts history has [User, Assistant(text+tool_use), User(tool_result)] with the id correlation intact. - `convert_history_pairs_interrupted_tool_calls_with_error_marker`: Interrupted entry → result body contains "interrupted". 1 new UI test: - `capture_partial_on_abort_preserves_pending_tool_calls_as_interrupted`: mix of Interrupted + Completed entries in the buffer → saved assistant message has both with their states intact. 5 new tests total. 644 pass (was 639), 0 fail, all build profiles clean. ## Test plan - [x] `cargo test --features plugin` -> 644 pass. - [x] `cargo build --all-features` -> compiles. - [x] `cargo build --no-default-features` -> compiles. ## Up next: Phase 4 (optional, pi-style branch summaries) Phase 2 drops sibling branches with a notification. If users report this feels too lossy, Phase 4 would add pi's `BranchSummaryMessage` pattern — generate per-branch LLM summaries and persist them as a new message variant so forks are preserved across compactions instead of discarded. Co-authored-by: Yogthos <yogthos@gmail.com>
allen-munsch
pushed a commit
to allen-munsch/dirge
that referenced
this pull request
Jun 3, 2026
…irge-code#73) Phase 5 of the plan. No clean equivalent in opencode or pi — both manage allowlist entries only via the interactive permission prompt's "allow always" answer. dirge adds explicit CRUD so users can inspect, manually add, drop one, or clear all entries without editing the session JSON. ## Commands - `/allow` or `/allow list` — show numbered entries - `/allow add <tool> <pattern>` — add a (tool, pattern) entry, e.g. `/allow add bash 'cargo *'` - `/allow remove <idx>` — drop one entry by 0-based index from the `list` output - `/allow clear` — drop all entries The `add` form preserves pattern strings containing spaces by re-deriving the args from the raw `text` slice (rather than relying on the SmallVec `parts[2]` which would get truncated at the first whitespace). Same approach `/cd` uses for paths. ## State sync Both surfaces stay aligned: - `PermissionChecker::session_allowlist` (in-memory, used by `check` / `check_path`) — updated via the existing `add_session_allowlist` and the two new `remove_session_allowlist_at(idx)` + `clear_session_allowlist`. - `Session::permission_allowlist` (persisted to JSON, restored on resume via `load_session_allowlist`) — mirrored manually in the slash handler so save/load round-trips show the user-edited list. Without the session-side mirror, a `/allow add foo bar` would stay in memory for this run but vanish on `-c` resume. Dedup at the session level too so `save()` writes a clean list. ## Tests 3 new permission tests, written failing first: - `remove_session_allowlist_at_returns_removed_entry`: remove(1) on [bash:cargo*, bash:git*, read:/tmp/*] yields Some(("bash","git *")) and the surviving entries shift down. - `remove_session_allowlist_at_out_of_range_returns_none`: remove(99) returns None without panicking; existing entries intact. - `clear_session_allowlist_empties_the_list`: clear() drops all entries. The slash handler itself is not unit-tested (requires the full UI loop fixtures); manual smoke test: ``` $ dirge --restrictive > bash 'echo hi' > (a) allow always > /allow list → [0] bash echo * > /allow add read /tmp/* > /allow list → [0] bash echo * [1] read /tmp/* > /allow remove 0 > /allow list → [0] read /tmp/* > /allow clear > /allow list → empty ``` ## Docs `/help` text gets four new lines covering each subcommand. ## Test plan - [x] `cargo test --features plugin` -> 649 pass (was 646). - [x] `cargo build --all-features` -> compiles. - [x] `cargo build --no-default-features` -> compiles. ## Plan status | Phase | PR | Done | |---|---|---| | 1 | dirge-code#69 | first-wins block + docs | | 2 | dirge-code#70 | sibling-branch pruning + notification | | 3 | dirge-code#71 | structured tool-call persistence | | 4 | dirge-code#72 | branch summary metadata in /tree | | 5 | this | /allow CRUD | | 6 | — | skipped per user request (cost tracking) | 5 of 6 phases complete; Phase 6 (cost tracking) skipped. 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.
Phase 3 of the 6-phase plan. Adds
tool_calls: Vec<ToolCallEntry>to SessionMessage (opencode's ToolPart pattern). On session resume,convert_historyre-emits paired tool_use/tool_result blocks to the LLM so it never sees orphan calls. Interrupted state pairs cleanly for Anthropic compatibility. Back-compat via#[serde(default)]. 5 new tests, 644 pass.