From 97daf3fae5c310bcf1434fa97a8a04a495910e37 Mon Sep 17 00:00:00 2001 From: Blomios Date: Wed, 10 Jun 2026 00:04:13 +0200 Subject: [PATCH] =?UTF-8?q?feat(agent):=20surfacer=20reply=20dans=20le=20w?= =?UTF-8?q?riter=20wire=20.response.json=20(D6b)=20=E2=80=94=20=C2=A717?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ferme le canal fichier de la messagerie inter-agents : un agent qui délègue via le protocole .ideai/requests reçoit désormais le CONTENU de la réponse, plus seulement l'ACK de cycle de vie. - OrchestratorResponse gagne reply: Option (skip_serializing_if None, camelCase). success() porte le reply ; failure() => None. - dispatch_file passe out.reply (de l'OrchestratorOutcome D6) à success. Champ additif : un .response.json legacy sans reply reste valide ; une commande non-ask (agent.run, spawn_agent...) => clé reply absente du JSON. Tests (QA) : +4 verts — ask => reply présent ET detail coexistent ; non-ask => clé absente (garde anti-always-green sur l'absence littérale) ; round-trip legacy ; failure => absent. cargo test --workspace : 824 passed. Orchestration structurée bouclée : in-process (D6) + protocole fichier (D6b). Co-Authored-By: Claude Opus 4.8 --- crates/infrastructure/src/orchestrator/mod.rs | 11 +- .../tests/orchestrator_watcher.rs | 254 +++++++++++++++++- 2 files changed, 259 insertions(+), 6 deletions(-) diff --git a/crates/infrastructure/src/orchestrator/mod.rs b/crates/infrastructure/src/orchestrator/mod.rs index a50cdd6..e613f4d 100644 --- a/crates/infrastructure/src/orchestrator/mod.rs +++ b/crates/infrastructure/src/orchestrator/mod.rs @@ -56,15 +56,22 @@ pub struct OrchestratorResponse { /// Human-readable failure reason (`ok == false`). #[serde(skip_serializing_if = "Option::is_none")] pub error: Option, + /// The queried agent's reply content, when the dispatched command produced one + /// (e.g. `ask` / `agent.message`). Absent for commands that return no content + /// (e.g. `agent.run`), so an existing `*.response.json` without this key stays + /// valid. + #[serde(skip_serializing_if = "Option::is_none")] + pub reply: Option, } impl OrchestratorResponse { - fn success(action: String, detail: String) -> Self { + fn success(action: String, detail: String, reply: Option) -> Self { Self { ok: true, action: Some(action), detail: Some(detail), error: None, + reply, } } fn failure(action: Option, error: String) -> Self { @@ -73,6 +80,7 @@ impl OrchestratorResponse { action, detail: None, error: Some(error), + reply: None, } } } @@ -258,6 +266,7 @@ async fn dispatch_file( Ok(out) => OrchestratorResponse::success( action.unwrap_or_else(|| "unknown".to_owned()), out.detail, + out.reply, ), Err(e) => OrchestratorResponse::failure(action, e.to_string()), } diff --git a/crates/infrastructure/tests/orchestrator_watcher.rs b/crates/infrastructure/tests/orchestrator_watcher.rs index d3b2814..c227813 100644 --- a/crates/infrastructure/tests/orchestrator_watcher.rs +++ b/crates/infrastructure/tests/orchestrator_watcher.rs @@ -19,11 +19,12 @@ use domain::events::DomainEvent; use domain::ids::SkillId; use domain::ids::{AgentId, ProfileId, ProjectId}; use domain::markdown::MarkdownDoc; +use domain::ids::NodeId; use domain::ports::{ - AgentContextStore, AgentRuntime, ContextInjectionPlan, DirEntry, EventBus, EventStream, - ExitStatus, FileSystem, FsError, IdGenerator, OutputStream, PreparedContext, ProfileStore, - PtyError, PtyHandle, PtyPort, RemotePath, RuntimeError, SessionPlan, SkillStore, SpawnSpec, - StoreError, + AgentContextStore, AgentRuntime, AgentSession, AgentSessionError, ContextInjectionPlan, + DirEntry, EventBus, EventStream, ExitStatus, FileSystem, FsError, IdGenerator, OutputStream, + PreparedContext, ProfileStore, PtyError, PtyHandle, PtyPort, RemotePath, ReplyEvent, + ReplyStream, RuntimeError, SessionPlan, SkillStore, SpawnSpec, StoreError, }; use domain::profile::{AgentProfile, ContextInjection}; use domain::project::{Project, ProjectPath}; @@ -34,7 +35,7 @@ use uuid::Uuid; use application::{ CloseTerminal, CreateAgentFromScratch, CreateSkill, LaunchAgent, ListAgents, - OrchestratorService, TerminalSessions, UpdateAgentContext, + OrchestratorService, StructuredSessions, TerminalSessions, UpdateAgentContext, }; use infrastructure::{process_request_file, OrchestratorResponse}; @@ -74,6 +75,23 @@ impl FakeContexts { fn entries(&self) -> Vec { self.0.lock().unwrap().manifest.entries.clone() } + /// Seeds the manifest with an already-existing agent so `find_agent_id_by_name` + /// resolves it without going through `CreateAgentFromScratch`. Returns the id. + fn seed_agent(&self, name: &str) -> AgentId { + let id = AgentId::from_uuid(Uuid::new_v4()); + let mut inner = self.0.lock().unwrap(); + inner.manifest.entries.push(ManifestEntry { + agent_id: id, + name: name.to_owned(), + md_path: format!("agents/{name}.md"), + profile_id: ProfileId::from_uuid(Uuid::from_u128(9)), + template_id: None, + synchronized: false, + synced_template_version: None, + skills: Vec::new(), + }); + id + } fn md_path_of(&self, agent: &AgentId) -> Option { self.0 .lock() @@ -271,6 +289,37 @@ impl PtyPort for FakePty { } } +/// A structured [`AgentSession`] whose turn deterministically ends on a `Final` +/// carrying a fixed reply. Lets us exercise the `ask`/`agent.message` path +/// (`OrchestratorOutcome.reply = Some(..)`) without a real CLI. +struct FakeSession { + id: SessionId, + reply: String, +} +#[async_trait] +impl AgentSession for FakeSession { + fn id(&self) -> SessionId { + self.id + } + fn conversation_id(&self) -> Option { + None + } + async fn send(&self, _prompt: &str) -> Result { + let events = vec![ + ReplyEvent::TextDelta { + text: "thinking…".to_owned(), + }, + ReplyEvent::Final { + content: self.reply.clone(), + }, + ]; + Ok(Box::new(events.into_iter())) + } + async fn shutdown(&self) -> Result<(), AgentSessionError> { + Ok(()) + } +} + #[derive(Default, Clone)] struct NoopBus; impl EventBus for NoopBus { @@ -352,6 +401,26 @@ fn build_service(contexts: FakeContexts) -> Arc { )) } +/// Like [`build_service`] but with the **structured sessions** registry wired +/// (`with_structured`), so `agent.message`/`ask` can find a live structured target. +/// Returns the service and the shared registry so a test can pre-insert a session. +fn build_service_with_structured( + contexts: FakeContexts, +) -> (Arc, Arc) { + let structured = Arc::new(StructuredSessions::new()); + // `build_service` already assembles every use case over the same fakes; we only + // need to fold the structured registry onto the resulting service. + let base = build_service(contexts); + // `OrchestratorService` is not `Clone`; rebuild via the builder on a fresh one is + // overkill, so we reconstruct minimally is not possible — instead, the service + // exposes `with_structured` as a consuming builder. We rely on `Arc::try_unwrap` + // since `build_service` returns a freshly-created `Arc` with refcount 1. + let service = Arc::try_unwrap(base) + .unwrap_or_else(|_| panic!("freshly built service must be uniquely owned")) + .with_structured(Arc::clone(&structured)); + (Arc::new(service), structured) +} + fn read_response(request_path: &std::path::Path) -> OrchestratorResponse { let mut name = request_path .file_name() @@ -364,6 +433,20 @@ fn read_response(request_path: &std::path::Path) -> OrchestratorResponse { serde_json::from_slice(&bytes).expect("response parses") } +/// Reads the raw `*.response.json` bytes as a UTF-8 string, for assertions on the +/// *literal wire shape* (e.g. that the `"reply"` key is absent — not just `None` +/// after deserialisation). +fn read_response_raw(request_path: &std::path::Path) -> String { + let mut name = request_path + .file_name() + .unwrap() + .to_string_lossy() + .into_owned(); + name.push_str(".response.json"); + let response_path = request_path.with_file_name(name); + std::fs::read_to_string(&response_path).expect("response file written") +} + #[tokio::test] async fn valid_spawn_request_succeeds_and_is_consumed() { let tmp = TempDir::new(); @@ -476,3 +559,164 @@ async fn unknown_action_yields_error_response() { .unwrap_or_default() .contains("unknown orchestrator action")); } + +// --------------------------------------------------------------------------- +// LOT D6b — `reply` surfaced into the `*.response.json` wire format. +// +// D6b makes `OrchestratorResponse` carry an optional `reply` (the queried agent's +// content for `ask`/`agent.message`), `#[serde(skip_serializing_if = "Option::is_none")]` +// so a response with no reply keeps the exact legacy shape. +// --------------------------------------------------------------------------- + +/// Point 1 — an `agent.message` (`ask`) whose target replies ⇒ the on-disk +/// `*.response.json` carries the `reply` content **and** `detail` (both coexist), +/// alongside `ok: true`. +#[tokio::test] +async fn ask_request_surfaces_reply_alongside_detail() { + let tmp = TempDir::new(); + let contexts = FakeContexts::new(); + // Seed an existing target agent and a live structured session that replies. + let agent_id = contexts.seed_agent("architect"); + let (service, structured) = build_service_with_structured(contexts.clone()); + structured.insert( + Arc::new(FakeSession { + id: SessionId::from_uuid(Uuid::from_u128(4242)), + reply: "the answer is 42".to_owned(), + }), + agent_id, + NodeId::from_uuid(Uuid::from_u128(7)), + ); + + let req = tmp.0.join("ask-req.json"); + std::fs::write( + &req, + br#"{ "type": "agent.message", "requestedBy": "Main", "targetAgent": "architect", "task": "What is the answer?" }"#, + ) + .unwrap(); + + let response = process_request_file(&req, &project(), &service).await; + + assert!(response.ok, "expected ok, got {response:?}"); + assert_eq!(response.action.as_deref(), Some("agent.message")); + // reply carries the target's Final content... + assert_eq!(response.reply.as_deref(), Some("the answer is 42")); + // ...and detail still summarises what IdeA did (the two coexist). + assert!( + response + .detail + .as_deref() + .unwrap_or_default() + .contains("architect"), + "detail should still be present: {response:?}" + ); + + // The on-disk wire format actually contains both keys with the right values. + let on_disk = read_response(&req); + assert!(on_disk.ok); + assert_eq!(on_disk.reply.as_deref(), Some("the answer is 42")); + assert!(on_disk.detail.is_some()); + + let raw = read_response_raw(&req); + let json: serde_json::Value = serde_json::from_str(&raw).unwrap(); + assert_eq!(json["reply"], serde_json::json!("the answer is 42")); + assert!(json.get("detail").is_some()); + assert!(!req.exists(), "request file must be removed"); +} + +/// Point 2 — a non-`ask` command (here `spawn_agent`, reply `None`) ⇒ the `reply` +/// key is **absent** from the serialised JSON (`skip_serializing_if`). Anti-always-green: +/// we assert on the *literal absence* of the key in the raw bytes, so the test would +/// fail if `reply: null` were emitted instead. +#[tokio::test] +async fn non_ask_request_omits_reply_key() { + let tmp = TempDir::new(); + let contexts = FakeContexts::new(); + let service = build_service(contexts.clone()); + + let req = tmp.0.join("spawn-no-reply.json"); + std::fs::write( + &req, + br#"{ "action": "spawn_agent", "name": "dev-backend", "profile": "claude-code" }"#, + ) + .unwrap(); + + let response = process_request_file(&req, &project(), &service).await; + + assert!(response.ok, "expected ok, got {response:?}"); + assert!(response.reply.is_none(), "spawn must not produce a reply"); + + // Deserialised: reply is None. + let on_disk = read_response(&req); + assert!(on_disk.reply.is_none()); + + // Raw wire: the "reply" key must not appear at all. (Guard the assertion itself: + // confirm we ARE looking at the right file by checking a key we know is present.) + let raw = read_response_raw(&req); + assert!( + raw.contains("\"ok\""), + "sanity: response JSON should contain ok — got {raw}" + ); + assert!( + !raw.contains("reply"), + "reply key must be skipped when None — got {raw}" + ); + // And via the parsed tree, the key is structurally absent (not null). + let json: serde_json::Value = serde_json::from_str(&raw).unwrap(); + assert!( + json.get("reply").is_none(), + "reply key must be structurally absent — got {json}" + ); +} + +/// Point 3 — round-trip a legacy `*.response.json` written **before** D6b (no +/// `reply` key): it deserialises to `reply == None`, and re-serialising does **not** +/// reintroduce the key (forward/backward wire compatibility). +#[test] +fn legacy_response_without_reply_round_trips_without_reintroducing_key() { + // A response payload exactly as a pre-D6b IdeA would have written it. + let legacy = r#"{"ok":true,"action":"spawn_agent","detail":"launched agent dev-backend in background"}"#; + + let parsed: OrchestratorResponse = + serde_json::from_str(legacy).expect("legacy response must still parse"); + assert!(parsed.ok); + assert_eq!(parsed.action.as_deref(), Some("spawn_agent")); + assert!( + parsed.reply.is_none(), + "absent reply key must deserialise to None" + ); + + // Re-serialising must not bring a "reply" key back. + let reserialised = serde_json::to_string(&parsed).unwrap(); + assert!( + !reserialised.contains("reply"), + "round-trip must not reintroduce the reply key — got {reserialised}" + ); + let json: serde_json::Value = serde_json::from_str(&reserialised).unwrap(); + assert!(json.get("reply").is_none()); +} + +/// Point 4 — a failure response carries no `reply` (the field is `None` for every +/// `failure(..)`), so the key is absent from the failing `*.response.json` too. +#[tokio::test] +async fn failure_response_omits_reply_key() { + let tmp = TempDir::new(); + let service = build_service(FakeContexts::new()); + + let req = tmp.0.join("broken-no-reply.json"); + std::fs::write(&req, b"{ not json at all").unwrap(); + + let response = process_request_file(&req, &project(), &service).await; + + assert!(!response.ok); + assert!(response.reply.is_none(), "failure must not carry a reply"); + + let raw = read_response_raw(&req); + assert!( + raw.contains("\"error\""), + "sanity: a failure JSON should contain error — got {raw}" + ); + assert!( + !raw.contains("reply"), + "reply key must be absent on failure — got {raw}" + ); +}