From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: iceBear67 Date: Sun, 16 Aug 2026 02:56:38 +0000 Subject: [PATCH] Promote dedicated tools via per-turn checklist and bash-discipline nudge Persist a cache-stable dispatch checklist on primary user turns so the model prefers explore/search/read/plan and deep-research over bash or a wide search of its own. After a bash call that used cat/grep/find/ls (and friends), append a post-hoc nudge pointing at the dedicated tool. Both blocks are stripped from the compaction summarizer copy only. diff --git a/crates/codegen/xai-grok-shell/src/session/acp_session_impl/reminders.rs b/crates/codegen/xai-grok-shell/src/session/acp_session_impl/reminders.rs index 27f75124fca625dcb9724185c26172926bb46346..d6224d9ea3b5a665c0d41fc7ca7561939a3310cb 100644 --- a/crates/codegen/xai-grok-shell/src/session/acp_session_impl/reminders.rs +++ b/crates/codegen/xai-grok-shell/src/session/acp_session_impl/reminders.rs @@ -514,6 +514,37 @@ impl SessionActor { "Injected date rollover reminder" ); } + /// Persist the dispatch checklist onto a primary user turn (cache-stable: + /// rendered once, never rewritten). Skipped for subagents, verbatim + /// prompts, and when system reminders are off. + pub(super) async fn append_dispatch_checklist( + &self, + user_message: String, + verbatim: bool, + ) -> String { + if !xai_grok_tools::reminders::tool_promotion::should_inject_dispatch_checklist( + self.startup_hints.is_subagent, + verbatim, + true, + self.agent.borrow().reminder_policy().enabled, + ) { + return user_message; + } + let Some(body) = self + .tool_bridge_handle() + .render_prompt( + xai_grok_tools::reminders::tool_promotion::DISPATCH_CHECKLIST_TEMPLATE, + &serde_json::json!({}), + ) + .await + else { + return user_message; + }; + if body.contains("${{") || body.contains("${%") { + return user_message; + } + xai_grok_tools::reminders::tool_promotion::append_dispatch_checklist(&user_message, &body) + } /// Frame the already-assembled user turn when a mid-stream abort left the model no other /// signal. /// One-shot; skipped for the harness that owns this surface. diff --git a/crates/codegen/xai-grok-shell/src/session/acp_session_impl/turn.rs b/crates/codegen/xai-grok-shell/src/session/acp_session_impl/turn.rs index 355d5146e2859778b9c3c99e71404ce9c0d3087f..f2e65c4f3758c7619b5e73b0d3eceda657552aca 100644 --- a/crates/codegen/xai-grok-shell/src/session/acp_session_impl/turn.rs +++ b/crates/codegen/xai-grok-shell/src/session/acp_session_impl/turn.rs @@ -803,6 +803,12 @@ impl SessionActor { attached_image_refs, )) .await; + let user_message = if matches!(&origin, super::super::PromptOrigin::User) { + let framed = self.maybe_apply_interrupt_envelope(user_message, verbatim); + self.append_dispatch_checklist(framed, verbatim).await + } else { + user_message + }; let prompt_text_for_hook = Some(user_message.clone()); { if trace_gcs_config.is_some() { @@ -832,9 +838,7 @@ impl SessionActor { } super::super::PromptOrigin::PlanResume => ConversationItem::user(user_message), super::super::PromptOrigin::User => { - let mut item = ConversationItem::user( - self.maybe_apply_interrupt_envelope(user_message, verbatim), - ); + let mut item = ConversationItem::user(user_message); if let Some(interrupt) = self .events .take_prior_interrupt_category() diff --git a/crates/codegen/xai-grok-shell/src/session/acp_session_tests/cancel_running_task_tests.rs b/crates/codegen/xai-grok-shell/src/session/acp_session_tests/cancel_running_task_tests.rs index 391b6dc17a46bb37d86bb479822001adbefdb1cd..485c5aa81b467271ac848ac5323ef04e9640c6b4 100644 --- a/crates/codegen/xai-grok-shell/src/session/acp_session_tests/cancel_running_task_tests.rs +++ b/crates/codegen/xai-grok-shell/src/session/acp_session_tests/cancel_running_task_tests.rs @@ -1492,7 +1492,15 @@ async fn handle_prompt_frames_interrupt_on_user_message() { .expect("the user message must be in the conversation"); let text = user.text_content(); let expected_assembled = format!("\n{query}\n"); - assert_eq!(text, frame_user_turn(INTERRUPT_NOTE, &expected_assembled)); + let framed = frame_user_turn(INTERRUPT_NOTE, &expected_assembled); + assert!( + text.starts_with(&framed), + "interrupt envelope must lead the persisted user turn: {text}" + ); + assert!( + text.contains("Dispatch checklist"), + "primary user turn must carry the dispatch checklist: {text}" + ); assert!(!actor.events.take_pending_interrupt_reminder()); prompt_task.abort(); }) @@ -1589,12 +1597,18 @@ async fn handle_prompt_send_now_frames_interjection_envelope() { }) .expect("the send-now user message must be in the conversation"); let expected_assembled = format!("\n{query}\n"); - assert_eq!( - user.text_content(), - frame_user_turn( - xai_interjection_core::INTERJECTION_NOTE, - &expected_assembled - ) + let framed = frame_user_turn( + xai_interjection_core::INTERJECTION_NOTE, + &expected_assembled, + ); + let text = user.text_content(); + assert!( + text.starts_with(&framed), + "send-now must use the full interjection envelope: {text}" + ); + assert!( + text.contains("Dispatch checklist"), + "send-now primary turn must carry the dispatch checklist: {text}" ); prompt_task.abort(); }) diff --git a/crates/codegen/xai-grok-shell/src/session/acp_session_tests/reminder_policy_tests.rs b/crates/codegen/xai-grok-shell/src/session/acp_session_tests/reminder_policy_tests.rs index d06dbc221e497ebe9a9796f91e87aed65fb8eb47..bc4fe8229c010202c845011d9a5792382d3cd04c 100644 --- a/crates/codegen/xai-grok-shell/src/session/acp_session_tests/reminder_policy_tests.rs +++ b/crates/codegen/xai-grok-shell/src/session/acp_session_tests/reminder_policy_tests.rs @@ -406,3 +406,72 @@ async fn rollover_reminder_fires_when_fallback_stamps_a_date_free_template() { }) .await; } + +#[tokio::test] +async fn append_dispatch_checklist_on_primary_user_turn() { + let local = tokio::task::LocalSet::new(); + local + .run_until(async { + let (gateway_tx, _) = + tokio::sync::mpsc::unbounded_channel::(); + let (persistence_tx, _) = tokio::sync::mpsc::unbounded_channel::(); + let actor = create_test_actor(50_000, 256_000, 85, gateway_tx, persistence_tx).await; + let query = "\nfix the bug\n"; + let out = actor.append_dispatch_checklist(query.into(), false).await; + assert!( + out.starts_with(query), + "checklist must follow the assembled query: {out}" + ); + assert!( + out.contains("\nDispatch checklist"), + "primary turn must persist the checklist: {out}" + ); + assert!( + !out.contains("${{"), + "rendered checklist must not leak template syntax: {out}" + ); + assert!( + out.contains("explore"), + "checklist must promote the explore subagent: {out}" + ); + assert!( + out.contains("deep-research"), + "checklist must promote deep-research: {out}" + ); + }) + .await; +} + +#[tokio::test] +async fn append_dispatch_checklist_skips_verbatim() { + let local = tokio::task::LocalSet::new(); + local + .run_until(async { + let (gateway_tx, _) = + tokio::sync::mpsc::unbounded_channel::(); + let (persistence_tx, _) = tokio::sync::mpsc::unbounded_channel::(); + let actor = create_test_actor(50_000, 256_000, 85, gateway_tx, persistence_tx).await; + let raw = "caller-owned bytes"; + let out = actor.append_dispatch_checklist(raw.into(), true).await; + assert_eq!(out, raw); + }) + .await; +} + +#[tokio::test] +async fn append_dispatch_checklist_skips_subagent() { + let local = tokio::task::LocalSet::new(); + local + .run_until(async { + let (gateway_tx, _) = + tokio::sync::mpsc::unbounded_channel::(); + let (persistence_tx, _) = tokio::sync::mpsc::unbounded_channel::(); + let mut actor = + create_test_actor(50_000, 256_000, 85, gateway_tx, persistence_tx).await; + actor.startup_hints.is_subagent = true; + let query = "\nsearch callers\n"; + let out = actor.append_dispatch_checklist(query.into(), false).await; + assert_eq!(out, query); + }) + .await; +} diff --git a/crates/codegen/xai-grok-shell/src/session/acp_session_tests/tool_layer_images_bridge_tests.rs b/crates/codegen/xai-grok-shell/src/session/acp_session_tests/tool_layer_images_bridge_tests.rs index b84ecab7966431a349ff7d264434cb1cf2901865..5665800490fa8fce4811275041959350892603e9 100644 --- a/crates/codegen/xai-grok-shell/src/session/acp_session_tests/tool_layer_images_bridge_tests.rs +++ b/crates/codegen/xai-grok-shell/src/session/acp_session_tests/tool_layer_images_bridge_tests.rs @@ -1,6 +1,7 @@ //! Wiring tests for MCP tool-layer images through `handle_bridge_tool_success`. use super::support::*; use super::*; +use base64::Engine; use xai_grok_sampling_types::{ContentPart, ConversationItem}; use xai_grok_tools::types::output::{MCPOutput, ToolOutput, ToolRunResult}; use xai_grok_tools::util::base64_images::{ExtractedImage, IMAGE_CONTENT_PLACEHOLDER}; diff --git a/crates/codegen/xai-grok-shell/src/session/compaction.rs b/crates/codegen/xai-grok-shell/src/session/compaction.rs index 0225e170b5b70e24e5ddbd598aa9422a010a1e02..e169f7001473a79b98d4cf373dd12727221fbcaa 100644 --- a/crates/codegen/xai-grok-shell/src/session/compaction.rs +++ b/crates/codegen/xai-grok-shell/src/session/compaction.rs @@ -31,7 +31,79 @@ use xai_chat_state::compaction_utils::{ prepare_conversation_for_verbatim_summarization, sanitize_compacted_history, validate_compacted_history, }; -use xai_grok_sampling_types::{ApiBackend, ConversationItem}; +use xai_grok_sampling_types::{ApiBackend, ContentPart, ConversationItem}; + +/// Drop dispatch-checklist / bash-discipline reminders from the copy sent to +/// the compaction summarizer. Segment persist keeps the live bytes. +fn strip_tool_promotion_from_conversation( + mut conversation: Vec, +) -> Vec { + use xai_grok_tools::reminders::tool_promotion::strip_tool_promotion_reminders; + for item in &mut conversation { + match item { + ConversationItem::User(u) => { + for part in &mut u.content { + if let ContentPart::Text { text } = part { + let stripped = strip_tool_promotion_reminders(text); + if stripped.as_str() != text.as_ref() { + *text = stripped.into(); + } + } + } + } + ConversationItem::ToolResult(t) => { + let stripped = strip_tool_promotion_reminders(&t.content); + if stripped.as_str() != t.content.as_ref() { + t.content = stripped.into(); + } + } + _ => {} + } + } + conversation +} + +fn prepare_summarizer_conversation( + conversation: Vec, + verbatim: bool, + strip_reasoning: bool, +) -> Vec { + let prepared = if verbatim { + prepare_conversation_for_verbatim_summarization(conversation, strip_reasoning) + } else { + xai_chat_state::compaction_utils::prepare_conversation_for_summarization(conversation) + }; + strip_tool_promotion_from_conversation(prepared) +} + +#[cfg(test)] +mod tool_promotion_strip_tests { + use super::*; + use xai_grok_tools::reminders::wrap_reminder; + + #[test] + fn summarizer_copy_drops_checklist_and_bash_nudge() { + let user = ConversationItem::user(format!( + "\nfix it\n\n\n{}", + wrap_reminder("Dispatch checklist — leftover.") + )); + let tool = ConversationItem::tool_result( + "c1", + format!( + "file contents\n\n{}", + wrap_reminder( + "This bash call did a file operation that has a dedicated tool: `cat` → the `read_file` tool." + ) + ), + ); + let stripped = strip_tool_promotion_from_conversation(vec![user, tool]); + assert_eq!( + stripped[0].text_content(), + "\nfix it\n" + ); + assert_eq!(stripped[1].text_content(), "file contents"); + } +} /// Default percentage points below the auto-compact threshold at which prefire /// (background pass-1) starts, giving pass-1 runway to finish before the limit. /// Override with `GROK_PREFIRE_LEAD_PERCENT`. @@ -255,8 +327,7 @@ impl SessionActor { .as_ref() .map(|c| c.model.to_string()) .unwrap_or_default(); - let prefix_prepared = - prepare_conversation_for_verbatim_summarization(split.prefix.to_vec(), strips); + let prefix_prepared = prepare_summarizer_conversation(split.prefix.to_vec(), true, strips); let prefix_est_tokens = prefix_prepared .iter() .map(xai_chat_state::estimate_item_tokens) @@ -357,11 +428,11 @@ impl SessionActor { } let prefix = &live[..cache.prefix_len]; let tail = &live[cache.prefix_len..]; - let prepared_tail = - prepare_conversation_for_verbatim_summarization(tail.to_vec(), strips_reasoning); + let prepared_tail = prepare_summarizer_conversation(tail.to_vec(), true, strips_reasoning); + let prepared_prefix = strip_tool_promotion_from_conversation(prefix.to_vec()); let prompt = build_two_pass_compaction_prompt(user_context); let pass2_history = - build_two_pass_pass2_history(prefix, &prepared_tail, &cache.note1, &prompt); + build_two_pass_pass2_history(&prepared_prefix, &prepared_tail, &cache.note1, &prompt); let started = std::time::Instant::now(); let mut out = self.two_pass_sample(pass2_history).await?; if is_degenerate_summary(&out.content) { @@ -930,16 +1001,11 @@ impl SessionActor { }; const SUMMARY_BUDGET_RESERVE_TOKENS: u64 = 32_768; let verbatim_input_enabled = self.compaction.verbatim_input; - let simplified_messages = if verbatim_input_enabled { - xai_chat_state::compaction_utils::prepare_conversation_for_verbatim_summarization( - full_conversation, - summary_strips_reasoning, - ) - } else { - xai_chat_state::compaction_utils::prepare_conversation_for_summarization( - full_conversation, - ) - }; + let simplified_messages = prepare_summarizer_conversation( + full_conversation, + verbatim_input_enabled, + summary_strips_reasoning, + ); if conv_len == 0 { tracing::error!( session_id = %self.session_info.id.0, @@ -1151,8 +1217,9 @@ impl SessionActor { let budget = context_window .saturating_sub(SUMMARY_BUDGET_RESERVE_TOKENS) .saturating_sub(compaction_tool_tokens); - let verbatim = xai_chat_state::compaction_utils::prepare_conversation_for_verbatim_summarization( + let verbatim = prepare_summarizer_conversation( conv, + true, summary_strips_reasoning, ); xai_chat_state::compaction_utils::fit_conversation_to_budget( @@ -1163,8 +1230,10 @@ impl SessionActor { let lossy_budget = (context_window.saturating_mul(7) / 10) .saturating_sub(compaction_tool_tokens); xai_chat_state::compaction_utils::fit_conversation_to_budget( - xai_chat_state::compaction_utils::prepare_conversation_for_summarization( + prepare_summarizer_conversation( conv, + false, + summary_strips_reasoning, ), lossy_budget, ) diff --git a/crates/codegen/xai-grok-tools/src/registry/types.rs b/crates/codegen/xai-grok-tools/src/registry/types.rs index fe143a291d401b4aed8ce37d146e0b118890db69..f277182c99e1c52318d06fa1f91182d10b917227 100644 --- a/crates/codegen/xai-grok-tools/src/registry/types.rs +++ b/crates/codegen/xai-grok-tools/src/registry/types.rs @@ -758,6 +758,7 @@ impl ToolRegistryBuilder { b.register_reminder(crate::reminders::LspDiagnosticsReminder); b.register_reminder(crate::reminders::TaskCompletionReminder); b.register_reminder(SkillDiscoveryReminder); + b.register_reminder(crate::reminders::BashDisciplineReminder); for pack in tool_packs().lock().iter() { pack(&mut b); } diff --git a/crates/codegen/xai-grok-tools/src/reminders/mod.rs b/crates/codegen/xai-grok-tools/src/reminders/mod.rs index a46e610adf318b887b87b730ab33950d8e9730c8..c66d1e1c5e8c196cf6806b8ab58bbc8ae2773d95 100644 --- a/crates/codegen/xai-grok-tools/src/reminders/mod.rs +++ b/crates/codegen/xai-grok-tools/src/reminders/mod.rs @@ -10,17 +10,20 @@ //! registry that fire after every tool call. //! //! This module contains the cross-cutting reminders: -//! - [`LspDiagnosticsReminder`], [`SkillDiscoveryReminder`], [`TaskCompletionReminder`] +//! - [`LspDiagnosticsReminder`], [`SkillDiscoveryReminder`], [`TaskCompletionReminder`], +//! [`BashDisciplineReminder`] //! //! All reminders are collected and appended in `call_new_tool()`. pub mod lsp_diagnostics; pub mod skill_discovery; pub mod task_completion; +pub mod tool_promotion; pub use lsp_diagnostics::LspDiagnosticsReminder; pub use skill_discovery::SkillDiscoveryReminder; pub use task_completion::TaskCompletionReminder; +pub use tool_promotion::BashDisciplineReminder; /// The default system-reminder tag name (hyphen). pub const DEFAULT_REMINDER_TAG: &str = "system-reminder"; diff --git a/crates/codegen/xai-grok-tools/src/reminders/tool_promotion.rs b/crates/codegen/xai-grok-tools/src/reminders/tool_promotion.rs new file mode 100644 index 0000000000000000000000000000000000000000..19834fe51a8acf05754e7503de79a23925682b63 --- /dev/null +++ b/crates/codegen/xai-grok-tools/src/reminders/tool_promotion.rs @@ -0,0 +1,608 @@ +//! Builtin-tool promotion: per-turn dispatch checklist and post-hoc bash nudge. +//! +//! Ports two complementary levers from the Claude-style reminder plugin: +//! +//! 1. A **dispatch checklist** persisted onto every primary user turn so the +//! model is reminded, at the moment of work, to use dedicated tools / +//! `explore` / `plan` / `deep-research` instead of reaching for bash or +//! doing a wide search itself. Written once into the user message (never +//! mutated later) so a cached prompt prefix stays valid. +//! 2. A **bash-discipline nudge** appended to the tool result when a bash +//! call used `cat`/`grep`/`find`/… for a file operation that has a +//! dedicated tool. Deliberately a nudge, not a block: the command still +//! ran. Not gated on primary-vs-subagent — subagents reach for bash just +//! as readily. +//! +//! Both blocks are stripped from the copy sent to the compaction summarizer +//! (see [`strip_tool_promotion_reminders`]) so agent-only meta-instructions +//! stay out of the conversation summary. Segment persist is left intact. + +use std::collections::HashSet; + +use crate::types::output::ToolOutput; +use crate::types::requirements::{Expr, ToolRequirement}; +use crate::types::resources::SharedResources; +use crate::types::template_renderer::TemplateRenderer; +use crate::types::tool::{Reminder, ToolKind}; + +use super::wrap_reminder; + +/// Opening of the wrapped dispatch checklist. Compaction uses this to find +/// and drop the block from summarizer input. +pub const DISPATCH_CHECKLIST_MARK: &str = "\nDispatch checklist"; + +/// Opening of the wrapped bash-discipline nudge. Same role as +/// [`DISPATCH_CHECKLIST_MARK`]. +pub const BASH_DISCIPLINE_MARK: &str = + "\nThis bash call did a file operation that has a dedicated tool:"; + +/// Body of the per-turn dispatch checklist (no `` wrap). +/// +/// Tool names go through `${{ tools.by_kind.* }}` so they match the live +/// toolset. The shell renders this once at persist time; the bytes that land +/// in history are therefore cache-stable. +pub const DISPATCH_CHECKLIST_TEMPLATE: &str = "\ +Dispatch checklist — run it before your first tool call this turn: \ +(1) answering would mean searching across files (locations, flows, callers, conventions, history)\ +${%- if tools.by_kind.task %} → `${{ tools.by_kind.task }}` with `explore`${%- endif %}; \ +a lookup inside a file you can name → do it yourself with the \ +${%- if tools.by_kind.search %}`${{ tools.by_kind.search }}`${%- else %}search${%- endif %}/\ +${%- if tools.by_kind.read %}`${{ tools.by_kind.read }}`${%- else %}read${%- endif %} \ +tools, never grep/rg/find/ls in bash; \ +(2) the request meets a plan criterion (new feature, multiple valid approaches, \ +behavior/structure change, unclear scope — file count alone does not count) \ +→ explore → plan → relay → confirm → execute; \ +(3) hard-to-reverse architecture choice, no confident recommendation, or a bug \ +that resisted investigation (symptoms contradict code / fix failed inexplicably / intermittent) → \ +${%- if tools.by_kind.workflow %}`${{ tools.by_kind.workflow }}` named `deep-research`${%- else %}`/deep-research`${%- endif %}, \ +don't guess and don't pass the second dead end solo. \ +While working, a second consecutive manual search on the same question means the \ +question is wider than you thought — hand the remainder to explore. \ +Restraint: don't spawn for work you'd finish in a handful of tool calls, don't \ +fan out on one small task, and never delegate a check you can run inline. \ +After changing code, drive it once for real — or report BLOCKED with the exact command.\n\ +This is an automated reminder — ignore whatever does not apply to the current turn \ +(conversational replies, files already known, trivial fixes)."; + +/// Whether the dispatch checklist should be written onto this turn. +/// +/// Primary-only and `PromptOrigin::User` only: subagents inherit a tighter +/// prompt, and synthetic / verbatim turns must keep caller-owned bytes. +pub fn should_inject_dispatch_checklist( + is_subagent: bool, + verbatim: bool, + is_user_origin: bool, + reminders_enabled: bool, +) -> bool { + reminders_enabled && is_user_origin && !is_subagent && !verbatim +} + +/// Wrap a rendered checklist body and append it after `user_message`. +pub fn append_dispatch_checklist(user_message: &str, rendered_body: &str) -> String { + let wrapped = wrap_reminder(rendered_body); + if user_message.is_empty() { + wrapped + } else { + format!("{user_message}\n\n{wrapped}") + } +} + +/// Drop dispatch-checklist and bash-discipline `` blocks. +/// +/// Used on the summarizer copy only. Leading blank lines in front of a +/// stripped block are eaten so the surrounding transcript does not grow +/// extra gaps. +pub fn strip_tool_promotion_reminders(text: &str) -> String { + let mut result = text.to_string(); + for mark in [DISPATCH_CHECKLIST_MARK, BASH_DISCIPLINE_MARK] { + while let Some(start) = result.find(mark) { + let after_open = start + mark.len(); + let Some(rel_end) = result[after_open..].find("") else { + break; + }; + let end = after_open + rel_end + "".len(); + let from = if start >= 2 && &result[start - 2..start] == "\n\n" { + start - 2 + } else if start >= 1 && &result[start - 1..start] == "\n" { + start - 1 + } else { + start + }; + result.replace_range(from..end, ""); + } + } + result +} + +/// Utilities whose dedicated-tool equivalent the standing bash description +/// either states weakly or omits. Values are rendered into the reminder, so +/// each one names the replacement rather than merely a kind id. +#[derive(Debug, Clone, Copy)] +enum DedicatedReplacement { + Kind(ToolKind), + /// `echo` / `printf` talking to the user — reply in text instead. + Reply, +} + +const DEDICATED_FOR: &[(&str, DedicatedReplacement)] = &[ + ("rg", DedicatedReplacement::Kind(ToolKind::Search)), + ("grep", DedicatedReplacement::Kind(ToolKind::Search)), + ("ag", DedicatedReplacement::Kind(ToolKind::Search)), + ("ack", DedicatedReplacement::Kind(ToolKind::Search)), + ("find", DedicatedReplacement::Kind(ToolKind::List)), + ("ls", DedicatedReplacement::Kind(ToolKind::List)), + ("cat", DedicatedReplacement::Kind(ToolKind::Read)), + ("head", DedicatedReplacement::Kind(ToolKind::Read)), + ("tail", DedicatedReplacement::Kind(ToolKind::Read)), + ("sed", DedicatedReplacement::Kind(ToolKind::Edit)), + ("awk", DedicatedReplacement::Kind(ToolKind::Edit)), + ("echo", DedicatedReplacement::Reply), + ("printf", DedicatedReplacement::Reply), +]; + +fn dedicated_for(name: &str) -> Option { + DEDICATED_FOR + .iter() + .find(|(n, _)| *n == name) + .map(|(_, r)| *r) +} + +fn is_env_assignment(token: &str) -> bool { + let Some(eq) = token.find('=') else { + return false; + }; + let key = &token[..eq]; + let mut chars = key.chars(); + match chars.next() { + Some(c) if c.is_ascii_alphabetic() || c == '_' => {} + _ => return false, + } + chars.all(|c| c.is_ascii_alphanumeric() || c == '_') +} + +fn head_utility(segment: &str) -> Option { + let tokens: Vec<&str> = segment.split_whitespace().collect(); + let mut index = 0; + while index < tokens.len() && is_env_assignment(tokens[index]) { + index += 1; + } + let token = tokens.get(index)?; + let name = token.rsplit('/').next().unwrap_or(token); + let name = name.trim_matches(|c| c == '\'' || c == '"'); + if name.is_empty() { + None + } else { + Some(name.to_string()) + } +} + +/// Blanks quoted runs before splitting so a separator *inside* an argument +/// cannot fabricate a command head — `bun -e 'f("a && rg x")'` otherwise +/// reports `rg`. +fn blank_quoted(command: &str) -> String { + let mut out = String::with_capacity(command.len()); + let mut chars = command.chars(); + while let Some(c) = chars.next() { + match c { + '\'' => { + out.push('\''); + out.push('\''); + for n in chars.by_ref() { + if n == '\'' { + break; + } + } + } + '"' => { + out.push('"'); + out.push('"'); + for n in chars.by_ref() { + if n == '"' { + break; + } + } + } + _ => out.push(c), + } + } + out +} + +/// Split on `;`, newline, `&&`, `||`, `$(`, backtick. Deliberately does +/// **not** split on a single `|`: a mapped utility that starts a command is +/// reaching for a file, while the same name after a pipe is consuming +/// another program's output (`git log | head`), which is what bash is for. +fn split_command_segments(command: &str) -> Vec<&str> { + let mut segments = Vec::new(); + let mut start = 0; + let mut iter = command.char_indices().peekable(); + while let Some((i, c)) = iter.next() { + let rest = &command[i..]; + let sep_len = if rest.starts_with("&&") || rest.starts_with("||") || rest.starts_with("$(") + { + 2 + } else if c == ';' || c == '\n' || c == '`' { + 1 + } else { + 0 + }; + if sep_len > 0 { + segments.push(&command[start..i]); + start = i + sep_len; + if sep_len == 2 { + let _ = iter.next(); + } + } + } + segments.push(&command[start..]); + segments +} + +fn command_heads(command: &str) -> Vec { + let blanked = blank_quoted(command); + split_command_segments(&blanked) + .into_iter() + .filter_map(head_utility) + .collect() +} + +/// Command-head utilities that have a dedicated-tool equivalent. +/// +/// `echo`/`printf` only count when they are the sole head: labelling the +/// sections of a compound command is formatting bash's own output, not +/// talking to the user. +pub fn misused_utilities(command: &str) -> Vec { + let heads = command_heads(command); + let mut seen = HashSet::new(); + let mut misused = Vec::new(); + for name in &heads { + if dedicated_for(name).is_none() { + continue; + } + if (name == "echo" || name == "printf") && heads.len() > 1 { + continue; + } + if seen.insert(name.clone()) { + misused.push(name.clone()); + } + } + misused +} + +fn bash_command<'a>(output: &'a ToolOutput) -> Option<&'a str> { + match output { + ToolOutput::Bash(b) => Some(b.command.as_str()), + ToolOutput::BackgroundTaskStarted(bg) => Some(bg.command.as_str()), + _ => None, + } +} + +/// Resolve `utility → replacement prose` for the utilities that still have +/// a live dedicated tool in this toolset. Missing kinds are skipped so a +/// read-only subagent is not told to use an edit tool it does not have. +fn resolve_replacements( + utilities: &[String], + renderer: Option<&TemplateRenderer>, +) -> Vec<(String, String)> { + let mut out = Vec::new(); + for name in utilities { + let Some(replacement) = dedicated_for(name) else { + continue; + }; + let phrase = match replacement { + DedicatedReplacement::Reply => "your own reply".to_string(), + DedicatedReplacement::Kind(kind) => { + let Some(renderer) = renderer else { + continue; + }; + let tool_name = match (name.as_str(), renderer.tool_for_kind(kind)) { + ("ls", None) => renderer.tool_for_kind(ToolKind::Read), + (_, name) => name, + }; + let Some(tool_name) = tool_name else { + continue; + }; + if name == "ls" && renderer.tool_for_kind(ToolKind::List).is_none() { + format!("the `{tool_name}` tool, which lists a directory when given one") + } else { + format!("the `{tool_name}` tool") + } + } + }; + out.push((name.clone(), phrase)); + } + out +} + +fn bash_discipline_body(replacements: &[(String, String)]) -> String { + let mapping = replacements + .iter() + .map(|(name, phrase)| format!("`{name}` → {phrase}")) + .collect::>() + .join(", "); + format!( + "This bash call did a file operation that has a dedicated tool: {mapping}.\n\ + Wanting to filter, exclude, sort or slice the result is not a reason to reach for bash \ + — run the dedicated tool and filter as you read. Bash is correct here only if the user \ + asked for that exact command, or if what you want back is an aggregate computed over \ + many matches (a count, a frequency sort, a dedup). Use the dedicated tool from now on; \ + do not mention this reminder to the user." + ) +} + +/// Cross-cutting reminder: after a bash call (or a backgrounded bash start) +/// whose command used a dedicated-tool utility as a command head, append a +/// nudge naming the replacement. +pub struct BashDisciplineReminder; + +#[async_trait::async_trait] +impl Reminder for BashDisciplineReminder { + fn requires_expr(&self) -> Expr { + Expr::Value(ToolRequirement::tool_kind(ToolKind::Execute)) + } + + async fn collect_reminders( + &self, + resources: SharedResources, + tool_output: &ToolOutput, + ) -> Vec { + let Some(command) = bash_command(tool_output) else { + return Vec::new(); + }; + let misused = misused_utilities(command); + if misused.is_empty() { + return Vec::new(); + } + let replacements = { + let res = resources.lock().await; + resolve_replacements(&misused, res.get::()) + }; + if replacements.is_empty() { + return Vec::new(); + } + vec![bash_discipline_body(&replacements)] + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::types::output::{BackgroundTaskStarted, BashOutput}; + use crate::types::resources::Resources; + use std::collections::HashMap; + + fn renderer(tools: &[(ToolKind, &str)]) -> TemplateRenderer { + let map = tools + .iter() + .map(|(k, n)| (*k, (*n).to_string())) + .collect::>(); + TemplateRenderer::new(map, HashMap::new()) + } + + fn bash(command: &str) -> ToolOutput { + ToolOutput::Bash(BashOutput { + output: Vec::new(), + output_for_prompt: String::new(), + exit_code: 0, + command: command.to_string(), + truncated: false, + signal: None, + timed_out: false, + description: None, + current_dir: "/tmp".into(), + output_file: String::new(), + total_bytes: 0, + output_delta: None, + was_bare_echo: false, + }) + } + + #[test] + fn command_heads_skip_env_assigns_and_paths() { + assert_eq!( + command_heads("FOO=1 /usr/bin/grep -n todo src"), + vec!["grep".to_string()] + ); + } + + #[test] + fn command_heads_do_not_split_on_single_pipe() { + assert_eq!(command_heads("git log | head -20"), vec!["git".to_string()]); + } + + #[test] + fn command_heads_blank_quoted_separators() { + assert_eq!( + command_heads(r#"bun -e 'f("a && rg x")'"#), + vec!["bun".to_string()] + ); + } + + #[test] + fn command_heads_split_on_and_and_semicolon() { + assert_eq!( + command_heads("cat foo && rg bar; find . -name x"), + vec!["cat".to_string(), "rg".to_string(), "find".to_string()] + ); + } + + #[test] + fn misused_skips_echo_in_compound_command() { + assert_eq!( + misused_utilities("echo section && rg foo"), + vec!["rg".to_string()] + ); + } + + #[test] + fn misused_reports_bare_echo() { + assert_eq!(misused_utilities("echo hello"), vec!["echo".to_string()]); + } + + #[test] + fn misused_dedupes_and_preserves_order() { + assert_eq!( + misused_utilities("cat a; cat b; grep x"), + vec!["cat".to_string(), "grep".to_string()] + ); + } + + #[test] + fn resolve_skips_missing_kinds() { + let r = renderer(&[(ToolKind::Read, "read_file")]); + let got = resolve_replacements(&["cat".into(), "grep".into(), "echo".into()], Some(&r)); + assert_eq!( + got, + vec![ + ("cat".into(), "the `read_file` tool".into()), + ("echo".into(), "your own reply".into()), + ] + ); + } + + #[test] + fn ls_falls_back_to_read_when_list_is_absent() { + let r = renderer(&[(ToolKind::Read, "read_file")]); + let got = resolve_replacements(&["ls".into()], Some(&r)); + assert_eq!( + got, + vec![( + "ls".into(), + "the `read_file` tool, which lists a directory when given one".into() + )] + ); + } + + #[test] + fn strip_removes_both_blocks_and_leading_blank_lines() { + let text = format!( + "\nfix it\n\n\n{}\n\ntool output{}", + wrap_reminder( + "Dispatch checklist — run it before your first tool call this turn: leftover." + ), + wrap_reminder(&bash_discipline_body(&[( + "cat".into(), + "the `read_file` tool".into() + )])), + ); + let stripped = strip_tool_promotion_reminders(&text); + assert_eq!( + stripped, + "\nfix it\n\n\ntool output" + ); + assert!(!stripped.contains("Dispatch checklist")); + assert!(!stripped.contains("This bash call did a file operation")); + } + + #[test] + fn append_wraps_and_separates() { + let out = + append_dispatch_checklist("\nhi\n", "Dispatch checklist — x"); + assert!(out.starts_with( + "\nhi\n\n\n\nDispatch checklist" + )); + assert!(out.ends_with("")); + } + + #[test] + fn should_inject_gates() { + assert!(should_inject_dispatch_checklist(false, false, true, true)); + assert!(!should_inject_dispatch_checklist(true, false, true, true)); + assert!(!should_inject_dispatch_checklist(false, true, true, true)); + assert!(!should_inject_dispatch_checklist(false, false, false, true)); + assert!(!should_inject_dispatch_checklist(false, false, true, false)); + } + + #[test] + fn checklist_template_renders_without_leftover_markers() { + let r = renderer(&[ + (ToolKind::Task, "spawn_subagent"), + (ToolKind::Search, "grep"), + (ToolKind::Read, "read_file"), + (ToolKind::Workflow, "workflow"), + ]); + let body = r + .render_with_extra(DISPATCH_CHECKLIST_TEMPLATE, &serde_json::json!({})) + .expect("checklist template must render"); + assert!(!body.contains("${{"), "leftover interpolation: {body}"); + assert!(!body.contains("${%"), "leftover tag: {body}"); + assert!(body.contains("`spawn_subagent` with `explore`")); + assert!(body.contains("`grep`")); + assert!(body.contains("`read_file`")); + assert!(body.contains("`workflow` named `deep-research`")); + assert!(body.starts_with("Dispatch checklist")); + } + + #[test] + fn checklist_template_falls_back_when_kinds_are_missing() { + let r = renderer(&[]); + let body = r + .render_with_extra(DISPATCH_CHECKLIST_TEMPLATE, &serde_json::json!({})) + .expect("checklist template must render with empty toolset"); + assert!(!body.contains("${{"), "leftover interpolation: {body}"); + assert!(body.contains("/deep-research")); + assert!(!body.contains("spawn_subagent")); + } + + #[tokio::test] + async fn reminder_fires_on_cat_when_read_exists() { + let mut res = Resources::new(); + res.insert(renderer(&[(ToolKind::Read, "read_file")])); + let shared = res.into_shared(); + let reminders = BashDisciplineReminder + .collect_reminders(shared, &bash("cat src/main.rs")) + .await; + assert_eq!(reminders.len(), 1); + assert!(reminders[0].contains("`cat` → the `read_file` tool")); + assert!(reminders[0].contains("do not mention this reminder to the user")); + } + + #[tokio::test] + async fn reminder_silent_when_no_dedicated_tool_is_live() { + let shared = Resources::new().into_shared(); + let reminders = BashDisciplineReminder + .collect_reminders(shared, &bash("cat src/main.rs")) + .await; + assert!(reminders.is_empty()); + } + + #[tokio::test] + async fn reminder_fires_on_backgrounded_bash() { + let mut res = Resources::new(); + res.insert(renderer(&[(ToolKind::Search, "grep")])); + let shared = res.into_shared(); + let output = ToolOutput::BackgroundTaskStarted(BackgroundTaskStarted { + task_id: "t1".into(), + task_type: "bash".into(), + output_file: "/tmp/out".into(), + status: "running".into(), + command: "rg TODO crates".into(), + summary: "running".into(), + retrieval_hint: String::new(), + pre_formatted: None, + pid: None, + }); + let reminders = BashDisciplineReminder + .collect_reminders(shared, &output) + .await; + assert_eq!(reminders.len(), 1); + assert!(reminders[0].contains("`rg` → the `grep` tool")); + } + + #[tokio::test] + async fn reminder_ignores_non_bash_output() { + let mut res = Resources::new(); + res.insert(renderer(&[(ToolKind::Read, "read_file")])); + let shared = res.into_shared(); + let reminders = BashDisciplineReminder + .collect_reminders( + shared, + &ToolOutput::Text(crate::types::output::TextOutput { + text: "cat foo".into(), + consumed_completion_task_id: None, + }), + ) + .await; + assert!(reminders.is_empty()); + } +}