diff --git a/Cargo.lock b/Cargo.lock index 8bf05b7bfc0..70f17f22c3b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3547,11 +3547,16 @@ dependencies = [ name = "ironclaw_attachments" version = "0.1.0" dependencies = [ + "async-trait", + "chrono", "ironclaw_common", "ironclaw_extractors", "ironclaw_filesystem", "ironclaw_host_api", + "ironclaw_product_contracts", + "ironclaw_threads", "serde", + "serde_json", "sha2 0.11.0", "thiserror 2.0.19", "tokio", @@ -3658,6 +3663,7 @@ version = "0.1.0" dependencies = [ "async-trait", "chrono", + "ironclaw_extension_contracts", "ironclaw_filesystem", "ironclaw_host_api", "ironclaw_safety", @@ -3752,6 +3758,7 @@ dependencies = [ "hmac 0.13.0", "http-body-util", "ironclaw_approvals", + "ironclaw_attachments", "ironclaw_auth", "ironclaw_authorization", "ironclaw_capabilities", @@ -5134,6 +5141,7 @@ dependencies = [ "hmac 0.13.0", "http 1.4.2", "http-body-util", + "ironclaw_attachments", "ironclaw_auth", "ironclaw_common", "ironclaw_extension_contracts", diff --git a/crates/AGENTS.md b/crates/AGENTS.md index 15d0d998138..f3cca57b09e 100644 --- a/crates/AGENTS.md +++ b/crates/AGENTS.md @@ -84,7 +84,7 @@ Boundary rule: if you need an upstream crate in a low-level crate, stop and chec | `ironclaw_reborn_event_store` | `ironclaw_reborn_event_store/AGENTS.md`, `docs/reborn/contracts/events.md` | Reborn-owned durable event/audit store backends and fixtures. | Product projections, transport fanout, workflow policy. | | `ironclaw_reborn_traces` | `Cargo.toml`, `src/lib.rs` | Trace Commons / TraceDAO client surface: contribution pipeline, trace client, redaction helpers, conversation-message compatibility, and trace preview re-exports. | Reborn CLI command behavior, LLM provider routing, unredacted trace submission. | | `ironclaw_memory_native` | `ironclaw_memory_native/AGENTS.md`, `ironclaw_memory_native/CLAUDE.md` | Native filesystem memory provider: `NativeMemoryService`, document repos, chunking, hybrid search, indexer, prompt-write-safety engine. | Provider-neutral memory contracts (`ironclaw_memory`) or product workflow. | -| `ironclaw_attachments` | `Cargo.toml`, `src/lib.rs` | The single inbound-attachment landing routine, writing through project-scoped `ScopedFilesystem` (fail-closed on read-only mounts). | Per-channel persistence paths; text extraction (that's `ironclaw_extractors`). | +| `ironclaw_attachments` | `Cargo.toml`, `src/lib.rs`, `src/ports.rs`, `src/project_scoped.rs`, `src/budgets.rs` | The inbound-attachment **ports** (`InboundAttachmentLander`, `InboundAttachmentReader`, `AttachmentCleanupReport`), the default `ProjectScopedAttachmentLander` over a project-scoped `ScopedFilesystem` (fail-closed on read-only mounts), and the one home for the size ceilings + advertised `AttachmentCapabilities` that WebUI and the OpenAI-compatible adapter read (WS5, PROPOSAL §6.4.9). | Per-channel persistence paths; text extraction (that's `ironclaw_extractors`); the *reader* implementation — `ProjectScopedAttachmentReader` stays in `ironclaw_product` because it also implements a `loops`-tier trait a substrate may not name. | | `ironclaw_extractors` | `Cargo.toml`, `src/lib.rs` | Pure bytes→text extraction by MIME (PDF/OOXML/legacy Office) with decompression-bomb caps; no I/O. | Network fetches, storage, channel logic. | | `ironclaw_triggers` | `ironclaw_triggers/AGENTS.md`, `docs/reborn/contracts/triggers.md` | Scheduled-trigger substrate: records, cron/timezone validation, deterministic fire identity, poller core, durable libSQL/Postgres repos, trusted-submit request minting. | Poller lifecycle/composition (composition owns it); any parallel agent loop. | | `ironclaw_projects` | `ironclaw_projects/CLAUDE.md` | Project entity + membership ACL (live `resolve_access`, never cached) + `ProjectRepository` over `RootFilesystem` with CAS create/delete. **W2 decision: keep standalone; do not fold into composition.** | Product workflow service logic. If revisited, `ironclaw_product` is the only acceptable consumer-side target. | diff --git a/crates/ironclaw_architecture/tests/reborn_conversations_threads_attachments.rs b/crates/ironclaw_architecture/tests/reborn_conversations_threads_attachments.rs new file mode 100644 index 00000000000..cb0e0553c7f --- /dev/null +++ b/crates/ironclaw_architecture/tests/reborn_conversations_threads_attachments.rs @@ -0,0 +1,390 @@ +//! CHECKLIST WS5 — the `conversations`/`threads` naming trap and the +//! `attachments` widening (PROPOSAL §6.4.1, §6.4.2, §6.4.9). +//! +//! Three rules, each pinning one half of that work so it cannot silently +//! regress: +//! +//! 1. **`ironclaw_conversations` and `ironclaw_threads` share no type name.** +//! Discovered, not enumerated: every type either crate declares is compared +//! against the other's, so a *new* collision is caught the day it is +//! introduced rather than the day someone has to alias around it. Before +//! this row the two crates shared five names — `SessionThreadService`, +//! `AcceptInboundMessageRequest`, `AcceptedInboundMessage`, +//! `AcceptedInboundMessageReplay`, `ThreadMessageRecord` — and the one file +//! that had to hold both, composition's trusted-trigger submit, carried four +//! `as Canonical…` aliases to say which was which. Names are the contract +//! here: a caller that picks the wrong `accept_inbound_message` writes to the +//! wrong store, and it still compiles. +//! +//! 2. **The external actor/conversation pair has one home**, and it is +//! `ironclaw_extension_contracts` — not `ironclaw_conversations`, which used +//! to carry a second, field-divergent copy (`thread_id`/`message_id` versus +//! `topic_id`/`reply_target_message_id`) that `ironclaw_product` bridged with +//! hand-written field-by-field translators. Neither the second definition nor +//! the translators may come back. This complements +//! `reborn_extension_contract_location_scan.rs`, whose `COLLISION_EXEMPT` +//! carve-out for the pair was **deleted** rather than repointed when the +//! duplicate went away; this file states the outcome positively so a +//! re-introduction fails with the reason attached. +//! +//! 3. **`ironclaw_attachments` is the one home for the attachment ports and the +//! size ceilings.** §6.4.9 widens it to hold "the single landing routine plus +//! its ports" and "one home for the size-ceiling constants (webui/openai +//! import them)"; the failure mode it closes is a transport advertising one +//! ceiling while the landing routine enforces another. So the ports and the +//! advertised-capabilities builder must be declared there and **not** in +//! `ironclaw_product`. +//! +//! **What is deliberately not asserted:** that `ProjectScopedAttachmentReader` +//! moved. It cannot. It implements `ironclaw_loop_host::LoopAttachmentReadPort` +//! as well as `InboundAttachmentReader`, and `ironclaw_loop_host` is a +//! `loops`-layer crate a `substrates` crate may not depend on — and once the +//! struct moved, that impl would have neither side in `ironclaw_product`, so the +//! orphan rule forbids leaving it behind too. Test 3c pins the reader *in +//! product* for that reason, so "finishing the row" cannot quietly mean moving +//! it and dragging the loop tier into a substrate. + +use std::collections::{BTreeMap, BTreeSet}; +use std::path::Path; + +// Only part of the shared ratchet module is reachable from this binary. +#[allow(dead_code)] +mod ratchet_support; + +use ratchet_support::{ + TypeDefOccurrence, collect_type_defs, strip_comments_and_strings, workspace_root, +}; + +const TYPE_KEYWORDS: &[&str] = &["trait ", "struct ", "enum "]; + +/// This scanner's own file, skipped so its fixtures do not look like +/// definitions. +const SELF_FILE: &str = "reborn_conversations_threads_attachments.rs"; + +fn is_rust_identifier(ident: &str) -> bool { + let mut chars = ident.chars(); + match chars.next() { + Some(first) if first.is_ascii_alphabetic() || first == '_' => {} + _ => return false, + } + chars.all(|ch| ch.is_ascii_alphanumeric() || ch == '_') +} + +/// Every type, trait and enum a crate declares in its production source. +fn declared_names(root: &Path, crate_name: &str) -> BTreeSet { + let mut found: BTreeMap> = BTreeMap::new(); + let dir = root.join("crates").join(crate_name).join("src"); + assert!( + dir.is_dir(), + "{crate_name} must have a src/ directory; this scan is vacuous otherwise" + ); + collect_type_defs( + &dir, + TYPE_KEYWORDS, + &is_rust_identifier, + &[SELF_FILE], + &mut found, + ); + assert!( + !found.is_empty(), + "no types were discovered in {crate_name} — the walk is broken, not the crate" + ); + found.into_keys().collect() +} + +/// Read a crate's production source as one string, with `//`-comments and +/// string literals removed, so a name in prose or in an error message never +/// counts as code. +fn stripped_production_source(root: &Path, crate_name: &str) -> String { + fn walk(dir: &Path, out: &mut String) { + let entries = std::fs::read_dir(dir) + .unwrap_or_else(|err| panic!("failed to read {}: {err}", dir.display())); + for entry in entries { + let entry = entry.unwrap_or_else(|err| panic!("failed to read dir entry: {err}")); + let path = entry.path(); + if path.is_dir() { + let name = path.file_name().and_then(|n| n.to_str()); + if matches!(name, Some("target" | "tests" | "examples" | "benches")) { + continue; + } + walk(&path, out); + continue; + } + if path.extension().and_then(|ext| ext.to_str()) != Some("rs") { + continue; + } + let source = std::fs::read_to_string(&path) + .unwrap_or_else(|err| panic!("failed to read {}: {err}", path.display())); + out.push_str(&strip_comments_and_strings(&source)); + out.push('\n'); + } + } + + let dir = root.join("crates").join(crate_name).join("src"); + let mut out = String::new(); + walk(&dir, &mut out); + assert!( + !out.trim().is_empty(), + "{crate_name}'s production source scanned empty — the walk is broken" + ); + out +} + +/// The five names that collided before this row, kept by name so the failure +/// message can say what the fix was the last time. +const HISTORIC_COLLISIONS: &[&str] = &[ + "SessionThreadService", + "AcceptInboundMessageRequest", + "AcceptedInboundMessage", + "AcceptedInboundMessageReplay", + "ThreadMessageRecord", +]; + +#[test] +fn conversations_and_threads_declare_no_name_in_common() { + let root = workspace_root(); + let conversations = declared_names(&root, "ironclaw_conversations"); + let threads = declared_names(&root, "ironclaw_threads"); + + // Non-vacuity: both sets must be substantial, or an empty intersection + // proves nothing. + assert!( + conversations.len() > 20, + "expected ironclaw_conversations to declare many types, found {}", + conversations.len() + ); + assert!( + threads.len() > 20, + "expected ironclaw_threads to declare many types, found {}", + threads.len() + ); + + let shared: Vec<&String> = conversations.intersection(&threads).collect(); + let shared_count = shared.len(); + assert!( + shared.is_empty(), + "ironclaw_conversations and ironclaw_threads declare {shared_count} name(s) in common: {shared:?}\n\ + \n\ + These two crates are the audited worst naming trap in the Reborn tree \ + (PROPOSAL §6.4.1/§6.4.2): one owns external↔canonical *binding* and \ + idempotency, the other owns the canonical *transcript*. A shared name \ + means a caller can import the wrong one, call the same method, and \ + write to the wrong store — and it compiles. CHECKLIST WS5 discharged \ + five such names ({HISTORIC_COLLISIONS:?}) by renaming the conversations \ + side (`SessionThreadService` → `InboundConversationService`, the \ + `AcceptedInboundMessage*` family → `AcceptedConversationMessage*`, \ + `ThreadMessageRecord` → `ConversationMessageRecord`).\n\ + Rename the new one on whichever side is the weaker claim to it — do not \ + add an exemption here, and do not alias at the import site." + ); + + // The rename actually landed on the conversations side, and the canonical + // transcript kept the names it had. Pinned so a later "fix" that renames + // `ironclaw_threads` instead — which would churn ~70 files and lose the + // argument — fails loudly. + for name in HISTORIC_COLLISIONS { + assert!( + threads.contains(*name), + "{name} must stay declared in ironclaw_threads: the transcript crate is the \ + stronger claim to the thread/message vocabulary, and the WS5 rename moved the \ + conversations side, not this one" + ); + assert!( + !conversations.contains(*name), + "{name} is declared in ironclaw_conversations again — this is the exact collision \ + CHECKLIST WS5 removed" + ); + } + for name in [ + "InboundConversationService", + "AcceptConversationMessageRequest", + "AcceptedConversationMessage", + "AcceptedConversationMessageReplay", + "ConversationMessageRecord", + ] { + assert!( + conversations.contains(name), + "{name} is the post-rename name and must be declared in ironclaw_conversations; \ + if it was renamed again, update this list in the same change" + ); + } +} + +#[test] +fn the_external_ref_pair_is_declared_only_by_the_extension_contracts_crate() { + let root = workspace_root(); + let pair = ["ExternalActorRef", "ExternalConversationRef"]; + + let owner = declared_names(&root, "ironclaw_extension_contracts"); + for name in pair { + assert!( + owner.contains(name), + "{name} must be declared in ironclaw_extension_contracts — it is the channel tier's \ + vocabulary (PROPOSAL §6.1.2). If it moved, repoint this test in the same change" + ); + } + + let conversations = declared_names(&root, "ironclaw_conversations"); + for name in pair { + assert!( + !conversations.contains(name), + "ironclaw_conversations declares its own {name} again.\n\ + \n\ + It used to, with *different fields*: `thread_id`/`message_id` where the canonical \ + type says `topic_id`/`reply_target_message_id`, and no `display_name`. \ + `ironclaw_product` bridged the two with hand-written field-by-field translators \ + (`conversation_actor_ref` / `conversation_conversation_ref`), which is how a \ + conversion could silently drop a field. CHECKLIST WS5 unified them and deleted the \ + translators; the durable record grammar is preserved by \ + `ironclaw_conversations::stored_refs`, not by a second type" + ); + } + + // The translators themselves must not come back. Checked against stripped + // source so the names surviving in prose (a module doc, this file) do not + // count. + let product = stripped_production_source(&root, "ironclaw_product"); + for translator in [ + "fn conversation_actor_ref", + "fn conversation_conversation_ref", + ] { + assert!( + !product.contains(translator), + "ironclaw_product declares `{translator}` again — the field-by-field translator \ + CHECKLIST WS5 deleted. The two types are one type now; pass the value through" + ); + } + + // And conversations must not become a second *import path* for the pair + // (PROPOSAL §11.2.4's re-export trap). + let conversations_source = stripped_production_source(&root, "ironclaw_conversations"); + for name in pair { + assert!( + !conversations_source.contains(&format!("pub use ids::{name}")) + && !conversations_source.contains(&format!( + "pub use ironclaw_extension_contracts::external::{name}" + )), + "ironclaw_conversations re-exports {name} — consumers must import it from its owner, \ + `ironclaw_extension_contracts::external`, not through this crate" + ); + } +} + +#[test] +fn the_attachment_ports_and_size_ceilings_live_in_the_attachments_crate() { + let root = workspace_root(); + let attachments = declared_names(&root, "ironclaw_attachments"); + let product = declared_names(&root, "ironclaw_product"); + + // The ports and the advertised-ceiling DTO belong to the crate that owns + // the landing routine (§6.4.9). + for name in [ + "InboundAttachmentLander", + "InboundAttachmentReader", + "AttachmentCleanupReport", + "AttachmentBudgets", + "AttachmentCapabilities", + "ProjectScopedAttachmentLander", + ] { + assert!( + attachments.contains(name), + "{name} must be declared in ironclaw_attachments (PROPOSAL §6.4.9: the landing \ + routine plus its ports, and one home for the size ceilings)" + ); + assert!( + !product.contains(name), + "{name} is declared in ironclaw_product again — that is the three-crate spread \ + CHECKLIST WS5's `attachments widened` row closed. A transport that advertises a \ + ceiling and the routine that enforces it must read the same constant" + ); + } + + // The builder is a free function, so the type walk cannot see it. + let attachments_source = stripped_production_source(&root, "ironclaw_attachments"); + assert!( + attachments_source.contains("pub fn attachment_capabilities"), + "ironclaw_attachments must declare `attachment_capabilities()` — the advertised half of \ + the ceilings it enforces" + ); + let product_source = stripped_production_source(&root, "ironclaw_product"); + assert!( + !product_source.contains("fn product_attachment_capabilities"), + "ironclaw_product declares `product_attachment_capabilities()` again — it moved to \ + ironclaw_attachments as `attachment_capabilities()`" + ); + + // 3c — the reader adapter deliberately stays in product. See the module doc. + assert!( + product.contains("ProjectScopedAttachmentReader"), + "ProjectScopedAttachmentReader must stay declared in ironclaw_product: it also \ + implements ironclaw_loop_host::LoopAttachmentReadPort, and ironclaw_attachments is a \ + `substrates` crate that may not depend on the `loops` layer — moving it would either \ + invert a layer edge or orphan that impl" + ); + assert!( + !attachments.contains("ProjectScopedAttachmentReader"), + "ProjectScopedAttachmentReader moved into ironclaw_attachments — check whether that \ + crate now depends on ironclaw_loop_host, which the layer matrix forbids" + ); + let attachments_manifest = + std::fs::read_to_string(root.join("crates/ironclaw_attachments/Cargo.toml")) + .expect("ironclaw_attachments must have a manifest"); + assert!( + !attachments_manifest.contains("ironclaw_loop_host"), + "ironclaw_attachments must not depend on ironclaw_loop_host: `substrates` may not name \ + `loops`" + ); +} + +/// Self-test for `ratchet_support::strip_comments_and_strings` as this gate +/// uses it: the strip must remove prose and literals but keep code, or every +/// assertion above that reads stripped source is measuring the wrong thing. +/// The shared lexer also handles the shapes a local line-based copy would miss +/// — nested block comments, raw strings, char literals — so those are pinned +/// here too rather than left to the other gates that share it. +#[test] +fn the_source_strip_removes_prose_and_literals_but_not_code() { + let stripped = strip_comments_and_strings( + r##" +/// fn conversation_actor_ref in a doc comment +// fn conversation_actor_ref in a line comment +/* fn conversation_actor_ref in a block comment + /* fn conversation_actor_ref in a NESTED block comment */ +*/ +fn real_code() -> &'static str { "fn conversation_actor_ref in a literal" } +fn raw_literal() -> &'static str { r#"fn conversation_actor_ref in a raw literal"# } +fn char_literal() -> char { '"' } +pub fn attachment_capabilities() -> u8 { 0 } // trailing fn conversation_actor_ref +"##, + ); + assert!( + !stripped.contains("fn conversation_actor_ref"), + "prose and literals must not survive the strip: {stripped}" + ); + assert!(stripped.contains("fn real_code"), "code must survive"); + assert!( + stripped.contains("fn raw_literal") && stripped.contains("fn char_literal"), + "a raw string and a char literal must not swallow the code around them: {stripped}" + ); + assert!( + stripped.contains("pub fn attachment_capabilities"), + "code before a trailing comment must survive" + ); +} + +/// Self-test for the declaration walk: a name only *mentioned* is not +/// discovered, and one actually declared is. +#[test] +fn the_declaration_walk_finds_declarations_not_mentions() { + let root = workspace_root(); + let conversations = declared_names(&root, "ironclaw_conversations"); + assert!( + conversations.contains("InboundTurnService"), + "a type ironclaw_conversations really declares must be discovered" + ); + assert!( + !conversations.contains("ThreadScope"), + "ThreadScope is only *named* by ironclaw_conversations' dependencies, never declared \ + there — if the walk reports it, the walk is matching references" + ); +} diff --git a/crates/ironclaw_architecture/tests/reborn_extension_contract_location_scan.rs b/crates/ironclaw_architecture/tests/reborn_extension_contract_location_scan.rs index 54b689a3267..0e90ee43e94 100644 --- a/crates/ironclaw_architecture/tests/reborn_extension_contract_location_scan.rs +++ b/crates/ironclaw_architecture/tests/reborn_extension_contract_location_scan.rs @@ -103,11 +103,15 @@ const FROZEN_CONTRACT_NAMES: &[&str] = &[ /// Names that are governed but whose *definition* the one-home half must not /// police, because an identically-named type legitimately exists elsewhere. /// -/// Four entries, all surfaced by WS1.4 when `channel_adapter`, `external`, and -/// `tool_adapter` arrived from `ironclaw_host_api` — where this scan could not -/// see them, because it governs only the owner crate's own definitions. Each is -/// recorded rather than silently passed, and each names the colliding -/// definition: +/// Two entries. Four arrived with WS1.4, when `channel_adapter`, `external`, +/// and `tool_adapter` came over from `ironclaw_host_api` — where this scan +/// could not see them, because it governs only the owner crate's own +/// definitions. **WS5's `conversations`/`threads` naming-trap row discharged +/// two of them**: `ExternalActorRef` and `ExternalConversationRef` no longer +/// have a second definition, because `ironclaw_conversations` now consumes the +/// canonical pair instead of carrying its own copy, so the carve-out was +/// deleted rather than repointed. What remains is recorded rather than silently +/// passed, and names the colliding definition: /// /// - `ToolCall` / `ToolResult` — `ironclaw_llm::provider` (`src/provider.rs`). /// **Not the same concept.** The LLM pair is wire vocabulary: a tool call a @@ -117,22 +121,15 @@ const FROZEN_CONTRACT_NAMES: &[&str] = &[ /// serializes". Two genuinely different types that happen to share the /// obvious English name; renaming either is a §5.1 type-name decision (WS10), /// not a contracts extraction. -/// - `ExternalActorRef` / `ExternalConversationRef` — `ironclaw_conversations` -/// (`src/ids.rs:48,72`). **The same concept, declared twice**, and this is a -/// real duplicate-surface finding, not a false positive: `conversations::ids` -/// carries a parallel copy of the adapter-identity vocabulary, including -/// `AdapterInstallationId` and `ExternalEventId`, which this walk cannot even -/// see because they are `bounded_string_id!` macro expansions (the same blind -/// spot the module doc records for `LifecyclePackageRef`). Unifying them -/// changes the conversations record grammar — a domain call. Exempted here so -/// the finding is visible and attributable rather than blocking, and so the -/// other two halves of the scan keep their reach. -const COLLISION_EXEMPT: &[&str] = &[ - "ExternalActorRef", - "ExternalConversationRef", - "ToolCall", - "ToolResult", -]; +/// +/// `ironclaw_conversations` still declares `AdapterInstallationId` and +/// `ExternalEventId` beside the canonical pair, and this walk cannot see either: +/// they are `bounded_string_id!` macro expansions, the same blind spot the +/// module doc records for `LifecyclePackageRef`. They are adapter-scoped +/// bounded strings the binding domain owns, not channel refs, so they are a +/// naming overlap rather than a shadow contract — but they are invisible here, +/// which is why the fact is written down instead of assumed. +const COLLISION_EXEMPT: &[&str] = &["ToolCall", "ToolResult"]; /// `macro_rules!` bodies declare types with metavariable names (`pub struct /// $name(String);` — the `bounded_lifecycle_string!` template in diff --git a/crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs b/crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs index cb0dfc0c170..e6541702395 100644 --- a/crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs +++ b/crates/ironclaw_architecture/tests/reborn_struct_test_support_ratchet.rs @@ -453,7 +453,7 @@ const FROZEN_PATH_COUNTS: &[FrozenPathCount] = &[ FrozenPathCount { category: "test-support", item_kind: "method", - path: "crates/ironclaw_product/src/scoped_fs/attachment_landing.rs", + path: "crates/ironclaw_product/src/scoped_fs/attachment_reader.rs", count: 1, }, FrozenPathCount { diff --git a/crates/ironclaw_architecture/tests/reborn_transport_product_boundary.rs b/crates/ironclaw_architecture/tests/reborn_transport_product_boundary.rs index 0945b3a96fc..9f8c5646ec6 100644 --- a/crates/ironclaw_architecture/tests/reborn_transport_product_boundary.rs +++ b/crates/ironclaw_architecture/tests/reborn_transport_product_boundary.rs @@ -60,16 +60,6 @@ const OPENAI_COMPAT: &str = "ironclaw_reborn_openai_compat"; /// `reborn_dependency_boundaries.rs`). const PRODUCT_SYMBOLS_WEBUI_STILL_NAMES: &[(&str, &str)] = &[ // --- contract-purity residue ------------------------------------------ - ( - "ProductAttachmentCapabilities", - "contract-purity: `budgets` is `ironclaw_attachments::AttachmentBudgets`; \ - CHECKLIST WS5 `attachments widened … one home for size ceilings` owns the move", - ), - ( - "product_attachment_capabilities", - "contract-purity: builds ProductAttachmentCapabilities from \ - ironclaw_attachments + ironclaw_common::accept_tokens", - ), ( "RebornCreateThreadResponse", "contract-purity: carries ironclaw_threads::SessionThreadRecord", @@ -214,7 +204,12 @@ const PRODUCT_SYMBOLS_OPENAI_COMPAT_STILL_NAMES: &[(&str, &str)] = &[ ]; /// Ceilings on the two residues. Only ever move down. -const WEBUI_PRODUCT_SYMBOL_BASELINE: usize = 102; +// 102 when the WS5 transport inversion landed; **100** after the WS5 +// `attachments widened` row moved `ProductAttachmentCapabilities` / +// `product_attachment_capabilities` to `ironclaw_attachments` as +// `AttachmentCapabilities` / `attachment_capabilities` — the two symbols this +// list called out by name as that row's to own. Shrink-only. +const WEBUI_PRODUCT_SYMBOL_BASELINE: usize = 100; const OPENAI_COMPAT_PRODUCT_SYMBOL_BASELINE: usize = 3; /// Boundary vocabulary this row moved: declared in `ironclaw_product_contracts` diff --git a/crates/ironclaw_attachments/Cargo.toml b/crates/ironclaw_attachments/Cargo.toml index 5fe7e17b0f0..f25f742b373 100644 --- a/crates/ironclaw_attachments/Cargo.toml +++ b/crates/ironclaw_attachments/Cargo.toml @@ -14,16 +14,30 @@ publish = false layer = "substrates" [dependencies] +async-trait = "0.1" +chrono = { version = "0.4", features = ["serde"] } ironclaw_common = { path = "../ironclaw_common", version = "0.4.2" } ironclaw_extractors = { path = "../ironclaw_extractors", version = "0.1.0" } ironclaw_filesystem = { path = "../ironclaw_filesystem", version = "0.1.0" } ironclaw_host_api = { path = "../ironclaw_host_api", version = "0.1.0" } +# The landing/read ports error with `ProductSurfaceError`: their caller is a +# product surface and the WebUI bytes endpoint maps the error code straight onto +# an HTTP status. `ironclaw_product_contracts` is the neutral product-tier +# contract crate and sits in the `contracts` layer, so this is a downward edge +# like `host_api` — never a dependency on `ironclaw_product` itself. +ironclaw_product_contracts = { path = "../ironclaw_product_contracts", version = "0.1.0" } +# `ThreadScope` — the ports are scoped per canonical thread. +ironclaw_threads = { path = "../ironclaw_threads", version = "0.1.0" } serde = { version = "1", features = ["derive"] } sha2 = "0.11" thiserror = "2" tracing = "0.1" [dev-dependencies] +ironclaw_filesystem = { path = "../ironclaw_filesystem", version = "0.1.0", features = [ + "test-support", +] } +serde_json = "1" tokio = { version = "1", features = ["macros", "rt-multi-thread"] } [lints] diff --git a/crates/ironclaw_attachments/src/budgets.rs b/crates/ironclaw_attachments/src/budgets.rs index 5193f8b8ecc..c722a040d87 100644 --- a/crates/ironclaw_attachments/src/budgets.rs +++ b/crates/ironclaw_attachments/src/budgets.rs @@ -16,10 +16,81 @@ pub const DEFAULT_ATTACHMENT_BUDGETS: AttachmentBudgets = AttachmentBudgets { max_total_bytes: 10 * 1024 * 1024, }; +/// Browser-facing inline-attachment contract. +/// +/// Carries the `accept` tokens generated from the shared +/// [`ironclaw_common`] format registry (so a file picker can never drift from +/// the server's allowed MIME set) plus the budgets the server-side decode +/// enforces. A surface uses this only for pre-submit hints; the server-side +/// decode remains the sole authority on what is accepted. +/// +/// It lives beside [`AttachmentBudgets`] because this crate is the one home for +/// attachment size ceilings (PROPOSAL §6.4.9): a transport that advertises a +/// ceiling and the routine that enforces it must read the same constant. +#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +pub struct AttachmentCapabilities { + /// HTML file-input `accept` tokens from the shared registry: exact MIME + /// types plus extensions, e.g. `["image/png", ".png", "application/pdf", + /// ".pdf"]` — never `image/*` wildcards (which would advertise unsupported + /// formats, and which break folder navigation in the native macOS picker). + pub accept: Vec, + /// The count/byte budgets the decode enforces. Flattened, so the wire shape + /// is unchanged and a new budget field reaches the browser without an + /// intermediate edit here. + #[serde(flatten)] + pub budgets: AttachmentBudgets, +} + +/// The inline-attachment contract advertised to browsers. Generated from the +/// shared format registry and the budgets the decode enforces, so the picker +/// and the server stay in lockstep by construction. +pub fn attachment_capabilities() -> AttachmentCapabilities { + AttachmentCapabilities { + accept: ironclaw_common::accept_tokens(), + budgets: DEFAULT_ATTACHMENT_BUDGETS, + } +} + #[cfg(test)] mod tests { use super::*; + #[test] + fn advertised_capabilities_carry_the_enforced_budgets_and_registry_tokens() { + let advertised = attachment_capabilities(); + assert_eq!( + advertised.budgets, DEFAULT_ATTACHMENT_BUDGETS, + "the advertised ceiling must be the enforced ceiling" + ); + assert_eq!( + advertised.accept, + ironclaw_common::accept_tokens(), + "accept tokens come from the shared registry, never a local list" + ); + assert!( + !advertised.accept.iter().any(|token| token.contains('*')), + "wildcards would advertise unsupported formats: {:?}", + advertised.accept + ); + } + + /// The budgets are `#[serde(flatten)]`ed, so the browser sees one flat + /// object. A nested `budgets` key would silently break every client that + /// reads `max_file_bytes` at the top level. + #[test] + fn advertised_capabilities_serialize_the_budgets_flat() { + let json = serde_json::to_value(attachment_capabilities()).expect("serialize"); + let object = json.as_object().expect("object"); + assert!(object.contains_key("accept")); + assert!(object.contains_key("max_count")); + assert!(object.contains_key("max_file_bytes")); + assert!(object.contains_key("max_total_bytes")); + assert!( + !object.contains_key("budgets"), + "budgets must stay flattened" + ); + } + #[test] fn default_budgets_match_webui_contract() { assert_eq!(DEFAULT_ATTACHMENT_BUDGETS.max_count, 10); diff --git a/crates/ironclaw_attachments/src/lib.rs b/crates/ironclaw_attachments/src/lib.rs index d9024c6b68d..9d4826d38f9 100644 --- a/crates/ironclaw_attachments/src/lib.rs +++ b/crates/ironclaw_attachments/src/lib.rs @@ -20,14 +20,20 @@ mod budgets; mod inbound; mod landing; +mod ports; +mod project_scoped; /// Canonical project-workspace mount alias used by attachment landing and /// scoped file reads. pub const WORKSPACE_ALIAS: &str = "/workspace"; -pub use budgets::{AttachmentBudgets, DEFAULT_ATTACHMENT_BUDGETS}; +pub use budgets::{ + AttachmentBudgets, AttachmentCapabilities, DEFAULT_ATTACHMENT_BUDGETS, attachment_capabilities, +}; pub use inbound::land_inbound_attachments; pub use landing::{ ATTACHMENTS_DIR, AttachmentLanding, AttachmentLandingError, DEFAULT_MAX_ATTACHMENT_BYTES, attachment_batch_scoped_path, attachment_scoped_path, land_attachment, }; +pub use ports::{AttachmentCleanupReport, InboundAttachmentLander, InboundAttachmentReader}; +pub use project_scoped::ProjectScopedAttachmentLander; diff --git a/crates/ironclaw_attachments/src/ports.rs b/crates/ironclaw_attachments/src/ports.rs new file mode 100644 index 00000000000..bfe1b5a682a --- /dev/null +++ b/crates/ironclaw_attachments/src/ports.rs @@ -0,0 +1,319 @@ +//! The attachment landing/read ports. +//! +//! These are declared here, next to the landing routine they front, rather than +//! in `ironclaw_product` (PROPOSAL §6.4.9, "the single landing routine **plus +//! its ports**" — ending the three-crate spread where the contract, the routine +//! and the adapter each lived in a different crate). The implementations stay +//! wherever the behavior does: [`crate::ProjectScopedAttachmentLander`] is the +//! default one, over a project-scoped `ScopedFilesystem`. +//! +//! They error with [`ProductSurfaceError`] because the caller that fails is a +//! product surface — the WebUI bytes endpoint maps +//! [`ProductSurfaceErrorCode::NotFound`](ironclaw_product_contracts::surface::ProductSurfaceErrorCode) +//! straight onto its 404. Narrowing the ports onto an attachment-owned error +//! would move that HTTP status mapping, which is a behavior change and belongs +//! to its own slice; `ironclaw_product_contracts` is the *neutral* product-tier +//! contract crate and sits in the `contracts` layer, so naming it here is a +//! downward edge like `host_api`. + +use async_trait::async_trait; +use ironclaw_common::AttachmentRef; +use ironclaw_host_api::attachment::InboundAttachment; +use ironclaw_product_contracts::surface::ProductSurfaceError; +use ironclaw_threads::ThreadScope; + +/// What one [`InboundAttachmentLander::cleanup_stale`] pass reconciled. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct AttachmentCleanupReport { + pub scanned_batches: usize, + pub deleted_batches: usize, +} + +/// Lands inbound attachment bytes into durable, agent-accessible storage and +/// returns the transcript references to persist on the user message. +/// +/// Injected by host composition, which owns the project-scoped filesystem +/// authority. `message_id` is a stable per-message id (the idempotency key) +/// used only to disambiguate the storage path; the implementation writes +/// through the same `MountView` the agent's file tools resolve through, so +/// landed bytes are readable by `file_read`/`list_dir` in later turns. +#[async_trait] +pub trait InboundAttachmentLander: Send + Sync { + async fn land( + &self, + thread_scope: &ThreadScope, + message_id: &str, + attachments: Vec, + ) -> Result, ProductSurfaceError>; + + /// Remove one complete batch previously returned by [`Self::land`]. + /// + /// The inbound workflow calls this only when durable message acceptance + /// fails after landing. Implementations must constrain deletion to the + /// batch represented by `attachments`; they must never sweep unrelated + /// workspace paths. + async fn rollback( + &self, + thread_scope: &ThreadScope, + attachments: &[AttachmentRef], + ) -> Result<(), ProductSurfaceError>; + + /// Reconcile old committed batches against an exhaustive set of durable + /// attachment storage keys for this exact thread scope. + /// + /// Callers must skip this operation when their reference scan was + /// truncated. The complete snapshot may include attachment domains the + /// implementation does not own, such as agent-created outbound workspace + /// files; implementations ignore those references and fail closed when no + /// owned reference proves the snapshot usable. Implementations keep a + /// reconciliation window and bounded filesystem scan so recent in-flight + /// work and unrelated workspace paths are never removed. + async fn cleanup_stale( + &self, + thread_scope: &ThreadScope, + referenced_storage_keys: &[String], + ) -> Result; +} + +/// Reads a landed attachment's bytes back for the WebUI bytes endpoint. The +/// read counterpart of [`InboundAttachmentLander`]: host composition implements +/// it over the same project-scoped workspace filesystem the lander wrote +/// through, so `storage_key` is re-scoped through that mount authority and never +/// treated as a host path. +#[async_trait] +pub trait InboundAttachmentReader: Send + Sync { + async fn read( + &self, + thread_scope: &ThreadScope, + storage_key: &str, + ) -> Result, ProductSurfaceError>; +} + +#[cfg(test)] +mod tests { + use std::sync::Arc; + use std::sync::Mutex; + + use ironclaw_host_api::ids::{AgentId, TenantId, UserId}; + use ironclaw_product_contracts::surface::{ProductSurfaceErrorCode, ProductSurfaceErrorKind}; + + use super::*; + + /// A named function, not a closure, so the mapping the double uses is the + /// same shape production code would extract and test. + fn not_found() -> ProductSurfaceError { + ProductSurfaceError { + code: ProductSurfaceErrorCode::NotFound, + kind: ProductSurfaceErrorKind::NotFound, + status_code: 404, + retryable: false, + field: None, + validation_code: None, + } + } + + fn scope(agent: &str) -> ThreadScope { + ThreadScope { + tenant_id: TenantId::new("tenant-1").expect("tenant"), + agent_id: AgentId::new(agent).expect("agent"), + project_id: None, + owner_user_id: Some(UserId::new("user-1").expect("user")), + mission_id: None, + } + } + + fn attachment_ref(storage_key: &str) -> AttachmentRef { + AttachmentRef { + id: storage_key.to_string(), + kind: ironclaw_common::AttachmentKind::Document, + mime_type: "text/plain".to_string(), + filename: Some("a.txt".to_string()), + size_bytes: Some(1), + storage_key: Some(storage_key.to_string()), + extracted_text: None, + } + } + + /// Records every argument each port method receives, and answers from a + /// per-`(scope, key)` table — so an implementation that ignored the scope + /// (or the message id) could not satisfy the assertions below. + #[derive(Default)] + struct RecordingAttachments { + landed: Mutex>, + rolled_back: Mutex)>>, + swept: Mutex)>>, + readable: Mutex)>>, + } + + #[async_trait] + impl InboundAttachmentLander for RecordingAttachments { + async fn land( + &self, + thread_scope: &ThreadScope, + message_id: &str, + attachments: Vec, + ) -> Result, ProductSurfaceError> { + self.landed.lock().expect("lock").push(( + thread_scope.clone(), + message_id.to_string(), + attachments.len(), + )); + Ok(attachments + .iter() + .enumerate() + .map(|(index, _)| attachment_ref(&format!("/workspace/{message_id}/{index}"))) + .collect()) + } + + async fn rollback( + &self, + thread_scope: &ThreadScope, + attachments: &[AttachmentRef], + ) -> Result<(), ProductSurfaceError> { + self.rolled_back.lock().expect("lock").push(( + thread_scope.clone(), + attachments + .iter() + .filter_map(|a| a.storage_key.clone()) + .collect(), + )); + Ok(()) + } + + async fn cleanup_stale( + &self, + thread_scope: &ThreadScope, + referenced_storage_keys: &[String], + ) -> Result { + self.swept + .lock() + .expect("lock") + .push((thread_scope.clone(), referenced_storage_keys.to_vec())); + Ok(AttachmentCleanupReport { + scanned_batches: referenced_storage_keys.len(), + deleted_batches: 0, + }) + } + } + + #[async_trait] + impl InboundAttachmentReader for RecordingAttachments { + async fn read( + &self, + thread_scope: &ThreadScope, + storage_key: &str, + ) -> Result, ProductSurfaceError> { + self.readable + .lock() + .expect("lock") + .iter() + .find(|(scope, key, _)| scope == thread_scope && key == storage_key) + .map(|(_, _, bytes)| bytes.clone()) + .ok_or_else(not_found) + } + } + + /// Both ports are held as `Arc` by the product surface and by + /// composition; a non-dyn-compatible signature would only fail at the far + /// call site, so it is pinned here at the declaration. + #[test] + fn both_ports_are_usable_as_trait_objects() { + let shared = Arc::new(RecordingAttachments::default()); + let _lander: Arc = shared.clone(); + let _reader: Arc = shared; + } + + #[tokio::test] + async fn the_lander_port_hands_the_implementation_its_scope_and_message_id() { + let lander = RecordingAttachments::default(); + let scope_a = scope("agent-a"); + let refs = lander + .land( + &scope_a, + "message-7", + vec![InboundAttachment { + id: "att-1".to_string(), + mime_type: "text/plain".to_string(), + filename: Some("a.txt".to_string()), + bytes: vec![1], + }], + ) + .await + .expect("land"); + + let landed = lander.landed.lock().expect("lock").clone(); + assert_eq!(landed, vec![(scope_a.clone(), "message-7".to_string(), 1)]); + assert_eq!(refs.len(), 1); + let storage_key = refs[0] + .storage_key + .as_deref() + .expect("a landed ref carries its storage key"); + assert!( + storage_key.contains("message-7"), + "the message id must reach the storage path: {storage_key}" + ); + + lander + .rollback(&scope_a, &refs) + .await + .expect("rollback the batch just landed"); + let rolled = lander.rolled_back.lock().expect("lock").clone(); + assert_eq!( + rolled, + vec![(scope_a, vec![storage_key.to_string()])], + "rollback receives exactly the batch `land` returned, scope included" + ); + } + + #[tokio::test] + async fn the_cleanup_port_hands_the_implementation_the_reference_snapshot() { + let lander = RecordingAttachments::default(); + let scope_a = scope("agent-a"); + let report = lander + .cleanup_stale(&scope_a, &["/workspace/x".to_string()]) + .await + .expect("cleanup"); + assert_eq!(report.scanned_batches, 1); + assert_eq!( + lander.swept.lock().expect("lock").clone(), + vec![(scope_a, vec!["/workspace/x".to_string()])] + ); + } + + /// The read port carries the scope so one thread's storage key cannot serve + /// another's bytes. Asserted in both directions against a double that keys + /// on `(scope, key)` — a double that ignored the scope would pass a + /// one-directional check. + #[tokio::test] + async fn the_read_port_answer_can_differ_by_thread_scope() { + let reader = RecordingAttachments::default(); + let scope_a = scope("agent-a"); + let scope_b = scope("agent-b"); + reader.readable.lock().expect("lock").push(( + scope_a.clone(), + "/workspace/k".to_string(), + vec![7], + )); + + assert_eq!( + reader.read(&scope_a, "/workspace/k").await.expect("read"), + vec![7] + ); + let denied = reader + .read(&scope_b, "/workspace/k") + .await + .expect_err("a different scope must not resolve the same key"); + assert_eq!(denied.code, ProductSurfaceErrorCode::NotFound); + + reader.readable.lock().expect("lock").push(( + scope_b.clone(), + "/workspace/k".to_string(), + vec![9], + )); + assert_eq!( + reader.read(&scope_b, "/workspace/k").await.expect("read"), + vec![9], + "and the other direction: the same key under its own scope resolves" + ); + } +} diff --git a/crates/ironclaw_product/src/scoped_fs/attachment_landing.rs b/crates/ironclaw_attachments/src/project_scoped.rs similarity index 72% rename from crates/ironclaw_product/src/scoped_fs/attachment_landing.rs rename to crates/ironclaw_attachments/src/project_scoped.rs index 66062ddbf15..98b09a46d2f 100644 --- a/crates/ironclaw_product/src/scoped_fs/attachment_landing.rs +++ b/crates/ironclaw_attachments/src/project_scoped.rs @@ -1,27 +1,32 @@ -//! Project-scoped inbound attachment landing for product adapters. +//! The default [`InboundAttachmentLander`] over a project-scoped workspace. //! -//! Implements the [`InboundAttachmentLander`] port the facade calls before -//! accepting a user message: it writes attachment bytes through the -//! project-scoped workspace [`ScopedFilesystem`] — the same filesystem -//! authority the agent's file tools resolve through — and returns the -//! transcript references to persist. Going through that one authority is what -//! makes a landed attachment readable by `file_read`/`list_dir` at the recorded -//! `storage_key` in this and later turns. +//! It writes attachment bytes through the project-scoped [`ScopedFilesystem`] — +//! the same filesystem authority the agent's file tools resolve through — and +//! returns the transcript references to persist. Going through that one +//! authority is what makes a landed attachment readable by `file_read` / +//! `list_dir` at the recorded `storage_key` in this and later turns. +//! +//! The read counterpart (`ProjectScopedAttachmentReader`) stays in +//! `ironclaw_product`: it also implements `ironclaw_loop_host`'s +//! `LoopAttachmentReadPort`, and `ironclaw_loop_host` is a `loops`-layer crate +//! this substrate may not name. See PROPOSAL §6.4.9 and the WS5 `attachments` +//! CHECKLIST row. +//! +//! [`ScopedFilesystem`]: ironclaw_filesystem::ScopedFilesystem use std::{collections::HashSet, sync::Arc}; -use crate::{AttachmentCleanupReport, InboundAttachmentLander, InboundAttachmentReader}; use async_trait::async_trait; -use ironclaw_attachments::{DEFAULT_MAX_ATTACHMENT_BYTES, land_inbound_attachments}; +use ironclaw_common::AttachmentRef; use ironclaw_filesystem::{FileType, FilesystemError, RootFilesystem, ScopedFilesystem}; -use ironclaw_host_api::{attachment::InboundAttachment, path::ScopedPath, resource::ResourceScope}; -use ironclaw_loop_host::{LoopAttachmentReadError, LoopAttachmentReadPort}; -use ironclaw_product_contracts::surface::{ - ProductSurfaceError, ProductSurfaceErrorCode, ProductSurfaceErrorKind, -}; -use ironclaw_threads::{AttachmentRef, ThreadScope}; +use ironclaw_host_api::{attachment::InboundAttachment, path::ScopedPath}; +use ironclaw_product_contracts::surface::ProductSurfaceError; +use ironclaw_threads::ThreadScope; -use ironclaw_attachments::WORKSPACE_ALIAS; +use crate::{ + AttachmentCleanupReport, DEFAULT_MAX_ATTACHMENT_BYTES, InboundAttachmentLander, + WORKSPACE_ALIAS, land_inbound_attachments, +}; const STALE_RECONCILIATION_DAYS: i64 = 1; const STALE_CLEANUP_MAX_DATE_DIRS: usize = 32; @@ -48,108 +53,6 @@ impl ProjectScopedAttachmentLander { } } -/// Reads landed attachment bytes back through the same project-scoped workspace -/// filesystem, so the loop model port can build multimodal image parts for a -/// vision-capable model. The read re-scopes `storage_key` through the -/// `MountView` authority (it is never treated as a host path), and is bounded so -/// a corrupt/oversized key can't materialize unbounded bytes. -pub struct ProjectScopedAttachmentReader { - filesystem: Arc>, - max_bytes: usize, -} - -impl ProjectScopedAttachmentReader { - pub fn new(filesystem: Arc>) -> Self { - Self { - filesystem, - max_bytes: DEFAULT_MAX_ATTACHMENT_BYTES, - } - } - - /// Construct a reader with an explicit read ceiling. Test-only: production - /// always uses [`DEFAULT_MAX_ATTACHMENT_BYTES`] via [`Self::new`], but the - /// oversized branch is only reachable in a test with a tiny ceiling. - #[cfg(test)] - fn with_max_bytes(filesystem: Arc>, max_bytes: usize) -> Self { - Self { - filesystem, - max_bytes, - } - } -} - -#[async_trait] -impl LoopAttachmentReadPort for ProjectScopedAttachmentReader { - async fn read_attachment_bytes( - &self, - scope: &ResourceScope, - storage_key: &str, - ) -> Result, LoopAttachmentReadError> { - let path = ScopedPath::new(storage_key.to_string()) - .map_err(|error| LoopAttachmentReadError::Backend(error.to_string()))?; - match self - .filesystem - .read_bytes_bounded(scope, &path, self.max_bytes) - .await - { - Ok(Some(bytes)) => Ok(bytes), - // `read_bytes_bounded` returns `Ok(None)` only when the file is - // larger than `max_bytes` — an oversized attachment we refuse to - // materialize, not a missing one. - Ok(None) => Err(LoopAttachmentReadError::Backend(format!( - "attachment exceeds the {}-byte read limit", - self.max_bytes - ))), - Err(FilesystemError::NotFound { .. }) => Err(LoopAttachmentReadError::NotFound), - Err(FilesystemError::PermissionDenied { .. }) => { - Err(LoopAttachmentReadError::Forbidden) - } - Err(error) => Err(LoopAttachmentReadError::Backend(error.to_string())), - } - } -} - -/// Read counterpart wired into the product surface so the bytes endpoint can serve -/// image thumbnails. It reuses the loop read port — the same bounded, -/// `MountView`-re-scoped read — and translates the scope and error taxonomy to -/// the product API surface. A missing/oversized/forbidden read becomes a sanitized -/// product error rather than leaking a host path or backend string. -#[async_trait] -impl InboundAttachmentReader for ProjectScopedAttachmentReader { - async fn read( - &self, - thread_scope: &ThreadScope, - storage_key: &str, - ) -> Result, ProductSurfaceError> { - let scope = thread_scope.to_resource_scope(); - self.read_attachment_bytes(&scope, storage_key) - .await - .map_err(|error| match error { - LoopAttachmentReadError::NotFound => ProductSurfaceError { - code: ProductSurfaceErrorCode::NotFound, - kind: ProductSurfaceErrorKind::NotFound, - status_code: 404, - retryable: false, - field: None, - validation_code: None, - }, - LoopAttachmentReadError::Forbidden => ProductSurfaceError { - code: ProductSurfaceErrorCode::Forbidden, - kind: ProductSurfaceErrorKind::ParticipantDenied, - status_code: 403, - retryable: false, - field: None, - validation_code: None, - }, - // Carry the cause to the log (sanitized 500 on the wire) rather - // than dropping it — see error-handling rule. - LoopAttachmentReadError::Backend(reason) => { - ProductSurfaceError::internal_from(reason) - } - }) - } -} - #[async_trait] impl InboundAttachmentLander for ProjectScopedAttachmentLander { async fn land( @@ -365,6 +268,7 @@ fn workspace_attachment_root() -> Result { mod tests { use super::*; + use ironclaw_common::AttachmentKind; use ironclaw_filesystem::InMemoryBackend; use ironclaw_host_api::{ ids::{AgentId, TenantId, UserId}, @@ -482,79 +386,99 @@ mod tests { )); } + /// Rollback deletes a whole batch directory, so it must refuse any + /// reference it cannot prove names one — otherwise a malformed + /// `storage_key` picks the delete target. Each row is a distinct + /// rejection: the first five are the guards in `attachment_batch_parent` + /// (no parent separator, outside the attachment root, then the three + /// batch-shape conditions), the last is the loop in `rollback` itself + /// meeting a later reference that was never landed. + /// + /// Driven through `rollback` rather than the helper directly, because + /// `rollback` is what decides whether a delete happens. `internal_from` + /// deliberately collapses every case to a sanitized `Internal` code, so + /// the input is what distinguishes them — and the assertion that matters + /// is that none of them reaches the delete. #[tokio::test] - async fn reader_reads_back_landed_attachment_bytes() { - // The reader is the producer side of the image-vision path: it must read - // back exactly what the lander wrote under the same workspace mount. - let fs = workspace_fs(MountPermissions::read_write()); + async fn rollback_refuses_malformed_batch_references() { + let fs = workspace_fs(MountPermissions::read_write_list_delete()); let lander = ProjectScopedAttachmentLander::new(Arc::clone(&fs)); - let refs = lander - .land( - &thread_scope(), - "msg1", - vec![InboundAttachment { - id: "att-0".to_string(), - mime_type: "image/png".to_string(), - filename: Some("diagram.png".to_string()), - bytes: vec![1, 2, 3, 4], - }], - ) - .await - .expect("landing succeeds through a read-write workspace mount"); - let storage_key = refs[0].storage_key.as_deref().expect("storage_key set"); - - let reader = ProjectScopedAttachmentReader::new(Arc::clone(&fs)); - let bytes = reader - .read_attachment_bytes(&thread_scope().to_resource_scope(), storage_key) - .await - .expect("reading back the landed attachment succeeds"); - assert_eq!(bytes, vec![1, 2, 3, 4]); - } - - #[tokio::test] - async fn reader_missing_attachment_maps_to_not_found() { - let reader = - ProjectScopedAttachmentReader::new(workspace_fs(MountPermissions::read_write())); - let err = reader - .read_attachment_bytes( - &thread_scope().to_resource_scope(), - "/workspace/attachments/2026-06-14/m1-0-missing.png", - ) - .await - .expect_err("an absent attachment is a not-found, not bytes"); - assert!(matches!(err, LoopAttachmentReadError::NotFound)); - } + let scope = thread_scope(); - #[tokio::test] - async fn reader_oversized_attachment_is_a_backend_refusal_not_not_found() { - let fs = workspace_fs(MountPermissions::read_write()); - let lander = ProjectScopedAttachmentLander::new(Arc::clone(&fs)); - let refs = lander + // Land a real batch first: if a malformed reference ever fell through + // to the delete, this is what it would destroy. + let landed = lander .land( - &thread_scope(), - "msg1", + &scope, + "keep-me", vec![InboundAttachment { - id: "att-0".to_string(), - mime_type: "image/png".to_string(), - filename: Some("diagram.png".to_string()), - bytes: vec![1, 2, 3, 4], + id: "keep".to_string(), + mime_type: "text/plain".to_string(), + filename: Some("keep.txt".to_string()), + bytes: b"keep".to_vec(), }], ) .await - .expect("landing succeeds through a read-write workspace mount"); - let storage_key = refs[0].storage_key.as_deref().expect("storage_key set"); + .expect("batch lands"); + let landed_path = + ScopedPath::new(landed[0].storage_key.clone().expect("storage key")).unwrap(); - // A 2-byte ceiling rejects the 4-byte attachment. The reader must not - // mislabel an oversized file as `NotFound`. - let reader = ProjectScopedAttachmentReader::with_max_bytes(Arc::clone(&fs), 2); - let err = reader - .read_attachment_bytes(&thread_scope().to_resource_scope(), storage_key) - .await - .expect_err("an oversized attachment is refused"); - match err { - LoopAttachmentReadError::Backend(reason) => assert!(reason.contains("exceeds")), - other => panic!("expected a backend refusal, got {other}"), + let root = workspace_attachment_root() + .expect("attachment root") + .as_str() + .to_string(); + let cases: Vec<(&str, Vec>)> = vec![ + ( + "storage key has no batch parent", + vec![Some("no-separator".to_string())], + ), + ( + "batch parent is outside the attachment root", + vec![Some("/elsewhere/2026-01-01/msg/f.txt".to_string())], + ), + ( + "batch shape: empty filename", + vec![Some(format!("{root}/2026-01-01/msg/"))], + ), + ( + "batch shape: wrong segment count", + vec![Some(format!("{root}/only-one/f.txt"))], + ), + ( + "batch shape: empty segment", + vec![Some(format!("{root}/2026-01-01//f.txt"))], + ), + ( + "a later reference carries no storage key", + vec![Some(format!("{root}/2026-01-01/msg/first.txt")), None], + ), + ]; + + for (case, keys) in cases { + let refs: Vec = keys + .into_iter() + .enumerate() + .map(|(index, storage_key)| AttachmentRef { + id: format!("att-{index}"), + kind: AttachmentKind::Document, + mime_type: "text/plain".to_string(), + filename: Some("f.txt".to_string()), + size_bytes: None, + storage_key, + extracted_text: None, + }) + .collect(); + let err = lander.rollback(&scope, &refs).await.expect_err(case); + assert_eq!(err.code, ProductSurfaceErrorCode::Internal, "{case}"); } + + // Nothing was deleted along the way. + assert!( + fs.stat(&scope.to_resource_scope(), &landed_path) + .await + .is_ok(), + "a refused rollback must not delete an unrelated landed batch" + ); } #[tokio::test] @@ -629,6 +553,13 @@ mod tests { ); } + /// All three of `cleanup_stale`'s pre-scan exits, in one fixture, because + /// they share one guarantee: a snapshot this pass cannot trust deletes + /// nothing. The three differ in how loud they are, and that difference is + /// deliberate — an empty or wholly-unowned snapshot is a legitimate state + /// and returns an empty report, whereas an in-root reference of the wrong + /// depth is a malformed input this reconciler owns and aborts the pass + /// loudly rather than reclaiming against a key it cannot parse. #[tokio::test] async fn stale_cleanup_with_empty_or_unowned_snapshot_deletes_nothing() { let fs = workspace_fs(MountPermissions::read_write_list_delete()); @@ -673,6 +604,23 @@ mod tests { fs.stat(&scope.to_resource_scope(), &path).await.is_ok(), "incomplete cleanup snapshots must not delete the batch" ); + + // An in-root reference whose relative depth is not `//` + // aborts the whole pass. One malformed key means the snapshot cannot be + // read as authoritative, and reclaiming from a partially-understood + // snapshot is how a live batch gets deleted. + let error = lander + .cleanup_stale( + &scope, + &["/workspace/attachments/2026-01-01/flat.txt".to_string()], + ) + .await + .expect_err("an in-root reference of the wrong depth must fail loudly"); + assert_eq!(error.code, ProductSurfaceErrorCode::Internal); + assert!( + fs.stat(&scope.to_resource_scope(), &path).await.is_ok(), + "an aborted pass must not delete anything" + ); } #[tokio::test] diff --git a/crates/ironclaw_conversations/CLAUDE.md b/crates/ironclaw_conversations/CLAUDE.md index eaf2bae4e18..7bdc10efa30 100644 --- a/crates/ironclaw_conversations/CLAUDE.md +++ b/crates/ironclaw_conversations/CLAUDE.md @@ -2,10 +2,10 @@ - Own adapter-safe conversation binding and inbound-turn service contracts only: external actor/conversation refs, source/reply binding refs, participant checks, message acceptance refs, and idempotency semantics. - Do not parse concrete Slack/Telegram/Web/CLI payloads in this crate. Product adapters normalize protocol payloads before calling these services. -- Do not persist raw user or assistant message content in turn-facing records. Use content/message refs; durable transcript content belongs to the SessionThreadService/TranscriptStore storage boundary. +- Do not persist raw user or assistant message content in turn-facing records. Use content/message refs; durable transcript content belongs to the `InboundConversationService`/TranscriptStore storage boundary (the service was named `SessionThreadService` before the WS5 naming-trap fix). - Keep `TurnCoordinator` inputs canonical: `TurnScope`, `TurnActor`, `AcceptedMessageRef`, `SourceBindingRef`, and `ReplyTargetBindingRef`. - Binding resolution must fail closed for unpaired actors, unknown/inaccessible threads, invalid refs, participant-policy denials, tenant/adapter-installation mismatches, and delimiter-like external IDs that could collide if flattened into strings. -- Conversation binding identity excludes per-message external IDs; bind on stable `(space_id, conversation_id, thread_id)` route identity so adapters that include message IDs do not fork canonical threads. +- Conversation binding identity excludes per-message external IDs; bind on stable `(space_id, conversation_id, topic_id)` route identity so adapters that include message IDs do not fork canonical threads. `topic_id` is the *channel's* sub-conversation (Slack thread, Telegram topic) and is never an `ironclaw_host_api::ids::ThreadId` — that collision is the naming trap WS5 removed. `thread_id` in this crate means the canonical thread, with one deliberate exception: the durable record grammar, which still spells the route's topic `thread_id` so a rollback can read it (see `stored_refs`). - Source binding and reply target binding refs are distinct. Egress paths must validate reply targets against the current external actor/thread before sending to external destinations, and validation must preserve adapter kind, adapter installation, and full external route fields. Reply routes are owner-scoped to the exact external actor pairing key unless the adapter explicitly marks the route shared/group; shared markers may widen existing routes only from direct to shared. - Accepted inbound messages must snapshot message-scoped reply targets and route access policy; stable conversation bindings ignore and strip per-message IDs, but egress routing for accepted messages must preserve them. New ingress writes must use the canonical binding-level reply ref, not an older message-scoped reply snapshot. - Accepted inbound message writes must reject mixed source/reply binding refs that do not belong to the same tenant/thread binding and reject external route snapshots whose stable identity differs from the binding. diff --git a/crates/ironclaw_conversations/Cargo.toml b/crates/ironclaw_conversations/Cargo.toml index e84ebca61a2..aed2a772f8b 100644 --- a/crates/ironclaw_conversations/Cargo.toml +++ b/crates/ironclaw_conversations/Cargo.toml @@ -16,6 +16,7 @@ layer = "substrates" [dependencies] async-trait = "0.1" chrono = { version = "0.4", features = ["serde"] } +ironclaw_extension_contracts = { path = "../ironclaw_extension_contracts", version = "0.1.0" } ironclaw_filesystem = { path = "../ironclaw_filesystem", version = "0.1.0" } ironclaw_host_api = { path = "../ironclaw_host_api", version = "0.1.0" } ironclaw_safety = { path = "../ironclaw_safety" } diff --git a/crates/ironclaw_conversations/src/conversation_state_store.rs b/crates/ironclaw_conversations/src/conversation_state_store.rs index b6d19575e25..8b9080ff73a 100644 --- a/crates/ironclaw_conversations/src/conversation_state_store.rs +++ b/crates/ironclaw_conversations/src/conversation_state_store.rs @@ -44,14 +44,14 @@ use ironclaw_turns::{AcceptedMessageRef, IdempotencyKey, SubmitTurnResponse}; use serde::{Deserialize, Serialize}; use crate::{ - AcceptInboundMessageRequest, AcceptedInboundMessage, AcceptedInboundMessageLookup, - AcceptedInboundMessageReplay, AdapterInstallationId, AdapterKind, ConditionalUnpairOutcome, - ConversationActorPairingService, ConversationBindingResolution, ConversationBindingService, - ExpectedExternalActorOwner, ExternalActorBindingEpoch, ExternalActorRef, - ExternalConversationIdentity, InMemoryConversationServices, InboundTurnError, + AcceptConversationMessageRequest, AcceptedConversationMessage, + AcceptedConversationMessageLookup, AcceptedConversationMessageReplay, AdapterInstallationId, + AdapterKind, ConditionalUnpairOutcome, ConversationActorPairingService, + ConversationBindingResolution, ConversationBindingService, ConversationMessageRecord, + ExpectedExternalActorOwner, ExternalActorBindingEpoch, ExternalConversationIdentity, + InMemoryConversationServices, InboundConversationService, InboundTurnError, LinkConversationRequest, LinkedConversationBinding, ReplyTargetBinding, - ResolveConversationRequest, SessionThreadService, ThreadMessageRecord, - ValidateReplyTargetRequest, + ResolveConversationRequest, ValidateReplyTargetRequest, memory::{ AcceptedMessageReplayKey, ActorKey, BindingKey, BindingRecord, ExternalEventRouteKey, InMemoryState, MessageIdempotencyKey, ReplyTargetRecord, StoredAcceptedMessageReplay, @@ -59,6 +59,7 @@ use crate::{ }, state_store::{ConversationStateRepository, PersistedConversationState}, }; +use ironclaw_extension_contracts::external::ExternalActorRef; const STATE_PREFIX: &str = "/conversations"; @@ -119,11 +120,11 @@ struct StoredConversationState { reply_targets: HashMap, threads: Vec<(ThreadKey, ThreadRecord)>, external_event_routes: Vec<(ExternalEventRouteKey, ExternalConversationIdentity)>, - message_idempotency: Vec<(MessageIdempotencyKey, AcceptedInboundMessage)>, + message_idempotency: Vec<(MessageIdempotencyKey, AcceptedConversationMessage)>, message_replays: Vec<(AcceptedMessageReplayKey, StoredAcceptedMessageReplay)>, submission_keys: Vec<(AcceptedMessageRef, IdempotencyKey)>, submitted_message_responses: Vec<(AcceptedMessageRef, SubmitTurnResponse)>, - messages: Vec, + messages: Vec, } impl StoredConversationState { @@ -673,18 +674,18 @@ impl ConversationBindingService for RebornFilesystemConversationServices { } #[async_trait] -impl SessionThreadService for RebornFilesystemConversationServices { +impl InboundConversationService for RebornFilesystemConversationServices { async fn accept_inbound_message( &self, - request: AcceptInboundMessageRequest, - ) -> Result { + request: AcceptConversationMessageRequest, + ) -> Result { self.inner.accept_inbound_message(request).await } async fn replay_accepted_inbound_message( &self, - lookup: AcceptedInboundMessageLookup, - ) -> Result, InboundTurnError> { + lookup: AcceptedConversationMessageLookup, + ) -> Result, InboundTurnError> { self.inner.replay_accepted_inbound_message(lookup).await } diff --git a/crates/ironclaw_conversations/src/ids.rs b/crates/ironclaw_conversations/src/ids.rs index 1509cabe3bd..c1ff54dc443 100644 --- a/crates/ironclaw_conversations/src/ids.rs +++ b/crates/ironclaw_conversations/src/ids.rs @@ -1,3 +1,19 @@ +//! Adapter-scoped identifier vocabulary for conversation binding. +//! +//! The external actor/conversation pair is **not** declared here. It is channel +//! vocabulary, and its one home is +//! [`ironclaw_extension_contracts::external`] — this crate consumes +//! [`ExternalActorRef`] and [`ExternalConversationRef`] from there rather than +//! carrying a parallel copy. What remains here is the vocabulary conversation +//! binding genuinely owns: the adapter-scoped bounded strings, and +//! [`ExternalConversationIdentity`], the *route* projection of a conversation +//! ref (space + conversation + topic, no reply-target hint) that keys durable +//! bindings. +//! +//! [`ExternalActorRef`]: ironclaw_extension_contracts::external::ExternalActorRef + +use ironclaw_extension_contracts::external::ExternalConversationRef; +use ironclaw_host_api::product_adapter_error::ProductAdapterError; use serde::{Deserialize, Serialize}; use crate::InboundTurnError; @@ -44,140 +60,83 @@ bounded_string_id!(AdapterInstallationId, "adapter_installation_id"); bounded_string_id!(ExternalEventId, "external_event_id"); bounded_string_id!(InboundMessageContentRef, "inbound_message_content_ref"); -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize)] -pub struct ExternalActorRef { - kind: String, - id: String, -} - -impl ExternalActorRef { - pub fn new(kind: impl Into, id: impl Into) -> Result { - let kind = kind.into(); - let id = id.into(); - validate_external_id("external_actor_kind", &kind)?; - validate_external_id("external_actor_id", &id)?; - Ok(Self { kind, id }) - } - - pub fn kind(&self) -> &str { - &self.kind - } - - pub fn id(&self) -> &str { - &self.id +/// Translate the channel tier's identifier-validation failure into this +/// crate's error vocabulary. +/// +/// Constructing an [`ExternalActorRef`](ironclaw_extension_contracts::external::ExternalActorRef) +/// or an [`ExternalConversationRef`] now fails with +/// [`ProductAdapterError`] because that pair is owned by +/// `ironclaw_extension_contracts`. Conversation callers still see +/// [`InboundTurnError::InvalidExternalRef`], so the error surface every +/// consumer already matches on is unchanged by the unification. A named +/// function rather than a closure so it is directly testable. +pub(crate) fn map_external_ref_error(error: ProductAdapterError) -> InboundTurnError { + match error { + ProductAdapterError::InvalidIdentifier { kind, reason } => { + InboundTurnError::InvalidExternalRef { kind, reason } + } + other => InboundTurnError::InvalidExternalRef { + kind: "external_ref", + reason: other.to_string(), + }, } } -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize)] -pub struct ExternalConversationRef { - space_id: Option, - conversation_id: String, - thread_id: Option, - message_id: Option, -} - -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize)] +/// The durable *route* identity of an external conversation: everything that +/// distinguishes one route from another, and nothing that varies per event. +/// +/// This is deliberately narrower than [`ExternalConversationRef`], which also +/// carries the per-event `reply_target_message_id`. Bindings are keyed by this +/// projection so two messages in the same conversation resolve to the same +/// binding. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct ExternalConversationIdentity { pub(crate) space_id: Option, pub(crate) conversation_id: String, - pub(crate) thread_id: Option, + /// The external thread/topic within the conversation. Named `topic_id` + /// after the `conversations`/`threads` naming-trap fix: it is the *channel's* + /// sub-conversation, never a canonical [`ThreadId`](ironclaw_host_api::ids::ThreadId). + /// + /// The *record* keeps the released spelling, `thread_id`. Both `Serialize` + /// and `Deserialize` below are hand-written for that reason, and + /// [`crate::stored_refs`] carries the argument: this identity keys + /// `BindingKey`, so a spelling the reader on the other side of a deploy + /// cannot see does not merely misroute a threaded reply — it collides two + /// bindings onto one key and drops the earlier one. + pub(crate) topic_id: Option, } -impl ExternalConversationRef { - pub fn new( - space_id: Option<&str>, - conversation_id: impl Into, - thread_id: Option<&str>, - message_id: Option<&str>, - ) -> Result { - let space_id = space_id.map(str::to_string); - let conversation_id = conversation_id.into(); - let thread_id = thread_id.map(str::to_string); - let message_id = message_id.map(str::to_string); - if let Some(value) = &space_id { - validate_external_id("external_space_id", value)?; - } - validate_external_id("external_conversation_id", &conversation_id)?; - if let Some(value) = &thread_id { - validate_external_id("external_thread_id", value)?; - } - if let Some(value) = &message_id { - validate_external_id("external_message_id", value)?; - } - Ok(Self { - space_id, - conversation_id, - thread_id, - message_id, - }) - } - - pub fn space_id(&self) -> Option<&str> { - self.space_id.as_deref() - } - - pub fn conversation_id(&self) -> &str { - &self.conversation_id - } - - pub fn thread_id(&self) -> Option<&str> { - self.thread_id.as_deref() - } - - pub fn message_id(&self) -> Option<&str> { - self.message_id.as_deref() - } - - pub fn without_message_id(&self) -> Self { +impl ExternalConversationIdentity { + /// Project a conversation ref onto its route identity. + pub(crate) fn from_ref(conversation_ref: &ExternalConversationRef) -> Self { Self { - space_id: self.space_id.clone(), - conversation_id: self.conversation_id.clone(), - thread_id: self.thread_id.clone(), - message_id: None, + space_id: conversation_ref.space_id().map(str::to_string), + conversation_id: conversation_ref.conversation_id().to_string(), + topic_id: conversation_ref.topic_id().map(str::to_string), } } - - pub(crate) fn identity(&self) -> ExternalConversationIdentity { - ExternalConversationIdentity { - space_id: self.space_id.clone(), - conversation_id: self.conversation_id.clone(), - thread_id: self.thread_id.clone(), - } - } - - pub fn conversation_fingerprint(&self) -> String { - length_prefixed_fingerprint(&[ - self.space_id.as_deref().unwrap_or(""), - &self.conversation_id, - self.thread_id.as_deref().unwrap_or(""), - ]) - } } -fn length_prefixed_fingerprint(parts: &[&str]) -> String { - let mut out = String::new(); - for part in parts { - out.push_str(&part.len().to_string()); - out.push(':'); - out.push_str(part); - out.push('|'); - } - out -} - -impl<'de> Deserialize<'de> for ExternalActorRef { - fn deserialize(deserializer: D) -> Result +impl Serialize for ExternalConversationIdentity { + fn serialize(&self, serializer: S) -> Result where - D: serde::Deserializer<'de>, + S: serde::Serializer, { - #[derive(Deserialize)] - struct RawExternalActorRef { - kind: String, - id: String, + /// The durable grammar. Field names here are the *record's*, not the + /// type's — see [`crate::stored_refs`]. + #[derive(Serialize)] + struct StoredExternalConversationIdentity<'a> { + space_id: Option<&'a str>, + conversation_id: &'a str, + thread_id: Option<&'a str>, } - let raw = RawExternalActorRef::deserialize(deserializer)?; - Self::new(raw.kind, raw.id).map_err(serde::de::Error::custom) + StoredExternalConversationIdentity { + space_id: self.space_id.as_deref(), + conversation_id: &self.conversation_id, + thread_id: self.topic_id.as_deref(), + } + .serialize(serializer) } } @@ -190,41 +149,24 @@ impl<'de> Deserialize<'de> for ExternalConversationIdentity { struct RawExternalConversationIdentity { space_id: Option, conversation_id: String, + /// `thread_id` is the durable spelling, written by released builds + /// and by this one; `topic_id` is accepted so a record written by + /// an intermediate build of the rename still loads. + #[serde(alias = "topic_id")] thread_id: Option, } let raw = RawExternalConversationIdentity::deserialize(deserializer)?; + // Re-validate through the canonical constructor so a hand-edited or + // corrupted durable record cannot smuggle an unvalidated identifier + // back into memory. ExternalConversationRef::new( raw.space_id.as_deref(), raw.conversation_id.as_str(), raw.thread_id.as_deref(), None, ) - .map(|conversation_ref| conversation_ref.identity()) - .map_err(serde::de::Error::custom) - } -} - -impl<'de> Deserialize<'de> for ExternalConversationRef { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - #[derive(Deserialize)] - struct RawExternalConversationRef { - space_id: Option, - conversation_id: String, - thread_id: Option, - message_id: Option, - } - - let raw = RawExternalConversationRef::deserialize(deserializer)?; - Self::new( - raw.space_id.as_deref(), - raw.conversation_id, - raw.thread_id.as_deref(), - raw.message_id.as_deref(), - ) + .map(|conversation_ref| Self::from_ref(&conversation_ref)) .map_err(serde::de::Error::custom) } } @@ -253,3 +195,105 @@ pub(crate) fn validate_external_id( } Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use ironclaw_extension_contracts::external::ExternalActorRef; + + fn conversation_ref( + topic: Option<&str>, + reply_target: Option<&str>, + ) -> ExternalConversationRef { + ExternalConversationRef::new(Some("space-1"), "conv-1", topic, reply_target) + .expect("valid conversation ref") + } + + #[test] + fn route_identity_drops_the_reply_target_but_keeps_the_topic() { + let with_reply = conversation_ref(Some("topic-7"), Some("msg-100")); + let without_reply = conversation_ref(Some("topic-7"), None); + assert_eq!( + ExternalConversationIdentity::from_ref(&with_reply), + ExternalConversationIdentity::from_ref(&without_reply), + "the per-event reply target must not split a route" + ); + + let other_topic = conversation_ref(Some("topic-8"), None); + assert_ne!( + ExternalConversationIdentity::from_ref(&without_reply), + ExternalConversationIdentity::from_ref(&other_topic), + "the topic is part of the route identity" + ); + } + + /// Durable binding keys written before the `thread_id` → `topic_id` rename + /// must still resolve to the same route, or every existing external + /// conversation silently re-binds as a fresh conversation-root route. + #[test] + fn legacy_persisted_identity_spelling_loads_as_the_same_route() { + let legacy: ExternalConversationIdentity = serde_json::from_str( + r#"{"space_id":"space-1","conversation_id":"conv-1","thread_id":"topic-7"}"#, + ) + .expect("legacy identity must still deserialize"); + assert_eq!( + legacy, + ExternalConversationIdentity::from_ref(&conversation_ref(Some("topic-7"), None)) + ); + + let current: ExternalConversationIdentity = serde_json::from_str( + r#"{"space_id":"space-1","conversation_id":"conv-1","topic_id":"topic-7"}"#, + ) + .expect("current identity must deserialize"); + assert_eq!(legacy, current); + } + + #[test] + fn identity_deserialization_revalidates_the_conversation_id() { + let result: Result = + serde_json::from_str(r#"{"space_id":null,"conversation_id":"","topic_id":null}"#); + assert!( + result.is_err(), + "an empty conversation id must not survive a durable round trip" + ); + } + + #[test] + fn channel_identifier_failures_keep_this_crate_error_vocabulary() { + let error = ExternalActorRef::new("slack", "", None::) + .map(|_| ()) + .expect_err("an empty actor id is invalid"); + assert!(matches!( + map_external_ref_error(error), + InboundTurnError::InvalidExternalRef { + kind: "external_actor_id", + .. + } + )); + + // The unification widened the input to the whole `ProductAdapterError` + // enum, so the mapper needs a total fallback: anything that is not an + // `InvalidIdentifier` still has to surface as this crate's + // `InvalidExternalRef` rather than escape as a foreign error. The + // generic `kind` is what marks it as the non-identifier path, and the + // source message must survive into `reason` or the caller loses the + // only description of what actually failed. + let non_identifier = ProductAdapterError::EgressUndeclaredHost { + host: "hooks.example.invalid".to_string(), + }; + let expected_reason = non_identifier.to_string(); + match map_external_ref_error(non_identifier) { + InboundTurnError::InvalidExternalRef { kind, reason } => { + assert_eq!( + kind, "external_ref", + "a non-identifier adapter error maps to the generic ref kind" + ); + assert_eq!( + reason, expected_reason, + "the source error's message must be carried through verbatim" + ); + } + other => panic!("expected InvalidExternalRef, got {other:?}"), + } + } +} diff --git a/crates/ironclaw_conversations/src/inbound.rs b/crates/ironclaw_conversations/src/inbound.rs index 77f0304b81b..7a4fb19c933 100644 --- a/crates/ironclaw_conversations/src/inbound.rs +++ b/crates/ironclaw_conversations/src/inbound.rs @@ -13,33 +13,36 @@ use ironclaw_turns::{ TurnCoordinator, TurnError, TurnSurfaceType, }; +use ironclaw_extension_contracts::external::{ExternalActorRef, ExternalConversationRef}; + +use crate::ids::map_external_ref_error; use crate::trusted_trigger::{TrustedTriggerInboundFailureKind, classify_inbound_error}; use crate::types::{TrustedInboundKind, TrustedInboundTurnRequest}; use crate::{ - AcceptInboundMessageRequest, AcceptedInboundMessage, AcceptedInboundMessageLookup, - AdapterInstallationId, AdapterKind, ConversationBindingResolution, ConversationBindingService, - ConversationRouteKind, ExternalActorRef, ExternalConversationRef, ExternalEventId, - InboundMessageContentRef, InboundTurnError, InboundTurnRequest, InboundTurnResponse, - MessageIdempotencyStatus, ResolveConversationRequest, SessionThreadService, + AcceptConversationMessageRequest, AcceptedConversationMessage, + AcceptedConversationMessageLookup, AdapterInstallationId, AdapterKind, + ConversationBindingResolution, ConversationBindingService, ConversationRouteKind, + ExternalEventId, InboundConversationService, InboundMessageContentRef, InboundTurnError, + InboundTurnRequest, InboundTurnResponse, MessageIdempotencyStatus, ResolveConversationRequest, }; #[derive(Clone)] pub struct InboundTurnService { binding_service: B, - session_thread_service: S, + conversation_service: S, turn_coordinator: Arc, } impl InboundTurnService where B: ConversationBindingService, - S: SessionThreadService, + S: InboundConversationService, C: TurnCoordinator + ?Sized, { - pub fn new(binding_service: B, session_thread_service: S, turn_coordinator: Arc) -> Self { + pub fn new(binding_service: B, conversation_service: S, turn_coordinator: Arc) -> Self { Self { binding_service, - session_thread_service, + conversation_service, turn_coordinator, } } @@ -121,7 +124,7 @@ where } })?; - let replay_lookup = AcceptedInboundMessageLookup { + let replay_lookup = AcceptedConversationMessageLookup { tenant_id: tenant_id.clone(), adapter_kind: adapter_kind.clone(), adapter_installation_id: adapter_installation_id.clone(), @@ -130,7 +133,7 @@ where external_event_id: external_event_id.clone(), }; if let Some(replay) = self - .session_thread_service + .conversation_service .replay_accepted_inbound_message(replay_lookup) .await? { @@ -183,8 +186,8 @@ where } }; let accepted_message = self - .session_thread_service - .accept_inbound_message(AcceptInboundMessageRequest { + .conversation_service + .accept_inbound_message(AcceptConversationMessageRequest { tenant_id: resolution.tenant_id.clone(), thread_id: resolution.turn_scope.thread_id.clone(), actor: resolution.actor.clone(), @@ -215,7 +218,7 @@ where async fn submit_or_replay( &self, mut resolution: ConversationBindingResolution, - accepted_message: AcceptedInboundMessage, + accepted_message: AcceptedConversationMessage, classification: ironclaw_turns::product_context::InboundClassification, run_adapter: RunOriginAdapter, surface_type: Option, @@ -224,7 +227,7 @@ where if accepted_message.idempotency == MessageIdempotencyStatus::Duplicate && let Some(turn_submission) = self - .session_thread_service + .conversation_service .inbound_message_turn_submission(&accepted_message.message_ref) .await? { @@ -237,7 +240,7 @@ where } let idempotency_key = self - .session_thread_service + .conversation_service .inbound_message_turn_submission_key(&accepted_message.message_ref) .await?; let turn_submission_result = self @@ -268,14 +271,14 @@ where Ok(response) => response, Err(error) => { if should_rotate_submit_key(&error) { - self.session_thread_service + self.conversation_service .rotate_inbound_message_turn_submission_key(&accepted_message.message_ref) .await?; } return Err(InboundTurnError::TurnSubmissionFailed { error }); } }; - self.session_thread_service + self.conversation_service .mark_inbound_message_turn_submitted( &accepted_message.message_ref, turn_submission.clone(), @@ -300,18 +303,18 @@ pub(crate) struct ConversationTrustedTriggerSubmitter { impl ConversationTrustedTriggerSubmitter where B: ConversationBindingService, - S: SessionThreadService, + S: InboundConversationService, C: TurnCoordinator + ?Sized, { pub(crate) fn new( binding_service: B, - session_thread_service: S, + conversation_service: S, turn_coordinator: Arc, ) -> Self { Self { inbound: InboundTurnService::new( binding_service, - session_thread_service, + conversation_service, turn_coordinator, ), prompt_safety: Arc::new(Sanitizer::new()), @@ -327,17 +330,17 @@ where /// worker, not in this public function. pub fn trusted_trigger_fire_submitter( binding_service: B, - session_thread_service: S, + conversation_service: S, turn_coordinator: Arc, ) -> Arc where B: ConversationBindingService + 'static, - S: SessionThreadService + 'static, + S: InboundConversationService + 'static, C: TurnCoordinator + ?Sized + 'static, { Arc::new(ConversationTrustedTriggerSubmitter::new( binding_service, - session_thread_service, + conversation_service, turn_coordinator, )) } @@ -346,7 +349,7 @@ where impl TrustedTriggerFireSubmitter for ConversationTrustedTriggerSubmitter where B: ConversationBindingService, - S: SessionThreadService, + S: InboundConversationService, C: TurnCoordinator + ?Sized, { async fn submit_trusted_trigger_fire( @@ -386,13 +389,17 @@ fn trusted_inbound_request_from_trigger( external_actor_ref: ExternalActorRef::new( trusted_inbound_binding.external_actor_namespace(), trusted_inbound_binding.external_actor_id(), - )?, + // A trigger fire has no human display name to carry. + None::, + ) + .map_err(map_external_ref_error)?, external_conversation_ref: ExternalConversationRef::new( None, trusted_inbound_binding.external_conversation_id(), Some(trusted_inbound_binding.route_thread_id()), None, - )?, + ) + .map_err(map_external_ref_error)?, external_event_id: ExternalEventId::new(trusted_inbound_binding.external_event_id())?, route_kind: ConversationRouteKind::Direct, content_ref: InboundMessageContentRef::new(content_ref.as_str())?, @@ -556,14 +563,14 @@ mod tests { }; use crate::types::{TrustedInboundKind, TrustedInboundTurnRequest}; use crate::{ - AcceptedInboundMessage, AdapterInstallationId, AdapterKind, ConversationBindingResolution, - ConversationBindingService, ConversationRouteKind, ExternalActorRef, - ExternalConversationRef, ExternalEventId, InMemoryConversationServices, - InboundMessageContentRef, InboundTurnError, InboundTurnRequest, InboundTurnResponse, - InboundTurnService, LinkConversationRequest, LinkedConversationBinding, - MessageIdempotencyStatus, ReplyTargetBinding, ThreadAccessDecision, - ValidateReplyTargetRequest, + AcceptedConversationMessage, AdapterInstallationId, AdapterKind, + ConversationBindingResolution, ConversationBindingService, ConversationRouteKind, + ExternalEventId, InMemoryConversationServices, InboundMessageContentRef, InboundTurnError, + InboundTurnRequest, InboundTurnResponse, InboundTurnService, LinkConversationRequest, + LinkedConversationBinding, MessageIdempotencyStatus, ReplyTargetBinding, + ThreadAccessDecision, ValidateReplyTargetRequest, }; + use ironclaw_extension_contracts::external::{ExternalActorRef, ExternalConversationRef}; #[tokio::test] async fn trusted_inbound_with_real_services_creates_binding_records_message_and_replays_submission() @@ -1074,7 +1081,7 @@ mod tests { reply_target_binding_ref: reply_target_binding_ref.clone(), access: ThreadAccessDecision::Allowed, }, - accepted_message: AcceptedInboundMessage { + accepted_message: AcceptedConversationMessage { tenant_id, thread_id, actor, @@ -1178,7 +1185,12 @@ mod tests { } fn external_actor(value: &str) -> ExternalActorRef { - ExternalActorRef::new(TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, value).unwrap() + ExternalActorRef::new( + TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, + value, + None::, + ) + .unwrap() } fn user(value: &str) -> UserId { @@ -1568,7 +1580,7 @@ mod tests { async fn trusted_non_trigger_adapter_records_inbound_origin() { let slack = AdapterKind::new("slack").unwrap(); let slack_install = AdapterInstallationId::new("slack-install").unwrap(); - let slack_actor = ExternalActorRef::new("slack", "alice").unwrap(); + let slack_actor = ExternalActorRef::new("slack", "alice", None::).unwrap(); let services = InMemoryConversationServices::default(); services .pair_external_actor( @@ -1700,7 +1712,7 @@ mod tests { async fn shared_route_kind_records_channel_surface_type() { let slack = AdapterKind::new("slack").unwrap(); let slack_install = AdapterInstallationId::new("slack-install").unwrap(); - let slack_actor = ExternalActorRef::new("slack", "user-alice").unwrap(); + let slack_actor = ExternalActorRef::new("slack", "user-alice", None::).unwrap(); let services = InMemoryConversationServices::default(); services .pair_external_actor( @@ -1764,7 +1776,8 @@ mod tests { async fn ordinary_inbound_adapter_records_product_inbound_with_adapter_name() { let telegram = AdapterKind::new("telegram").unwrap(); let telegram_install = AdapterInstallationId::new("telegram-install").unwrap(); - let telegram_actor = ExternalActorRef::new("telegram", "user-alice").unwrap(); + let telegram_actor = + ExternalActorRef::new("telegram", "user-alice", None::).unwrap(); let services = InMemoryConversationServices::default(); services .pair_external_actor( @@ -1836,7 +1849,7 @@ mod tests { let long_adapter = AdapterKind::new(&long_name) .expect("300-byte adapter kind must be valid — AdapterKind allows up to 512 bytes"); let long_install = AdapterInstallationId::new("long-adapter-install").unwrap(); - let long_actor = ExternalActorRef::new("long", "user-alice").unwrap(); + let long_actor = ExternalActorRef::new("long", "user-alice", None::).unwrap(); let services = InMemoryConversationServices::default(); services .pair_external_actor( diff --git a/crates/ironclaw_conversations/src/lib.rs b/crates/ironclaw_conversations/src/lib.rs index a272e4c4154..a8a5117dd9a 100644 --- a/crates/ironclaw_conversations/src/lib.rs +++ b/crates/ironclaw_conversations/src/lib.rs @@ -1,4 +1,4 @@ -//! Conversation binding and session-thread contracts for IronClaw Reborn. +//! Conversation binding and inbound-message contracts for IronClaw Reborn. //! //! This crate is the adapter-safe boundary between product/channel adapters and //! `ironclaw_turns::TurnCoordinator`. It resolves external actor/conversation @@ -6,6 +6,16 @@ //! asking the turn coordinator to parse raw channel payloads or store message //! content. //! +//! **It is not the transcript.** `ironclaw_threads` owns canonical threads and +//! their message content; this crate owns the *binding* from an external +//! conversation to one of them, plus inbound idempotency. The two used to share +//! five type names — including a `SessionThreadService` on each side with the +//! same `accept_inbound_message` signature and a different store behind it — +//! which is why everything here is spelled `…Conversation…` +//! ([`InboundConversationService`], [`AcceptedConversationMessage`], +//! [`ConversationMessageRecord`], …). Keep it that way: +//! `reborn_conversations_threads_attachments.rs` fails on any new shared name. +//! //! Durable persistence is provided by [`ConversationStateStore`] //! over a [`ScopedFilesystem`](ironclaw_filesystem::ScopedFilesystem). The //! `RootFilesystem` choice (libSQL-backed, PostgreSQL-backed, in-memory, or @@ -18,27 +28,35 @@ mod ids; mod inbound; mod memory; mod state_store; +mod stored_refs; mod traits; mod trusted_trigger; mod types; pub use conversation_state_store::{ConversationStateStore, RebornFilesystemConversationServices}; pub use error::InboundTurnError; +// `ExternalActorRef` / `ExternalConversationRef` are deliberately **not** +// re-exported. Their one home is +// `ironclaw_extension_contracts::external`; consumers import them from there, +// so this crate never becomes a second import path for channel vocabulary +// (PROPOSAL §11.2.4, and the rule `reborn_extension_contract_location_scan` +// enforces). pub use ids::{ - AdapterInstallationId, AdapterKind, ExternalActorRef, ExternalConversationIdentity, - ExternalConversationRef, ExternalEventId, InboundMessageContentRef, + AdapterInstallationId, AdapterKind, ExternalConversationIdentity, ExternalEventId, + InboundMessageContentRef, }; pub use inbound::{InboundTurnService, trusted_trigger_fire_submitter}; pub use memory::InMemoryConversationServices; pub use traits::{ - ConversationActorPairingService, ConversationBindingService, SessionThreadService, + ConversationActorPairingService, ConversationBindingService, InboundConversationService, }; pub use types::{ - AcceptInboundMessageRequest, AcceptedInboundMessage, AcceptedInboundMessageLookup, - AcceptedInboundMessageReplay, ConditionalUnpairOutcome, ConversationBindingResolution, - ConversationRouteKind, ExpectedExternalActorOwner, ExternalActorBindingEpoch, - InboundTurnRequest, InboundTurnResponse, LinkConversationRequest, LinkedConversationBinding, - MessageIdempotencyStatus, ReplyTargetBinding, ResolveConversationRequest, - ResolveStoredReplyTargetRequest, StoredReplyTargetAccess, StoredReplyTargetBinding, - ThreadAccessDecision, ThreadMessageRecord, ValidateReplyTargetRequest, + AcceptConversationMessageRequest, AcceptedConversationMessage, + AcceptedConversationMessageLookup, AcceptedConversationMessageReplay, ConditionalUnpairOutcome, + ConversationBindingResolution, ConversationMessageRecord, ConversationRouteKind, + ExpectedExternalActorOwner, ExternalActorBindingEpoch, InboundTurnRequest, InboundTurnResponse, + LinkConversationRequest, LinkedConversationBinding, MessageIdempotencyStatus, + ReplyTargetBinding, ResolveConversationRequest, ResolveStoredReplyTargetRequest, + StoredReplyTargetAccess, StoredReplyTargetBinding, ThreadAccessDecision, + ValidateReplyTargetRequest, }; diff --git a/crates/ironclaw_conversations/src/memory.rs b/crates/ironclaw_conversations/src/memory.rs index 48cdf238c62..c2614ddb43e 100644 --- a/crates/ironclaw_conversations/src/memory.rs +++ b/crates/ironclaw_conversations/src/memory.rs @@ -16,16 +16,18 @@ use serde::{Deserialize, Serialize}; use uuid::Uuid; use crate::{ - AcceptInboundMessageRequest, AcceptedInboundMessage, AcceptedInboundMessageLookup, - AcceptedInboundMessageReplay, AdapterInstallationId, AdapterKind, ConditionalUnpairOutcome, - ConversationActorPairingService, ConversationBindingResolution, ConversationBindingService, - ConversationRouteKind, ExpectedExternalActorOwner, ExternalActorBindingEpoch, ExternalActorRef, - ExternalConversationIdentity, ExternalConversationRef, InboundTurnError, + AcceptConversationMessageRequest, AcceptedConversationMessage, + AcceptedConversationMessageLookup, AcceptedConversationMessageReplay, AdapterInstallationId, + AdapterKind, ConditionalUnpairOutcome, ConversationActorPairingService, + ConversationBindingResolution, ConversationBindingService, ConversationMessageRecord, + ConversationRouteKind, ExpectedExternalActorOwner, ExternalActorBindingEpoch, + ExternalConversationIdentity, InboundConversationService, InboundTurnError, LinkConversationRequest, LinkedConversationBinding, MessageIdempotencyStatus, ReplyTargetBinding, ResolveConversationRequest, ResolveStoredReplyTargetRequest, - SessionThreadService, StoredReplyTargetAccess, StoredReplyTargetBinding, ThreadAccessDecision, - ThreadMessageRecord, ValidateReplyTargetRequest, + StoredReplyTargetAccess, StoredReplyTargetBinding, ThreadAccessDecision, + ValidateReplyTargetRequest, }; +use ironclaw_extension_contracts::external::{ExternalActorRef, ExternalConversationRef}; #[derive(Clone)] pub struct InMemoryConversationServices { @@ -190,7 +192,7 @@ impl InMemoryConversationServices { self.persist_state(old_state, snapshot).await } - pub async fn accepted_messages(&self) -> Vec { + pub async fn accepted_messages(&self) -> Vec { match self.state.lock() { Ok(state) => state.messages.clone(), Err(_) => Vec::new(), @@ -463,7 +465,8 @@ impl ConversationBindingService for InMemoryConversationServices { &request.external_actor_ref, )?; let binding_key = BindingKey::from_request(&request); - let external_conversation_identity = request.external_conversation_ref.identity(); + let external_conversation_identity = + ExternalConversationIdentity::from_ref(&request.external_conversation_ref); state.ensure_external_event_route( &request.tenant_id, &request.adapter_kind, @@ -522,7 +525,9 @@ impl ConversationBindingService for InMemoryConversationServices { tenant_id: request.tenant_id.clone(), adapter_kind: request.adapter_kind.clone(), adapter_installation_id: request.adapter_installation_id.clone(), - external_conversation_identity: request.external_conversation_ref.identity(), + external_conversation_identity: ExternalConversationIdentity::from_ref( + &request.external_conversation_ref, + ), }; if state.bindings.contains_key(&binding_key) { let existing = state @@ -739,7 +744,8 @@ impl InMemoryConversationServices { &request.external_actor_ref, )?; let binding_key = BindingKey::from_request(&request); - let external_conversation_identity = request.external_conversation_ref.identity(); + let external_conversation_identity = + ExternalConversationIdentity::from_ref(&request.external_conversation_ref); state.ensure_external_event_route( &request.tenant_id, &request.adapter_kind, @@ -851,11 +857,11 @@ impl InMemoryConversationServices { } #[async_trait] -impl SessionThreadService for InMemoryConversationServices { +impl InboundConversationService for InMemoryConversationServices { async fn accept_inbound_message( &self, - request: AcceptInboundMessageRequest, - ) -> Result { + request: AcceptConversationMessageRequest, + ) -> Result { let _mutation = self.mutation_lock.lock().await; self.refresh_state_from_repository().await?; let old_state = self.lock_state()?.clone(); @@ -897,7 +903,8 @@ impl SessionThreadService for InMemoryConversationServices { .ok_or_else(|| InboundTurnError::ThreadNotFound { thread_id: request.source_binding_ref.as_str().to_string(), })?; - let external_conversation_identity = request.external_conversation_ref.identity(); + let external_conversation_identity = + ExternalConversationIdentity::from_ref(&request.external_conversation_ref); if source_binding.external_conversation_identity != external_conversation_identity { return Err(InboundTurnError::AccessDenied { actor_id: request.actor.user_id.to_string(), @@ -956,7 +963,7 @@ impl SessionThreadService for InMemoryConversationServices { request.external_conversation_ref.clone(), ), ); - let accepted = AcceptedInboundMessage { + let accepted = AcceptedConversationMessage { tenant_id: request.tenant_id, thread_id: request.thread_id, actor: request.actor.clone(), @@ -974,7 +981,7 @@ impl SessionThreadService for InMemoryConversationServices { replay_key, StoredAcceptedMessageReplay { external_conversation_identity, - replay: AcceptedInboundMessageReplay { + replay: AcceptedConversationMessageReplay { resolution: source_binding.resolution( accepted.actor.user_id.clone(), binding_epoch, @@ -984,7 +991,7 @@ impl SessionThreadService for InMemoryConversationServices { }, }, ); - state.messages.push(ThreadMessageRecord { + state.messages.push(ConversationMessageRecord { accepted: accepted.clone(), actor: request.actor, external_event_id: request.external_event_id, @@ -1000,8 +1007,8 @@ impl SessionThreadService for InMemoryConversationServices { async fn replay_accepted_inbound_message( &self, - lookup: AcceptedInboundMessageLookup, - ) -> Result, InboundTurnError> { + lookup: AcceptedConversationMessageLookup, + ) -> Result, InboundTurnError> { let _mutation = self.mutation_lock.lock().await; self.refresh_state_from_repository().await?; let state = self.lock_state()?; @@ -1015,7 +1022,9 @@ impl SessionThreadService for InMemoryConversationServices { let Some(stored) = state.message_replays.get(&key) else { return Ok(None); }; - if stored.external_conversation_identity != lookup.external_conversation_ref.identity() { + if stored.external_conversation_identity + != ExternalConversationIdentity::from_ref(&lookup.external_conversation_ref) + { return Err(InboundTurnError::AccessDenied { actor_id: lookup.external_actor_ref.id().to_string(), thread_id: "external_event_route_mismatch".to_string(), @@ -1123,11 +1132,11 @@ pub(crate) struct InMemoryState { pub(crate) reply_targets: HashMap, pub(crate) threads: HashMap, pub(crate) external_event_routes: HashMap, - pub(crate) message_idempotency: HashMap, + pub(crate) message_idempotency: HashMap, pub(crate) message_replays: HashMap, pub(crate) submission_keys: HashMap, pub(crate) submitted_message_responses: HashMap, - pub(crate) messages: Vec, + pub(crate) messages: Vec, } impl InMemoryState { @@ -1429,7 +1438,9 @@ impl BindingKey { tenant_id: request.tenant_id.clone(), adapter_kind: request.adapter_kind.clone(), adapter_installation_id: request.adapter_installation_id.clone(), - external_conversation_identity: request.external_conversation_ref.identity(), + external_conversation_identity: ExternalConversationIdentity::from_ref( + &request.external_conversation_ref, + ), } } } @@ -1513,6 +1524,7 @@ pub(crate) struct ReplyTargetRecord { pub(crate) tenant_id: TenantId, pub(crate) adapter_kind: AdapterKind, pub(crate) adapter_installation_id: AdapterInstallationId, + #[serde(with = "crate::stored_refs::conversation_ref")] pub(crate) external_conversation_ref: ExternalConversationRef, pub(crate) thread_id: ThreadId, pub(crate) source_binding_ref: SourceBindingRef, @@ -1584,6 +1596,7 @@ pub(crate) struct BindingRecord { pub(crate) tenant_id: TenantId, pub(crate) adapter_kind: AdapterKind, pub(crate) adapter_installation_id: AdapterInstallationId, + #[serde(with = "crate::stored_refs::conversation_ref")] pub(crate) external_conversation_ref: ExternalConversationRef, pub(crate) external_conversation_identity: ExternalConversationIdentity, pub(crate) thread_id: ThreadId, @@ -1610,12 +1623,13 @@ impl BindingRecord { let reply_target_binding_ref = ReplyTargetBindingRef::new(format!("reply:{}", Uuid::new_v4())) .map_err(|reason| InboundTurnError::InvalidCanonicalRef { reason })?; - let external_conversation_identity = external_conversation_ref.identity(); + let external_conversation_identity = + ExternalConversationIdentity::from_ref(&external_conversation_ref); Ok(Self { tenant_id, adapter_kind, adapter_installation_id, - external_conversation_ref: external_conversation_ref.without_message_id(), + external_conversation_ref: external_conversation_ref.without_reply_target(), external_conversation_identity, thread_id: target.thread_id, agent_id: target.agent_id, @@ -1663,7 +1677,7 @@ impl BindingRecord { #[derive(Debug, Clone, Serialize, Deserialize)] pub(crate) struct StoredAcceptedMessageReplay { pub(crate) external_conversation_identity: ExternalConversationIdentity, - pub(crate) replay: AcceptedInboundMessageReplay, + pub(crate) replay: AcceptedConversationMessageReplay, } #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] diff --git a/crates/ironclaw_conversations/src/stored_refs.rs b/crates/ironclaw_conversations/src/stored_refs.rs new file mode 100644 index 00000000000..197a4ec2b93 --- /dev/null +++ b/crates/ironclaw_conversations/src/stored_refs.rs @@ -0,0 +1,266 @@ +//! Durable spelling of the channel-owned external refs this crate persists. +//! +//! [`ExternalConversationRef`] is owned by `ironclaw_extension_contracts`, but +//! the *records* that embed it belong to this crate, and **a record grammar +//! outlives the type that shaped it**. Released builds wrote +//! `{space_id,conversation_id,thread_id,message_id}`; the canonical type spells +//! the last two `topic_id` / `reply_target_message_id`. +//! +//! This adapter keeps the released grammar on the wire and accepts either +//! spelling on the way in. The rename is therefore invisible to durable +//! storage, which is the intent: it is a Rust-side fix for the +//! `conversations`/`threads` naming trap (`topic_id` is the *channel's* +//! sub-conversation, never a canonical +//! [`ThreadId`](ironclaw_host_api::ids::ThreadId)), and a naming trap in Rust +//! is not a reason to rewrite a persisted document. +//! +//! # Why the write side matters as much as the read side +//! +//! Every renamed field is an `Option`. Serde fills a missing `Option` with +//! `None` and does not error, so a spelling mismatch in *either* direction is +//! silent: the reader sees a route with no topic, and every threaded reply +//! collapses to its conversation root. The collapse is worse than misrouting — +//! it is lossy. [`ExternalConversationIdentity`](crate::ExternalConversationIdentity) +//! keys `BindingKey`, and `StoredConversationState::into_state` rebuilds the +//! binding map with `Vec<(K, V)>::into_iter().collect()`, so two threaded +//! bindings in one conversation that both lose their topic become the *same* +//! key and the later one silently wins. +//! +//! That failure mode is symmetric, which is why this module writes the legacy +//! grammar rather than the canonical one: +//! +//! - **Upgrade** (a released record read by this build) — handled by the +//! `#[serde(alias)]`es below. +//! - **Rollback** (a record written by this build read by a released binary) — +//! handled by writing `thread_id`/`message_id`. The released reader names +//! those two fields exactly and carries no aliases, so any other spelling +//! reads back as `None`. +//! +//! Emitting *both* spellings is a different thing from the write-legacy / +//! read-either shape used here, and it was refused: `#[serde(alias)]` makes a +//! dual-written record self-defeating, because a reader that accepts both +//! spellings rejects a document carrying both as a duplicate field. That +//! rejection is kept deliberately — an ambiguous record fails closed rather +//! than letting field order pick the winner. +//! +//! Binding identity is durable, so nothing ages out of it; the 48-hour +//! `DELIVERED_GATE_ROUTE_TTL` heals the *fingerprint* format change recorded on +//! the same CHECKLIST row, not this. +//! +//! # Scope: no actor adapter +//! +//! Only the conversation ref needs one. [`ExternalActorRef`]'s change was +//! purely *additive* — released builds wrote `{kind,id}`, this build writes +//! `{kind,id,display_name}` — and an added field is safe in both directions: +//! serde ignores unknown fields by default, and a missing `Option` reads as +//! `None`. Its canonical `Serialize`/`Deserialize` already revalidate through +//! `ExternalActorRef::new`, so an adapter here would only restate them. +//! `actor_serde_needs_no_adapter` pins that equivalence, so the claim is +//! checked rather than asserted. +//! +//! [`ExternalActorRef`]: ironclaw_extension_contracts::external::ExternalActorRef + +use ironclaw_extension_contracts::external::ExternalConversationRef; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; + +/// Serde adapter for a persisted [`ExternalConversationRef`] field. +pub(crate) mod conversation_ref { + use super::*; + + /// The durable grammar, borrowed from the canonical ref for writing. The + /// field names here are the *record's*, not the type's. + #[derive(Serialize)] + struct Written<'a> { + space_id: Option<&'a str>, + conversation_id: &'a str, + thread_id: Option<&'a str>, + message_id: Option<&'a str>, + } + + /// The durable grammar for reading. Released records spell the last two + /// fields `thread_id`/`message_id`; a record written by an intermediate + /// build of this rename spells them canonically. Both load. + #[derive(Deserialize)] + struct Read { + space_id: Option, + conversation_id: String, + #[serde(alias = "topic_id")] + thread_id: Option, + #[serde(alias = "reply_target_message_id")] + message_id: Option, + } + + pub(crate) fn serialize( + value: &ExternalConversationRef, + serializer: S, + ) -> Result + where + S: Serializer, + { + Written { + space_id: value.space_id(), + conversation_id: value.conversation_id(), + thread_id: value.topic_id(), + message_id: value.reply_target_message_id(), + } + .serialize(serializer) + } + + pub(crate) fn deserialize<'de, D>(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let stored = Read::deserialize(deserializer)?; + // Re-validate through the canonical constructor so a hand-edited or + // corrupted durable record cannot smuggle an unvalidated identifier + // back into memory. + ExternalConversationRef::new( + stored.space_id.as_deref(), + stored.conversation_id, + stored.thread_id.as_deref(), + stored.message_id.as_deref(), + ) + .map_err(serde::de::Error::custom) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use ironclaw_extension_contracts::external::ExternalActorRef; + + #[derive(Debug, PartialEq, Eq, Serialize, Deserialize)] + struct StoredConversation { + #[serde(with = "conversation_ref")] + external_conversation_ref: ExternalConversationRef, + } + + /// Deliberately carries no `#[serde(with)]` — the canonical impls are the + /// subject of `actor_serde_needs_no_adapter`. + #[derive(Debug, PartialEq, Eq, Serialize, Deserialize)] + struct StoredActor { + external_actor_ref: ExternalActorRef, + } + + fn threaded() -> ExternalConversationRef { + ExternalConversationRef::new(Some("T1"), "C1", Some("1700.1"), Some("1800.2")) + .expect("valid") + } + + #[test] + fn legacy_conversation_record_keeps_its_topic_and_reply_target() { + let legacy: StoredConversation = serde_json::from_str( + r#"{"external_conversation_ref":{"space_id":"T1","conversation_id":"C1", + "thread_id":"1700.1","message_id":"1800.2"}}"#, + ) + .expect("a record written before the unification must still load"); + let route = &legacy.external_conversation_ref; + assert_eq!(route.topic_id(), Some("1700.1")); + assert_eq!(route.reply_target_message_id(), Some("1800.2")); + } + + /// The rollback half. A released binary's reader names `thread_id` and + /// `message_id` and carries no aliases, so that is the only spelling it can + /// see. Asserting the *absence* of the canonical spelling is what fails if + /// someone reverts `serialize` to delegate to the canonical derive. + #[test] + fn current_conversation_record_is_written_in_the_durable_grammar() { + let json = serde_json::to_string(&StoredConversation { + external_conversation_ref: threaded(), + }) + .expect("serialize"); + let written: serde_json::Value = + serde_json::from_str(&json).expect("the written record parses"); + let route = &written["external_conversation_ref"]; + assert_eq!(route["thread_id"], "1700.1", "writes the durable grammar"); + assert_eq!(route["message_id"], "1800.2", "writes the durable grammar"); + assert!( + route.get("topic_id").is_none() && route.get("reply_target_message_id").is_none(), + "a released reader has no alias for the canonical spelling, so writing it \ + would silently strand every threaded route: {json}" + ); + } + + #[test] + fn current_conversation_record_round_trips() { + let record = StoredConversation { + external_conversation_ref: threaded(), + }; + let json = serde_json::to_string(&record).expect("serialize"); + let parsed: StoredConversation = serde_json::from_str(&json).expect("deserialize"); + assert_eq!(record, parsed); + } + + /// The alias is not dead code: an intermediate build of this rename wrote + /// the canonical spelling, and such a record must still load. + #[test] + fn canonically_spelled_conversation_record_still_loads() { + let canonical: StoredConversation = serde_json::from_str( + r#"{"external_conversation_ref":{"space_id":"T1","conversation_id":"C1", + "topic_id":"1700.1","reply_target_message_id":"1800.2"}}"#, + ) + .expect("the canonical spelling remains readable"); + let route = &canonical.external_conversation_ref; + assert_eq!(route.topic_id(), Some("1700.1")); + assert_eq!(route.reply_target_message_id(), Some("1800.2")); + } + + /// A record carrying both spellings for one field is ambiguous. Fail closed + /// rather than let serde's field order pick the winner. + #[test] + fn a_record_carrying_both_spellings_is_rejected() { + let result: Result = serde_json::from_str( + r#"{"external_conversation_ref":{"space_id":"T1","conversation_id":"C1", + "thread_id":"1700.1","topic_id":"9900.9"}}"#, + ); + assert!( + result.is_err(), + "an ambiguous record must not silently resolve to one of its two topics" + ); + } + + #[test] + fn stored_conversation_ref_revalidates_on_load() { + let result: Result = serde_json::from_str( + r#"{"external_conversation_ref":{"space_id":null,"conversation_id":"", + "thread_id":null,"message_id":null}}"#, + ); + assert!(result.is_err(), "an empty conversation id must not load"); + } + + /// The evidence for this module carrying no actor adapter: the canonical + /// impls already do everything one would — accept a released `{kind,id}` + /// payload, default the added field, and revalidate through + /// `ExternalActorRef::new`. + #[test] + fn actor_serde_needs_no_adapter() { + let legacy: StoredActor = + serde_json::from_str(r#"{"external_actor_ref":{"kind":"slack","id":"U1"}}"#) + .expect("a record written before the unification must still load"); + assert_eq!(legacy.external_actor_ref.kind(), "slack"); + assert_eq!(legacy.external_actor_ref.id(), "U1"); + assert_eq!(legacy.external_actor_ref.display_name(), None); + + let named = StoredActor { + external_actor_ref: ExternalActorRef::new("slack", "U1", Some("Alice")).expect("valid"), + }; + assert_eq!( + legacy.external_actor_ref, named.external_actor_ref, + "pairing lookups key on (kind, id), so a display name must not re-key them" + ); + + // Additive, so safe in the rollback direction too: a released reader + // names `kind`/`id` and ignores the field it does not know. + let json = serde_json::to_string(&named).expect("serialize"); + let written: serde_json::Value = serde_json::from_str(&json).expect("parses"); + assert_eq!(written["external_actor_ref"]["kind"], "slack"); + assert_eq!(written["external_actor_ref"]["id"], "U1"); + } + + #[test] + fn stored_actor_ref_revalidates_on_load() { + let result: Result = + serde_json::from_str(r#"{"external_actor_ref":{"kind":"slack","id":""}}"#); + assert!(result.is_err(), "an empty actor id must not load"); + } +} diff --git a/crates/ironclaw_conversations/src/traits.rs b/crates/ironclaw_conversations/src/traits.rs index 19abc59d31c..cc982f30666 100644 --- a/crates/ironclaw_conversations/src/traits.rs +++ b/crates/ironclaw_conversations/src/traits.rs @@ -2,13 +2,15 @@ use async_trait::async_trait; use ironclaw_turns::{AcceptedMessageRef, IdempotencyKey, SubmitTurnResponse}; use crate::{ - AcceptInboundMessageRequest, AcceptedInboundMessage, AcceptedInboundMessageLookup, - AcceptedInboundMessageReplay, AdapterInstallationId, AdapterKind, ConditionalUnpairOutcome, - ConversationBindingResolution, ExpectedExternalActorOwner, ExternalActorBindingEpoch, - ExternalActorRef, InboundTurnError, LinkConversationRequest, LinkedConversationBinding, - ReplyTargetBinding, ResolveConversationRequest, ResolveStoredReplyTargetRequest, - StoredReplyTargetBinding, ValidateReplyTargetRequest, + AcceptConversationMessageRequest, AcceptedConversationMessage, + AcceptedConversationMessageLookup, AcceptedConversationMessageReplay, AdapterInstallationId, + AdapterKind, ConditionalUnpairOutcome, ConversationBindingResolution, + ExpectedExternalActorOwner, ExternalActorBindingEpoch, InboundTurnError, + LinkConversationRequest, LinkedConversationBinding, ReplyTargetBinding, + ResolveConversationRequest, ResolveStoredReplyTargetRequest, StoredReplyTargetBinding, + ValidateReplyTargetRequest, }; +use ironclaw_extension_contracts::external::ExternalActorRef; #[async_trait] pub trait ConversationBindingService: Send + Sync { @@ -117,16 +119,16 @@ pub trait ConversationActorPairingService: Send + Sync { } #[async_trait] -pub trait SessionThreadService: Send + Sync { +pub trait InboundConversationService: Send + Sync { async fn accept_inbound_message( &self, - request: AcceptInboundMessageRequest, - ) -> Result; + request: AcceptConversationMessageRequest, + ) -> Result; async fn replay_accepted_inbound_message( &self, - lookup: AcceptedInboundMessageLookup, - ) -> Result, InboundTurnError>; + lookup: AcceptedConversationMessageLookup, + ) -> Result, InboundTurnError>; async fn inbound_message_turn_submission( &self, diff --git a/crates/ironclaw_conversations/src/types.rs b/crates/ironclaw_conversations/src/types.rs index 85a74a35fd0..8fa53867e76 100644 --- a/crates/ironclaw_conversations/src/types.rs +++ b/crates/ironclaw_conversations/src/types.rs @@ -6,10 +6,8 @@ use ironclaw_turns::{ }; use serde::{Deserialize, Serialize}; -use crate::{ - AdapterInstallationId, AdapterKind, ExternalActorRef, ExternalConversationRef, ExternalEventId, - InboundMessageContentRef, -}; +use crate::{AdapterInstallationId, AdapterKind, ExternalEventId, InboundMessageContentRef}; +use ironclaw_extension_contracts::external::{ExternalActorRef, ExternalConversationRef}; #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] #[serde(try_from = "String")] @@ -198,7 +196,7 @@ pub enum MessageIdempotencyStatus { } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub struct AcceptedInboundMessageLookup { +pub struct AcceptedConversationMessageLookup { pub tenant_id: TenantId, pub adapter_kind: AdapterKind, pub adapter_installation_id: AdapterInstallationId, @@ -208,13 +206,13 @@ pub struct AcceptedInboundMessageLookup { } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub struct AcceptedInboundMessageReplay { +pub struct AcceptedConversationMessageReplay { pub resolution: ConversationBindingResolution, - pub accepted_message: AcceptedInboundMessage, + pub accepted_message: AcceptedConversationMessage, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub struct AcceptInboundMessageRequest { +pub struct AcceptConversationMessageRequest { pub tenant_id: TenantId, pub thread_id: ThreadId, pub actor: TurnActor, @@ -232,7 +230,7 @@ pub struct AcceptInboundMessageRequest { } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub struct AcceptedInboundMessage { +pub struct AcceptedConversationMessage { pub tenant_id: TenantId, pub thread_id: ThreadId, pub actor: TurnActor, @@ -245,8 +243,8 @@ pub struct AcceptedInboundMessage { } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub struct ThreadMessageRecord { - pub accepted: AcceptedInboundMessage, +pub struct ConversationMessageRecord { + pub accepted: AcceptedConversationMessage, pub actor: TurnActor, pub external_event_id: ExternalEventId, pub content_ref: InboundMessageContentRef, @@ -317,7 +315,7 @@ impl TrustedInboundTurnRequest { #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct InboundTurnResponse { pub resolution: ConversationBindingResolution, - pub accepted_message: AcceptedInboundMessage, + pub accepted_message: AcceptedConversationMessage, pub turn_submission: Option, #[serde(default, skip_serializing_if = "std::ops::Not::not")] pub replayed_turn_submission: bool, diff --git a/crates/ironclaw_conversations/tests/conversation_state_store_contract.rs b/crates/ironclaw_conversations/tests/conversation_state_store_contract.rs index 5de13f88d31..468312e9b6b 100644 --- a/crates/ironclaw_conversations/tests/conversation_state_store_contract.rs +++ b/crates/ironclaw_conversations/tests/conversation_state_store_contract.rs @@ -11,12 +11,16 @@ use std::sync::Arc; +use chrono::{TimeZone, Utc}; use ironclaw_conversations::{ - AdapterInstallationId, AdapterKind, ConditionalUnpairOutcome, ConversationBindingService, - ConversationRouteKind, ExpectedExternalActorOwner, ExternalActorBindingEpoch, ExternalActorRef, - ExternalConversationRef, ExternalEventId, InboundTurnError, - RebornFilesystemConversationServices, ResolveConversationRequest, + AcceptConversationMessageRequest, AcceptedConversationMessageLookup, + AcceptedConversationMessageReplay, AdapterInstallationId, AdapterKind, + ConditionalUnpairOutcome, ConversationBindingService, ConversationMessageRecord, + ConversationRouteKind, ExpectedExternalActorOwner, ExternalActorBindingEpoch, ExternalEventId, + InboundConversationService, InboundMessageContentRef, InboundTurnError, + MessageIdempotencyStatus, RebornFilesystemConversationServices, ResolveConversationRequest, }; +use ironclaw_extension_contracts::external::{ExternalActorRef, ExternalConversationRef}; use ironclaw_filesystem::{CasExpectation, InMemoryBackend, RootFilesystem, ScopedFilesystem}; use ironclaw_host_api::{ ids::{AgentId, ProjectId, TenantId, UserId}, @@ -59,7 +63,7 @@ fn default_installation() -> AdapterInstallationId { } fn external_actor(id: &str) -> ExternalActorRef { - ExternalActorRef::new("user", id).unwrap() + ExternalActorRef::new("user", id, None::).unwrap() } fn external_conversation(id: &str) -> ExternalConversationRef { @@ -137,6 +141,138 @@ async fn filesystem_conversation_services_round_trip_persisted_state_on_reopen() .unwrap(); assert_eq!(resolution.tenant_id, tenant_id("tenant-a")); assert_eq!(resolution.actor.user_id, user_id("alice")); + + // The durable wrapper forwards the inbound-message half of the contract + // too, not just binding resolution. `inbound_contract.rs` only ever drives + // these two methods through `InMemoryConversationServices`, so without + // this the filesystem-backed `accept_inbound_message` / + // `replay_accepted_inbound_message` forwarders are never executed — + // and a broken delegation here would look exactly like a passing suite. + let accepted = reopened + .accept_inbound_message(AcceptConversationMessageRequest { + tenant_id: tenant_id("tenant-a"), + thread_id: resolution.turn_scope.thread_id, + actor: resolution.actor, + adapter_kind: telegram(), + adapter_installation_id: default_installation(), + external_actor_ref: external_actor("telegram-user-1"), + source_binding_ref: resolution.source_binding_ref, + reply_target_binding_ref: resolution.reply_target_binding_ref, + external_conversation_ref: external_conversation("chat-1"), + external_event_id: ExternalEventId::new("event-2").unwrap(), + route_kind: ConversationRouteKind::Direct, + content_ref: InboundMessageContentRef::new("content:event-2").unwrap(), + received_at: Utc.with_ymd_and_hms(2026, 5, 6, 12, 0, 0).unwrap(), + requested_run_profile: None, + }) + .await + .expect("the durable services accept an inbound message"); + assert_eq!(accepted.tenant_id, tenant_id("tenant-a")); + + let replay = reopened + .replay_accepted_inbound_message(AcceptedConversationMessageLookup { + tenant_id: tenant_id("tenant-a"), + adapter_kind: telegram(), + adapter_installation_id: default_installation(), + external_actor_ref: external_actor("telegram-user-1"), + external_conversation_ref: external_conversation("chat-1"), + external_event_id: ExternalEventId::new("event-2").unwrap(), + }) + .await + .expect("the replay lookup succeeds") + .expect("the accepted message replays by its external event id"); + assert_eq!( + replay.accepted_message.message_ref, accepted.message_ref, + "the replay must return the message the accept produced" + ); + assert_eq!(replay.accepted_message.thread_id, accepted.thread_id); + assert_eq!( + accepted.idempotency, + MessageIdempotencyStatus::Inserted, + "the first accept inserts" + ); + assert_eq!( + replay.accepted_message.idempotency, + MessageIdempotencyStatus::Duplicate, + "replaying an already-accepted event reports it as a duplicate, not a fresh insert" + ); + + // WS5 renamed this DTO family (`AcceptedInboundMessage*` -> + // `AcceptedConversationMessage*`, `ThreadMessageRecord` -> + // `ConversationMessageRecord`). Serde keys on FIELD names, not type names, + // so the rename must be invisible on the wire — these types are persisted + // through the conversation state store, and a rename that silently changed + // the encoding would strand every durable record written before it. Nothing + // else in the suite serializes this family, so without this the derives are + // never even instantiated. + let record = reopened + .inner() + .accepted_messages() + .await + .into_iter() + .find(|record| record.accepted.message_ref == accepted.message_ref) + .expect("the accepted message is recorded in conversation state"); + + let encoded_record = serde_json::to_value(&record).expect("record serializes"); + for field in [ + "accepted", + "actor", + "external_event_id", + "content_ref", + "received_at", + ] { + assert!( + encoded_record.get(field).is_some(), + "ConversationMessageRecord must keep its `{field}` wire field after the type rename" + ); + } + for field in [ + "tenant_id", + "thread_id", + "actor", + "message_ref", + "source_binding_ref", + "reply_target_binding_ref", + "received_at", + "idempotency", + ] { + assert!( + encoded_record["accepted"].get(field).is_some(), + "AcceptedConversationMessage must keep its `{field}` wire field after the type rename" + ); + } + + assert_eq!( + serde_json::from_value::(encoded_record) + .expect("record round-trips"), + record, + "ConversationMessageRecord must survive a durable round trip unchanged" + ); + + let encoded_replay = serde_json::to_value(&replay).expect("replay serializes"); + assert_eq!( + serde_json::from_value::(encoded_replay) + .expect("replay round-trips"), + replay, + "AcceptedConversationMessageReplay must survive a durable round trip unchanged" + ); + + let lookup = AcceptedConversationMessageLookup { + tenant_id: tenant_id("tenant-a"), + adapter_kind: telegram(), + adapter_installation_id: default_installation(), + external_actor_ref: external_actor("telegram-user-1"), + external_conversation_ref: external_conversation("chat-1"), + external_event_id: ExternalEventId::new("event-2").unwrap(), + }; + assert_eq!( + serde_json::from_value::( + serde_json::to_value(&lookup).expect("lookup serializes") + ) + .expect("lookup round-trips"), + lookup, + "AcceptedConversationMessageLookup must survive a durable round trip unchanged" + ); } #[tokio::test] @@ -461,3 +597,273 @@ async fn filesystem_conversation_state_store_isolates_two_tenants_with_same_user "cross-tenant first-contact bindings must produce distinct thread ids" ); } + +/// A threaded route: a topic inside the conversation plus the per-event reply +/// target. This is the shape the `thread_id` -> `topic_id` rename touched, so +/// it is the shape the durable-grammar tests drive. +fn threaded_conversation(id: &str, topic: &str, reply_target: &str) -> ExternalConversationRef { + ExternalConversationRef::new(Some("space-1"), id, Some(topic), Some(reply_target)) + .expect("threaded conversation") +} + +/// Collect every `(key, value)` pair in a JSON document, at any depth. +fn json_fields(value: &serde_json::Value, out: &mut Vec<(String, serde_json::Value)>) { + match value { + serde_json::Value::Object(map) => { + for (key, child) in map { + out.push((key.clone(), child.clone())); + json_fields(child, out); + } + } + serde_json::Value::Array(items) => { + for item in items { + json_fields(item, out); + } + } + _ => {} + } +} + +async fn read_state_document( + backend: &InMemoryBackend, + tenant: &str, + user: &str, +) -> serde_json::Value { + let state_path = VirtualPath::new(format!( + "/tenants/{tenant}/users/{user}/conversations/state.json" + )) + .expect("state path"); + let versioned = backend + .get(&state_path) + .await + .expect("read state") + .expect("stored state"); + serde_json::from_slice(&versioned.entry.body).expect("state json") +} + +/// Drive a paired actor, a threaded binding and an accepted message through the +/// durable services, and return the persisted document. +async fn seed_threaded_state( + backend: Arc, + scoped: Arc>, + conversation: &ExternalConversationRef, +) -> serde_json::Value { + let services = RebornFilesystemConversationServices::new(Arc::clone(&scoped)) + .await + .expect("services"); + services + .pair_external_actor( + tenant_id("tenant-a"), + telegram(), + default_installation(), + external_actor("telegram-user-threaded"), + user_id("alice"), + ) + .await + .expect("pair actor"); + let resolution = services + .resolve_or_create_binding(resolve_request( + tenant_id("tenant-a"), + external_actor("telegram-user-threaded"), + conversation.clone(), + "event-threaded-1", + )) + .await + .expect("resolve threaded binding"); + services + .accept_inbound_message(AcceptConversationMessageRequest { + tenant_id: tenant_id("tenant-a"), + thread_id: resolution.turn_scope.thread_id, + actor: resolution.actor, + adapter_kind: telegram(), + adapter_installation_id: default_installation(), + external_actor_ref: external_actor("telegram-user-threaded"), + source_binding_ref: resolution.source_binding_ref, + reply_target_binding_ref: resolution.reply_target_binding_ref, + external_conversation_ref: conversation.clone(), + external_event_id: ExternalEventId::new("event-threaded-2").unwrap(), + route_kind: ConversationRouteKind::Direct, + content_ref: InboundMessageContentRef::new("content:event-threaded-2").unwrap(), + received_at: Utc.with_ymd_and_hms(2026, 5, 6, 12, 0, 0).unwrap(), + requested_run_profile: None, + }) + .await + .expect("accept a threaded message"); + drop(services); + read_state_document(&backend, "tenant-a", "alice").await +} + +/// The rollback half of the `thread_id` -> `topic_id` rename, asserted on the +/// *document* rather than on a surrogate struct. +/// +/// `stored_refs.rs`'s own tests prove the adapter maps the grammar correctly, +/// but they declare their own record types — so an adapter that was never +/// attached, or attached to only some of the fields that carry a ref, leaves +/// them green. This drives the real `StoredConversationState` through the real +/// services and then reads every key in the persisted document, so a missing or +/// misplaced annotation at *any* site fails here. +/// +/// The assertion is deliberately the absence of the canonical spelling: a +/// released binary's readers name `thread_id`/`message_id` and carry no +/// aliases, so a document written in the canonical spelling reads back with +/// `None` for both, with no error — and because the topic keys `BindingKey`, +/// two threaded bindings in one conversation would collapse onto one key and +/// the earlier one would be dropped. +#[tokio::test] +async fn filesystem_conversation_services_persist_external_refs_in_the_durable_grammar() { + let backend = Arc::new(InMemoryBackend::new()); + let scoped = scoped_conversations_fs(Arc::clone(&backend), "tenant-a", "alice"); + let conversation = threaded_conversation("chat-threaded", "topic-1", "msg-1700.1"); + let state = seed_threaded_state(Arc::clone(&backend), Arc::clone(&scoped), &conversation).await; + + let mut fields = Vec::new(); + json_fields(&state, &mut fields); + assert!( + !fields.is_empty(), + "the persisted document scanned empty — the walk is broken" + ); + + let canonical: Vec<&String> = fields + .iter() + .map(|(key, _)| key) + .filter(|key| key.as_str() == "topic_id" || key.as_str() == "reply_target_message_id") + .collect(); + assert!( + canonical.is_empty(), + "durable records must keep the released grammar so a rollback can still read them; \ + found the canonical spelling at {canonical:?} in {state}" + ); + + // Non-vacuity: the topic and the reply target must actually be in there + // under the durable names, or "no canonical spelling" would pass on an + // empty document. + assert!( + fields + .iter() + .any(|(key, value)| key == "thread_id" && value == "topic-1"), + "the external topic must persist as `thread_id`: {state}" + ); + assert!( + fields + .iter() + .any(|(key, value)| key == "message_id" && value == "msg-1700.1"), + "the reply target must persist as `message_id`: {state}" + ); + + // And the route still resolves after a reopen, which is what the grammar + // is protecting. + let reopened = RebornFilesystemConversationServices::new(scoped) + .await + .expect("reopen"); + let resolution = reopened + .resolve_or_create_binding(resolve_request( + tenant_id("tenant-a"), + external_actor("telegram-user-threaded"), + conversation, + "event-threaded-3", + )) + .await + .expect("the threaded binding survives a reopen"); + assert_eq!(resolution.actor.user_id, user_id("alice")); +} + +/// The upgrade half, and the reason the aliases are not dead code: a document +/// written in the canonical spelling — by an intermediate build of this rename +/// — must still load, with its topic and reply target intact. +/// +/// Rewriting is scoped to objects that carry a `conversation_id`, so the +/// canonical `ThreadId` fields elsewhere in the document (`BindingRecord`, +/// `ReplyTargetRecord`, `ThreadKey` all have a real `thread_id`) are left +/// alone. If the rewrite ever stopped finding anything, the assertion below it +/// would still have to hold, so the test cannot pass vacuously. +#[tokio::test] +async fn filesystem_conversation_services_reopen_pre_unification_external_refs() { + let backend = Arc::new(InMemoryBackend::new()); + let scoped = scoped_conversations_fs(Arc::clone(&backend), "tenant-a", "alice"); + let conversation = threaded_conversation("chat-canonical", "topic-9", "msg-1900.9"); + let mut state = + seed_threaded_state(Arc::clone(&backend), Arc::clone(&scoped), &conversation).await; + + fn rewrite_refs_to_canonical(value: &mut serde_json::Value, rewritten: &mut usize) { + match value { + serde_json::Value::Object(map) => { + if map.contains_key("conversation_id") { + if let Some(topic) = map.remove("thread_id") { + map.insert("topic_id".to_string(), topic); + *rewritten += 1; + } + if let Some(reply_target) = map.remove("message_id") { + map.insert("reply_target_message_id".to_string(), reply_target); + *rewritten += 1; + } + } + for child in map.values_mut() { + rewrite_refs_to_canonical(child, rewritten); + } + } + serde_json::Value::Array(items) => { + for item in items { + rewrite_refs_to_canonical(item, rewritten); + } + } + _ => {} + } + } + + let mut rewritten = 0usize; + rewrite_refs_to_canonical(&mut state, &mut rewritten); + assert!( + rewritten > 0, + "the rewrite found no external ref to re-spell, so the reopen below would \ + prove nothing: {state}" + ); + + let state_path = VirtualPath::new("/tenants/tenant-a/users/alice/conversations/state.json") + .expect("state path"); + let mut versioned = backend + .get(&state_path) + .await + .expect("read state") + .expect("stored state"); + versioned.entry.body = serde_json::to_vec(&state).expect("canonical state json"); + backend + .put( + &state_path, + versioned.entry, + CasExpectation::Version(versioned.version), + ) + .await + .expect("write canonically spelled snapshot"); + + let reopened = RebornFilesystemConversationServices::new(scoped) + .await + .expect("a canonically spelled snapshot remains readable"); + let resolution = reopened + .resolve_or_create_binding(resolve_request( + tenant_id("tenant-a"), + external_actor("telegram-user-threaded"), + conversation, + "event-canonical-1", + )) + .await + .expect("the threaded route survives the alias path"); + assert_eq!(resolution.actor.user_id, user_id("alice")); + + // The topic must have survived as a topic, not have been dropped into the + // conversation root: a route with a *different* topic in the same + // conversation must not resolve to the same binding. + let other_topic = reopened + .resolve_or_create_binding(resolve_request( + tenant_id("tenant-a"), + external_actor("telegram-user-threaded"), + threaded_conversation("chat-canonical", "topic-other", "msg-1900.9"), + "event-canonical-2", + )) + .await + .expect("a second topic resolves"); + assert_ne!( + resolution.turn_scope.thread_id, other_topic.turn_scope.thread_id, + "two topics in one conversation must key different bindings — if the topic had \ + been read as None, both would collapse onto the conversation root" + ); +} diff --git a/crates/ironclaw_conversations/tests/inbound_contract.rs b/crates/ironclaw_conversations/tests/inbound_contract.rs index 001b43e0364..51c589c1f56 100644 --- a/crates/ironclaw_conversations/tests/inbound_contract.rs +++ b/crates/ironclaw_conversations/tests/inbound_contract.rs @@ -4,17 +4,18 @@ use std::sync::{Arc, Mutex}; use async_trait::async_trait; use chrono::{TimeZone, Utc}; use ironclaw_conversations::{ - AcceptInboundMessageRequest, AcceptedInboundMessage, AcceptedInboundMessageLookup, - AcceptedInboundMessageReplay, AdapterInstallationId, AdapterKind, ConditionalUnpairOutcome, - ConversationBindingResolution, ConversationBindingService, ConversationRouteKind, - ExpectedExternalActorOwner, ExternalActorBindingEpoch, ExternalActorRef, - ExternalConversationIdentity, ExternalConversationRef, ExternalEventId, - InMemoryConversationServices, InboundMessageContentRef, InboundTurnError, InboundTurnRequest, - InboundTurnService, LinkConversationRequest, LinkedConversationBinding, - MessageIdempotencyStatus, ReplyTargetBinding, ResolveStoredReplyTargetRequest, - SessionThreadService, StoredReplyTargetAccess, ThreadAccessDecision, + AcceptConversationMessageRequest, AcceptedConversationMessage, + AcceptedConversationMessageLookup, AcceptedConversationMessageReplay, AdapterInstallationId, + AdapterKind, ConditionalUnpairOutcome, ConversationBindingResolution, + ConversationBindingService, ConversationRouteKind, ExpectedExternalActorOwner, + ExternalActorBindingEpoch, ExternalConversationIdentity, ExternalEventId, + InMemoryConversationServices, InboundConversationService, InboundMessageContentRef, + InboundTurnError, InboundTurnRequest, InboundTurnService, LinkConversationRequest, + LinkedConversationBinding, MessageIdempotencyStatus, ReplyTargetBinding, + ResolveStoredReplyTargetRequest, StoredReplyTargetAccess, ThreadAccessDecision, ValidateReplyTargetRequest, }; +use ironclaw_extension_contracts::external::{ExternalActorRef, ExternalConversationRef}; use ironclaw_host_api::ids::{AgentId, ProjectId, TenantId, ThreadId, UserId}; use ironclaw_host_api::turn::{ AcceptedMessageRef, IdempotencyKey, ReplyTargetBindingRef, RunProfileId, RunProfileRequest, @@ -1183,7 +1184,7 @@ async fn external_ref_keying_cannot_be_collided_with_delimiter_characters() { tenant(), telegram(), default_installation(), - ExternalActorRef::new("user;id=x", "y").unwrap(), + ExternalActorRef::new("user;id=x", "y", None::).unwrap(), user("alice"), ) .await; @@ -1191,7 +1192,7 @@ async fn external_ref_keying_cannot_be_collided_with_delimiter_characters() { let colliding_actor = services .resolve_or_create_binding(resolve_request( telegram(), - ExternalActorRef::new("user", "x;id=y").unwrap(), + ExternalActorRef::new("user", "x;id=y", None::).unwrap(), external_conversation("chat-1", None), "actor-collision-event", )) @@ -1305,11 +1306,15 @@ async fn per_message_external_ids_do_not_fork_conversation_bindings() { .await .unwrap(); assert_eq!( - first_target.external_conversation_ref.message_id(), + first_target + .external_conversation_ref + .reply_target_message_id(), Some("message-1") ); assert_eq!( - second_target.external_conversation_ref.message_id(), + second_target + .external_conversation_ref + .reply_target_message_id(), Some("message-2") ); assert_eq!(coordinator.submissions().len(), 2); @@ -1451,11 +1456,11 @@ async fn validated_reply_target_preserves_adapter_installation_and_external_rout "channel-1" ); assert_eq!( - target.external_conversation_ref.thread_id(), + target.external_conversation_ref.topic_id(), Some("thread-1") ); assert_eq!( - target.external_conversation_ref.message_id(), + target.external_conversation_ref.reply_target_message_id(), None, "binding-level reply targets must not preserve stale per-message routing" ); @@ -2304,7 +2309,7 @@ async fn direct_route_rejects_borrowed_owner_actor_key() { assert!(matches!(err, InboundTurnError::AccessDenied { .. })); let err = services - .accept_inbound_message(AcceptInboundMessageRequest { + .accept_inbound_message(AcceptConversationMessageRequest { tenant_id: tenant(), thread_id: resolution.turn_scope.thread_id, actor: TurnActor::new(user("bob")), @@ -2503,7 +2508,7 @@ async fn shared_route_rejects_wrong_adapter_context() { assert!(matches!(err, InboundTurnError::AccessDenied { .. })); let err = services - .accept_inbound_message(AcceptInboundMessageRequest { + .accept_inbound_message(AcceptConversationMessageRequest { tenant_id: tenant(), thread_id: resolution.turn_scope.thread_id, actor: resolution.actor, @@ -2940,7 +2945,7 @@ async fn accept_inbound_message_rejects_stale_message_scoped_reply_ref() { inbound.handle_inbound_turn(widen).await.unwrap(); let err = services - .accept_inbound_message(AcceptInboundMessageRequest { + .accept_inbound_message(AcceptConversationMessageRequest { tenant_id: tenant(), thread_id: first.resolution.turn_scope.thread_id, actor: first.resolution.actor, @@ -2988,7 +2993,7 @@ async fn message_scoped_reply_target_rejects_same_thread_different_actor_route() .await .unwrap(); let accepted = services - .accept_inbound_message(AcceptInboundMessageRequest { + .accept_inbound_message(AcceptConversationMessageRequest { tenant_id: tenant(), thread_id: resolution.turn_scope.thread_id.clone(), actor: resolution.actor, @@ -3098,7 +3103,7 @@ async fn accept_inbound_message_rejects_external_route_mismatch() { .unwrap(); let err = services - .accept_inbound_message(AcceptInboundMessageRequest { + .accept_inbound_message(AcceptConversationMessageRequest { tenant_id: tenant(), thread_id: resolution.turn_scope.thread_id, actor: resolution.actor, @@ -3143,7 +3148,7 @@ async fn duplicate_accept_rejects_external_route_mismatch() { .unwrap(); services - .accept_inbound_message(AcceptInboundMessageRequest { + .accept_inbound_message(AcceptConversationMessageRequest { tenant_id: tenant(), thread_id: resolution.turn_scope.thread_id.clone(), actor: resolution.actor.clone(), @@ -3164,7 +3169,7 @@ async fn duplicate_accept_rejects_external_route_mismatch() { .unwrap(); let err = services - .accept_inbound_message(AcceptInboundMessageRequest { + .accept_inbound_message(AcceptConversationMessageRequest { tenant_id: tenant(), thread_id: resolution.turn_scope.thread_id, actor: resolution.actor, @@ -3219,7 +3224,7 @@ async fn accept_inbound_message_rejects_mixed_source_and_reply_bindings() { .unwrap(); let err = services - .accept_inbound_message(AcceptInboundMessageRequest { + .accept_inbound_message(AcceptConversationMessageRequest { tenant_id: tenant(), thread_id: first.turn_scope.thread_id, actor: first.actor, @@ -3251,17 +3256,33 @@ fn serde_deserialization_revalidates_external_ref_invariants() { assert!(serde_json::from_str::("\"\"").is_err()); assert!(serde_json::from_str::("\"\"").is_err()); assert!(serde_json::from_str::("\"bad\\u0000epoch\"").is_err()); + // The external actor/conversation pair is now the canonical + // `ironclaw_extension_contracts` type, so these invariants are asserted + // against that type's field spelling (`topic_id` / + // `reply_target_message_id`). The *durable* spelling this crate's own + // records use — which still accepts the pre-unification `thread_id` / + // `message_id` — is pinned by `stored_refs::tests`. assert!(serde_json::from_str::(r#"{"kind":"user","id":""}"#).is_err()); - assert!(serde_json::from_str::( - r#"{"space_id":null,"conversation_id":"chat-1","thread_id":"ok","message_id":"bad\u0001"}"# - ) - .is_err()); + assert!( + serde_json::from_str::( + r#"{"space_id":null,"conversation_id":"chat-1","topic_id":"ok","reply_target_message_id":"bad\u0001"}"# + ) + .is_err() + ); assert!( serde_json::from_str::( - r#"{"space_id":null,"conversation_id":"","thread_id":null}"# + r#"{"space_id":null,"conversation_id":"","topic_id":null}"# ) .is_err() ); + // The route identity keeps reading the pre-unification `thread_id` key, so + // durable binding keys written by released builds resolve unchanged. + assert!( + serde_json::from_str::( + r#"{"space_id":null,"conversation_id":"chat-1","thread_id":"topic-9"}"# + ) + .is_ok() + ); } #[tokio::test] @@ -3441,7 +3462,7 @@ fn default_installation() -> AdapterInstallationId { } fn external_actor(id: &str) -> ExternalActorRef { - ExternalActorRef::new("user", id).unwrap() + ExternalActorRef::new("user", id, None::).unwrap() } fn external_conversation( @@ -3453,7 +3474,7 @@ fn external_conversation( struct FixedMessageSessionService { message_ref: AcceptedMessageRef, - accepted: Mutex>, + accepted: Mutex>, submitted: Mutex>, } @@ -3468,18 +3489,18 @@ impl FixedMessageSessionService { } #[async_trait] -impl SessionThreadService for FixedMessageSessionService { +impl InboundConversationService for FixedMessageSessionService { async fn accept_inbound_message( &self, - request: AcceptInboundMessageRequest, - ) -> Result { + request: AcceptConversationMessageRequest, + ) -> Result { let mut accepted = self.accepted.lock().unwrap(); if let Some(existing) = accepted.clone() { let mut duplicate = existing; duplicate.idempotency = MessageIdempotencyStatus::Duplicate; return Ok(duplicate); } - let message = AcceptedInboundMessage { + let message = AcceptedConversationMessage { tenant_id: request.tenant_id, thread_id: request.thread_id, actor: request.actor, @@ -3496,8 +3517,8 @@ impl SessionThreadService for FixedMessageSessionService { async fn replay_accepted_inbound_message( &self, - _lookup: AcceptedInboundMessageLookup, - ) -> Result, InboundTurnError> { + _lookup: AcceptedConversationMessageLookup, + ) -> Result, InboundTurnError> { Ok(None) } diff --git a/crates/ironclaw_extension_contracts/src/external.rs b/crates/ironclaw_extension_contracts/src/external.rs index 9b3bebdae5b..9a015a77c47 100644 --- a/crates/ironclaw_extension_contracts/src/external.rs +++ b/crates/ironclaw_extension_contracts/src/external.rs @@ -190,6 +190,23 @@ impl ExternalConversationRef { self.reply_target_message_id.as_deref() } + /// The same conversation route with the reply-target hint dropped. + /// + /// The reply target is a per-event hint, never part of the route's + /// identity — which is why [`Self::conversation_fingerprint`], `PartialEq` + /// and `Hash` all exclude it. Durable binding records store the route, so + /// they store this form: persisting the hint would pin a binding to the one + /// message that happened to create it. Infallible by construction: every + /// surviving field was validated by [`Self::new`]. + pub fn without_reply_target(&self) -> Self { + Self { + space_id: self.space_id.clone(), + conversation_id: self.conversation_id.clone(), + topic_id: self.topic_id.clone(), + reply_target_message_id: None, + } + } + /// Canonical conversation fingerprint. Length-prefixed segments prevent /// delimiter collisions and the reply-target hint is deliberately excluded. pub fn conversation_fingerprint(&self) -> String { diff --git a/crates/ironclaw_extension_host/Cargo.toml b/crates/ironclaw_extension_host/Cargo.toml index 377eb2274d0..59343419f27 100644 --- a/crates/ironclaw_extension_host/Cargo.toml +++ b/crates/ironclaw_extension_host/Cargo.toml @@ -35,6 +35,9 @@ ed25519-dalek = "2.2.0" hmac = "0.13" hex = "0.4.3" ironclaw_auth = { path = "../ironclaw_auth" } +# `InboundAttachmentLander` — the channel host lands inbound attachment bytes +# before accepting a message. +ironclaw_attachments = { path = "../ironclaw_attachments" } ironclaw_approvals = { path = "../ironclaw_approvals" } ironclaw_authorization = { path = "../ironclaw_authorization" } ironclaw_capabilities = { path = "../ironclaw_capabilities" } diff --git a/crates/ironclaw_extension_host/src/channel_host.rs b/crates/ironclaw_extension_host/src/channel_host.rs index ca40fa4969e..e2a44491f32 100644 --- a/crates/ironclaw_extension_host/src/channel_host.rs +++ b/crates/ironclaw_extension_host/src/channel_host.rs @@ -23,6 +23,7 @@ use std::collections::{BTreeMap, HashMap}; use std::sync::{Arc, Mutex as StdMutex}; use async_trait::async_trait; +use ironclaw_attachments::InboundAttachmentLander; use ironclaw_conversations::RebornFilesystemConversationServices; use ironclaw_extension_contracts::external::{ExternalConversationRef, ExternalEventId}; use ironclaw_extension_contracts::preference_target::PreferenceTargetCodec; @@ -46,10 +47,10 @@ use ironclaw_product::ProjectFilesystemReader; use ironclaw_product::{ ApprovalInteractionService, AuthInteractionService, BlockedAuthFlowCanceller, ConversationBindingService, DefaultInboundTurnService, DefaultProductSurface, - DeliveryCoordinator, IdempotencyLedger, InboundAttachmentLander, - ProductActorUserResolutionRequest, ProductActorUserResolver, ProductInstallationKey, - ProductInstallationScope, ProductSurfaceFailure, RebornFilesystemIdempotencyLedger, - ResolvedProductActorUser, RunDeliveryObserver, RunDeliveryServices, RunDeliverySettings, + DeliveryCoordinator, IdempotencyLedger, ProductActorUserResolutionRequest, + ProductActorUserResolver, ProductInstallationKey, ProductInstallationScope, + ProductSurfaceFailure, RebornFilesystemIdempotencyLedger, ResolvedProductActorUser, + RunDeliveryObserver, RunDeliveryServices, RunDeliverySettings, StaticProductInstallationResolver, }; use ironclaw_product_contracts::account_setup::ChannelConnectionNoticePolicy; @@ -1111,7 +1112,7 @@ impl GenericChannelHostAssembly { pub mod test_support { use std::sync::Arc; - use ironclaw_product::InboundAttachmentLander; + use ironclaw_attachments::InboundAttachmentLander; use super::GenericChannelHostAssembly; diff --git a/crates/ironclaw_extension_host/src/channel_host/e2e_tests.rs b/crates/ironclaw_extension_host/src/channel_host/e2e_tests.rs index 4b27c32e7c2..eb008e94be2 100644 --- a/crates/ironclaw_extension_host/src/channel_host/e2e_tests.rs +++ b/crates/ironclaw_extension_host/src/channel_host/e2e_tests.rs @@ -129,7 +129,7 @@ use e2e_auth_challenge::FakeAuthChallengeProvider; struct InertAttachmentLander; #[async_trait::async_trait] -impl ironclaw_product::InboundAttachmentLander for InertAttachmentLander { +impl ironclaw_attachments::InboundAttachmentLander for InertAttachmentLander { async fn land( &self, _thread_scope: &ironclaw_threads::ThreadScope, @@ -155,10 +155,10 @@ impl ironclaw_product::InboundAttachmentLander for InertAttachmentLander { _thread_scope: &ironclaw_threads::ThreadScope, _referenced_storage_keys: &[String], ) -> Result< - ironclaw_product::AttachmentCleanupReport, + ironclaw_attachments::AttachmentCleanupReport, ironclaw_product_contracts::surface::ProductSurfaceError, > { - Ok(ironclaw_product::AttachmentCleanupReport::default()) + Ok(ironclaw_attachments::AttachmentCleanupReport::default()) } } @@ -890,7 +890,7 @@ impl ApprovalInteractionService for ForeignScopeApprovalService { /// /// `length_prefixed_fingerprint(["T-A", "D123", ""])` = `"3:T-A|4:D123|0:|"`. fn dm_conversation_fingerprint() -> String { - ironclaw_conversations::ExternalConversationRef::new(Some(TEAM), CHANNEL, None, None) + ExternalConversationRef::new(Some(TEAM), CHANNEL, None, None) .expect("DM conversation ref") // safety: static test DM ref is valid. .conversation_fingerprint() } diff --git a/crates/ironclaw_extension_host/src/channel_pairing.rs b/crates/ironclaw_extension_host/src/channel_pairing.rs index c23782d66b2..c5efc5652cc 100644 --- a/crates/ironclaw_extension_host/src/channel_pairing.rs +++ b/crates/ironclaw_extension_host/src/channel_pairing.rs @@ -27,8 +27,9 @@ use ironclaw_auth::{ AuthSurface, }; use ironclaw_conversations::{ - AdapterKind, ConversationActorPairingService, ExpectedExternalActorOwner, ExternalActorRef, + AdapterKind, ConversationActorPairingService, ExpectedExternalActorOwner, }; +use ironclaw_extension_contracts::external::ExternalActorRef; use ironclaw_filesystem::{ CasApply, ContentType, Entry, FilesystemError, RootFilesystem, ScopedFilesystem, cas_update, }; @@ -896,8 +897,14 @@ impl ChannelPairingService { // user id alone (accepted delta from the epoch-guarded host-state // shape this generalizes). for actor in cleanup { - let actor_ref = ExternalActorRef::new(&actor.actor_kind, &actor.external_actor_id) - .map_err(store_unavailable)?; + let actor_ref = ExternalActorRef::new( + &actor.actor_kind, + &actor.external_actor_id, + // Cleanup matches on identity only; the canonical ref excludes + // the display name from `PartialEq`/`Hash`. + None::, + ) + .map_err(store_unavailable)?; let installation_id = ironclaw_conversations::AdapterInstallationId::new(actor.installation_id.as_str()) .map_err(store_unavailable)?; diff --git a/crates/ironclaw_extension_host/src/channel_pairing/tests.rs b/crates/ironclaw_extension_host/src/channel_pairing/tests.rs index d620da21d26..6a4b1e664ab 100644 --- a/crates/ironclaw_extension_host/src/channel_pairing/tests.rs +++ b/crates/ironclaw_extension_host/src/channel_pairing/tests.rs @@ -9,9 +9,7 @@ use std::{ }; use ironclaw_auth::{AuthProductError, RebornAuthContinuationDispatcher}; -use ironclaw_conversations::{ - ConditionalUnpairOutcome, ExternalActorRef as ConversationActorRef, InboundTurnError, -}; +use ironclaw_conversations::{ConditionalUnpairOutcome, InboundTurnError}; use ironclaw_extension_contracts::auth_prompt::AuthPromptChallengeKind; use ironclaw_extension_contracts::channel_adapter::NormalizedInboundMessage; use ironclaw_extension_contracts::channel_adapter::ProductTriggerReason; @@ -268,7 +266,7 @@ impl ConversationActorPairingService for RecordingActorPairings { _tenant_id: TenantId, _adapter_kind: AdapterKind, _adapter_installation_id: ironclaw_conversations::AdapterInstallationId, - _external_actor_ref: ConversationActorRef, + _external_actor_ref: ExternalActorRef, _user_id: UserId, ) -> Result<(), InboundTurnError> { Ok(()) @@ -279,7 +277,7 @@ impl ConversationActorPairingService for RecordingActorPairings { _tenant_id: TenantId, _adapter_kind: AdapterKind, _adapter_installation_id: ironclaw_conversations::AdapterInstallationId, - _external_actor_ref: ConversationActorRef, + _external_actor_ref: ExternalActorRef, _user_id: UserId, _epoch: ironclaw_conversations::ExternalActorBindingEpoch, ) -> Result<(), InboundTurnError> { @@ -291,7 +289,7 @@ impl ConversationActorPairingService for RecordingActorPairings { _tenant_id: TenantId, _adapter_kind: AdapterKind, _adapter_installation_id: ironclaw_conversations::AdapterInstallationId, - _external_actor_ref: ConversationActorRef, + _external_actor_ref: ExternalActorRef, ) -> Result<(), InboundTurnError> { Ok(()) } @@ -301,7 +299,7 @@ impl ConversationActorPairingService for RecordingActorPairings { _tenant_id: &TenantId, _adapter_kind: &AdapterKind, adapter_installation_id: &ironclaw_conversations::AdapterInstallationId, - external_actor_ref: &ConversationActorRef, + external_actor_ref: &ExternalActorRef, expected: &ExpectedExternalActorOwner, ) -> Result { self.unpairs.lock().expect("unpairs lock").push(( diff --git a/crates/ironclaw_product/src/conversation_binding.rs b/crates/ironclaw_product/src/conversation_binding.rs index 322d1560228..b15d3956edc 100644 --- a/crates/ironclaw_product/src/conversation_binding.rs +++ b/crates/ironclaw_product/src/conversation_binding.rs @@ -427,7 +427,7 @@ impl ProductConversationBindingService { let tenant_id = installation_scope.tenant_id.clone(); let adapter_kind = conversation_adapter_kind(&request.adapter_id)?; let installation_id = conversation_installation_id(&request.installation_id)?; - let external_actor_ref = conversation_actor_ref(&request.external_actor_ref)?; + let external_actor_ref = request.external_actor_ref.clone(); match resolved_actor.binding_epoch.clone() { Some(binding_epoch) => { actor_pairings @@ -488,7 +488,7 @@ impl ProductConversationBindingService { &installation_scope.tenant_id, &conversation_adapter_kind(&request.adapter_id)?, &conversation_installation_id(&request.installation_id)?, - &conversation_actor_ref(&request.external_actor_ref)?, + &request.external_actor_ref, &ironclaw_conversations::ExpectedExternalActorOwner { user_id: expected_actor.user_id.clone(), binding_epoch: expected_actor.binding_epoch.clone(), @@ -737,10 +737,8 @@ fn conversation_request( tenant_id, adapter_kind: conversation_adapter_kind(&request.adapter_id)?, adapter_installation_id: conversation_installation_id(&request.installation_id)?, - external_actor_ref: conversation_actor_ref(&request.external_actor_ref)?, - external_conversation_ref: conversation_conversation_ref( - &request.external_conversation_ref, - )?, + external_actor_ref: request.external_actor_ref.clone(), + external_conversation_ref: request.external_conversation_ref.clone(), external_event_id: conversation_event_id(&request.external_event_id)?, route_kind: conversation_route_kind(request.route_kind), requested_agent_id: None, @@ -767,25 +765,6 @@ fn conversation_event_id( ironclaw_conversations::ExternalEventId::new(event_id.as_str()).map_err(map_conversation_error) } -fn conversation_actor_ref( - actor_ref: &crate::ExternalActorRef, -) -> Result { - ironclaw_conversations::ExternalActorRef::new(actor_ref.kind(), actor_ref.id()) - .map_err(map_conversation_error) -} - -fn conversation_conversation_ref( - conversation_ref: &crate::ExternalConversationRef, -) -> Result { - ironclaw_conversations::ExternalConversationRef::new( - conversation_ref.space_id(), - conversation_ref.conversation_id(), - conversation_ref.topic_id(), - conversation_ref.reply_target_message_id(), - ) - .map_err(map_conversation_error) -} - fn conversation_route_kind( route_kind: ProductConversationRouteKind, ) -> ironclaw_conversations::ConversationRouteKind { diff --git a/crates/ironclaw_product/src/inbound_turn.rs b/crates/ironclaw_product/src/inbound_turn.rs index 57f6fa32af5..f1d374ab926 100644 --- a/crates/ironclaw_product/src/inbound_turn.rs +++ b/crates/ironclaw_product/src/inbound_turn.rs @@ -46,7 +46,7 @@ use crate::policy::{ BeforeInboundPolicy, BeforeInboundPolicyOutcome, BeforeInboundPolicyRequest, NoopBeforeInboundPolicy, }; -use crate::reborn_services::InboundAttachmentLander; +use ironclaw_attachments::InboundAttachmentLander; #[cfg(not(any(test, feature = "test-support")))] const BEFORE_INBOUND_POLICY_TIMEOUT: Duration = Duration::from_secs(5); diff --git a/crates/ironclaw_product/src/inbound_turn/tests/attachments.rs b/crates/ironclaw_product/src/inbound_turn/tests/attachments.rs index 59c7f505e34..9774db0af0c 100644 --- a/crates/ironclaw_product/src/inbound_turn/tests/attachments.rs +++ b/crates/ironclaw_product/src/inbound_turn/tests/attachments.rs @@ -3,10 +3,10 @@ use super::*; use crate::{ - AttachmentCleanupReport, AuthRequirement, ExternalEventId, ParsedProductInbound, - ProductInboundEnvelope, ProductInboundPayload, ProjectScopedAttachmentLander, - ProtocolAuthEvidence, TrustedInboundContext, + AuthRequirement, ExternalEventId, ParsedProductInbound, ProductInboundEnvelope, + ProductInboundPayload, ProtocolAuthEvidence, TrustedInboundContext, }; +use ironclaw_attachments::{AttachmentCleanupReport, ProjectScopedAttachmentLander}; use ironclaw_filesystem::{ Fault, FaultInjecting, FilesystemError, FilesystemOperation, InMemoryBackend, ScopedFilesystem, }; diff --git a/crates/ironclaw_product/src/lib.rs b/crates/ironclaw_product/src/lib.rs index 6bdb1fcdeed..8cbb9589e41 100644 --- a/crates/ironclaw_product/src/lib.rs +++ b/crates/ironclaw_product/src/lib.rs @@ -145,7 +145,6 @@ pub use fakes::{ FakeInboundTurnService, NoProjectFilesystem, rejecting_product_surface_error, }; pub use scoped_fs::{ - ProjectScopedAttachmentLander, ProjectScopedAttachmentReader, ProjectScopedFilesystemReader, // Shared scoped-path helpers: the mount-browse reader in composition @@ -308,21 +307,20 @@ pub use reborn_services::{ AUTOMATION_RENAME_COMMAND, AUTOMATION_RESUME_CAPABILITY, AUTOMATION_RESUME_CAPABILITY_ID, AUTOMATION_RESUME_COMMAND, AUTOMATION_RUN_HISTORY_DEFAULT_PAGE_SIZE, AUTOMATION_RUN_HISTORY_MAX_PAGE_SIZE, AUTOMATIONS_VIEW, ActiveModelReader, - AttachmentCleanupReport, AutomationListRequest, AutomationProductService, CANCEL_RUN_COMMAND, - CREATE_THREAD_COMMAND, ChannelAuthAccountState, ChannelConnectionService, - ChannelInboundSurfaceAdmission, ChannelInboundSurfaceOutcome, - ChannelInboundSurfaceRejectedAdmission, ChannelInboundSurfaceRequest, CodexLoginStart, - EXTENSION_ACTIVATE_CAPABILITY, EXTENSION_ACTIVATE_CAPABILITY_ID, EXTENSION_IMPORT_CAPABILITY, - EXTENSION_IMPORT_CAPABILITY_ID, EXTENSION_INSTALL_CAPABILITY, EXTENSION_INSTALL_CAPABILITY_ID, + AutomationListRequest, AutomationProductService, CANCEL_RUN_COMMAND, CREATE_THREAD_COMMAND, + ChannelAuthAccountState, ChannelConnectionService, ChannelInboundSurfaceAdmission, + ChannelInboundSurfaceOutcome, ChannelInboundSurfaceRejectedAdmission, + ChannelInboundSurfaceRequest, CodexLoginStart, EXTENSION_ACTIVATE_CAPABILITY, + EXTENSION_ACTIVATE_CAPABILITY_ID, EXTENSION_IMPORT_CAPABILITY, EXTENSION_IMPORT_CAPABILITY_ID, + EXTENSION_INSTALL_CAPABILITY, EXTENSION_INSTALL_CAPABILITY_ID, EXTENSION_REGISTER_HOSTED_MCP_CAPABILITY, EXTENSION_REGISTER_HOSTED_MCP_CAPABILITY_ID, EXTENSION_REGISTRY_VIEW, EXTENSION_REMOVE_CAPABILITY, EXTENSION_REMOVE_CAPABILITY_ID, EXTENSION_SETUP_SUBMIT_CAPABILITY, EXTENSION_SETUP_SUBMIT_CAPABILITY_ID, EXTENSION_SETUP_VIEW, EXTENSIONS_VIEW, EmptyProductCommandInput, ExtensionCredentialSetupService, ExtensionCredentialStatusRequest, ExtensionCredentialSubmitRequest, FS_LIST_VIEW, FS_MOUNTS_VIEW, FS_READ_COMMAND, FS_STAT_VIEW, FilesystemBrowseReader, FsMount, - GLOBAL_AUTO_APPROVE_VIEW, InboundAttachmentLander, InboundAttachmentReader, - LLM_ACTIVE_SET_CAPABILITY, LLM_ACTIVE_SET_CAPABILITY_ID, LLM_CODEX_LOGIN_COMMAND, - LLM_CONFIG_VIEW, LLM_LIST_MODELS_COMMAND, LLM_NEARAI_LOGIN_COMMAND, + GLOBAL_AUTO_APPROVE_VIEW, LLM_ACTIVE_SET_CAPABILITY, LLM_ACTIVE_SET_CAPABILITY_ID, + LLM_CODEX_LOGIN_COMMAND, LLM_CONFIG_VIEW, LLM_LIST_MODELS_COMMAND, LLM_NEARAI_LOGIN_COMMAND, LLM_NEARAI_WALLET_LOGIN_COMMAND, LLM_PROVIDER_DELETE_CAPABILITY, LLM_PROVIDER_DELETE_CAPABILITY_ID, LLM_PROVIDER_UPSERT_CAPABILITY, LLM_PROVIDER_UPSERT_CAPABILITY_ID, LLM_TEST_CONNECTION_COMMAND, LOGS_VIEW, LlmActiveSelection, @@ -443,7 +441,6 @@ pub use ironclaw_product_contracts::inbound_requests::{ ProductRetryRunRequest, ProductSetupExtensionRequest, ProductSubmitTurnRequest, }; pub use product_surface_inbound::{ - DecodeInboundAttachments, IntoProductInboundCommand, ProductAttachmentCapabilities, - ProductInboundCommand, product_attachment_capabilities, + DecodeInboundAttachments, IntoProductInboundCommand, ProductInboundCommand, }; pub use workflow::DefaultProductSurface; diff --git a/crates/ironclaw_product/src/product_surface_inbound.rs b/crates/ironclaw_product/src/product_surface_inbound.rs index f22e0b98fca..5148d1ed5e3 100644 --- a/crates/ironclaw_product/src/product_surface_inbound.rs +++ b/crates/ironclaw_product/src/product_surface_inbound.rs @@ -8,17 +8,22 @@ //! //! - [`ProductInboundCommand`] — its `CancelRun` variant carries //! `ironclaw_turns::CancelRunRequest`. -//! - [`ProductAttachmentCapabilities`] / [`product_attachment_capabilities`] — -//! the budgets are `ironclaw_attachments` types. //! - the validation/normalization that turns a body into a canonical command, -//! which produces the two above and is not contract vocabulary in any case. +//! which produces the command above and is not contract vocabulary in any case. +//! +//! The browser-facing attachment contract that used to live here +//! (`ProductAttachmentCapabilities` / `product_attachment_capabilities`) went +//! the other way, to `ironclaw_attachments`, with the WS5 attachments widening: +//! it is the advertised half of the size ceilings that crate enforces, and one +//! home for a ceiling is the point (PROPOSAL §6.4.9). Transports import +//! `ironclaw_attachments::attachment_capabilities` directly. //! //! Because the DTOs now live in another crate, the normalization is exposed as //! the [`IntoProductInboundCommand`] and [`DecodeInboundAttachments`] extension //! traits rather than inherent methods; call sites are unchanged apart from //! bringing the trait into scope. -use ironclaw_attachments::{AttachmentBudgets, DEFAULT_ATTACHMENT_BUDGETS}; +use ironclaw_attachments::DEFAULT_ATTACHMENT_BUDGETS; use ironclaw_host_api::turn::{IdempotencyKey, SanitizedCancelReason, TurnGateRef, TurnRunId}; use ironclaw_host_api::{ attachment::InboundAttachment, @@ -42,38 +47,6 @@ const GATE_REF_MAX_BYTES: usize = 256; const CREDENTIAL_REF_MAX_BYTES: usize = 512; const ATTACHMENT_FILENAME_MAX_BYTES: usize = 256; -/// Browser-facing inline-attachment contract advertised to the WebUI. -/// -/// Carries the `accept` tokens generated from the shared -/// [`ironclaw_common`] format registry (so the file picker can never drift -/// from the server's allowed MIME set) plus the same budgets -/// [`DecodeInboundAttachments::decode_attachments`] enforces. The browser -/// uses this only for pre-submit hints; the server-side decode remains the -/// sole authority on what is accepted. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -pub struct ProductAttachmentCapabilities { - /// HTML file-input `accept` tokens from the shared registry: exact MIME - /// types plus extensions, e.g. `["image/png", ".png", "application/pdf", - /// ".pdf"]` — never `image/*` wildcards (which would advertise unsupported - /// formats, and which break folder navigation in the native macOS picker). - pub accept: Vec, - /// The count/byte budgets `decode_attachments` enforces. Flattened, so the - /// wire shape is unchanged and a new budget field reaches the browser - /// without an intermediate edit here. - #[serde(flatten)] - pub budgets: AttachmentBudgets, -} - -/// The inline-attachment contract advertised to browsers. Generated from the -/// shared format registry and the budgets `decode_attachments` enforces, so -/// the picker and the server stay in lockstep by construction. -pub fn product_attachment_capabilities() -> ProductAttachmentCapabilities { - ProductAttachmentCapabilities { - accept: ironclaw_common::accept_tokens(), - budgets: DEFAULT_ATTACHMENT_BUDGETS, - } -} - /// Decode the inline attachments a submit-turn body carries into bytes-bearing /// [`InboundAttachment`]s ready for landing. pub trait DecodeInboundAttachments { diff --git a/crates/ironclaw_product/src/reborn_services.rs b/crates/ironclaw_product/src/reborn_services.rs index dfc81491c50..8a6c7b3b8f5 100644 --- a/crates/ironclaw_product/src/reborn_services.rs +++ b/crates/ironclaw_product/src/reborn_services.rs @@ -34,6 +34,7 @@ use crate::{ use async_trait::async_trait; use chrono::Utc; use futures::future::try_join_all; +use ironclaw_attachments::{InboundAttachmentLander, InboundAttachmentReader}; use ironclaw_auth::{ AuthFlowStatus, AuthProductScope, AuthProviderId, CredentialAccountId, CredentialAccountProjection, CredentialAccountStatus, CredentialAccountUpdateBinding, @@ -44,7 +45,6 @@ use ironclaw_host_api::turn::{ TurnScope, TurnStatus, }; use ironclaw_host_api::{ - attachment::InboundAttachment, capability::{EffectKind, GrantConstraints, PermissionMode}, ids::{ ActivityId, AgentId, CapabilityId, ExtensionId, InvocationId, ProjectId, ResultRef, @@ -61,10 +61,9 @@ use ironclaw_product_contracts::surface::{ ProductSurfaceValidationCode, }; use ironclaw_threads::{ - AcceptInboundMessageRequest, AcceptedInboundMessageReplay, AttachmentRef, EnsureThreadRequest, - MessageContent, MessageStatus, ReplayAcceptedInboundMessageRequest, SessionThreadError, - SessionThreadRecord, SessionThreadService, ThreadHistory, ThreadHistoryRequest, - ThreadMessageId, ThreadScope, + AcceptInboundMessageRequest, AcceptedInboundMessageReplay, EnsureThreadRequest, MessageContent, + MessageStatus, ReplayAcceptedInboundMessageRequest, SessionThreadError, SessionThreadRecord, + SessionThreadService, ThreadHistory, ThreadHistoryRequest, ThreadMessageId, ThreadScope, }; use ironclaw_triggers::{AutomationName, AutomationNameError}; use ironclaw_turns::{ @@ -2168,72 +2167,6 @@ fn operator_diagnostics_surface_status( } } -/// Lands inbound attachment bytes into durable, agent-accessible storage and -/// returns the transcript references to persist on the user message. -/// -/// Injected by host composition, which owns the project-scoped filesystem -/// authority. `message_id` is a stable per-message id (the idempotency key) -/// used only to disambiguate the storage path; the implementation writes -/// through the same `MountView` the agent's file tools resolve through, so -/// landed bytes are readable by `file_read`/`list_dir` in later turns. -#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] -pub struct AttachmentCleanupReport { - pub scanned_batches: usize, - pub deleted_batches: usize, -} - -#[async_trait] -pub trait InboundAttachmentLander: Send + Sync { - async fn land( - &self, - thread_scope: &ThreadScope, - message_id: &str, - attachments: Vec, - ) -> Result, ProductSurfaceError>; - - /// Remove one complete batch previously returned by [`Self::land`]. - /// - /// The inbound workflow calls this only when durable message acceptance - /// fails after landing. Implementations must constrain deletion to the - /// batch represented by `attachments`; they must never sweep unrelated - /// workspace paths. - async fn rollback( - &self, - thread_scope: &ThreadScope, - attachments: &[AttachmentRef], - ) -> Result<(), ProductSurfaceError>; - - /// Reconcile old committed batches against an exhaustive set of durable - /// attachment storage keys for this exact thread scope. - /// - /// Callers must skip this operation when their reference scan was - /// truncated. The complete snapshot may include attachment domains the - /// implementation does not own, such as agent-created outbound workspace - /// files; implementations ignore those references and fail closed when no - /// owned reference proves the snapshot usable. Implementations keep a - /// reconciliation window and bounded filesystem scan so recent in-flight - /// work and unrelated workspace paths are never removed. - async fn cleanup_stale( - &self, - thread_scope: &ThreadScope, - referenced_storage_keys: &[String], - ) -> Result; -} - -/// Reads a landed attachment's bytes back for the WebUI bytes endpoint. The -/// read counterpart of [`InboundAttachmentLander`]: host composition implements -/// it over the same project-scoped workspace filesystem the lander wrote -/// through, so `storage_key` is re-scoped through that mount authority and never -/// treated as a host path. -#[async_trait] -pub trait InboundAttachmentReader: Send + Sync { - async fn read( - &self, - thread_scope: &ThreadScope, - storage_key: &str, - ) -> Result, ProductSurfaceError>; -} - /// Product-side command membrane for the generic [`ProductSurface::invoke`] /// conduit. /// diff --git a/crates/ironclaw_product/src/run_delivery/gate_routes.rs b/crates/ironclaw_product/src/run_delivery/gate_routes.rs index 3f8cac993d4..6fe6faa44f2 100644 --- a/crates/ironclaw_product/src/run_delivery/gate_routes.rs +++ b/crates/ironclaw_product/src/run_delivery/gate_routes.rs @@ -37,51 +37,48 @@ pub(crate) async fn record_gate_route_if_needed( // Space-qualified refs (matches inbound events that carry the // vendor space id), threaded on the prompt and at conversation root. if let Some(space) = space { - if let Ok(conv_ref) = ironclaw_conversations::ExternalConversationRef::new( - Some(space), - conversation_id, - Some(vendor_ref), - None, - ) { + if let Ok(conv_ref) = + ExternalConversationRef::new(Some(space), conversation_id, Some(vendor_ref), None) + { conversation_fingerprints.insert(conv_ref.conversation_fingerprint()); } - if let Ok(conv_ref) = ironclaw_conversations::ExternalConversationRef::new( - Some(space), - conversation_id, - None, - None, - ) { + if let Ok(conv_ref) = + ExternalConversationRef::new(Some(space), conversation_id, None, None) + { conversation_fingerprints.insert(conv_ref.conversation_fingerprint()); } } // No-space fallbacks for events that omit the space id; the set // deduplicates when the two forms coincide. - if let Ok(conv_ref) = ironclaw_conversations::ExternalConversationRef::new( - None, - conversation_id, - Some(vendor_ref), - None, - ) { - conversation_fingerprints.insert(conv_ref.conversation_fingerprint()); - } if let Ok(conv_ref) = - ironclaw_conversations::ExternalConversationRef::new(None, conversation_id, None, None) + ExternalConversationRef::new(None, conversation_id, Some(vendor_ref), None) { conversation_fingerprints.insert(conv_ref.conversation_fingerprint()); } + if let Ok(conv_ref) = ExternalConversationRef::new(None, conversation_id, None, None) { + conversation_fingerprints.insert(conv_ref.conversation_fingerprint()); + } } // The originating conversation (without its message ref) also routes: // a bare reply next to the prompt resolves the same gate. + // + // `None` for the reply target is the *route*, spelled explicitly. It is not + // load-bearing — `conversation_fingerprint` hashes space + conversation + + // topic and deliberately excludes the reply-target hint, so this is + // byte-identical to threading the source's own hint through. It is written + // this way because a per-event message id inside a stable route key reads + // like a bug on every future review, and the previous spelling had to be + // read together with the fingerprint function to be seen as correct. if let Some(source) = source_conversation - && let Ok(conv_ref) = ironclaw_conversations::ExternalConversationRef::new( + && let Ok(conv_ref) = ExternalConversationRef::new( source.space_id(), source.conversation_id(), source.topic_id(), - source.reply_target_message_id(), + None, ) { - conversation_fingerprints.insert(conv_ref.without_message_id().conversation_fingerprint()); + conversation_fingerprints.insert(conv_ref.conversation_fingerprint()); } if conversation_fingerprints.is_empty() { diff --git a/crates/ironclaw_product/src/scoped_fs/attachment_reader.rs b/crates/ironclaw_product/src/scoped_fs/attachment_reader.rs new file mode 100644 index 00000000000..13f69b59f73 --- /dev/null +++ b/crates/ironclaw_product/src/scoped_fs/attachment_reader.rs @@ -0,0 +1,342 @@ +//! Project-scoped inbound attachment *read-back* for product adapters. +//! +//! Named for what it holds. After the WS5 widening this module is the reader +//! and nothing else, so the path no longer says `attachment_landing` — a path +//! that would send discovery here looking for landing behavior that lives in +//! another crate. +//! +//! The write half — the [`InboundAttachmentLander`] port and its default +//! implementation — moved to `ironclaw_attachments` with that widening +//! (PROPOSAL §6.4.9). This reader could not follow it: besides +//! [`InboundAttachmentReader`] it also implements +//! [`LoopAttachmentReadPort`], and `ironclaw_loop_host` is a `loops`-layer +//! crate a `substrates` crate may not depend on — and once the struct moved, +//! that impl would have neither side in this crate either. It stays here, where +//! both traits are nameable. + +use std::sync::Arc; + +use async_trait::async_trait; +use ironclaw_attachments::{DEFAULT_MAX_ATTACHMENT_BYTES, InboundAttachmentReader}; +use ironclaw_filesystem::{FilesystemError, RootFilesystem, ScopedFilesystem}; +use ironclaw_host_api::{path::ScopedPath, resource::ResourceScope}; +use ironclaw_loop_host::{LoopAttachmentReadError, LoopAttachmentReadPort}; +use ironclaw_product_contracts::surface::{ + ProductSurfaceError, ProductSurfaceErrorCode, ProductSurfaceErrorKind, +}; +use ironclaw_threads::ThreadScope; + +/// a corrupt/oversized key can't materialize unbounded bytes. +pub struct ProjectScopedAttachmentReader { + filesystem: Arc>, + max_bytes: usize, +} + +impl ProjectScopedAttachmentReader { + pub fn new(filesystem: Arc>) -> Self { + Self { + filesystem, + max_bytes: DEFAULT_MAX_ATTACHMENT_BYTES, + } + } + + /// Construct a reader with an explicit read ceiling. Test-only: production + /// always uses [`DEFAULT_MAX_ATTACHMENT_BYTES`] via [`Self::new`], but the + /// oversized branch is only reachable in a test with a tiny ceiling. + #[cfg(test)] + fn with_max_bytes(filesystem: Arc>, max_bytes: usize) -> Self { + Self { + filesystem, + max_bytes, + } + } +} + +#[async_trait] +impl LoopAttachmentReadPort for ProjectScopedAttachmentReader { + async fn read_attachment_bytes( + &self, + scope: &ResourceScope, + storage_key: &str, + ) -> Result, LoopAttachmentReadError> { + let path = ScopedPath::new(storage_key.to_string()) + .map_err(|error| LoopAttachmentReadError::Backend(error.to_string()))?; + match self + .filesystem + .read_bytes_bounded(scope, &path, self.max_bytes) + .await + { + Ok(Some(bytes)) => Ok(bytes), + // `read_bytes_bounded` returns `Ok(None)` only when the file is + // larger than `max_bytes` — an oversized attachment we refuse to + // materialize, not a missing one. + Ok(None) => Err(LoopAttachmentReadError::Backend(format!( + "attachment exceeds the {}-byte read limit", + self.max_bytes + ))), + Err(FilesystemError::NotFound { .. }) => Err(LoopAttachmentReadError::NotFound), + Err(FilesystemError::PermissionDenied { .. }) => { + Err(LoopAttachmentReadError::Forbidden) + } + Err(error) => Err(LoopAttachmentReadError::Backend(error.to_string())), + } + } +} + +/// Read counterpart wired into the product surface so the bytes endpoint can serve +/// image thumbnails. It reuses the loop read port — the same bounded, +/// `MountView`-re-scoped read — and translates the scope and error taxonomy to +/// the product API surface. A missing/oversized/forbidden read becomes a sanitized +/// product error rather than leaking a host path or backend string. +#[async_trait] +impl InboundAttachmentReader for ProjectScopedAttachmentReader { + async fn read( + &self, + thread_scope: &ThreadScope, + storage_key: &str, + ) -> Result, ProductSurfaceError> { + let scope = thread_scope.to_resource_scope(); + self.read_attachment_bytes(&scope, storage_key) + .await + .map_err(|error| match error { + LoopAttachmentReadError::NotFound => ProductSurfaceError { + code: ProductSurfaceErrorCode::NotFound, + kind: ProductSurfaceErrorKind::NotFound, + status_code: 404, + retryable: false, + field: None, + validation_code: None, + }, + LoopAttachmentReadError::Forbidden => ProductSurfaceError { + code: ProductSurfaceErrorCode::Forbidden, + kind: ProductSurfaceErrorKind::ParticipantDenied, + status_code: 403, + retryable: false, + field: None, + validation_code: None, + }, + // Carry the cause to the log (sanitized 500 on the wire) rather + // than dropping it — see error-handling rule. + LoopAttachmentReadError::Backend(reason) => { + ProductSurfaceError::internal_from(reason) + } + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + use ironclaw_attachments::InboundAttachmentLander; + use ironclaw_attachments::{ProjectScopedAttachmentLander, WORKSPACE_ALIAS}; + use ironclaw_filesystem::InMemoryBackend; + use ironclaw_host_api::{ + attachment::InboundAttachment, + ids::{AgentId, TenantId, UserId}, + mount::{MountGrant, MountPermissions, MountView}, + path::{MountAlias, VirtualPath}, + }; + + fn workspace_fs(permissions: MountPermissions) -> Arc> { + let view = MountView::new(vec![MountGrant::new( + MountAlias::new(WORKSPACE_ALIAS).unwrap(), + VirtualPath::new("/projects/workspace").unwrap(), + permissions, + )]) + .unwrap(); + Arc::new(ScopedFilesystem::with_fixed_view( + Arc::new(InMemoryBackend::new()), + view, + )) + } + + fn thread_scope() -> ThreadScope { + ThreadScope { + tenant_id: TenantId::new("tenant-test").unwrap(), + agent_id: AgentId::new("agent-test").unwrap(), + project_id: None, + owner_user_id: Some(UserId::new("user-test").unwrap()), + mission_id: None, + } + } + + #[tokio::test] + async fn reader_reads_back_landed_attachment_bytes() { + // The reader is the producer side of the image-vision path: it must read + // back exactly what the lander wrote under the same workspace mount. + let fs = workspace_fs(MountPermissions::read_write()); + let lander = ProjectScopedAttachmentLander::new(Arc::clone(&fs)); + let refs = lander + .land( + &thread_scope(), + "msg1", + vec![InboundAttachment { + id: "att-0".to_string(), + mime_type: "image/png".to_string(), + filename: Some("diagram.png".to_string()), + bytes: vec![1, 2, 3, 4], + }], + ) + .await + .expect("landing succeeds through a read-write workspace mount"); + let storage_key = refs[0].storage_key.as_deref().expect("storage_key set"); + + let reader = ProjectScopedAttachmentReader::new(Arc::clone(&fs)); + let bytes = reader + .read_attachment_bytes(&thread_scope().to_resource_scope(), storage_key) + .await + .expect("reading back the landed attachment succeeds"); + assert_eq!(bytes, vec![1, 2, 3, 4]); + } + + #[tokio::test] + async fn reader_missing_attachment_maps_to_not_found() { + let reader = + ProjectScopedAttachmentReader::new(workspace_fs(MountPermissions::read_write())); + let err = reader + .read_attachment_bytes( + &thread_scope().to_resource_scope(), + "/workspace/attachments/2026-06-14/m1-0-missing.png", + ) + .await + .expect_err("an absent attachment is a not-found, not bytes"); + assert!(matches!(err, LoopAttachmentReadError::NotFound)); + } + + #[tokio::test] + async fn reader_oversized_attachment_is_a_backend_refusal_not_not_found() { + let fs = workspace_fs(MountPermissions::read_write()); + let lander = ProjectScopedAttachmentLander::new(Arc::clone(&fs)); + let refs = lander + .land( + &thread_scope(), + "msg1", + vec![InboundAttachment { + id: "att-0".to_string(), + mime_type: "image/png".to_string(), + filename: Some("diagram.png".to_string()), + bytes: vec![1, 2, 3, 4], + }], + ) + .await + .expect("landing succeeds through a read-write workspace mount"); + let storage_key = refs[0].storage_key.as_deref().expect("storage_key set"); + + // A 2-byte ceiling rejects the 4-byte attachment. The reader must not + // mislabel an oversized file as `NotFound`. + let reader = ProjectScopedAttachmentReader::with_max_bytes(Arc::clone(&fs), 2); + let err = reader + .read_attachment_bytes(&thread_scope().to_resource_scope(), storage_key) + .await + .expect_err("an oversized attachment is refused"); + match err { + LoopAttachmentReadError::Backend(reason) => assert!(reason.contains("exceeds")), + other => panic!("expected a backend refusal, got {other}"), + } + } + + // The `InboundAttachmentReader` half — the product-surface translation — + // had no coverage at all: every test above drives `read_attachment_bytes` + // (the loop port), so the whole `map_err` taxonomy below it was dead to the + // suite. These four pin it, because the taxonomy IS the contract for the + // bytes endpoint: a caller distinguishes "gone" from "not yours" from + // "broken" only by the status this closure picks. + // + // Crate tier rather than `tests/integration/`: the int tier reaches the + // success path already (`tests/integration/attach.rs` drives the production + // wiring), but the three failure arms need a mount that denies reads and a + // storage key that fails `ScopedPath` validation — both are properties of + // this adapter's construction, not of a wired runtime. + + #[tokio::test] + async fn product_reader_serves_landed_bytes_through_the_thread_scope() { + let fs = workspace_fs(MountPermissions::read_write()); + let lander = ProjectScopedAttachmentLander::new(Arc::clone(&fs)); + let refs = lander + .land( + &thread_scope(), + "msg1", + vec![InboundAttachment { + id: "att-0".to_string(), + mime_type: "image/png".to_string(), + filename: Some("diagram.png".to_string()), + bytes: vec![9, 8, 7], + }], + ) + .await + .expect("landing succeeds"); + let storage_key = refs[0].storage_key.as_deref().expect("storage_key set"); + + let reader = ProjectScopedAttachmentReader::new(Arc::clone(&fs)); + let bytes = InboundAttachmentReader::read(&reader, &thread_scope(), storage_key) + .await + .expect("the product reader serves the landed bytes"); + assert_eq!(bytes, vec![9, 8, 7]); + } + + #[tokio::test] + async fn product_reader_maps_a_missing_attachment_to_404_not_found() { + let reader = + ProjectScopedAttachmentReader::new(workspace_fs(MountPermissions::read_write())); + let error = InboundAttachmentReader::read( + &reader, + &thread_scope(), + "/workspace/attachments/2026-06-14/m1-0-missing.png", + ) + .await + .expect_err("an absent attachment is an error, not empty bytes"); + assert_eq!(error.status_code, 404); + assert_eq!(error.code, ProductSurfaceErrorCode::NotFound); + assert_eq!(error.kind, ProductSurfaceErrorKind::NotFound); + assert!( + !error.retryable, + "a missing attachment never becomes present" + ); + } + + #[tokio::test] + async fn product_reader_maps_a_denied_mount_to_403_rather_than_404() { + // The distinction matters: answering 404 for a readable-but-forbidden + // attachment would tell a caller it does not exist. + let reader = ProjectScopedAttachmentReader::new(workspace_fs(MountPermissions::none())); + let error = InboundAttachmentReader::read( + &reader, + &thread_scope(), + "/workspace/attachments/2026-06-14/m1-0-diagram.png", + ) + .await + .expect_err("a mount that grants no read must not serve bytes"); + assert_eq!(error.status_code, 403); + assert_eq!(error.code, ProductSurfaceErrorCode::Forbidden); + assert_eq!(error.kind, ProductSurfaceErrorKind::ParticipantDenied); + } + + #[tokio::test] + async fn product_reader_maps_a_malformed_storage_key_to_a_sanitized_500() { + // `storage_key` reaches here from a stored attachment ref, so a + // corrupted one must fail closed at `ScopedPath::new` rather than be + // handed to the filesystem — and it must classify as Internal, not as + // NotFound/Forbidden, which would tell the caller something false about + // an attachment that may well exist. + // + // Note on what is NOT asserted: that the rejected value is absent from + // the error. `ProductSurfaceError` has no reason-bearing field and + // `internal_from` logs its source and discards it, so any such check + // could never fail — the non-leak is structural, not behavioral, and an + // assertion for it would be vacuous. Equality against `internal()` + // pins the whole sanitized shape instead, so a future variant that + // *did* add a detail field would fail here. + let reader = + ProjectScopedAttachmentReader::new(workspace_fs(MountPermissions::read_write())); + let error = InboundAttachmentReader::read( + &reader, + &thread_scope(), + "https://evil.example.com/attachment.png", + ) + .await + .expect_err("a URL is not a scoped path"); + assert_eq!(error, ProductSurfaceError::internal()); + assert_eq!(error.status_code, 500); + } +} diff --git a/crates/ironclaw_product/src/scoped_fs/mod.rs b/crates/ironclaw_product/src/scoped_fs/mod.rs index b9270f1b42b..68255924618 100644 --- a/crates/ironclaw_product/src/scoped_fs/mod.rs +++ b/crates/ironclaw_product/src/scoped_fs/mod.rs @@ -1,16 +1,20 @@ //! Scoped project-filesystem adapters over the `RootFilesystem` mount plane. //! -//! These implement this crate's own `ProjectFilesystemReader` / -//! `InboundAttachmentLander` ports: alias confinement, sensitive-filename -//! omission, the two-stage size guard, extension→MIME derivation, and the -//! substrate→port error sanitization table. That is contract policy owned by -//! the port, not deployment shape — composition still chooses the backend and -//! hands these a `ScopedFilesystem`. +//! These implement this crate's own `ProjectFilesystemReader` port and +//! `ironclaw_attachments`' `InboundAttachmentReader`: alias confinement, +//! sensitive-filename omission, the two-stage size guard, extension→MIME +//! derivation, and the substrate→port error sanitization table. That is +//! contract policy owned by the port, not deployment shape — composition still +//! chooses the backend and hands these a `ScopedFilesystem`. +//! +//! The write half (`ProjectScopedAttachmentLander`) moved to +//! `ironclaw_attachments` with the WS5 widening; the reader stays because it +//! also implements `ironclaw_loop_host`'s `LoopAttachmentReadPort`. -pub mod attachment_landing; +pub mod attachment_reader; pub mod project_filesystem_reader; -pub use attachment_landing::{ProjectScopedAttachmentLander, ProjectScopedAttachmentReader}; +pub use attachment_reader::ProjectScopedAttachmentReader; pub use project_filesystem_reader::{ ProjectScopedFilesystemReader, file_name_of, guard_readable_file, map_filesystem_error, map_kind, mime_for_path, diff --git a/crates/ironclaw_product/src/workflow.rs b/crates/ironclaw_product/src/workflow.rs index 51eb4bc9dc0..f03115cfc07 100644 --- a/crates/ironclaw_product/src/workflow.rs +++ b/crates/ironclaw_product/src/workflow.rs @@ -594,21 +594,6 @@ fn direct_base_binding_request( Ok(request) } -fn delivered_route_conversation_ref( - envelope: &ProductInboundEnvelope, -) -> Result { - let external_ref = envelope.external_conversation_ref(); - ironclaw_conversations::ExternalConversationRef::new( - external_ref.space_id(), - external_ref.conversation_id(), - external_ref.topic_id(), - None, - ) - .map_err(|error| ProductSurfaceFailure::InvalidBindingRequest { - reason: error.to_string(), - }) -} - async fn delivered_route_base_binding( envelope: &ProductInboundEnvelope, binding_service: &dyn ConversationBindingService, @@ -730,17 +715,13 @@ async fn load_delivered_routes_for_envelope( // are not silently dropped before the predicate runs. gate_kind_filter: fn(&str) -> bool, ) -> Result, ProductSurfaceFailure> { - let conversation_ref = match delivered_route_conversation_ref(envelope) { - Ok(conversation_ref) => conversation_ref, - Err(error) => { - debug!( - error = %error, - "delivered gate route fallback skipped because conversation reference was invalid" - ); - return Ok(Vec::new()); - } - }; - let conversation_fingerprint = conversation_ref.conversation_fingerprint(); + // The gate-route index is keyed by the *route* fingerprint, which already + // excludes the reply-target hint — the envelope's ref needs no rebuilding + // and, since the unification, no revalidation: it is the same validated + // type the recording side wrote. + let conversation_fingerprint = envelope + .external_conversation_ref() + .conversation_fingerprint(); let now = Utc::now(); let all_routes = match delivered_gate_routes .load_delivered_gate_route_by_conversation_fingerprint( diff --git a/crates/ironclaw_product/tests/product_surface_contract.rs b/crates/ironclaw_product/tests/product_surface_contract.rs index 806285c854c..8b7fb277252 100644 --- a/crates/ironclaw_product/tests/product_surface_contract.rs +++ b/crates/ironclaw_product/tests/product_surface_contract.rs @@ -13,6 +13,7 @@ use ironclaw_conversations::{ ConversationBindingService as ConversationBindingPort, ExternalActorBindingEpoch, InMemoryConversationServices, }; +use ironclaw_extension_contracts::external::{ExternalActorRef, ExternalConversationRef}; use ironclaw_filesystem::{InMemoryBackend, ScopedFilesystem}; use ironclaw_host_api::turn::{ AcceptedMessageRef, EventCursor, LoopGateRef, RunProfileId, RunProfileVersion, TurnActor, @@ -44,15 +45,14 @@ use ironclaw_product::{ }; use ironclaw_product::{ AdapterInstallationId, ApprovalDecision, ApprovalResolutionPayload, AuthRequirement, - AuthResolutionPayload, AuthResolutionResult, ExternalActorRef, ExternalConversationRef, - ExternalEventId, InboundCommandPayload, LinkedThreadActionPayload, ParsedProductInbound, - ProductAdapterError, ProductAdapterId, ProductControlActionPayload, ProductInboundAck, - ProductInboundEnvelope, ProductInboundPayload, ProductProjectionReadInput, - ProductProjectionSubject, ProductProjectionSubscribeInput, ProductRejection, - ProductRejectionDisposition, ProductRejectionKind, ProductSurfaceRejectionKind, - ProductTriggerReason, ProjectionCursor, ProjectionReadPayload, ProjectionSubscriptionPayload, - ProtocolAuthEvidence, ScopedApprovalResolutionPayload, TrustedInboundContext, - UserMessagePayload, + AuthResolutionPayload, AuthResolutionResult, ExternalEventId, InboundCommandPayload, + LinkedThreadActionPayload, ParsedProductInbound, ProductAdapterError, ProductAdapterId, + ProductControlActionPayload, ProductInboundAck, ProductInboundEnvelope, ProductInboundPayload, + ProductProjectionReadInput, ProductProjectionSubject, ProductProjectionSubscribeInput, + ProductRejection, ProductRejectionDisposition, ProductRejectionKind, + ProductSurfaceRejectionKind, ProductTriggerReason, ProjectionCursor, ProjectionReadPayload, + ProjectionSubscriptionPayload, ProtocolAuthEvidence, ScopedApprovalResolutionPayload, + TrustedInboundContext, UserMessagePayload, }; use ironclaw_product_contracts::action::{ ActionFingerprintKey, AuthRequestRef, LinkedThreadActionId, ProductCommandName, @@ -1052,14 +1052,9 @@ fn auth_thread_reply_envelope(event_suffix: &str, gate_ref: &str) -> ProductInbo } fn delivered_gate_thread_fingerprint() -> String { - ironclaw_conversations::ExternalConversationRef::new( - None, - "conv1", - Some("delivered-gate-thread"), - None, - ) - .expect("conversation route") - .conversation_fingerprint() + ExternalConversationRef::new(None, "conv1", Some("delivered-gate-thread"), None) + .expect("conversation route") + .conversation_fingerprint() } async fn record_conversation_route_for_gate_ref( @@ -1311,7 +1306,7 @@ async fn auth_deny_from_threaded_direct_prompt_uses_base_direct_binding() { ironclaw_conversations::AdapterKind::new("test_adapter").expect("adapter"), ironclaw_conversations::AdapterInstallationId::new("install_alpha") .expect("installation"), - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -2123,14 +2118,9 @@ async fn scoped_approval_actor_mismatch_filtered_out() { ), recorded_at: Utc::now(), delivered_conversation_fingerprints: vec![ - ironclaw_conversations::ExternalConversationRef::new( - None, - "conv1", - Some("delivered-gate-thread"), - None, - ) - .expect("conversation route") - .conversation_fingerprint(), + ExternalConversationRef::new(None, "conv1", Some("delivered-gate-thread"), None) + .expect("conversation route") + .conversation_fingerprint(), ], }) .await @@ -2236,14 +2226,9 @@ async fn auth_two_live_routes_same_conversation_rejects_ambiguous() { ), recorded_at: Utc::now(), delivered_conversation_fingerprints: vec![ - ironclaw_conversations::ExternalConversationRef::new( - None, - "conv1", - Some("delivered-gate-thread"), - None, - ) - .expect("conversation ref") - .conversation_fingerprint(), + ExternalConversationRef::new(None, "conv1", Some("delivered-gate-thread"), None) + .expect("conversation ref") + .conversation_fingerprint(), ], }; let expected_fingerprint = delivered_gate_thread_fingerprint(); @@ -3875,7 +3860,7 @@ async fn projection_subscription_requires_existing_conversation_binding() { TenantId::new("tenant:alpha").expect("tenant"), ironclaw_conversations::AdapterKind::new("test_adapter").expect("adapter"), ironclaw_conversations::AdapterInstallationId::new("install_alpha").expect("install"), - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -4069,7 +4054,7 @@ async fn actor_user_resolver_rewrites_pairing_after_explicit_unpair() { &TenantId::new("tenant:alpha").expect("tenant"), &ironclaw_conversations::AdapterKind::new("test_adapter").expect("adapter"), &ironclaw_conversations::AdapterInstallationId::new("install_alpha").expect("install"), - &ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + &ExternalActorRef::new("test", "user1", None::).expect("actor"), ) .await; @@ -4170,12 +4155,10 @@ async fn actor_user_resolver_rechecks_revocation_before_turn_submission() { "install_alpha", ) .expect("install"), - external_actor_ref: ironclaw_conversations::ExternalActorRef::new("test", "user1") + external_actor_ref: ExternalActorRef::new("test", "user1", None::) .expect("actor"), - external_conversation_ref: ironclaw_conversations::ExternalConversationRef::new( - None, "conv1", None, None, - ) - .expect("conversation"), + external_conversation_ref: ExternalConversationRef::new(None, "conv1", None, None) + .expect("conversation"), external_event_id: ironclaw_conversations::ExternalEventId::new( "evt:resolver-revoked-mid-resolution-lookup", ) @@ -4221,12 +4204,10 @@ async fn actor_user_resolver_revalidation_cannot_unpair_a_newer_generation() { "install_alpha", ) .expect("install"), - external_actor_ref: ironclaw_conversations::ExternalActorRef::new("test", "user1") + external_actor_ref: ExternalActorRef::new("test", "user1", None::) .expect("actor"), - external_conversation_ref: ironclaw_conversations::ExternalConversationRef::new( - None, "conv1", None, None, - ) - .expect("conversation"), + external_conversation_ref: ExternalConversationRef::new(None, "conv1", None, None) + .expect("conversation"), external_event_id: ironclaw_conversations::ExternalEventId::new( "evt:resolver-replaced-mid-resolution-lookup", ) @@ -4319,7 +4300,7 @@ async fn lookup_binding_with_actor_user_resolver_rejects_a_stale_actor_pairing() TenantId::new("tenant:alpha").expect("tenant"), ironclaw_conversations::AdapterKind::new("test_adapter").expect("adapter"), ironclaw_conversations::AdapterInstallationId::new("install_alpha").expect("install"), - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:paired-bob").expect("user"), ) .await; @@ -4427,7 +4408,7 @@ async fn concrete_product_surface_accepts_user_message_for_trusted_installation( TenantId::new("tenant:alpha").expect("tenant"), ironclaw_conversations::AdapterKind::new("test_adapter").expect("adapter"), ironclaw_conversations::AdapterInstallationId::new("install_alpha").expect("install"), - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -4495,7 +4476,7 @@ async fn concrete_product_surface_accepts_shared_route_participant_on_existing_t tenant_id.clone(), adapter_kind.clone(), installation_id.clone(), - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -4504,7 +4485,7 @@ async fn concrete_product_surface_accepts_shared_route_participant_on_existing_t tenant_id.clone(), adapter_kind, installation_id, - ironclaw_conversations::ExternalActorRef::new("test", "user2").expect("actor"), + ExternalActorRef::new("test", "user2", None::).expect("actor"), UserId::new("user:bob").expect("user"), ) .await; @@ -4591,7 +4572,7 @@ async fn concrete_product_surface_persists_first_bind_default_scope() { TenantId::new("tenant:alpha").expect("tenant"), ironclaw_conversations::AdapterKind::new("test_adapter").expect("adapter"), ironclaw_conversations::AdapterInstallationId::new("install_alpha").expect("install"), - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -4673,7 +4654,7 @@ async fn concrete_product_surface_keeps_installations_tenant_isolated() { TenantId::new(tenant).expect("tenant"), ironclaw_conversations::AdapterKind::new("test_adapter").expect("adapter"), ironclaw_conversations::AdapterInstallationId::new(install).expect("install"), - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new(user).expect("user"), ) .await; @@ -4752,7 +4733,7 @@ async fn shared_route_without_configured_subject_requires_binding() { tenant_id.clone(), adapter_kind, installation_id, - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -4802,7 +4783,7 @@ async fn shared_route_uses_conversation_specific_subject_over_installation_defau tenant_id.clone(), adapter_kind, installation_id, - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -4864,7 +4845,7 @@ async fn static_shared_route_does_not_probe_existing_binding_before_resolve() { tenant_id.clone(), adapter_kind, installation_id, - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -4927,7 +4908,7 @@ async fn shared_route_uses_dynamic_subject_route_resolver_without_rebuilding_sco tenant_id.clone(), adapter_kind, installation_id, - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -5186,7 +5167,7 @@ async fn shared_route_can_disable_default_subject_for_unrouted_conversations() { tenant_id.clone(), adapter_kind, installation_id, - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -5391,7 +5372,7 @@ async fn shared_lookup_binding_rejects_existing_binding_when_resolved_actor_diff tenant_id.clone(), adapter_kind.clone(), installation_id.clone(), - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -5413,9 +5394,9 @@ async fn shared_lookup_binding_rejects_existing_binding_when_resolved_actor_diff tenant_id: tenant_id.clone(), adapter_kind, adapter_installation_id: installation_id, - external_actor_ref: ironclaw_conversations::ExternalActorRef::new("test", "user1") + external_actor_ref: ExternalActorRef::new("test", "user1", None::) .expect("actor"), - external_conversation_ref: ironclaw_conversations::ExternalConversationRef::new( + external_conversation_ref: ExternalConversationRef::new( Some("T-team"), "C-eng", Some("thread-1"), @@ -5479,7 +5460,7 @@ async fn lookup_binding_does_not_backfill_legacy_ownerless_shared_route() { tenant_id.clone(), adapter_kind.clone(), installation_id.clone(), - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -5489,9 +5470,9 @@ async fn lookup_binding_does_not_backfill_legacy_ownerless_shared_route() { tenant_id: tenant_id.clone(), adapter_kind, adapter_installation_id: installation_id, - external_actor_ref: ironclaw_conversations::ExternalActorRef::new("test", "user1") + external_actor_ref: ExternalActorRef::new("test", "user1", None::) .expect("actor"), - external_conversation_ref: ironclaw_conversations::ExternalConversationRef::new( + external_conversation_ref: ExternalConversationRef::new( Some("T-team"), "C-eng", Some("thread-legacy"), @@ -5572,7 +5553,7 @@ async fn direct_route_skips_dynamic_subject_route_resolver() { tenant_id.clone(), adapter_kind, installation_id, - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -5700,7 +5681,7 @@ async fn concrete_product_surface_reply_to_bot_requires_existing_binding() { TenantId::new("tenant:alpha").expect("tenant"), ironclaw_conversations::AdapterKind::new("test_adapter").expect("adapter"), ironclaw_conversations::AdapterInstallationId::new("install_alpha").expect("install"), - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -5865,7 +5846,7 @@ async fn concrete_product_surface_rejects_unknown_installation_as_terminal() { TenantId::new("tenant:alpha").expect("tenant"), ironclaw_conversations::AdapterKind::new("test_adapter").expect("adapter"), ironclaw_conversations::AdapterInstallationId::new("install_alpha").expect("install"), - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -5960,7 +5941,7 @@ async fn terminal_rejection_for_unpaired_actor_does_not_poison_other_actor_event TenantId::new("tenant:alpha").expect("tenant"), ironclaw_conversations::AdapterKind::new("test_adapter").expect("adapter"), ironclaw_conversations::AdapterInstallationId::new("install_alpha").expect("install"), - ironclaw_conversations::ExternalActorRef::new("test", "user2").expect("actor"), + ExternalActorRef::new("test", "user2", None::).expect("actor"), UserId::new("user:bob").expect("user"), ) .await; @@ -6036,7 +6017,7 @@ async fn accepted_message_replay_validates_current_actor_before_submit() { TenantId::new("tenant:alpha").expect("tenant"), ironclaw_conversations::AdapterKind::new("test_adapter").expect("adapter"), ironclaw_conversations::AdapterInstallationId::new("install_alpha").expect("install"), - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -6100,7 +6081,7 @@ async fn concrete_product_surface_replays_binding_access_denied_rejection() { TenantId::new("tenant:alpha").expect("tenant"), ironclaw_conversations::AdapterKind::new("test_adapter").expect("adapter"), ironclaw_conversations::AdapterInstallationId::new("install_alpha").expect("install"), - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), UserId::new("user:alice").expect("user"), ) .await; @@ -6135,7 +6116,7 @@ async fn concrete_product_surface_replays_binding_access_denied_rejection() { TenantId::new("tenant:alpha").expect("tenant"), ironclaw_conversations::AdapterKind::new("test_adapter").expect("adapter"), ironclaw_conversations::AdapterInstallationId::new("install_alpha").expect("install"), - ironclaw_conversations::ExternalActorRef::new("test", "user2").expect("actor"), + ExternalActorRef::new("test", "user2", None::).expect("actor"), UserId::new("user:bob").expect("user"), ) .await; @@ -6605,7 +6586,7 @@ impl ProductActorUserResolver for ReplacingProductActorUserResolver { ironclaw_conversations::AdapterKind::new("test_adapter").expect("adapter"), ironclaw_conversations::AdapterInstallationId::new("install_alpha") .expect("install"), - ironclaw_conversations::ExternalActorRef::new("test", "user1").expect("actor"), + ExternalActorRef::new("test", "user1", None::).expect("actor"), self.user_id.clone(), epoch.clone(), ) diff --git a/crates/ironclaw_product/tests/reborn_services_contract.rs b/crates/ironclaw_product/tests/reborn_services_contract.rs index c59b95695ae..15019a90aa7 100644 --- a/crates/ironclaw_product/tests/reborn_services_contract.rs +++ b/crates/ironclaw_product/tests/reborn_services_contract.rs @@ -21,6 +21,7 @@ use ironclaw_approvals::{ PersistentApprovalPolicyKey, PersistentApprovalPolicyStorePort, ToolPermissionOverride, ToolPermissionOverrideInput, ToolPermissionOverrideKey, ToolPermissionOverrideStorePort, }; +use ironclaw_attachments::{InboundAttachmentLander, InboundAttachmentReader}; use ironclaw_auth::{ AuthAccountLastError, AuthAccountState, CredentialAccountId, CredentialAccountProjection, CredentialAccountStatus, @@ -65,10 +66,9 @@ use ironclaw_product::{ EXTENSION_SETUP_SUBMIT_CAPABILITY_ID, EXTENSION_SETUP_VIEW, EXTENSIONS_VIEW, EmptyProductCommandInput, ExtensionCredentialSetupService, ExtensionCredentialStatusRequest, ExtensionCredentialSubmitRequest, FS_LIST_VIEW, FS_MOUNTS_VIEW, FS_STAT_VIEW, - FilesystemBrowseReader, FsMount, GLOBAL_AUTO_APPROVE_VIEW, InboundAttachmentLander, - InboundAttachmentReader, LLM_ACTIVE_SET_CAPABILITY_ID, LLM_CONFIG_VIEW, - LLM_PROVIDER_DELETE_CAPABILITY_ID, LLM_PROVIDER_UPSERT_CAPABILITY_ID, LOGS_VIEW, - LifecycleChannelDirections, LifecycleExtensionCredentialRequirement, + FilesystemBrowseReader, FsMount, GLOBAL_AUTO_APPROVE_VIEW, LLM_ACTIVE_SET_CAPABILITY_ID, + LLM_CONFIG_VIEW, LLM_PROVIDER_DELETE_CAPABILITY_ID, LLM_PROVIDER_UPSERT_CAPABILITY_ID, + LOGS_VIEW, LifecycleChannelDirections, LifecycleExtensionCredentialRequirement, LifecycleExtensionCredentialSetup, LifecycleExtensionOnboarding, LifecycleExtensionRuntimeKind, LifecycleExtensionSource, LifecycleExtensionSummary, LifecycleInstalledExtensionSummary, LifecyclePackageKind, LifecyclePackageRef, LifecycleProductAction, LifecycleProductPayload, @@ -14837,8 +14837,8 @@ impl InboundAttachmentLander for RecordingLander { &self, _thread_scope: &ThreadScope, _referenced_storage_keys: &[String], - ) -> Result { - Ok(ironclaw_product::AttachmentCleanupReport::default()) + ) -> Result { + Ok(ironclaw_attachments::AttachmentCleanupReport::default()) } } diff --git a/crates/ironclaw_product/tests/run_delivery_contract.rs b/crates/ironclaw_product/tests/run_delivery_contract.rs index 60633552e99..409fe18fd69 100644 --- a/crates/ironclaw_product/tests/run_delivery_contract.rs +++ b/crates/ironclaw_product/tests/run_delivery_contract.rs @@ -550,6 +550,16 @@ fn envelope_for_conversation( payload: ProductInboundPayload, event_id: &str, conversation_id: &str, +) -> ProductInboundEnvelope { + envelope_for_conversation_replying_to(payload, event_id, conversation_id, None, None) +} + +fn envelope_for_conversation_replying_to( + payload: ProductInboundPayload, + event_id: &str, + conversation_id: &str, + topic_id: Option<&str>, + reply_target_message_id: Option<&str>, ) -> ProductInboundEnvelope { let adapter_id = ProductAdapterId::new("acme_v1").expect("adapter"); let installation_id = AdapterInstallationId::new("install_alpha").expect("installation"); @@ -569,8 +579,13 @@ fn envelope_for_conversation( let parsed = ParsedProductInbound::new( ExternalEventId::new(event_id).expect("event"), ExternalActorRef::new("acme_user", "U-1", None::).expect("actor"), - ExternalConversationRef::new(Some("space-1"), conversation_id, None, None) - .expect("conversation"), + ExternalConversationRef::new( + Some("space-1"), + conversation_id, + topic_id, + reply_target_message_id, + ) + .expect("conversation"), payload, ) .expect("parsed"); @@ -1252,10 +1267,30 @@ async fn observer_records_gate_route_after_approval_prompt() { ); let run_id = TurnRunId::new(); + // A *threaded* prompting event that is itself a reply. Both halves are + // load-bearing: + // + // - the topic (`1700.1`) makes the source branch's key distinguishable from + // the delivered-message loop's, which only ever keys the topic off a + // vendor message ref (`ts-N` here) or leaves it empty. Without a topic the + // two branches produce the same conversation-root key and an assertion on + // it passes no matter what the source branch does. + // - the reply target (`1800.2`) is the per-event id the recorded route must + // NOT inherit: a later bare `approve` in the same topic carries a + // different one (or none), and a key that varied with it would never match. harness .observer .observe_ack( - user_message_envelope(ProductTriggerReason::DirectChat, "evt-gate"), + envelope_for_conversation_replying_to( + ProductInboundPayload::UserMessage( + UserMessagePayload::new("hello", Vec::new(), ProductTriggerReason::DirectChat) + .expect("payload"), + ), + "evt-gate", + "conv-1", + Some("1700.1"), + Some("1800.2"), + ), accepted_ack(run_id), ) .await; @@ -1283,16 +1318,126 @@ async fn observer_records_gate_route_after_approval_prompt() { !route.delivered_conversation_fingerprints.is_empty(), "fingerprints recorded" ); - // The source conversation (bare replies next to the prompt) routes too. + // The source topic (bare replies next to the prompt) routes too, keyed by + // the topic and WITHOUT the prompting event's reply target. Only the source + // branch can produce this key — the delivered loop's topics are vendor + // message refs. let source_fingerprint = - ironclaw_conversations::ExternalConversationRef::new(Some("space-1"), "conv-1", None, None) + ExternalConversationRef::new(Some("space-1"), "conv-1", Some("1700.1"), None) .expect("conversation") .conversation_fingerprint(); + // Non-vacuity for the membership check below: if the topic did NOT + // participate in the fingerprint, `source_fingerprint` would just be the + // untargeted conversation's key, which the route records anyway — so the + // assertion would pass while proving nothing about topic routing. + assert_ne!( + source_fingerprint, + ExternalConversationRef::new(Some("space-1"), "conv-1", None, None) + .expect("conversation") + .conversation_fingerprint(), + "the conversation topic must participate in the fingerprint" + ); assert!( route .delivered_conversation_fingerprints .contains(&source_fingerprint), - "source conversation fingerprint recorded" + "a gate route recorded from a threaded reply must still be resolvable by a \ + bare reply in the same topic that carries no reply target: {:?}", + route.delivered_conversation_fingerprints + ); + // The invariant the assertion above leans on, pinned here rather than left + // to be re-derived from `conversation_fingerprint`'s body: the fingerprint + // is the ROUTE, so it does not vary with the per-event reply target. If that + // ever stopped holding, the recording branch would start baking a message id + // into a stable key and the failure above would look unrelated to the cause. + assert_eq!( + ExternalConversationRef::new(Some("space-1"), "conv-1", Some("1700.1"), Some("1800.2")) + .expect("conversation") + .conversation_fingerprint(), + source_fingerprint, + "conversation_fingerprint must exclude the reply-target hint" + ); +} + +#[tokio::test] +async fn observer_records_gate_route_without_a_vendor_ref_that_cannot_key_a_route() { + // `vendor_message_ref` is an unvalidated vendor string, so a channel can + // hand back a ref that is not a legal route segment (here: a control + // character). The two topic-keyed route variants must then be DROPPED + // rather than recorded malformed -- and, crucially, the conversation-root + // variants must still be recorded, or one bad ref would silently cost the + // gate every route and a bare `approve` would resolve nothing. + let harness = build_harness( + vec![scripted_state( + TurnStatus::BlockedApproval, + Some("gate:approval-00000000000000000000000000000001"), + )], + false, + None, + Duration::from_millis(40), + ); + harness + .adapter + .reports + .lock() + .expect("reports lock") + .push_back(DeliveryReport { + parts: vec![PartDeliveryOutcome::Sent { + vendor_message_ref: Some("ts-\u{7}1".to_string()), + }], + }); + let run_id = TurnRunId::new(); + + harness + .observer + .observe_ack( + envelope_for_conversation_replying_to( + ProductInboundPayload::UserMessage( + UserMessagePayload::new("hello", Vec::new(), ProductTriggerReason::DirectChat) + .expect("payload"), + ), + "evt-gate-bad-ref", + "conv-1", + Some("1700.1"), + None, + ), + accepted_ack(run_id), + ) + .await; + + let route = harness + .route_store + .load_delivered_gate_route( + &tenant(), + &user(), + "gate:approval-00000000000000000000000000000001", + ) + .await + .expect("route lookup") + .expect("gate route recorded"); + let fingerprint = |space: Option<&str>, topic: Option<&str>| { + ExternalConversationRef::new(space, "conv-1", topic, None) + .expect("conversation") + .conversation_fingerprint() + }; + let mut recorded = route.delivered_conversation_fingerprints.clone(); + recorded.sort(); + let mut expected = vec![ + // Delivered loop, space-qualified conversation root. + fingerprint(Some("space-1"), None), + // Delivered loop, no-space fallback. + fingerprint(None, None), + // The prompting (source) conversation, keyed by its own topic. + fingerprint(Some("space-1"), Some("1700.1")), + ]; + expected.sort(); + assert_eq!( + recorded, expected, + "an unusable vendor message ref drops only the two ref-keyed variants" + ); + assert!( + !recorded.iter().any(|entry| entry.contains('\u{7}')), + "a vendor ref that is not a legal external id must never reach a route key: {recorded:?}" ); } diff --git a/crates/ironclaw_reborn_composition/src/automation/trigger_poller_trusted_submit.rs b/crates/ironclaw_reborn_composition/src/automation/trigger_poller_trusted_submit.rs index ebf9e638fce..a9d0f253acb 100644 --- a/crates/ironclaw_reborn_composition/src/automation/trigger_poller_trusted_submit.rs +++ b/crates/ironclaw_reborn_composition/src/automation/trigger_poller_trusted_submit.rs @@ -2,21 +2,23 @@ use std::sync::Arc; use async_trait::async_trait; use ironclaw_conversations::{ - AcceptedInboundMessage, AdapterInstallationId, AdapterKind, ConversationBindingResolution, - ConversationBindingService, ConversationRouteKind, ExternalActorRef, ExternalConversationRef, - ExternalEventId, InboundTurnError, ResolveConversationRequest, + AcceptedConversationMessage, AdapterInstallationId, AdapterKind, ConversationBindingResolution, + ConversationBindingService, ConversationRouteKind, ExternalEventId, InboundTurnError, + ResolveConversationRequest, }; +use ironclaw_extension_contracts::external::{ExternalActorRef, ExternalConversationRef}; use ironclaw_host_api::{ Timestamp, ids::{AgentId, ProjectId, TenantId, UserId}, + product_adapter_error::ProductAdapterError, }; use ironclaw_product::automation_trigger_thread_metadata_json; use ironclaw_safety::{ InjectionScanner, PromptSafetyRejection, Sanitizer, validate_trusted_trigger_prompt, }; use ironclaw_threads::{ - AcceptInboundMessageRequest as ThreadAcceptInboundMessageRequest, EnsureThreadRequest, - MessageContent, SessionThreadService as CanonicalSessionThreadService, ThreadScope, + AcceptInboundMessageRequest, AcceptedInboundMessage, EnsureThreadRequest, MessageContent, + SessionThreadService, ThreadScope, }; use ironclaw_triggers::{ TriggerError, TriggerFire, TriggerId, TriggerMaterializedPrompt, TriggerPromptMaterializer, @@ -133,7 +135,7 @@ impl TriggerFireAuthorizer for TenantScopedTrustedTriggerFireAuthorizer { pub(crate) struct ConversationContentRefMaterializer { binding_service: B, - thread_service: Arc, + thread_service: Arc, default_agent_id: AgentId, prompt_safety: Arc, authorizer: Arc, @@ -145,7 +147,7 @@ where { pub(crate) fn new( binding_service: B, - thread_service: Arc, + thread_service: Arc, default_agent_id: AgentId, authorizer: Arc, ) -> Self { @@ -238,11 +240,13 @@ fn trigger_conversation_fields( adapter_installation_id: conversation_id(AdapterInstallationId::new( trusted_inbound_binding.adapter_installation_id(), ))?, - external_actor_ref: conversation_id(ExternalActorRef::new( + external_actor_ref: external_ref(ExternalActorRef::new( trusted_inbound_binding.external_actor_namespace(), trusted_inbound_binding.external_actor_id(), + // A trigger fire has no human display name to carry. + None::, ))?, - external_conversation_ref: conversation_id(ExternalConversationRef::new( + external_conversation_ref: external_ref(ExternalConversationRef::new( None, trusted_inbound_binding.external_conversation_id(), Some(trusted_inbound_binding.route_thread_id()), @@ -274,14 +278,14 @@ fn trigger_resolve_request( } async fn record_trigger_prompt( - thread_service: Arc, + thread_service: Arc, resolution: &ConversationBindingResolution, trigger_id: TriggerId, prompt: &str, external_event_id: &str, default_agent_id: &AgentId, - accepted_message: Option<&AcceptedInboundMessage>, -) -> Result { + accepted_message: Option<&AcceptedConversationMessage>, +) -> Result { let agent_id = resolution .turn_scope .agent_id @@ -307,7 +311,7 @@ async fn record_trigger_prompt( reason: format!("trigger prompt thread ensure failed: {error}"), })?; thread_service - .accept_inbound_message(ThreadAcceptInboundMessageRequest { + .accept_inbound_message(AcceptInboundMessageRequest { scope, thread_id: resolution.turn_scope.thread_id.clone(), actor_id: resolution.actor.user_id.as_str().to_string(), @@ -429,6 +433,17 @@ fn conversation_id(result: Result) -> Result(result: Result) -> Result { + result.map_err(|error| TriggerError::InvalidMaterialization { + reason: error.to_string(), + }) +} + /// Test-support materializer: runs the REAL trusted-trigger materialization /// pipeline (`ConversationContentRefMaterializer::materialize_prompt` — /// authorize, validate, resolve-or-create binding, record the prompt as a @@ -452,7 +467,7 @@ fn conversation_id(result: Result) -> Result( binding_service: B, - thread_service: Arc, + thread_service: Arc, default_agent_id: AgentId, fire: TriggerFire, ) -> Result<(TriggerMaterializedPrompt, TurnScope), TriggerError> @@ -485,16 +500,15 @@ mod tests { use ironclaw_product::AUTOMATION_TRIGGER_THREAD_SOURCE_TAG; use ironclaw_safety::{InjectionWarning, Severity}; use ironclaw_threads::{ - AcceptedInboundMessage as CanonicalAcceptedInboundMessage, - AcceptedInboundMessageReplay as CanonicalAcceptedInboundMessageReplay, - AppendAssistantDraftRequest, AppendCapabilityDisplayPreviewRequest, - AppendToolResultReferenceRequest, ContextMessages, ContextWindow, - CreateSummaryArtifactRequest, InMemorySessionThreadService, LatestThreadMessageRequest, - ListThreadsForScopeRequest, ListThreadsForScopeResponse, LoadContextMessagesRequest, - LoadContextWindowRequest, RedactMessageRequest, ReplayAcceptedInboundMessageRequest, - SessionThreadError, SessionThreadRecord, SummaryArtifact, ThreadGoal, ThreadHistoryRequest, - ThreadMessageId, ThreadMessageRange, ThreadMessageRangeRequest, ThreadMessageRecord, - UpdateAssistantDraftRequest, UpdateThreadGoalRequest, UpdateToolResultReferenceRequest, + AcceptedInboundMessageReplay, AppendAssistantDraftRequest, + AppendCapabilityDisplayPreviewRequest, AppendToolResultReferenceRequest, ContextMessages, + ContextWindow, CreateSummaryArtifactRequest, InMemorySessionThreadService, + LatestThreadMessageRequest, ListThreadsForScopeRequest, ListThreadsForScopeResponse, + LoadContextMessagesRequest, LoadContextWindowRequest, RedactMessageRequest, + ReplayAcceptedInboundMessageRequest, SessionThreadError, SessionThreadRecord, + SummaryArtifact, ThreadGoal, ThreadHistoryRequest, ThreadMessageId, ThreadMessageRange, + ThreadMessageRangeRequest, ThreadMessageRecord, UpdateAssistantDraftRequest, + UpdateThreadGoalRequest, UpdateToolResultReferenceRequest, }; use ironclaw_triggers::{ InMemoryTriggerRepository, NoopTriggerFireSettlementObserver, @@ -1066,7 +1080,7 @@ mod tests { } #[async_trait] - impl CanonicalSessionThreadService for InterceptingPromptThreadService { + impl SessionThreadService for InterceptingPromptThreadService { async fn ensure_thread( &self, request: EnsureThreadRequest, @@ -1076,8 +1090,8 @@ mod tests { async fn accept_inbound_message( &self, - _request: ThreadAcceptInboundMessageRequest, - ) -> Result { + _request: AcceptInboundMessageRequest, + ) -> Result { Err(SessionThreadError::Backend( "prompt thread write failed".to_string(), )) @@ -1086,7 +1100,7 @@ mod tests { async fn replay_accepted_inbound_message( &self, _request: ReplayAcceptedInboundMessageRequest, - ) -> Result, SessionThreadError> { + ) -> Result, SessionThreadError> { unimplemented!("trigger prompt recorder tests do not replay canonical inbound messages") } @@ -1389,6 +1403,7 @@ mod tests { ExternalActorRef::new( TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, creator_user_id.as_str(), + None::, ) .expect("actor ref"), creator_user_id.clone(), @@ -1523,6 +1538,7 @@ mod tests { ExternalActorRef::new( TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, creator_user_id.as_str(), + None::, ) .expect("actor ref"), creator_user_id.clone(), @@ -1600,7 +1616,7 @@ mod tests { reply_target_binding_ref: reply_target_binding_ref.clone(), access: ThreadAccessDecision::Allowed, }; - let accepted_message = AcceptedInboundMessage { + let accepted_message = AcceptedConversationMessage { tenant_id, thread_id: thread_id.clone(), actor: TurnActor::new(actor_user_id), @@ -1677,6 +1693,7 @@ mod tests { ExternalActorRef::new( TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, creator_user_id.as_str(), + None::, ) .expect("actor ref"), creator_user_id.clone(), @@ -1800,6 +1817,7 @@ mod tests { ExternalActorRef::new( TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, creator_user_id.as_str(), + None::, ) .expect("actor ref"), creator_user_id.clone(), @@ -1954,6 +1972,7 @@ mod tests { ExternalActorRef::new( TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, creator_user_id.as_str(), + None::, ) .expect("actor ref"), creator_user_id.clone(), @@ -2058,6 +2077,7 @@ mod tests { ExternalActorRef::new( TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, creator_user_id.as_str(), + None::, ) .expect("actor ref"), creator_user_id.clone(), @@ -2134,6 +2154,7 @@ mod tests { ExternalActorRef::new( TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, creator_user_id.as_str(), + None::, ) .expect("actor ref"), creator_user_id.clone(), @@ -2159,4 +2180,92 @@ mod tests { "expected InvalidMaterialization from the real safety validator, got {error:?}" ); } + + /// [`external_ref`] is the `ProductAdapterError` -> `TriggerError` mapping + /// the channel-owned external refs take since the + /// `conversations`/`extension_contracts` unification. The `Ok` arm is + /// exercised by every materialization test above; the `Err` arm was not, + /// so this pins the mapping itself: the trigger failure must carry the + /// source error's rendered text verbatim, and a `RedactedString`-carrying + /// variant must map through the same redacting `Display` so the secret + /// detail never reaches the materialization failure. + #[test] + fn external_ref_maps_product_adapter_error_to_invalid_materialization() { + assert_eq!( + external_ref::(Ok(7)).expect("the Ok arm passes the value through unchanged"), + 7 + ); + + let source = ProductAdapterError::InvalidIdentifier { + kind: "external_actor_id", + reason: "must not be empty".to_string(), + }; + let rendered_source = source.to_string(); + let error = + external_ref::<()>(Err(source)).expect_err("the Err arm maps to a TriggerError"); + assert!( + matches!( + &error, + TriggerError::InvalidMaterialization { reason } if *reason == rendered_source + ), + "expected the source error text verbatim, got {error:?}" + ); + + let redacted = ProductAdapterError::Internal { + detail: ironclaw_host_api::product_adapter_error::RedactedString::new( + "super-secret-token", + ), + }; + let error = + external_ref::<()>(Err(redacted)).expect_err("the Err arm maps to a TriggerError"); + let TriggerError::InvalidMaterialization { reason } = &error else { + panic!("expected InvalidMaterialization, got {error:?}"); + }; + assert!( + reason.contains(ironclaw_host_api::product_adapter_error::REDACTED_PLACEHOLDER) + && !reason.contains("super-secret-token"), + "a redacted detail must not leak into the materialization failure: {reason}" + ); + } + + /// Caller-level companion to the mapping test above: `trigger_conversation_fields` + /// is the only production caller of [`external_ref`], so drive the rejection + /// through it. A binding whose external actor id the `ExternalActorRef` + /// contract rejects must surface as a mapped `InvalidMaterialization` + /// naming the offending field — never a half-built resolve request. + #[test] + fn trigger_conversation_fields_rejects_invalid_external_actor_ref() { + let fire = TriggerFire { + identity: TriggerFireIdentity::new( + TenantId::new("external-ref-tenant").expect("tenant id"), + TriggerId::new(), + Utc::now(), + ), + // `from_trusted` skips `UserId` validation exactly as a host-minted + // sentinel identity does, so the binding reaches + // `ExternalActorRef::new` carrying an id the external-ref contract + // rejects — the production shape of this failure. + creator_user_id: UserId::from_trusted(String::new()), + agent_id: None, + project_id: None, + prompt: "external ref mapping".to_string(), + delivery_target: None, + }; + let binding = TriggerTrustedInboundBinding::for_fire(&fire); + + // `TriggerConversationFields` does not implement `Debug`, so destructure + // rather than `expect_err`. + let Err(error) = trigger_conversation_fields(&fire, &binding) else { + panic!("an empty external actor id must not build conversation fields"); + }; + + assert!( + matches!( + &error, + TriggerError::InvalidMaterialization { reason } + if reason.contains("external_actor_id") + ), + "expected the ExternalActorRef rejection mapped through external_ref, got {error:?}" + ); + } } diff --git a/crates/ironclaw_reborn_composition/src/extension_host_assembly.rs b/crates/ironclaw_reborn_composition/src/extension_host_assembly.rs index 9c373b56302..6a7415d9fbd 100644 --- a/crates/ironclaw_reborn_composition/src/extension_host_assembly.rs +++ b/crates/ironclaw_reborn_composition/src/extension_host_assembly.rs @@ -1,6 +1,7 @@ use std::collections::BTreeSet; use std::sync::Arc; +use ironclaw_attachments::InboundAttachmentLander; use ironclaw_extension_contracts::extension::ExtensionHostAssemblyConfig; use ironclaw_extensions::ExtensionInstallationStorePort; use ironclaw_filesystem::{CompositeRootFilesystem, RootFilesystem, ScopedFilesystem}; @@ -11,8 +12,8 @@ use ironclaw_host_api::{ use ironclaw_host_runtime::{ExtensionLaneToolBinder, HostRuntimeHttpEgressPort}; use ironclaw_product::{ ApprovalInteractionService, AuthChallengeProvider, AuthInteractionService, - BlockedAuthFlowCanceller, ExtensionAccountSetupRegistry, InboundAttachmentLander, - ProjectFilesystemReader, RunDeliverySettings, + BlockedAuthFlowCanceller, ExtensionAccountSetupRegistry, ProjectFilesystemReader, + RunDeliverySettings, }; use ironclaw_product_contracts::account_setup::ExtensionAccountSetupDescriptor; use ironclaw_product_contracts::prompt_source::{ @@ -392,7 +393,7 @@ fn channel_host_source(services: &RebornRuntimeStores) -> Option = Arc::new( - ironclaw_product::ProjectScopedAttachmentLander::new(inbound_filesystem), + ironclaw_attachments::ProjectScopedAttachmentLander::new(inbound_filesystem), ); let project_filesystem: Arc = Arc::new( ironclaw_product::ProjectScopedFilesystemReader::with_max_read_bytes( diff --git a/crates/ironclaw_reborn_composition/src/factory.rs b/crates/ironclaw_reborn_composition/src/factory.rs index 6a15af844b8..c581c521a67 100644 --- a/crates/ironclaw_reborn_composition/src/factory.rs +++ b/crates/ironclaw_reborn_composition/src/factory.rs @@ -82,10 +82,9 @@ use ironclaw_capabilities::{ CapabilityObligationPhase, CapabilityObligationRequest, }; use ironclaw_conversations::RebornFilesystemConversationServices; -use ironclaw_conversations::{ - AdapterInstallationId, AdapterKind, ConversationActorPairingService, ExternalActorRef, -}; +use ironclaw_conversations::{AdapterInstallationId, AdapterKind, ConversationActorPairingService}; use ironclaw_events::{DurableAuditLog, DurableEventLog}; +use ironclaw_extension_contracts::external::ExternalActorRef; use ironclaw_extension_contracts::recipe::RecipeClientCredentials; use ironclaw_extension_host::channel_pairing::ChannelPairingRegistry; use ironclaw_extension_host::{ diff --git a/crates/ironclaw_reborn_composition/src/factory/test_support.rs b/crates/ironclaw_reborn_composition/src/factory/test_support.rs index 2c793222bbf..b384133ec52 100644 --- a/crates/ironclaw_reborn_composition/src/factory/test_support.rs +++ b/crates/ironclaw_reborn_composition/src/factory/test_support.rs @@ -545,7 +545,7 @@ impl RebornRuntimeStores { let read_write_workspace_filesystem = self.read_write_workspace_filesystem()?; Some(AttachmentTestSupport { read_port, - lander: Arc::new(ironclaw_product::ProjectScopedAttachmentLander::new( + lander: Arc::new(ironclaw_attachments::ProjectScopedAttachmentLander::new( read_write_workspace_filesystem, )), }) @@ -603,9 +603,9 @@ impl RebornRuntimeStores { #[cfg(feature = "test-support")] pub(crate) fn standalone_inbound_attachment_reader_for_test( &self, - ) -> Option> { + ) -> Option> { Some(self.standalone_workspace_attachment_reader_for_test()? - as Arc) + as Arc) } /// C-JOURNEY: publish a bundled first-party WASM extension package (e.g. a @@ -792,7 +792,7 @@ fn active_extension_network_policy_for_test( #[derive(Clone)] pub struct AttachmentTestSupport { pub read_port: Arc, - pub lander: Arc, + pub lander: Arc, } #[cfg(feature = "test-support")] @@ -993,3 +993,83 @@ pub(crate) async fn open_standalone_trigger_repository_for_test( let backend = build_default_database_roots(storage_root, &mut composite).await?; trigger_repository_for_durable_backend(&backend).await } + +#[cfg(all(test, feature = "test-support"))] +mod attachment_seam_tests { + use ironclaw_host_api::ids::{AgentId, TenantId, UserId}; + use ironclaw_threads::ThreadScope; + + /// Store-level regression for the two C-ATTACH accessors + /// (`RebornRuntimeStores::standalone_attachment_test_support_for_test` and + /// `RebornRuntimeStores::standalone_inbound_attachment_reader_for_test`). + /// The downstream integration harness reaches this seam through the + /// `RebornRuntime` wrapper's same-named methods, so nothing ever drove the + /// store-level recipe itself: a regression here — a lander built over the + /// read-only `workspace_filesystem` handle (which fails closed with + /// `PermissionDenied`), or a reader pointed at a different mount view than + /// the lander wrote through — would only surface downstream. Landing real + /// bytes and reading them back through BOTH returned read views proves the + /// two accessors hand out usable, mutually consistent ports. + #[tokio::test] + async fn standalone_attachment_seams_land_and_read_back_the_same_bytes() { + let dir = tempfile::tempdir().expect("tempdir"); + let services = crate::factory::build_runtime_substrate( + crate::deployment::local_filesystem_build_input( + "attachment-seam-owner", + dir.path().join("standalone"), + ), + ) + .await + .expect("standalone services build"); + + let support = services + .standalone_attachment_test_support_for_test() + .expect("a standalone composition exposes the C-ATTACH seam"); + let inbound_reader = services + .standalone_inbound_attachment_reader_for_test() + .expect("a standalone composition exposes the WebUI-facing inbound reader"); + + let thread_scope = ThreadScope { + tenant_id: TenantId::new("attachment-seam-tenant").expect("tenant id"), + agent_id: AgentId::new("attachment-seam-agent").expect("agent id"), + project_id: None, + owner_user_id: Some(UserId::new("attachment-seam-owner").expect("user id")), + mission_id: None, + }; + + let refs = support + .lander + .land( + &thread_scope, + "msg-attachment-seam", + vec![ironclaw_host_api::attachment::InboundAttachment { + id: "att-0".to_string(), + mime_type: "image/png".to_string(), + filename: Some("seam.png".to_string()), + bytes: b"attachment-seam-bytes".to_vec(), + }], + ) + .await + .expect("the seam's lander writes through a read-write workspace view"); + let storage_key = refs[0] + .storage_key + .as_deref() + .expect("a landed attachment carries a storage_key"); + + assert_eq!( + support + .read_port + .read_attachment_bytes(&thread_scope.to_resource_scope(), storage_key) + .await + .expect("the seam's model-injection read port reads the landed bytes"), + b"attachment-seam-bytes".to_vec(), + ); + assert_eq!( + inbound_reader + .read(&thread_scope, storage_key) + .await + .expect("the WebUI-facing inbound reader reads the same landed bytes"), + b"attachment-seam-bytes".to_vec(), + ); + } +} diff --git a/crates/ironclaw_reborn_composition/src/factory/trigger_creation_assembly.rs b/crates/ironclaw_reborn_composition/src/factory/trigger_creation_assembly.rs index 6e54f1bf82b..25161b68de4 100644 --- a/crates/ironclaw_reborn_composition/src/factory/trigger_creation_assembly.rs +++ b/crates/ironclaw_reborn_composition/src/factory/trigger_creation_assembly.rs @@ -225,6 +225,8 @@ pub(super) async fn pair_trigger_creator( let external_actor_ref = ExternalActorRef::new( TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, record.creator_user_id.as_str(), + // A trigger's creator actor has no channel display name. + None::, ) .map_err(|error| trigger_pairing_error(TriggerPairingFailureSource::TypedIdentity, error))?; pairing diff --git a/crates/ironclaw_reborn_composition/src/product_surface.rs b/crates/ironclaw_reborn_composition/src/product_surface.rs index ada297ea4de..deda3bdcd3e 100644 --- a/crates/ironclaw_reborn_composition/src/product_surface.rs +++ b/crates/ironclaw_reborn_composition/src/product_surface.rs @@ -5,16 +5,17 @@ use std::sync::atomic::{AtomicBool, Ordering}; use chrono::Utc; use async_trait::async_trait; +use ironclaw_attachments::ProjectScopedAttachmentLander; #[cfg(test)] use ironclaw_extensions::SharedExtensionRegistry; use ironclaw_host_api::{ids::InvocationId, resource::ResourceScope}; use ironclaw_operator::OperatorServiceLifecycle; use ironclaw_product::{ - ChannelConnectionService, OperatorStatusService, ProjectScopedAttachmentLander, - ProjectScopedAttachmentReader, ProjectScopedFilesystemReader, RebornAutomationProductService, - RebornOperatorStatusCheck, RebornOperatorStatusResponse, RebornOperatorStatusSeverity, - RebornOperatorStatusState, RebornServices as ProductRebornServices, RebornSkillContentResponse, - RebornSkillInfo, RebornSkillListResponse, RebornSkillSearchResponse, RebornSkillSourceKind, + ChannelConnectionService, OperatorStatusService, ProjectScopedAttachmentReader, + ProjectScopedFilesystemReader, RebornAutomationProductService, RebornOperatorStatusCheck, + RebornOperatorStatusResponse, RebornOperatorStatusSeverity, RebornOperatorStatusState, + RebornServices as ProductRebornServices, RebornSkillContentResponse, RebornSkillInfo, + RebornSkillListResponse, RebornSkillSearchResponse, RebornSkillSourceKind, RebornSkillTrustLevel, SkillsProductService, }; use ironclaw_product_contracts::projection::ProjectionStream; diff --git a/crates/ironclaw_reborn_composition/src/runtime.rs b/crates/ironclaw_reborn_composition/src/runtime.rs index fa16b477e22..72b9f7a199c 100644 --- a/crates/ironclaw_reborn_composition/src/runtime.rs +++ b/crates/ironclaw_reborn_composition/src/runtime.rs @@ -1028,8 +1028,8 @@ impl RebornRuntime { run_delivery_settings, } = wiring; let attachment_filesystem = self.read_write_workspace_filesystem()?; - let inbound_attachments: Arc = - Arc::new(ironclaw_product::ProjectScopedAttachmentLander::new( + let inbound_attachments: Arc = + Arc::new(ironclaw_attachments::ProjectScopedAttachmentLander::new( Arc::clone(&attachment_filesystem), )); let project_filesystem: Arc = Arc::new( @@ -1267,9 +1267,9 @@ impl RebornRuntime { #[cfg(feature = "test-support")] pub fn standalone_inbound_attachment_reader_for_test( &self, - ) -> Option> { + ) -> Option> { Some(self.standalone_workspace_attachment_reader_for_test()? - as Arc) + as Arc) } #[cfg(feature = "test-support")] @@ -1281,7 +1281,7 @@ impl RebornRuntime { let read_write_workspace_filesystem = self.read_write_workspace_filesystem()?; Some(crate::factory::AttachmentTestSupport { read_port, - lander: Arc::new(ironclaw_product::ProjectScopedAttachmentLander::new( + lander: Arc::new(ironclaw_attachments::ProjectScopedAttachmentLander::new( read_write_workspace_filesystem, )), }) diff --git a/crates/ironclaw_reborn_composition/src/runtime/tests/core.rs b/crates/ironclaw_reborn_composition/src/runtime/tests/core.rs index 7288e938060..f363bbe5756 100644 --- a/crates/ironclaw_reborn_composition/src/runtime/tests/core.rs +++ b/crates/ironclaw_reborn_composition/src/runtime/tests/core.rs @@ -5145,7 +5145,7 @@ async fn webui_workspace_filesystem_lands_attachment_with_read_write_mount() { // this test resolves the same authority a vision-capable model would. let read_port = ironclaw_product::ProjectScopedAttachmentReader::new(Arc::clone(&read_write_filesystem)); - let lander = ironclaw_product::ProjectScopedAttachmentLander::new(read_write_filesystem); + let lander = ironclaw_attachments::ProjectScopedAttachmentLander::new(read_write_filesystem); let thread_scope = ThreadScope { tenant_id: TenantId::new("runtime-attachment-mount-tenant").unwrap(), @@ -5154,7 +5154,7 @@ async fn webui_workspace_filesystem_lands_attachment_with_read_write_mount() { owner_user_id: Some(UserId::new("runtime-attachment-mount-owner").unwrap()), mission_id: None, }; - let refs = ironclaw_product::InboundAttachmentLander::land( + let refs = ironclaw_attachments::InboundAttachmentLander::land( &lander, &thread_scope, "msg-attachment-mount", diff --git a/crates/ironclaw_reborn_composition/src/trigger_poller_assembly.rs b/crates/ironclaw_reborn_composition/src/trigger_poller_assembly.rs index c1ac53edf98..198e09666a7 100644 --- a/crates/ironclaw_reborn_composition/src/trigger_poller_assembly.rs +++ b/crates/ironclaw_reborn_composition/src/trigger_poller_assembly.rs @@ -37,7 +37,7 @@ pub(crate) fn build_trigger_poller_services( ) -> Result where C: ironclaw_conversations::ConversationBindingService - + ironclaw_conversations::SessionThreadService + + ironclaw_conversations::InboundConversationService + ironclaw_conversations::ConversationActorPairingService + Clone + 'static, diff --git a/crates/ironclaw_reborn_composition/tests/production_runtime_trigger_poller.rs b/crates/ironclaw_reborn_composition/tests/production_runtime_trigger_poller.rs index 24067ad26a6..0785b29bff4 100644 --- a/crates/ironclaw_reborn_composition/tests/production_runtime_trigger_poller.rs +++ b/crates/ironclaw_reborn_composition/tests/production_runtime_trigger_poller.rs @@ -45,7 +45,8 @@ use std::time::{Duration, Instant}; use async_trait::async_trait; use chrono::Utc; -use ironclaw_conversations::{AdapterInstallationId, AdapterKind, ExternalActorRef}; +use ironclaw_conversations::{AdapterInstallationId, AdapterKind}; +use ironclaw_extension_contracts::external::ExternalActorRef; use ironclaw_host_api::{ ids::{AgentId, TenantId, UserId}, runtime_policy::{ @@ -223,8 +224,12 @@ async fn production_runtime_trigger_poller_fires_due_scheduled_trigger() { AdapterKind::new(TRIGGER_TRUSTED_ADAPTER_KIND).expect("adapter kind"), AdapterInstallationId::new(TRIGGER_TRUSTED_ADAPTER_INSTALLATION_ID) .expect("installation id"), - ExternalActorRef::new(TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, user_id.as_str()) - .expect("actor ref"), + ExternalActorRef::new( + TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, + user_id.as_str(), + None::, + ) + .expect("actor ref"), user_id.clone(), ) .await diff --git a/crates/ironclaw_reborn_composition/tests/trigger_poller_e2e.rs b/crates/ironclaw_reborn_composition/tests/trigger_poller_e2e.rs index 38556158bda..ab9f4494ade 100644 --- a/crates/ironclaw_reborn_composition/tests/trigger_poller_e2e.rs +++ b/crates/ironclaw_reborn_composition/tests/trigger_poller_e2e.rs @@ -15,7 +15,8 @@ use std::time::{Duration, Instant}; use async_trait::async_trait; use chrono::{TimeZone, Utc}; -use ironclaw_conversations::{AdapterInstallationId, AdapterKind, ExternalActorRef}; +use ironclaw_conversations::{AdapterInstallationId, AdapterKind}; +use ironclaw_extension_contracts::external::ExternalActorRef; use ironclaw_host_api::product_adapter::AdapterInstallationId as ProductAdapterInstallationId; use ironclaw_host_api::{ action::NetworkPolicy, @@ -760,8 +761,12 @@ async fn pair_trigger_creator(runtime: &RebornRuntime) { AdapterKind::new(TRIGGER_TRUSTED_ADAPTER_KIND).expect("valid adapter kind"), AdapterInstallationId::new(TRIGGER_TRUSTED_ADAPTER_INSTALLATION_ID) .expect("valid trigger installation id"), - ExternalActorRef::new(TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, USER) - .expect("valid trigger actor ref"), + ExternalActorRef::new( + TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, + USER, + None::, + ) + .expect("valid trigger actor ref"), UserId::new(USER).expect("valid user id"), ) .await @@ -1010,8 +1015,12 @@ async fn trigger_poller_drives_trusted_ingress_for_due_scheduled_trigger() { AdapterKind::new(TRIGGER_TRUSTED_ADAPTER_KIND).expect("adapter kind"), AdapterInstallationId::new(TRIGGER_TRUSTED_ADAPTER_INSTALLATION_ID) .expect("installation id"), - ExternalActorRef::new(TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, user_id.as_str()) - .expect("actor ref"), + ExternalActorRef::new( + TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, + user_id.as_str(), + None::, + ) + .expect("actor ref"), user_id.clone(), ) .await @@ -1546,8 +1555,12 @@ async fn trigger_poller_does_not_fire_trigger_with_future_next_run_at() { AdapterKind::new(TRIGGER_TRUSTED_ADAPTER_KIND).expect("adapter kind"), AdapterInstallationId::new(TRIGGER_TRUSTED_ADAPTER_INSTALLATION_ID) .expect("installation id"), - ExternalActorRef::new(TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, user_id.as_str()) - .expect("actor ref"), + ExternalActorRef::new( + TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, + user_id.as_str(), + None::, + ) + .expect("actor ref"), user_id.clone(), ) .await @@ -1770,8 +1783,12 @@ async fn trigger_poller_fires_recurring_trigger_and_leaves_it_scheduled() { AdapterKind::new(TRIGGER_TRUSTED_ADAPTER_KIND).expect("adapter kind"), AdapterInstallationId::new(TRIGGER_TRUSTED_ADAPTER_INSTALLATION_ID) .expect("installation id"), - ExternalActorRef::new(TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, user_id.as_str()) - .expect("actor ref"), + ExternalActorRef::new( + TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, + user_id.as_str(), + None::, + ) + .expect("actor ref"), user_id.clone(), ) .await diff --git a/crates/ironclaw_reborn_composition/tests/trigger_webui_timeline_e2e.rs b/crates/ironclaw_reborn_composition/tests/trigger_webui_timeline_e2e.rs index acc1ede469c..d7335a82b3b 100644 --- a/crates/ironclaw_reborn_composition/tests/trigger_webui_timeline_e2e.rs +++ b/crates/ironclaw_reborn_composition/tests/trigger_webui_timeline_e2e.rs @@ -54,6 +54,7 @@ use axum::body::Body; use axum::http::{Method, Request, StatusCode, header}; use chrono::Utc; use http_body_util::BodyExt; +use ironclaw_extension_contracts::external::ExternalActorRef; use ironclaw_host_api::ids::{AgentId, TenantId, UserId}; use ironclaw_loop_host::{ HostManagedModelError, HostManagedModelGateway, HostManagedModelRequest, @@ -363,9 +364,10 @@ async fn fire_trigger_and_get_run_thread_id( TRIGGER_TRUSTED_ADAPTER_INSTALLATION_ID, ) .expect("installation id"), - ironclaw_conversations::ExternalActorRef::new( + ExternalActorRef::new( TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, user_id.as_str(), + None::, ) .expect("actor ref"), user_id.clone(), diff --git a/crates/ironclaw_webui/AGENTS.md b/crates/ironclaw_webui/AGENTS.md index 384929a5ecb..dd492f6b206 100644 --- a/crates/ironclaw_webui/AGENTS.md +++ b/crates/ironclaw_webui/AGENTS.md @@ -68,8 +68,12 @@ one `products`-layer crate above `ironclaw_reborn_composition`. Driven by the `ironclaw_product` (wire DTOs and product command/view descriptors), `ironclaw_host_api` (`ProductSurface`, caller/error vocabulary, identity newtypes, and ingress descriptors), `ironclaw_host_ingress` (Axum route-mount -carriers), and `ironclaw_reborn_openai_compat`. Plus infra crates: `axum`, `tokio`, `tower*`, -`tracing`, `thiserror`, `async-trait`, `secrecy`, `subtle`, `jsonwebtoken`, etc. +carriers), `ironclaw_reborn_openai_compat`, and `ironclaw_attachments` (the +advertised attachment ceilings only — WS5 gave the size limits one home, so the +transport reads the same constants the landing routine enforces rather than +keeping a second copy; PROPOSAL §6.4.9). Plus infra crates: `axum`, `tokio`, +`tower*`, `tracing`, `thiserror`, `async-trait`, `secrecy`, `subtle`, +`jsonwebtoken`, etc. Any other workspace-crate edge requires an `ironclaw_architecture` boundary-test update (`tests/reborn_dependency_boundaries.rs`) plus explicit PR rationale. diff --git a/crates/ironclaw_webui/Cargo.toml b/crates/ironclaw_webui/Cargo.toml index b31e254ba34..37811adc639 100644 --- a/crates/ironclaw_webui/Cargo.toml +++ b/crates/ironclaw_webui/Cargo.toml @@ -33,6 +33,10 @@ ironclaw_product_contracts = { path = "../ironclaw_product_contracts", version = ironclaw_extension_contracts = { path = "../ironclaw_extension_contracts", version = "0.1.0" } ironclaw_host_ingress = { path = "../ironclaw_host_ingress" } ironclaw_auth = { path = "../ironclaw_auth" } +# The advertised attachment ceilings the WebUI hands the browser: one home +# for a size ceiling means the transport reads the same constant the landing +# routine enforces (PROPOSAL §6.4.9). +ironclaw_attachments = { path = "../ironclaw_attachments" } ironclaw_common = { path = "../ironclaw_common" } # Folded up from the former `ironclaw_webui_v2` crate: the v2 handlers consume # only the `ProductSurface` facade + ingress/error vocabulary. diff --git a/crates/ironclaw_webui/src/webui_v2/handlers.rs b/crates/ironclaw_webui/src/webui_v2/handlers.rs index 1ebe1b4786e..276c4a445e3 100644 --- a/crates/ironclaw_webui/src/webui_v2/handlers.rs +++ b/crates/ironclaw_webui/src/webui_v2/handlers.rs @@ -30,6 +30,7 @@ use axum::response::{IntoResponse, Response}; use base64::{Engine as _, engine::general_purpose::STANDARD}; use futures::SinkExt; use futures::stream::Stream; +use ironclaw_attachments::{AttachmentCapabilities, attachment_capabilities}; use ironclaw_product::{ ADMIN_CONFIGURATION_REPLACE_CAPABILITY, ADMIN_CONFIGURATION_VIEW, ADMIN_USER_CREATE_COMMAND, ADMIN_USER_DELETE_CAPABILITY, ADMIN_USER_DELETE_SECRET_COMMAND, @@ -53,14 +54,14 @@ use ironclaw_product::{ PROJECT_CREATE_COMMAND, PROJECT_DELETE_CAPABILITY, PROJECT_FS_LIST_VIEW, PROJECT_FS_READ_COMMAND, PROJECT_FS_STAT_VIEW, PROJECT_MEMBER_ADD_CAPABILITY, PROJECT_MEMBER_REMOVE_CAPABILITY, PROJECT_MEMBER_UPDATE_CAPABILITY, PROJECT_MEMBERS_VIEW, - PROJECT_UPDATE_CAPABILITY, PROJECT_VIEW, PROJECTS_VIEW, ProductAttachmentCapabilities, - RESOLVE_GATE_COMMAND, RETRY_RUN_COMMAND, RebornCreateThreadResponse, - RebornExtensionListResponse, RebornListThreadsResponse, RebornTimelineResponse, - SKILL_AUTO_ACTIVATE_LEARNED_SET_CAPABILITY, SKILL_AUTO_ACTIVATE_SET_CAPABILITY, - SKILL_CONTENT_VIEW, SKILL_INSTALL_CAPABILITY, SKILL_REMOVE_CAPABILITY, SKILL_SEARCH_VIEW, - SKILL_UPDATE_CAPABILITY, SKILLS_VIEW, SUBMIT_TURN_COMMAND, THREAD_DELETE_CAPABILITY, - THREADS_VIEW, TIMELINE_VIEW, TRACE_ACCOUNT_LOGIN_LINK_COMMAND, TRACE_ACCOUNT_TRACES_VIEW, - TRACE_CREDITS_VIEW, TRACE_HOLD_AUTHORIZE_COMMAND, product_attachment_capabilities, + PROJECT_UPDATE_CAPABILITY, PROJECT_VIEW, PROJECTS_VIEW, RESOLVE_GATE_COMMAND, + RETRY_RUN_COMMAND, RebornCreateThreadResponse, RebornExtensionListResponse, + RebornListThreadsResponse, RebornTimelineResponse, SKILL_AUTO_ACTIVATE_LEARNED_SET_CAPABILITY, + SKILL_AUTO_ACTIVATE_SET_CAPABILITY, SKILL_CONTENT_VIEW, SKILL_INSTALL_CAPABILITY, + SKILL_REMOVE_CAPABILITY, SKILL_SEARCH_VIEW, SKILL_UPDATE_CAPABILITY, SKILLS_VIEW, + SUBMIT_TURN_COMMAND, THREAD_DELETE_CAPABILITY, THREADS_VIEW, TIMELINE_VIEW, + TRACE_ACCOUNT_LOGIN_LINK_COMMAND, TRACE_ACCOUNT_TRACES_VIEW, TRACE_CREDITS_VIEW, + TRACE_HOLD_AUTHORIZE_COMMAND, }; use ironclaw_product_contracts::admin_users::{ RebornAdminCreateUserRequest, RebornAdminDeleteSecretProductRequest, @@ -169,7 +170,7 @@ pub struct WebUiV2SessionResponse { /// the browser advertises on its file picker. Generated from the shared /// format registry so the picker can never drift from the server's /// allowed set; the send-message decode remains authoritative. - pub attachments: ProductAttachmentCapabilities, + pub attachments: AttachmentCapabilities, } /// Deployment-wide WebUI feature gates surfaced to the browser on @@ -218,7 +219,7 @@ pub async fn get_session( regression_artifact_export: state.regression_artifact_export_enabled(), global_auto_approve, }, - attachments: product_attachment_capabilities(), + attachments: attachment_capabilities(), }) } diff --git a/crates/ironclaw_webui/tests/webui_v2_handlers_contract.rs b/crates/ironclaw_webui/tests/webui_v2_handlers_contract.rs index 87246b5edad..b8d1f810650 100644 --- a/crates/ironclaw_webui/tests/webui_v2_handlers_contract.rs +++ b/crates/ironclaw_webui/tests/webui_v2_handlers_contract.rs @@ -3780,7 +3780,7 @@ async fn get_session_returns_caller_identity_and_capabilities() { // rather than a static frontend list that can drift. The `accept` tokens // must be exactly the shared format registry's output (drift kill), and // the budgets must match what `decode_attachments` enforces. - let expected = ironclaw_product::product_attachment_capabilities(); + let expected = ironclaw_attachments::attachment_capabilities(); let accept: Vec = body["attachments"]["accept"] .as_array() .expect("attachments.accept is an array") diff --git a/docs/reborn/contracts/conversation-binding.md b/docs/reborn/contracts/conversation-binding.md index d0b9044a461..af8d29963ff 100644 --- a/docs/reborn/contracts/conversation-binding.md +++ b/docs/reborn/contracts/conversation-binding.md @@ -4,6 +4,30 @@ **Date:** 2026-05-06 **Depends on:** [`turn-persistence.md`](turn-persistence.md), [`turns-agent-loop.md`](turns-agent-loop.md), [`migration-compatibility.md`](migration-compatibility.md) +> ✎ **Amended 2026-08-02 (CHECKLIST WS5, the `conversations`/`threads` naming +> trap).** Three of this contract's answers moved, so they are corrected in +> place below rather than left to be re-derived from code: +> +> 1. **`SessionThreadService` is now `InboundConversationService`**, and its DTO +> trio renamed with it (`AcceptInboundMessageRequest` → +> `AcceptConversationMessageRequest`, `AcceptedInboundMessage*` → +> `AcceptedConversationMessage*`, `ThreadMessageRecord` → +> `ConversationMessageRecord`). The old names collided with +> `ironclaw_threads`, and a caller that picked the wrong +> `accept_inbound_message` wrote to the wrong store and still compiled. +> 2. **`ExternalActorRef` / `ExternalConversationRef` are no longer owned here.** +> Their one home is `ironclaw_extension_contracts::external`; +> `ironclaw_conversations` consumes them. It used to declare a second, +> field-divergent copy, and `ironclaw_product` bridged the two with +> hand-written translators that are now deleted. +> 3. **The route's third field is `topic_id`, not `thread_id`** — it is the +> *channel's* sub-conversation, never a canonical `ThreadId`. Only the Rust +> spelling changed: the durable record keeps `thread_id`, deliberately, so a +> rollback can still read it. See `ironclaw_conversations::stored_refs`. +> +> `crates/ironclaw_architecture/tests/reborn_conversations_threads_attachments.rs` +> pins 1 and 2 — the first rule by discovery, not enumeration. + --- ## 1. Purpose @@ -31,7 +55,7 @@ Reply-target binding is separate from external ingress identity. This contract b | Component | Owns | Does not own | | --- | --- | --- | | `ConversationBindingService` | Pairing/authenticated actor resolution, external conversation binding lookup/creation keyed by stable conversation identity, explicit conversation-to-thread links, source/reply target binding refs, reply-target validation with adapter installation and external routing data | Raw transcript content, run lifecycle, product payload parsing | -| `SessionThreadService` | Accepted inbound message refs, external event idempotency, message-to-thread/source/reply refs | Durable transcript schema details owned by #3204, turn/run locks | +| `InboundConversationService` (was `SessionThreadService`) | Accepted inbound message refs, external event idempotency, message-to-thread/source/reply refs | Durable transcript schema details owned by #3204, turn/run locks | | `InboundTurnService` | Facade composition: resolve binding, accept message, submit canonical turn; current untrusted ingress entry point and planned host-trusted ingress entry point | Adapter protocol parsing, assistant egress fanout | | `TurnCoordinator` | Turn/run admission and lifecycle after accepted message refs exist | External actor/conversation parsing, raw message storage | @@ -41,8 +65,10 @@ Reply-target binding is separate from external ingress identity. This contract b `crates/ironclaw_conversations` provides the first contract slice: -- typed external refs: `AdapterKind`, `AdapterInstallationId`, `ExternalActorRef`, `ExternalConversationRef`, `ExternalEventId`; -- `ConversationBindingService`, `SessionThreadService`, and `InboundTurnService` traits/DTOs; +- adapter-scoped typed refs it declares: `AdapterKind`, `AdapterInstallationId`, `ExternalEventId` + (the external actor/conversation pair — `ExternalActorRef`, `ExternalConversationRef` — + is *consumed* from `ironclaw_extension_contracts::external`, not declared here); +- `ConversationBindingService`, `InboundConversationService`, and `InboundTurnService` traits/DTOs; - `InMemoryConversationServices` for semantic contract tests and future adapter wiring spikes; - optional `RebornLibSqlConversationServices` and `RebornPostgresConversationServices` durable wrappers backed by normalized PostgreSQL/libSQL tables for pairings, threads/participants, bindings, reply targets, event routes, accepted messages, replay records, submit idempotency keys, and submit responses; - caller-level tests proving the facade submits only canonical refs to `TurnCoordinator`. @@ -60,7 +86,7 @@ This is not the final durable transcript store. The conversation contract stores 5. First-contact binding creation does not trust raw adapter-supplied agent/project scope hints. Scope selection must happen through a trusted thread-creation/authorization seam or by explicit linking to an existing accessible thread. 6. Pairing/authenticated actor resolution is scoped by `(tenant_id, adapter_kind, adapter_installation_id, external_actor_ref)`; a pairing on one tenant or adapter installation does not authorize another. 7. External actor/conversation refs stay structured for equality. String fingerprints, when exposed for diagnostics, must be collision-safe for delimiter-like external IDs. -8. Conversation binding identity uses stable conversation fields `(space_id, conversation_id, thread_id)`; per-message external IDs do not fork bindings or canonical threads. +8. Conversation binding identity uses stable conversation fields `(space_id, conversation_id, topic_id)`; per-message external IDs do not fork bindings or canonical threads. `topic_id` is the channel's sub-conversation (Slack thread, Telegram topic), never a canonical `ThreadId`; the durable record still spells it `thread_id` so a rollback can read it. 9. Explicit linking resolves the target thread inside the requested tenant; a caller cannot attach a different tenant's thread by reusing or guessing a thread id. 10. Explicit linking is idempotent for the same target thread and fails closed rather than silently retargeting an already-bound external conversation to a different thread, including when only per-message external IDs differ. 11. External inbound idempotency is keyed by `(tenant_id, source_binding_ref, external_event_id)` and replays the original accepted message ref and canonical actor without inserting a duplicate message only after route/ref validation passes; duplicate retries with mismatched stable routes fail closed. Implementations must reserve installation-wide external event IDs at resolution time and reject replays on a different stable conversation route before creating a second canonical binding/thread. diff --git a/docs/reborn/target-architecture/CHECKLIST.md b/docs/reborn/target-architecture/CHECKLIST.md index 9143dae1b83..d9fec72e926 100644 --- a/docs/reborn/target-architecture/CHECKLIST.md +++ b/docs/reborn/target-architecture/CHECKLIST.md @@ -55,7 +55,7 @@ Conventions: every code item lands with its tests and its guidance updates in th 7. **§6.1.5 names one eviction the row omits, and it moved**: "automation vocabulary (→ owner)". `AutomationName`/`AutomationNameError`/`MAX_AUTOMATION_NAME_BYTES` now live in `ironclaw_triggers`, which is unambiguously the owner — it stores automations, and its `MAX_TRIGGER_NAME_BYTES` was already defined as an alias of the common constant. `ironclaw_product`'s cross-crate `pub use ironclaw_common::{AutomationName, …}` (a second import path with **zero** external consumers) is deleted rather than re-sourced, following the WS1.3/WS1.4 dual-path rule. - [ ] ⚠ Move failure-summary data tables (`runner::failure_summary`) into `host_api::failure`; sever `product→runner` and `product→loop_host` (prompt constant becomes a product asset). **Two of the three clauses landed with the WS1.6/WS1.7 PR (#6982); the `loop_host` sever did not, and cannot here.** The row calls these "the two single-symbol product edges"; measured on this base, **neither is single-symbol**: - **`product → runner` — SEVERED.** It was two modules, not one symbol: `projection/turn_events.rs` imported `failure_categories::CHECKPOINT_REJECTED_CATEGORY` *and* four items from `failure_summary`. All the *data* moved to `ironclaw_host_api::failure::{categories, summary}` and `ironclaw_product`'s manifest no longer names `ironclaw_runner` under `[dependencies]` (the dev-dep stays and is now documented: product's harnesses legitimately build a full turn stack). What could **not** move, because `ironclaw_host_api` may hold no internal dependency: `checkpoint_rejection_host_explanation` (typed on `agent_loop`'s `CheckpointKind` and `loop_contracts`' `LoopSafeSummary`) and every classifier (`host_stage_unavailable_category`, `MODEL_CREDITS_EXHAUSTED_REASON_KIND`). Both runner modules are now **private**. One disposition worth review: the envelope's *reader* moved and now revalidates its cause with `SafeSummary::new` instead of `LoopSafeSummary::new`. That is behavior-preserving, not a tightening, and the claim is pinned rather than asserted — `validate_loop_safe_summary` delegates to `SafeSummary::new` with exactly one bypass, the fixed `INPUT_ENCODE_HUMAN_SUMMARY` literal, which independently satisfies the canonical rule (`loop_input_encode_sentinel_needs_no_bypass_here`). Splitting writer from reader also split the checkpoint-stage vocabulary across two crates, so the pre-existing round-trip (which covered only `BeforeModel`) gained a sibling driving **all four** `CheckpointKind`s writer→reader across the boundary. Un-masking: 10 table tests moved `runner` → `host_api` with identical names and no content edits (runner 467 → 458, host_api 248 → 260; the deltas are the 10 moved plus 1 new round-trip in runner and 2 new pins in host_api). - - **`product → loop_host` — NOT severed; the prompt clause is done.** `FAILURE_EXPLANATION_SYSTEM_PROMPT` and its `prompts/failure_explanation.md` asset are now product-owned (prompt *content* is out of charter for the loop tier, §6.1.4/§6.7.2), and the constant is gone from `loop_host`. But the edge has **three** production import sites, not one, and the other two are real behavior: `project_create_capability.rs` (six `SyntheticCapability*` symbols — the #6691 arrival that PROPOSAL §2.3 already flags for a second hop to `identity::projects` in Wave 4) and, **not previously recorded anywhere**, `scoped_fs/attachment_landing.rs`, which consumes the `LoopAttachmentReadPort`/`LoopAttachmentReadError` port pair. Severing needs those two owners moved, which is WS5's product narrowing and WS6's project re-shed — not a Wave 1 data move. The box stays open on that clause. + - **`product → loop_host` — NOT severed; the prompt clause is done.** `FAILURE_EXPLANATION_SYSTEM_PROMPT` and its `prompts/failure_explanation.md` asset are now product-owned (prompt *content* is out of charter for the loop tier, §6.1.4/§6.7.2), and the constant is gone from `loop_host`. But the edge has **three** production import sites, not one, and the other two are real behavior: `project_create_capability.rs` (six `SyntheticCapability*` symbols — the #6691 arrival that PROPOSAL §2.3 already flags for a second hop to `identity::projects` in Wave 4) and, **not previously recorded anywhere**, `scoped_fs/attachment_reader.rs` (renamed from `attachment_landing.rs` when the lander moved), which consumes the `LoopAttachmentReadPort`/`LoopAttachmentReadError` port pair. Severing needs those two owners moved, which is WS5's product narrowing and WS6's project re-shed — not a Wave 1 data move. The box stays open on that clause. - **Neither edge was ever a `LAYER_MATRIX_EXCEPTION`,** so this row cannot move the count: `products → kernel` and `products → loops` are both matrix-legal. Its value is dependency-graph narrowing and prompt-content placement, not exception reduction — worth stating because the wave milestone is written in exceptions. - Enumerating gates touched, all shrink-only: the composition pub-use snapshot lost exactly one line (`docs/plans/composition-pubuse.snapshot` 127 → 126) because the `reborn_failure_summary_for_category` re-export is **deleted** rather than re-sourced — its sole consumer, the CLI, already depends on `ironclaw_host_api` directly, so the facade hop bought nothing; and the extension-specificity `PATH_TERM_COLLISIONS` list lost its now-stale `ironclaw_common/src/platform.rs` carve-out, which the gate itself demanded (it fails on carve-outs that match nothing — the property it was built with). The product-side category-coverage scan's cross-crate `include_str!` was **repointed**, not added, so the §11.2.7 inventory is unchanged at 19. - [ ] New crates registered in CI lane selectors / coverage jobs in their creation PRs (loop_contracts, extension_contracts, product_contracts, extension_manager, sandbox — the known new-crate selector trap). *`loop_contracts` done with the WS1.2 PR (#6975): root `members`, `[package.metadata.ironclaw] layer`, a `boundary_rules()` entry, `scripts/ci/classify-test-scope.sh`'s shared arm (the one both `libsql_runtime` and `memory_mem0` missed — a diff touching only those crates still classifies `has_reborn_tests=false` today, which is the trap in its live form), and `scripts/ci/reborn-crate-test-buckets.sh`'s `agent-runtime` bucket. Verified rather than assumed: `discover-reborn-package-crates.sh` picks it up through the shipped-binary closure (`cargo tree -p ironclaw`) and needs no allowlist entry; both CI self-tests pass and the bash/python crate inventories agree at 64.* *`extension_contracts` done the same way with the WS1.3 PR (#6977): root `members`, `layer = "contracts"`, a `boundary_rules()` entry plus the §11.2.3 allowlist, `classify-test-scope.sh`'s shared arm, and `reborn-crate-test-buckets.sh`'s `extension-operator` bucket (beside `extension_host`/`extensions`, not the contracts crates' `agent-runtime` — the bucket groups by what a change to it can break). Verified rather than assumed: `discover-reborn-package-crates.sh` resolves it through the shipped-binary closure, `test-classify-test-scope.sh` and `test-reborn-crate-test-buckets.sh` both pass, and the bash/python inventories agree at 65. It also joined the `untrusted_ingress_paths_cannot_submit_host_trusted_inbound` scan roots and the extension-specificity allowlist, so neither guard lost reach over code that left `host_api`.* *`product_contracts` done the same way with the WS1.4 PR (#6980): root `members`, `layer = "contracts"`, a `boundary_rules()` entry plus the §11.2.3 allowlist (`host_api` + `extension_contracts` — the one-way street §6.1.3 grants), the shared framework/driver deny roster, `classify-test-scope.sh`'s shared arm, and `reborn-crate-test-buckets.sh`'s `product-workflow` bucket (beside `ironclaw_product` — the bucket groups by what a change to it can break). Verified rather than assumed: `discover-reborn-package-crates.sh` resolves it through the shipped-binary closure, both CI self-tests pass, the bash/python inventories agree at **66**, and all **10** exact-test selectors in `scripts/reborn-e2e-rust.sh` were executed and each matched exactly one test. It also joined the `untrusted_ingress_paths_cannot_submit_host_trusted_inbound` scan roots; the extension-specificity allowlist's four `outbound.rs` entries were **repointed** (to `extension_contracts/src/auth_prompt.rs`) rather than added, so the shrink-only baseline is untouched; and the `reborn_service_method_freeze_ratchet` path constant was repointed to the trait's new home — it failed loudly on the missing file, which is the property that gate was built with.* @@ -133,8 +133,18 @@ Conventions: every code item lands with its tests and its guidance updates in th - [ ] `operator`: implements `product_contracts` ports (ownership un-inverts; product dep dropped); route fragments move behind `host_ingress` carriers wired by composition; gains AGENTS/CLAUDE + a boundary rule (has neither today). - [ ] `openai_compat`: rename from `reborn_openai_compat` **[decision — severable]**; dep flips to contracts; stale `storage`/`libsql`/`postgres` feature guidance corrected in all five audited places; collapse the LibSql/Postgres ref-store newtype wrappers onto the generic fabric form (same for product's ledger wrappers). **Dep-flip half landed with the WS5 transport PR:** production `ironclaw_product::` usage **23 → 3 symbols across 7 → 2 files**, and the three survivors are `SUBMIT_TURN_COMMAND` / `CREATE_THREAD_COMMAND` / `CANCEL_RUN_COMMAND` — the frozen inventory again, the same structural reason webui's dep survives. Every DTO it speaks now comes from `ironclaw_product_contracts`; it also took the `+extension_contracts` edge §6.9.3 grants, for the one channel-facing enum it stamps (`ProductTriggerReason`). Rename and ref-store collapse untouched. ✎ *Row correction:* the "stale feature guidance in five audited places" item was already discharged by the WS11.3 drift-hotfix PR (see the WS11 row's "stale feature-gating in `product`/`openai_compat`/`event_store`/`webui`/`llm`"); it is double-counted here and should be struck when this row is next edited. - [ ] `product` narrows: ports/DTOs out (WS1); `adapter_registry` manifest parsing → `extension_contracts`/`extension_registry` (resolving its guidance-vs-code contradiction); the ~120-symbol `host_api::product_adapter` re-export facade dissolved; slack/telegram token heuristics → packages; `external_tool_catalog` moves in from `turns`; `reborn_services` module-charter map committed (freeze ratchet stays). -- [ ] `conversations`/`threads` naming trap fixed: rename conversations' `SessionThreadService` (→ `InboundConversationService`) + its same-named DTO trio; unify `ExternalActorRef`/`ExternalConversationRef` with `host_api` (delete product's field-by-field translators). ✎ **Amended 2026-08-01 (Wave 1 truth audit) — the counterpart crate in this row is stale, and the unification is a design decision, not a delete.** WS1.4 (#6980) moved the pair out of `host_api`, so the two declarations today are `crates/ironclaw_conversations/src/ids.rs:48,72` and **`crates/ironclaw_extension_contracts/src/external.rs:69,144`**; `ironclaw_product_contracts` only imports them (`inbound.rs:13-15`, `projection.rs:11-13`). Both are hand-written `pub struct`s. **They are not field-compatible**, so "one canonical definition, the other deleted" understates the work: conversations' actor ref is `{kind, id}` vs extension_contracts' `{kind, id, display_name}`; conversations' conversation ref is `{space_id, conversation_id, thread_id, message_id}` vs `{space_id, conversation_id, topic_id, reply_target_message_id}`; the error types differ (`InboundTurnError` vs `ProductAdapterError`). The translators are correspondingly lossy and **inconsistent with each other** — `product/src/conversation_binding.rs:818-821` silently drops `display_name`, `:826-835` maps `topic_id → thread_id` / `reply_target_message_id → message_id`, and `product/src/workflow.rs:595-606` is a *second* translator that hardcodes `None` for the fourth field, with five more inline constructions at `product/src/run_delivery/gate_routes.rs:40,48,59,68,77`. Picking the field set and the `None` semantics is the actual decision this row owns. The duplicate is already **pinned as a deliberate exemption** rather than missed — `reborn_extension_contract_location_scan.rs:120-136` lists both names in `COLLISION_EXEMPT` with the note "the same concept, declared twice … a real duplicate-surface finding, not a false positive" — so it cannot regress, but the scan will not close it either. Slot 4's finding on #6980; PROPOSAL §6.4.2 carries the matching amendment. -- [ ] `attachments` widened: ports + composition impls move in; one home for size ceilings (webui/openai import them). +- [x] `conversations`/`threads` naming trap fixed: rename conversations' `SessionThreadService` (→ `InboundConversationService`) + its same-named DTO trio; unify `ExternalActorRef`/`ExternalConversationRef` with `host_api` (delete product's field-by-field translators). ✎ **Amended 2026-08-01 (Wave 1 truth audit) — the counterpart crate in this row is stale, and the unification is a design decision, not a delete.** WS1.4 (#6980) moved the pair out of `host_api`, so the two declarations today are `crates/ironclaw_conversations/src/ids.rs:48,72` and **`crates/ironclaw_extension_contracts/src/external.rs:69,144`**; `ironclaw_product_contracts` only imports them (`inbound.rs:13-15`, `projection.rs:11-13`). Both are hand-written `pub struct`s. **They are not field-compatible**, so "one canonical definition, the other deleted" understates the work: conversations' actor ref is `{kind, id}` vs extension_contracts' `{kind, id, display_name}`; conversations' conversation ref is `{space_id, conversation_id, thread_id, message_id}` vs `{space_id, conversation_id, topic_id, reply_target_message_id}`; the error types differ (`InboundTurnError` vs `ProductAdapterError`). The translators are correspondingly lossy and **inconsistent with each other** — `product/src/conversation_binding.rs:818-821` silently drops `display_name`, `:826-835` maps `topic_id → thread_id` / `reply_target_message_id → message_id`, and `product/src/workflow.rs:595-606` is a *second* translator that hardcodes `None` for the fourth field, with five more inline constructions at `product/src/run_delivery/gate_routes.rs:40,48,59,68,77`. Picking the field set and the `None` semantics is the actual decision this row owns. The duplicate is already **pinned as a deliberate exemption** rather than missed — `reborn_extension_contract_location_scan.rs:120-136` lists both names in `COLLISION_EXEMPT` with the note "the same concept, declared twice … a real duplicate-surface finding, not a false positive" — so it cannot regress, but the scan will not close it either. Slot 4's finding on #6980; PROPOSAL §6.4.2 carries the matching amendment. **Landed with the WS5 naming-traps PR.** Both halves done; pinned by the new `reborn_conversations_threads_attachments.rs`, whose first rule is *discovered, not enumerated* — it compares every type either crate declares against the other's, so the next collision fails the day it is written. Four row corrections, each re-verified against the PR's base `ba009786d`: + 1. **The trio is a quartet — five names collided, not four.** The row names `SessionThreadService` "+ its same-named DTO trio". Comparing the two crates' `pub use` sets found **five**: `SessionThreadService`, `AcceptInboundMessageRequest`, `AcceptedInboundMessage`, `AcceptedInboundMessageReplay`, and **`ThreadMessageRecord`** (`crates/ironclaw_conversations/src/types.rs:243` against `crates/ironclaw_threads/src/contract.rs`), which no row had named. Renamed with the others (`ConversationMessageRecord`); `AcceptedInboundMessageLookup` went too, for family coherence, though it never collided. + 2. **"unify … with `host_api`" is stale by one wave — the canonical pair lives in `ironclaw_extension_contracts` now.** WS1.4 moved `external.rs` out of `host_api` (`crates/ironclaw_extension_contracts/src/external.rs:69,144`; the location scan's own module doc records the arrival at `reborn_extension_contract_location_scan.rs:107-110`). Unified onto that copy; `ironclaw_conversations` gained the dep (`substrates → contracts`, a downward edge, no layer exception). + 3. **The duplicate was field-*divergent*, and one divergence was a live semantic difference.** Conversations' copy spelled the last two fields `thread_id`/`message_id` where the canonical type says `topic_id`/`reply_target_message_id`, and carried no `display_name` — but the one that mattered is **equality**: conversations' `PartialEq`/`Hash` were *derived*, so they included the per-event message id, while the canonical type excludes it by hand ("the reply-target hint is deliberately excluded"). Two refs for the same route therefore compared **equal or unequal depending on which copy the caller happened to hold**, and `ironclaw_product`'s field-by-field translators (`conversation_actor_ref`, `conversation_conversation_ref`) were the only thing bridging them. Both translators are deleted and the callers pass the value through. + 4. **Delivered gate-route fingerprints change format — recorded, not mitigated.** The two copies had *different* `conversation_fingerprint()` algorithms (length-prefixed `"{len}:{part}|"` versus `"space:{len}:{v};…"`). Unifying picks the canonical one, so `DeliveredGateRouteRecord.delivered_conversation_fingerprints` is written and read in a new spelling. Write and read are both in `ironclaw_product` and flip together, and `DELIVERED_GATE_ROUTE_TTL` is 48 hours (`crates/ironclaw_outbound/src/delivered_gate_routes.rs:43`), so the only exposure is a gate prompt *delivered before* an upgrade and answered *after* it: its bare `approve`/`deny` reply misses the delivered-route fallback and self-heals within the TTL. Dual-writing both formats would be exactly the compatibility shim the type-placement rule forbids, so it is recorded here instead. **The durable state that is *not* self-healing was preserved**: binding and reply-target records embed the ref permanently, and a missing `Option` field deserializes to `None` **silently**, so every threaded reply route would have collapsed to its conversation root without one error. `ironclaw_conversations::stored_refs` keeps that grammar in the crate that owns the *records*, not in the contracts crate that owns the *type*. ✎ **Corrected 2026-08-02 on review (`@serrrfirat`, `stored_refs.rs:53`; CodeRabbit, `:38`).** It first shipped writing the *canonical* spelling and reading either, with a "# Rollback boundary" section declaring the one-way compatibility deliberate. That was wrong, and the argument recorded for it refuted a proposal nobody made: it rejected **dual-writing** (emitting both spellings, which `#[serde(alias)]` makes self-defeating — a reader accepting both rejects a record carrying both) when the fix is **write-legacy / read-either**, which that objection does not touch. The module now writes `{space_id, conversation_id, thread_id, message_id}` through a borrowed representation and `ExternalConversationIdentity` has a matching hand-written `Serialize`, so the rename is invisible to durable storage in both directions. The severity was also understated: the identity keys `BindingKey` and `into_state` rebuilds that map with `Vec<(K, V)>::into_iter().collect()`, so two threaded bindings in one conversation that lose their topic collapse onto one key and the earlier is **dropped** — binding loss, not a remap. Pinned through the durable store rather than a surrogate by `filesystem_conversation_services_persist_external_refs_in_the_durable_grammar`, which walks every key of the persisted document; the surrogate unit tests stayed green under the mutation that broke it. + - **What the trap was costing, concretely:** exactly one file had to hold both vocabularies, composition's trusted-trigger submit, and it carried **four `as Canonical…` aliases** to disambiguate (`crates/ironclaw_reborn_composition/src/automation/trigger_poller_trusted_submit.rs:18-19,488-489`), plus a fifth in `extension_host` (`ExternalActorRef as ConversationActorRef`, `channel_pairing/tests.rs`). All five are deleted; nothing aliases these names anywhere in the tree now. + - **Enumerating gate shrunk, not repointed:** `reborn_extension_contract_location_scan.rs`'s `COLLISION_EXEMPT` goes **4 → 2**. Its two entries for this pair described the duplicate as "the same concept, declared twice … unifying them changes the conversations record grammar — a domain call"; the domain call is taken, so the carve-out is deleted rather than moved. What is *added* to that scan's prose is the fact it still cannot see: `AdapterInstallationId`/`ExternalEventId` are `bounded_string_id!` expansions no `pub struct` walk resolves. +- [~] `attachments` widened: ports + composition impls move in; one home for size ceilings (webui/openai import them). **Landed with the WS5 naming-traps PR, minus one item that cannot land — see 3.** `ironclaw_attachments` now declares `InboundAttachmentLander`, `InboundAttachmentReader`, `AttachmentCleanupReport`, the default `ProjectScopedAttachmentLander`, and the advertised-ceiling pair `AttachmentCapabilities` / `attachment_capabilities()`; none of them is declared in `ironclaw_product` any more. Pinned by `reborn_conversations_threads_attachments.rs`. + 1. **"composition impls" is wrong about where they were.** §6.4.9 says the impls "move in from … their composition impls"; they were never in composition. Both adapters lived in `ironclaw_product::scoped_fs::attachment_landing` (`crates/ironclaw_product/src/scoped_fs/attachment_landing.rs`), and composition only *constructs* them. The construction sites are unchanged — they now name `ironclaw_attachments::ProjectScopedAttachmentLander`. + 2. **The size-ceiling clause was already cashed out by name in another gate.** `reborn_transport_product_boundary.rs` listed `ProductAttachmentCapabilities` / `product_attachment_capabilities` as WebUI residue with the reason "CHECKLIST WS5 `attachments widened … one home for size ceilings` owns the move". They moved, so those two rows are **deleted** and the shrink-only baseline goes **102 → 100** — WebUI reads the advertised ceilings from the crate that enforces them, which is the whole point of the clause. + 3. **`ProjectScopedAttachmentReader` cannot move, for two independent reasons.** It implements `ironclaw_loop_host::LoopAttachmentReadPort` as well as `InboundAttachmentReader`; `ironclaw_loop_host` is a `loops`-layer crate and `ironclaw_attachments` is `substrates`, so the dep is an illegal upward edge — and even granting it, once the struct moved that impl would have neither side in `ironclaw_product` and the orphan rule forbids leaving it behind. It stays in product, and the gate pins it *there* (plus asserts `ironclaw_attachments` never acquires a `loop_host` dep) so "finishing the row" cannot quietly mean dragging the loop tier into a substrate. **The row should be re-worded to exclude the read adapter**, or the read port re-homed first. ✎ **Amended 2026-08-01 (review of #7005):** that closing sentence — quoted verbatim as it stood: *"**The row should be re-worded to exclude the read adapter**, or the read port re-homed first."* — stated an obligation with no tracked home, so nothing would surface it when someone next tried to close this `[~]`. **It is now tracked in #7010**, which carries the two options (re-word §6.4.9 to exclude the read adapter, or re-home `LoopAttachmentReadPort` first) and records that adding a `loop_host` dep to `ironclaw_attachments` is not one of them — the arch gate asserts that dependency never appears, and that assertion is the guard, not an obstacle to route around. This box stays `[~]` until #7010 closes. The two blockers above are unchanged and still correct. + 4. **[decision] — `ironclaw_attachments` now names `ironclaw_product_contracts`.** The two ports error with `ProductSurfaceError` because their failing caller is a product surface and the WebUI bytes endpoint maps the code straight onto its 404/403. That is layer-legal (`contracts` is the lowest layer, and this is the *neutral* product-tier contract crate, not `ironclaw_product`), but it does put product-tier error vocabulary in a substrate. The alternative — narrow the ports onto an attachments-owned error and map at the product boundary — **moves an HTTP status mapping**, which is behavior, not placement, and belongs to its own slice the way WS2.2 gave `ProductSurfaceFailure` one. Recorded, not taken. ## WS6 — Composition, app, and domain evictions diff --git a/docs/reborn/target-architecture/PROPOSAL.md b/docs/reborn/target-architecture/PROPOSAL.md index 6269d663123..c95f45b9de6 100644 --- a/docs/reborn/target-architecture/PROPOSAL.md +++ b/docs/reborn/target-architecture/PROPOSAL.md @@ -63,7 +63,7 @@ Bottom→top (longest-path levels, re-derived 2026-07-30): `host_api`(fan-in 53, Load-bearing facts this proposal is built on (each verified; ✎ = re-measured 2026-07-30): - **`extension_host` sits *above* product today** (normal dep on `ironclaw_product`, ✎ 113 references), because the ports it implements (delivery resolver/reply-context/admission/pairing/preference-codec) are *defined in product*, and its ingress calls product's sealed host-auth mint. - ✎ **`product` sits above `runner`/`loop_host`** — at authoring this was one pure-data import each (failure-summary formatters at `projection/turn_events.rs:34`; a prompt constant, now `:994`). #6691 added a **third, non-data** edge: the project-create capability it evicted from composition (`product/src/project_create_capability.rs:8`) imports `ironclaw_loop_host` for real behavior. The §6.9.1 shed ("`runner`/`loop_host` single-symbol deps → `host_api::failure` + a product-owned prompt asset") now has one more site to resolve, and it is behavior rather than a constant — noted, not re-designed. - - ✎ **Amended 2026-08-01 (Wave 1 truth audit, measuring PR #6982 / WS1.7): "single-symbol" was wrong on both edges, and only one of the two is gone.** **`product → runner` is SEVERED** — it was two modules, not one symbol (`projection/turn_events.rs` imported `failure_categories::CHECKPOINT_REJECTED_CATEGORY` *and* four items from `failure_summary`); the data moved to `ironclaw_host_api::failure::{categories, summary}`, `ironclaw_product`'s manifest no longer names `ironclaw_runner` under `[dependencies]` (the dev-dep stays, and is now documented — product's harnesses legitimately build a full turn stack), and both runner modules are private. What could not follow, because `host_api` may hold no internal dependency: `checkpoint_rejection_host_explanation` (typed on `agent_loop`'s `CheckpointKind` and `loop_contracts`' `LoopSafeSummary`) and every classifier. **`product → loop_host` SURVIVES on two behavioral sites**, and the prompt clause is the only part done (`FAILURE_EXPLANATION_SYSTEM_PROMPT` and its `prompts/failure_explanation.md` asset are product-owned now; prompt *content* is out of charter for the loop tier, §6.1.4/§6.7.2). The two survivors are `project_create_capability.rs` (the #6691 arrival named above, six `SyntheticCapability*` symbols, owed a second hop to `identity::projects` per §6.4.11) and — **not previously recorded in any document** — `scoped_fs/attachment_landing.rs`, which consumes the `LoopAttachmentReadPort`/`LoopAttachmentReadError` port pair. Severing needs both owners moved: WS5's product narrowing and WS6's project re-shed. **Neither edge was ever a `LAYER_MATRIX_EXCEPTION`** (`products → kernel` and `products → loops` are matrix-legal), so this work cannot move the exception count — its value is dependency-graph narrowing and prompt-content placement, worth stating because the wave milestone is written in exceptions. + - ✎ **Amended 2026-08-01 (Wave 1 truth audit, measuring PR #6982 / WS1.7): "single-symbol" was wrong on both edges, and only one of the two is gone.** **`product → runner` is SEVERED** — it was two modules, not one symbol (`projection/turn_events.rs` imported `failure_categories::CHECKPOINT_REJECTED_CATEGORY` *and* four items from `failure_summary`); the data moved to `ironclaw_host_api::failure::{categories, summary}`, `ironclaw_product`'s manifest no longer names `ironclaw_runner` under `[dependencies]` (the dev-dep stays, and is now documented — product's harnesses legitimately build a full turn stack), and both runner modules are private. What could not follow, because `host_api` may hold no internal dependency: `checkpoint_rejection_host_explanation` (typed on `agent_loop`'s `CheckpointKind` and `loop_contracts`' `LoopSafeSummary`) and every classifier. **`product → loop_host` SURVIVES on two behavioral sites**, and the prompt clause is the only part done (`FAILURE_EXPLANATION_SYSTEM_PROMPT` and its `prompts/failure_explanation.md` asset are product-owned now; prompt *content* is out of charter for the loop tier, §6.1.4/§6.7.2). The two survivors are `project_create_capability.rs` (the #6691 arrival named above, six `SyntheticCapability*` symbols, owed a second hop to `identity::projects` per §6.4.11) and — **not previously recorded in any document** — `scoped_fs/attachment_reader.rs` (renamed from `attachment_landing.rs` when the lander moved), which consumes the `LoopAttachmentReadPort`/`LoopAttachmentReadError` port pair. Severing needs both owners moved: WS5's product narrowing and WS6's project re-shed. **Neither edge was ever a `LAYER_MATRIX_EXCEPTION`** (`products → kernel` and `products → loops` are matrix-legal), so this work cannot move the exception count — its value is dependency-graph narrowing and prompt-content placement, worth stating because the wave milestone is written in exceptions. - ✎ **`turns` now sits *above* `processes` and `approvals`** (normal dep, added by #6696 when the turn store became a journal projection). Both ends are `kernel` in the target, so the edge is legal by the matrix and adds no exception — but it does mean `turns` is no longer a bottom-tier "domain store" in the topology, which is what §6.5.8 predicted would happen. - ✎ **`libsql_runtime` is a true leaf** (fan-out 0, no workspace dependencies; consumed by `filesystem`, `triggers`, and `composition`) — the driver-admission runtime #6863 introduced. - **`slack_extension` depends only on `host_api`** (the clean model); `telegram_extension` needs `product`+`turns` for one 228-line module because `PreferenceTargetCodec` lives in product; `telegram_v2_adapter` has exactly one consumer (its sibling). @@ -537,13 +537,13 @@ Purpose: transport-neutral stream manager — authorization, RAII admission, bou Compact entries (all: layer `substrates`; forbidden = anything ≥ kernel unless stated; boundary role = domain ownership unless stated): -- **6.4.1 `ironclaw_threads`** — retain. Canonical transcript service (`SessionThreadService`, filesystem/in-memory impls). Never: turn lifecycle authority, delivery policy. Deps: `common`, `filesystem`, `host_api`, `safety`. Why a crate: contract w/ 5 consumers + 2 impls. **Naming fix obligation:** the `conversations` collision (§6.4.2). -- **6.4.2 `ironclaw_conversations`** — retain, rename internals. External↔canonical binding, actor pairing, accepted-message/turn-submission idempotency, trusted-trigger submitter. Never: payload parsing, transcript content. **Contract fixes:** rename its `SessionThreadService` (→ `InboundConversationService`) and its same-named DTO trio — the audited worst naming trap; unify `ExternalActorRef`/`ExternalConversationRef` with the `host_api` pair (one canonical definition, the other deleted; product's field-by-field translators removed). ✎ **Amended 2026-08-01 (Wave 1 truth audit): "the `host_api` pair" is stale, and the duplication is lossier than "one canonical definition, the other deleted" implies.** WS1.4 (#6980) moved the counterpart out of `host_api`, so the two declarations today are `ironclaw_conversations/src/ids.rs:48,72` and **`ironclaw_extension_contracts/src/external.rs:69,144`** (`ironclaw_product_contracts` only *imports* them, at `inbound.rs:13-15` and `projection.rs:11-13`). Both sites are hand-written `pub struct`s, not `bounded_ref!` output. **They are not field-compatible:** conversations' `ExternalActorRef` is `{kind, id}` while extension_contracts' adds `display_name`; conversations' `ExternalConversationRef` is `{space_id, conversation_id, thread_id, message_id}` against extension_contracts' `{space_id, conversation_id, topic_id, reply_target_message_id}`, and the error types differ (`InboundTurnError` vs `ProductAdapterError`). So the "translators" are lossy and inconsistent, not mechanical: `product/src/conversation_binding.rs:818-821` **silently drops `display_name`**, `:826-835` maps `topic_id → thread_id` and `reply_target_message_id → message_id`, and `product/src/workflow.rs:595-606` is a **second, differently-behaving** translator that hardcodes `None` for the fourth field, with five more ad-hoc constructions inline at `product/src/run_delivery/gate_routes.rs:40,48,59,68,77`. The unification therefore has to pick a field set and a `None`-semantics before it can delete anything. The finding is already pinned in-tree as a deliberate exemption — `reborn_extension_contract_location_scan.rs:120-136` carries both names in `COLLISION_EXEMPT` and calls them "the same concept, declared twice … a real duplicate-surface finding, not a false positive" — so the scan will not regress it, but it will not close it either. Tracked on CHECKLIST WS5's `conversations`/`threads` row. Move the safety-scanning of trusted trigger prompts behind the triggers/kernel seam it guards (module move). Deps: `filesystem`, `host_api`, `safety`, `triggers` + turn vocabulary via `host_api`. Why a crate: distinct identity/idempotency authority consumed by extension_host/product/composition. +- **6.4.1 `ironclaw_threads`** — retain. Canonical transcript service (`SessionThreadService`, filesystem/in-memory impls). Never: turn lifecycle authority, delivery policy. Deps: `common`, `filesystem`, `host_api`, `safety`. Why a crate: contract w/ 5 consumers + 2 impls. **Naming fix obligation:** the `conversations` collision (§6.4.2). ✎ **Discharged 2026-08-01 (WS5 naming traps):** the collision was **five** names, not four — `ThreadMessageRecord` collided too — and every one was renamed on the *conversations* side, so this crate's vocabulary is unchanged. `reborn_conversations_threads_attachments.rs` now compares the two crates' declared names by discovery, so a new collision fails at introduction. +- **6.4.2 `ironclaw_conversations`** — retain, rename internals. External↔canonical binding, actor pairing, accepted-message/turn-submission idempotency, trusted-trigger submitter. Never: payload parsing, transcript content. **Contract fixes:** rename its `SessionThreadService` (→ `InboundConversationService`) and its same-named DTO trio — the audited worst naming trap; unify `ExternalActorRef`/`ExternalConversationRef` with the `host_api` pair (one canonical definition, the other deleted; product's field-by-field translators removed). ✎ **Amended 2026-08-01 (Wave 1 truth audit): "the `host_api` pair" is stale, and the duplication is lossier than "one canonical definition, the other deleted" implies.** WS1.4 (#6980) moved the counterpart out of `host_api`, so the two declarations today are `ironclaw_conversations/src/ids.rs:48,72` and **`ironclaw_extension_contracts/src/external.rs:69,144`** (`ironclaw_product_contracts` only *imports* them, at `inbound.rs:13-15` and `projection.rs:11-13`). Both sites are hand-written `pub struct`s, not `bounded_ref!` output. **They are not field-compatible:** conversations' `ExternalActorRef` is `{kind, id}` while extension_contracts' adds `display_name`; conversations' `ExternalConversationRef` is `{space_id, conversation_id, thread_id, message_id}` against extension_contracts' `{space_id, conversation_id, topic_id, reply_target_message_id}`, and the error types differ (`InboundTurnError` vs `ProductAdapterError`). So the "translators" are lossy and inconsistent, not mechanical: `product/src/conversation_binding.rs:818-821` **silently drops `display_name`**, `:826-835` maps `topic_id → thread_id` and `reply_target_message_id → message_id`, and `product/src/workflow.rs:595-606` is a **second, differently-behaving** translator that hardcodes `None` for the fourth field, with five more ad-hoc constructions inline at `product/src/run_delivery/gate_routes.rs:40,48,59,68,77`. The unification therefore has to pick a field set and a `None`-semantics before it can delete anything. The finding is already pinned in-tree as a deliberate exemption — `reborn_extension_contract_location_scan.rs:120-136` carries both names in `COLLISION_EXEMPT` and calls them "the same concept, declared twice … a real duplicate-surface finding, not a false positive" — so the scan will not regress it, but it will not close it either. Tracked on CHECKLIST WS5's `conversations`/`threads` row. ✎ **Done 2026-08-01 (WS5 naming traps), with three corrections.** (a) The trio is a **quartet**: `ThreadMessageRecord` collided as well (`src/types.rs:243`), so the renames are `InboundConversationService`, `AcceptConversationMessageRequest`, `AcceptedConversationMessage`, `AcceptedConversationMessageReplay`, `AcceptedConversationMessageLookup`, `ConversationMessageRecord`. (b) **"with the `host_api` pair" is stale**: WS1.4 moved `external.rs` to `ironclaw_extension_contracts` (`src/external.rs:69,144`), which is where the unification landed; this crate's target deps gain `extension_contracts` (`substrates → contracts`, no layer exception). (c) The duplicate was **field-divergent**, and the divergence that mattered was *equality* — conversations' derived `PartialEq`/`Hash` included the per-event message id that the canonical type deliberately excludes, so the same route compared equal or unequal depending on which copy the caller held. The durable record grammar (`thread_id`/`message_id`, no `display_name`) is preserved by `ironclaw_conversations::stored_refs`, in the crate that owns the records rather than the crate that owns the type — ✎ **and as of the 2026-08-02 review correction that means preserved on the *write* side too**, not only accepted on read; see the CHECKLIST WS5 row for why the original one-way shape was a mistake; the delivered-gate-route *fingerprint* format does change, and self-heals inside its 48-hour TTL (CHECKLIST WS5 records the exposure). Move the safety-scanning of trusted trigger prompts behind the triggers/kernel seam it guards (module move). Deps: `filesystem`, `host_api`, `safety`, `triggers` + turn vocabulary via `host_api`. Why a crate: distinct identity/idempotency authority consumed by extension_host/product/composition. - **6.4.3 `ironclaw_triggers`** — retain. Scheduled-trigger records, cron/timezone validation, deterministic fire identity, `TriggerPollerWorker::tick_once`, trusted-submit minting (`TriggerTrustedInboundBinding`). Never: poller *lifecycle* (composition), a parallel agent loop. **Persistence idiom flag:** its hand-written libSQL/Postgres repos (3,347 lines) are the family's documented exception; converge on the filesystem fabric or write the ADR (§12.6). Boundary role: **security-relevant** (host-trusted ingress minting — the sealed trusted-submitter path stays here, pinned by the existing trusted-trigger tests). Why a crate: distinct domain + trusted-mint authority. - **6.4.4 `ironclaw_memory` / 6.4.5 `ironclaw_memory_native` / 6.4.6 `ironclaw_memory_mem0`** — retain all three. The audited *justified* provider seam: neutral contract (allowlist `{host_api, prompt_envelope}`), two production providers, shared conformance suite, composition-only mem0 naming (dedicated test). Fixes: delete `memory_native`'s dead `EmbeddingProvider` port (restoring vector search is §12.10), delete its six path-preservation re-export shims, drop its unused `prompt_envelope` dep (the write-safety engine consumes envelope vocabulary via `ironclaw_memory`, which owns that dep). Why crates: criteria 1+4 (2 production impls) + 6 (mem0's HTTP cone off-by-default). **Amendment (2026-07-29, owner decision):** the two *providers* are extension packages, not domains crates — `ironclaw_memory_native` → `extensions/packages/memory-native/` and `ironclaw_memory_mem0` → `extensions/packages/mem0/`, at the same level, each declaring a `[memory]` manifest surface and linked only by the binary; the native package ships installed by default so memory stays always-on. `ironclaw_memory` (contract + conformance suite) stays here, and the kernel and composition keep consuming the contract only. The seam, the conformance suite, and every fix above are unchanged — what changes is where provider code ships. Mapping rows 21–22 updated; `families/domains.md` and `families/extensions.md` carry the amended layout. - **6.4.7 `ironclaw_skills`** — retain, narrow. Skill parsing/validation/selection/management + pure learning (prompts as crate assets; `SkillInferencePort` stays the intended inversion port). Deletes: `registry`/`catalog`/`v2`/`gating` (~4k lines, zero consumers) or explicit revival with a consumer named; fully rewrite the stale v1 `lib.rs` doc. Layer: **substrates** (today `loops`; its consumers are kernel/hosting-tier — reassignment makes current reality legal). Gains: `SkillActivationObserver` + observed-event type (from `first_party_extension_ports`) so product's projection needs only this domain. - **6.4.8 `ironclaw_auth`** — retain, narrow. Product-auth flow/account/interaction/cleanup contracts + durable services + the recipe-driven `AuthEngine` (vendor differences are recipe data — the invariant stays). Deletes: `loopback_oauth` (dead, §2.6) + its `urlencoding` dep; gate `fakes.rs` behind `test-support` (today ships ungated in release builds — a real hygiene bug). Drops the `turns` dep via the gate-prompt port in `host_api` (the named follow-up exception). Internal two-engine split (engine vs product_auth) becomes two chartered top-level modules. Why a crate: credential-custody domain, 8 consumers, boundary rule already comprehensive. -- **6.4.9 `ironclaw_attachments`** — retain, widen. The single landing routine **plus its ports** (`InboundAttachmentLander`/`InboundAttachmentReader` move in from product; their composition impls move in as the default impl over `ScopedFilesystem`) — ending the 3-crate spread; one home for the size-ceiling constants (webui/openai_compat import them). Why a crate: single-authority landing path with 3 consumers. +- **6.4.9 `ironclaw_attachments`** — retain, widen. The single landing routine **plus its ports** (`InboundAttachmentLander`/`InboundAttachmentReader` move in from product; their composition impls move in as the default impl over `ScopedFilesystem`) — ending the 3-crate spread; one home for the size-ceiling constants (webui/openai_compat import them). Why a crate: single-authority landing path with 3 consumers. ✎ **Widened 2026-08-01 (WS5), with one carve-out and one correction.** Both ports, `AttachmentCleanupReport`, the default `ProjectScopedAttachmentLander`, and the advertised-ceiling pair (`AttachmentCapabilities` / `attachment_capabilities()`, moved out of `ironclaw_product`) now live here; WebUI reads its advertised ceilings from the crate that enforces them. **Correction:** "their composition impls" — the impls were never in composition, they were in `ironclaw_product::scoped_fs::attachment_landing`; composition only constructs them. **Carve-out:** `ProjectScopedAttachmentReader` **cannot** move — it also implements `ironclaw_loop_host::LoopAttachmentReadPort`, a `loops`-layer trait a `substrates` crate may not name, and moving the struct would orphan that impl. It stays in product and the gate pins it there. The ports keep erroring with `ProductSurfaceError`, so this crate names `ironclaw_product_contracts` (layer-legal, but a **[decision]** the CHECKLIST row records: narrowing the error would move the WebUI 404/403 status mapping, which is behavior). - **6.4.10 `ironclaw_extractors`** — retain. Pure MIME→text with bomb caps. Fixes: typed error across the boundary (today `Result`), remove the caller-less `extract_text` from the public surface, add a guidance file. Why a crate: pure leaf with heavy deps (pdf/zip) kept out of consumers. - **6.4.11 `ironclaw_projects`** — **merge into `ironclaw_identity`** as its `projects` module (decided 2026-07-30; the consolidation audit overturns the W2 retain: 842 lines, one wiring consumer, a dependency set byte-identical to identity's pinned allowlist, and no rule anywhere that distinguishes it). Migration: identity's pinned allowlist is unchanged — `{host_api, filesystem}` already covers the merged crate verbatim; the authorization-gating adapter (665 lines in composition today) moves in as the module's service half; the product port stays in `product_contracts`; "access resolution is never cached" becomes a module test. - **6.4.12 `ironclaw_identity`** — retain, rename (from `ironclaw_reborn_identity`). External identity → stable `UserId` + minimal user directory; keeps its allowlist `{host_api, filesystem}`. Absorbs: `host_api::user_identity` store ports (persistence ports don't belong in the vocabulary crate) — resolving the audited "two parallel identity-binding stores" ambiguity in its CONTRACT (unresolved half → §12.10). Trim: the three zero-caller resolver methods per open issue #5618 or wire them. Why a crate: bottom-of-stack identity authority with machine-enforced never-reach-upstream rule. @@ -608,7 +608,7 @@ Compact entries (all: layer `substrates`; forbidden = anything ≥ kernel unless **Family role:** the supported product experience above the kernel. **Belongs:** ProductSurface implementation, admission/workflow, bindings, delivery semantics, projections-to-views, transports, operator control plane. **Does not belong:** authority decisions, lane mechanics, vendor protocol (packages), assembly. **Layer range:** `products`. **Family AGENTS.md:** the frozen-surface rule (`host_product_surface_method_set_is_frozen`), the "transports consume contracts, not owners" rule, and the trusted-ingress prohibition (product code never mints trusted inbound). -- **6.9.1 `ironclaw_assistant`** (renamed from `ironclaw_product`, amended 2026-07-29 — the personal assistant *is* the product, and the rename keeps the family's central crate from collapsing into `product/product`) — retain, narrow, rename. The `ProductSurface` implementation (`RebornServices`) + workflow/admission (`ChannelInboundProductSurface` impl, command grammar — including the parsing/rendering helpers evicted from `host_api`), conversation/target binding, idempotency ledger, delivery **semantics** (`DeliveryCoordinator` + run-delivery drivers), product projections/views assembly, approval/auth interaction services. Sheds: port/DTO definitions → `product_contracts` (its ~120-symbol re-export facade over `host_api::product_adapter` dissolves; consumers import contracts); `adapter_registry`'s manifest-section parsing → `extension_contracts`/`extension_registry` (resolving its guidance-vs-code contradiction on the `ironclaw_extensions` dep); operator vocabulary → contracts; `runner`/`loop_host` single-symbol deps (→ `host_api::failure` + a product-owned prompt asset) ✎ **partly landed 2026-07-31 (#6982/WS1.7) and the phrase "single-symbol" is retired — see §2.3: `runner` is severed (it was two modules of failure-summary/category data), `loop_host` is not, and the two behavioral sites that hold it up are `project_create_capability.rs` (→ `identity::projects`, §6.4.11/WS6) and `scoped_fs/attachment_landing.rs`'s `LoopAttachmentReadPort`/`LoopAttachmentReadError` pair (→ this shed, WS5). Plan the WS5 narrowing around two owners, not one constant.**; slack/telegram token heuristics (`xoxb-` prefixes etc.) → the packages that own them. Internal: the `reborn_services` god-object keeps its freeze ratchet and gains a module-charter map (the audited ≥11 sub-owners). Why a crate: the product authority (bindings, admission, delivery semantics, product gates) — the dossier's product contract, now with the mass that belongs to it and nothing else. +- **6.9.1 `ironclaw_assistant`** (renamed from `ironclaw_product`, amended 2026-07-29 — the personal assistant *is* the product, and the rename keeps the family's central crate from collapsing into `product/product`) — retain, narrow, rename. The `ProductSurface` implementation (`RebornServices`) + workflow/admission (`ChannelInboundProductSurface` impl, command grammar — including the parsing/rendering helpers evicted from `host_api`), conversation/target binding, idempotency ledger, delivery **semantics** (`DeliveryCoordinator` + run-delivery drivers), product projections/views assembly, approval/auth interaction services. Sheds: port/DTO definitions → `product_contracts` (its ~120-symbol re-export facade over `host_api::product_adapter` dissolves; consumers import contracts); `adapter_registry`'s manifest-section parsing → `extension_contracts`/`extension_registry` (resolving its guidance-vs-code contradiction on the `ironclaw_extensions` dep); operator vocabulary → contracts; `runner`/`loop_host` single-symbol deps (→ `host_api::failure` + a product-owned prompt asset) ✎ **partly landed 2026-07-31 (#6982/WS1.7) and the phrase "single-symbol" is retired — see §2.3: `runner` is severed (it was two modules of failure-summary/category data), `loop_host` is not, and the two behavioral sites that hold it up are `project_create_capability.rs` (→ `identity::projects`, §6.4.11/WS6) and `scoped_fs/attachment_reader.rs`'s `LoopAttachmentReadPort`/`LoopAttachmentReadError` pair (→ this shed, WS5). Plan the WS5 narrowing around two owners, not one constant.**; slack/telegram token heuristics (`xoxb-` prefixes etc.) → the packages that own them. Internal: the `reborn_services` god-object keeps its freeze ratchet and gains a module-charter map (the audited ≥11 sub-owners). Why a crate: the product authority (bindings, admission, delivery semantics, product gates) — the dossier's product contract, now with the mass that belongs to it and nothing else. - **6.9.2 `ironclaw_operator`** — retain, narrow. Deployment-operator control plane implementations: LLM provider admin (registry write-side, keys, active model, NEAR-AI/Codex logins), operator log ring, OS service lifecycle — now implementing `product_contracts` ports (its product dep flips to a contracts dep; ownership un-inverts). Its Axum route fragments move behind `host_ingress` carriers wired by composition (it stops owning routers). Gets: guidance files + a boundary rule (today it has neither). Why a crate: distinct operator authority with a vendor-integration cone (this *is* the LLM-vendor admin layer), consumed only by app-family crates. - **6.9.3 `ironclaw_openai_compat`** — retain, rename (drop `reborn_`). The OpenAI-shaped ingress adapter: route descriptors, wire DTOs, sanitized error envelope, ref/idempotency store, workflows over `BoundProductSurface`. Change: depends on `product_contracts` (+`extension_contracts` where channel DTOs are shared) instead of `ironclaw_product`; stale feature-gating guidance corrected. ✎ *2026-08-01: both edges landed with the WS5 transport inversion — 23 → 3 product symbols, the three survivors being the same frozen command constants that keep webui's edge alive. The `extension_contracts` edge carries exactly one type, `ProductTriggerReason`.* Open modeling question (adapter-as-extension?) stays §12.10 — not forced. Why a crate: a protocol surface with its own wire-stability contract and the tightest honored guardrails in the audit. - **6.9.4 `ironclaw_webui`** — retain. Route surface + descriptor table (✎ **92** routes, contract-locked — re-counted 2026-07-31 at `2e6522580`: `rg -c 'pub const WEBUI_V2_ROUTE_' crates/ironclaw_webui/src/webui_v2/descriptors.rs` → 92, was 91; #6930 added `WEBUI_V2_ROUTE_REGISTER_HOSTED_MCP_EXTENSION` with its frozen-table row and updated the crate's own `CLAUDE.md` route table in the same PR — the contract lock working as intended), gateway middleware order, serve loop, host authentication (Env/Session/OIDC/composite + `/auth/*` login), product-auth HTTP routes, embedded SPA. Changes: `ironclaw_product` dep → `product_contracts` (the one non-DTO import, the bearer-evidence mint, moves to `host_api`'s sealed evidence home, deleting the `host-auth-mint` feature plumbing) ✎ **Corrected 2026-08-01 (WS5 transport inversion): "the one non-DTO import" is wrong by 91.** Beyond the mint (which left with WS1.5), webui names **91 concrete command/view/capability constants** — the frozen inventory §6.1.3 keeps in product — plus 11 wire DTOs whose fields name `ironclaw_attachments`/`threads`/`auth`/`common`/`loop_contracts`. Measured at `f4819bb50`: 228 product symbols before the inversion, 102 after. **The dep therefore does not flip in this row**; whether the inventory follows the descriptor types into contracts is the open §6.1.3-vs-§6.9.4 decision recorded on the CHECKLIST row; gains the pairing routes from `extension_host`; its second OAuth stack (host login) stays by charter (documented, distinct concern) — §12.10 records the consolidation question. Why a crate: the transport/presentation artifact (axum + SPA cone) with a comprehensive boundary rule. diff --git a/tests/integration/changed-coverage-exemptions.toml b/tests/integration/changed-coverage-exemptions.toml index 556aa88e839..9a0ed8aefe3 100644 --- a/tests/integration/changed-coverage-exemptions.toml +++ b/tests/integration/changed-coverage-exemptions.toml @@ -799,3 +799,105 @@ owner = "@nearai/reborn" reason = "A `tracing::debug!` message literal. `tracing` bakes the message into the callsite's `static` Metadata, so LLVM attributes this line's region to a static initializer and never counts it as executed -- while the surrounding lines 213/214/219/220 each score 1 hit in the same tracefile and the event body is asserted by a DEBUG-subscriber test. Attribution artifact, not a dead path; see the block comment above for the per-line evidence." issue = "https://github.com/nearai/ironclaw/issues/6963" review_after = "2026-10-31" + +# --------------------------------------------------------------------------- +# WS5 naming traps: seven `struct`/field declaration lines in +# `ironclaw_conversations/src/types.rs`. +# +# These are the DTO-quartet renames -- `AcceptInboundMessageRequest` -> +# `AcceptConversationMessageRequest`, `AcceptedInboundMessage` -> +# `AcceptedConversationMessage`, its `Replay`/`Lookup` siblings, and +# `ThreadMessageRecord` -> `ConversationMessageRecord`. Every changed line is +# either a `pub struct {` header or a `pub : ,` +# declaration. Nothing executable changed; the bodies that construct and read +# these types are unchanged and are covered elsewhere. +# +# They are exempted because the file has NO LLVM region on any of them, which +# is checkable in the merged tracefile for this PR (run 30713493949, +# `reborn-integration-merged.lcov`): +# +# the file carries 42 DA records spanning lines 17..312, and the suite does +# execute it -- `pub(crate) fn new(` at line 298 scores 18 hits. But there is +# a complete DA gap between line 60 and line 298, and all seven lines below +# (199, 209, 211, 215, 233, 246, 247) fall inside it. Not one of them has a +# DA record at all, so there is no counter for a test to move. +# +# This is why the gate reports the file as "contributed no instrumented lines" +# rather than listing them as uncovered: every changed candidate line in the +# instrumented span resolves to zero measurable lines. A type declaration +# emits no region; only the code that constructs the type does, and that code +# is not what this PR changed. +# +# This was NOT concluded by inference -- the obvious objection is that these +# types all `#[derive(Serialize, Deserialize)]`, so a test that round-trips +# them might instantiate the derived impls and light the declaration lines up. +# That was tried, and it does not: +# `filesystem_conversation_services_round_trip_persisted_state_on_reopen` +# (crates/ironclaw_conversations/tests/conversation_state_store_contract.rs) +# now serializes AND deserializes ConversationMessageRecord, +# AcceptedConversationMessage, AcceptedConversationMessageReplay and +# AcceptedConversationMessageLookup, asserting their wire field names survive +# the rename. Re-measured with `cargo llvm-cov` over that test binary, the file +# still reports exactly 42 DA records over exactly lines 17..312 -- byte for +# byte the same set as before the test existed, with none of the seven lines +# gaining a record. Derive-generated code is `#[automatically_derived]` and +# carries no coverage regions at the declaration site, so no test can move +# these counters. The round-trip test is kept anyway: it is the regression +# that pins the persisted encoding across the rename, which is the risk the +# WS5 CHECKLIST row actually cares about. +# +# Line 318 (`InboundTurnResponse::accepted_message`) has no record either, but +# it sits outside the 17..312 instrumented span, so the gate never makes it a +# candidate and it is deliberately not listed below. + +[[exemption]] +path = "crates/ironclaw_conversations/src/types.rs" +lines = [199, 209, 211, 215, 233, 246, 247] +owner = "@nearai/reborn" +reason = "Type positions only: `pub struct` headers and `pub : ` declarations for the renamed accepted-message DTO quartet (199 AcceptedConversationMessageLookup, 209/211 AcceptedConversationMessageReplay + its accepted_message field, 215 AcceptConversationMessageRequest, 233 AcceptedConversationMessage, 246/247 ConversationMessageRecord + its accepted field). A declaration carries no LLVM coverage region, so none of these lines can ever be covered: the merged tracefile has 42 DA records for this file over lines 17..312 -- including line 298 at 18 hits, proving the file executes -- yet a complete DA gap from 60 to 298 leaves all seven with no record at all. Proven, not inferred: a serde round-trip contract test for all five types was added and the file STILL reports the identical 42 records over the identical 17..312 span, because derive-generated code is `#[automatically_derived]` and emits no region at the declaration site. The only change on each line is the type rename; see the block comment above." +issue = "https://github.com/nearai/ironclaw/issues/6963" +review_after = "2026-10-31" + +# --------------------------------------------------------------------------- +# Gate-route recording: three of the five +# `if let Ok(..) = ExternalConversationRef::new(..)` sites in +# `record_gate_route_if_needed` have an `Err` arm no test can reach. +# +# Two of the five CAN fail and are NOT exempted. Lines 40 and 53 also pass +# `vendor_message_ref` as the topic -- a raw, unvalidated `String` field on +# `DeliveredChannelMessage` (crates/ironclaw_product/src/run_delivery.rs:143) +# copied verbatim out of whatever the channel adapter returned in +# `PartDeliveryOutcome::Sent` +# (crates/ironclaw_product/src/delivery_coordinator.rs:659-662). A vendor can +# therefore hand back a ref that is not a legal external id, and +# `observer_records_gate_route_without_a_vendor_ref_that_cannot_key_a_route` +# (crates/ironclaw_product/tests/run_delivery_contract.rs) now drives both of +# those arms down `Err` with a control-character ref, asserting the exact +# surviving fingerprint set. +# +# The three below pass ONLY accessor reads off an existing, already-validated +# `ExternalConversationRef` plus literal `None`s, and that type makes an +# invalid value unconstructible: +# +# * all four fields are private +# (crates/ironclaw_extension_contracts/src/external.rs:144-149); +# * the only value-producing paths are `new` (152-175), which runs +# `validate_external_id` over every non-`None` argument, `Deserialize` +# (234-248), which delegates to `new`, `Clone`, and `without_reply_target` +# (201-208), which copies fields `new` already validated; +# * `validate_external_id` (12-32) is a pure predicate on the string -- +# non-empty, <= 512 bytes, no NUL/control characters. +# +# So `space_id()`, `conversation_id()` and `topic_id()` can only return a +# string that already satisfies that predicate, and re-running the identical +# predicate inside a fresh `new` cannot fail. No program state, hostile +# channel package, or malformed inbound payload reaches these three `Err` +# arms; a test asserting one would have to construct a value the type forbids. + +[[exemption]] +path = "crates/ironclaw_product/src/run_delivery/gate_routes.rs" +branch_lines = [45, 58, 74] +owner = "@nearai/reborn" +reason = "The `Err` arm of three `ExternalConversationRef::new` calls whose every argument is an accessor read off an already-validated `ExternalConversationRef` plus literal `None`s: line 45 = the delivered message's (space_id, conversation_id), line 58 = its conversation_id alone, line 74 = the source conversation's space_id/conversation_id/topic_id. The type's four fields are private and its only value-producing paths -- `new`, `Deserialize` (delegates to `new`), `Clone`, `without_reply_target` -- all run or inherit `validate_external_id`, a pure predicate on the string (non-empty, <= 512 bytes, no NUL/control chars), so re-validating a value that already passed it cannot fail. The two sibling sites that also take the unvalidated `vendor_message_ref` (lines 40 and 53) ARE reachable and are covered by `observer_records_gate_route_without_a_vendor_ref_that_cannot_key_a_route`; no exemption is claimed for those. See the block comment above." +issue = "https://github.com/nearai/ironclaw/issues/6963" +review_after = "2026-10-31" diff --git a/tests/integration/support/harness/mod.rs b/tests/integration/support/harness/mod.rs index 871c0a3961d..bcc7276c6f7 100644 --- a/tests/integration/support/harness/mod.rs +++ b/tests/integration/support/harness/mod.rs @@ -309,7 +309,7 @@ pub(crate) struct HostRuntimeCapabilityHarness { /// trait than `attachment_test_support`'s `LoopAttachmentReadPort`, though /// the same concrete reader implements both. `Some` only for /// `new_with_options`-built harnesses. - inbound_attachment_reader: Option>, + inbound_attachment_reader: Option>, /// Backing handles for the synthetic outbound target list/set test seam. /// `Some` only for `outbound_target_tools()`; route-current stays on the /// normal first-party lane and uses the composed product service/store. @@ -1628,7 +1628,7 @@ impl HostRuntimeCapabilityHarness { /// `RebornServices::with_inbound_attachment_reader`. pub(crate) fn inbound_attachment_reader_for_test( &self, - ) -> Option> { + ) -> Option> { self.inbound_attachment_reader.clone() } diff --git a/tests/integration/support/triggered_submit.rs b/tests/integration/support/triggered_submit.rs index 10804923420..b9769fa757a 100644 --- a/tests/integration/support/triggered_submit.rs +++ b/tests/integration/support/triggered_submit.rs @@ -20,9 +20,10 @@ use std::time::Duration; use chrono::{TimeZone, Utc}; use ironclaw_conversations::{ - AdapterInstallationId, AdapterKind, ExternalActorRef, InMemoryConversationServices, + AdapterInstallationId, AdapterKind, InMemoryConversationServices, trusted_trigger_fire_submitter, }; +use ironclaw_extension_contracts::external::ExternalActorRef; use ironclaw_host_api::turn::{TurnGateRef, TurnRunId, TurnScope, TurnStatus}; use ironclaw_llm::testing::provider_chain_over; use ironclaw_llm::{LlmProvider, SessionConfig, create_session_manager}; @@ -95,6 +96,7 @@ impl RebornIntegrationHarness { ExternalActorRef::new( TRIGGER_TRUSTED_EXTERNAL_ACTOR_NAMESPACE, self.binding.actor_user_id.as_str(), + None::, )?, self.binding.actor_user_id.clone(), )