Synced from monorepo
Synced from monorepo Changes: - Workspace server: surface preview-proxy metrics through the hub metric pump - Shell: reclaim a session’s retained state in one entry - Shell: reclaim a session’s resident state in one entry - Pager: withhold key event types from Alacritty builds that double keys - Tools: cancel a session’s subagents when it closes - Pager: keep the whole plan in scrollback and separate reasoning from output in minimal mode - Pager: probe terminal version over DA2 and include it with feedback - SuperGrok Plus: identity, CLI, and analytics tier surfaces - Shell: inherit the session process scope into subagents - Pager: build @-file-search matcher lazily on first use - Tools: fix description and output contradictions in tool definitions - Workspace: degrade @-file-search instead of aborting on thread exhaustion - Tools: reap a session’s LSP servers when it closes - Tools: fix contradictions and defects in tool descriptions, schemas, and harness pools - MCP: reap stdio MCP children on session close - Shell: reuse spawn-time skill discovery for session telemetry - Tools: stop leaking shell-wrapper positional params into sourced scripts (fixes activate_conda under persistent/static shell) - Shell: self-heal corrupt session-search SQLite cache - Workspace: cap workspace-server tokio workers on many-core hosts - Shell: reap a session’s child processes when it closes - Crash handler: capture SIGABRT so panic-aborts leave crash reports - CLI chat proxy: team-scoped Grok Code managed-config admin routes - MCP: add CLI enable/disable for MCP servers - Shell: cap tokio worker threads for startup thread demand - Workspace: harden git_commit and add git_sync_base operation - Circuit breaker: add feature-gated gRPC retry policy Source-Revision: 2a818575225183d8ca915f5632a09b8067b5156a
This commit is contained in:
grokkybara[bot]
parent
02d9359435
commit
5da6962e4a
192 changed files with 10337 additions and 3421 deletions
|
|
@ -420,6 +420,16 @@ impl ToolBridge {
|
|||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
/// Every skill name from session-start discovery
|
||||
/// (see `SkillManager::discovery_snapshot_names`).
|
||||
pub async fn skill_discovery_snapshot_names(&self) -> Vec<String> {
|
||||
let registry = &*self.registry;
|
||||
let res = registry.resources.lock().await;
|
||||
res.get::<crate::types::skill_discovery_tracker::SkillManager>()
|
||||
.map(|m| m.discovery_snapshot_names().to_vec())
|
||||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
/// Get the paths that have been reminded about.
|
||||
pub async fn agents_md_reminded_paths(&self) -> std::collections::HashSet<std::path::PathBuf> {
|
||||
let registry = &*self.registry;
|
||||
|
|
|
|||
|
|
@ -424,6 +424,22 @@ impl ShellState {
|
|||
// Re-export GROK_AGENT=1 after snapshot eval so agent-definition
|
||||
// selectors (or other values) from prior shells cannot clear the
|
||||
// agent sentinel (process env alone is insufficient).
|
||||
//
|
||||
// Copy $1/$2 into plain variables and clear the positional
|
||||
// parameters (`builtin set --`) BEFORE eval'ing the user
|
||||
// command: `source <script>` with no arguments makes the
|
||||
// sourced script inherit the caller's positional parameters,
|
||||
// so e.g. conda's `bin/activate` (which forwards "$@" to
|
||||
// `conda activate`) would receive the entire wrapped command
|
||||
// string as an environment name. Clearing them matches the
|
||||
// plain `bash -c "<command>"` execution path, where $# is 0.
|
||||
//
|
||||
// The snapshot can restore `allexport` (set -a), which would
|
||||
// auto-export the temp variable — so strip the export
|
||||
// attribute post-assignment (`declare +x`; inline `declare +x
|
||||
// var=value` does NOT beat allexport) and unset it after the
|
||||
// eval so it can never reach child processes or the state
|
||||
// dump (`export -p`).
|
||||
"{dump_script} \
|
||||
snap=$(command cat <&3) && builtin shopt -s extglob && builtin eval -- \"$snap\" && \
|
||||
{{ builtin set +u 2>/dev/null || true; \
|
||||
|
|
@ -431,10 +447,14 @@ impl ShellState {
|
|||
builtin export PWD=\"$(builtin pwd)\"; \
|
||||
builtin shopt -s expand_aliases 2>/dev/null; {sudo_inject}{search_inject}\
|
||||
builtin printf '%s' \"${{2:-}}\"; \
|
||||
builtin eval \"$1\" 2>&1; }}; \
|
||||
COMMAND_EXIT_CODE=$?; {dump_fn} >&4; builtin exit $COMMAND_EXIT_CODE"
|
||||
__grok_user_cmd=\"$1\"; builtin declare +x __grok_user_cmd 2>/dev/null; builtin set --; \
|
||||
builtin eval \"$__grok_user_cmd\" 2>&1; }}; \
|
||||
COMMAND_EXIT_CODE=$?; builtin unset __grok_user_cmd 2>/dev/null; {dump_fn} >&4; builtin exit $COMMAND_EXIT_CODE"
|
||||
),
|
||||
// After snapshot restore: force nonomatch so login dumps cannot re-arm NOMATCH for model globs.
|
||||
// See the bash wrapper comment for why positional parameters are
|
||||
// cleared before the user-command eval (zsh's `source`/`.` inherits
|
||||
// them identically).
|
||||
ShellKind::Zsh => format!(
|
||||
"{dump_script} \
|
||||
snap=$(command cat <&3); \
|
||||
|
|
@ -447,8 +467,9 @@ impl ShellState {
|
|||
builtin export PWD=\"$(builtin pwd)\"; \
|
||||
builtin setopt aliases 2>/dev/null; {sudo_inject}{search_inject}\
|
||||
builtin printf '%s' \"${{2:-}}\"; \
|
||||
builtin eval \"$1\" 2>&1; }}; \
|
||||
COMMAND_EXIT_CODE=$?; {dump_fn} >&4; builtin exit $COMMAND_EXIT_CODE"
|
||||
__grok_user_cmd=\"$1\"; builtin typeset +x __grok_user_cmd 2>/dev/null; builtin set --; \
|
||||
builtin eval \"$__grok_user_cmd\" 2>&1; }}; \
|
||||
COMMAND_EXIT_CODE=$?; builtin unset __grok_user_cmd 2>/dev/null; {dump_fn} >&4; builtin exit $COMMAND_EXIT_CODE"
|
||||
),
|
||||
};
|
||||
|
||||
|
|
@ -1119,6 +1140,97 @@ mod tests {
|
|||
assert_eq!(stdout.trim(), "hello world");
|
||||
}
|
||||
|
||||
/// Regression test: a script sourced WITHOUT arguments by the user command
|
||||
/// must not see the wrapper's positional parameters ($1 = the whole
|
||||
/// command string, $2 = the spawn notice). Conda's `bin/activate` forwards
|
||||
/// "$@" to `conda activate`, so a leak makes every `activate_conda`-
|
||||
/// prefixed command fail with `EnvironmentLocationNotFound: Not a conda
|
||||
/// environment: <cwd>/<the entire command string>`.
|
||||
#[tokio::test]
|
||||
async fn test_sourced_script_does_not_inherit_wrapper_positional_args_bash() {
|
||||
if !bash_available() {
|
||||
return;
|
||||
}
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let probe = dir.path().join("activate_probe.sh");
|
||||
std::fs::write(&probe, "echo \"SOURCED_ARGC=$#\"\n").unwrap();
|
||||
|
||||
let cwd = std::env::current_dir().unwrap();
|
||||
let mut state = ShellState::init(ShellKind::Bash, &cwd, None).await.unwrap();
|
||||
|
||||
let (code, stdout) = run_command(
|
||||
&mut state,
|
||||
&format!("source {} && echo AFTER_SOURCE_OK", probe.display()),
|
||||
)
|
||||
.await;
|
||||
assert_eq!(code, 0, "command failed, stdout={stdout:?}");
|
||||
assert!(
|
||||
stdout.contains("SOURCED_ARGC=0"),
|
||||
"sourced script must see zero positional args, got: {stdout:?}"
|
||||
);
|
||||
assert!(stdout.contains("AFTER_SOURCE_OK"), "got: {stdout:?}");
|
||||
}
|
||||
|
||||
/// Regression test: with `allexport` active (restored from the snapshot
|
||||
/// after the model runs `set -a`), the wrapper's `__grok_user_cmd` temp
|
||||
/// variable must not leak into child-process environments or persist into
|
||||
/// subsequent commands via the state dump.
|
||||
#[tokio::test]
|
||||
async fn test_user_cmd_var_not_exported_under_allexport_bash() {
|
||||
if !bash_available() {
|
||||
return;
|
||||
}
|
||||
let cwd = std::env::current_dir().unwrap();
|
||||
let mut state = ShellState::init(ShellKind::Bash, &cwd, None).await.unwrap();
|
||||
|
||||
// Turn on allexport; the option is captured by the dump and replayed
|
||||
// into every subsequent command's shell.
|
||||
let (code, _) = run_command(&mut state, "set -a").await;
|
||||
assert_eq!(code, 0);
|
||||
|
||||
// This command's wrapper assigns __grok_user_cmd under allexport.
|
||||
// printenv only sees exported vars — it must not see the temp var
|
||||
// (neither from this command's own assignment nor re-exported from a
|
||||
// previous command's state dump).
|
||||
let (code, stdout) = run_command(
|
||||
&mut state,
|
||||
"printenv __grok_user_cmd >/dev/null 2>&1 && echo LEAKED_TO_ENV || echo ENV_CLEAN",
|
||||
)
|
||||
.await;
|
||||
assert_eq!(code, 0);
|
||||
assert!(
|
||||
stdout.contains("ENV_CLEAN") && !stdout.contains("LEAKED_TO_ENV"),
|
||||
"temp var must not be exported to child processes under allexport, got: {stdout:?}"
|
||||
);
|
||||
}
|
||||
|
||||
/// Same as the bash variant: zsh's `source`/`.` also inherits the caller's
|
||||
/// positional parameters when invoked without arguments.
|
||||
#[tokio::test]
|
||||
async fn test_sourced_script_does_not_inherit_wrapper_positional_args_zsh() {
|
||||
if !zsh_available() {
|
||||
return;
|
||||
}
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let probe = dir.path().join("activate_probe.sh");
|
||||
std::fs::write(&probe, "echo \"SOURCED_ARGC=$#\"\n").unwrap();
|
||||
|
||||
let cwd = std::env::current_dir().unwrap();
|
||||
let mut state = ShellState::init(ShellKind::Zsh, &cwd, None).await.unwrap();
|
||||
|
||||
let (code, stdout) = run_command(
|
||||
&mut state,
|
||||
&format!("source {} && echo AFTER_SOURCE_OK", probe.display()),
|
||||
)
|
||||
.await;
|
||||
assert_eq!(code, 0, "command failed, stdout={stdout:?}");
|
||||
assert!(
|
||||
stdout.contains("SOURCED_ARGC=0"),
|
||||
"sourced script must see zero positional args, got: {stdout:?}"
|
||||
);
|
||||
assert!(stdout.contains("AFTER_SOURCE_OK"), "got: {stdout:?}");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_alias_persists_across_commands() {
|
||||
if !bash_available() {
|
||||
|
|
|
|||
|
|
@ -128,6 +128,18 @@ impl StaticShellSnapshot {
|
|||
let (state_in_read, state_in_write) = os_pipe()?;
|
||||
set_cloexec(&state_in_write)?;
|
||||
|
||||
// Copy $1 into a plain variable and clear the positional parameters
|
||||
// (`builtin set --`) BEFORE eval'ing the user command: `source
|
||||
// <script>` with no arguments makes the sourced script inherit the
|
||||
// caller's positional parameters, so e.g. conda's `bin/activate`
|
||||
// (which forwards "$@" to `conda activate`) would receive the entire
|
||||
// wrapped command string as an environment name. Clearing them
|
||||
// matches the plain `bash -c "<command>"` execution path, where $# is 0.
|
||||
//
|
||||
// The login snapshot can restore `allexport` (set -a), which would
|
||||
// auto-export the temp variable into the user command's child
|
||||
// processes — strip the export attribute post-assignment (inline
|
||||
// `declare +x var=value` does NOT beat allexport).
|
||||
let wrapper = match self.shell {
|
||||
UnixShellKind::Bash => format!(
|
||||
"snap=$(command cat <&3); builtin shopt -s extglob 2>/dev/null; \
|
||||
|
|
@ -135,7 +147,8 @@ impl StaticShellSnapshot {
|
|||
builtin eval -- \"$snap\"; \
|
||||
builtin export GROK_AGENT=1; \
|
||||
builtin export PWD=\"$(builtin pwd)\"; {sudo_inject}{search_inject}\
|
||||
builtin eval \"$1\" 2>&1"
|
||||
__grok_user_cmd=\"$1\"; builtin declare +x __grok_user_cmd 2>/dev/null; builtin set --; \
|
||||
builtin eval \"$__grok_user_cmd\" 2>&1"
|
||||
),
|
||||
UnixShellKind::Zsh => format!(
|
||||
"snap=$(command cat <&3); \
|
||||
|
|
@ -144,7 +157,8 @@ impl StaticShellSnapshot {
|
|||
builtin export GROK_AGENT=1; \
|
||||
builtin export PWD=\"$(builtin pwd)\"; \
|
||||
builtin setopt aliases 2>/dev/null; {sudo_inject}{search_inject}\
|
||||
builtin eval \"$1\" 2>&1"
|
||||
__grok_user_cmd=\"$1\"; builtin typeset +x __grok_user_cmd 2>/dev/null; builtin set --; \
|
||||
builtin eval \"$__grok_user_cmd\" 2>&1"
|
||||
),
|
||||
};
|
||||
|
||||
|
|
@ -308,4 +322,33 @@ mod tests {
|
|||
"empty snapshot must degrade to a plain shell"
|
||||
);
|
||||
}
|
||||
|
||||
/// Regression test: a script sourced WITHOUT arguments by the user command
|
||||
/// must not see the wrapper's positional parameters ($1 = the whole
|
||||
/// command string). Conda's `bin/activate` forwards "$@" to `conda
|
||||
/// activate`, so a leak makes every `activate_conda`-prefixed command fail
|
||||
/// with `EnvironmentLocationNotFound: Not a conda environment: <cwd>/<the
|
||||
/// entire command string>`.
|
||||
#[tokio::test]
|
||||
async fn sourced_script_does_not_inherit_wrapper_positional_args() {
|
||||
if !bash_available() {
|
||||
return;
|
||||
}
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
let probe = dir.path().join("activate_probe.sh");
|
||||
std::fs::write(&probe, "echo \"SOURCED_ARGC=$#\"\n").unwrap();
|
||||
|
||||
let output = run_static(
|
||||
"",
|
||||
&format!("source {} && echo AFTER_SOURCE_OK", probe.display()),
|
||||
)
|
||||
.await;
|
||||
assert!(output.status.success(), "command failed: {output:?}");
|
||||
let stdout = String::from_utf8_lossy(&output.stdout);
|
||||
assert!(
|
||||
stdout.contains("SOURCED_ARGC=0"),
|
||||
"sourced script must see zero positional args, got: {stdout:?}"
|
||||
);
|
||||
assert!(stdout.contains("AFTER_SOURCE_OK"), "got: {stdout:?}");
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -41,6 +41,9 @@ Each operation starts with one of three headers:
|
|||
May be immediately followed by *** Move to: <new path> if you want to rename the file.
|
||||
Then one or more “hunks”, each introduced by @@ (optionally followed by a hunk header).
|
||||
Within a hunk each line starts with:
|
||||
+ for inserted text,
|
||||
- for removed text, or
|
||||
(a space) for unchanged context.
|
||||
|
||||
For instructions on [context_before] and [context_after]:
|
||||
- By default, show 3 lines of code immediately above and 3 lines immediately below each change. If a change is within 3 lines of a previous change, do NOT duplicate the first change’s [context_after] lines in the second change’s [context_before] lines.
|
||||
|
|
@ -277,7 +280,7 @@ impl xai_tool_runtime::Tool for ApplyPatchTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"apply_patch",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -169,7 +169,7 @@ impl xai_tool_runtime::Tool for CodexGrepFilesTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"grep_files",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -45,6 +45,7 @@ pub struct CodexListDirInput {
|
|||
|
||||
/// The entry number to start listing from. Must be 1 or greater.
|
||||
#[serde(default = "default_offset")]
|
||||
#[schemars(range(min = 1))]
|
||||
pub offset: usize,
|
||||
|
||||
/// The maximum number of entries to return.
|
||||
|
|
@ -53,6 +54,7 @@ pub struct CodexListDirInput {
|
|||
|
||||
/// The maximum directory depth to traverse. Must be 1 or greater.
|
||||
#[serde(default = "default_depth")]
|
||||
#[schemars(range(min = 1))]
|
||||
pub depth: usize,
|
||||
}
|
||||
|
||||
|
|
@ -285,7 +287,7 @@ impl xai_tool_runtime::Tool for CodexListDirTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"list_dir",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -31,6 +31,7 @@ pub struct CodexReadFileInput {
|
|||
|
||||
/// The line number to start reading from. Must be 1 or greater.
|
||||
#[serde(default = "defaults::offset")]
|
||||
#[schemars(range(min = 1))]
|
||||
pub offset: usize,
|
||||
|
||||
/// The maximum number of lines to return.
|
||||
|
|
@ -158,7 +159,7 @@ impl xai_tool_runtime::Tool for CodexReadFileTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"read_file",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -329,7 +329,7 @@ impl xai_tool_runtime::Tool for AskUserQuestionTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"ask_user_question",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1,6 +1,10 @@
|
|||
//! `run_terminal_cmd` (Bash) tool — new architecture (`Tool` trait).
|
||||
//!
|
||||
//! Executes bash commands in a persistent shell session with optional timeout.
|
||||
//! Executes bash commands with optional timeout. Whether shell state (cwd,
|
||||
//! env vars, aliases) persists between calls depends on the `Terminal`
|
||||
//! backend: the local backend emulates persistence by capturing and
|
||||
//! replaying state across commands (see [`crate::computer::local::shell_state`]);
|
||||
//! other backends may run each command in a fresh session.
|
||||
//! Supports both foreground (blocking) and background (returns task_id) execution.
|
||||
//!
|
||||
//! Optional `find`→`bfs` / `grep`→`ugrep` shadows: see
|
||||
|
|
@ -259,12 +263,12 @@ pub struct BashToolInput {
|
|||
#[cfg_attr(not(unix), schemars(description = "The command to run."))]
|
||||
pub command: String,
|
||||
|
||||
/// Optional timeout in milliseconds (max 300000). Default: 120000 (2 minutes).
|
||||
/// `timeout: 0` in background mode disables the wrapper timeout entirely;
|
||||
/// the task runs until it exits or is killed via the kill task tool.
|
||||
/// Optional timeout in milliseconds (max 300000). Default: 120000
|
||||
/// (2 minutes), enforced for foreground commands only. Background
|
||||
/// semantics live in the tool-description usage notes.
|
||||
// keep in sync with the rustdoc above
|
||||
#[schemars(
|
||||
description = "Optional timeout in milliseconds (max 300000). Default: 120000 (2 minutes). `timeout: 0` in background mode disables the wrapper timeout entirely; the task runs until it exits or is killed via the kill task tool.",
|
||||
description = "Optional timeout in milliseconds (max 300000). Default: 120000 (2 minutes), enforced for foreground commands only.",
|
||||
default = "schema_default_timeout_ms"
|
||||
)]
|
||||
// Some models serialize numeric tool args
|
||||
|
|
@ -289,8 +293,11 @@ pub struct BashToolInput {
|
|||
/// Returns a task id immediately while the command keeps running in the background; you are notified on completion, so do not poll or sleep-wait for it.
|
||||
// "task id" stays plain English: the kill/get-output input params are
|
||||
// renameable, so naming a literal key here goes stale after randomization.
|
||||
// The notification sentence renders only when the client delivers system
|
||||
// reminders (`system_reminders_enabled` template flag); otherwise it
|
||||
// points at the get-output tool when one is served.
|
||||
#[schemars(
|
||||
description = "Set to true for long-running commands that should run in the background (e.g., dev servers, long builds). Returns a task id immediately while the command keeps running in the background; you are notified on completion, so do not poll or sleep-wait for it."
|
||||
description = "Set to true for long-running commands that should run in the background (e.g., dev servers, long builds). Returns a task id immediately while the command keeps running in the background${%- if system_reminders_enabled %}; you are notified on completion, so do not poll or sleep-wait for it${%- elif tools.by_kind.background_task_action %}; check on it later with the ${{ tools.by_kind.background_task_action }} tool${%- endif %}."
|
||||
)]
|
||||
#[serde(
|
||||
default,
|
||||
|
|
@ -1251,7 +1258,11 @@ impl BashTool {
|
|||
/// without the floor the schema would advertise `max 0` while timeout
|
||||
/// resolution enforces a 1ms ceiling — a model-visible mismatch. Flooring
|
||||
/// here keeps the advertised max equal to the enforced max (≥1ms).
|
||||
pub(crate) fn effective_max_timeout_ms(params: &BashParams) -> u64 {
|
||||
///
|
||||
/// `pub` (not `pub(crate)`): the cursor `Shell` adapter
|
||||
/// (xai-grok-cursor) uses this ceiling to report the true FG wait in
|
||||
/// its auto-background template.
|
||||
pub fn effective_max_timeout_ms(params: &BashParams) -> u64 {
|
||||
let configured = params
|
||||
.max_timeout_secs
|
||||
.filter(|s| *s > 0.0)
|
||||
|
|
@ -1403,9 +1414,12 @@ impl BashTool {
|
|||
) -> Result<String, xai_tool_runtime::ToolError> {
|
||||
let res = resources.lock().await;
|
||||
let renderer = res.require::<TemplateRenderer>()?;
|
||||
// Presence-aware lookup (not a template render): a missing kind
|
||||
// renders as empty-`Ok`, so a `Result` fallback never fires.
|
||||
let get_task_name = renderer
|
||||
.render("${{ tools.by_kind.background_task_action }}")
|
||||
.unwrap_or_else(|_| "get_command_or_subagent_output".to_string());
|
||||
.tool_for_kind(ToolKind::BackgroundTaskAction)
|
||||
.unwrap_or("get_task_output")
|
||||
.to_string();
|
||||
let task_ids_param = renderer
|
||||
.param_for_kind(ToolKind::BackgroundTaskAction, "task_ids")
|
||||
.unwrap_or("task_ids");
|
||||
|
|
@ -1433,22 +1447,24 @@ impl BashTool {
|
|||
{
|
||||
let max_ms = Self::effective_max_timeout_ms(params);
|
||||
let default_ms = Self::effective_default_timeout_ms(params);
|
||||
// `max_timeout_secs` is a foreground-only ceiling; background
|
||||
// `{name}: 0` is always unbounded, so this note is
|
||||
// unconditional.
|
||||
let bg_zero = format!(
|
||||
"`{timeout_param_name}: 0` in background mode disables the wrapper timeout entirely; the task runs until it exits or is killed via the kill task tool."
|
||||
);
|
||||
// The default and max ceiling are foreground-only.
|
||||
// Background semantics live in the tool-description usage
|
||||
// notes (single copy); the "foreground only" scoping here
|
||||
// keeps the property from contradicting them.
|
||||
// Keep main-style auto-bg wording (no FG-budget ms advertised).
|
||||
// Follow-up: surface effective_auto_bg_wait_ms / FG budget here
|
||||
// once we deliberately change model-facing copy.
|
||||
let desc = if auto_bg {
|
||||
let desc = if !background_enabled {
|
||||
format!(
|
||||
"Optional {timeout_param_name} in milliseconds (max {max_ms}). Default: {default_ms}. If not specified, commands exceeding the default timeout will be automatically backgrounded. {bg_zero}"
|
||||
"Optional {timeout_param_name} in milliseconds (max {max_ms}). Default: {default_ms}."
|
||||
)
|
||||
} else if auto_bg {
|
||||
format!(
|
||||
"Optional {timeout_param_name} in milliseconds (max {max_ms}). Default: {default_ms}; foreground commands exceeding it are automatically backgrounded."
|
||||
)
|
||||
} else {
|
||||
format!(
|
||||
"Optional {timeout_param_name} in milliseconds (max {max_ms}). Default: {default_ms}. {bg_zero}"
|
||||
"Optional {timeout_param_name} in milliseconds (max {max_ms}). Default: {default_ms}, enforced for foreground commands only."
|
||||
)
|
||||
};
|
||||
timeout_prop.insert("description".to_string(), serde_json::json!(desc));
|
||||
|
|
@ -1488,8 +1504,7 @@ impl BashTool {
|
|||
renderer
|
||||
.render_with_extra(raw_desc, &extras)
|
||||
.unwrap_or_else(|e| {
|
||||
tracing::warn!("Description template render failed, using raw: {e}");
|
||||
raw_desc.to_string()
|
||||
crate::types::template_renderer::strip_markers_on_render_failure(raw_desc, &e)
|
||||
})
|
||||
}
|
||||
|
||||
|
|
@ -1508,10 +1523,10 @@ impl BashTool {
|
|||
r#"Run a ${%- if is_windows %} shell command${%- else %} bash command${%- endif %} and return its output.
|
||||
|
||||
Usage notes:
|
||||
- You can specify an optional ${{ params.execute.timeout }} in milliseconds (up to ${{ max_timeout_ms | default(300000) }}ms). ${%- if auto_background_on_timeout %} If not specified, commands exceeding the default timeout will be automatically backgrounded instead of killed. You will receive a task id to check output later.${%- else %} If not specified, commands will timeout after ${{ default_timeout_ms | default(120000) }}ms.${%- endif %}
|
||||
- Timeout enforcement: when the timeout fires, the wrapper${%- if is_windows %} terminates the child's Job Object, killing every descendant process immediately (no graceful-termination grace period).${%- else %} kills the child process group (SIGTERM, escalated to SIGKILL after a ~1s grace period). Descendants that did not detach via `setsid` / `nohup` will also be killed.${%- endif %} `${{ params.execute.timeout }}: 0` in `${%- if params is defined and params.execute is defined and params.execute.is_background %}${{ params.execute.is_background }}${%- else %}background${%- endif %}: true` mode disables the wrapper timeout entirely; the child's lifetime is owned by the model via ${{ tools.by_kind.kill_task_action }}.
|
||||
- If the output exceeds {max_output_bytes} characters, output will be truncated before being returned to you.
|
||||
- You can use the ${{ params.execute.is_background }} parameter to run the command in the background (e.g., dev servers, long builds): it returns a task id immediately and keeps running in the background. You are notified on completion, so do not poll or sleep-wait for it.${%- if has_unix_utilities %} You do not need to use '&' at the end of the command when using this parameter.${%- endif %}
|
||||
- You can specify an optional ${{ params.execute.timeout }} in milliseconds (up to ${{ max_timeout_ms | default(300000) }}ms). ${%- if auto_background_on_timeout %} If not specified, foreground commands exceeding the default timeout will be automatically backgrounded instead of killed. You will receive a task id to check output later.${%- else %} If not specified, foreground commands will timeout after ${{ default_timeout_ms | default(120000) }}ms.${%- endif %} Background tasks are not bounded by the default: with ${{ params.execute.timeout }} omitted or 0 they run until they exit or are killed; a positive ${{ params.execute.timeout }} still applies.
|
||||
- Timeout enforcement: when the timeout fires, the wrapper${%- if is_windows %} terminates the child's Job Object, killing every descendant process immediately (no graceful-termination grace period).${%- else %} kills the child process group (SIGTERM, escalated to SIGKILL after a ~1s grace period). Descendants that did not detach via `setsid` / `nohup` will also be killed.${%- endif %} `${{ params.execute.timeout }}: 0` in `${%- if params is defined and params.execute is defined and params.execute.is_background %}${{ params.execute.is_background }}${%- else %}background${%- endif %}: true` mode disables the wrapper timeout entirely${%- if tools.by_kind.kill_task_action %}; the child's lifetime is owned by the model via ${{ tools.by_kind.kill_task_action }}${%- endif %}.
|
||||
- If the output exceeds {max_output_bytes} characters, the middle is truncated (you keep the beginning and end) and the result includes the path to a log file with the full output, which you can read or search.
|
||||
- You can use the ${{ params.execute.is_background }} parameter to run the command in the background (e.g., dev servers, long builds): it returns a task id immediately and keeps running in the background.${%- if system_reminders_enabled %} You are notified on completion, so do not poll or sleep-wait for it.${%- elif tools.by_kind.background_task_action %} Check on it later with the ${{ tools.by_kind.background_task_action }} tool.${%- endif %}${%- if has_unix_utilities %} You do not need to use '&' at the end of the command when using this parameter.${%- endif %}
|
||||
${%- if shell_uses_semicolon %}
|
||||
- '&&' is not supported in this shell; chain sequential commands with ';'.
|
||||
${%- endif %}
|
||||
|
|
@ -1679,7 +1694,7 @@ impl xai_tool_runtime::Tool for BashTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"run_terminal_cmd",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
@ -1946,13 +1961,16 @@ impl xai_tool_runtime::Tool for BashTool {
|
|||
is_legacy,
|
||||
) {
|
||||
// `is_background` is the canonical param key (the input-schema
|
||||
// property name); `background` has no entry and resolves to "".
|
||||
let bg_param_name = crate::types::template_renderer::TemplateRenderer::resolve(
|
||||
&resources,
|
||||
"${{ params.execute.is_background }}",
|
||||
)
|
||||
.await
|
||||
.unwrap_or_else(|_| "is_background".to_string());
|
||||
// property name). Presence-aware lookup (not a template render):
|
||||
// a missing entry renders as empty-`Ok`, so a `Result` fallback
|
||||
// never fires.
|
||||
let bg_param_name = {
|
||||
let res = resources.lock().await;
|
||||
res.get::<TemplateRenderer>()
|
||||
.and_then(|r| r.param_for_kind(ToolKind::Execute, "is_background"))
|
||||
.unwrap_or("is_background")
|
||||
.to_string()
|
||||
};
|
||||
let message = match violation {
|
||||
BackgroundOpViolation::Bash => Self::background_operator_validation_message(
|
||||
is_legacy,
|
||||
|
|
@ -3409,6 +3427,38 @@ mod tests {
|
|||
}
|
||||
}
|
||||
|
||||
/// The get-output tool is absent from the finalized toolset (no
|
||||
/// `BackgroundTaskAction` mapping): the retrieval hint must fall back to
|
||||
/// the canonical `get_task_output` name instead of rendering an empty
|
||||
/// tool name. A missing kind renders as empty-`Ok` (lenient undefined),
|
||||
/// so the old `Result`-based fallback never fired.
|
||||
#[tokio::test]
|
||||
async fn background_hint_falls_back_when_get_output_tool_absent() {
|
||||
let mut resources = make_resources(MockTerminal::background_ok("t2"));
|
||||
resources.insert(TemplateRenderer::new(HashMap::new(), HashMap::new()));
|
||||
|
||||
let tool = BashTool;
|
||||
let result = xai_tool_runtime::Tool::run(
|
||||
&tool,
|
||||
test_ctx(resources.into_shared()),
|
||||
make_bg_input("sleep 60"),
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
match result {
|
||||
BashToolOutput::Background(bg) => {
|
||||
assert!(
|
||||
bg.retrieval_hint
|
||||
.contains("Use get_task_output tool with task_ids=[\"t2\"]"),
|
||||
"Hint should fall back to canonical names: {}",
|
||||
bg.retrieval_hint
|
||||
);
|
||||
}
|
||||
BashToolOutput::Foreground(_) => panic!("Expected background output"),
|
||||
}
|
||||
}
|
||||
|
||||
// -----------------------------------------------------------------------
|
||||
// format_default_prompt tests
|
||||
// -----------------------------------------------------------------------
|
||||
|
|
@ -4347,13 +4397,11 @@ mod tests {
|
|||
.as_str()
|
||||
.expect("max_wait description");
|
||||
assert!(
|
||||
desc.contains("Optional max_wait in milliseconds")
|
||||
&& desc.contains("`max_wait: 0`"),
|
||||
desc.contains("Optional max_wait in milliseconds"),
|
||||
"renamed timeout must appear in property description:\n{desc}"
|
||||
);
|
||||
assert!(
|
||||
!desc.contains("`timeout: 0`")
|
||||
&& !desc.contains("Optional timeout in milliseconds"),
|
||||
!desc.contains("Optional timeout in milliseconds"),
|
||||
"canonical timeout must not remain in property description:\n{desc}"
|
||||
);
|
||||
}
|
||||
|
|
@ -4397,7 +4445,7 @@ mod tests {
|
|||
.as_str()
|
||||
.expect("timeout description");
|
||||
assert!(
|
||||
desc.contains("Optional timeout in milliseconds") && desc.contains("`timeout: 0`"),
|
||||
desc.contains("Optional timeout in milliseconds"),
|
||||
"property description must match schema key, not kind-wide alias:\n{desc}"
|
||||
);
|
||||
assert!(
|
||||
|
|
|
|||
|
|
@ -86,7 +86,7 @@ impl xai_tool_runtime::Tool for EnterPlanModeTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"enter_plan_mode",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
@ -121,17 +121,22 @@ impl xai_tool_runtime::Tool for EnterPlanModeTool {
|
|||
let (seed_target, plan_file_path) = resolve_plan_file_path(&res);
|
||||
|
||||
// Resolve client-facing tool names via TemplateRenderer.
|
||||
// Presence-aware lookups (not template renders): a missing kind
|
||||
// renders as empty-`Ok`, so a `Result` fallback never fires.
|
||||
let hints = if let Some(renderer) = res.get::<TemplateRenderer>() {
|
||||
EnterPlanModeToolHints {
|
||||
ask_user: renderer
|
||||
.render("${{ tools.by_kind.ask_user }}")
|
||||
.unwrap_or_else(|_| "ask_user_question".to_owned()),
|
||||
.tool_for_kind(crate::types::tool::ToolKind::AskUser)
|
||||
.unwrap_or("ask_user_question")
|
||||
.to_owned(),
|
||||
exit_plan: renderer
|
||||
.render("${{ tools.by_kind.exit_plan }}")
|
||||
.unwrap_or_else(|_| "exit_plan_mode".to_owned()),
|
||||
.tool_for_kind(crate::types::tool::ToolKind::ExitPlan)
|
||||
.unwrap_or("exit_plan_mode")
|
||||
.to_owned(),
|
||||
task: renderer
|
||||
.render("${{ tools.by_kind.task }}")
|
||||
.unwrap_or_default(),
|
||||
.tool_for_kind(crate::types::tool::ToolKind::Task)
|
||||
.unwrap_or_default()
|
||||
.to_owned(),
|
||||
}
|
||||
} else {
|
||||
EnterPlanModeToolHints::default()
|
||||
|
|
|
|||
|
|
@ -97,7 +97,7 @@ impl xai_tool_runtime::Tool for ExitPlanModeTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"exit_plan_mode",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -266,7 +266,7 @@ impl xai_tool_runtime::Tool for GrepTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"grep",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -287,7 +287,7 @@ impl xai_tool_runtime::Tool for ImageEditTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"image_edit",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -419,7 +419,7 @@ impl xai_tool_runtime::Tool for ImageGenTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"image_gen",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -168,7 +168,7 @@ impl xai_tool_runtime::Tool for KillTaskTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"kill_task",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -50,7 +50,7 @@ impl xai_tool_runtime::Tool for KillTerminalCommandTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"kill_terminal_command",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -463,7 +463,7 @@ impl xai_tool_runtime::Tool for ListDirTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"list_dir",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
fn capabilities(&self) -> xai_tool_protocol::ToolCapabilities {
|
||||
|
|
|
|||
|
|
@ -63,7 +63,7 @@ impl xai_tool_runtime::Tool for LspTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"lsp",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -27,7 +27,7 @@ impl crate::types::tool_metadata::ToolMetadata for MonitorTool {
|
|||
}
|
||||
|
||||
fn description_template(&self) -> &str {
|
||||
r#"Start a background monitor that streams events from a long-running script. Each stdout line is an event - you can keep working and notifications arrive in the chat. Exit ends the watch.
|
||||
r#"Start a background monitor that streams events from a long-running script. Each stdout line is an event${%- if system_reminders_enabled %} - you can keep working and notifications arrive in the chat${%- endif %}. Exit ends the watch.
|
||||
|
||||
**Output volume**: Every stdout line becomes a message in the conversation, so write selective filters. In pipes use `grep --line-buffered` (plain `grep` buffers and delays events by minutes).
|
||||
|
||||
|
|
@ -57,7 +57,7 @@ impl xai_tool_runtime::Tool for MonitorTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"monitor",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -105,7 +105,7 @@ pub(crate) const DESCRIPTION_FULL: &str = r#"Read a file.
|
|||
Usage:
|
||||
- The ${{ params.read.target_file }} parameter can be a relative path in the workspace or an absolute path
|
||||
- By default, it reads up to {max_lines_read} lines starting from the beginning of the file
|
||||
- Results are returned with line numbers starting at 1. The format is: LINE_NUMBER→LINE_CONTENT
|
||||
- Line numbers (1-based) appear as anchors in the format LINE_NUMBER→LINE_CONTENT on the first returned line and on every 10th line of the file; the lines in between show content only. Count from the nearest anchor when referring to a specific line
|
||||
- This tool can read PDF files (.pdf), PowerPoint files (.pptx), Jupyter notebooks (.ipynb files), and image files (e.g. PNG, JPG, etc).
|
||||
- When reading an image file the contents are presented visually as this tool uses multimodal LLMs."#;
|
||||
/// Schema-only advertised default (runtime still treats omit as line 1 via unwrap_or).
|
||||
|
|
@ -608,7 +608,7 @@ impl xai_tool_runtime::Tool for ReadFileTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"read_file",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
fn capabilities(&self) -> xai_tool_protocol::ToolCapabilities {
|
||||
|
|
|
|||
|
|
@ -143,7 +143,7 @@ impl xai_tool_runtime::Tool for SchedulerCreateTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"scheduler_create",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -70,7 +70,7 @@ impl xai_tool_runtime::Tool for SchedulerDeleteTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"scheduler_delete",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -67,7 +67,7 @@ impl xai_tool_runtime::Tool for SchedulerListTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"scheduler_list",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -50,7 +50,7 @@ impl SearchReplaceVersion {
|
|||
self == Self::Legacy0_4_10
|
||||
}
|
||||
}
|
||||
/// Full description with read-before-edit guidance (for non-concise toolset).
|
||||
/// Full description (for the non-concise toolset).
|
||||
///
|
||||
/// Uses MiniJinja template placeholders with ToolKind-based keys:
|
||||
/// - `${{ tools.by_kind.read }}` — client-facing name for the Read tool
|
||||
|
|
@ -58,9 +58,17 @@ impl SearchReplaceVersion {
|
|||
/// - `${{ params.edit.replace_all }}` — client-facing param name
|
||||
pub(crate) const DESCRIPTION_FULL: &str = r#"Replace an exact string in a file.
|
||||
|
||||
- Read the file with `${{ tools.by_kind.read }}` before editing it.
|
||||
${% if tools.by_kind.read -%}
|
||||
- `${{ tools.by_kind.read }}` prefixes each line with "LINE_NUMBER→". That prefix is not part of the file: match only what comes after the →, with its exact indentation.
|
||||
- `${{ params.edit.old_string }}` must match exactly one place in the file. If it appears more than once, add surrounding lines to make it unique, or set `${{ params.edit.replace_all }}` to change every occurrence (handy for renaming an identifier)."#;
|
||||
${% endif -%}
|
||||
- `${{ params.edit.old_string }}` must match exactly one place in the file. If it appears more than once, add surrounding lines to make it unique, or set `${{ params.edit.replace_all }}` to change every occurrence (handy for renaming an identifier).
|
||||
- To create a new file, set `${{ params.edit.old_string }}` to an empty string. An empty `${{ params.edit.old_string }}` cannot overwrite an existing non-empty file."#;
|
||||
/// The overwrite-guard sentence in [`DESCRIPTION_FULL`]. Only accurate while
|
||||
/// `empty_old_string_does_not_override` is enabled (opt-in; the default is the
|
||||
/// legacy overwrite behavior); `versioned_definition` strips it unless a
|
||||
/// config enables the guard.
|
||||
pub(crate) const EMPTY_OLD_STRING_GUARD_SENTENCE: &str =
|
||||
" An empty `${{ params.edit.old_string }}` cannot overwrite an existing non-empty file.";
|
||||
/// Input for the search_replace tool.
|
||||
#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, schemars::JsonSchema)]
|
||||
pub struct SearchReplaceInput {
|
||||
|
|
@ -89,15 +97,19 @@ fn default_true() -> bool {
|
|||
/// Configuration for the search_replace tool, stored as `Params<SearchReplaceParams>` in Resources.
|
||||
///
|
||||
/// Replaces the old `SearchReplaceOptions` that was stored via `tool_options_as()`.
|
||||
#[derive(Debug, Clone, Default, serde::Serialize, serde::Deserialize)]
|
||||
#[derive(Debug, Clone, serde::Serialize, serde::Deserialize)]
|
||||
#[serde(deny_unknown_fields)]
|
||||
pub struct SearchReplaceParams {
|
||||
/// Deprecated runtime no-op, kept so configs still sending it deserialize under
|
||||
/// `deny_unknown_fields`. Still gates the config-time Read-tool requirement (`requires_expr`).
|
||||
#[serde(default)]
|
||||
pub skip_read_before_edit: bool,
|
||||
/// Empty old string DOES not override the file unless its empty, by default we allow
|
||||
/// empty old string to override the file content completely``
|
||||
/// When true (opt-in), an empty `old_string` may only create a new file
|
||||
/// or fill an empty one — it never silently overwrites an existing
|
||||
/// non-empty file. Defaults to false (the legacy behavior): an empty
|
||||
/// `old_string` replaces the file's entire contents. The served
|
||||
/// description includes the guard sentence only when this is enabled
|
||||
/// (see `versioned_definition`).
|
||||
#[serde(default)]
|
||||
pub empty_old_string_does_not_override: bool,
|
||||
/// When true, enable normalized-fallback matching for Unicode confusable
|
||||
|
|
@ -116,6 +128,16 @@ pub struct SearchReplaceParams {
|
|||
#[serde(default = "default_true")]
|
||||
pub include_user_edit_hint: bool,
|
||||
}
|
||||
impl Default for SearchReplaceParams {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
skip_read_before_edit: false,
|
||||
empty_old_string_does_not_override: false,
|
||||
unicode_normalized_fallback: false,
|
||||
include_user_edit_hint: true,
|
||||
}
|
||||
}
|
||||
}
|
||||
register_resource!("grok_build", "SearchReplace", SearchReplaceParams);
|
||||
/// SearchReplace tool — new architecture.
|
||||
///
|
||||
|
|
@ -337,7 +359,7 @@ async fn handle_new_file_creation(
|
|||
}
|
||||
if let Some(old_text) = old_text
|
||||
&& file_exists
|
||||
&& empty_old_string_does_not_override
|
||||
&& !empty_old_string_does_not_override
|
||||
{
|
||||
notification_handle.send_file_written(FileWritten {
|
||||
tool_call_id: tool_call_id.to_string(),
|
||||
|
|
@ -762,6 +784,45 @@ impl crate::types::tool_metadata::ToolMetadata for SearchReplaceTool {
|
|||
fn description_template(&self) -> &str {
|
||||
DESCRIPTION_FULL
|
||||
}
|
||||
/// Params-aware description: the "cannot overwrite" sentence in
|
||||
/// [`DESCRIPTION_FULL`] only holds while `empty_old_string_does_not_override`
|
||||
/// is enabled, so it is served only for configs that opt into the guard and
|
||||
/// stripped by default (legacy overwrite behavior).
|
||||
fn versioned_definition(
|
||||
&self,
|
||||
_contract_version: Option<&str>,
|
||||
client_name: &str,
|
||||
description_override: Option<&str>,
|
||||
renderer: &TemplateRenderer,
|
||||
param_map: &std::collections::HashMap<String, String>,
|
||||
input_schema: &serde_json::Value,
|
||||
effective_params: &serde_json::Value,
|
||||
) -> crate::types::definition::ToolDefinition {
|
||||
let params: SearchReplaceParams =
|
||||
serde_json::from_value(effective_params.clone()).unwrap_or_default();
|
||||
let raw_desc = match description_override {
|
||||
Some(d) => d.to_string(),
|
||||
None if params.empty_old_string_does_not_override => {
|
||||
self.description_template().to_string()
|
||||
}
|
||||
None => self
|
||||
.description_template()
|
||||
.replace(EMPTY_OLD_STRING_GUARD_SENTENCE, ""),
|
||||
};
|
||||
let description = renderer.render(&raw_desc).unwrap_or_else(|e| {
|
||||
crate::types::template_renderer::strip_markers_on_render_failure(&raw_desc, &e)
|
||||
});
|
||||
let remapped_schema = if param_map.is_empty() {
|
||||
input_schema.clone()
|
||||
} else {
|
||||
crate::util::remap::remap_schema_properties(input_schema, param_map)
|
||||
};
|
||||
crate::types::definition::ToolDefinition::function(
|
||||
client_name,
|
||||
Some(&description),
|
||||
remapped_schema,
|
||||
)
|
||||
}
|
||||
fn emitted_notifications(&self) -> &'static [&'static str] {
|
||||
&["FileWritten"]
|
||||
}
|
||||
|
|
@ -800,7 +861,7 @@ impl xai_tool_runtime::Tool for SearchReplaceTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"search_replace",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
fn capabilities(&self) -> xai_tool_protocol::ToolCapabilities {
|
||||
|
|
@ -869,6 +930,94 @@ mod tests {
|
|||
replace_all: false,
|
||||
}
|
||||
}
|
||||
fn description_renderer() -> TemplateRenderer {
|
||||
let edit_params = std::collections::HashMap::from([
|
||||
("old_string".to_string(), "old_string".to_string()),
|
||||
("new_string".to_string(), "new_string".to_string()),
|
||||
("replace_all".to_string(), "replace_all".to_string()),
|
||||
]);
|
||||
TemplateRenderer::new(
|
||||
std::collections::HashMap::from([(ToolKind::Read, "read_file".to_string())]),
|
||||
std::collections::HashMap::from([(ToolKind::Edit, edit_params)]),
|
||||
)
|
||||
}
|
||||
/// The strip in `versioned_definition` matches the template verbatim, so
|
||||
/// the sentence must stay in sync with `DESCRIPTION_FULL`.
|
||||
#[test]
|
||||
fn overwrite_guard_sentence_stays_in_sync_with_template() {
|
||||
assert!(DESCRIPTION_FULL.contains(EMPTY_OLD_STRING_GUARD_SENTENCE));
|
||||
}
|
||||
#[test]
|
||||
fn overwrite_guard_sentence_is_conditional_on_param() {
|
||||
use crate::types::tool_metadata::ToolMetadata;
|
||||
let renderer = description_renderer();
|
||||
let schema = serde_json::json!({"type": "object", "properties": {}});
|
||||
let param_map = std::collections::HashMap::new();
|
||||
let default_def = ToolMetadata::versioned_definition(
|
||||
&SearchReplaceTool,
|
||||
None,
|
||||
"search_replace",
|
||||
None,
|
||||
&renderer,
|
||||
¶m_map,
|
||||
&schema,
|
||||
&serde_json::json!({}),
|
||||
);
|
||||
let default_desc = default_def.function.description.unwrap();
|
||||
assert!(
|
||||
!default_desc.contains("cannot overwrite"),
|
||||
"guard sentence must be absent by default (legacy overwrite behavior):\n{default_desc}"
|
||||
);
|
||||
assert!(
|
||||
default_desc.contains("To create a new file"),
|
||||
"create-file guidance must remain:\n{default_desc}"
|
||||
);
|
||||
let opt_in_def = ToolMetadata::versioned_definition(
|
||||
&SearchReplaceTool,
|
||||
None,
|
||||
"search_replace",
|
||||
None,
|
||||
&renderer,
|
||||
¶m_map,
|
||||
&schema,
|
||||
&serde_json::json!({"empty_old_string_does_not_override": true}),
|
||||
);
|
||||
let opt_in_desc = opt_in_def.function.description.unwrap();
|
||||
assert!(
|
||||
opt_in_desc.contains("cannot overwrite an existing non-empty file"),
|
||||
"guard sentence must appear when the guard is enabled:\n{opt_in_desc}"
|
||||
);
|
||||
}
|
||||
#[test]
|
||||
fn description_read_bullet_guarded_on_read_tool() {
|
||||
use crate::types::tool_metadata::ToolMetadata;
|
||||
let rendered = description_renderer()
|
||||
.render(ToolMetadata::description_template(&SearchReplaceTool))
|
||||
.unwrap();
|
||||
assert!(
|
||||
rendered.contains("read_file` prefixes each line") && !rendered.contains("${%"),
|
||||
"read bullet must render with the resolved name:\n{rendered}"
|
||||
);
|
||||
assert!(
|
||||
!rendered.contains("before editing it"),
|
||||
"read-before-edit guidance must be gone:\n{rendered}"
|
||||
);
|
||||
let edit_params = std::collections::HashMap::from([
|
||||
("old_string".to_string(), "old_string".to_string()),
|
||||
("new_string".to_string(), "new_string".to_string()),
|
||||
("replace_all".to_string(), "replace_all".to_string()),
|
||||
]);
|
||||
let no_read = TemplateRenderer::new(
|
||||
std::collections::HashMap::new(),
|
||||
std::collections::HashMap::from([(ToolKind::Edit, edit_params)]),
|
||||
)
|
||||
.render(ToolMetadata::description_template(&SearchReplaceTool))
|
||||
.unwrap();
|
||||
assert!(
|
||||
!no_read.contains("prefixes each line") && !no_read.contains("- \n"),
|
||||
"read bullet must vanish cleanly without a Read tool:\n{no_read}"
|
||||
);
|
||||
}
|
||||
#[tokio::test]
|
||||
async fn basic_replacement() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
|
|
@ -1270,7 +1419,7 @@ mod tests {
|
|||
}
|
||||
}
|
||||
#[tokio::test]
|
||||
async fn empty_old_string_overrides_existing_file_by_default() {
|
||||
async fn empty_old_string_overwrites_existing_file_by_default() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
std::fs::write(tmp.path().join("existing.txt"), "existing content\n").unwrap();
|
||||
let tool = SearchReplaceTool;
|
||||
|
|
@ -1289,6 +1438,29 @@ mod tests {
|
|||
}
|
||||
}
|
||||
#[tokio::test]
|
||||
async fn empty_old_string_overrides_with_explicit_false() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
std::fs::write(tmp.path().join("existing.txt"), "existing content\n").unwrap();
|
||||
let tool = SearchReplaceTool;
|
||||
let mut resources = test_resources(tmp.path());
|
||||
resources.insert(Params(SearchReplaceParams {
|
||||
empty_old_string_does_not_override: false,
|
||||
..Default::default()
|
||||
}));
|
||||
let input = make_input("existing.txt", "", "completely new content\n");
|
||||
let result = xai_tool_runtime::Tool::run(&tool, test_ctx(resources.into_shared()), input)
|
||||
.await
|
||||
.unwrap();
|
||||
match result {
|
||||
SearchReplaceOutput::EditsApplied(applied) => {
|
||||
assert!(applied.tool_output_for_prompt.contains("has been created"));
|
||||
let content = std::fs::read_to_string(tmp.path().join("existing.txt")).unwrap();
|
||||
assert_eq!(content, "completely new content\n");
|
||||
}
|
||||
other => panic!("Expected EditsApplied, got {:?}", other),
|
||||
}
|
||||
}
|
||||
#[tokio::test]
|
||||
async fn empty_old_string_blocked_when_param_set() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
std::fs::write(tmp.path().join("existing.txt"), "existing content\n").unwrap();
|
||||
|
|
|
|||
|
|
@ -163,7 +163,7 @@ impl<R: ChildRunner> SubagentCoordinator<R> {
|
|||
match command {
|
||||
SubagentEvent::Spawn(command) => {
|
||||
let mut request = *command.request;
|
||||
if let Some((root_parent, loop_task_id)) = self
|
||||
if let Some((root_parent, loop_task_id, spawner_cancelled)) = self
|
||||
.active
|
||||
.values()
|
||||
.find(|child| child.child_session_id == request.parent_session_id)
|
||||
|
|
@ -171,9 +171,24 @@ impl<R: ChildRunner> SubagentCoordinator<R> {
|
|||
(
|
||||
child.request.parent_session_id.clone(),
|
||||
child.request.runtime_overrides.loop_task_id.clone(),
|
||||
child.cancellation.is_cancelled(),
|
||||
)
|
||||
})
|
||||
{
|
||||
if spawner_cancelled {
|
||||
// The parent subagent is being torn down, so its late
|
||||
// child would be orphaned against the closed scope.
|
||||
let id = request.id.clone();
|
||||
let _ = command.result_tx.send(SubagentResult {
|
||||
success: false,
|
||||
cancelled: true,
|
||||
error: Some("parent subagent is being torn down".to_owned()),
|
||||
subagent_id: id.clone(),
|
||||
child_session_id: id,
|
||||
..Default::default()
|
||||
});
|
||||
return;
|
||||
}
|
||||
request.parent_session_id = root_parent;
|
||||
request.surface_completion = false;
|
||||
if request.runtime_overrides.loop_task_id.is_none() {
|
||||
|
|
@ -291,9 +306,10 @@ impl<R: ChildRunner> SubagentCoordinator<R> {
|
|||
.collect();
|
||||
let _ = request.respond_to.send(completions);
|
||||
}
|
||||
SubagentEvent::DiscardSessionCompletions { parent_session_id } => {
|
||||
SubagentEvent::TeardownSession { parent_session_id } => {
|
||||
self.pending_completions
|
||||
.retain(|completion| completion.parent_session_id != parent_session_id);
|
||||
self.teardown_session_children(&parent_session_id);
|
||||
}
|
||||
SubagentEvent::Outstanding(request) => {
|
||||
// Reap again here so turn-freeze / Outstanding polls see
|
||||
|
|
@ -606,7 +622,7 @@ impl<R: ChildRunner> SubagentCoordinator<R> {
|
|||
summary,
|
||||
});
|
||||
// Bound the buffer (drop oldest): sessions unloaded without a
|
||||
// DiscardSessionCompletions cannot grow it unboundedly.
|
||||
// TeardownSession cannot grow it unboundedly.
|
||||
const MAX_PENDING_COMPLETIONS: usize = 256;
|
||||
if self.pending_completions.len() > MAX_PENDING_COMPLETIONS {
|
||||
let excess = self.pending_completions.len() - MAX_PENDING_COMPLETIONS;
|
||||
|
|
@ -719,6 +735,34 @@ impl<R: ChildRunner> SubagentCoordinator<R> {
|
|||
}
|
||||
}
|
||||
|
||||
fn teardown_session_children(&mut self, parent_session_id: &str) {
|
||||
let mut cancelled = 0;
|
||||
for child in self.active.values_mut() {
|
||||
if child.request.parent_session_id == parent_session_id {
|
||||
// Parent is gone: do not rebuffer this completion for a later
|
||||
// resume of the same session id.
|
||||
child.request.surface_completion = false;
|
||||
child.cancellation.cancel();
|
||||
child.control.cancel();
|
||||
cancelled += 1;
|
||||
}
|
||||
}
|
||||
for child in self.pending.values_mut() {
|
||||
if child.request.parent_session_id == parent_session_id {
|
||||
child.request.surface_completion = false;
|
||||
child.cancellation.cancel();
|
||||
cancelled += 1;
|
||||
}
|
||||
}
|
||||
if cancelled > 0 {
|
||||
tracing::info!(
|
||||
parent_session_id,
|
||||
cancelled,
|
||||
"cancelled subagents on session teardown"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
fn cancel_workflow_children(&mut self, run_id: &str, parent_session_id: Option<&str>) {
|
||||
for child in self.active.values() {
|
||||
if child.request.owner.workflow_run_id() == Some(run_id)
|
||||
|
|
|
|||
|
|
@ -760,6 +760,160 @@ async fn workflow_cancel_waits_for_drain_and_hides_owned_children() {
|
|||
harness.actor.abort();
|
||||
}
|
||||
|
||||
/// Spawn an await-to-completion child under `session` and consume its request
|
||||
/// event, returning the join handle for the in-flight spawn.
|
||||
async fn spawn_session_child(
|
||||
harness: &mut Harness,
|
||||
id: &str,
|
||||
session: &str,
|
||||
) -> tokio::task::JoinHandle<Result<SubagentResult, xai_tool_runtime::ToolError>> {
|
||||
let mut req = request(id, false);
|
||||
req.await_to_completion = true;
|
||||
req.parent_session_id = session.to_owned();
|
||||
let backend = harness.backend.clone();
|
||||
let handle = tokio::spawn(async move { backend.spawn(req).await });
|
||||
assert_eq!(
|
||||
harness
|
||||
.requests
|
||||
.recv()
|
||||
.await
|
||||
.as_ref()
|
||||
.map(|r| r.id.as_str()),
|
||||
Some(id)
|
||||
);
|
||||
handle
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn teardown_session_children_spares_other_sessions() {
|
||||
let mut harness = harness(true, std::time::Duration::from_secs(60));
|
||||
|
||||
// Two children under "parent" (one active, one pending) plus one under a
|
||||
// different session that must survive.
|
||||
let keep = spawn_session_child(&mut harness, "keep-active", "other").await;
|
||||
let kill_active = spawn_session_child(&mut harness, "kill-active", "parent").await;
|
||||
|
||||
// Start the children spawned so far; kill-pending subscribes after start, so
|
||||
// it never receives it and stays pending.
|
||||
let _ = harness.start.send(());
|
||||
let mut started = std::collections::HashSet::new();
|
||||
started.insert(harness.started.recv().await.unwrap());
|
||||
started.insert(harness.started.recv().await.unwrap());
|
||||
assert!(started.contains("keep-active") && started.contains("kill-active"));
|
||||
|
||||
let kill_pending = spawn_session_child(&mut harness, "kill-pending", "parent").await;
|
||||
|
||||
harness
|
||||
.backend
|
||||
.sender()
|
||||
.send(SubagentEvent::TeardownSession {
|
||||
parent_session_id: "parent".to_owned(),
|
||||
})
|
||||
.expect("actor command channel open");
|
||||
|
||||
assert!(kill_active.await.unwrap().unwrap().cancelled);
|
||||
assert!(kill_pending.await.unwrap().unwrap().cancelled);
|
||||
|
||||
assert!(
|
||||
!keep.is_finished(),
|
||||
"a different session's child must not be cancelled"
|
||||
);
|
||||
let _ = harness.finish.send(());
|
||||
let keep = keep.await.unwrap().unwrap();
|
||||
assert!(keep.success && !keep.cancelled);
|
||||
harness.actor.abort();
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn teardown_cancels_background_child_without_rebuffering() {
|
||||
let mut harness = harness_with_config(
|
||||
true,
|
||||
CoordinatorConfig {
|
||||
buffer_completions: true,
|
||||
..CoordinatorConfig::default()
|
||||
},
|
||||
);
|
||||
|
||||
// A background subagent that outlives its parent is the production case that
|
||||
// rebuffers a completion for a later resume of the same session id.
|
||||
let mut req = request("bg", true);
|
||||
req.parent_session_id = "parent".to_owned();
|
||||
let spawn = tokio::spawn({
|
||||
let backend = harness.backend.clone();
|
||||
async move { backend.spawn(req).await }
|
||||
});
|
||||
assert_eq!(
|
||||
harness
|
||||
.requests
|
||||
.recv()
|
||||
.await
|
||||
.as_ref()
|
||||
.map(|r| r.id.as_str()),
|
||||
Some("bg")
|
||||
);
|
||||
let _ = harness.start.send(());
|
||||
assert_eq!(harness.started.recv().await.as_deref(), Some("bg"));
|
||||
|
||||
harness
|
||||
.backend
|
||||
.sender()
|
||||
.send(SubagentEvent::TeardownSession {
|
||||
parent_session_id: "parent".to_owned(),
|
||||
})
|
||||
.expect("actor command channel open");
|
||||
|
||||
// Wait for the cancelled child to finish, then assert it buffered nothing.
|
||||
let _ = harness.completions.recv().await;
|
||||
let (tx, rx) = oneshot::channel();
|
||||
harness
|
||||
.backend
|
||||
.sender()
|
||||
.send(SubagentEvent::Completions(SubagentCompletionsRequest {
|
||||
parent_session_id: Some("parent".to_owned()),
|
||||
suppress_ids: Vec::new(),
|
||||
respond_to: tx,
|
||||
}))
|
||||
.expect("actor command channel open");
|
||||
assert!(
|
||||
rx.await.unwrap().is_empty(),
|
||||
"torn-down background child must not rebuffer a completion"
|
||||
);
|
||||
let _ = spawn.await;
|
||||
harness.actor.abort();
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn teardown_rejects_spawn_from_cancelled_parent() {
|
||||
// wait_after_cancel keeps the cancelled parent in `active`, so its late
|
||||
// nested Spawn still finds it.
|
||||
let mut harness = harness_with_options(true, true, CoordinatorConfig::default());
|
||||
|
||||
// A parent subagent whose child_session_id is "A".
|
||||
let parent = spawn_session_child(&mut harness, "A", "parent").await;
|
||||
let _ = harness.start.send(());
|
||||
assert_eq!(harness.started.recv().await.as_deref(), Some("A"));
|
||||
|
||||
harness
|
||||
.backend
|
||||
.sender()
|
||||
.send(SubagentEvent::TeardownSession {
|
||||
parent_session_id: "parent".to_owned(),
|
||||
})
|
||||
.expect("actor command channel open");
|
||||
|
||||
// A nested Spawn from the now-cancelled parent (parent_session_id = its
|
||||
// child_session_id) must be rejected, not reparented and left running.
|
||||
let mut nested = request("B", false);
|
||||
nested.await_to_completion = true;
|
||||
nested.parent_session_id = "A".to_owned();
|
||||
let outcome = harness.backend.spawn(nested).await.unwrap();
|
||||
assert!(outcome.cancelled && !outcome.success);
|
||||
|
||||
let _ = harness.finish.send(());
|
||||
let _ = parent.await;
|
||||
harness.actor.abort();
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn usage_events_feed_sorted_outstanding_reply() {
|
||||
let mut harness = harness(true, std::time::Duration::from_secs(60));
|
||||
|
|
@ -1032,7 +1186,7 @@ async fn buffered_completion_output_cap_bounds_buffered_summary() {
|
|||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn discard_session_completions_drops_only_that_sessions_buffer() {
|
||||
async fn teardown_session_drops_only_that_sessions_buffer() {
|
||||
let mut harness = harness_with_config(
|
||||
false,
|
||||
CoordinatorConfig {
|
||||
|
|
@ -1053,11 +1207,11 @@ async fn discard_session_completions_drops_only_that_sessions_buffer() {
|
|||
let _ = harness.completions.recv().await;
|
||||
}
|
||||
|
||||
// Removing parent-a (session unload) discards its buffered completion...
|
||||
// Tearing down parent-a discards its buffered completion...
|
||||
harness
|
||||
.backend
|
||||
.sender()
|
||||
.send(SubagentEvent::DiscardSessionCompletions {
|
||||
.send(SubagentEvent::TeardownSession {
|
||||
parent_session_id: "parent-a".to_owned(),
|
||||
})
|
||||
.expect("actor command channel open");
|
||||
|
|
|
|||
|
|
@ -29,6 +29,7 @@ use crate::types::requirements::{Expr, ToolRequirement};
|
|||
#[allow(unused_imports)]
|
||||
use crate::types::resources::SharedResources;
|
||||
use crate::types::tool::{ToolKind, ToolNamespace};
|
||||
use regex::Regex;
|
||||
use xai_tool_types::{SubagentCompletedOutput, SubagentIsolationMode, TaskToolInput};
|
||||
|
||||
/// Default max nesting depth when [`MaxSubagentDepth`] is not injected.
|
||||
|
|
@ -62,14 +63,59 @@ impl crate::types::tool_metadata::ToolMetadata for TaskTool {
|
|||
}
|
||||
|
||||
fn description_template(&self) -> &str {
|
||||
// The Task tool description for Grok Build is *never* taken from here.
|
||||
// It is always supplied via `ToolConfig::with_description(...)` using
|
||||
// the dynamically built string from `build_task_description()` in
|
||||
// xai-grok-agent/src/builder.rs (HEADER + per-subagent blocks + FOOTER).
|
||||
//
|
||||
// This path is only hit by low-level ToolsetBuilder registration or
|
||||
// direct calls to ToolMetadata::description_template in tests.
|
||||
"<see build_task_description() in xai-grok-agent>"
|
||||
// Grok Build normally supplies the description via
|
||||
// `ToolConfig::with_description(...)` using `build_task_description()`
|
||||
// in xai-grok-agent/src/builder.rs (live subagent roster). But a
|
||||
// registration without an override must still ship a real
|
||||
// description, never a placeholder: default to the built-in roster
|
||||
// with templated tool/param names, resolved by the registry renderer
|
||||
// at finalize time.
|
||||
/// Wrap each `${{ tools.by_kind.X }}` token in an if/else so kinds
|
||||
/// absent from the registry render as the bare kind name instead of
|
||||
/// an empty slot ("read, , and plan"), mirroring the bare-kind
|
||||
/// fallback of `BuiltinSubagent::render_tools`.
|
||||
///
|
||||
/// These guards sit inline in the roster, so they use the
|
||||
/// non-stripping `${% %}` form: `${%-` would eat the ", " before
|
||||
/// each token and render "has access to:read,grep".
|
||||
fn guard_kind_tokens(template: &str) -> String {
|
||||
static TOKEN: std::sync::LazyLock<Regex> = std::sync::LazyLock::new(|| {
|
||||
Regex::new(r"\$\{\{\s*tools\.by_kind\.([a-z_]+)\s*\}\}").expect("valid regex")
|
||||
});
|
||||
TOKEN
|
||||
.replace_all(template, |caps: ®ex::Captures| {
|
||||
let kind = &caps[1];
|
||||
format!(
|
||||
"${{% if tools.by_kind.{kind} %}}${{{{ tools.by_kind.{kind} }}}}\
|
||||
${{% else %}}{kind}${{% endif %}}"
|
||||
)
|
||||
})
|
||||
.into_owned()
|
||||
}
|
||||
|
||||
static DESC: std::sync::LazyLock<String> = std::sync::LazyLock::new(|| {
|
||||
let subagents: Vec<xai_tool_types::SubagentDescriptor> =
|
||||
xai_tool_types::BUILTIN_SUBAGENTS
|
||||
.iter()
|
||||
.map(|b| xai_tool_types::SubagentDescriptor {
|
||||
name: b.name.to_owned(),
|
||||
description: b.description.to_owned(),
|
||||
tools: Some(guard_kind_tokens(b.tools_template)),
|
||||
})
|
||||
.collect();
|
||||
xai_tool_types::build_task_description(
|
||||
&subagents,
|
||||
&xai_tool_types::TaskToolNaming {
|
||||
task_tool: "${{ tools.by_kind.task }}",
|
||||
subagent_type_param: "${{ params.task.subagent_type }}",
|
||||
run_in_background_param: "${{ params.task.run_in_background }}",
|
||||
resume_from_param: "${{ params.task.resume_from }}",
|
||||
background_retrieval_tool: "${{ tools.by_kind.background_task_action }}",
|
||||
isolation_param: "${{ params.task.isolation }}",
|
||||
},
|
||||
)
|
||||
});
|
||||
&DESC
|
||||
}
|
||||
|
||||
fn requires_expr(&self) -> Expr<ToolRequirement> {
|
||||
|
|
@ -101,7 +147,7 @@ impl xai_tool_runtime::Tool for TaskTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"task",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
@ -392,12 +438,15 @@ impl xai_tool_runtime::Tool for TaskTool {
|
|||
}
|
||||
});
|
||||
|
||||
let task_output_name = crate::types::template_renderer::TemplateRenderer::resolve(
|
||||
&resources,
|
||||
"${{ tools.by_kind.background_task_action }}",
|
||||
)
|
||||
.await
|
||||
.unwrap_or_else(|_| "get_command_or_subagent_output".to_string());
|
||||
// `resolve_tool_name` (not a template render): a missing kind
|
||||
// renders as empty-`Ok`, so a `Result` fallback never fires.
|
||||
let task_output_name =
|
||||
crate::types::template_renderer::TemplateRenderer::resolve_tool_name(
|
||||
&resources,
|
||||
crate::types::tool::ToolKind::BackgroundTaskAction,
|
||||
)
|
||||
.await
|
||||
.unwrap_or_else(|| "get_task_output".to_string());
|
||||
|
||||
return Ok(ToolOutput::Text(
|
||||
xai_tool_types::format_subagent_started_background(
|
||||
|
|
@ -422,18 +471,32 @@ impl xai_tool_runtime::Tool for TaskTool {
|
|||
// still-running child — return a task_id to poll, like the background
|
||||
// branch above (the result arrives via auto-wake or a later poll).
|
||||
if result.backgrounded {
|
||||
let task_output_name = crate::types::template_renderer::TemplateRenderer::resolve(
|
||||
&resources,
|
||||
"${{ tools.by_kind.background_task_action }}",
|
||||
)
|
||||
.await
|
||||
.unwrap_or_else(|_| "get_command_or_subagent_output".to_string());
|
||||
// `resolve_tool_name` (not a template render): a missing kind
|
||||
// renders as empty-`Ok`, so a `Result` fallback never fires.
|
||||
let task_output_name =
|
||||
crate::types::template_renderer::TemplateRenderer::resolve_tool_name(
|
||||
&resources,
|
||||
crate::types::tool::ToolKind::BackgroundTaskAction,
|
||||
)
|
||||
.await
|
||||
.unwrap_or_else(|| "get_task_output".to_string());
|
||||
// Only promise a completion notification when the client
|
||||
// actually delivers system reminders.
|
||||
let notify_clause = if resources
|
||||
.lock()
|
||||
.await
|
||||
.get::<crate::types::resources::SystemRemindersEnabled>()
|
||||
.is_none_or(|e| e.0)
|
||||
{
|
||||
" — you will be notified when it completes"
|
||||
} else {
|
||||
""
|
||||
};
|
||||
|
||||
return Ok(ToolOutput::Text(
|
||||
format!(
|
||||
"Subagent took longer than the foreground budget and was moved to the \
|
||||
background to keep the conversation responsive. It is still running — you \
|
||||
will be notified when it completes.\n\
|
||||
background to keep the conversation responsive. It is still running{notify_clause}.\n\
|
||||
subagent_id: {id}\n\
|
||||
type: {}\n\
|
||||
description: {}\n\n\
|
||||
|
|
|
|||
|
|
@ -848,9 +848,8 @@ pub enum SubagentEvent {
|
|||
ListActive(SubagentListActiveRequest),
|
||||
ListRunning(SubagentListRunningRequest),
|
||||
Completions(SubagentCompletionsRequest),
|
||||
/// Fire-and-forget: drop buffered completions owned by a removed session
|
||||
/// so unloaded sessions cannot leak entries into the shared buffer.
|
||||
DiscardSessionCompletions {
|
||||
/// Discard a closed session's buffered completions and cancel its children.
|
||||
TeardownSession {
|
||||
parent_session_id: String,
|
||||
},
|
||||
Outstanding(SubagentOutstandingRequest),
|
||||
|
|
@ -866,12 +865,7 @@ pub enum SubagentEvent {
|
|||
|
||||
// Resource types
|
||||
|
||||
/// Unified sender for all subagent coordinator events.
|
||||
///
|
||||
/// Cloned into each session's `ToolContext` / `ToolBridge Resources` so
|
||||
/// that `TaskTool`, `TaskOutputTool`, `KillTaskTool`, completion
|
||||
/// reminders, compaction queries, and turn-end guards all send through
|
||||
/// a single channel.
|
||||
/// One shared channel to the subagent coordinator, cloned into each session.
|
||||
#[derive(Clone, Educe)]
|
||||
#[educe(Debug)]
|
||||
pub struct SubagentEventSender(#[educe(Debug(ignore))] pub mpsc::UnboundedSender<SubagentEvent>);
|
||||
|
|
|
|||
|
|
@ -265,7 +265,7 @@ impl TaskOutputTool {
|
|||
|
||||
let completed_count = results
|
||||
.iter()
|
||||
.filter(|r| r.status == "completed" || r.status == "failed" || r.status == "cancelled")
|
||||
.filter(|r| is_terminal_status(&r.status))
|
||||
.count();
|
||||
let total = results.len();
|
||||
let mode_str = if waits { "wait_all" } else { "poll" };
|
||||
|
|
@ -281,6 +281,12 @@ impl TaskOutputTool {
|
|||
|
||||
pub(crate) use xai_tool_types::MAX_MULTI_WAIT_IDS;
|
||||
|
||||
/// Terminal task statuses as produced by `snapshot_to_result` /
|
||||
/// `format_subagent_snapshot`; multi-wait summaries count these as finished.
|
||||
pub(crate) fn is_terminal_status(status: &str) -> bool {
|
||||
matches!(status, "completed" | "failed" | "cancelled" | "timed_out")
|
||||
}
|
||||
|
||||
pub(crate) fn not_found_result(task_id: &str) -> TaskOutputResult {
|
||||
TaskOutputResult {
|
||||
task_id: task_id.to_string(),
|
||||
|
|
@ -769,7 +775,7 @@ impl xai_tool_runtime::Tool for TaskOutputTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"get_task_output",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -25,7 +25,7 @@ impl crate::types::tool_metadata::ToolMetadata for GetTerminalCommandOutputTool
|
|||
r#"Get output and status from a background terminal command${%- if tools.by_kind.monitor %} or monitor${%- endif %}.
|
||||
|
||||
Usage notes:
|
||||
- Pass ${{ params.background_task_action.task_ids }} with one or more ids from ${{ params.execute.is_background }}=true commands${%- if tools.by_kind.monitor %} (a monitor's ${{ params.kill_task_action.task_id }} is returned by ${{ tools.by_kind.monitor }})${%- endif %}; for a single task use a one-element array. Multiple ids with a positive ${{ params.background_task_action.timeout_ms }} wait until all complete
|
||||
- Pass ${{ params.background_task_action.task_ids }} with one or more ids from ${%- if params is defined and params.execute is defined and params.execute.is_background %} ${{ params.execute.is_background }}=true commands${%- else %} background commands${%- endif %}${%- if tools.by_kind.monitor %} (a monitor's ${{ params.kill_task_action.task_id }} is returned by ${{ tools.by_kind.monitor }})${%- endif %}; for a single task use a one-element array. Multiple ids with a positive ${{ params.background_task_action.timeout_ms }} wait until all complete
|
||||
- Omit ${{ params.background_task_action.timeout_ms }} or pass 0 for a non-blocking status snapshot; set a positive ${{ params.background_task_action.timeout_ms }} to wait up to that many milliseconds, capped at ~10 min
|
||||
- Returns current output, status, and exit code if completed${%- if tools.by_kind.read %}
|
||||
- If output is large, use ${{ tools.by_kind.read }} on the output_file path${%- endif %}"#
|
||||
|
|
@ -54,7 +54,7 @@ impl xai_tool_runtime::Tool for GetTerminalCommandOutputTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"get_terminal_command_output",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -33,10 +33,10 @@ impl crate::types::tool_metadata::ToolMetadata for WaitTasksTool {
|
|||
fn description_template(&self) -> &str {
|
||||
// Canonical wording lives in the shared builder; `versioned_definition`
|
||||
// renders it context-aware from the finalized toolset. This static
|
||||
// fallback mirrors the default grok-build toolset.
|
||||
// fallback uses canonical tool/param names.
|
||||
static DESC: std::sync::LazyLock<String> = std::sync::LazyLock::new(|| {
|
||||
xai_tool_types::build_wait_tasks_description(&xai_tool_types::WaitTasksToolNaming {
|
||||
background_retrieval_tool: "get_command_or_subagent_output",
|
||||
background_retrieval_tool: "get_task_output",
|
||||
bash_background_param: Some("is_background"),
|
||||
subagent_background_param: Some("run_in_background"),
|
||||
})
|
||||
|
|
@ -114,7 +114,7 @@ impl xai_tool_runtime::Tool for WaitTasksTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"wait_tasks",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
@ -229,7 +229,7 @@ impl xai_tool_runtime::Tool for WaitTasksTool {
|
|||
|
||||
let completed_count = results
|
||||
.iter()
|
||||
.filter(|r| r.status == "completed" || r.status == "failed" || r.status == "cancelled")
|
||||
.filter(|r| super::is_terminal_status(&r.status))
|
||||
.count();
|
||||
let total = results.len();
|
||||
let summary = format!("{completed_count}/{total} tasks completed (wait_any)");
|
||||
|
|
|
|||
|
|
@ -293,7 +293,7 @@ impl xai_tool_runtime::Tool for TodoWriteTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"todo_write",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -229,7 +229,7 @@ impl xai_tool_runtime::Tool for UpdateGoalTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
UPDATE_GOAL_TOOL_NAME,
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1011,7 +1011,7 @@ impl xai_tool_runtime::Tool for ImageToVideoTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
IMAGE_TO_VIDEO_TOOL_NAME,
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
@ -1107,7 +1107,7 @@ impl xai_tool_runtime::Tool for ReferenceToVideoTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
REFERENCE_TO_VIDEO_TOOL_NAME,
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -146,7 +146,7 @@ impl xai_tool_runtime::Tool for WebFetchTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"web_fetch",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -60,7 +60,7 @@ impl xai_tool_runtime::Tool for WebSearchTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"web_search",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -169,7 +169,7 @@ impl crate::types::tool_metadata::ToolMetadata for WorkflowTool {
|
|||
}
|
||||
|
||||
fn description_template(&self) -> &str {
|
||||
r##"Launch a workflow: a Rhai script that orchestrates subagents as one background run. Provide exactly one source: `name` (a registered workflow — built-in, or from the project `.grok/workflows/` or user `~/.grok/workflows/`), an inline `script`, or a `script_path`. Optionally pass `args` (bound to the script's `args`) and `agent_budget`, an absolute cap on cumulative child-agent calls: every agent() and parallel() item consumes one slot (schema retries do not); default 128. The call returns immediately; progress appears in `/workflows` and completion is reported automatically — do not poll or sleep-wait.
|
||||
r##"Launch a workflow: a Rhai script that orchestrates subagents as one background run. Provide exactly one source: `name` (a registered workflow — built-in, or from the project `.grok/workflows/` or user `~/.grok/workflows/`), an inline `script`, or a `script_path`. Optionally pass `args` (bound to the script's `args`) and `agent_budget`, an absolute cap on cumulative child-agent calls: every agent() and parallel() item consumes one slot (schema retries do not); default 128. The call returns immediately; progress appears in `/workflows`${%- if system_reminders_enabled %} and completion is reported automatically — do not poll or sleep-wait${%- endif %}.
|
||||
|
||||
Prefer a registered workflow when one fits; author a script for bounded fan-out over a known work list, staged research and verification, or several independent perspectives, and confirm unusually large fan-out first. Before writing or editing a script, read the `create-workflow` skill's SKILL.md. `validate_only: true` runs a path-specific smoke check (metadata, compile, one canned-host path) — not proof that every branch or live tool works.
|
||||
|
||||
|
|
@ -199,7 +199,7 @@ impl xai_tool_runtime::Tool for WorkflowTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
WORKFLOW_TOOL_NAME,
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -128,7 +128,7 @@ impl xai_tool_runtime::Tool for BashConciseTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"run_terminal_cmd",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -52,7 +52,7 @@ impl xai_tool_runtime::Tool for ReadFileConciseTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"read_file",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -61,7 +61,7 @@ impl xai_tool_runtime::Tool for SearchReplaceConciseTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"search_replace",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -22,7 +22,7 @@ use crate::types::tool::{ToolKind, ToolNamespace};
|
|||
use crate::types::resources::resolve_model_path;
|
||||
use crate::util::format_not_found_error;
|
||||
|
||||
const DESCRIPTION: &str = r#"Edit a file using anchors from ${{ tools.by_kind.read }} or ${{ tools.by_kind.search }}.
|
||||
const DESCRIPTION: &str = r#"Edit a file using anchors${%- if tools.by_kind.read and tools.by_kind.search %} from ${{ tools.by_kind.read }} or ${{ tools.by_kind.search }}${%- elif tools.by_kind.read %} from ${{ tools.by_kind.read }}${%- elif tools.by_kind.search %} from ${{ tools.by_kind.search }}${%- endif %}.
|
||||
|
||||
Operations (use the "op" field):
|
||||
|
||||
|
|
@ -65,7 +65,7 @@ Follow-up edits:
|
|||
(e.g. "{example_anchor}"). Always include the line number. Do NOT include → or
|
||||
the line content after it.
|
||||
- Never fabricate or modify anchors — only use exact anchors as returned by
|
||||
previous read, grep, or edit calls."#;
|
||||
previous tool outputs."#;
|
||||
|
||||
/// `hashline_edit` tool — edits files using anchor references.
|
||||
#[derive(Debug, Default)]
|
||||
|
|
@ -264,7 +264,7 @@ impl xai_tool_runtime::Tool for HashlineEditTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"hashline_edit",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -133,11 +133,9 @@ fn parse_rg_line(line: &str) -> Option<(usize, char, &str)> {
|
|||
Some((num, sep, &line[idx + 1..]))
|
||||
}
|
||||
|
||||
const DESCRIPTION: &str = r#"Search file contents with anchor-annotated results for use with ${{ tools.by_kind.edit }}.
|
||||
const DESCRIPTION: &str = r#"Search file contents with anchor-annotated results${%- if tools.by_kind.edit %} for use with ${{ tools.by_kind.edit }}${%- endif %}.
|
||||
|
||||
Match lines include anchors you can pass directly to ${{ tools.by_kind.edit }} without
|
||||
needing to ${{ tools.by_kind.read }} the file first. Unlike ${{ tools.by_kind.read }},
|
||||
this grep format keeps grep-style separators after the anchor: `:` for
|
||||
Match lines include anchors${%- if tools.by_kind.edit %} you can pass directly to ${{ tools.by_kind.edit }}${%- if tools.by_kind.read %} without needing to ${{ tools.by_kind.read }} the file first${%- endif %}${%- endif %}. This grep format keeps grep-style separators after the anchor: `:` for
|
||||
match lines and `-` for context lines.
|
||||
|
||||
Content output format:
|
||||
|
|
@ -210,7 +208,7 @@ impl xai_tool_runtime::Tool for HashlineGrepTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"hashline_grep",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -84,7 +84,7 @@ Anchors are valid only for the file state at read time — after any edit,
|
|||
use the fresh anchors returned by ${{ tools.by_kind.edit }} or re-read the file.${%- endif %}
|
||||
|
||||
Usage:
|
||||
- The ${{ params.read.target_file }} parameter must be an absolute path, not a relative path
|
||||
- The ${{ params.read.target_file }} parameter accepts either a relative path in the workspace or an absolute path
|
||||
- By default reads up to {max_lines_read} lines from the beginning
|
||||
- Optionally specify ${{ params.read.offset }} and ${{ params.read.limit }} for large files
|
||||
- Can read images (PNG, JPG, etc.) and PDF files (each page rendered as an image; use ${{ params.read.pages }} for PDFs with more than 10 pages, max 20 per call)
|
||||
|
|
@ -153,7 +153,7 @@ impl xai_tool_runtime::Tool for HashlineReadTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"hashline_read",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -17,6 +17,7 @@ use async_lsp::lsp_types::{
|
|||
|
||||
use super::config::{LspServerConfig, LspTransport};
|
||||
use super::{DiagnosticsMap, DiagnosticsNotify, LspError, LspMainLoop, file_uri};
|
||||
use crate::util::{ProcessGroup, ProcessScope};
|
||||
|
||||
#[cfg(test)]
|
||||
use super::config::REQUEST_TIMEOUT;
|
||||
|
|
@ -38,6 +39,11 @@ pub struct LspClient {
|
|||
pub main_loop: tokio::task::JoinHandle<()>,
|
||||
pub stderr_task: Option<tokio::task::JoinHandle<()>>,
|
||||
pub child_process: Option<std::process::Child>,
|
||||
/// Strong owner of the server child's process group. The session
|
||||
/// [`ProcessScope`] holds only a `Weak`, so dropping this on clean teardown
|
||||
/// stops the scope from reaping a reused PID. `None` for the socket transport
|
||||
/// (no child) or if group creation failed.
|
||||
process_group: Option<Arc<ProcessGroup>>,
|
||||
pub shutdown_timeout: std::time::Duration,
|
||||
}
|
||||
|
||||
|
|
@ -49,6 +55,16 @@ impl std::fmt::Debug for LspClient {
|
|||
}
|
||||
}
|
||||
|
||||
impl Drop for LspClient {
|
||||
/// Teardown backstop. `LspBackendAdapter`'s graceful shutdown only runs when
|
||||
/// a tokio runtime is current, so killing the child here avoids orphaning one
|
||||
/// language-server process per session. Idempotent with `shutdown`, which
|
||||
/// takes the same fields first.
|
||||
fn drop(&mut self) {
|
||||
self.reap_children();
|
||||
}
|
||||
}
|
||||
|
||||
// ── Startup helpers (called by LspClient::start) ────────────────────────
|
||||
|
||||
type LspMainLoopAndServer = (LspMainLoop, async_lsp::ServerSocket);
|
||||
|
|
@ -226,10 +242,49 @@ impl LspClient {
|
|||
main_loop: main_loop_handle,
|
||||
stderr_task,
|
||||
child_process,
|
||||
process_group: None,
|
||||
shutdown_timeout: std::time::Duration::from_millis(config.shutdown_timeout_ms()),
|
||||
})
|
||||
}
|
||||
|
||||
/// Install a process group for this freshly started stdio server: register a
|
||||
/// `Weak` into the session [`ProcessScope`] (when set) while this client keeps
|
||||
/// the strong `Arc`; installed even without a scope so this client's own
|
||||
/// `Drop` killpg's the whole child tree. No-op for the socket transport.
|
||||
/// See the `process_group` field doc for the Weak/reuse-safety argument.
|
||||
///
|
||||
/// Returns `false` when the scope was already closed (session teardown raced
|
||||
/// this start): the child has been killed at registration, so the caller must
|
||||
/// discard this client instead of installing it as ready.
|
||||
pub(crate) fn enroll(&mut self, scope: Option<&ProcessScope>) -> bool {
|
||||
let Some(child) = self.child_process.as_ref() else {
|
||||
return true;
|
||||
};
|
||||
// Group-creation failures degrade to leader-only cleanup and exempt this
|
||||
// server from session-close reaping — actionable and otherwise invisible,
|
||||
// so they warrant `warn`.
|
||||
let mut group = match ProcessGroup::new() {
|
||||
Ok(g) => g,
|
||||
Err(e) => {
|
||||
tracing::warn!(server = %self.server_name, pid = child.id(), error = %e, "LSP: ProcessGroup::new failed; server exempt from session reaping");
|
||||
return true;
|
||||
}
|
||||
};
|
||||
if let Err(e) = group.attach_std(child) {
|
||||
tracing::warn!(server = %self.server_name, pid = child.id(), error = %e, "LSP: attach to process group failed; server exempt from session reaping");
|
||||
return true;
|
||||
}
|
||||
let group = Arc::new(group);
|
||||
let enrolled = match scope {
|
||||
Some(scope) => scope.register(&group),
|
||||
None => true,
|
||||
};
|
||||
// Keep the strong Arc either way so `Drop`/`shutdown` reap the tree —
|
||||
// including the already-killed leader in the closed-scope case.
|
||||
self.process_group = Some(group);
|
||||
enrolled
|
||||
}
|
||||
|
||||
async fn start_stdio(
|
||||
server_name: &str,
|
||||
config: &LspServerConfig,
|
||||
|
|
@ -346,6 +401,15 @@ impl LspClient {
|
|||
}
|
||||
|
||||
pub async fn shutdown(mut self) {
|
||||
// Dead transport — a crashed server, or the session scope's
|
||||
// SIGKILL-on-close (see grok-shell `take_session`) landing before this
|
||||
// Drop-spawned graceful task ran. The shutdown/exit handshake can only
|
||||
// fail, so skip it (and its warnings) and just reap.
|
||||
if self.main_loop.is_finished() {
|
||||
tracing::debug!(server = %self.server_name, "LSP transport already down; skipping shutdown handshake");
|
||||
self.reap_children();
|
||||
return;
|
||||
}
|
||||
self.close_all_documents();
|
||||
|
||||
let result = tokio::time::timeout(self.shutdown_timeout, async {
|
||||
|
|
@ -367,16 +431,29 @@ impl LspClient {
|
|||
self.main_loop.abort();
|
||||
}
|
||||
|
||||
if let Err(e) = self.main_loop.await
|
||||
// `&mut`-await (not move) so `self` is never partially moved: `LspClient`
|
||||
// has a `Drop` impl, and you cannot move fields out of a `Drop` type.
|
||||
if let Err(e) = (&mut self.main_loop).await
|
||||
&& !e.is_cancelled()
|
||||
{
|
||||
tracing::warn!(server = %self.server_name, error = %e, "LSP main loop task panicked");
|
||||
}
|
||||
|
||||
if let Some(task) = self.stderr_task {
|
||||
self.reap_children();
|
||||
}
|
||||
|
||||
/// Shared teardown for `Drop` and `shutdown`: abort the tasks, then reap
|
||||
/// grandchildren via the group before the leader. Idempotent via `take`, so
|
||||
/// running it from both paths is safe.
|
||||
fn reap_children(&mut self) {
|
||||
self.main_loop.abort();
|
||||
if let Some(task) = self.stderr_task.take() {
|
||||
task.abort();
|
||||
}
|
||||
if let Some(mut child) = self.child_process {
|
||||
if let Some(group) = self.process_group.take() {
|
||||
let _ = group.kill();
|
||||
}
|
||||
if let Some(mut child) = self.child_process.take() {
|
||||
let _ = child.kill();
|
||||
let _ = child.wait();
|
||||
}
|
||||
|
|
|
|||
|
|
@ -111,10 +111,10 @@ async fn bootstrap_lsp(
|
|||
mgr.restartable_servers()
|
||||
};
|
||||
for name in restartable {
|
||||
let mgr_clone = lsp_manager.clone();
|
||||
tokio::spawn(crate::implementations::lsp::restart_monitor(
|
||||
mgr_clone, name,
|
||||
));
|
||||
// Hand the monitor a `Weak` so it never keeps the manager (and its
|
||||
// language-server children) alive past the owning session.
|
||||
let mgr_weak = Arc::downgrade(&lsp_manager);
|
||||
tokio::spawn(crate::implementations::lsp::restart_monitor(mgr_weak, name));
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
|
|
|||
|
|
@ -9,6 +9,7 @@ use async_lsp::lsp_types::{DiagnosticSeverity, Url};
|
|||
use super::client::LspClient;
|
||||
use super::config::LspServerConfig;
|
||||
use super::{DiagnosticsNotify, file_uri};
|
||||
use crate::util::ProcessScope;
|
||||
|
||||
#[cfg(test)]
|
||||
use super::format::{format_locations_labeled, format_symbols};
|
||||
|
|
@ -45,6 +46,10 @@ pub struct LspManager {
|
|||
pub shutting_down: bool,
|
||||
pub next_lifecycle_id: u64,
|
||||
pub notification_handle: crate::notification::ToolNotificationHandle,
|
||||
/// When set, each spawned server's process group is registered here so the
|
||||
/// agent can reap the language-server trees on session close. `None` outside
|
||||
/// an agent session.
|
||||
pub process_scope: Option<ProcessScope>,
|
||||
}
|
||||
|
||||
impl Default for LspManager {
|
||||
|
|
@ -60,6 +65,7 @@ impl Default for LspManager {
|
|||
shutting_down: false,
|
||||
next_lifecycle_id: 1,
|
||||
notification_handle: crate::notification::ToolNotificationHandle::noop(),
|
||||
process_scope: None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -80,6 +86,13 @@ impl LspManager {
|
|||
}
|
||||
}
|
||||
|
||||
/// Attach the session process scope so spawned servers are enrolled for
|
||||
/// reclaim.
|
||||
pub fn with_process_scope(mut self, scope: Option<ProcessScope>) -> Self {
|
||||
self.process_scope = scope;
|
||||
self
|
||||
}
|
||||
|
||||
pub fn is_initialized(&self) -> bool {
|
||||
self.initialized
|
||||
}
|
||||
|
|
@ -140,7 +153,24 @@ impl LspManager {
|
|||
)
|
||||
.await
|
||||
{
|
||||
Ok(client) => {
|
||||
Ok(mut client) => {
|
||||
if !client.enroll(self.process_scope.as_ref()) {
|
||||
// Session teardown raced this start: the closed scope
|
||||
// killed the child at registration. Installing the
|
||||
// client would advertise a dead server and feed the
|
||||
// restart monitor respawn churn, so stop starting
|
||||
// servers for this manager instead.
|
||||
tracing::info!(server = %name, "session scope closed during LSP start; discarding server");
|
||||
self.shutting_down = true;
|
||||
return;
|
||||
}
|
||||
// Same race as the restart path: `kill_all` landing between
|
||||
// enroll and insert has already SIGKILLed the child.
|
||||
if self.process_scope.as_ref().is_some_and(|s| s.is_closed()) {
|
||||
tracing::info!(server = %name, "session scope closed during LSP start; discarding server");
|
||||
self.shutting_down = true;
|
||||
return;
|
||||
}
|
||||
tracing::info!(server = %name, "LSP server ready");
|
||||
self.notification_handle
|
||||
.send_lsp_ready(crate::notification::LspServerReady {
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
//! Monitors LSP servers for crashes and auto-restarts them.
|
||||
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
use std::sync::{Arc, Weak};
|
||||
|
||||
use async_lsp::lsp_types::Url;
|
||||
|
||||
|
|
@ -9,16 +9,21 @@ use super::client::LspClient;
|
|||
use super::config::LspServerConfig;
|
||||
use super::manager::LspManager;
|
||||
use super::{DiagnosticsNotify, file_uri};
|
||||
use crate::util::ProcessScope;
|
||||
|
||||
/// Waits for the current lifecycle to exit.
|
||||
/// Waits for the current lifecycle to exit. Returns `None` (stop monitoring)
|
||||
/// when the manager is gone or the server's client has been removed.
|
||||
///
|
||||
/// See `restart_monitor` for the Weak/lifetime argument.
|
||||
async fn wait_for_crashed_lifecycle(
|
||||
lsp_manager: &Arc<tokio::sync::Mutex<LspManager>>,
|
||||
lsp_manager: &Weak<tokio::sync::Mutex<LspManager>>,
|
||||
server_name: &str,
|
||||
poll_interval: std::time::Duration,
|
||||
) -> Option<u64> {
|
||||
loop {
|
||||
tokio::time::sleep(poll_interval).await;
|
||||
let mgr = lsp_manager.lock().await;
|
||||
let mgr_arc = lsp_manager.upgrade()?;
|
||||
let mgr = mgr_arc.lock().await;
|
||||
match mgr.clients.get(server_name) {
|
||||
Some(client) if client.main_loop.is_finished() => return Some(client.lifecycle_id),
|
||||
Some(_) => continue,
|
||||
|
|
@ -51,13 +56,7 @@ async fn take_crashed_client_if_current(
|
|||
lsp_manager: &Arc<tokio::sync::Mutex<LspManager>>,
|
||||
server_name: &str,
|
||||
crashed_lifecycle_id: u64,
|
||||
) -> Option<(
|
||||
u64,
|
||||
LspServerConfig,
|
||||
PathBuf,
|
||||
DiagnosticsNotify,
|
||||
Vec<(String, String)>,
|
||||
)> {
|
||||
) -> Option<RestartContext> {
|
||||
let mut mgr = lsp_manager.lock().await;
|
||||
let current = mgr.clients.get(server_name)?;
|
||||
if current.lifecycle_id != crashed_lifecycle_id {
|
||||
|
|
@ -70,16 +69,15 @@ async fn take_crashed_client_if_current(
|
|||
.map(|client| client.tracked_documents())
|
||||
.unwrap_or_default();
|
||||
let server_config = mgr.servers.get(server_name).cloned()?;
|
||||
let new_lifecycle_id = mgr.alloc_lifecycle_id();
|
||||
let workspace_root = mgr.workspace_root.clone();
|
||||
let diagnostics_notify = mgr.diagnostics_ready.clone();
|
||||
Some((
|
||||
new_lifecycle_id,
|
||||
let next_lifecycle_id = mgr.alloc_lifecycle_id();
|
||||
Some(RestartContext {
|
||||
next_lifecycle_id,
|
||||
server_config,
|
||||
workspace_root,
|
||||
diagnostics_notify,
|
||||
workspace_root: mgr.workspace_root.clone(),
|
||||
diagnostics_notify: mgr.diagnostics_ready.clone(),
|
||||
tracked_docs,
|
||||
))
|
||||
process_scope: mgr.process_scope.clone(),
|
||||
})
|
||||
}
|
||||
|
||||
/// Removes the crashed client if it is still current.
|
||||
|
|
@ -125,6 +123,9 @@ struct RestartContext {
|
|||
workspace_root: PathBuf,
|
||||
diagnostics_notify: DiagnosticsNotify,
|
||||
tracked_docs: Vec<(String, String)>,
|
||||
/// Snapshotted under the same manager lock as the rest of the context so
|
||||
/// each restart enrolls without an extra lock roundtrip.
|
||||
process_scope: Option<ProcessScope>,
|
||||
}
|
||||
|
||||
enum RestartOutcome {
|
||||
|
|
@ -183,6 +184,7 @@ async fn restart_lsp_with_retries(
|
|||
workspace_root,
|
||||
diagnostics_notify,
|
||||
tracked_docs,
|
||||
process_scope,
|
||||
} = restart_ctx;
|
||||
|
||||
loop {
|
||||
|
|
@ -208,8 +210,23 @@ async fn restart_lsp_with_retries(
|
|||
.await
|
||||
{
|
||||
Ok(mut restarted_client) => {
|
||||
if !restarted_client.enroll(process_scope.as_ref()) {
|
||||
// Closed scope == session teardown killed the child at
|
||||
// registration; drop the client rather than install a dead
|
||||
// server and churn through further respawns.
|
||||
tracing::info!(server = %server_name, "session scope closed, dropping restarted server");
|
||||
return RestartOutcome::Shutdown;
|
||||
}
|
||||
let replayed_doc_count = tracked_docs.len();
|
||||
let replayed_uris = replay_tracked_documents(&mut restarted_client, &tracked_docs);
|
||||
// Re-check after the replay window: a `kill_all` between enroll
|
||||
// and install has already SIGKILLed the enrolled child, and
|
||||
// `install` only checks `shutting_down` (never set by
|
||||
// `kill_all`) — installing here would mark a dead server ready.
|
||||
if process_scope.as_ref().is_some_and(|s| s.is_closed()) {
|
||||
tracing::info!(server = %server_name, "session scope closed during restart, dropping restarted server");
|
||||
return RestartOutcome::Shutdown;
|
||||
}
|
||||
if let Err(restarted_client) = install_restarted_client(
|
||||
lsp_manager,
|
||||
server_name,
|
||||
|
|
@ -264,8 +281,13 @@ async fn restart_lsp_with_retries(
|
|||
}
|
||||
|
||||
/// Monitors one server entry and replaces crashed lifecycles.
|
||||
///
|
||||
/// Takes a `Weak` to the manager so the monitor never keeps the `LspManager`
|
||||
/// (and its child processes) alive past the owning session: it upgrades only
|
||||
/// briefly per poll and for the duration of a single restart. When the manager
|
||||
/// is dropped at session teardown, the next upgrade fails and the monitor exits.
|
||||
pub async fn restart_monitor(
|
||||
lsp_manager: Arc<tokio::sync::Mutex<LspManager>>,
|
||||
lsp_manager: Weak<tokio::sync::Mutex<LspManager>>,
|
||||
server_name: String,
|
||||
) {
|
||||
// Lifetime restart budget for this server monitor. Successful restarts
|
||||
|
|
@ -282,6 +304,12 @@ pub async fn restart_monitor(
|
|||
return;
|
||||
};
|
||||
|
||||
// Upgrade to a strong ref only for this restart; stop if the manager was
|
||||
// dropped between crash detection and here.
|
||||
let Some(lsp_manager) = lsp_manager.upgrade() else {
|
||||
return;
|
||||
};
|
||||
|
||||
let max_restarts = {
|
||||
let mgr = lsp_manager.lock().await;
|
||||
mgr.servers
|
||||
|
|
@ -339,25 +367,12 @@ pub async fn restart_monitor(
|
|||
backoff_ms: backoff.as_millis() as u64,
|
||||
});
|
||||
|
||||
let Some((
|
||||
next_lifecycle_id,
|
||||
server_config,
|
||||
workspace_root,
|
||||
diagnostics_notify,
|
||||
tracked_docs,
|
||||
)) = take_crashed_client_if_current(&lsp_manager, &server_name, crashed_lifecycle_id).await
|
||||
let Some(restart) =
|
||||
take_crashed_client_if_current(&lsp_manager, &server_name, crashed_lifecycle_id).await
|
||||
else {
|
||||
continue;
|
||||
};
|
||||
|
||||
let restart = RestartContext {
|
||||
next_lifecycle_id,
|
||||
server_config,
|
||||
workspace_root,
|
||||
diagnostics_notify,
|
||||
tracked_docs,
|
||||
};
|
||||
|
||||
match restart_lsp_with_retries(
|
||||
&lsp_manager,
|
||||
&server_name,
|
||||
|
|
|
|||
|
|
@ -938,7 +938,7 @@ async fn e2e_restart_monitor_preserves_replacement_client() {
|
|||
|
||||
let lsp_manager = Arc::new(tokio::sync::Mutex::new(mgr));
|
||||
let monitor = tokio::task::spawn_local(restart_monitor(
|
||||
lsp_manager.clone(),
|
||||
Arc::downgrade(&lsp_manager),
|
||||
"mock-ts".to_string(),
|
||||
));
|
||||
|
||||
|
|
@ -946,18 +946,15 @@ async fn e2e_restart_monitor_preserves_replacement_client() {
|
|||
|
||||
{
|
||||
let mut mgr = lsp_manager.lock().await;
|
||||
let stale = mgr.clients.remove("mock-ts").unwrap();
|
||||
mgr.clients.insert(
|
||||
"mock-ts".to_string(),
|
||||
LspClient {
|
||||
lifecycle_id: original_lifecycle_id,
|
||||
open_documents: tracked_docs
|
||||
.into_iter()
|
||||
.map(|(uri, lang)| (uri, (0, lang)))
|
||||
.collect(),
|
||||
..stale
|
||||
},
|
||||
);
|
||||
// Mutate in place: LspClient now implements Drop, so moving
|
||||
// fields out with `..stale` is rejected.
|
||||
let mut stale = mgr.clients.remove("mock-ts").unwrap();
|
||||
stale.lifecycle_id = original_lifecycle_id;
|
||||
stale.open_documents = tracked_docs
|
||||
.into_iter()
|
||||
.map(|(uri, lang)| (uri, (0, lang)))
|
||||
.collect();
|
||||
mgr.clients.insert("mock-ts".to_string(), stale);
|
||||
let healthy_lifecycle_id = mgr.alloc_lifecycle_id();
|
||||
let healthy = LspClient::start(
|
||||
"mock-ts".to_string(),
|
||||
|
|
@ -989,6 +986,36 @@ async fn e2e_restart_monitor_preserves_replacement_client() {
|
|||
.await;
|
||||
}
|
||||
|
||||
/// The monitor holds only a `Weak` to the manager, so once the sole strong
|
||||
/// `Arc` drops at session teardown the next poll's upgrade fails and the task
|
||||
/// must exit. A `Weak`->`Arc` regression would keep the manager (and its child
|
||||
/// processes) alive and the join would time out.
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
async fn restart_monitor_exits_when_manager_arc_dropped() {
|
||||
tokio::task::LocalSet::new()
|
||||
.run_until(async {
|
||||
let (_dir, script_path) = write_mock_server();
|
||||
let workspace = tempfile::tempdir().unwrap();
|
||||
let mgr = single_server_manager(&script_path, &workspace).await;
|
||||
|
||||
// A live client keeps the monitor polling (rather than exiting on a
|
||||
// missing client), so the only way out is a failed `Weak` upgrade.
|
||||
let lsp_manager = Arc::new(tokio::sync::Mutex::new(mgr));
|
||||
let monitor = tokio::task::spawn_local(restart_monitor(
|
||||
Arc::downgrade(&lsp_manager),
|
||||
"mock-ts".to_string(),
|
||||
));
|
||||
|
||||
drop(lsp_manager);
|
||||
|
||||
tokio::time::timeout(std::time::Duration::from_secs(5), monitor)
|
||||
.await
|
||||
.expect("monitor must exit once the manager Arc is dropped")
|
||||
.expect("monitor task should not panic");
|
||||
})
|
||||
.await;
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
async fn e2e_finalize_no_longer_blocks_on_slow_lsp_startup() {
|
||||
let (_dir, script_path) = write_slow_init_server(1_500);
|
||||
|
|
@ -1152,7 +1179,7 @@ async fn e2e_restart_monitor_emits_failed_on_restart_init_error() {
|
|||
|
||||
let lsp_manager = Arc::new(tokio::sync::Mutex::new(mgr));
|
||||
let monitor = tokio::task::spawn_local(restart_monitor(
|
||||
lsp_manager.clone(),
|
||||
Arc::downgrade(&lsp_manager),
|
||||
"failing".to_string(),
|
||||
));
|
||||
|
||||
|
|
@ -1311,3 +1338,110 @@ async fn e2e_restart_replay_requeues_pending_diagnostics() {
|
|||
|
||||
mgr.lock().await.shutdown().await;
|
||||
}
|
||||
|
||||
/// Polls `try_wait` until the child is reaped or the budget expires; a live
|
||||
/// child (failed kill) times out and returns false.
|
||||
#[cfg(unix)]
|
||||
async fn std_child_died(child: &mut std::process::Child) -> bool {
|
||||
for _ in 0..50 {
|
||||
if matches!(child.try_wait(), Ok(Some(_))) {
|
||||
return true;
|
||||
}
|
||||
tokio::time::sleep(std::time::Duration::from_millis(100)).await;
|
||||
}
|
||||
false
|
||||
}
|
||||
|
||||
/// A language-server child enrolled via a `ProcessScope` is reaped by
|
||||
/// `kill_all`, proving session close reclaims LSP servers even while the owning
|
||||
/// client is still alive (the wedged-actor case).
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn scope_reaps_enrolled_language_server_child() {
|
||||
let (_script_dir, _workspace, mut client) = start_mock_client().await;
|
||||
|
||||
let scope = crate::util::ProcessScope::new();
|
||||
assert!(
|
||||
client.enroll(Some(&scope)),
|
||||
"enroll on an open scope must succeed"
|
||||
);
|
||||
assert_eq!(
|
||||
scope.live_count(),
|
||||
1,
|
||||
"enroll must register the server group"
|
||||
);
|
||||
|
||||
// Take the child handle to observe death; the client keeps the owning
|
||||
// Arc<ProcessGroup>, so the scope's weak stays live across kill_all.
|
||||
let mut child = client.child_process.take().expect("stdio child");
|
||||
scope.kill_all();
|
||||
|
||||
// Prove kill_all killed the child WHILE the client is still alive (the
|
||||
// wedged-actor case) — dropping the client first would let its own Drop
|
||||
// killpg mask a broken kill_all. `waitid(WNOWAIT)` observes the exit
|
||||
// without reaping (signal 0 can't: it succeeds on a zombie), so the
|
||||
// SIGKILLed leader stays unreaped and its pgid reserved. nix only
|
||||
// exposes waitid on Linux; macOS falls back to the weaker
|
||||
// drop-then-reap order below (CI runs the strong branch).
|
||||
#[cfg(target_os = "linux")]
|
||||
{
|
||||
let pid = nix::unistd::Pid::from_raw(child.id() as i32);
|
||||
let mut died = false;
|
||||
for _ in 0..50 {
|
||||
use nix::sys::wait::{Id, WaitPidFlag, WaitStatus, waitid};
|
||||
match waitid(
|
||||
Id::Pid(pid),
|
||||
WaitPidFlag::WEXITED | WaitPidFlag::WNOWAIT | WaitPidFlag::WNOHANG,
|
||||
) {
|
||||
Ok(WaitStatus::StillAlive) => {
|
||||
tokio::time::sleep(std::time::Duration::from_millis(100)).await;
|
||||
}
|
||||
Ok(_) => {
|
||||
died = true;
|
||||
break;
|
||||
}
|
||||
Err(e) => panic!("waitid on the server child failed: {e}"),
|
||||
}
|
||||
}
|
||||
assert!(
|
||||
died,
|
||||
"scope.kill_all must kill the enrolled server child while the client is alive"
|
||||
);
|
||||
}
|
||||
|
||||
// Linux: the leader is dead but unreaped, so its pgid is still reserved —
|
||||
// the client Drop's killpg targets the zombie's group, not a reused pgid.
|
||||
// macOS: dropping before the reap keeps the same pgid-reservation safety,
|
||||
// at the cost of not isolating kill_all from the Drop killpg.
|
||||
drop(client);
|
||||
|
||||
assert!(
|
||||
std_child_died(&mut child).await,
|
||||
"scope.kill_all must reap the enrolled server child"
|
||||
);
|
||||
}
|
||||
|
||||
/// Dropping an `LspClient` without `shutdown` still reaps its server child, so a
|
||||
/// session never orphans a language server when graceful teardown is skipped.
|
||||
/// Probes the pid with signal 0 (ESRCH once reaped) since the client owns the
|
||||
/// child handle and waits on it during `Drop`.
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn drop_reaps_server_child_without_shutdown() {
|
||||
let (_script_dir, _workspace, client) = start_mock_client().await;
|
||||
|
||||
let pid = client.child_process.as_ref().expect("stdio child").id() as i32;
|
||||
let alive = |pid: i32| nix::sys::signal::kill(nix::unistd::Pid::from_raw(pid), None).is_ok();
|
||||
assert!(alive(pid), "server child should be running before drop");
|
||||
|
||||
drop(client);
|
||||
|
||||
let deadline = std::time::Instant::now() + std::time::Duration::from_secs(3);
|
||||
while alive(pid) && std::time::Instant::now() < deadline {
|
||||
tokio::time::sleep(std::time::Duration::from_millis(50)).await;
|
||||
}
|
||||
assert!(
|
||||
!alive(pid),
|
||||
"Drop must reap the server child even without shutdown"
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -66,7 +66,7 @@ impl xai_tool_runtime::Tool for MemoryGetImpl {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"memory_get",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
@ -97,8 +97,11 @@ impl xai_tool_runtime::Tool for MemoryGetImpl {
|
|||
};
|
||||
let memory = memory.clone();
|
||||
tracing::info!(target: crate::types::memory_backend::MEMORY_LOG_TARGET,"MEMORY_GET: invoked");
|
||||
// `from` is 1-based in the client schema (matching displayed line
|
||||
// numbers); the backend expects a 0-based offset. 0 is treated as 1.
|
||||
let from_zero_based = input.from.map(|f| f.saturating_sub(1));
|
||||
let content = memory
|
||||
.get(&input.path, input.from, input.lines)
|
||||
.get(&input.path, from_zero_based, input.lines)
|
||||
.map_err(|e| {
|
||||
xai_tool_runtime::ToolError::execution(
|
||||
xai_tool_protocol::ToolId::new("memory_get").expect("valid"),
|
||||
|
|
@ -106,7 +109,7 @@ impl xai_tool_runtime::Tool for MemoryGetImpl {
|
|||
)
|
||||
})?;
|
||||
let total_lines = content.lines().count();
|
||||
let first_line_num = input.from.unwrap_or(0) + 1;
|
||||
let first_line_num = from_zero_based.unwrap_or(0) + 1;
|
||||
let numbered = format_with_line_numbers(&content, first_line_num);
|
||||
let output = format!(
|
||||
"**File:** {}\n**Lines:** {} (from: {}, limit: {})\n\n{}",
|
||||
|
|
@ -135,8 +138,8 @@ mod tests {
|
|||
/// actual position in the source file, not the slice position.
|
||||
#[test]
|
||||
fn test_format_offset_adjusts_line_numbers() {
|
||||
// Simulates memory_get called with from=4 (0-based) — first displayed
|
||||
// line should be labelled "5" (1-based).
|
||||
// Simulates memory_get called with from=5 (1-based) — first displayed
|
||||
// line should be labelled "5".
|
||||
let out = format_with_line_numbers("line five\nline six", 5);
|
||||
assert!(out.starts_with("5→line five"), "got: {out}");
|
||||
assert!(out.ends_with("6→line six"), "got: {out}");
|
||||
|
|
|
|||
|
|
@ -45,7 +45,7 @@ impl xai_tool_runtime::Tool for MemorySearchImpl {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"memory_search",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -37,7 +37,8 @@ pub struct MemorySearchOutput {
|
|||
pub struct MemoryGetInput {
|
||||
/// Path to the memory file to read.
|
||||
pub path: String,
|
||||
/// 0-based start line (default: beginning of file).
|
||||
/// 1-based start line, matching the line numbers in the tool's output
|
||||
/// (default: beginning of file). 0 is accepted and treated as 1.
|
||||
#[serde(default)]
|
||||
pub from: Option<usize>,
|
||||
/// Maximum number of lines to return (default: all).
|
||||
|
|
|
|||
|
|
@ -1,9 +1,11 @@
|
|||
//! `bash` tool — OpenCode namespace.
|
||||
//!
|
||||
//! Executes shell commands in a persistent terminal session with optional
|
||||
//! timeout and working directory override. Delegates to the shared
|
||||
//! `TerminalBackend` for process management, output streaming, and
|
||||
//! background task support.
|
||||
//! Executes shell commands with optional timeout and working directory
|
||||
//! override. Delegates to the shared `TerminalBackend` for process
|
||||
//! management, output streaming, and background task support. The
|
||||
//! grok-tools-server serves this tool on the stateless local backend
|
||||
//! (fresh shell per command; persistent shell state is only enabled when
|
||||
//! `Cursor:Shell` is served), which is what the description documents.
|
||||
//!
|
||||
//! ## Resources
|
||||
//!
|
||||
|
|
@ -44,7 +46,7 @@ const DEFAULT_TIMEOUT: Duration = Duration::from_secs(120);
|
|||
// Description
|
||||
// ───────────────────────────────────────────────────────────────────────────
|
||||
|
||||
const DESCRIPTION: &str = r#"Executes a given bash command in a persistent shell session with optional timeout, ensuring proper handling and security measures.
|
||||
const DESCRIPTION: &str = r#"Executes a given bash command in a shell session with optional timeout, ensuring proper handling and security measures. Each command runs in a fresh shell: working directory changes and environment variables do not persist between calls.
|
||||
IMPORTANT: This tool is for terminal operations like git, npm, docker, etc. DO NOT use it for file operations (reading, writing, editing, searching, finding files) - use the specialized tools for this instead.
|
||||
|
||||
Before executing the command, please follow these steps:
|
||||
|
|
@ -71,8 +73,8 @@ Usage notes:
|
|||
${%- if tools.by_kind.list or tools.by_kind.search or tools.by_kind.read or tools.by_kind.edit or tools.by_kind.write %}
|
||||
${%- if has_unix_utilities %}
|
||||
- Avoid using this tool with the `find`, `grep`, `cat`, `head`, `tail`, `sed`, `awk`, or `echo` commands, unless explicitly instructed or when these commands are truly necessary for the task. Instead, always prefer using the dedicated tools for these commands:${%- if tools.by_kind.list %}
|
||||
- File search: Use ${{ tools.by_kind.list }} (NOT find or ls)${%- endif %}${%- if tools.by_kind.search %}
|
||||
- Content search: Use ${{ tools.by_kind.search }} (NOT grep or rg)${%- endif %}${%- if tools.by_kind.read %}
|
||||
- File search: Use the ${{ tools.by_kind.list }} tool (NOT the `find` or `ls` shell commands)${%- endif %}${%- if tools.by_kind.search %}
|
||||
- Content search: Use the ${{ tools.by_kind.search }} tool (NOT the `grep` or `rg` shell commands)${%- endif %}${%- if tools.by_kind.read %}
|
||||
- Read files: Use ${{ tools.by_kind.read }} (NOT cat/head/tail)${%- endif %}${%- if tools.by_kind.edit %}
|
||||
- Edit files: Use ${{ tools.by_kind.edit }} (NOT sed/awk)${%- endif %}${%- if tools.by_kind.write %}
|
||||
- Write files: Use ${{ tools.by_kind.write }} (NOT echo >/cat <<EOF)${%- endif %}
|
||||
|
|
@ -319,7 +321,7 @@ impl xai_tool_runtime::Tool for BashTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"bash",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -48,9 +48,8 @@ use crate::types::tool::{ToolKind, ToolNamespace};
|
|||
// to "" here (the kind-params map is keyed by schema property names).
|
||||
const DESCRIPTION: &str = r#"Performs exact string replacements in files.
|
||||
|
||||
Usage:
|
||||
- You must use your `${{ tools.by_kind.read }}` tool at least once in the conversation before editing.
|
||||
- When editing text from ${{ tools.by_kind.read }} tool output, ensure you preserve the exact indentation (tabs/spaces) as it appears AFTER the line number prefix. The line number prefix format is: line number + →. Everything after that → separator is the actual file content to match. Never include any part of the line number prefix in the ${{ params.edit.oldString }} or ${{ params.edit.newString }}.
|
||||
Usage:${%- if tools.by_kind.read %}
|
||||
- When editing text from ${{ tools.by_kind.read }} tool output, ensure you preserve the exact indentation (tabs/spaces) as it appears AFTER the line number prefix. The line number prefix format is: line number + ": ". Everything after that ": " separator is the actual file content to match. Never include any part of the line number prefix in the ${{ params.edit.oldString }} or ${{ params.edit.newString }}.${%- endif %}
|
||||
- ALWAYS prefer editing existing files in the codebase. NEVER write new files unless explicitly required.
|
||||
- The edit will FAIL if `${{ params.edit.oldString }}` is not unique in the file. Either provide a larger string with more surrounding context to make it unique or use `${{ params.edit.replaceAll }}` to change every instance of `${{ params.edit.oldString }}`.
|
||||
- Use `${{ params.edit.replaceAll }}` for replacing and renaming strings across the file. This parameter is useful if you want to rename a variable for instance.
|
||||
|
|
@ -156,7 +155,7 @@ impl xai_tool_runtime::Tool for EditTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"edit",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -28,12 +28,14 @@ const MAX_STDOUT_BYTES: usize = 5_000_000;
|
|||
|
||||
// ─── Description ────────────────────────────────────────────────────
|
||||
|
||||
const DESCRIPTION: &str = r#"Lists files and directories in a given path.
|
||||
const DESCRIPTION: &str = r#"Fast file pattern matching tool that works with any codebase size.
|
||||
|
||||
Other details:
|
||||
- The result does not display dot-files and dot-directories.
|
||||
- Respects .gitignore patterns (files/directories ignored by git are not shown).
|
||||
- Large directories are summarized with file counts and extension breakdowns instead of listing all files."#;
|
||||
- Supports glob patterns like "**/*.js" or "src/**/*.ts" via the required ${{ params.list.pattern }} parameter
|
||||
- Optionally set ${{ params.list.path }} to pick the directory to search in (defaults to the current working directory)
|
||||
- Returns matching file paths sorted by modification time (most recent first), capped at 100 results
|
||||
- Hidden (dot) files are included; .gitignore patterns are respected for paths the pattern does not explicitly match
|
||||
- Use this tool when you need to find files by name patterns
|
||||
- You can call multiple tools in a single response. It is always better to speculatively perform multiple searches as a batch that are potentially useful."#;
|
||||
|
||||
// ─── Input ──────────────────────────────────────────────────────────
|
||||
|
||||
|
|
@ -132,7 +134,7 @@ impl xai_tool_runtime::Tool for GlobTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"glob",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
@ -336,6 +338,34 @@ mod tests {
|
|||
resources
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn description_template_tracks_renamed_pattern_and_path() {
|
||||
use crate::types::template_renderer::TemplateRenderer;
|
||||
use crate::types::tool_metadata::ToolMetadata;
|
||||
use std::collections::HashMap;
|
||||
|
||||
let tools = HashMap::from([(ToolKind::List, "glob".to_string())]);
|
||||
let params = HashMap::from([(
|
||||
ToolKind::List,
|
||||
HashMap::from([
|
||||
("pattern".to_string(), "file_pattern".to_string()),
|
||||
("path".to_string(), "search_dir".to_string()),
|
||||
]),
|
||||
)]);
|
||||
let rendered = TemplateRenderer::new(tools, params)
|
||||
.render(ToolMetadata::description_template(&GlobTool))
|
||||
.unwrap();
|
||||
assert!(
|
||||
rendered.contains("required file_pattern parameter")
|
||||
&& rendered.contains("set search_dir"),
|
||||
"renamed pattern/path params must appear:\n{rendered}"
|
||||
);
|
||||
assert!(
|
||||
!rendered.contains("extension breakdowns") && !rendered.contains("dot-directories"),
|
||||
"stale list_dir-style claims must not remain:\n{rendered}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn glob_finds_matching_files() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
|
|
|
|||
|
|
@ -124,7 +124,7 @@ impl xai_tool_runtime::Tool for GrepTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"grep",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -34,19 +34,18 @@ const MAX_BYTES: usize = 50 * 1024;
|
|||
|
||||
// ─── Description ────────────────────────────────────────────────────
|
||||
|
||||
const DESCRIPTION: &str = r#"Reads a file from the local filesystem. You can access any file directly by using this tool.
|
||||
const DESCRIPTION: &str = r#"Reads a file or directory from the local filesystem. You can access any file directly by using this tool.
|
||||
Assume this tool is able to read all files on the machine. If the User provides a path to a file assume that path is valid. It is okay to read a file that does not exist; an error will be returned.
|
||||
|
||||
Usage:
|
||||
- The ${{ params.read.filePath }} parameter must be an absolute path, not a relative path
|
||||
- By default, it reads up to {max_lines_read} lines starting from the beginning of the file
|
||||
- By default, it reads up to 2000 lines starting from the beginning of the file
|
||||
- You can optionally specify ${{ params.read.offset }} and ${{ params.read.limit }} (especially handy for long files), but it's recommended to read the whole file by not providing these parameters
|
||||
- Any lines longer than {max_chars_per_line} characters will be truncated
|
||||
- Results are returned using cat -n format, with line numbers starting at 1. The format is: LINE_NUMBER→LINE_CONTENT, where LINE_NUMBER is right-aligned and padded with spaces
|
||||
- Contents are returned with each line prefixed by its line number as `LINE_NUMBER: LINE_CONTENT`, with line numbers starting at 1. For example, if a file has contents "foo\n", you will receive "1: foo"
|
||||
- For directories, entries are returned one per line (without line numbers) with a trailing `/` for subdirectories
|
||||
- This tool can read images (eg PNG, JPG, etc). When reading an image file the contents are presented visually as this tool uses multimodal LLMs.
|
||||
- This tool can read PDF files (.pdf). PDFs are processed page by page, extracting both text and visual content for analysis.
|
||||
- This tool can read Jupyter notebooks (.ipynb files) and returns all cells with their outputs, combining code, text, and visualizations.
|
||||
- This tool can only read files, not directories.${%- if tools.by_kind.execute %} To read a directory, use an ls command via the ${{ tools.by_kind.execute }} tool.${%- endif %}
|
||||
- This tool can read PDF files (.pdf). PDFs are presented visually to the multimodal LLM as attachments.
|
||||
- You can call multiple tools in a single response. It is always better to speculatively read multiple potentially useful files in parallel.
|
||||
- You will regularly be asked to read screenshots. If the user provides a path to a screenshot, ALWAYS use this tool to view the file at the path. This tool will work with all temporary file paths.
|
||||
- If you read a file that exists but has empty contents you will receive a system reminder warning in place of file contents."#;
|
||||
|
|
@ -129,7 +128,7 @@ impl xai_tool_runtime::Tool for ReadTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"read",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
@ -541,12 +540,16 @@ mod tests {
|
|||
"renamed offset/limit must appear:\n{rendered}"
|
||||
);
|
||||
assert!(
|
||||
rendered.contains("via the run_command tool"),
|
||||
"resolved execute tool name must appear:\n{rendered}"
|
||||
rendered.contains("file or directory")
|
||||
&& rendered.contains("trailing `/` for subdirectories"),
|
||||
"directory support must be documented:\n{rendered}"
|
||||
);
|
||||
assert!(
|
||||
!rendered.contains("a line offset and limit") && !rendered.contains("Bash tool"),
|
||||
"stale offset/limit/Bash-tool literals must not remain:\n{rendered}"
|
||||
!rendered.contains("only read files")
|
||||
&& !rendered.contains("ls command")
|
||||
&& !rendered.contains("a line offset and limit")
|
||||
&& !rendered.contains("Bash tool"),
|
||||
"stale files-only/ls/offset/Bash-tool literals must not remain:\n{rendered}"
|
||||
);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -224,7 +224,7 @@ impl xai_tool_runtime::Tool for SkillTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"skill",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -312,7 +312,7 @@ impl xai_tool_runtime::Tool for TodoWriteTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"todowrite",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -19,7 +19,7 @@ use crate::types::tool::{ToolKind, ToolNamespace};
|
|||
|
||||
const DESCRIPTION: &str = r#"Create or overwrite a file.
|
||||
|
||||
- Writing to an existing path replaces the file — read it first with the ${{ tools.by_kind.read }} tool.
|
||||
- Writing to an existing path replaces the file${%- if tools.by_kind.read %} — read it first with the ${{ tools.by_kind.read }} tool${%- endif %}.
|
||||
- Parent directories are created for you."#;
|
||||
|
||||
// ─── Input ───────────────────────────────────────────────────────────
|
||||
|
|
@ -78,7 +78,7 @@ impl xai_tool_runtime::Tool for WriteTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"write",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -236,7 +236,7 @@ impl xai_tool_runtime::Tool for SearchTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"search_tool",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -56,6 +56,10 @@ pub(crate) fn snapshot_to_result(
|
|||
// status so callers don't treat an intentional kill as an
|
||||
// error. Matches the subagent "cancelled" status.
|
||||
"cancelled"
|
||||
} else if s.signal.as_deref() == Some("timeout") {
|
||||
// Wrapper-timeout kill (backend sets the sentinel "timeout"
|
||||
// signal); a generic "failed" would hide why the task died.
|
||||
"timed_out"
|
||||
} else if s.exit_code == Some(0) {
|
||||
"completed"
|
||||
} else {
|
||||
|
|
@ -154,6 +158,52 @@ mod tests {
|
|||
assert_ne!(result.status, "failed");
|
||||
}
|
||||
|
||||
/// A wrapper-timeout kill (backend sets the sentinel "timeout" signal,
|
||||
/// no exit code) must surface as "timed_out", not a generic "failed".
|
||||
#[test]
|
||||
fn test_snapshot_to_result_timeout_kill_is_timed_out() {
|
||||
let mut snapshot = make_test_snapshot("test-1", true, None);
|
||||
snapshot.signal = Some("timeout".to_string());
|
||||
let result = snapshot_to_result(snapshot, "read_file", DEFAULT_TOOL_OUTPUT_BYTES);
|
||||
|
||||
assert_eq!(result.status, "timed_out");
|
||||
assert_eq!(result.exit_code, None);
|
||||
}
|
||||
|
||||
/// An explicit kill wins over the timeout signal: the model asked for the
|
||||
/// kill, so it must see "cancelled" regardless of how the process died.
|
||||
#[test]
|
||||
fn test_snapshot_to_result_explicit_kill_beats_timeout_signal() {
|
||||
let mut snapshot = make_test_snapshot("test-1", true, None);
|
||||
snapshot.explicitly_killed = true;
|
||||
snapshot.signal = Some("timeout".to_string());
|
||||
let result = snapshot_to_result(snapshot, "read_file", DEFAULT_TOOL_OUTPUT_BYTES);
|
||||
|
||||
assert_eq!(result.status, "cancelled");
|
||||
}
|
||||
|
||||
/// Empty output renders "(no output yet)" only while running; terminal
|
||||
/// states must not imply more output may arrive.
|
||||
#[test]
|
||||
fn test_no_output_wording_tracks_task_state() {
|
||||
let mut running = make_test_snapshot("test-1", false, None);
|
||||
running.output = String::new();
|
||||
let result = snapshot_to_result(running, "read_file", DEFAULT_TOOL_OUTPUT_BYTES);
|
||||
let prompt = ToolOutput::TaskOutput(xai_tool_types::TaskOutputOutput::Result(result))
|
||||
.to_prompt_format();
|
||||
assert!(prompt.contains("(no output yet)"), "running: {prompt}");
|
||||
|
||||
let mut timed_out = make_test_snapshot("test-2", true, None);
|
||||
timed_out.output = String::new();
|
||||
timed_out.signal = Some("timeout".to_string());
|
||||
let result = snapshot_to_result(timed_out, "read_file", DEFAULT_TOOL_OUTPUT_BYTES);
|
||||
let prompt = ToolOutput::TaskOutput(xai_tool_types::TaskOutputOutput::Result(result))
|
||||
.to_prompt_format();
|
||||
assert!(prompt.contains("Status: timed_out"), "prompt: {prompt}");
|
||||
assert!(prompt.contains("(no output)"), "prompt: {prompt}");
|
||||
assert!(!prompt.contains("(no output yet)"), "prompt: {prompt}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_snapshot_to_result_truncates_large_output() {
|
||||
let mut snapshot = make_test_snapshot("test-1", true, Some(0));
|
||||
|
|
|
|||
|
|
@ -281,7 +281,8 @@ impl crate::types::tool_metadata::ToolMetadata for UseTool {
|
|||
fn description_template(&self) -> &str {
|
||||
"Call an MCP integration tool.\n\n\
|
||||
The `tool_name` must be the qualified `server__tool` name (e.g., `linear__save_issue`). \
|
||||
The `tool_input` must conform exactly to the input schema returned by `${{ tools.by_kind.search_tool }}`."
|
||||
The `tool_input` must conform exactly to the tool's input schema\
|
||||
${%- if tools.by_kind.search_tool %} as returned by `${{ tools.by_kind.search_tool }}`${%- endif %}."
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -299,7 +300,7 @@ impl xai_tool_runtime::Tool for UseTool {
|
|||
) -> xai_tool_types::ToolDescription {
|
||||
xai_tool_types::ToolDescription::new(
|
||||
"use_tool",
|
||||
crate::types::tool_metadata::ToolMetadata::description_template(self),
|
||||
crate::types::tool_metadata::ToolMetadata::sanitized_description_template(self),
|
||||
)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -529,6 +529,14 @@ pub struct ToolRegistryBuilder {
|
|||
tools: HashMap<String, ToolEntry>,
|
||||
reminders: Vec<ReminderEntry>,
|
||||
shared_local_registry: Option<xai_computer_hub_sdk::LocalRegistry>,
|
||||
/// Whether the client delivers system reminders (completion
|
||||
/// notifications for backgrounded commands/subagents) to the model.
|
||||
/// Exposed to description templates as `system_reminders_enabled` so
|
||||
/// "you are notified on completion" promises are only rendered when
|
||||
/// the client actually delivers them. Defaults to `true` (prod CLI
|
||||
/// behavior); the tools server sets it from
|
||||
/// `FinalizeToolServerConfigRequest.system_reminders_enabled`.
|
||||
system_reminders_enabled: bool,
|
||||
}
|
||||
impl Default for ToolRegistryBuilder {
|
||||
fn default() -> Self {
|
||||
|
|
@ -668,6 +676,7 @@ impl ToolRegistryBuilder {
|
|||
tools: HashMap::new(),
|
||||
reminders: Vec::new(),
|
||||
shared_local_registry: None,
|
||||
system_reminders_enabled: true,
|
||||
};
|
||||
b.register_with_params::<grok_build::BashTool, grok_build::bash::BashParams>();
|
||||
b.register_with_params::<grok_build::ReadFileTool, grok_build::read_file::ReadFileParams>();
|
||||
|
|
@ -925,6 +934,13 @@ impl ToolRegistryBuilder {
|
|||
}
|
||||
/// Validate and finalize into an immutable toolset.
|
||||
/// Consumes the builder — no further modifications possible.
|
||||
/// Set whether the client delivers system reminders to the model.
|
||||
/// Must be called before [`finalize`]; affects how description
|
||||
/// templates render notification promises (see
|
||||
/// `TemplateContext::system_reminders_enabled`).
|
||||
pub fn set_system_reminders_enabled(&mut self, enabled: bool) {
|
||||
self.system_reminders_enabled = enabled;
|
||||
}
|
||||
pub fn finalize(
|
||||
self,
|
||||
config: ToolServerConfig,
|
||||
|
|
@ -974,7 +990,8 @@ impl ToolRegistryBuilder {
|
|||
map.extend(overrides.iter().map(|(k, v)| (k.clone(), v.clone())));
|
||||
}
|
||||
}
|
||||
let renderer = TemplateRenderer::new(kind_to_name.clone(), kind_params.clone());
|
||||
let renderer = TemplateRenderer::new(kind_to_name.clone(), kind_params.clone())
|
||||
.with_system_reminders_enabled(self.system_reminders_enabled);
|
||||
let mut tools = Vec::new();
|
||||
let mut resources = Resources::new();
|
||||
resources.insert(crate::types::resources::Terminal(ctx.backend));
|
||||
|
|
@ -1002,6 +1019,9 @@ impl ToolRegistryBuilder {
|
|||
));
|
||||
{
|
||||
let mut mgr = crate::types::skill_discovery_tracker::SkillManager::new();
|
||||
mgr.set_discovery_snapshot_names(
|
||||
startup_skills.iter().map(|s| s.name.clone()).collect(),
|
||||
);
|
||||
mgr.seed(Some(cwd.clone()), None, startup_skills, None, None, None);
|
||||
let _ = mgr.take_pending();
|
||||
resources.insert(mgr);
|
||||
|
|
@ -2324,6 +2344,51 @@ mod tests {
|
|||
let toolset = builder
|
||||
.finalize(config, ctx)
|
||||
.expect("full toolset should finalize");
|
||||
fn assert_no_render_whitespace_artifacts(name: &str, text: &str) {
|
||||
let mut in_code_block = false;
|
||||
for line in text.lines() {
|
||||
let trimmed = line.trim_start();
|
||||
if trimmed.starts_with("```") {
|
||||
in_code_block = !in_code_block;
|
||||
continue;
|
||||
}
|
||||
if in_code_block {
|
||||
continue;
|
||||
}
|
||||
assert!(
|
||||
!trimmed.contains(" "),
|
||||
"{name}: double space (template render artifact?) in line: {line:?}"
|
||||
);
|
||||
assert!(
|
||||
!trimmed.contains(" , "),
|
||||
"{name}: stranded space before comma in line: {line:?}"
|
||||
);
|
||||
if let Some((_, roster)) = trimmed.split_once("access to:") {
|
||||
assert!(
|
||||
roster.starts_with(' '),
|
||||
"{name}: roster lost its separators (stripping guard?) in line: {line:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
fn collect_descriptions(v: &serde_json::Value, out: &mut Vec<String>) {
|
||||
match v {
|
||||
serde_json::Value::Object(map) => {
|
||||
if let Some(serde_json::Value::String(d)) = map.get("description") {
|
||||
out.push(d.clone());
|
||||
}
|
||||
for val in map.values() {
|
||||
collect_descriptions(val, out);
|
||||
}
|
||||
}
|
||||
serde_json::Value::Array(arr) => {
|
||||
for val in arr {
|
||||
collect_descriptions(val, out);
|
||||
}
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
for def in toolset.tool_definitions() {
|
||||
let name = &def.function.name;
|
||||
let desc = def.function.description.as_deref().unwrap_or_default();
|
||||
|
|
@ -2343,6 +2408,7 @@ mod tests {
|
|||
!desc.contains("the tool"),
|
||||
"{name}: empty tool name (missing conditional guard)"
|
||||
);
|
||||
assert_no_render_whitespace_artifacts(name, desc);
|
||||
let params_str = def.function.parameters.to_string();
|
||||
assert!(
|
||||
!params_str.contains("${{"),
|
||||
|
|
@ -2352,6 +2418,11 @@ mod tests {
|
|||
!params_str.contains("${%"),
|
||||
"{name}: unresolved jinja block in a field description"
|
||||
);
|
||||
let mut field_descs = Vec::new();
|
||||
collect_descriptions(&def.function.parameters, &mut field_descs);
|
||||
for field_desc in &field_descs {
|
||||
assert_no_render_whitespace_artifacts(name, field_desc);
|
||||
}
|
||||
}
|
||||
}
|
||||
/// Bash mode resolves the toolset's execute tool by kind, not a hardcoded
|
||||
|
|
@ -2532,6 +2603,74 @@ mod tests {
|
|||
"replace_all description should reference the renamed param: {replace_all_desc}"
|
||||
);
|
||||
}
|
||||
/// Descriptions promising completion notifications ("You are notified on
|
||||
/// completion") render conditionally on the client's system-reminders
|
||||
/// setting, plumbed via `set_system_reminders_enabled` into the
|
||||
/// `TemplateRenderer`. With reminders disabled, the bash description and
|
||||
/// the `is_background` field description must not promise notifications;
|
||||
/// they point at the get-output tool instead when one is served.
|
||||
#[tokio::test]
|
||||
async fn bash_descriptions_track_system_reminders_setting() {
|
||||
let config_with = |ids: &[&str]| ToolServerConfig {
|
||||
tools: ids
|
||||
.iter()
|
||||
.map(|id| ToolConfig::from_id((*id).to_string()))
|
||||
.collect(),
|
||||
behavior_preset: None,
|
||||
};
|
||||
let bash_texts = |toolset: &FinalizedToolset| {
|
||||
let defs = toolset.tool_definitions();
|
||||
let bash = defs
|
||||
.iter()
|
||||
.find(|d| d.function.name == "run_terminal_cmd")
|
||||
.expect("run_terminal_cmd definition not found")
|
||||
.clone();
|
||||
let desc = bash.function.description.clone().unwrap_or_default();
|
||||
let field_desc = bash.function.parameters["properties"]["is_background"]["description"]
|
||||
.as_str()
|
||||
.unwrap_or_default()
|
||||
.to_string();
|
||||
(desc, field_desc)
|
||||
};
|
||||
let ids = [
|
||||
"GrokBuild:run_terminal_cmd",
|
||||
"GrokBuild:get_task_output",
|
||||
"GrokBuild:kill_task",
|
||||
];
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let toolset = ToolRegistryBuilder::new()
|
||||
.finalize(config_with(&ids), test_session_context(&tmp))
|
||||
.expect("finalize");
|
||||
let (desc, field_desc) = bash_texts(&toolset);
|
||||
assert!(
|
||||
desc.contains("You are notified on completion"),
|
||||
"reminders on: description should promise notification: {desc}"
|
||||
);
|
||||
assert!(
|
||||
field_desc.contains("you are notified on completion"),
|
||||
"reminders on: is_background description should promise notification: {field_desc}"
|
||||
);
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let mut builder = ToolRegistryBuilder::new();
|
||||
builder.set_system_reminders_enabled(false);
|
||||
let toolset = builder
|
||||
.finalize(config_with(&ids), test_session_context(&tmp))
|
||||
.expect("finalize");
|
||||
let (desc, field_desc) = bash_texts(&toolset);
|
||||
assert!(
|
||||
!desc.contains("notified on completion"),
|
||||
"reminders off: description must not promise notification: {desc}"
|
||||
);
|
||||
assert!(
|
||||
desc.contains("Check on it later with the get_task_output tool"),
|
||||
"reminders off: description should point at get_task_output: {desc}"
|
||||
);
|
||||
assert!(
|
||||
!field_desc.contains("notified on completion")
|
||||
&& field_desc.contains("check on it later with the get_task_output tool"),
|
||||
"reminders off: is_background description should point at get_task_output: {field_desc}"
|
||||
);
|
||||
}
|
||||
/// Each assertion pattern-matches on the exact `ToolOutput::SearchReplace`
|
||||
/// variant so the test fails if the renderer silently returns empty strings
|
||||
/// or the tool returns the wrong variant.
|
||||
|
|
@ -2626,7 +2765,8 @@ mod tests {
|
|||
ToolOutput::SearchReplace(SearchReplaceOutput::NoMatchesFound(e)) => {
|
||||
assert_eq!(
|
||||
e.message,
|
||||
"The string to replace was not found in the file, use the read_file tool to see the correct string.",
|
||||
"The string to replace was not found in the file, use the read_file tool to see the correct string. \
|
||||
The user may have changed the file since you last read it.",
|
||||
);
|
||||
}
|
||||
other => panic!("Expected SearchReplace(NoMatchesFound), got: {other:?}"),
|
||||
|
|
|
|||
|
|
@ -259,7 +259,7 @@ pub fn format_monitor_events(
|
|||
task_output_name: Option<&str>,
|
||||
) -> Option<String> {
|
||||
use std::fmt::Write as _;
|
||||
let tool_hint = task_output_name.unwrap_or("get_command_or_subagent_output");
|
||||
let tool_hint = task_output_name.unwrap_or("get_task_output");
|
||||
match events {
|
||||
[] => None,
|
||||
[event] => {
|
||||
|
|
@ -1934,7 +1934,7 @@ mod tests {
|
|||
assert!(
|
||||
batched.starts_with(
|
||||
"3 monitor events from 2 monitors \
|
||||
(use get_command_or_subagent_output to identify each monitor):"
|
||||
(use get_task_output to identify each monitor):"
|
||||
),
|
||||
"batch must lead with event + monitor counts and default tool hint: {batched}"
|
||||
);
|
||||
|
|
|
|||
|
|
@ -702,6 +702,22 @@ impl ToolOutput {
|
|||
pub fn to_prompt_format(&self) -> String {
|
||||
match self {
|
||||
ToolOutput::ReadFile(read_file_output) => match read_file_output {
|
||||
ReadFileOutput::FileContent(file_content) if file_content.content.is_empty() => {
|
||||
if file_content.total_lines == 0 {
|
||||
"File is empty.".to_string()
|
||||
} else if file_content
|
||||
.offset
|
||||
.is_some_and(|offset| offset > file_content.total_lines)
|
||||
{
|
||||
format!(
|
||||
"(no lines returned: the requested window is past the end of the \
|
||||
file; the file has {} lines)",
|
||||
file_content.total_lines
|
||||
)
|
||||
} else {
|
||||
"(no lines returned)".to_string()
|
||||
}
|
||||
}
|
||||
ReadFileOutput::FileContent(file_content) => file_content.content.clone(),
|
||||
ReadFileOutput::ImageContent(image_content) => {
|
||||
format!(
|
||||
|
|
@ -812,7 +828,11 @@ impl ToolOutput {
|
|||
lines.push(String::new());
|
||||
lines.push("=== Output ===".to_string());
|
||||
if r.output.is_empty() {
|
||||
lines.push("(no output yet)".to_string());
|
||||
if r.status == "running" {
|
||||
lines.push("(no output yet)".to_string());
|
||||
} else {
|
||||
lines.push("(no output)".to_string());
|
||||
}
|
||||
} else {
|
||||
lines.push(r.output.clone());
|
||||
}
|
||||
|
|
@ -1283,6 +1303,65 @@ mod tests {
|
|||
fn to_json(output: ToolOutput) -> serde_json::Value {
|
||||
serde_json::to_value(&output).unwrap()
|
||||
}
|
||||
fn empty_file_content(offset: Option<usize>, total_lines: usize) -> FileContent {
|
||||
FileContent {
|
||||
content: String::new(),
|
||||
content_concise: None,
|
||||
absolute_path: PathBuf::from("/tmp/f.txt"),
|
||||
offset,
|
||||
limit: None,
|
||||
raw_output: String::new(),
|
||||
total_lines,
|
||||
extracted_images: vec![],
|
||||
}
|
||||
}
|
||||
/// An empty file must render an explicit notice, not a blank result.
|
||||
#[test]
|
||||
fn read_empty_file_prompt_says_file_is_empty() {
|
||||
let output = ToolOutput::ReadFile(ReadFileOutput::FileContent(empty_file_content(None, 0)));
|
||||
assert_eq!(output.to_prompt_format(), "File is empty.");
|
||||
}
|
||||
/// An offset beyond the last line must say past-EOF and report the real
|
||||
/// line count.
|
||||
#[test]
|
||||
fn read_past_eof_prompt_reports_line_count() {
|
||||
let output = ToolOutput::ReadFile(ReadFileOutput::FileContent(empty_file_content(
|
||||
Some(101),
|
||||
100,
|
||||
)));
|
||||
let prompt = output.to_prompt_format();
|
||||
assert!(
|
||||
prompt.contains("past the end of the file"),
|
||||
"expected past-EOF notice, got: {prompt}"
|
||||
);
|
||||
assert!(
|
||||
prompt.contains("100 lines"),
|
||||
"expected real line count, got: {prompt}"
|
||||
);
|
||||
}
|
||||
/// An empty window with an in-range offset (e.g. `limit: 0`) must render
|
||||
/// the generic notice, not a bogus past-EOF claim.
|
||||
#[test]
|
||||
fn read_empty_window_in_range_offset_is_not_past_eof() {
|
||||
let output = ToolOutput::ReadFile(ReadFileOutput::FileContent(empty_file_content(
|
||||
Some(5),
|
||||
100,
|
||||
)));
|
||||
let prompt = output.to_prompt_format();
|
||||
assert_eq!(prompt, "(no lines returned)");
|
||||
assert!(
|
||||
!prompt.contains("past the end of the file"),
|
||||
"in-range empty window must not claim past-EOF: {prompt}"
|
||||
);
|
||||
}
|
||||
/// Non-empty content renders unchanged.
|
||||
#[test]
|
||||
fn read_non_empty_content_renders_verbatim() {
|
||||
let mut fc = empty_file_content(None, 3);
|
||||
fc.content = "1→a\nb\nc".to_string();
|
||||
let output = ToolOutput::ReadFile(ReadFileOutput::FileContent(fc));
|
||||
assert_eq!(output.to_prompt_format(), "1→a\nb\nc");
|
||||
}
|
||||
#[test]
|
||||
fn text_output_to_prompt_format_omits_consumed_completion_task_id() {
|
||||
let output = ToolOutput::Text(TextOutput {
|
||||
|
|
@ -1576,6 +1655,58 @@ mod tests {
|
|||
);
|
||||
}
|
||||
#[test]
|
||||
fn apply_patch_parse_error_json() {
|
||||
let json = to_json(ApplyPatchOutput::ParseError("Invalid patch: boom".into()).into());
|
||||
assert_eq!(
|
||||
json,
|
||||
json!({"type": "ApplyPatch", "ParseError": "Invalid patch: boom"})
|
||||
);
|
||||
}
|
||||
#[test]
|
||||
fn apply_patch_application_error_json() {
|
||||
let json = to_json(
|
||||
ApplyPatchOutput::ApplicationError("File /tmp/x.rs does not exist".into()).into(),
|
||||
);
|
||||
assert_eq!(
|
||||
json,
|
||||
json!({"type": "ApplyPatch", "ApplicationError": "File /tmp/x.rs does not exist"})
|
||||
);
|
||||
}
|
||||
#[test]
|
||||
fn apply_patch_empty_patch_json() {
|
||||
let json = to_json(ApplyPatchOutput::EmptyPatch("No files were modified.".into()).into());
|
||||
assert_eq!(
|
||||
json,
|
||||
json!({"type": "ApplyPatch", "EmptyPatch": "No files were modified."})
|
||||
);
|
||||
}
|
||||
/// `Success` must not serialize under any of the keys Python treats as a
|
||||
/// failure, otherwise a successful patch would be taxed as a tool error.
|
||||
#[test]
|
||||
fn apply_patch_success_json_is_not_an_error_shape() {
|
||||
let json = to_json(
|
||||
ApplyPatchOutput::Success {
|
||||
files: vec![ApplyPatchFileResult {
|
||||
path: PathBuf::from("/repo/src/main.rs"),
|
||||
action: "modified".into(),
|
||||
old_text: Some("old".into()),
|
||||
new_text: "new".into(),
|
||||
move_to: None,
|
||||
}],
|
||||
tool_output_for_prompt: "Updated /repo/src/main.rs".into(),
|
||||
}
|
||||
.into(),
|
||||
);
|
||||
assert_eq!(json["type"], "ApplyPatch");
|
||||
assert!(json.get("Success").is_some(), "missing Success key: {json}");
|
||||
for key in ["ParseError", "ApplicationError", "EmptyPatch"] {
|
||||
assert!(
|
||||
json.get(key).is_none(),
|
||||
"success must not serialize under the error key {key}: {json}"
|
||||
);
|
||||
}
|
||||
}
|
||||
#[test]
|
||||
fn kill_task_result_json() {
|
||||
let json = to_json(
|
||||
KillTaskOutput::Result(KillTaskResult {
|
||||
|
|
|
|||
|
|
@ -141,6 +141,11 @@ pub struct SkillManager {
|
|||
/// `paths:`-gated skills held back from the listing until a matching file
|
||||
/// is touched, plus their activation state. See [`ConditionalSkills`].
|
||||
conditional: ConditionalSkills,
|
||||
|
||||
/// Every skill name from session-start discovery, set once by
|
||||
/// `ToolRegistryBuilder::finalize` from the unfiltered
|
||||
/// `SessionContext.skills`. The `paths:` gate never applies here.
|
||||
discovery_snapshot_names: Vec<String>,
|
||||
}
|
||||
|
||||
/// Canonicalize a skill path, falling back to the raw path for not-yet-created
|
||||
|
|
@ -469,6 +474,19 @@ impl SkillManager {
|
|||
dedupe_by_canonical_path_and_name(&self.discovered_skills, &self.startup_skills)
|
||||
}
|
||||
|
||||
/// Set the full-discovery snapshot (see `discovery_snapshot_names`).
|
||||
/// Write-once by design: the only caller is `ToolRegistryBuilder::finalize`.
|
||||
pub(crate) fn set_discovery_snapshot_names(&mut self, names: Vec<String>) {
|
||||
self.discovery_snapshot_names = names;
|
||||
}
|
||||
|
||||
/// Every skill name from session-start discovery. Set only by
|
||||
/// `ToolRegistryBuilder::finalize`; empty for a manager seeded
|
||||
/// without it.
|
||||
pub fn discovery_snapshot_names(&self) -> &[String] {
|
||||
&self.discovery_snapshot_names
|
||||
}
|
||||
|
||||
/// Render the canonical listing for the entire current skill set, for
|
||||
/// `/context` accounting. Leaves announce state untouched.
|
||||
///
|
||||
|
|
|
|||
|
|
@ -64,6 +64,12 @@ struct TemplateContext {
|
|||
/// true everywhere else. Tool descriptions branch on this to swap
|
||||
/// Unix-centric guidance for PowerShell-aware guidance.
|
||||
has_unix_utilities: bool,
|
||||
/// Whether the client delivers system reminders (e.g. completion
|
||||
/// notifications for backgrounded commands/subagents) to the model.
|
||||
/// Descriptions that promise "you are notified on completion" branch
|
||||
/// on this so the promise is only made when it can be kept. Defaults
|
||||
/// to `true` (prod CLI behavior).
|
||||
system_reminders_enabled: bool,
|
||||
}
|
||||
|
||||
/// Shared render implementation: fast-path check + MiniJinja render.
|
||||
|
|
@ -117,6 +123,59 @@ impl std::error::Error for TemplateRenderError {
|
|||
|
||||
/// Pre-built template renderer stored in Resources.
|
||||
///
|
||||
/// Strip unrendered template markers (`${{ … }}` and `${% … %}`) from `raw`.
|
||||
///
|
||||
/// Last-resort fallback for when MiniJinja rendering fails: the model must
|
||||
/// never see raw template syntax, so the offending spans are dropped
|
||||
/// entirely. The result may read slightly awkwardly around the removed
|
||||
/// spans, but it is always plain prose.
|
||||
/// Render-failure fallback: strip template markers so the model never sees
|
||||
/// raw syntax. A render failure always indicates a template bug, so debug
|
||||
/// builds (tests, local dev) panic loudly instead of degrading; release
|
||||
/// builds log and degrade gracefully.
|
||||
///
|
||||
/// Not for the *intentional* pre-finalize sanitize path
|
||||
/// (`sanitized_description_template`), which calls
|
||||
/// [`strip_template_markers`] directly.
|
||||
#[track_caller]
|
||||
pub fn strip_markers_on_render_failure(raw: &str, err: &TemplateRenderError) -> String {
|
||||
debug_assert!(
|
||||
false,
|
||||
"description template failed to render — fix the template instead of \
|
||||
relying on the marker-strip fallback: {err}"
|
||||
);
|
||||
tracing::warn!("Description template render failed, stripping markers: {err}");
|
||||
strip_template_markers(raw)
|
||||
}
|
||||
|
||||
pub fn strip_template_markers(raw: &str) -> String {
|
||||
let mut out = String::with_capacity(raw.len());
|
||||
let mut rest = raw;
|
||||
while let Some(start) = rest.find("${") {
|
||||
let after = &rest[start + 2..];
|
||||
let close: Option<usize> = if after.starts_with('{') {
|
||||
after.find("}}").map(|i| i + 2)
|
||||
} else if after.starts_with('%') {
|
||||
after.find("%}").map(|i| i + 2)
|
||||
} else {
|
||||
None
|
||||
};
|
||||
match close {
|
||||
Some(end_in_after) => {
|
||||
out.push_str(&rest[..start]);
|
||||
rest = &rest[start + 2 + end_in_after..];
|
||||
}
|
||||
None => {
|
||||
// `${` that is not a marker (or unterminated) — keep as-is.
|
||||
out.push_str(&rest[..start + 2]);
|
||||
rest = &rest[start + 2..];
|
||||
}
|
||||
}
|
||||
}
|
||||
out.push_str(rest);
|
||||
out
|
||||
}
|
||||
|
||||
/// Created once at finalize time and available to all tools and reminders
|
||||
/// via `resources.get::<TemplateRenderer>()`. Uses MiniJinja with custom
|
||||
/// `${{ }}` / `${% %}` delimiters to avoid collisions with literal `{{ }}`
|
||||
|
|
@ -158,10 +217,20 @@ impl TemplateRenderer {
|
|||
// comparison is naturally false there — no cfg guard needed.
|
||||
shell_uses_semicolon: xai_grok_config::shell::chain_separator() == ";",
|
||||
has_unix_utilities: xai_grok_config::shell::has_unix_utilities(),
|
||||
system_reminders_enabled: true,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
/// Override whether templates see `system_reminders_enabled` as true.
|
||||
/// Called at finalize time with the client's setting; every other
|
||||
/// construction site keeps the default (`true`).
|
||||
#[must_use]
|
||||
pub fn with_system_reminders_enabled(mut self, enabled: bool) -> Self {
|
||||
self.ctx.system_reminders_enabled = enabled;
|
||||
self
|
||||
}
|
||||
|
||||
/// Render a template string with the full context.
|
||||
///
|
||||
/// Template syntax:
|
||||
|
|
@ -193,8 +262,8 @@ impl TemplateRenderer {
|
|||
/// `$defs`. Property keys are remapped separately; this resolves
|
||||
/// descriptions that reference another param/tool via
|
||||
/// `${{ params.<kind>.<param> }}` or `${{ tools.by_kind.<kind> }}`.
|
||||
/// Untemplated descriptions are left as-is; a render failure logs and leaves
|
||||
/// the raw description in place.
|
||||
/// Untemplated descriptions are left as-is; a render failure logs and
|
||||
/// strips the template markers so raw syntax never reaches the model.
|
||||
pub fn render_schema_descriptions(&self, schema: &mut serde_json::Value) {
|
||||
match schema {
|
||||
serde_json::Value::Object(map) => {
|
||||
|
|
@ -204,12 +273,7 @@ impl TemplateRenderer {
|
|||
{
|
||||
match self.render(desc) {
|
||||
Ok(r) => Some(r),
|
||||
Err(e) => {
|
||||
tracing::warn!(
|
||||
"schema description template render failed, leaving raw: {e}"
|
||||
);
|
||||
None
|
||||
}
|
||||
Err(e) => Some(strip_markers_on_render_failure(desc, &e)),
|
||||
}
|
||||
}
|
||||
_ => None,
|
||||
|
|
@ -341,6 +405,21 @@ impl std::fmt::Debug for TemplateRenderer {
|
|||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn strip_template_markers_removes_interpolations_and_tags() {
|
||||
let raw = "Use ${{ tools.by_kind.read }} first.${%- if tools.by_kind.edit %} Then ${{ tools.by_kind.edit }}.${%- endif %}";
|
||||
assert_eq!(strip_template_markers(raw), "Use first. Then .");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn strip_template_markers_keeps_plain_text_and_literal_dollar_brace() {
|
||||
assert_eq!(strip_template_markers("no markers here"), "no markers here");
|
||||
// `${` not followed by `{`/`%` is preserved (e.g. shell `${VAR}`).
|
||||
assert_eq!(strip_template_markers("echo ${VAR}"), "echo ${VAR}");
|
||||
// Unterminated marker is preserved rather than eating the rest.
|
||||
assert_eq!(strip_template_markers("broken ${{ tail"), "broken ${{ tail");
|
||||
}
|
||||
|
||||
fn make_renderer(
|
||||
tools: &[(ToolKind, &str)],
|
||||
params: &[(ToolKind, &[(&str, &str)])],
|
||||
|
|
@ -538,6 +617,18 @@ mod tests {
|
|||
assert_eq!(result, "OK");
|
||||
}
|
||||
|
||||
/// Documents MiniJinja's default (lenient) undefined behavior: a missing
|
||||
/// kind in *output* position renders as an empty string with `Ok`, it
|
||||
/// does NOT return `Err`. Callers that want a fallback name for a
|
||||
/// missing kind must check for an empty result — `.unwrap_or_else` on
|
||||
/// the `Result` alone never fires for this case.
|
||||
#[test]
|
||||
fn render_missing_kind_output_position_is_empty_ok() {
|
||||
let r = make_renderer(&[], &[]);
|
||||
let result = r.render("${{ tools.by_kind.background_task_action }}");
|
||||
assert_eq!(result.unwrap(), "");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn render_no_markers_fast_path() {
|
||||
let r = make_renderer(&[], &[]);
|
||||
|
|
|
|||
|
|
@ -72,6 +72,17 @@ pub trait ToolMetadata: Send + Sync {
|
|||
Expr::True
|
||||
}
|
||||
|
||||
/// Model-safe fallback description for `xai_tool_runtime::Tool::description()`
|
||||
/// implementations: the raw template with all `${{ … }}` / `${% … %}`
|
||||
/// markers stripped.
|
||||
///
|
||||
/// The registry path (`versioned_definition`) renders templates properly
|
||||
/// with the finalized toolset context; this is only for consumers that
|
||||
/// bypass the registry, which must never see raw template syntax.
|
||||
fn sanitized_description_template(&self) -> String {
|
||||
crate::types::template_renderer::strip_template_markers(self.description_template())
|
||||
}
|
||||
|
||||
/// Build the tool definition for a given contract version.
|
||||
///
|
||||
/// Default: renders `description_template()` via the `TemplateRenderer`
|
||||
|
|
@ -90,8 +101,7 @@ pub trait ToolMetadata: Send + Sync {
|
|||
) -> ToolDefinition {
|
||||
let raw_desc = description_override.unwrap_or_else(|| self.description_template());
|
||||
let description = renderer.render(raw_desc).unwrap_or_else(|e| {
|
||||
tracing::warn!("Description template render failed, using raw: {e}");
|
||||
raw_desc.to_string()
|
||||
crate::types::template_renderer::strip_markers_on_render_failure(raw_desc, &e)
|
||||
});
|
||||
let remapped_schema = if param_map.is_empty() {
|
||||
input_schema.clone()
|
||||
|
|
|
|||
Loading…
Reference in a new issue