Skip to content

Commit 70e7290

Browse files
author
Yogthos
committed
fix(ui): track last_tool_call_id; paint fresh chamber on parallel-result mismatch
Closes dirge-jzj. The UI's chamber state model was single-slot (one `tool_chamber_open: bool` + one `last_tool_name: Option<String>`). Under parallel tool execution — the default per `agent_loop/types.rs:402` since this matches pi — the `execute_tool_calls_parallel` loop fires `ToolExecutionStart` for ALL tool calls in a turn back-to-back before any `ToolExecutionEnd`. Each new ToolCall passively closes the prior chamber via the close at line 2137. ToolResults arrive in completion order, which often differs from call order. Symptoms before this fix (after 7403792's passive-close change): - `╭─ READ ─ "/foo" ─╮ ╰────╯` empty chamber for the displaced call - `↳ first_line` trailer painted below an unrelated chamber when the displaced call's result finally arrives - Pre-7403792 the same race produced false "⚠ tool denied · aborted · no result" wording instead of empty chambers; same root cause, just a different visible symptom. Fix: track `last_tool_call_id: Option<String>` alongside the existing `last_tool_name`. Set at ToolCall, cleared at ToolResult end and chamber-close-during-result paths. At ToolResult, if the result's id doesn't match `last_tool_call_id`, the chamber on screen belongs to a sibling call — paint a fresh complete chamber for THIS id below the current scroll position (close current passive, look up name+args from `tool_calls_buf`, paint TOP, set chamber_open=true). The existing render paths (a/b/c) then handle the body for the newly-opened chamber. Each parallel ToolResult lands in its own correctly-labeled frame, rendered in completion order. The display can appear out of source order when tools finish at different speeds — that's accurate to what happened. Trade-off: deny flow with parallel calls + alert response now paints a fresh chamber containing the deny error text below the existing `↳ denied: tool args` trailer line. Slightly redundant but informative. Threading call_id through the ask flow would let us suppress the redundancy; deferred to a follow-up since the parallel-deny intersection is rare. The id-matches case (sequential / single-tool-per-turn, which is the common path) falls through to existing path (a) unchanged. Tests: existing 192 ui:: tests still pass. The multi-chamber behavior is event-stream-sequenced and not easily covered by the current unit-test harness (which tests chamber close helpers in isolation, not full event loops); integration test is a follow-up.
1 parent 2124feb commit 70e7290

2 files changed

Lines changed: 80 additions & 1 deletion

File tree

