subagent: give each run its own signal URL, and drop the name argument
`goal_reached`/`need_help` took the session name as a tool argument, so identity was an assertion by the caller and the only guard on it was `occupancy()` — "does that name have a turn in flight", which two concurrently running siblings both satisfy for each other. A subagent could stop its sibling's run by naming it. Identity moves into the URL. Each spawned run is minted an unguessable token (`Uuid::new_v4`, the OS CSPRNG), the URL carrying it goes into that one subagent's own `--mcp-config`, and the route resolves it back to a session before dispatching to a handler bound to that session. Neither tool takes a `name` any more: a subagent has no field in which to name a sibling, and a sibling's name — which a brief may well mention — is not a token. One route with a path parameter, not a route per session: the `Router` is built once at startup and subagents come and go for the daemon's whole life. An unminted or revoked token gets a bare 404, the same answer either way, so nothing enumerates. A run's token is revoked when the run ends (`finish_turn`) or when a call never reached a spawn. Two things fall out of that: - the config file becomes one per session. A single shared path was already a race between two `start`s; with a per-session URL in it, the loser would read the winner's identity. - `occupancy()` stops being the identity guard and is gone from the signal path entirely rather than kept "just in case" — a revoked token can't reach it, and it never answered the question it was standing in for. It still backs `status`, which is what it was always actually for. Refs #4403 Refs #4413
This commit is contained in:
parent
b5fc17aa59
commit
34129d776c
11 changed files with 575 additions and 180 deletions
|
|
@ -3,13 +3,28 @@
|
|||
//! socket.
|
||||
//!
|
||||
//! Two surfaces, two routes. `/mcp` is the parent's: the four tools above.
|
||||
//! `/signal/mcp` is the *subagent's*, and carries `goal_reached` and
|
||||
//! `/signal/mcp/<token>` is the *subagent's*, and carries `goal_reached` and
|
||||
//! `need_help` only — it is what a subagent is handed in its own
|
||||
//! `--mcp-config` (see [`crate::mcp_config`]), so the ability to report on
|
||||
//! its own run can't be the ability to spawn a nested one.
|
||||
//!
|
||||
//! **The token in that path is the subagent's identity, and neither signal
|
||||
//! tool takes a session name.** One route is registered, once, at startup
|
||||
//! (sessions come and go far faster than a `Router` can be rebuilt); every
|
||||
//! request resolves its token to a session and dispatches to a handler bound
|
||||
//! to *that* session, so the name a signal lands against is never a value the
|
||||
//! sender chose. A token that resolves to nothing — never minted, or revoked
|
||||
//! when its run ended — is a bare 404, which is also why the route says
|
||||
//! nothing that would let a subagent enumerate its siblings.
|
||||
|
||||
use std::sync::Arc;
|
||||
use std::collections::HashMap;
|
||||
use std::sync::{Arc, Mutex, PoisonError};
|
||||
|
||||
use axum::extract::{Path, Request, State as RouteState};
|
||||
use axum::response::{IntoResponse as _, Response};
|
||||
use rmcp::transport::streamable_http_server::{
|
||||
StreamableHttpServerConfig, StreamableHttpService, session::local::LocalSessionManager,
|
||||
};
|
||||
use rmcp::{
|
||||
ServerHandler,
|
||||
handler::server::wrapper::Parameters,
|
||||
|
|
@ -92,11 +107,11 @@ fn default_trigger() -> String {
|
|||
"Carry out the task described in your instructions.".to_owned()
|
||||
}
|
||||
|
||||
/// No session name, deliberately: which session this signal is about is
|
||||
/// decided by the URL it arrives on (see this module's doc), so there is no
|
||||
/// field here in which one subagent could name another.
|
||||
#[derive(Debug, Deserialize, JsonSchema)]
|
||||
struct GoalReachedArgs {
|
||||
/// Your own session name — the one your brief and your continuation
|
||||
/// prompts address you by.
|
||||
name: String,
|
||||
/// Optionally, what you did. It's shown to whoever spawned you.
|
||||
#[serde(default)]
|
||||
msg: Option<String>,
|
||||
|
|
@ -106,11 +121,9 @@ struct GoalReachedArgs {
|
|||
report_file: Option<String>,
|
||||
}
|
||||
|
||||
/// No session name here either — same reason as [`GoalReachedArgs`].
|
||||
#[derive(Debug, Deserialize, JsonSchema)]
|
||||
struct NeedHelpArgs {
|
||||
/// Your own session name — the one your brief and your continuation
|
||||
/// prompts address you by.
|
||||
name: String,
|
||||
/// What is blocking you, specifically enough for someone else to act on
|
||||
/// it. This is the whole content of the signal, which is why it's
|
||||
/// required.
|
||||
|
|
@ -280,9 +293,15 @@ impl ServerHandler for SubagentMcp {}
|
|||
/// Separate handler rather than two more tools on [`SubagentMcp`] so the
|
||||
/// split is structural — there is no route a subagent holds that `start` is
|
||||
/// reachable from.
|
||||
///
|
||||
/// `session` is the whole point of the type: an instance is built only by
|
||||
/// `signal_route`, from a token that had already resolved, and both tools
|
||||
/// read the session they act on out of `self`. Nothing in an incoming
|
||||
/// payload can reach that field.
|
||||
#[derive(Clone)]
|
||||
struct SubagentSignalMcp {
|
||||
state: Arc<State>,
|
||||
session: String,
|
||||
}
|
||||
|
||||
#[tool_router]
|
||||
|
|
@ -290,76 +309,148 @@ impl SubagentSignalMcp {
|
|||
#[tool(
|
||||
description = "Report that you have reached the goal you were given. Stops the harness \
|
||||
from starting another turn to re-prompt you toward it, and extends the \"subagent \
|
||||
done\" message your parent gets with what you say here. This records a claim, not a \
|
||||
result: whoever spawned you reads the diff and the gate output regardless, so \
|
||||
calling it does not make unfinished work finished. Call it when the goal is actually \
|
||||
met — otherwise keep working, or call `need_help` if you can't proceed."
|
||||
done\" message your parent gets with what you say here. It always applies to your \
|
||||
own session and takes no session name — the endpoint you are calling it on is yours \
|
||||
alone, so there is nothing to address it to. This records a claim, not a result: \
|
||||
whoever spawned you reads the diff and the gate output regardless, so calling it \
|
||||
does not make unfinished work finished. Call it when the goal is actually met — \
|
||||
otherwise keep working, or call `need_help` if you can't proceed."
|
||||
)]
|
||||
fn goal_reached(&self, Parameters(args): Parameters<GoalReachedArgs>) -> String {
|
||||
match session::goal_reached(
|
||||
session::goal_reached(
|
||||
&self.state,
|
||||
&args.name,
|
||||
&self.session,
|
||||
args.msg,
|
||||
args.report_file.as_deref(),
|
||||
) {
|
||||
Ok(msg) => msg,
|
||||
Err(e) => format!("goal_reached error: {e:#}"),
|
||||
}
|
||||
)
|
||||
}
|
||||
|
||||
#[tool(
|
||||
description = "Report that you cannot proceed, and why. Stops the harness from starting \
|
||||
another turn to re-prompt you toward your goal, marks your session as blocked so \
|
||||
whoever spawned you sees it in `status` without reading any file, and extends the \
|
||||
\"subagent done\" message with your reason. Use it for a genuine block — a missing \
|
||||
\"subagent done\" message with your reason. Like `goal_reached` it takes no session \
|
||||
name and always applies to your own session. Use it for a genuine block — a missing \
|
||||
credential, a decision that isn't yours, an instruction that contradicts what you \
|
||||
found — not for work that is merely hard. Say enough that someone else can act on it."
|
||||
)]
|
||||
fn need_help(&self, Parameters(args): Parameters<NeedHelpArgs>) -> String {
|
||||
match session::need_help(
|
||||
session::need_help(
|
||||
&self.state,
|
||||
&args.name,
|
||||
&self.session,
|
||||
args.msg,
|
||||
args.report_file.as_deref(),
|
||||
) {
|
||||
Ok(msg) => msg,
|
||||
Err(e) => format!("need_help error: {e:#}"),
|
||||
}
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
#[tool_handler]
|
||||
impl ServerHandler for SubagentSignalMcp {}
|
||||
|
||||
/// Path the subagent-facing signal surface is served at, and the tail of the
|
||||
/// URL [`crate::session::State`] hands to every subagent it spawns. Kept
|
||||
/// here, next to the route that answers it, so the two can't drift.
|
||||
/// Prefix the subagent-facing signal surface is served under, and the part
|
||||
/// of a subagent's URL that is *not* secret: every real endpoint is this plus
|
||||
/// that session's own token. Kept here, next to the route that answers it, so
|
||||
/// the two can't drift. Nothing is served at the bare prefix.
|
||||
pub const SIGNAL_PATH: &str = "/signal/mcp";
|
||||
|
||||
/// Everything the one registered signal route needs to answer a request for
|
||||
/// any session: the daemon state the token resolves against, and a session
|
||||
/// manager per live token.
|
||||
///
|
||||
/// **Per token, not one shared manager.** rmcp keeps an MCP session (created
|
||||
/// by `initialize`, named by an `mcp-session-id` header) inside its manager,
|
||||
/// and a manager shared across tokens would make that header a second way to
|
||||
/// pick which bound handler serves a request — an id is unguessable, but this
|
||||
/// is the one property the route exists to make structural rather than
|
||||
/// probabilistic. With a manager per token, an id minted under one token
|
||||
/// simply does not exist under another.
|
||||
#[derive(Clone)]
|
||||
struct SignalRouting {
|
||||
state: Arc<State>,
|
||||
managers: Arc<Mutex<HashMap<String, Arc<LocalSessionManager>>>>,
|
||||
}
|
||||
|
||||
impl SignalRouting {
|
||||
/// The session manager for `token`, created on its first request.
|
||||
///
|
||||
/// Entries for tokens that no longer resolve are dropped on the way
|
||||
/// past — the map would otherwise grow for the daemon's whole life, and
|
||||
/// a revoked token's MCP sessions have nothing left to serve. It's a
|
||||
/// linear scan of a map with one entry per *live* subagent, on a request
|
||||
/// a subagent makes a handful of times per run.
|
||||
fn manager_for(&self, token: &str) -> Arc<LocalSessionManager> {
|
||||
let mut managers = self.managers.lock().unwrap_or_else(PoisonError::into_inner);
|
||||
managers.retain(|t, _| self.state.session_for_signal_token(t).is_some());
|
||||
Arc::clone(
|
||||
managers
|
||||
.entry(token.to_owned())
|
||||
.or_insert_with(session_manager),
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
/// A session manager with this daemon's keep-alive: a subagent turn can run
|
||||
/// considerably longer than a bash command — same 24h rationale as the bash
|
||||
/// and matrix daemons.
|
||||
fn session_manager() -> Arc<LocalSessionManager> {
|
||||
let mut manager = LocalSessionManager::default();
|
||||
manager.session_config.keep_alive = Some(std::time::Duration::from_hours(24));
|
||||
Arc::new(manager)
|
||||
}
|
||||
|
||||
/// Resolve the request's token to a session and serve it with a handler bound
|
||||
/// to that session — or 404, for a token this daemon never minted or has
|
||||
/// since revoked.
|
||||
///
|
||||
/// The `StreamableHttpService` is built per request, which is cheap (a few
|
||||
/// `Arc`s) and is what lets the bound session come from the URL: rmcp's
|
||||
/// service factory takes no request, so the binding has to happen outside it.
|
||||
/// Only a request that starts an MCP session ever calls the factory; the rest
|
||||
/// are answered from the token's own manager, against the handler built when
|
||||
/// that session was initialized.
|
||||
async fn signal_route(
|
||||
RouteState(routing): RouteState<SignalRouting>,
|
||||
Path(token): Path<String>,
|
||||
request: Request,
|
||||
) -> Response {
|
||||
let Some(session) = routing.state.session_for_signal_token(&token) else {
|
||||
tracing::debug!("signal route: unknown or expired token, refusing with 404");
|
||||
return axum::http::StatusCode::NOT_FOUND.into_response();
|
||||
};
|
||||
let manager = routing.manager_for(&token);
|
||||
let state = Arc::clone(&routing.state);
|
||||
let service = StreamableHttpService::new(
|
||||
move || {
|
||||
Ok(SubagentSignalMcp {
|
||||
state: Arc::clone(&state),
|
||||
session: session.clone(),
|
||||
})
|
||||
},
|
||||
manager,
|
||||
StreamableHttpServerConfig::default(),
|
||||
);
|
||||
service.handle(request).await.into_response()
|
||||
}
|
||||
|
||||
/// Run the MCP server over HTTP (rmcp streamable-http transport) on `addr`.
|
||||
/// Loopback-only bind, one long-lived session — same shape as the bash and
|
||||
/// matrix daemons' own `serve_http`.
|
||||
///
|
||||
/// Two routes off one listener: `/mcp` for the parent's four tools, and
|
||||
/// [`SIGNAL_PATH`] for the subagent's two. Separate session managers because
|
||||
/// they're separate MCP servers to separate clients — the parent's harness
|
||||
/// on one, each subagent's own claude on the other.
|
||||
/// `<SIGNAL_PATH>/{token}` for the subagent's two. Separate session managers
|
||||
/// because they're separate MCP servers to separate clients — the parent's
|
||||
/// harness on one, each subagent's own claude on its own token's.
|
||||
///
|
||||
/// The signal route is registered **once**, with the token as a path
|
||||
/// parameter, because this `Router` is built at startup and subagents come
|
||||
/// and go for the daemon's whole life: there is no mounting a fresh route per
|
||||
/// session. Resolution therefore happens per request, in `signal_route`.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns an error if the listener cannot bind `addr` or the HTTP server
|
||||
/// exits with a fatal error.
|
||||
pub async fn serve_http(addr: std::net::SocketAddr, state: Arc<State>) -> anyhow::Result<()> {
|
||||
use rmcp::transport::streamable_http_server::{
|
||||
StreamableHttpServerConfig, StreamableHttpService, session::local::LocalSessionManager,
|
||||
};
|
||||
// A subagent turn can run considerably longer than a bash command —
|
||||
// same 24h keep-alive rationale as the bash/matrix daemons.
|
||||
let manager = || {
|
||||
let mut session_manager = LocalSessionManager::default();
|
||||
session_manager.session_config.keep_alive = Some(std::time::Duration::from_hours(24));
|
||||
std::sync::Arc::new(session_manager)
|
||||
};
|
||||
let parent_state = Arc::clone(&state);
|
||||
let service = StreamableHttpService::new(
|
||||
move || {
|
||||
|
|
@ -367,21 +458,20 @@ pub async fn serve_http(addr: std::net::SocketAddr, state: Arc<State>) -> anyhow
|
|||
state: Arc::clone(&parent_state),
|
||||
})
|
||||
},
|
||||
manager(),
|
||||
StreamableHttpServerConfig::default(),
|
||||
);
|
||||
let signal_service = StreamableHttpService::new(
|
||||
move || {
|
||||
Ok(SubagentSignalMcp {
|
||||
state: Arc::clone(&state),
|
||||
})
|
||||
},
|
||||
manager(),
|
||||
session_manager(),
|
||||
StreamableHttpServerConfig::default(),
|
||||
);
|
||||
let routing = SignalRouting {
|
||||
state,
|
||||
managers: Arc::new(Mutex::new(HashMap::new())),
|
||||
};
|
||||
let app = axum::Router::new()
|
||||
.nest_service("/mcp", service)
|
||||
.nest_service(SIGNAL_PATH, signal_service);
|
||||
.route(
|
||||
&format!("{SIGNAL_PATH}/{{token}}"),
|
||||
axum::routing::any(signal_route),
|
||||
)
|
||||
.with_state(routing);
|
||||
let listener = tokio::net::TcpListener::bind(addr).await?;
|
||||
tracing::info!(
|
||||
%addr,
|
||||
|
|
@ -391,3 +481,104 @@ pub async fn serve_http(addr: std::net::SocketAddr, state: Arc<State>) -> anyhow
|
|||
axum::serve(listener, app).await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// The state a route test resolves against, with `name` already minted a
|
||||
/// token — the same shape `start` leaves behind.
|
||||
fn state() -> Arc<State> {
|
||||
Arc::new(State::new(
|
||||
std::path::PathBuf::from("/dev/null"),
|
||||
format!("http://127.0.0.1:1{SIGNAL_PATH}"),
|
||||
))
|
||||
}
|
||||
|
||||
fn routing(state: Arc<State>) -> SignalRouting {
|
||||
SignalRouting {
|
||||
state,
|
||||
managers: Arc::new(Mutex::new(HashMap::new())),
|
||||
}
|
||||
}
|
||||
|
||||
/// Mint `name` a signal URL the way a spawning run does, and hand back
|
||||
/// just the token — what the route would have taken out of the path.
|
||||
fn minted(state: &State, name: &str) -> String {
|
||||
let url = state.mint_signal_url(name);
|
||||
url.rsplit('/')
|
||||
.next()
|
||||
.expect("a minted URL always has a last segment")
|
||||
.to_owned()
|
||||
}
|
||||
|
||||
fn post(uri: &str) -> Request {
|
||||
axum::http::Request::builder()
|
||||
.method("POST")
|
||||
.uri(uri)
|
||||
.body(axum::body::Body::empty())
|
||||
.expect("a well-formed test request")
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_signal_tools_take_no_session_name() {
|
||||
// The fix itself, pinned: a subagent has no field in which to name a
|
||||
// sibling. Asserted against the generated JSON schema rather than the
|
||||
// struct, because the schema is what actually reaches the subagent —
|
||||
// a `name` re-added under any serde rename would show up here.
|
||||
for (tool, schema) in [
|
||||
("goal_reached", schemars::schema_for!(GoalReachedArgs)),
|
||||
("need_help", schemars::schema_for!(NeedHelpArgs)),
|
||||
] {
|
||||
let schema = serde_json::to_value(&schema).expect("a schema serializes");
|
||||
let properties = schema
|
||||
.get("properties")
|
||||
.and_then(serde_json::Value::as_object)
|
||||
.cloned()
|
||||
.unwrap_or_default();
|
||||
assert!(
|
||||
!properties.contains_key("name"),
|
||||
"`{tool}` must not take a session name — identity comes from the route's token, \
|
||||
and an argument for it is an argument one subagent can fill in with another's \
|
||||
name. Properties offered: {:?}",
|
||||
properties.keys().collect::<Vec<_>>()
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn an_unminted_signal_token_is_a_bare_404() {
|
||||
// No enumeration: a token this daemon never issued gets the same
|
||||
// answer whether or not a session by any name exists.
|
||||
let state = state();
|
||||
minted(&state, "live");
|
||||
let response = signal_route(
|
||||
RouteState(routing(state)),
|
||||
Path("not-a-token".to_owned()),
|
||||
post("/signal/mcp/not-a-token"),
|
||||
)
|
||||
.await;
|
||||
assert_eq!(response.status(), axum::http::StatusCode::NOT_FOUND);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn a_minted_signal_token_reaches_the_surface_at_all() {
|
||||
// The other half of the 404 above: it is the *resolution* refusing,
|
||||
// not the route being unreachable. The body here is not a valid MCP
|
||||
// request, so rmcp rejects it — on its own terms, with some answer
|
||||
// that is not this route's "no such token".
|
||||
let state = state();
|
||||
let token = minted(&state, "live");
|
||||
let response = signal_route(
|
||||
RouteState(routing(state)),
|
||||
Path(token.clone()),
|
||||
post(&format!("/signal/mcp/{token}")),
|
||||
)
|
||||
.await;
|
||||
assert_ne!(
|
||||
response.status(),
|
||||
axum::http::StatusCode::NOT_FOUND,
|
||||
"a live session's own token must resolve"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue