hive-runtime: record a new ACP session only once its first prompt is answered
The session id was written to the session file right after session/new, but the system prompt rides on the first prompt only. If that prompt failed, the next turn (or the next harness start) resumed the recorded session as not-new and the system prompt never reached it. The id is now written after the first session/prompt gets its reply. A failed first prompt leaves nothing recorded, so the next turn starts a new session and sends the system prompt again. Chosen over a separate "system prompt delivered" marker: one file, and "recorded" already means "usable". Tests drive AcpRuntime against a scripted sh agent that fails the first prompt: in-process and across a restart, the retry is a new session carrying the system prompt. Also: PermissionPolicy now sees a PermissionAsk (kind plus the MCP server the tool belongs to, matched by name against the servers handed to the session), so a caller can tell MCP tool calls from other `other` requests. Refs #4391
This commit is contained in:
parent
1b24edf4b4
commit
a2ab40cc69
6 changed files with 240 additions and 61 deletions
|
|
@ -12,7 +12,8 @@ use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader};
|
|||
use tokio::process::{Child, ChildStdin, Command};
|
||||
use tokio::sync::{mpsc, oneshot};
|
||||
|
||||
use super::{AcpError, PermissionPolicy};
|
||||
use super::stream::mcp_server_of;
|
||||
use super::{AcpError, PermissionAsk, PermissionPolicy};
|
||||
use crate::spec::AcpCommand;
|
||||
|
||||
/// Something the agent sent that is not a response to one of our requests.
|
||||
|
|
@ -42,6 +43,7 @@ impl Connection {
|
|||
command: &AcpCommand,
|
||||
cwd: &Path,
|
||||
permit: PermissionPolicy,
|
||||
servers: Vec<String>,
|
||||
) -> Result<Self, AcpError> {
|
||||
let mut child = Command::new(&command.command)
|
||||
.args(&command.args)
|
||||
|
|
@ -80,6 +82,7 @@ impl Connection {
|
|||
pending: pending.clone(),
|
||||
tx,
|
||||
permit,
|
||||
servers,
|
||||
};
|
||||
tokio::spawn(async move {
|
||||
let mut lines = BufReader::new(stdout).lines();
|
||||
|
|
@ -151,6 +154,7 @@ struct Reader {
|
|||
pending: Pending,
|
||||
tx: mpsc::UnboundedSender<Incoming>,
|
||||
permit: PermissionPolicy,
|
||||
servers: Vec<String>,
|
||||
}
|
||||
|
||||
impl Reader {
|
||||
|
|
@ -169,7 +173,7 @@ impl Reader {
|
|||
let reply = match method {
|
||||
"session/request_permission" => json!({
|
||||
"jsonrpc": "2.0", "id": id,
|
||||
"result": permission_outcome(&message["params"], &self.permit),
|
||||
"result": permission_outcome(&message["params"], &self.permit, &self.servers),
|
||||
}),
|
||||
_ => json!({
|
||||
"jsonrpc": "2.0", "id": id,
|
||||
|
|
@ -225,15 +229,21 @@ async fn write(stdin: &tokio::sync::Mutex<ChildStdin>, message: &Value) -> Resul
|
|||
}
|
||||
|
||||
/// The `session/request_permission` result: the agent's first option of the
|
||||
/// matching kind, allow if `permit` accepts the tool call's kind and reject
|
||||
/// otherwise. With no such option the request is answered `cancelled`.
|
||||
pub(super) fn permission_outcome(params: &Value, permit: &PermissionPolicy) -> Value {
|
||||
let kind = params
|
||||
.get("toolCall")
|
||||
.and_then(|t| t.get("kind"))
|
||||
.and_then(Value::as_str)
|
||||
.unwrap_or("other");
|
||||
let wanted = if permit(kind) {
|
||||
/// matching kind, allow if `permit` accepts the request and reject otherwise.
|
||||
/// With no such option the request is answered `cancelled`. `servers` are the
|
||||
/// MCP servers handed to the session, matched against the tool call's title.
|
||||
pub(super) fn permission_outcome(
|
||||
params: &Value,
|
||||
permit: &PermissionPolicy,
|
||||
servers: &[String],
|
||||
) -> Value {
|
||||
let tool_call = params.get("toolCall");
|
||||
let field = |k: &str| tool_call.and_then(|t| t.get(k)).and_then(Value::as_str);
|
||||
let ask = PermissionAsk {
|
||||
kind: field("kind").unwrap_or("other"),
|
||||
mcp_server: field("title").and_then(|title| mcp_server_of(title, servers)),
|
||||
};
|
||||
let wanted = if permit(&ask) {
|
||||
["allow_once", "allow_always"]
|
||||
} else {
|
||||
["reject_once", "reject_always"]
|
||||
|
|
@ -257,14 +267,14 @@ pub(super) fn permission_outcome(params: &Value, permit: &PermissionPolicy) -> V
|
|||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::permission_outcome;
|
||||
use crate::acp::PermissionPolicy;
|
||||
use crate::acp::{PermissionAsk, PermissionPolicy};
|
||||
use serde_json::json;
|
||||
use std::sync::Arc;
|
||||
|
||||
fn request(kind: &str) -> serde_json::Value {
|
||||
fn request(kind: &str, title: &str) -> serde_json::Value {
|
||||
json!({
|
||||
"sessionId": "s",
|
||||
"toolCall": { "toolCallId": "t", "kind": kind },
|
||||
"toolCall": { "toolCallId": "t", "kind": kind, "title": title },
|
||||
"options": [
|
||||
{ "optionId": "once", "kind": "allow_once", "name": "Allow once" },
|
||||
{ "optionId": "always", "kind": "allow_always", "name": "Always allow" },
|
||||
|
|
@ -273,34 +283,54 @@ mod tests {
|
|||
})
|
||||
}
|
||||
|
||||
fn no_execute() -> PermissionPolicy {
|
||||
Arc::new(|kind: &str| kind != "execute")
|
||||
fn servers() -> Vec<String> {
|
||||
vec!["hyperhive".to_owned()]
|
||||
}
|
||||
|
||||
/// Allows `edit` and MCP tools, nothing else.
|
||||
fn edit_or_mcp() -> PermissionPolicy {
|
||||
Arc::new(|ask: &PermissionAsk<'_>| ask.kind == "edit" || ask.mcp_server.is_some())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_permitted_kind_is_allowed_once() {
|
||||
fn a_permitted_request_is_allowed_once() {
|
||||
assert_eq!(
|
||||
permission_outcome(&request("edit"), &no_execute()),
|
||||
permission_outcome(&request("edit", "edit"), &edit_or_mcp(), &servers()),
|
||||
json!({ "outcome": { "outcome": "selected", "optionId": "once" } })
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_refused_kind_is_rejected() {
|
||||
fn a_refused_request_is_rejected() {
|
||||
assert_eq!(
|
||||
permission_outcome(&request("execute"), &no_execute()),
|
||||
permission_outcome(&request("execute", "bash"), &edit_or_mcp(), &servers()),
|
||||
json!({ "outcome": { "outcome": "selected", "optionId": "reject" } })
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_missing_kind_is_judged_as_other() {
|
||||
let policy: PermissionPolicy = Arc::new(|kind: &str| kind == "other");
|
||||
let mut req = request("x");
|
||||
fn the_policy_sees_which_mcp_server_a_tool_belongs_to() {
|
||||
let seen = Arc::new(std::sync::Mutex::new(Vec::new()));
|
||||
let record = seen.clone();
|
||||
let policy: PermissionPolicy = Arc::new(move |ask: &PermissionAsk<'_>| {
|
||||
record
|
||||
.lock()
|
||||
.unwrap()
|
||||
.push((ask.kind.to_owned(), ask.mcp_server.map(str::to_owned)));
|
||||
true
|
||||
});
|
||||
permission_outcome(&request("other", "hyperhive_send"), &policy, &servers());
|
||||
permission_outcome(&request("other", "skill"), &policy, &servers());
|
||||
let mut req = request("x", "read");
|
||||
req["toolCall"].as_object_mut().unwrap().remove("kind");
|
||||
permission_outcome(&req, &policy, &servers());
|
||||
assert_eq!(
|
||||
permission_outcome(&req, &policy)["outcome"]["optionId"],
|
||||
"once"
|
||||
*seen.lock().unwrap(),
|
||||
vec![
|
||||
("other".to_owned(), Some("hyperhive".to_owned())),
|
||||
("other".to_owned(), None),
|
||||
("other".to_owned(), None),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -309,7 +339,7 @@ mod tests {
|
|||
let req = json!({ "toolCall": { "kind": "execute" },
|
||||
"options": [{ "optionId": "once", "kind": "allow_once" }] });
|
||||
assert_eq!(
|
||||
permission_outcome(&req, &no_execute()),
|
||||
permission_outcome(&req, &edit_or_mcp(), &servers()),
|
||||
json!({ "outcome": { "outcome": "cancelled" } })
|
||||
);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue