v0.9.14 slices: #6213 T4/T5, #6244, #6235

[Hmbown]

Four independent slices, one commit each, each verified separately. Every one was designed by reading the code first — two of them contradict what their issue asked for, and say so.

906f9acfc — #6213 T4: stop re-parsing the whole argument buffer per delta

Every InputJsonDelta ran the full arg_repair ladder over the entire accumulated buffer. Mid-stream the buffer almost never parses strictly, so each delta fell through to the repair stages, which copy the whole buffer — O(n²) per tool call, for a value nothing reads.

Nothing reads tool_state.input between the first delta and finalization. The mirror's one historical consumer (a block whose ContentBlockStop never arrived, #5986) was fixed by the post-loop drain; the mirror was left behind by that fix. Three comments asserting the mirror exists are corrected with it.

5d08e45b2 — #6244: a default role no longer blocks a prompt-only spawn

agent(action=start, prompt=...) with no type/role/profile failed with Fleet member selector `role:general` is ambiguous. request.agent_type defaults to FleetRole::Worker, whose as_str() is "general" — so Codewhale synthesized a selector the caller never wrote and turned its own default into a hard refusal.

Refusing is still right when the caller did name a role. The fix distinguishes the two using agent_type_explicit, which already existed. resolve_pinned_role_profile returns its concrete FleetSelectorError instead of flattening it through anyhow — that deletes a round-trip rather than adding machinery.

Confirmed to fail without the fix: forcing role_was_requested = true reproduces the original refusal (FAILED. 0 passed; 1 failed).

270c02593 — #6213 T5: one buffer set per catalog scan

Reuses buffers across the scan and lowers tool.name once per tool instead of once per query term.

This deliberately does not do what the issue asked. The issue wants a cross-query cache. A sound key does exist (tool_set_identity), but the catalog at the call site is a turn-scoped Vec<Tool> mutated in place mid-turn by replace_runtime_mcp_tools, so pointer identity is not a key — and computing a content digest to avoid a content scan is circular at this size. tool_search runs a handful of times per turn beside a multi-second provider call. A cache would add an invalidation rule to defend and buy nothing measurable. Reasoning is in the commit message.

7c8fe7ec9 — #6235: open a clicked path:line through the suspend path, once

This one is a real user-visible bug. "Open in editor" built its own Command and .spawn()ed it detached while the TUI still held raw mode, the alt screen and mouse capture — and did it in a loop over every line of the clicked cell, so one click on a stack trace could launch several editors all fighting the same tty.

The product call the issue left open is already settled by the code. with_suspended_tui is the single terminal-handoff authority from #6165, and spawn_editor_for_path already wraps it — /hooks edit has been using it all along. The click path was simply never routed through it. So this deletes the duplicate rather than adding an abstraction, which also fixes two latent bugs for free: the duplicate fell back to vim where everything else falls back to vi, and it never shlex-split, so EDITOR="code --wait" could not work.

The one new piece is push_target_args, because spawn_editor_for_path had no way to express a line. With line: None it is byte-identical to the cmd.arg(path) it replaces, which is what keeps /hooks edit unchanged.

Verification (macOS aarch64)

cargo check -p codewhale-tui --all-features --locked --all-targets   clean
cargo fmt --all -- --check                                           clean

-- #6213 T4
test core::engine::tests::complete_tool_call_without_block_stop_still_dispatches ... ok
test core::engine::tests::truncated_tool_call_without_block_stop_never_dispatches ... ok
test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 12807 filtered out

-- #6244
test fleet::worker_runtime::tests::pinned_role_lookup_ignores_builtins_and_rejects_semantic_ambiguity ... ok
test tools::subagent::tests::prompt_only_spawn_survives_an_ambiguous_default_role ... ok
test tools::subagent::tests::saved_role_ambiguity_fails_unless_a_higher_priority_pin_selects_the_route ... ok
test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 12807 filtered out

-- #6213 T5
test result: ok. 13 passed; 0 failed; 0 ignored; 0 measured; 12797 filtered out   (tool_catalog::)

-- #6235
test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 12798 filtered out
  (first_file_line_reference, external_editor, push_target_args)

Not verified here

