feat(agent): surfacer reply dans le writer wire .response.json (D6b) — §17
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<String> (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 <noreply@anthropic.com>
This commit is contained in:
@ -56,15 +56,22 @@ pub struct OrchestratorResponse {
|
|||||||
/// Human-readable failure reason (`ok == false`).
|
/// Human-readable failure reason (`ok == false`).
|
||||||
#[serde(skip_serializing_if = "Option::is_none")]
|
#[serde(skip_serializing_if = "Option::is_none")]
|
||||||
pub error: Option<String>,
|
pub error: Option<String>,
|
||||||
|
/// 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<String>,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl OrchestratorResponse {
|
impl OrchestratorResponse {
|
||||||
fn success(action: String, detail: String) -> Self {
|
fn success(action: String, detail: String, reply: Option<String>) -> Self {
|
||||||
Self {
|
Self {
|
||||||
ok: true,
|
ok: true,
|
||||||
action: Some(action),
|
action: Some(action),
|
||||||
detail: Some(detail),
|
detail: Some(detail),
|
||||||
error: None,
|
error: None,
|
||||||
|
reply,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
fn failure(action: Option<String>, error: String) -> Self {
|
fn failure(action: Option<String>, error: String) -> Self {
|
||||||
@ -73,6 +80,7 @@ impl OrchestratorResponse {
|
|||||||
action,
|
action,
|
||||||
detail: None,
|
detail: None,
|
||||||
error: Some(error),
|
error: Some(error),
|
||||||
|
reply: None,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@ -258,6 +266,7 @@ async fn dispatch_file(
|
|||||||
Ok(out) => OrchestratorResponse::success(
|
Ok(out) => OrchestratorResponse::success(
|
||||||
action.unwrap_or_else(|| "unknown".to_owned()),
|
action.unwrap_or_else(|| "unknown".to_owned()),
|
||||||
out.detail,
|
out.detail,
|
||||||
|
out.reply,
|
||||||
),
|
),
|
||||||
Err(e) => OrchestratorResponse::failure(action, e.to_string()),
|
Err(e) => OrchestratorResponse::failure(action, e.to_string()),
|
||||||
}
|
}
|
||||||
|
|||||||
@ -19,11 +19,12 @@ use domain::events::DomainEvent;
|
|||||||
use domain::ids::SkillId;
|
use domain::ids::SkillId;
|
||||||
use domain::ids::{AgentId, ProfileId, ProjectId};
|
use domain::ids::{AgentId, ProfileId, ProjectId};
|
||||||
use domain::markdown::MarkdownDoc;
|
use domain::markdown::MarkdownDoc;
|
||||||
|
use domain::ids::NodeId;
|
||||||
use domain::ports::{
|
use domain::ports::{
|
||||||
AgentContextStore, AgentRuntime, ContextInjectionPlan, DirEntry, EventBus, EventStream,
|
AgentContextStore, AgentRuntime, AgentSession, AgentSessionError, ContextInjectionPlan,
|
||||||
ExitStatus, FileSystem, FsError, IdGenerator, OutputStream, PreparedContext, ProfileStore,
|
DirEntry, EventBus, EventStream, ExitStatus, FileSystem, FsError, IdGenerator, OutputStream,
|
||||||
PtyError, PtyHandle, PtyPort, RemotePath, RuntimeError, SessionPlan, SkillStore, SpawnSpec,
|
PreparedContext, ProfileStore, PtyError, PtyHandle, PtyPort, RemotePath, ReplyEvent,
|
||||||
StoreError,
|
ReplyStream, RuntimeError, SessionPlan, SkillStore, SpawnSpec, StoreError,
|
||||||
};
|
};
|
||||||
use domain::profile::{AgentProfile, ContextInjection};
|
use domain::profile::{AgentProfile, ContextInjection};
|
||||||
use domain::project::{Project, ProjectPath};
|
use domain::project::{Project, ProjectPath};
|
||||||
@ -34,7 +35,7 @@ use uuid::Uuid;
|
|||||||
|
|
||||||
use application::{
|
use application::{
|
||||||
CloseTerminal, CreateAgentFromScratch, CreateSkill, LaunchAgent, ListAgents,
|
CloseTerminal, CreateAgentFromScratch, CreateSkill, LaunchAgent, ListAgents,
|
||||||
OrchestratorService, TerminalSessions, UpdateAgentContext,
|
OrchestratorService, StructuredSessions, TerminalSessions, UpdateAgentContext,
|
||||||
};
|
};
|
||||||
use infrastructure::{process_request_file, OrchestratorResponse};
|
use infrastructure::{process_request_file, OrchestratorResponse};
|
||||||
|
|
||||||
@ -74,6 +75,23 @@ impl FakeContexts {
|
|||||||
fn entries(&self) -> Vec<ManifestEntry> {
|
fn entries(&self) -> Vec<ManifestEntry> {
|
||||||
self.0.lock().unwrap().manifest.entries.clone()
|
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<String> {
|
fn md_path_of(&self, agent: &AgentId) -> Option<String> {
|
||||||
self.0
|
self.0
|
||||||
.lock()
|
.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<String> {
|
||||||
|
None
|
||||||
|
}
|
||||||
|
async fn send(&self, _prompt: &str) -> Result<ReplyStream, AgentSessionError> {
|
||||||
|
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)]
|
#[derive(Default, Clone)]
|
||||||
struct NoopBus;
|
struct NoopBus;
|
||||||
impl EventBus for NoopBus {
|
impl EventBus for NoopBus {
|
||||||
@ -352,6 +401,26 @@ fn build_service(contexts: FakeContexts) -> Arc<OrchestratorService> {
|
|||||||
))
|
))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// 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<OrchestratorService>, Arc<StructuredSessions>) {
|
||||||
|
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 {
|
fn read_response(request_path: &std::path::Path) -> OrchestratorResponse {
|
||||||
let mut name = request_path
|
let mut name = request_path
|
||||||
.file_name()
|
.file_name()
|
||||||
@ -364,6 +433,20 @@ fn read_response(request_path: &std::path::Path) -> OrchestratorResponse {
|
|||||||
serde_json::from_slice(&bytes).expect("response parses")
|
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]
|
#[tokio::test]
|
||||||
async fn valid_spawn_request_succeeds_and_is_consumed() {
|
async fn valid_spawn_request_succeeds_and_is_consumed() {
|
||||||
let tmp = TempDir::new();
|
let tmp = TempDir::new();
|
||||||
@ -476,3 +559,164 @@ async fn unknown_action_yields_error_response() {
|
|||||||
.unwrap_or_default()
|
.unwrap_or_default()
|
||||||
.contains("unknown orchestrator action"));
|
.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}"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user