.beads/issues.jsonl

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
{"_type":"issue","id":"dirge-ojn","title":"Perm M4: flip unmatched-tool default from Allow to Ask","description":"Today src/permission/checker.rs:103 has default_action = Action::Allow. Anything not explicitly configured falls through to Allow without prompting — a security gap especially for write/edit/apply_patch which currently have NO default rules installed at all (mismatch with bash which has default_bash_rules, mcp_tool which defaults Ask since 27bd70a).\n\nFlip default to Ask. Add builtin-allow list for safe read-only tools (port maki's BUILTIN_ALLOW_RULES at permissions.rs:16-24, adapted for dirge's tool set: read/glob/grep/list_dir/list_symbols/find_definition/find_callers/find_callees/get_symbol_body/repo_overview).\n\nwrite/edit/apply_patch/bash/webfetch/websearch/task/skill/memory all become Ask by default unless explicitly allowlisted. Document the migration in README + CHANGELOG. Users with existing configs unaffected — only the no-config baseline changes.\n\nAdd --yolo CLI flag (currently only via config). Maps to set the global allow_all atomic (port maki's PermissionManager::toggle_yolo at permissions.rs:219-222).\n\nDepends on: dirge-{M2}, dirge-{M3}","status":"open","priority":1,"issue_type":"feature","owner":"yogthos@gmail.com","created_at":"2026-05-23T14:25:47Z","created_by":"Yogthos","updated_at":"2026-05-23T14:25:47Z","dependencies":[{"issue_id":"dirge-ojn","depends_on_id":"dirge-6ab","type":"blocks","created_at":"2026-05-23T10:25:53Z","created_by":"Yogthos","metadata":"{}"},{"issue_id":"dirge-ojn","depends_on_id":"dirge-cep","type":"blocks","created_at":"2026-05-23T10:25:52Z","created_by":"Yogthos","metadata":"{}"}],"dependency_count":2,"dependent_count":0,"comment_count":0}
33
{"_type":"issue","id":"dirge-01s","title":"Perm M1: single chokepoint (port maki's enforce shape)","description":"Refactor dirge's 3 permission entry points (check_perm, check_perm_path, check_perm_path_resolve in src/agent/tools/mod.rs) into a single chokepoint patterned after maki's PermissionManager::enforce (maki-agent/src/permissions.rs:283). One function, takes (tool, scope), routes internally based on tool category. No behavior change for users — pure refactor with all existing callers updated. Behind-the-scenes the new fn calls the same PermissionChecker logic.\n\nRef: maki-agent/src/permissions.rs:283-350 — port the signature shape (async, returns Result\u003c(), PermissionError\u003e, takes \u0026EventSender + user_response_rx + cancel for UI integration). Adapt to dirge's existing AskSender + PermCheck.","status":"open","priority":1,"issue_type":"feature","owner":"yogthos@gmail.com","created_at":"2026-05-23T14:25:25Z","created_by":"Yogthos","updated_at":"2026-05-23T14:25:25Z","dependency_count":0,"dependent_count":2,"comment_count":0}
44
{"_type":"issue","id":"dirge-cep","title":"Perm M2: unified rule schema (port maki's TOML shape via JSON)","description":"Replace dirge's per-tool field PermissionConfig (separate Option\u003cToolPerm\u003e per built-in tool) with maki's uniform shape: { tool_name: { allow: [patterns], deny: [patterns] } }. Single PermissionRule struct (maki-config/src/lib.rs:268-273). Keep existing JSON config (dirge uses JSON not TOML), but flatten the schema. Add a dual-read path so old config.json files (existing user configs in the wild) still parse — log a deprecation warning and auto-migrate on save. Migration test required.\n\nDepends on: dirge-{M1}","status":"open","priority":1,"issue_type":"feature","owner":"yogthos@gmail.com","created_at":"2026-05-23T14:25:25Z","created_by":"Yogthos","updated_at":"2026-05-23T14:25:25Z","dependencies":[{"issue_id":"dirge-cep","depends_on_id":"dirge-01s","type":"blocks","created_at":"2026-05-23T10:25:50Z","created_by":"Yogthos","metadata":"{}"}],"dependency_count":1,"dependent_count":1,"comment_count":0}
5-
{"_type":"issue","id":"dirge-jzj","title":"UI: per-tool-call-id chamber state (parallel tool execution)","description":"The UI tracks chamber state via a single bool tool_chamber_open + Option\u003cString\u003e last_tool_name. Under parallel tool execution (the default per agent_loop/types.rs:402), multiple ToolExecutionStart events fire before any ToolExecutionEnd. Subsequent ToolCalls close prior chambers prematurely (now via passive close after 7403792, previously with false 'denied' wording). ToolResults that arrive after a newer ToolCall's chamber opens land as out-of-place '↳ trailers' below the wrong chamber. Fix: chamber state keyed by tool_call_id (HashMap), each in-flight tool gets its own chamber frame, the abort/passive close functions take an id.","status":"open","priority":1,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-23T14:25:11Z","created_by":"Yogthos","updated_at":"2026-05-23T14:25:11Z","dependency_count":0,"dependent_count":0,"comment_count":0}
5+
{"_type":"issue","id":"dirge-jzj","title":"UI: per-tool-call-id chamber state (parallel tool execution)","description":"The UI tracks chamber state via a single bool tool_chamber_open + Option\u003cString\u003e last_tool_name. Under parallel tool execution (the default per agent_loop/types.rs:402), multiple ToolExecutionStart events fire before any ToolExecutionEnd. Subsequent ToolCalls close prior chambers prematurely (now via passive close after 7403792, previously with false 'denied' wording). ToolResults that arrive after a newer ToolCall's chamber opens land as out-of-place '↳ trailers' below the wrong chamber. Fix: chamber state keyed by tool_call_id (HashMap), each in-flight tool gets its own chamber frame, the abort/passive close functions take an id.","status":"closed","priority":1,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-23T14:25:11Z","created_by":"Yogthos","updated_at":"2026-05-23T14:35:50Z","started_at":"2026-05-23T14:27:32Z","closed_at":"2026-05-23T14:35:50Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
66
{"_type":"issue","id":"dirge-5uv","title":"H-batch1-2: history up/down byte-offset panic risk on multi-byte input","description":"ui/input.rs:988-1023 — col = self.cursor - line_start is byte distance; self.cursor = (pos + col).min(target_line_end) can land mid-codepoint when previous line has multi-byte chars. Subsequent replace_range/slice panics. Convert column to char index then back to byte offset on target line.","status":"closed","priority":1,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-23T02:34:57Z","created_by":"Yogthos","updated_at":"2026-05-23T03:09:57Z","closed_at":"2026-05-23T03:09:57Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
77
{"_type":"issue","id":"dirge-le5","title":"H-batch1-1: bash inherits all env vars including API keys","description":"tools/bash.rs:208-320 no .env_clear(). Sensitive vars (OPENROUTER_API_KEY, EXA_API_KEY, PARALLEL_API_KEY, ANTHROPIC_API_KEY, etc.) flow to every bash child. Use .env_clear() + curated allowlist (PATH, HOME, USER, LANG, TERM, etc.). See pi bash-executor.ts.","status":"closed","priority":1,"issue_type":"bug","owner":"yogthos@gmail.com","created_at":"2026-05-23T02:34:57Z","created_by":"Yogthos","updated_at":"2026-05-23T03:09:57Z","closed_at":"2026-05-23T03:09:57Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}
88
{"_type":"issue","id":"dirge-846","title":"C8: /cd, /worktree, /wt-exit, /prompt, /regen-prompts silently drop LSP","description":"ui/slash.rs:1290, 1300, 1763 + analogues in mod.rs — every build_agent call after these commands passes None for lsp_manager. User silently loses LSP for the rest of the session after first /cd. Re-thread lsp_manager (and re-root for /cd/worktree). opencode cd handler re-roots LSP.","status":"closed","priority":1,"issue_type":"bug","assignee":"Yogthos","owner":"yogthos@gmail.com","created_at":"2026-05-23T02:34:56Z","created_by":"Yogthos","updated_at":"2026-05-23T03:09:57Z","started_at":"2026-05-23T02:45:40Z","closed_at":"2026-05-23T03:09:57Z","close_reason":"Closed","dependency_count":0,"dependent_count":0,"comment_count":0}

src/ui/mod.rs

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -689,6 +689,28 @@ pub async fn run_interactive(
689689
let mut was_reasoning = false;
690690
let mut todo_tools_enabled = false;
691691
let mut last_tool_name: Option<String> = None;
692+
// The tool_call_id of the in-flight chamber (or the most-recent
693+
// chamber that was closed without a matching ToolResult yet). Lets
694+
// the ToolResult handler distinguish "this result belongs to the
695+
// currently-painted chamber" (sequential / single-tool case) from
696+
// "this result belongs to an earlier call whose chamber was
697+
// displaced by a parallel sibling" (the dirge-jzj scenario).
698+
//
699+
// When parallel tool execution is enabled (the default per
700+
// agent_loop/types.rs), the LLM emits N ToolCalls back-to-back and
701+
// the agent_loop's `execute_tool_calls_parallel` fires
702+
// ToolExecutionStart for ALL of them before any ToolExecutionEnd.
703+
// Each new ToolCall passively closes the prior chamber. Completion
704+
// order is whatever finishes first, so ToolResults arrive
705+
// arbitrarily — most never match the currently-open chamber's id.
706+
// Without this tracker, mismatched results either landed inside
707+
// the wrong chamber (path a, body painted under another tool's
708+
// banner) or as a `↳ first_line` trailer below an unrelated chamber
709+
// (path b). The fix: when a result's id doesn't match the open
710+
// chamber, paint a fresh complete chamber for THIS id below the
711+
// current scroll position. Completion-order rendering, each tool
712+
// gets its own correctly-labeled frame.
713+
let mut last_tool_call_id: Option<String> = None;
692714
// Tracks whether a tool chamber TOP has been drawn but no matching
693715
// BOTTOM has been written yet. Used by the ask/alert handler to
694716
// close the in-flight chamber BEFORE rendering the ALERT box.
@@ -2136,6 +2158,7 @@ pub async fn run_interactive(
21362158
// `close_tool_chamber_if_open`.
21372159
close_tool_chamber_passive(&mut renderer, &mut last_tool_name, &mut tool_chamber_open)?;
21382160
last_tool_name = Some(name.to_string());
2161+
last_tool_call_id = Some(id.to_string());
21392162
if agent_line_started {
21402163
renderer.write_line("", Color::White)?;
21412164
agent_line_started = false;
@@ -2201,6 +2224,61 @@ pub async fn run_interactive(
22012224
let max_chars = cfg.resolve_tool_result_max_chars();
22022225
let show_diff = cfg.resolve_show_edit_diff();
22032226

2227+
// dirge-jzj: if the chamber on screen belongs to a
2228+
// DIFFERENT tool call (parallel-execution race
2229+
// where ToolResults arrive out of order, or a
2230+
// newer ToolCall's TOP displaced this result's
2231+
// chamber before the result arrived), paint a
2232+
// fresh complete chamber for THIS id below the
2233+
// current scroll position. Lets each result land
2234+
// in its own correctly-labeled frame regardless
2235+
// of completion order. The id-matches case (the
2236+
// common sequential path) falls through to the
2237+
// existing render paths below.
2238+
if !id.is_empty()
2239+
&& last_tool_call_id.as_deref() != Some(id.as_str())
2240+
&& show_details
2241+
{
2242+
// Close whatever chamber is on screen first,
2243+
// then paint a fresh TOP for this id. We
2244+
// don't reuse the ToolCall handler's TOP-
2245+
// paint code path because that fires from a
2246+
// different event; the body of the new
2247+
// chamber will land via path (a) below now
2248+
// that tool_chamber_open=true.
2249+
if tool_chamber_open {
2250+
close_tool_chamber_passive(
2251+
&mut renderer,
2252+
&mut last_tool_name,
2253+
&mut tool_chamber_open,
2254+
)?;
2255+
}
2256+
let (resolved_name, resolved_args) = tool_calls_buf
2257+
.iter()
2258+
.rev()
2259+
.find(|e| e.id == id.as_str())
2260+
.map(|e| (e.name.to_string(), e.args.clone()))
2261+
.unwrap_or_else(|| (String::new(), serde_json::Value::Null));
2262+
if !resolved_name.is_empty() {
2263+
let upper = resolved_name.to_ascii_uppercase();
2264+
let raw_value =
2265+
format_tool_banner_value(&resolved_name, &resolved_args);
2266+
let raw_value = sanitize_output(&raw_value).into_string();
2267+
let (frame_w, _) = chamber_widths(&renderer);
2268+
let header =
2269+
fit_banner_header(&upper, &raw_value, frame_w);
2270+
renderer.write_line("", Color::White)?;
2271+
renderer.write_line(&header, c_tool())?;
2272+
tool_chamber_open = true;
2273+
last_tool_name = Some(resolved_name);
2274+
last_tool_call_id = Some(id.to_string());
2275+
}
2276+
// If the call wasn't in tool_calls_buf (id
2277+
// unknown — shouldn't happen post-ToolCall
2278+
// but defensive), fall through to path (b)
2279+
// trailer; we have no banner to paint.
2280+
}
2281+
22042282
// on-tool-end is also fired by HookedToolDyn so the
22052283
// host doesn't re-dispatch it here.
22062284

@@ -2410,6 +2488,7 @@ pub async fn run_interactive(
24102488
// Clear after consuming so a future stray ToolResult
24112489
// can't be coloured with a stale tool name.
24122490
last_tool_name = None;
2491+
last_tool_call_id = None;
24132492
}
24142493
AgentEvent::Done { response, tokens, cost } => {
24152494
was_reasoning = false;

0 commit comments

Comments
 (0)