feat(#1653): optional name param for bash run tool
This commit is contained in:
parent
0b6a52e81f
commit
9bd51a7440
5 changed files with 119 additions and 8 deletions
|
|
@ -9,7 +9,7 @@ invocation regardless of tool groups.
|
||||||
|
|
||||||
## Tools
|
## Tools
|
||||||
|
|
||||||
### `run(cmd, timeout_secs?, wait_seconds?)`
|
### `run(cmd, timeout_secs?, wait_seconds?, name?)`
|
||||||
|
|
||||||
Submit a shell command for background execution (`sh -c <cmd>`).
|
Submit a shell command for background execution (`sh -c <cmd>`).
|
||||||
Stdout and stderr stream to `harness/bash-tasks/<id>.{out,err}`.
|
Stdout and stderr stream to `harness/bash-tasks/<id>.{out,err}`.
|
||||||
|
|
@ -25,6 +25,14 @@ harness fires a wake with `from: "bash-task-<id>"` and the exit code
|
||||||
the task keeps running and the normal `task started: id=<id>`
|
the task keeps running and the normal `task started: id=<id>`
|
||||||
response is returned. **Defaults to 3** — pass `wait_seconds: 0`
|
response is returned. **Defaults to 3** — pass `wait_seconds: 0`
|
||||||
to disable inline waiting and always get the immediate response.
|
to disable inline waiting and always get the immediate response.
|
||||||
|
- `name` — optional caller-chosen task id. When set it replaces the
|
||||||
|
auto-generated hex id, so it surfaces in the wake `from`
|
||||||
|
(`bash-task-<name>`), in `status(<name>)` lookups, and in the
|
||||||
|
loose-ends list — a memorable label instead of an opaque id. A name
|
||||||
|
is **reusable once its previous task has finished**; submitting a
|
||||||
|
name whose task is still `pending`/`running` is rejected. Allowed
|
||||||
|
characters: ASCII letters, digits, `.`, `_`, `-` (max 64). Omit for
|
||||||
|
the auto-generated id.
|
||||||
|
|
||||||
Exposed as `mcp__bash__run`.
|
Exposed as `mcp__bash__run`.
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -166,6 +166,15 @@ struct BashRunArgs {
|
||||||
/// `0` to disable inline waiting and always get the immediate response.
|
/// `0` to disable inline waiting and always get the immediate response.
|
||||||
#[serde(default = "default_wait")]
|
#[serde(default = "default_wait")]
|
||||||
wait_seconds: Option<u64>,
|
wait_seconds: Option<u64>,
|
||||||
|
/// Optional task name. When set it becomes the task id, so it appears
|
||||||
|
/// in the completion wake (`from: "bash-task-<name>"`), in `status`
|
||||||
|
/// lookups, and in the loose-ends list — handy for recognising a task
|
||||||
|
/// later instead of an opaque hex id. A name can be reused once its
|
||||||
|
/// previous task has finished; reusing a name whose task is still
|
||||||
|
/// running is rejected. Allowed chars: ASCII letters, digits, `.`,
|
||||||
|
/// `_`, `-` (max 64). Omit to get the auto-generated id.
|
||||||
|
#[serde(default)]
|
||||||
|
name: Option<String>,
|
||||||
}
|
}
|
||||||
|
|
||||||
#[allow(
|
#[allow(
|
||||||
|
|
@ -204,13 +213,17 @@ impl BashMcp {
|
||||||
wake is fired; when the timeout expires the task keeps running and the normal \
|
wake is fired; when the timeout expires the task keeps running and the normal \
|
||||||
`task started: id=<id>` response is returned. `wait_seconds` defaults to 3; \
|
`task started: id=<id>` response is returned. `wait_seconds` defaults to 3; \
|
||||||
pass `wait_seconds: 0` to disable inline waiting and always get the immediate \
|
pass `wait_seconds: 0` to disable inline waiting and always get the immediate \
|
||||||
response."
|
response. Pass `name` to label the task with a memorable id (used in the wake \
|
||||||
|
`from`, `status` lookups, and the loose-ends list) instead of an opaque hex id; \
|
||||||
|
a name is reusable once its prior task has finished, and rejected while one is \
|
||||||
|
still running."
|
||||||
)]
|
)]
|
||||||
async fn run(&self, Parameters(args): Parameters<BashRunArgs>) -> String {
|
async fn run(&self, Parameters(args): Parameters<BashRunArgs>) -> String {
|
||||||
let req = DaemonRequest::BashRun {
|
let req = DaemonRequest::BashRun {
|
||||||
cmd: args.cmd,
|
cmd: args.cmd,
|
||||||
timeout_secs: args.timeout_secs,
|
timeout_secs: args.timeout_secs,
|
||||||
wait_seconds: args.wait_seconds,
|
wait_seconds: args.wait_seconds,
|
||||||
|
name: args.name,
|
||||||
};
|
};
|
||||||
let resp = round_trip(req).await;
|
let resp = round_trip(req).await;
|
||||||
// Extract the id from the response to format the result.
|
// Extract the id from the response to format the result.
|
||||||
|
|
|
||||||
|
|
@ -70,6 +70,13 @@ pub enum DaemonRequest {
|
||||||
/// task-started-id response immediately.
|
/// task-started-id response immediately.
|
||||||
#[serde(default)]
|
#[serde(default)]
|
||||||
wait_seconds: Option<u64>,
|
wait_seconds: Option<u64>,
|
||||||
|
/// Optional caller-chosen task name, used as the task id (so it
|
||||||
|
/// flows into the wake `from`, status lookups, and the loose-ends
|
||||||
|
/// summary). Must be filesystem-safe. Reusable once any prior task
|
||||||
|
/// of the same name has finished; rejected while one is still
|
||||||
|
/// running. `None` falls back to the auto-generated id.
|
||||||
|
#[serde(default)]
|
||||||
|
name: Option<String>,
|
||||||
},
|
},
|
||||||
|
|
||||||
/// Query the current status of a task. Returns the full `TaskFile`
|
/// Query the current status of a task. Returns the full `TaskFile`
|
||||||
|
|
|
||||||
|
|
@ -20,7 +20,7 @@ use std::sync::atomic::{AtomicU64, Ordering};
|
||||||
use std::sync::{Arc, Mutex};
|
use std::sync::{Arc, Mutex};
|
||||||
use std::time::{Duration, SystemTime, UNIX_EPOCH};
|
use std::time::{Duration, SystemTime, UNIX_EPOCH};
|
||||||
|
|
||||||
use anyhow::Result;
|
use anyhow::{Result, bail};
|
||||||
use tokio::io::AsyncWriteExt as _;
|
use tokio::io::AsyncWriteExt as _;
|
||||||
|
|
||||||
use crate::paths;
|
use crate::paths;
|
||||||
|
|
@ -122,15 +122,62 @@ fn refresh_loose_ends() {
|
||||||
// Public API used by daemon dispatch
|
// Public API used by daemon dispatch
|
||||||
// ---------------------------------------------------------------------------
|
// ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
/// Longest accepted caller-chosen task name.
|
||||||
|
const MAX_TASK_NAME_LEN: usize = 64;
|
||||||
|
|
||||||
|
/// Validate a caller-chosen task name. The name doubles as the task id
|
||||||
|
/// and therefore as the `<name>.json` filename, so it must be a single
|
||||||
|
/// safe path segment. Allows ASCII alphanumerics plus `.`, `_`, `-`;
|
||||||
|
/// rejects empties, over-long names, `.`/`..`, and anything that could
|
||||||
|
/// escape the tasks dir or collide with the `.json.tmp` scratch suffix.
|
||||||
|
fn validate_task_name(name: &str) -> Result<()> {
|
||||||
|
if name.is_empty() {
|
||||||
|
bail!("task name must not be empty");
|
||||||
|
}
|
||||||
|
if name.len() > MAX_TASK_NAME_LEN {
|
||||||
|
bail!("task name too long (max {MAX_TASK_NAME_LEN} chars)");
|
||||||
|
}
|
||||||
|
if name == "." || name == ".." {
|
||||||
|
bail!("task name {name:?} is reserved");
|
||||||
|
}
|
||||||
|
if !name
|
||||||
|
.chars()
|
||||||
|
.all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-'))
|
||||||
|
{
|
||||||
|
bail!("task name {name:?} may only contain ASCII letters, digits, '.', '_', '-'");
|
||||||
|
}
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
/// Submit a new pending task. Returns the task ID.
|
/// Submit a new pending task. Returns the task ID.
|
||||||
///
|
///
|
||||||
|
/// When `name` is `Some`, it is validated and used as the task id (so it
|
||||||
|
/// surfaces in the wake `from`, status lookups, and the loose-ends list).
|
||||||
|
/// A name may be reused once any prior task of that name has finished;
|
||||||
|
/// submitting a name whose task is still `Pending`/`Running` is rejected.
|
||||||
|
/// `None` falls back to the auto-generated timestamp id.
|
||||||
|
///
|
||||||
/// # Errors
|
/// # Errors
|
||||||
///
|
///
|
||||||
/// Returns an error if the tasks directory cannot be created or the
|
/// Returns an error if the name is invalid, a task of that name is still
|
||||||
/// task file cannot be written.
|
/// running, the tasks directory cannot be created, or the task file
|
||||||
pub fn submit_task(cmd: String, timeout_secs: Option<u64>) -> Result<String> {
|
/// cannot be written.
|
||||||
|
pub fn submit_task(cmd: String, timeout_secs: Option<u64>, name: Option<String>) -> Result<String> {
|
||||||
std::fs::create_dir_all(paths::tasks_dir())?;
|
std::fs::create_dir_all(paths::tasks_dir())?;
|
||||||
let id = new_task_id();
|
let id = match name {
|
||||||
|
Some(name) => {
|
||||||
|
validate_task_name(&name)?;
|
||||||
|
if let Some(existing) = read_task(&name)
|
||||||
|
&& matches!(existing.status, TaskStatus::Pending | TaskStatus::Running)
|
||||||
|
{
|
||||||
|
bail!(
|
||||||
|
"a bash task named `{name}` is already running — wait for it to finish or pick another name"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
name
|
||||||
|
}
|
||||||
|
None => new_task_id(),
|
||||||
|
};
|
||||||
let task = TaskFile {
|
let task = TaskFile {
|
||||||
id: id.clone(),
|
id: id.clone(),
|
||||||
cmd,
|
cmd,
|
||||||
|
|
@ -506,3 +553,38 @@ pub(crate) async fn send_wake(
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
mod tests {
|
||||||
|
use super::{MAX_TASK_NAME_LEN, validate_task_name};
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn accepts_reasonable_names() {
|
||||||
|
for ok in ["build", "ci-check", "nix_flake.check", "t1", "A.B-C_9"] {
|
||||||
|
assert!(validate_task_name(ok).is_ok(), "{ok} should be valid");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn rejects_unsafe_names() {
|
||||||
|
// Empty, traversal, separators, control/space, and over-long.
|
||||||
|
for bad in [
|
||||||
|
"",
|
||||||
|
".",
|
||||||
|
"..",
|
||||||
|
"a/b",
|
||||||
|
"../escape",
|
||||||
|
"has space",
|
||||||
|
"tab\tname",
|
||||||
|
"slash\\back",
|
||||||
|
] {
|
||||||
|
assert!(
|
||||||
|
validate_task_name(bad).is_err(),
|
||||||
|
"{bad:?} should be rejected"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
assert!(validate_task_name(&"x".repeat(MAX_TASK_NAME_LEN + 1)).is_err());
|
||||||
|
// Exactly at the cap is allowed.
|
||||||
|
assert!(validate_task_name(&"x".repeat(MAX_TASK_NAME_LEN)).is_ok());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -61,8 +61,9 @@ async fn dispatch(req: DaemonRequest) -> DaemonResponse {
|
||||||
cmd,
|
cmd,
|
||||||
timeout_secs,
|
timeout_secs,
|
||||||
wait_seconds,
|
wait_seconds,
|
||||||
|
name,
|
||||||
} => {
|
} => {
|
||||||
let id = match runner::submit_task(cmd, timeout_secs) {
|
let id = match runner::submit_task(cmd, timeout_secs, name) {
|
||||||
Ok(id) => id,
|
Ok(id) => id,
|
||||||
Err(e) => return DaemonResponse::error(format!("submit_task: {e:#}")),
|
Err(e) => return DaemonResponse::error(format!("submit_task: {e:#}")),
|
||||||
};
|
};
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue