Skip to content

Commit ed974fa

Browse files
yogthosYogthos
andauthored
fix: audit followups — @-picker UTF-8, flag conflict, session prompt restore, MCP timeout + rustfmt CI (#103)
Five real bugs from the latest 6-agent audit pass. CI was also red on rustfmt — included in this PR. ## Bug 1 — @-picker Esc handler corrupted UTF-8 buffers (CRITICAL) `src/ui/input.rs` Esc handler in the file picker mixed byte offsets with char counts: let at = self.buffer.rfind('@'); // byte offset let before: String = self.buffer.chars().take(at).collect(); // char count let after: String = self.buffer.chars().skip(at + 1 + picker.query.len()).collect(); For any input containing multi-byte UTF-8 chars before the `@` (accented letters, emoji, CJK), the offsets disagreed and the buffer corrupted on Esc-cancel. Fixed by switching to byte-level slicing, matching the Enter path right above which was already correct. ## Bug 2 — Conflicting permission flags silently overridden (HIGH) `src/main.rs::resolve_mode` checked `--yolo` first, then `--accept-all`, then `--restrictive`. Passing `--yolo --restrictive` silently picked yolo. User thought they restricted permissions and got the opposite. Now detects multiple CLI permission flags and emits a stderr warning naming the active mode + the conflicting flags. Still picks the most-permissive (first match) for non-breaking behavior, but the user sees the conflict. ## Bug 3 — Session load didn't restore active prompt (MEDIUM) `/sessions <id>` swapped `*session = s` and called `render_session` but never read `session.current_prompt_name` into the context. A session saved with `current_prompt_name = Some("plan")` would resume with the default "code" prompt because the agent gets rebuilt from the context (which still held the previous prompt). Now reads `session.current_prompt_name`, looks up the prompt content in `context.prompts`, sets both context fields, and rebuilds the agent with the restored prompt before `render_session`. Loaded-message confirms with `; prompt: <name>` when a prompt was restored. ## Bug 4 — MCP tool calls had no timeout (MEDIUM) `src/extras/mcp/tool.rs` called `peer.call_tool(params).await` unconditionally. If the MCP server hung (deadlock, lost stdio pipe, internal error), the agent turn stalled forever — no way out short of Ctrl+C to kill the whole agent. Wrapped in `tokio::time::timeout(120s, …)`. Error message names the offending server + tool (`MCP tool foo::bar timed out after 120s`) so the user can identify which MCP to restart/disable. 120s matches `bash`'s default timeout — any real tool call done in less. ## Bug 5 — rustfmt CI failure `src/ui/theme.rs:306` (from PR #102's load_custom_theme): a single `let` line was slightly over the rustfmt width. `cargo fmt` wraps it; that's what CI was failing on. Included the format fix in this PR. ## False positives confirmed (NOT acted on) The audit also flagged several items I verified as non-bugs: - Theme JSON "not wired into startup" — actually wired in PR #102 (`load_custom_theme` is called from `theme::init`). - Subagent isolation "incomplete" — intentional by design, documented in docs/PLUGINS.md. - check_path dual matching (canonical + literal) — by design for path tools. - Restrictive mode precedence — by design, dense but correct. - find_callees regex false positives — known limitation; LSP references is the AST-aware path for high-confidence use. ## Tests 715 pass (no new — these are integration / behavioral fixes that would need substantial mocking to unit-test, and the changes either preserve existing behavior on the happy path or affect narrow error paths). `cargo fmt --check` now passes. Co-authored-by: Yogthos <yogthos@gmail.com>
1 parent 7054561 commit ed974fa

5 files changed

Lines changed: 102 additions & 14 deletions

File tree

src/extras/mcp/tool.rs

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,32 @@ impl ToolDyn for McpTool {
7979
.map(|a| CallToolRequestParams::new(tool_name.clone()).with_arguments(a))
8080
.unwrap_or_else(|| CallToolRequestParams::new(tool_name.clone()));
8181

82-
let result = peer.call_tool(params).await.map_err(|e| {
83-
ToolError::ToolCallError(Box::new(McpToolError(format!("MCP tool error: {e}"))))
84-
})?;
82+
// MCP tool calls go over JSON-RPC to a spawned server
83+
// process. If the server hangs (deadlock, infinite
84+
// loop, lost stdin pipe), the await never resolves and
85+
// the agent turn stalls indefinitely. Cap at 120s to
86+
// match `bash`'s default timeout — anything longer is
87+
// clearly broken on the server side. The error message
88+
// names the server + tool so the user can identify
89+
// which MCP server is misbehaving.
90+
const MCP_CALL_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(120);
91+
let result = match tokio::time::timeout(MCP_CALL_TIMEOUT, peer.call_tool(params)).await
92+
{
93+
Ok(r) => r.map_err(|e| {
94+
ToolError::ToolCallError(Box::new(McpToolError(format!(
95+
"MCP tool error ({}::{}): {e}",
96+
server_name, tool_name,
97+
))))
98+
})?,
99+
Err(_) => {
100+
return Err(ToolError::ToolCallError(Box::new(McpToolError(format!(
101+
"MCP tool {}::{} timed out after {}s",
102+
server_name,
103+
tool_name,
104+
MCP_CALL_TIMEOUT.as_secs(),
105+
)))));
106+
}
107+
};
85108

86109
if result.is_error.unwrap_or(false) {
87110
let error_msg = result

src/main.rs

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,29 @@ struct Channels {
5757
}
5858

5959
fn resolve_mode(cli: &cli::Cli, cfg: &config::Config) -> SecurityMode {
60+
// Warn on conflicting CLI flags. Previously `--yolo --restrictive`
61+
// silently picked yolo (the first-match in the if-else chain)
62+
// without surfacing the conflict — the user thought they had
63+
// restricted permissions and got the opposite. Emit a stderr
64+
// warning naming the active mode so the user can correct it.
65+
let cli_modes: &[(bool, &str)] = &[
66+
(cli.yolo, "--yolo"),
67+
(cli.accept_all, "--accept-all"),
68+
(cli.restrictive, "--restrictive"),
69+
];
70+
let cli_picks: Vec<&str> = cli_modes
71+
.iter()
72+
.filter(|(v, _)| *v)
73+
.map(|(_, name)| *name)
74+
.collect();
75+
if cli_picks.len() > 1 {
76+
eprintln!(
77+
"warning: conflicting permission flags {:?}; using the most permissive ({}). \
78+
Pass only one of --yolo / --accept-all / --restrictive.",
79+
cli_picks, cli_picks[0],
80+
);
81+
}
82+
6083
if cli.yolo || cfg.yolo.unwrap_or(false) {
6184
SecurityMode::Yolo
6285
} else if cli.accept_all || cfg.accept_all.unwrap_or(false) {

src/ui/input.rs

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -626,16 +626,20 @@ impl InputEditor {
626626
true
627627
}
628628
KeyCode::Esc => {
629-
let at_pos = self.buffer.rfind('@');
630-
if let Some(at) = at_pos {
631-
let before: String = self.buffer.chars().take(at).collect();
632-
let after: String = self
633-
.buffer
634-
.chars()
635-
.skip(at + 1 + picker.query.len())
636-
.collect();
637-
self.buffer = format!("{}{}", before, after).into();
629+
// Use BYTE-level slicing here, matching the Enter
630+
// path above. `rfind('@')` returns a byte offset;
631+
// the previous implementation used `chars().take(at)`
632+
// and `chars().skip(at + ...)` which mixed byte
633+
// offsets with char counts and corrupted the buffer
634+
// for any input containing multi-byte UTF-8 chars
635+
// before the `@` (accented letters, emoji, CJK, …).
636+
if let Some(at) = self.buffer.rfind('@') {
637+
let before = &self.buffer[..at];
638+
let after_byte = at + 1 + picker.query.len();
639+
let after = self.buffer.get(after_byte..).unwrap_or("");
640+
let new_buf = format!("{}{}", before, after);
638641
self.cursor = at;
642+
self.buffer = new_buf.into();
639643
}
640644
picker.deactivate();
641645
true

src/ui/slash.rs

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -383,9 +383,46 @@ pub async fn handle_slash(
383383
if let Some(s) = sessions.into_iter().next() {
384384
let msg_count = s.messages.len();
385385
*session = s;
386+
// Restore the prompt that was active when the
387+
// session was saved. Without this, `/sessions
388+
// <id>` would always rebuild the agent with the
389+
// default prompt, even if the saved session
390+
// recorded `current_prompt_name = Some("plan")`.
391+
// Sets both context fields + rebuilds the agent
392+
// so the new prompt actually takes effect.
393+
let restored = session.current_prompt_name.clone();
394+
if let Some(name) = restored.as_deref()
395+
&& let Some(content) = context.prompts.get(name)
396+
{
397+
context.current_prompt = Some(content.clone());
398+
context.current_prompt_name = Some(name.to_string());
399+
let model = client.completion_model(session.model.to_string());
400+
*agent = crate::provider::build_agent(
401+
model,
402+
cli,
403+
cfg,
404+
context,
405+
permission.clone(),
406+
ask_tx.clone(),
407+
None,
408+
None,
409+
bg_store.clone(),
410+
#[cfg(feature = "lsp")]
411+
None,
412+
sandbox.clone(),
413+
#[cfg(feature = "mcp")]
414+
mcp_manager,
415+
#[cfg(feature = "semantic")]
416+
semantic_manager,
417+
)
418+
.await;
419+
}
386420
render_session(renderer, session, cli, cfg, context)?;
421+
let prompt_note = restored
422+
.map(|n| format!("; prompt: {}", n))
423+
.unwrap_or_default();
387424
renderer.write_line(
388-
&format!("loaded session ({} msgs)", msg_count),
425+
&format!("loaded session ({} msgs{})", msg_count, prompt_note),
389426
c_agent(),
390427
)?;
391428
}

src/ui/theme.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -303,7 +303,8 @@ fn load_custom_theme(name: &str) -> Result<Theme, String> {
303303
if !path.exists() {
304304
return Err(format!("no such file: {}", path.display()));
305305
}
306-
let raw = std::fs::read_to_string(&path).map_err(|e| format!("read {}: {e}", path.display()))?;
306+
let raw =
307+
std::fs::read_to_string(&path).map_err(|e| format!("read {}: {e}", path.display()))?;
307308
let overrides: ThemeJson =
308309
serde_json::from_str(&raw).map_err(|e| format!("parse {}: {e}", path.display()))?;
309310
// Label defaults to the filename's stem in uppercase if the JSON

0 commit comments

Comments
 (0)