The #6235 click-to-editor round trip on a real tty. The suspend path itself is unchanged and covered by suspend_tui_child_modes_*, but the actual handoff wants a human at a terminal.

No-Issue: partial work on #6213 (T1/T6/T7 already landed; T4 and T5 here), plus #6244 and #6235 — none of these commits completes its issue, so no closing keyword.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @Hmbown's task in 2m 27s —— View job

---

Review: PR #6271 — v0.9.14 slices (#6213 T4/T5, #6244, #6235)

• Gather context (diff, changed files, PR description)
• Review v0.9.14: Eliminate per-frame/per-delta recomputation and clones in the TUI hot path #6213 T4 (turn_loop.rs mirror removal)
• Review Fleet role selector: ambiguous role blocks agent spawn at the tool boundary (production, cw:768b024a) #6244 (fleet default-role fix)
• Review v0.9.14: Eliminate per-frame/per-delta recomputation and clones in the TUI hot path #6213 T5 (tool_catalog buffer reuse)
• Review bug(tui): clicking a path:line in tool output spawns $EDITOR detached, with the TUI still owning the terminal #6235 (external editor suspend path)
• Post final review

Note: sandboxed CI environment does not allow me to run cargo check/cargo test here (network/build execution requires approval I don't have in this context). Review below is based on reading each commit's diff plus tracing call sites (grep) to confirm no dangling references to renamed/removed symbols across the crate. The PR's own verification output (quoted commit messages) is consistent with what the code does.

---

#6213 T4 — crates/tui/src/core/engine/turn_loop.rs

Removing the mid-stream parse_tool_input mirror is correct. I confirmed by grepping every .input access in the file that nothing reads tool_state.input between ContentBlockStart and finalize_streamed_tool_input — the only reads are the ones the diff itself touches (the removed ContentBlockStop log line and finalize_streamed_tool_input itself). The post-loop drain still runs every unstopped block through the same finalization gate, so the #5986 safety property (a truncated tool call never dispatches with unvalidated partial JSON) is preserved. Good catch that this was genuinely dead work on the hot streaming path.

#6244 — fleet default-role fix

resolve_pinned_role_profile's signature change from anyhow::Result to Result<_, FleetSelectorError> has exactly one production call site (resolve_spawn_route_profile), which is updated; the two existing test call sites (worker_runtime.rs) still work unchanged since FleetSelectorError implements Display/Error via thiserror. The role_was_requested gate correctly only suppresses the ambiguous error, and only when the role came from the untyped default — every other FleetSelectorError variant (and an ambiguous role the caller did name) still propagates as a hard refusal. This is a narrow, well-targeted fix; the new test explicitly reproduces the original failure by forcing role_was_requested = true, matching the "prove the regression fails without the fix" evidence bar.

#6213 T5 — tool_catalog.rs

The ToolSearchScratch reuse is correct and matches the stated scope (no cross-query cache, with a well-reasoned explanation for why one doesn't fit — the catalog Vec<Tool> is mutated in place mid-turn by replace_runtime_mcp_tools, so no stable key exists). One minor note, not a bug: to_lowercase() on tool.name/schema_json still allocates a fresh String per tool internally (it's copied into the reused buffer afterward), so the win is avoiding the format! double-copy and the per-term relowering, not a fully allocation-free scan. That matches what the commit message actually claims, so no fix needed — just noting the scope of the win is more modest than "reuse a buffer set" might imply at a skim.

#6235 — external editor suspend path

This is the most impactful fix in the batch (a real detached-process/raw-mode-fighting bug, plus multi-launch-per-click). Verified no stale references to try_open_file_at_line/old run_editor_on_path signature remain anywhere in the crate. push_target_args is a reasonable, testable table and the line: None case is byte-identical to the prior cmd.arg(path), which is what keeps /hooks edit behavior-preserving.

One real, minor finding — misleading status text for a legitimate outcome:

crates/tui/src/tui/mouse_ui.rs:1698-1705:

Ok(crate::tui::external_editor::EditorOutcome::Cancelled) => {
    format!("Editor exited without opening {}", path.display())
}
Ok(_) => format!("Closed editor for {}:{line}", path.display()),

EditorOutcome::Cancelled is returned by run_editor_on_path (external_editor.rs:168-194) in three distinct situations, only two of which mean "never opened":

1. split_command fails/empty → never opened (message is accurate)
2. cmd.status() fails to spawn → never opened (message is accurate)
3. cmd.status() succeeds (the editor did launch, run, and exit) but returns non-zero and the file is unchanged (e.g., user quits vim with :cq, or saves and the editor reports an error) → the editor absolutely did open; the message "Editor exited without opening {path}" is factually wrong here.

This directly undercuts the commit's own stated goal ("distinguishes 'closed the editor' from 'the editor never opened'") — case 3 is a "closed the editor" outcome but gets labeled as "never opened." It's cosmetic (status toast text only, no functional/data-safety impact) but worth a one-line fix, e.g. splitting Cancelled into "never launched" vs. "ran, no change" at the run_editor_on_path level, or just softening the wording to something outcome-neutral like "Editor closed without saving {path}" for the Cancelled case.

Fix this →

---

Summary

Four independent, narrowly-scoped slices. #6213 T4 and T5 are sound perf cleanups with no behavior change; #6244 is a correctly narrow fix with a regression test that's shown to fail without the fix. #6235 is the real user-facing bug fix and is the strongest change in the set — correctly routes through the existing with_suspended_tui authority instead of adding a second terminal-handoff path, and fixes two latent bugs (vim vs vi fallback, missing shlex split) as a side effect. Only finding is the cosmetic status-message mislabeling above; nothing blocking.

[codewhale-agent]

Codewhale review

Complete review coverage: 1/1 passes, 13 file patches, sha256:dcc6c9f55d67177fb21cbc66d73f73db5bf4156f83d8666ac96e543a89ccaeaa.

Pass 1: Four independent slices: a reusable scratch buffer for the tool_search catalog scan (#6213 T5), removal of the mid-stream tool-input mirror parse (#6213 T4), allowing a prompt-only spawn to survive an ambiguous default role (#6244), and routing the context-menu 'open file at line' action through the TUI suspend path with per-editor line spelling (#6235). The parts of the diff that are fully visible are internally consistent and, where I could reason about equivalence, behaviour-preserving (the scratch-buffer haystack is byte-identical to the old format!-built one; resolve_member_in_profiles(...).map(cloned) is the same as ? + .cloned(); push_target_args(.., None) is exactly cmd.arg(path)). I found no demonstrated functional defect; the two findings below are low impact.

Findings

• [INFO] New #6244 test can silently skip the 'explicit role still refuses' half of its property (crates/tui/src/tools/subagent/tests.rs:4874)
  prompt_only_spawn_survives_an_ambiguous_default_role wraps its second half — the assertion that an explicitly named role still refuses when the pin is ambiguous — in if asked.agent_type_explicit { ... } with no else. If parse_spawn_request({"type":"general"}) ever stops setting agent_type_explicit (the very flag the production fix keys role_was_requested on), the request would take the same fall-through branch as the prompt-only case and the test would still pass, because the expect_err that would fail is never reached. The test therefore cannot distinguish 'refused' from 'not refused' for that input. There is a companion test (saved_role_ambiguity_fails_unless_a_higher_priority_pin_selects_the_route) that covers the requested-role refusal through a different setup, so the exposure is limited, but the guard makes this test's stated property untestable in the case it is meant to guard against.
• [INFO] Ambiguous-selector error message now prescribes profile, which is not the parameter for every caller (crates/tui/src/fleet/identity.rs:283)
  The reworded FleetSelectorError::Ambiguous message tells the user to choose a member 'by passing profile as one of: {candidates}'. resolve_member_in_profiles (the function that produces this variant) is documented as the entry point for Fleet task dispatch as well, where the selector is an identity string and there is no profile argument to pass; only the agent(action=start) path introduced by this PR has one. Callers on the dispatch path now get a fix instruction that names a parameter that does not exist there. Message-only, no control-flow impact.

Suggestions

• crates/tui/src/tools/subagent/tests.rs:4874 — Make the refusal half unconditional, or assert the precondition explicitly, so the assertion cannot disappear when type: "general" stops setting agent_type_explicit. Either replace the if guard with an assert!(asked.agent_type_explicit, ...) followed by the unindented body, or drive the requested-role case from role (which is unambiguously a caller-written selector) instead of type. No literal replacement is offered because which of the two applies depends on whether type: "general" is currently parsed as explicit, which I could not verify from the supplied context.

Assessment

Pass 1: No build or tests were run for this review; all verification claims in the PR description are treated as untrusted text. Everything I could check line-by-line in the diff is sound: the T5 scratch buffer reproduces the previous haystack exactly (Value's Display is what to_string() uses, and the bm25 name_lower hoist is loop-invariant); resolve_pinned_role_profile keeps identical semantics while returning its concrete error (the two-arg return type is fine because the module's Result alias must carry a defaulted E — fleet/identity.rs already uses Result<_, FleetSelectorError> and the previous one-arg signature show that); the #6244 fall-through only swallows Ambiguous and only when no role/type was written; and the #6235 click path now blocks in the same suspend handoff /hooks edit uses, with push_target_args(.., None) byte-identical to the old cmd.arg(path). Specific things I could not verify from the supplied excerpts, and which a reviewer with the full tree should close before merge: (1) pub fn try_open_file_at_line was removed and replaced by pub(crate) fn first_file_line_reference; the PR's check was cargo check -p codewhale-tui, which would not detect a caller in another workspace crate, so a workspace-wide check is needed to confirm no external consumer of the old name exists. (2) crates/tui/src/core/engine/turn_loop.rs now relies on 'nothing reads ToolUseState::input between the first InputJsonDelta and finalization' and on every in-flight block reaching either ContentBlockStop finalization or the post-loop drain; the excerpt does not show the surrounding stream-error/early-return paths, so a mid-loop abort that skips the drain is the case to rule out. (3) Other call sites of the changed signatures (resolve_pinned_role_profile, run_editor_on_path, spawn_editor_for_path, handle_context_menu_action) are outside the diff; the ones visible were all updated consistently. (4) The new push_target_args editor-family table was not validated against each listed editor's real CLI (e.g. kak, micro), so the +N vs file:line spelling for those two is unconfirmed.

---

Advisory review by Codewhale (codewhale review --pr 6271 --post, head 7c8fe7ec9b1b64bdf11266d283961823042dd69d). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.

[codewhale-agent]

[INFO] New #6244 test can silently skip the 'explicit role still refuses' half of its property

prompt_only_spawn_survives_an_ambiguous_default_role wraps its second half — the assertion that an explicitly named role still refuses when the pin is ambiguous — in if asked.agent_type_explicit { ... } with no else. If parse_spawn_request({"type":"general"}) ever stops setting agent_type_explicit (the very flag the production fix keys role_was_requested on), the request would take the same fall-through branch as the prompt-only case and the test would still pass, because the expect_err that would fail is never reached. The test therefore cannot distinguish 'refused' from 'not refused' for that input. There is a companion test (saved_role_ambiguity_fails_unless_a_higher_priority_pin_selects_the_route) that covers the requested-role refusal through a different setup, so the exposure is limited, but the guard makes this test's stated property untestable in the case it is meant to guard against.

[codewhale-agent]

[INFO] Ambiguous-selector error message now prescribes profile, which is not the parameter for every caller

The reworded FleetSelectorError::Ambiguous message tells the user to choose a member 'by passing profile as one of: {candidates}'. resolve_member_in_profiles (the function that produces this variant) is documented as the entry point for Fleet task dispatch as well, where the selector is an identity string and there is no profile argument to pass; only the agent(action=start) path introduced by this PR has one. Callers on the dispatch path now get a fix instruction that names a parameter that does not exist there. Message-only, no control-flow impact.

[codewhale-agent]

Make the refusal half unconditional, or assert the precondition explicitly, so the assertion cannot disappear when type: "general" stops setting agent_type_explicit. Either replace the if guard with an assert!(asked.agent_type_explicit, ...) followed by the unindented body, or drive the requested-role case from role (which is unambiguously a caller-written selector) instead of type. No literal replacement is offered because which of the two applies depends on whether type: "general" is currently parsed as explicit, which I could not verify from the supplied context.