agent conversation fix
This commit is contained in:
@ -8,17 +8,16 @@
|
||||
//! use (`orchestrator_watcher.rs`), so we assert MCP behaviour against a real
|
||||
//! [`OrchestratorService`] without any I/O:
|
||||
//!
|
||||
//! 1. `tools/list` advertises the six `idea_*` tools, each with an input schema.
|
||||
//! 1. `tools/list` advertises the MCP `idea_*` tools, each with an input schema.
|
||||
//! 2. `tools/call` maps every tool to the right `OrchestratorCommand` (observed
|
||||
//! through the fakes: spawn creates an agent, stop closes the session, …).
|
||||
//! 3. `idea_ask_agent` returns the target's `reply` **inline**.
|
||||
//! 4. `idea_list_agents` returns the agent list **inline** as a JSON array.
|
||||
//! 5. A failed IdeA command ⇒ tool result `isError: true` (not a transport error,
|
||||
//! 3. `idea_list_agents` returns the agent list **inline** as a JSON array.
|
||||
//! 4. A failed IdeA command ⇒ tool result `isError: true` (not a transport error,
|
||||
//! not a panic); the server stays alive for the next call.
|
||||
//! 6. Malformed JSON-RPC ⇒ a typed `PARSE_ERROR`, never a panic.
|
||||
//! 7. Unknown method / unknown tool ⇒ typed errors, never a panic.
|
||||
//! 8. `initialize` answers the minimal handshake.
|
||||
//! 9. A `tools/call` missing a required argument is rejected by the **same**
|
||||
//! 5. Malformed JSON-RPC ⇒ a typed `PARSE_ERROR`, never a panic.
|
||||
//! 6. Unknown method / unknown tool ⇒ typed errors, never a panic.
|
||||
//! 7. `initialize` answers the minimal handshake.
|
||||
//! 8. A `tools/call` missing a required argument is rejected by the **same**
|
||||
//! `OrchestratorRequest::validate` (typed error, **no** dispatch).
|
||||
|
||||
use std::collections::HashMap;
|
||||
@ -52,7 +51,7 @@ use application::{
|
||||
CloseTerminal, CreateAgentFromScratch, CreateSkill, LaunchAgent, ListAgents,
|
||||
OrchestratorService, TerminalSessions, UpdateAgentContext,
|
||||
};
|
||||
use infrastructure::orchestrator::mcp::jsonrpc::{error_codes, Transport, TransportError};
|
||||
use infrastructure::orchestrator::mcp::jsonrpc::error_codes;
|
||||
use infrastructure::{
|
||||
InMemoryConversationRegistry, InMemoryMailbox, McpServer, MediatedInbox, MemoryTransport,
|
||||
SystemMillisClock,
|
||||
@ -421,20 +420,6 @@ fn build_service_with_mailbox(
|
||||
(Arc::new(service), mailbox, sessions)
|
||||
}
|
||||
|
||||
/// Pre-seeds a live PTY terminal session for `agent_id` so `ask_agent` reuses it.
|
||||
fn seed_live_pty(sessions: &TerminalSessions, agent_id: AgentId, session_id: SessionId) {
|
||||
sessions.insert(
|
||||
PtyHandle { session_id },
|
||||
TerminalSession::starting(
|
||||
session_id,
|
||||
NodeId::from_uuid(Uuid::from_u128(7)),
|
||||
ProjectPath::new("/home/me/proj").unwrap(),
|
||||
SessionKind::Agent { agent_id },
|
||||
PtySize { rows: 24, cols: 80 },
|
||||
),
|
||||
);
|
||||
}
|
||||
|
||||
fn server(service: Arc<OrchestratorService>) -> McpServer {
|
||||
McpServer::new(service, project())
|
||||
}
|
||||
@ -479,7 +464,7 @@ fn result_text(result: &Value) -> &str {
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
#[tokio::test]
|
||||
async fn tools_list_advertises_the_seven_idea_tools_with_schemas() {
|
||||
async fn tools_list_advertises_the_idea_tools_with_schemas() {
|
||||
let (service, _s) = build_service(FakeContexts::new());
|
||||
let server = server(service);
|
||||
|
||||
@ -501,7 +486,6 @@ async fn tools_list_advertises_the_seven_idea_tools_with_schemas() {
|
||||
for expected in [
|
||||
"idea_list_agents",
|
||||
"idea_ask_agent",
|
||||
"idea_reply",
|
||||
"idea_launch_agent",
|
||||
"idea_stop_agent",
|
||||
"idea_update_context",
|
||||
@ -522,10 +506,11 @@ async fn tools_list_advertises_the_seven_idea_tools_with_schemas() {
|
||||
"missing tool {expected}; got {names:?}"
|
||||
);
|
||||
}
|
||||
assert!(!names.contains(&"idea_reply"));
|
||||
assert_eq!(
|
||||
tools.len(),
|
||||
14,
|
||||
"exactly the fourteen idea_* tools (7 base + idea_skill_read + 4 FileGuard C7 + 2 live-state LS4); got {names:?}"
|
||||
13,
|
||||
"exactly the thirteen exposed idea_* tools; got {names:?}"
|
||||
);
|
||||
|
||||
// Every tool advertises an object input schema.
|
||||
@ -634,76 +619,32 @@ async fn update_context_and_create_skill_calls_succeed() {
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// 3. idea_ask_agent returns the reply inline
|
||||
// 3. Inter-agent delegation is exposed through MCP; reply protocol is not
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
#[tokio::test]
|
||||
async fn ask_agent_returns_target_reply_inline() {
|
||||
// Option 1 (B-3/B-4): `idea_ask_agent` writes the task into the target's live
|
||||
// terminal and blocks on the mailbox; the target's `idea_reply` (carrying its
|
||||
// handshake identity as requester) resolves it, and the ask returns the result
|
||||
// inline. We drive both over the MCP server, the ask on a spawned task.
|
||||
async fn ask_agent_tool_reaches_shared_validation_before_dispatch() {
|
||||
let contexts = FakeContexts::new();
|
||||
let agent_id = contexts.seed_agent("architect");
|
||||
let (service, mailbox, sessions) = build_service_with_mailbox(contexts);
|
||||
seed_live_pty(
|
||||
&sessions,
|
||||
agent_id,
|
||||
SessionId::from_uuid(Uuid::from_u128(4242)),
|
||||
);
|
||||
contexts.seed_agent("architect");
|
||||
let (service, _mailbox, _sessions) = build_service_with_mailbox(contexts);
|
||||
let server = server(service);
|
||||
|
||||
let ask_server = server(Arc::clone(&service));
|
||||
let ask = tokio::spawn(async move {
|
||||
let raw = tools_call(
|
||||
7,
|
||||
"idea_ask_agent",
|
||||
json!({ "target": "architect", "task": "What is the answer?" }),
|
||||
);
|
||||
ask_server.handle_raw(&raw).await.expect("reply owed")
|
||||
});
|
||||
|
||||
// Wait until the ask has enqueued its ticket (it is now blocked awaiting a reply).
|
||||
tokio::time::timeout(std::time::Duration::from_secs(10), async {
|
||||
while mailbox.pending(&agent_id) == 0 {
|
||||
tokio::task::yield_now().await;
|
||||
}
|
||||
})
|
||||
.await
|
||||
.expect("ask must enqueue a ticket");
|
||||
|
||||
// The target replies via idea_reply — its identity comes from the handshake
|
||||
// requester, which `for_requester` injects as the `from` of the Reply command.
|
||||
let reply_server = server(Arc::clone(&service)).for_requester(agent_id.to_string());
|
||||
let reply_raw = tools_call(8, "idea_reply", json!({ "result": "the answer is 42" }));
|
||||
let reply_resp = reply_server
|
||||
.handle_raw(&reply_raw)
|
||||
.await
|
||||
.expect("reply owed");
|
||||
assert_eq!(reply_resp.result.expect("result")["isError"], json!(false));
|
||||
|
||||
let response = tokio::time::timeout(std::time::Duration::from_secs(10), ask)
|
||||
.await
|
||||
.expect("ask completes after reply")
|
||||
.expect("join ok");
|
||||
assert_eq!(response.id, json!(7), "id must be echoed");
|
||||
let raw = tools_call(7, "idea_ask_agent", json!({ "target": "architect" }));
|
||||
let response = server.handle_raw(&raw).await.expect("reply owed");
|
||||
let error = response.error.expect("validation error expected");
|
||||
assert_eq!(error.code, error_codes::INVALID_PARAMS);
|
||||
assert!(
|
||||
response.error.is_none(),
|
||||
"transport error: {:?}",
|
||||
response.error
|
||||
error.message.contains("task"),
|
||||
"shared validation should reject the missing task, got {error:?}"
|
||||
);
|
||||
let result = response.result.expect("result");
|
||||
assert_eq!(result["isError"], json!(false), "got {result}");
|
||||
assert_eq!(
|
||||
result_text(&result),
|
||||
"the answer is 42",
|
||||
"ask reply must be returned inline; got {result}"
|
||||
assert!(
|
||||
response.result.is_none(),
|
||||
"no dispatch on validation failure"
|
||||
);
|
||||
}
|
||||
|
||||
/// `idea_reply` with no in-flight ask for the connected peer ⇒ the IdeA command
|
||||
/// fails, surfaced as a tool execution error (`isError: true`), never a panic.
|
||||
#[tokio::test]
|
||||
async fn reply_without_pending_ask_is_a_tool_error() {
|
||||
async fn reply_tool_is_not_exposed_over_mcp() {
|
||||
let contexts = FakeContexts::new();
|
||||
let agent_id = contexts.seed_agent("architect");
|
||||
let (service, _mailbox, _sessions) = build_service_with_mailbox(contexts);
|
||||
@ -711,9 +652,9 @@ async fn reply_without_pending_ask_is_a_tool_error() {
|
||||
|
||||
let raw = tools_call(9, "idea_reply", json!({ "result": "orphan" }));
|
||||
let resp = reply_server.handle_raw(&raw).await.expect("reply owed");
|
||||
// Mapped + dispatched, but the command failed ⇒ tool error, transport intact.
|
||||
assert!(resp.error.is_none(), "no transport error: {:?}", resp.error);
|
||||
assert_eq!(resp.result.expect("result")["isError"], json!(true));
|
||||
let error = resp.error.expect("unknown tool error expected");
|
||||
assert_eq!(error.code, error_codes::METHOD_NOT_FOUND);
|
||||
assert!(resp.result.is_none());
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
@ -911,22 +852,18 @@ async fn initialize_fires_ready_sink_with_requester() {
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
#[tokio::test]
|
||||
async fn ask_agent_missing_task_is_rejected_by_shared_validation_no_dispatch() {
|
||||
async fn ask_agent_missing_required_argument_is_rejected_before_dispatch() {
|
||||
let contexts = FakeContexts::new();
|
||||
contexts.seed_agent("architect");
|
||||
// If validation wrongly let this through to dispatch, ask_agent would try to
|
||||
// launch/contact the target and the error would differ. A validation rejection
|
||||
// here is INVALID_PARAMS, before dispatch.
|
||||
let (service, _mailbox, _sessions) = build_service_with_mailbox(contexts);
|
||||
let server = server(service);
|
||||
|
||||
let raw = tools_call(5, "idea_ask_agent", json!({ "target": "architect" }));
|
||||
let response = server.handle_raw(&raw).await.expect("reply owed");
|
||||
|
||||
// Rejected at the mapping/validation seam → a JSON-RPC INVALID_PARAMS error,
|
||||
// NOT a tool result (which would mean dispatch happened).
|
||||
let error = response.error.expect("validation error expected");
|
||||
assert_eq!(error.code, error_codes::INVALID_PARAMS, "got {error:?}");
|
||||
assert!(error.message.contains("task"), "got {error:?}");
|
||||
assert!(response.result.is_none(), "no dispatch ⇒ no tool result");
|
||||
}
|
||||
|
||||
@ -1096,179 +1033,6 @@ async fn server_without_event_sink_emits_nothing_and_does_not_panic() {
|
||||
assert_eq!(contexts.entries().len(), 1);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// 10. Anti-wedge (régression du bug serveur lockstep) + timeout du rendezvous.
|
||||
//
|
||||
// Cœur de la régression : un `idea_ask_agent` en attente de son `idea_reply`
|
||||
// ne doit PLUS parquer toute la connexion. La preuve : pendant qu'un ask est
|
||||
// bloqué (rendezvous jamais résolu), un second appel (`tools/list`) sur la
|
||||
// MÊME connexion reçoit sa réponse. L'ancien `serve` lockstep ne lisait plus
|
||||
// rien tant que `handle_raw` de l'ask n'avait pas rendu → ce test bouclait.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
/// Transport scriptable qui **reste ouvert** : il livre `inbound` dans l'ordre,
|
||||
/// puis, une fois la file vide, `recv` reste en attente (au lieu de fermer) tant
|
||||
/// que le test n'a pas appelé `close()`. Cela laisse la boucle `serve` vivante —
|
||||
/// donc capable de drainer les réponses des tâches encore en vol — pendant qu'un
|
||||
/// `idea_ask_agent` est parqué. Les `send` sont capturés sur un canal `mpsc`.
|
||||
struct GatedTransport {
|
||||
inbound: std::collections::VecDeque<Vec<u8>>,
|
||||
outbound: tokio::sync::mpsc::UnboundedSender<Vec<u8>>,
|
||||
close: Arc<tokio::sync::Notify>,
|
||||
}
|
||||
|
||||
impl GatedTransport {
|
||||
fn new(
|
||||
messages: Vec<Vec<u8>>,
|
||||
) -> (
|
||||
Self,
|
||||
tokio::sync::mpsc::UnboundedReceiver<Vec<u8>>,
|
||||
Arc<tokio::sync::Notify>,
|
||||
) {
|
||||
let (tx, rx) = tokio::sync::mpsc::unbounded_channel();
|
||||
let close = Arc::new(tokio::sync::Notify::new());
|
||||
(
|
||||
Self {
|
||||
inbound: messages.into(),
|
||||
outbound: tx,
|
||||
close: Arc::clone(&close),
|
||||
},
|
||||
rx,
|
||||
close,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl Transport for GatedTransport {
|
||||
async fn recv(&mut self) -> Result<Vec<u8>, TransportError> {
|
||||
if let Some(next) = self.inbound.pop_front() {
|
||||
return Ok(next);
|
||||
}
|
||||
// File vide : on n'émet PAS Closed tout de suite — on attend le signal de
|
||||
// fermeture pour ne pas tuer la boucle pendant qu'un ask est encore parqué.
|
||||
self.close.notified().await;
|
||||
Err(TransportError::Closed)
|
||||
}
|
||||
|
||||
async fn send(&mut self, message: &[u8]) -> Result<(), TransportError> {
|
||||
self.outbound
|
||||
.send(message.to_vec())
|
||||
.map_err(|_| TransportError::Closed)
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn pending_ask_does_not_wedge_the_connection_concurrent_call_still_answered() {
|
||||
// Un `idea_ask_agent` vers une cible vivante bloque sur la mailbox (aucun
|
||||
// `idea_reply` ne viendra). Sur la MÊME connexion, un `tools/list` qui suit
|
||||
// doit recevoir sa réponse SANS attendre la résolution de l'ask.
|
||||
let contexts = FakeContexts::new();
|
||||
let agent_id = contexts.seed_agent("architect");
|
||||
let (service, mailbox, sessions) = build_service_with_mailbox(contexts);
|
||||
seed_live_pty(
|
||||
&sessions,
|
||||
agent_id,
|
||||
SessionId::from_uuid(Uuid::from_u128(909)),
|
||||
);
|
||||
let server = server(service);
|
||||
|
||||
let ask = tools_call(
|
||||
1,
|
||||
"idea_ask_agent",
|
||||
json!({ "target": "architect", "task": "blocking..." }),
|
||||
);
|
||||
let list = serde_json::to_vec(&json!({
|
||||
"jsonrpc": "2.0", "id": 2, "method": "tools/list"
|
||||
}))
|
||||
.unwrap();
|
||||
|
||||
let (transport, mut rx, close) = GatedTransport::new(vec![ask, list]);
|
||||
let serve = tokio::spawn(async move {
|
||||
let mut transport = transport;
|
||||
server.serve(&mut transport).await;
|
||||
});
|
||||
|
||||
// L'ask a bien été enqueué (donc il est parqué en attente de reply).
|
||||
tokio::time::timeout(std::time::Duration::from_secs(10), async {
|
||||
while mailbox.pending(&agent_id) == 0 {
|
||||
tokio::task::yield_now().await;
|
||||
}
|
||||
})
|
||||
.await
|
||||
.expect("ask must enqueue a ticket");
|
||||
|
||||
// La réponse au `tools/list` arrive AVANT que l'ask soit résolu : c'est la
|
||||
// preuve anti-wedge. (Sur l'ancien code lockstep, ce recv timeout-ait.)
|
||||
let response = tokio::time::timeout(std::time::Duration::from_secs(10), rx.recv())
|
||||
.await
|
||||
.expect("tools/list must be answered while the ask is still pending")
|
||||
.expect("a response payload");
|
||||
let response: Value = serde_json::from_slice(&response).unwrap();
|
||||
assert_eq!(response["id"], json!(2), "the answered call is tools/list");
|
||||
assert!(
|
||||
response["result"]["tools"].is_array(),
|
||||
"tools/list result, got {response}"
|
||||
);
|
||||
|
||||
// L'ask est toujours en vol (non résolu, aucun `idea_reply` ne viendra) : la
|
||||
// boucle restera en attente de sa réponse, c'est attendu. On la ferme et on
|
||||
// abandonne la tâche serve (le rendezvous parqué ne se résoudra jamais ici).
|
||||
assert_eq!(mailbox.pending(&agent_id), 1, "ask still pending");
|
||||
close.notify_one();
|
||||
serve.abort();
|
||||
let _ = serve.await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn ask_agent_rendezvous_times_out_with_a_jsonrpc_error() {
|
||||
// La cible ne répondra jamais. Avec une borne courte injectée, l'ask doit
|
||||
// finir par renvoyer une ERREUR JSON-RPC propre (filet de sécurité serveur),
|
||||
// au lieu de pendre indéfiniment.
|
||||
let contexts = FakeContexts::new();
|
||||
let agent_id = contexts.seed_agent("architect");
|
||||
let (service, _mailbox, sessions) = build_service_with_mailbox(contexts);
|
||||
seed_live_pty(
|
||||
&sessions,
|
||||
agent_id,
|
||||
SessionId::from_uuid(Uuid::from_u128(910)),
|
||||
);
|
||||
let server = server(service).with_ask_rendezvous_timeout(std::time::Duration::from_millis(50));
|
||||
|
||||
let raw = tools_call(
|
||||
1,
|
||||
"idea_ask_agent",
|
||||
json!({ "target": "architect", "task": "never answered" }),
|
||||
);
|
||||
let response =
|
||||
tokio::time::timeout(std::time::Duration::from_secs(10), server.handle_raw(&raw))
|
||||
.await
|
||||
.expect("must not hang past the injected timeout")
|
||||
.expect("reply owed");
|
||||
|
||||
let error = response.error.expect("a JSON-RPC error on timeout");
|
||||
// Filet serveur : code DISTINCT de l'INTERNAL_ERROR opaque, sémantique typée et
|
||||
// retryable (miroir de `AppError::TargetReturnedNoReply`).
|
||||
assert_eq!(
|
||||
error.code,
|
||||
error_codes::RENDEZVOUS_NO_REPLY,
|
||||
"rendezvous timeout maps to the distinct typed code, got {error:?}"
|
||||
);
|
||||
assert_ne!(
|
||||
error.code,
|
||||
error_codes::INTERNAL_ERROR,
|
||||
"must NOT be the opaque internal error"
|
||||
);
|
||||
assert!(
|
||||
error.message.contains("idea_reply"),
|
||||
"message must steer the caller back to idea_reply, got {error:?}"
|
||||
);
|
||||
let data = error.data.expect("typed data on the rendezvous error");
|
||||
assert_eq!(data["code"], "TARGET_RETURNED_NO_REPLY", "got {data:?}");
|
||||
assert_eq!(data["retryable"], true, "got {data:?}");
|
||||
assert!(response.result.is_none(), "error responses carry no result");
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Diagnostics capture (instrumentation des blocages de délégation)
|
||||
//
|
||||
@ -1290,57 +1054,6 @@ fn read_diag(path: &std::path::Path) -> String {
|
||||
std::fs::read_to_string(path).unwrap_or_default()
|
||||
}
|
||||
|
||||
/// Proves the `[mcp]` rendezvous trace: a wedged `idea_ask_agent` (target that
|
||||
/// never replies) emits begin → armed → EXPIRED, plus the `[mcp] mapped` beacon.
|
||||
#[tokio::test]
|
||||
async fn diag_capture_mcp_ask_begin_armed_expired() {
|
||||
let log = diag_log_path();
|
||||
// Unique target name ⇒ only THIS test's beacons match `target=diag-wedge`.
|
||||
let contexts = FakeContexts::new();
|
||||
let agent_id = contexts.seed_agent("diag-wedge");
|
||||
let (service, _mailbox, sessions) = build_service_with_mailbox(contexts);
|
||||
seed_live_pty(
|
||||
&sessions,
|
||||
agent_id,
|
||||
SessionId::from_uuid(Uuid::from_u128(0xD1A6_0911)),
|
||||
);
|
||||
let server = server(service).with_ask_rendezvous_timeout(std::time::Duration::from_millis(50));
|
||||
|
||||
let raw = tools_call(
|
||||
1,
|
||||
"idea_ask_agent",
|
||||
json!({ "target": "diag-wedge", "task": "never answered diag" }),
|
||||
);
|
||||
let response =
|
||||
tokio::time::timeout(std::time::Duration::from_secs(10), server.handle_raw(&raw))
|
||||
.await
|
||||
.expect("must not hang past the injected timeout")
|
||||
.expect("reply owed");
|
||||
assert!(
|
||||
response.error.is_some(),
|
||||
"wedged ask returns a JSON-RPC error"
|
||||
);
|
||||
|
||||
let body = read_diag(&log);
|
||||
assert!(
|
||||
body.contains("[mcp] mapped kind=ask_agent target=diag-wedge"),
|
||||
"mapped beacon missing: {body}"
|
||||
);
|
||||
assert!(
|
||||
body.contains("[mcp] tools_call begin tool=idea_ask_agent")
|
||||
&& body.contains("target=diag-wedge"),
|
||||
"begin beacon missing: {body}"
|
||||
);
|
||||
assert!(
|
||||
body.contains("[mcp] ask armed") && body.contains("target=diag-wedge"),
|
||||
"armed beacon missing: {body}"
|
||||
);
|
||||
assert!(
|
||||
body.contains("[mcp] ask EXPIRED") && body.contains("target=diag-wedge"),
|
||||
"EXPIRED beacon missing: {body}"
|
||||
);
|
||||
}
|
||||
|
||||
/// Proves the `[rendezvous] idea_reply received` beacon fires BEFORE correlation and
|
||||
/// is followed by `UNMATCHED` when no in-flight ask exists for the emitter.
|
||||
#[tokio::test]
|
||||
|
||||
Reference in New Issue
Block a user