diff --git a/crates/buzz-sdk/src/broker/actions.rs b/crates/buzz-sdk/src/broker/actions.rs index ee51a3a860..d440f7a40b 100644 --- a/crates/buzz-sdk/src/broker/actions.rs +++ b/crates/buzz-sdk/src/broker/actions.rs @@ -1,22 +1,19 @@ //! Broker actions — the closed set of operations an agent may ask a host to perform. //! -//! An action is the unit of policy. `message.post` and `agents.delete` are -//! separate names so a host can permit one without permitting the other, and so -//! a later policy layer has something per-operation to attach to. There is -//! deliberately no `sign` action and no `publish` action: an interface that can -//! sign arbitrary bytes is a signing oracle, and a host holding it could not -//! reason about what it authorized. +//! An action is the unit of policy; see the [module docs](super) for why this is +//! an operation enum rather than a signing primitive. //! //! Two consequences of that choice are visible in the types below. Every args -//! type is `deny_unknown_fields`, so a credential or environment map smuggled -//! into a payload fails to deserialize instead of reaching an executor. Every -//! outcome type can structurally hold only public identifiers — no field of any -//! outcome can transport key material. +//! and outcome type is `deny_unknown_fields`, so a credential or environment map +//! smuggled into a payload fails to deserialize instead of reaching an executor. +//! And the wire key set of every type is pinned by test, so a secret-bearing +//! field cannot be added without a test failing. use serde::{Deserialize, Serialize}; use crate::SdkError; use buzz_core::engram::validate_slug; +use nostr::Event; /// Maximum characters in a display name or agent name. pub const MAX_NAME_CHARS: usize = 120; @@ -42,6 +39,9 @@ pub const MAX_MENTIONS: usize = 50; /// Maximum events a single read may return. pub const MAX_PAGE_LIMIT: u32 = 500; +/// Maximum accepted length of a read cursor, in bytes. +pub const MAX_CURSOR_LEN: usize = 256; + /// Inbound author gate modes a requester may ask for. /// /// `allowlist` is deliberately absent: it needs a pubkey list this request @@ -53,8 +53,7 @@ pub const RESPOND_TO_MODES: [&str; 2] = ["owner-only", "anyone"]; /// /// #6467 asks for identity to be separable from signing. This type is that /// separation made structural: it holds 64 hex characters of public key and has -/// no counterpart in this module for the corresponding secret. Nothing here can -/// name, hold, or ask for one. +/// no counterpart in this module for the corresponding secret. #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] #[serde(try_from = "String", into = "String")] pub struct PubkeyHex(String); @@ -164,11 +163,9 @@ impl Action { /// Whether a host may refuse this action without harming the agent. /// - /// #6467 requires non-essential signed housekeeping to be skippable, so - /// that an agent can still run where it is unavailable. For a best-effort - /// action, [`super::BrokerErrorCode::Unsupported`] is a normal answer and a - /// caller must carry on. For every other action, `Unsupported` means the - /// agent cannot do its job on this host and should say so out loud. + /// #6467 requires non-essential signed housekeeping to be skippable, so an + /// agent can still run where it is unavailable. See + /// [`super::BrokerErrorCode::Unsupported`] for how a caller reacts. #[must_use] pub fn is_best_effort(self) -> bool { matches!(self, Self::ReactionAdd) @@ -194,16 +191,10 @@ impl Action { /// Reads are actions like any other, because #6467 requires a host with no /// relay route of its own to be able to serve them. /// -/// One action covers three scopes, because they differ only by filter and a -/// separate name per scope would be a separate policy decision for the same -/// underlying permission — *may this agent see this channel*. `rootEventId` -/// narrows to one thread; `mentionsOnly` narrows to messages that mention the -/// requester, which is the wake path. -/// -/// `since` is an inclusive lower bound in Unix seconds, matching the relay's -/// filter granularity. A caller must de-duplicate by event id: second-precision -/// cursors can straddle events, so consecutive pages may overlap. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +/// One action covers channel, thread, and mention-feed scope, because they +/// differ only by filter and a name per scope would split one permission — +/// *may this agent see this channel* — across three policy decisions. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct ChannelReadArgs { /// Channel to read. @@ -211,68 +202,45 @@ pub struct ChannelReadArgs { /// Narrow to one thread by its root event. #[serde(default, skip_serializing_if = "Option::is_none")] pub root_event_id: Option, - /// Narrow to messages mentioning the requester. + /// Narrow to messages mentioning the requester — the wake path. /// - /// The requester is never named. The host answers for the identity it - /// authenticated; a body that could ask "what mentions *someone else*" - /// would be a body that picks its own subject. + /// The requester is never named; see [`super::BrokerRequest`] on why no body + /// names its own subject. #[serde(default, skip_serializing_if = "is_false")] pub mentions_only: bool, - /// Inclusive lower bound, Unix seconds. Absent = host's default window. + /// Opaque position to resume from, as returned in [`MessagePage::next_cursor`]. + /// + /// Absent on a first read, which starts at the host's default window. A + /// cursor is **opaque**: callers must round-trip it verbatim and must not + /// parse, compare, or synthesize one. That is deliberate — a timestamp + /// cursor cannot page safely when more events than `limit` share one + /// second, and it would commit every future host to one relay ordering + /// strategy. The host defines ordering and cursor stability, including + /// whether a cursor stays valid across restarts. #[serde(default, skip_serializing_if = "Option::is_none")] - pub since: Option, + pub cursor: Option, /// Maximum events to return, capped at [`MAX_PAGE_LIMIT`]. #[serde(default, skip_serializing_if = "Option::is_none")] pub limit: Option, } impl ChannelReadArgs { - /// Read a whole channel. + /// Read a whole channel from the host's default window. #[must_use] pub fn channel(channel_id: impl Into) -> Self { Self { channel_id: channel_id.into(), - root_event_id: None, - mentions_only: false, - since: None, - limit: None, + ..Self::default() } } - /// Narrow this read to one thread. - #[must_use] - pub fn in_thread(mut self, root_event_id: impl Into) -> Self { - self.root_event_id = Some(root_event_id.into()); - self - } - - /// Narrow this read to messages mentioning the requester. - #[must_use] - pub fn mentions_only(mut self) -> Self { - self.mentions_only = true; - self - } - - /// Start this read at `since` (inclusive, Unix seconds). - #[must_use] - pub fn since(mut self, since: u64) -> Self { - self.since = Some(since); - self - } - - /// Cap this read at `limit` events. - #[must_use] - pub fn limit(mut self, limit: u32) -> Self { - self.limit = Some(limit); - self - } - /// Validate and normalize. /// /// # Errors /// /// Returns [`SdkError::InvalidInput`] for a malformed channel UUID, a - /// malformed root event id, or an out-of-range limit. + /// malformed root event id, an over-long or non-printable cursor, or an + /// out-of-range limit. pub fn validated(&self) -> Result { Ok(Self { channel_id: channel(&self.channel_id)?, @@ -282,7 +250,7 @@ impl ChannelReadArgs { .map(|id| event_id(id, "rootEventId")) .transpose()?, mentions_only: self.mentions_only, - since: self.since, + cursor: self.cursor.as_deref().map(cursor).transpose()?, limit: limit(self.limit)?, }) } @@ -434,10 +402,9 @@ impl ProfileSetArgs { /// Arguments for `storage.address`. /// -/// Encrypted-memory records are addressed by a tag derived from a conversation -/// key, so deriving one needs the secret this contract exists to avoid holding. -/// #6467 lists that derivation among the operations to route through the -/// interface, which is why it is an action rather than a local computation. +/// Deriving a record's address needs the secret this contract exists to avoid +/// holding, which is why #6467 lists it among the operations to route through +/// the interface rather than compute locally. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct StorageAddressArgs { @@ -461,10 +428,8 @@ impl StorageAddressArgs { // ── Agent arguments ───────────────────────────────────────────────────────── -/// Which agent an update or delete targets. -/// -/// Exactly one selector, because a request naming the target twice is ambiguous -/// and the host would have to guess which wins. +/// Which agent an update or delete targets — exactly one selector, so a host +/// never has to guess which of two names wins. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub enum AgentTarget { @@ -490,10 +455,9 @@ impl AgentTarget { /// Arguments for `agents.create`. /// -/// There is no owner field. The owner of a created agent is whoever the host -/// authenticated for this request — an agent creating an agent owns it, and the -/// chain of ownership ends at a human. A request that could name its own -/// authority would let any caller mint agents under someone else. +/// There is no owner field: the owner is whoever the host authenticated, so an +/// agent creating an agent owns it. See the [module docs](super) on ownership +/// recursion, and [`super::BrokerRequest`] on why no body names its own subject. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct AgentsCreateArgs { @@ -707,48 +671,88 @@ impl ActionArgs { // ── Outcomes ──────────────────────────────────────────────────────────────── -/// One message returned by a read. +/// One message returned by a read: the signed Nostr event, verbatim. /// -/// A projection, not a Nostr event: no signature, no tag array. A caller that -/// needs to verify signatures needs the relay, which is exactly what a -/// broker-only deployment does not have. Trust here is trust in the host. +/// The event is authoritative. It is carried whole — signature and tags +/// included — rather than reduced to a projection, because Schnorr verification +/// is local and needs no relay: see [`Self::verify`]. A keyless agent therefore +/// still gets independently verifiable authorship and content, and only has to +/// trust the host for *completeness* (that no message was withheld) and for +/// authorization. Reducing to a projection would have widened that trust to +/// cover authorship too, and dropped the tags later consumers need. +/// +/// The wire form is the event's own JSON, so this adds no envelope of its own. +/// Ancestry and mentions are exposed as derived accessors rather than sibling +/// fields, so there is nothing that can disagree with the signed bytes. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] -#[serde(rename_all = "camelCase", deny_unknown_fields)] -pub struct BrokerMessage { - /// Event id (hex). - pub event_id: String, - /// Author's pubkey. - pub author_pubkey: PubkeyHex, - /// Event kind. - pub kind: u32, - /// Creation time, Unix seconds. - pub created_at: u64, - /// Message body. - pub content: String, - /// Thread root, when this message is threaded. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub root_event_id: Option, - /// Immediate parent, when this message is a reply. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub parent_event_id: Option, - /// Pubkeys this message mentions. - #[serde(default, skip_serializing_if = "Vec::is_empty")] - pub mentions: Vec, +#[serde(transparent)] +pub struct BrokerMessage(pub Event); + +impl BrokerMessage { + /// The signed event. + #[must_use] + pub fn event(&self) -> &Event { + &self.0 + } + + /// Verify the event's id and Schnorr signature. + /// + /// This is the caller's provenance check and it is entirely local. A host + /// that fabricated or altered a message fails here regardless of what it + /// claims. It is deliberately *not* called by + /// [`super::BrokerResponse::validate_for`]: whether to pay for verification, + /// and what to do when it fails, is the caller's policy. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] when the id does not match the + /// content or the signature does not match the author. + pub fn verify(&self) -> Result<(), SdkError> { + self.0.verify().map_err(|e| { + SdkError::InvalidInput(format!("broker returned an unverifiable event: {e}")) + }) + } + + /// The author's pubkey, in this contract's identity type. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] if the event's author is not + /// expressible as 64 hex characters. + pub fn author(&self) -> Result { + PubkeyHex::parse(self.0.pubkey.to_hex()) + } + + /// NIP-10 `root`/`reply` ancestry, parsed from the signed tags. + #[must_use] + pub fn thread(&self) -> buzz_core::nip10::ThreadMarkers { + buzz_core::nip10::parse_thread_markers(&self.0.tags) + } + + /// Pubkeys this message mentions, from the signed `p` tags. + #[must_use] + pub fn mentions(&self) -> Vec { + self.0 + .tags + .public_keys() + .map(nostr::PublicKey::to_hex) + .collect() + } } /// Outcome of any read action. -/// -/// `nextCursor` is the value to pass as `since` on the following call. It is -/// absent when the host has nothing further, which is how a caller learns to -/// stop rather than by comparing lengths against a limit it may not have set. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct MessagePage { - /// Messages in ascending `createdAt` order. + /// Messages in the host's declared order. pub messages: Vec, - /// Cursor for the next page, if more may exist. + /// Opaque cursor to pass as [`ChannelReadArgs::cursor`] on the next call. + /// + /// Absent when the host has nothing further, which is how a caller learns + /// to stop rather than by comparing lengths against a limit it may not have + /// set. #[serde(default, skip_serializing_if = "Option::is_none")] - pub next_cursor: Option, + pub next_cursor: Option, } /// Outcome of an action that published one event. @@ -780,9 +784,10 @@ pub struct StorageAddress { /// Outcome of a successful `agents.create`. /// -/// Carries the new agent's **public** key only. There is no field for the -/// minted secret: it stays on the host, and this type could not transport it -/// even if a handler tried. +/// Carries the new agent's **public** key only: pubkey, name, channel, and +/// nothing else. There is no field for the minted secret — it stays on the +/// host — and `deny_unknown_fields` plus a test pinning this exact key set is +/// what enforces that rather than a comment. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase", deny_unknown_fields)] pub struct AgentsCreateOutcome { @@ -865,6 +870,50 @@ impl ActionOutcome { Self::AgentsDelete(_) => Action::AgentsDelete, } } + + /// Validate the identifiers and cursors this outcome asserts. + /// + /// A well-typed outcome can still carry a malformed id or an unusable + /// cursor, and a caller that trusted either would fail later and further + /// away. Signature verification is deliberately *not* here — that is + /// [`BrokerMessage::verify`], and whether to pay for it is the caller's + /// choice. + /// + /// # Errors + /// + /// Returns [`SdkError::InvalidInput`] for a malformed event id, `d` tag, + /// channel UUID, or cursor, an empty name, or an over-long page. + pub fn validate(&self) -> Result<(), SdkError> { + match self { + Self::ChannelRead(page) => { + if page.messages.len() > MAX_PAGE_LIMIT as usize { + return Err(SdkError::InvalidInput(format!( + "page holds {} messages, over the {MAX_PAGE_LIMIT} cap", + page.messages.len() + ))); + } + page.next_cursor.as_deref().map(cursor).transpose()?; + } + Self::MessagePost(published) + | Self::MessageReply(published) + | Self::ReactionAdd(published) + | Self::ProfileSet(published) => { + event_id(&published.event_id, "eventId")?; + } + Self::StorageAddress(address) => { + event_id(&address.d_tag, "dTag")?; + } + Self::AgentsCreate(outcome) => { + channel(&outcome.channel_id)?; + required(&outcome.display_name, "display name", MAX_NAME_CHARS)?; + } + Self::AgentsUpdate(AgentsUpdateOutcome { display_name, .. }) + | Self::AgentsDelete(AgentsDeleteOutcome { display_name, .. }) => { + required(display_name, "display name", MAX_NAME_CHARS)?; + } + } + Ok(()) + } } fn is_false(value: &bool) -> bool { @@ -941,6 +990,30 @@ fn limit(value: Option) -> Result, SdkError> { } } +/// Validate an opaque read cursor: printable ASCII, bounded, never parsed. +/// +/// The bound exists so a host cannot be made to store an unbounded token; the +/// character set keeps it safe to log. Nothing here interprets the value. +fn cursor(value: &str) -> Result { + if value.is_empty() { + return Err(SdkError::InvalidInput( + "cursor must not be empty (omit it to start from the host's default window)".into(), + )); + } + if value.len() > MAX_CURSOR_LEN { + return Err(SdkError::InvalidInput(format!( + "cursor exceeds {MAX_CURSOR_LEN} bytes (got {})", + value.len() + ))); + } + if !value.bytes().all(|b| (0x21..=0x7e).contains(&b)) { + return Err(SdkError::InvalidInput( + "cursor must be printable ASCII without spaces".into(), + )); + } + Ok(value.to_owned()) +} + fn respond_to(value: Option<&String>) -> Result, SdkError> { let value = optional(value, "respond-to", MAX_SCALAR_CHARS)?; if let Some(mode) = value.as_deref() { diff --git a/crates/buzz-sdk/src/broker/client.rs b/crates/buzz-sdk/src/broker/client.rs index f161e4d518..3793e43672 100644 --- a/crates/buzz-sdk/src/broker/client.rs +++ b/crates/buzz-sdk/src/broker/client.rs @@ -7,21 +7,21 @@ //! Authorization: Bearer //! Content-Type: application/json //! -//! +//! //! ``` //! //! The response body is a [`BrokerResponse`] as JSON. Every terminal -//! disposition — including [`super::BrokerResult::Failed`] — is a well-formed -//! envelope, so a host that has an answer returns `200` and puts the verdict in -//! `status`. HTTP status codes carry no information the envelope does not, and a -//! client must read the envelope rather than the status line. +//! disposition the *host* reached — including [`super::BrokerResult::Failed`], +//! and including a rejected credential — is a well-formed envelope returned with +//! `200`, because the verdict lives in `status` and a second copy in the status +//! line could only ever disagree with it. //! -//! A non-2xx response is only meaningful when there is *no* envelope: the -//! credential was rejected before dispatch, the path does not exist, or an -//! intermediary answered instead of the host. Those surface as -//! [`BrokerTransportError`], which is not a `Failed` result — it means no answer -//! arrived, so the caller learned nothing about side effects and may retry with -//! identical bytes per the [`BrokerRequest`] retry contract. +//! A client must nonetheless **attempt to parse an envelope regardless of HTTP +//! status**. A host or intermediary may map dispositions onto conventional +//! statuses for observability or for middleware that cannot read bodies; if a +//! valid envelope is present, it is the answer and the status line is +//! decoration. Only when no envelope can be parsed does the status matter, and +//! then only as detail for an operator: see [`BrokerTransportError`]. //! //! # The credential //! @@ -29,6 +29,12 @@ //! received at startup and can only replay; it is not a key, not a signature, //! and not derived from anything the agent holds. //! +//! A rejected credential is a host verdict, not a transport failure: it arrives +//! as `Failed` with [`super::BrokerErrorCode::Unauthenticated`], which carries +//! the promise every `Failed` carries — the action did not run. Modelling it as +//! a transport error would have thrown that knowledge away and told the caller +//! to reconcile something that provably never happened. +//! //! Binding a credential to a specific (agent, conversation) pair — so that a //! leaked token cannot act as a different agent or outside the conversation it //! was issued for — **is the host's concern**. This contract documents the @@ -43,7 +49,7 @@ use std::future::Future; use std::pin::Pin; -use super::{BrokerRequest, BrokerResponse}; +use super::{BrokerResponse, PreparedRequest}; /// Path of the single broker endpoint. pub const BROKER_ACTION_PATH: &str = "/v1/action"; @@ -51,20 +57,38 @@ pub const BROKER_ACTION_PATH: &str = "/v1/action"; /// Header carrying the opaque session credential, as `Bearer `. pub const BROKER_CREDENTIAL_HEADER: &str = "authorization"; -/// A transport-level failure: no [`BrokerResponse`] was obtained. +/// No usable [`BrokerResponse`] was obtained, so the request's fate is unknown. /// -/// Distinct from [`super::BrokerResult::Failed`], which is a host's considered -/// verdict. These variants all mean the same thing to a caller — the request's -/// fate is unknown — and differ only in what to tell an operator. +/// Every variant means the same thing to a caller: nothing can be concluded +/// about side effects, and the only safe next step is to retry the identical +/// bytes (which the host will deduplicate) or to reconcile by reading state. +/// The variants differ only in what to tell an operator. +/// +/// Host verdicts never appear here — not even a rejected credential, which is +/// [`super::BrokerErrorCode::Unauthenticated`] inside a `Failed` envelope. This +/// type is strictly for the absence of an answer. #[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] pub enum BrokerTransportError { /// The host could not be reached, or the connection failed mid-request. #[error("broker unreachable: {0}")] Unreachable(String), - /// The credential was rejected before the host produced an envelope. - #[error("broker rejected the session credential")] - CredentialRejected, - /// A response arrived but was not a well-formed [`BrokerResponse`]. + /// An HTTP response arrived carrying no parseable envelope. + /// + /// Typically an intermediary answering instead of the host: a proxy `401`, + /// a `404` for a missing route, a `502`. The status is recorded for + /// operators and carries no contractual meaning — an intermediary's `401` + /// does not prove the host never ran the action. + #[error("no broker envelope in HTTP {status} response: {detail}")] + NoEnvelope { + /// The HTTP status observed. + status: u16, + /// Operator-facing detail about what arrived instead. + detail: String, + }, + /// An envelope arrived but did not validate against the request that was + /// sent — wrong `requestId`, wrong action, or a malformed outcome. + /// + /// See [`BrokerResponse::validate_for`]. #[error("malformed broker response: {0}")] MalformedResponse(String), } @@ -83,14 +107,22 @@ pub type BrokerFuture<'a> = /// [`super::Action`] inside the request, so adding an action never changes this /// trait and an implementation never needs updating to carry one. /// +/// The argument is a [`PreparedRequest`] rather than a typed request so that an +/// implementation cannot reserialize between attempts; see the +/// [`super::BrokerRequest`] retry contract. +/// /// Implementations must be usable as `dyn BrokerClient`. pub trait BrokerClient: Send + Sync { - /// Execute `request` and return the host's terminal response. + /// Send `request`'s frozen bytes and return the host's terminal response. + /// + /// Implementations should call [`BrokerResponse::validate_for`] before + /// returning `Ok`, so a caller never has to consider a response that does + /// not answer its request. /// /// # Errors /// - /// Returns [`BrokerTransportError`] when no response could be obtained. - /// A host that answered — even to refuse — returns `Ok` with the verdict in - /// [`BrokerResponse::result`]. - fn execute<'a>(&'a self, request: &'a BrokerRequest) -> BrokerFuture<'a>; + /// Returns [`BrokerTransportError`] when no usable response could be + /// obtained. A host that answered — even to refuse — returns `Ok` with the + /// verdict in [`BrokerResponse::result`]. + fn execute<'a>(&'a self, request: &'a PreparedRequest) -> BrokerFuture<'a>; } diff --git a/crates/buzz-sdk/src/broker/mod.rs b/crates/buzz-sdk/src/broker/mod.rs index 48645de288..925b26fc51 100644 --- a/crates/buzz-sdk/src/broker/mod.rs +++ b/crates/buzz-sdk/src/broker/mod.rs @@ -38,14 +38,28 @@ //! //! | #6467 requirement | Where | //! |---|---| -//! | Identity separable from signing; pubkey-only identity | [`PubkeyHex`] is the only identity type. No secret-key type exists in this module, and no args or outcome field can hold one. | -//! | Secret-dependent operations funnelled through the interface | Event signing is subsumed by the write actions (`message.post`, `message.reply`, `reaction.add`, `profile.set`, `agents.*`) — the agent states intent, the host signs. Encrypted-storage address derivation is [`Action::StorageAddress`]. NIP-42 relay auth, NIP-44 encryption, and NIP-98 request auth are **not** actions: they are mechanisms internal to whoever holds the key, and this contract never asks for them by name. | -//! | Relay auth is a control-flow choice | Nothing here mentions relay authentication. An agent using this contract does not authenticate to a relay, so there is no local auth step to skip. | +//! | Identity separable from signing; pubkey-only identity | [`PubkeyHex`] is the only identity type; no secret-key type exists in this module. | +//! | Secret-dependent operations funnelled through the interface | Event signing is subsumed by the write actions (`message.post`, `message.reply`, `reaction.add`, `profile.set`, `agents.*`) — the agent states intent, the host signs. Encrypted-storage address derivation is [`Action::StorageAddress`]. NIP-42 relay auth, NIP-44 encryption, and NIP-98 request auth are **not** actions: they are mechanisms internal to whoever holds the key. | +//! | Relay auth is a control-flow choice | Nothing here mentions relay authentication, so there is no local auth step to skip. | //! | Every relay-touching op, reads included, routable | [`Action::ChannelRead`] covers channel, thread, and mention-feed reads. No action assumes the caller can reach a relay. | //! | Non-essential housekeeping skippable | [`Action::is_best_effort`] marks such actions; a host answers [`BrokerErrorCode::Unsupported`] and the agent carries on. | //! | No secret leaks to children via env | No args type carries an environment map, and `deny_unknown_fields` means one cannot be smuggled in. Process spawning is a host concern this contract cannot express. | //! | Lives in the shared client layer | `buzz-sdk`, which the CLI and the harness already depend on. | //! +//! # The no-secret rule +//! +//! No secret key material crosses this boundary in either direction. What +//! enforces it is the wire schema, not a comment: every args and outcome type is +//! `deny_unknown_fields`, and tests pin each type's exact key set, so a +//! secret-bearing field cannot be added without a test failing. +//! [`AgentsCreateOutcome`] is the case that matters — it returns public identity +//! only, never the key it just minted. +//! +//! Two limits are worth stating. A `String` field can physically hold secret +//! text, so keeping secrets out of message content and error messages is host +//! policy this contract cannot enforce. And nothing stops a host from *holding* +//! keys — that is the point; it stops one from handing them over. +//! //! # Ownership recursion //! //! [`Action::AgentsCreate`] has no owner field. The owner of a created agent is @@ -115,23 +129,30 @@ pub const MAX_REQUEST_ID_LEN: usize = 128; /// /// # What this envelope deliberately omits /// -/// There is no requester, owner, or relay field. Those are derived by the host -/// from the authenticated session credential. A body that could name its own -/// subject would let any caller act as anyone. +/// There is no requester, owner, scope, or relay field. Those are derived by the +/// host from the authenticated session credential. **A body that could name its +/// own subject would let any caller act as anyone** — that one rule is why +/// `channel.read` cannot ask about another identity's mentions, why +/// `profile.set` has no subject, and why `agents.create` has no owner. /// /// # Retry contract /// -/// Retrying means **resending the identical serialized request** with the same -/// `requestId`. The host hashes what it receives and compares that digest -/// against the digest recorded under the same idempotency key: +/// Retrying means resending the identical serialized request with the same +/// `requestId`, which is why a client never sends this type directly: call +/// [`Self::prepare`] to freeze it into a [`PreparedRequest`] and hand *that* to +/// [`BrokerClient::execute`]. The host hashes the bytes it receives and compares +/// that digest against the digest recorded under the same idempotency key: /// /// - same key, same digest → the recorded outcome is replayed, nothing re-runs /// - same key, different digest → rejected as a request-ID conflict /// -/// Callers must therefore not regenerate, re-order, or re-render the payload -/// between attempts. There is no client-computed digest field: idempotency is -/// decided host-side, and a caller-supplied digest would be a claim the host -/// has to recompute anyway. +/// A typed value cannot carry that guarantee. Two serializations of one value +/// can differ in bytes across serde versions or implementations, and the +/// difference would surface as a spurious [`BrokerErrorCode::RequestIdConflict`] +/// on retry. Serializing once removes the possibility rather than warning about +/// it. There is no client-computed digest field: idempotency is decided +/// host-side, and a caller-supplied digest would be a claim the host has to +/// recompute anyway. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(deny_unknown_fields, rename_all = "camelCase")] pub struct BrokerRequest { @@ -174,6 +195,23 @@ impl BrokerRequest { self.action.action() } + /// Validate, then serialize once into the bytes every attempt will send. + /// + /// # Errors + /// + /// Returns [`SdkError`] when [`Self::validate`] fails, or + /// [`SdkError::InvalidInput`] if serialization fails. + pub fn prepare(self) -> Result { + self.validate()?; + let body = serde_json::to_vec(&self).map_err(|e| { + SdkError::InvalidInput(format!("broker request is not serializable: {e}")) + })?; + Ok(PreparedRequest { + request: self, + body, + }) + } + /// Validate every field the host must agree on before executing anything. /// /// Callers validate before sending; the host revalidates on receipt, @@ -242,8 +280,48 @@ pub fn validate_request_id(request_id: &str) -> Result<(), SdkError> { Ok(()) } -/// Machine-readable broker error code. +/// A validated request together with the exact bytes to send. /// +/// This is what [`BrokerClient::execute`] takes, so the retry contract is +/// structural rather than documented: the first attempt and every retry send +/// `body` verbatim, and no implementation gets the chance to reserialize. +/// +/// Construct one with [`BrokerRequest::prepare`]. The typed [`BrokerRequest`] is +/// kept alongside so a client can correlate the response without reparsing — +/// see [`BrokerResponse::validate_for`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PreparedRequest { + request: BrokerRequest, + body: Vec, +} + +impl PreparedRequest { + /// The frozen JSON body. Every attempt sends exactly these bytes. + #[must_use] + pub fn body(&self) -> &[u8] { + &self.body + } + + /// The typed request these bytes encode. + #[must_use] + pub fn request(&self) -> &BrokerRequest { + &self.request + } + + /// The idempotency key the host keys replay on. + #[must_use] + pub fn request_id(&self) -> &str { + &self.request.request_id + } + + /// The action being invoked. + #[must_use] + pub fn action(&self) -> Action { + self.request.action() + } +} + +/// Machine-readable broker error code. /// These name failures the *broker* is responsible for. Failures inside an /// action arrive as [`BrokerErrorCode::ActionFailed`] with detail in the /// message. @@ -265,6 +343,10 @@ pub enum BrokerErrorCode { /// on this host. Unsupported, /// The session credential was missing, malformed, or rejected. + /// + /// A host verdict, delivered as [`BrokerResult::Failed`], never as a + /// transport error: the request was refused before execution, so the caller + /// knows no side effects occurred. Unauthenticated, /// The requester is authenticated but not permitted this action. Unauthorized, @@ -456,10 +538,16 @@ impl BrokerResponse { /// Validate discriminator, version, and request id. /// + /// This checks only what a response asserts about itself. It cannot tell + /// whether the response answers the request that was sent — for that, and + /// for outcome-field validation, use [`Self::validate_for`]. A client should + /// always prefer `validate_for`. + /// /// # Errors /// /// Returns [`SdkError::InvalidInput`] on a wrong `type`, an unsupported - /// `protocolVersion`, or a malformed `requestId`. + /// `protocolVersion`, a malformed `requestId`, an outcome with malformed + /// identifiers, or an error code paired with the wrong status. pub fn validate(&self) -> Result<(), SdkError> { if self.r#type != BROKER_RESULT_TYPE { return Err(SdkError::InvalidInput(format!( @@ -473,7 +561,62 @@ impl BrokerResponse { self.protocol_version ))); } - validate_request_id(&self.request_id) + validate_request_id(&self.request_id)?; + match &self.result { + BrokerResult::Succeeded { outcome } => outcome.validate()?, + // `OutcomeUnknown` is the one code that describes not knowing + // whether side effects landed. Paired with `Failed` — which promises + // they did not — it would be self-contradictory, so it is rejected + // rather than left for each caller to notice. + BrokerResult::Failed { error } if error.code == BrokerErrorCode::OutcomeUnknown => { + return Err(SdkError::InvalidInput( + "outcome_unknown is only valid with an indeterminate status".into(), + )); + } + BrokerResult::Failed { .. } | BrokerResult::Indeterminate { .. } => {} + } + Ok(()) + } + + /// Validate this response *as the answer to `request`*. + /// + /// A response that validates in isolation can still be the wrong answer: a + /// host (or a confused proxy) could return a `message.post` success to a + /// `channel.read`, and a caller matching on the outcome enum would quietly + /// take the wrong branch. This is the check that makes such a response + /// unusable instead of merely surprising, which is why + /// [`BrokerClient::execute`] implementations are expected to run it before + /// returning `Ok` and to report a failure as + /// [`BrokerTransportError::MalformedResponse`]. + /// + /// Signature verification of read results is deliberately not included; see + /// [`BrokerMessage::verify`]. + /// + /// # Errors + /// + /// Returns everything [`Self::validate`] returns, plus + /// [`SdkError::InvalidInput`] when the `requestId` does not correlate or a + /// success outcome names a different action than the request. + pub fn validate_for(&self, request: &PreparedRequest) -> Result<(), SdkError> { + self.validate()?; + if self.request_id != request.request_id() { + return Err(SdkError::InvalidInput(format!( + "response requestId \"{}\" does not match request \"{}\"", + self.request_id, + request.request_id() + ))); + } + if let BrokerResult::Succeeded { outcome } = &self.result { + let expected = request.action(); + if outcome.action() != expected { + return Err(SdkError::InvalidInput(format!( + "response carries a {} outcome for a {} request", + outcome.action().as_str(), + expected.as_str() + ))); + } + } + Ok(()) } } diff --git a/crates/buzz-sdk/src/broker/tests.rs b/crates/buzz-sdk/src/broker/tests.rs index a362cd2da3..05aef211cd 100644 --- a/crates/buzz-sdk/src/broker/tests.rs +++ b/crates/buzz-sdk/src/broker/tests.rs @@ -1,6 +1,7 @@ //! Contract tests for the broker envelope, actions, and client trait. use super::*; +use nostr::{EventBuilder, Keys, Kind, Tag}; const CHANNEL: &str = "b2c38ca8-9ec3-411e-bab5-f9deab34d52e"; const PUBKEY: &str = "a02c4e0850e5e612b4ddf95dbe2f5c56467cf27c6552203bc833ff438fb31971"; @@ -10,17 +11,31 @@ fn pubkey() -> PubkeyHex { PubkeyHex::parse(PUBKEY).expect("fixture pubkey is valid hex") } +/// A genuinely signed event, so read fixtures exercise real verification rather +/// than a hand-built value that could never verify. +fn signed_message(keys: &Keys) -> BrokerMessage { + let event = EventBuilder::new(Kind::Custom(9), "hello") + .tags([ + Tag::parse(["h", CHANNEL]).expect("h tag"), + Tag::parse(["e", EVENT, "", "root"]).expect("e tag"), + Tag::parse(["p", PUBKEY]).expect("p tag"), + ]) + .sign_with_keys(keys) + .expect("fixture event signs"); + BrokerMessage(event) +} + /// One valid `args` value per action, so table-driven tests cannot silently -/// skip an action: [`action_fixtures_cover_every_action`] pins the coverage. +/// skip an action: [`fixtures_cover_every_action`] pins the coverage. fn action_fixtures() -> Vec { vec![ - ActionArgs::ChannelRead( - ChannelReadArgs::channel(CHANNEL) - .in_thread(EVENT) - .mentions_only() - .since(1_764_000_000) - .limit(50), - ), + ActionArgs::ChannelRead(ChannelReadArgs { + channel_id: CHANNEL.into(), + root_event_id: Some(EVENT.into()), + mentions_only: true, + cursor: Some("opaque-host-cursor-v1".into()), + limit: Some(50), + }), ActionArgs::MessagePost(MessagePostArgs { channel_id: CHANNEL.into(), content: "shipping the contract".into(), @@ -30,7 +45,7 @@ fn action_fixtures() -> Vec { channel_id: CHANNEL.into(), reply_to_event_id: EVENT.into(), content: "agreed".into(), - mentions: vec![], + mentions: vec![pubkey()], }), ActionArgs::ReactionAdd(ReactionAddArgs { channel_id: CHANNEL.into(), @@ -40,7 +55,7 @@ fn action_fixtures() -> Vec { ActionArgs::ProfileSet(ProfileSetArgs { display_name: Some("ss-dev-00".into()), about: Some("implementation".into()), - picture: None, + picture: Some("https://example.invalid/avatar.png".into()), }), ActionArgs::StorageAddress(StorageAddressArgs { slug: "mem/broker-foundation".into(), @@ -49,19 +64,19 @@ fn action_fixtures() -> Vec { channel_id: CHANNEL.into(), display_name: "Research helper".into(), system_prompt: "Find sources.".into(), - runtime: None, - provider: None, - model: None, + runtime: Some("buzz-acp".into()), + provider: Some("anthropic".into()), + model: Some("claude-sonnet-4-5".into()), respond_to: Some("owner-only".into()), }), ActionArgs::AgentsUpdate(AgentsUpdateArgs { target: AgentTarget::Pubkey(pubkey()), display_name: Some("Research helper v2".into()), - system_prompt: None, - runtime: None, - provider: None, - model: None, - respond_to: None, + system_prompt: Some("Find better sources.".into()), + runtime: Some("buzz-acp".into()), + provider: Some("anthropic".into()), + model: Some("claude-sonnet-4-5".into()), + respond_to: Some("anyone".into()), }), ActionArgs::AgentsDelete(AgentsDeleteArgs { target: AgentTarget::Name("Research helper".into()), @@ -70,19 +85,10 @@ fn action_fixtures() -> Vec { } /// One outcome per action, matching the fixture order above. -fn outcome_fixtures() -> Vec { +fn outcome_fixtures(keys: &Keys) -> Vec { let page = MessagePage { - messages: vec![BrokerMessage { - event_id: EVENT.into(), - author_pubkey: pubkey(), - kind: 9, - created_at: 1_764_000_001, - content: "hello".into(), - root_event_id: Some(EVENT.into()), - parent_event_id: None, - mentions: vec![pubkey()], - }], - next_cursor: Some(1_764_000_002), + messages: vec![signed_message(keys)], + next_cursor: Some("opaque-host-cursor-v2".into()), }; let published = EventPublished { event_id: EVENT.into(), @@ -117,17 +123,37 @@ fn outcome_fixtures() -> Vec { ] } +fn prepared(args: ActionArgs) -> PreparedRequest { + BrokerRequest::new("req-1", args) + .expect("fixture request builds") + .prepare() + .expect("fixture request prepares") +} + +/// Sorted JSON object keys of `value`, for exact-schema assertions. +fn keys_of(value: &serde_json::Value) -> Vec { + let mut keys: Vec = value + .as_object() + .expect("expected a JSON object") + .keys() + .cloned() + .collect(); + keys.sort(); + keys +} + // ── Coverage ──────────────────────────────────────────────────────────────── /// The fixture tables are the input to every table-driven test below, so an /// action added without a fixture would be silently untested. This is the guard. #[test] -fn action_fixtures_cover_every_action() { +fn fixtures_cover_every_action() { + let keys = Keys::generate(); let mut from_args: Vec<&str> = action_fixtures() .iter() .map(|args| args.action().as_str()) .collect(); - let mut from_outcomes: Vec<&str> = outcome_fixtures() + let mut from_outcomes: Vec<&str> = outcome_fixtures(&keys) .iter() .map(|outcome| outcome.action().as_str()) .collect(); @@ -142,19 +168,10 @@ fn action_fixtures_cover_every_action() { from_outcomes, declared, "every action needs an outcome fixture" ); -} -#[test] -fn action_names_are_unique_and_round_trip_through_parse() { - let mut names: Vec<&str> = Action::ALL.iter().map(|a| a.as_str()).collect(); - let count = names.len(); - names.sort_unstable(); - names.dedup(); - assert_eq!(names.len(), count, "action wire names must be unique"); - - for action in Action::ALL { - assert_eq!(Action::parse(action.as_str()).unwrap(), action); - } + let mut unique = declared.clone(); + unique.dedup(); + assert_eq!(unique.len(), declared.len(), "wire names must be unique"); } // ── Envelope round-trip ───────────────────────────────────────────────────── @@ -192,7 +209,8 @@ fn every_action_round_trips_through_a_request_envelope() { #[test] fn every_outcome_round_trips_through_a_response_envelope() { - for outcome in outcome_fixtures() { + let signer = Keys::generate(); + for outcome in outcome_fixtures(&signer) { let action = outcome.action(); let response = BrokerResponse::new("req-1", BrokerResult::succeeded(outcome.clone())); response.validate().expect("response is valid"); @@ -230,12 +248,34 @@ fn an_args_shape_cannot_be_paired_with_another_action_name() { // ── Envelope rejection ────────────────────────────────────────────────────── +/// Unknown names must not resolve, and neither must the *mechanism* names this +/// contract deliberately refuses to expose: an interface that can sign arbitrary +/// bytes is a signing oracle. #[test] -fn unknown_action_name_is_rejected() { - for unknown in ["channel.write", "agents.exfiltrate", "", "channel.read "] { +fn only_declared_action_names_resolve() { + for action in Action::ALL { + assert_eq!(Action::parse(action.as_str()).unwrap(), action); + } + for rejected in [ + "channel.write", + "agents.exfiltrate", + "", + "channel.read ", + "sign", + "sign_event", + "publish", + "nip44.encrypt", + "nip44.decrypt", + "nip42.auth", + "nip98.auth", + "keys.export", + "identity.nsec", + "presence.set", + "typing.set", + ] { assert!( - Action::parse(unknown).is_err(), - "\"{unknown}\" must not parse as an action" + Action::parse(rejected).is_err(), + "\"{rejected}\" must not parse as an action" ); } @@ -250,83 +290,38 @@ fn unknown_action_name_is_rejected() { assert!(serde_json::from_value::(json).is_err()); } -/// Signing and publishing are mechanisms, not actions. An interface that can -/// sign arbitrary bytes is a signing oracle, so these names must not resolve. #[test] -fn signing_and_credential_access_are_not_actions() { - for forbidden in [ - "sign", - "sign_event", - "publish", - "nip44.encrypt", - "nip44.decrypt", - "nip42.auth", - "nip98.auth", - "keys.export", - "identity.nsec", - "tool.exec", - "agents.manage", - ] { - assert!( - Action::parse(forbidden).is_err(), - "\"{forbidden}\" must not be an action" - ); - } -} - -#[test] -fn unknown_protocol_version_is_rejected() { - let args = ActionArgs::AgentsDelete(AgentsDeleteArgs { - target: AgentTarget::Pubkey(pubkey()), - }); +fn envelope_metadata_must_match_this_protocol_version() { + let args = || { + ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Pubkey(pubkey()), + }) + }; + let failed = || BrokerResult::failed(BrokerError::unsupported("no")); for bad in [0_u16, 2, 999] { - let mut request = BrokerRequest::new("req-1", args.clone()).unwrap(); + let mut request = BrokerRequest::new("req-1", args()).unwrap(); request.protocol_version = bad; let error = request.validate().unwrap_err().to_string(); assert!(error.contains("protocolVersion"), "unexpected: {error}"); - let mut response = BrokerResponse::new( - "req-1", - BrokerResult::failed(BrokerError::unsupported("no")), - ); + let mut response = BrokerResponse::new("req-1", failed()); response.protocol_version = bad; assert!(response.validate().is_err()); } -} -#[test] -fn unknown_action_version_is_rejected() { - let mut request = BrokerRequest::new( - "req-1", - ActionArgs::AgentsDelete(AgentsDeleteArgs { - target: AgentTarget::Pubkey(pubkey()), - }), - ) - .unwrap(); - request.action_version = 7; - let error = request.validate().unwrap_err().to_string(); + let mut wrong_action_version = BrokerRequest::new("req-1", args()).unwrap(); + wrong_action_version.action_version = 7; + let error = wrong_action_version.validate().unwrap_err().to_string(); assert!(error.contains("actionVersion"), "unexpected: {error}"); -} -#[test] -fn wrong_type_discriminator_is_rejected() { - let mut request = BrokerRequest::new( - "req-1", - ActionArgs::AgentsDelete(AgentsDeleteArgs { - target: AgentTarget::Pubkey(pubkey()), - }), - ) - .unwrap(); - request.r#type = BROKER_RESULT_TYPE.into(); - assert!(request.validate().is_err()); + let mut wrong_request_type = BrokerRequest::new("req-1", args()).unwrap(); + wrong_request_type.r#type = BROKER_RESULT_TYPE.into(); + assert!(wrong_request_type.validate().is_err()); - let mut response = BrokerResponse::new( - "req-1", - BrokerResult::failed(BrokerError::unsupported("no")), - ); - response.r#type = BROKER_REQUEST_TYPE.into(); - assert!(response.validate().is_err()); + let mut wrong_response_type = BrokerResponse::new("req-1", failed()); + wrong_response_type.r#type = BROKER_REQUEST_TYPE.into(); + assert!(wrong_response_type.validate().is_err()); } #[test] @@ -336,21 +331,161 @@ fn request_id_must_be_present_bounded_and_printable() { target: AgentTarget::Pubkey(pubkey()), }) }; - assert!(BrokerRequest::new("", args()).is_err()); + for (id, valid) in [ + ("", false), + ("has space", false), + ("has\nnewline", false), + ("has\u{7f}del", false), + ("req/1-a.b:c", true), + ] { + assert_eq!( + BrokerRequest::new(id, args()).is_ok(), + valid, + "requestId {id:?} validity" + ); + } assert!(BrokerRequest::new("a".repeat(MAX_REQUEST_ID_LEN), args()).is_ok()); assert!(BrokerRequest::new("a".repeat(MAX_REQUEST_ID_LEN + 1), args()).is_err()); - assert!(BrokerRequest::new("has space", args()).is_err()); - assert!(BrokerRequest::new("has\nnewline", args()).is_err()); - assert!(BrokerRequest::new("has\u{7f}del", args()).is_err()); - assert!(BrokerRequest::new("req/1-a.b:c", args()).is_ok()); +} + +// ── Wire schemas: the enforceable no-secret invariant ─────────────────────── + +/// The exact wire key set of every args and outcome type, with every optional +/// field populated so nothing escapes the pin by being absent. +/// +/// This table *is* the no-secret invariant. Combined with +/// `deny_unknown_fields`, it means no field — secret-bearing or otherwise — can +/// be added to this contract without a reviewer changing a line here. The +/// `agents.create` outcome is the case that matters: public identity only, never +/// the key the host just minted. +#[test] +fn every_payload_has_an_exact_and_secret_free_wire_schema() { + let signer = Keys::generate(); + let expected: Vec<(&str, Vec<&str>)> = vec![ + // Args, fully populated (optional fields present). + ( + "channel.read/args", + vec![ + "channelId", + "cursor", + "limit", + "mentionsOnly", + "rootEventId", + ], + ), + ( + "message.post/args", + vec!["channelId", "content", "mentions"], + ), + ( + "message.reply/args", + vec!["channelId", "content", "mentions", "replyToEventId"], + ), + ( + "reaction.add/args", + vec!["channelId", "reaction", "targetEventId"], + ), + ("profile.set/args", vec!["about", "displayName", "picture"]), + ("storage.address/args", vec!["slug"]), + ( + "agents.create/args", + vec![ + "channelId", + "displayName", + "model", + "provider", + "respondTo", + "runtime", + "systemPrompt", + ], + ), + ( + "agents.update/args", + vec![ + "displayName", + "model", + "provider", + "respondTo", + "runtime", + "systemPrompt", + "target", + ], + ), + ("agents.delete/args", vec!["target"]), + // Outcomes. + ("channel.read/outcome", vec!["messages", "nextCursor"]), + ("message.post/outcome", vec!["createdAt", "eventId", "kind"]), + ( + "message.reply/outcome", + vec!["createdAt", "eventId", "kind"], + ), + ("reaction.add/outcome", vec!["createdAt", "eventId", "kind"]), + ("profile.set/outcome", vec!["createdAt", "eventId", "kind"]), + ( + "storage.address/outcome", + vec!["authorPubkey", "dTag", "kind"], + ), + ( + "agents.create/outcome", + vec!["agentPubkey", "channelId", "displayName"], + ), + ( + "agents.update/outcome", + vec!["agentPubkey", "displayName", "updatedFields"], + ), + ("agents.delete/outcome", vec!["agentPubkey", "displayName"]), + ]; + + let mut actual: Vec<(String, Vec)> = Vec::new(); + for args in action_fixtures() { + let json = serde_json::to_value(&args).expect("args serialize"); + actual.push(( + format!("{}/args", args.action().as_str()), + keys_of(&json["args"]), + )); + } + for outcome in outcome_fixtures(&signer) { + let json = serde_json::to_value(&outcome).expect("outcome serializes"); + actual.push(( + format!("{}/outcome", outcome.action().as_str()), + keys_of(&json["outcome"]), + )); + } + + let expected: Vec<(String, Vec)> = expected + .into_iter() + .map(|(name, keys)| { + ( + name.to_string(), + keys.into_iter().map(str::to_string).collect(), + ) + }) + .collect(); + assert_eq!( + actual, expected, + "a payload's wire keys changed — confirm no field can carry key material" + ); + + // And no key anywhere in the contract even *looks* like secret material. + for (name, keys) in &actual { + for key in keys { + let lower = key.to_ascii_lowercase(); + for forbidden in ["secret", "private", "nsec", "seckey", "credential", "token"] { + assert!( + !lower.contains(forbidden), + "{name} exposes \"{key}\", which reads as secret material" + ); + } + } + } } /// The envelope must not carry requester, owner, or scope: those are derived by /// the host from the credential. A body that could name its own subject would -/// let any caller act as anyone. +/// let any caller act as anyone — the same reason `agents.create` has no owner. #[test] -fn request_carries_no_caller_supplied_authority() { - let json = serde_json::to_value( +fn no_payload_can_name_its_own_authority() { + let request = serde_json::to_value( BrokerRequest::new( "req-1", ActionArgs::ChannelRead(ChannelReadArgs::channel(CHANNEL)), @@ -358,163 +493,58 @@ fn request_carries_no_caller_supplied_authority() { .unwrap(), ) .unwrap(); - let object = json.as_object().unwrap(); - - for forbidden in [ - "ownerPubkey", - "owner", - "requesterPubkey", - "requester", - "agentPubkey", - "authorization", - "credential", - "token", - "scope", - "expiry", - "relayUrl", - "requestDigest", - ] { - assert!( - !object.contains_key(forbidden), - "broker request must not carry \"{forbidden}\"" - ); - } - - let mut keys: Vec<&str> = object.keys().map(String::as_str).collect(); - keys.sort_unstable(); assert_eq!( - keys, + keys_of(&request), vec![ "action", "actionVersion", "args", "protocolVersion", "requestId", - "type", + "type" ] ); -} -/// `agents.create` must not let a request name its own authority: ownership is -/// the authenticated requester, which is why there is no field for it. -#[test] -fn agents_create_has_no_owner_field() { - let json = serde_json::to_value(AgentsCreateArgs { - channel_id: CHANNEL.into(), - display_name: "A".into(), - system_prompt: "B".into(), - runtime: None, - provider: None, - model: None, - respond_to: None, - }) - .unwrap(); - for forbidden in ["owner", "ownerPubkey", "createdBy", "onBehalfOf"] { - assert!(json.get(forbidden).is_none()); - } - - let with_owner = serde_json::json!({ - "channelId": CHANNEL, - "displayName": "A", - "systemPrompt": "B", - "ownerPubkey": PUBKEY, - }); - assert!(serde_json::from_value::(with_owner).is_err()); -} - -// ── The no-secret invariant ───────────────────────────────────────────────── - -/// No type in this contract may name, hold, or accept secret key material. -/// -/// Scanning the source is the load-bearing half of this invariant: a -/// deserialization test can only reject fields someone thought to add, while -/// this fails the moment a secret-key type appears in a signature at all. -#[test] -fn no_secret_key_type_appears_in_the_contract_source() { - // `tests.rs` is excluded: this file necessarily names the forbidden tokens. - let sources = [ - ("mod.rs", include_str!("mod.rs")), - ("actions.rs", include_str!("actions.rs")), - ("client.rs", include_str!("client.rs")), - ]; - // Substrings, so `Option` and `nostr::Keys` are caught too. - let forbidden = [ - "SecretKey", - "nostr::Keys", - "Keys::", - "secret_key", - "seckey", - "private_key", - "nsec1", - "sign_event", - "ConversationKey", - ]; - - for (name, source) in sources { - for token in forbidden { - for (index, line) in source.lines().enumerate() { - // Doc comments may discuss what is absent and why. - let code = line.trim_start(); - if code.starts_with("//") { - continue; - } - assert!( - !code.contains(token), - "{name}:{} names \"{token}\" in code: {code}", - index + 1 - ); - } - } - } -} - -/// A secret smuggled into args must fail to deserialize rather than reach an -/// executor. -#[test] -fn secret_bearing_args_fail_to_deserialize() { - let smuggled = [ + // Authority-naming fields are rejected, not ignored, wherever they appear. + for rejected in [ serde_json::json!({ - "channelId": CHANNEL, - "displayName": "Sneaky", - "systemPrompt": "hi", + "channelId": CHANNEL, "displayName": "A", "systemPrompt": "B", + "ownerPubkey": PUBKEY, + }), + serde_json::json!({ + "channelId": CHANNEL, "displayName": "A", "systemPrompt": "B", + "onBehalfOf": PUBKEY, + }), + serde_json::json!({ + "channelId": CHANNEL, "displayName": "A", "systemPrompt": "B", "envVars": { "ANTHROPIC_API_KEY": "sk-live" }, }), serde_json::json!({ - "channelId": CHANNEL, - "displayName": "Sneaky", - "systemPrompt": "hi", + "channelId": CHANNEL, "displayName": "A", "systemPrompt": "B", "secretKey": "nsec1deadbeef", }), - serde_json::json!({ - "channelId": CHANNEL, - "displayName": "Sneaky", - "systemPrompt": "hi", - "privateKey": "hexsecret", - }), - ]; - for args in smuggled { + ] { assert!( - serde_json::from_value::(args.clone()).is_err(), - "must reject: {args}" + serde_json::from_value::(rejected.clone()).is_err(), + "must reject: {rejected}" ); } -} -/// An outcome must be structurally unable to carry secret material, including -/// the freshly minted key of an agent it just created. -#[test] -fn outcomes_cannot_carry_secrets() { - for extra in [ - "privateKeyNsec", - "nsec", - "secretKey", - "seckey", - "credential", - ] { + // A read cannot ask about someone else's mentions, and a profile write + // cannot name a subject. + assert!(serde_json::from_value::( + serde_json::json!({ "channelId": CHANNEL, "mentionsOf": PUBKEY }) + ) + .is_err()); + assert!(serde_json::from_value::( + serde_json::json!({ "displayName": "A", "pubkey": PUBKEY }) + ) + .is_err()); + + // An outcome cannot smuggle a minted secret past the schema either. + for extra in ["nsec", "secretKey", "seckey", "credential"] { let mut outcome = serde_json::json!({ - "agentPubkey": PUBKEY, - "displayName": "A", - "channelId": CHANNEL, + "agentPubkey": PUBKEY, "displayName": "A", "channelId": CHANNEL, }); outcome[extra] = serde_json::json!("nsec1deadbeef"); let json = serde_json::json!({ "action": "agents.create", "outcome": outcome }); @@ -525,26 +555,6 @@ fn outcomes_cannot_carry_secrets() { } } -/// A `storage.address` outcome carries addressing material only — never the key -/// that derived it. -#[test] -fn storage_address_outcome_carries_addressing_only() { - let json = serde_json::to_value(StorageAddress { - author_pubkey: pubkey(), - kind: 30174, - d_tag: EVENT.into(), - }) - .unwrap(); - let mut keys: Vec<&str> = json - .as_object() - .unwrap() - .keys() - .map(String::as_str) - .collect(); - keys.sort_unstable(); - assert_eq!(keys, vec!["authorPubkey", "dTag", "kind"]); -} - #[test] fn pubkey_hex_rejects_anything_but_a_public_key() { assert!(PubkeyHex::parse("nothex").is_err()); @@ -562,137 +572,102 @@ fn pubkey_hex_rejects_anything_but_a_public_key() { // ── Argument validation ───────────────────────────────────────────────────── +/// Boundaries of every shared validator, in one table. #[test] -fn every_fixture_validates_and_normalization_is_idempotent() { - for args in action_fixtures() { - let once = args.validated().expect("fixture validates"); - let twice = once.validated().expect("normalized form validates"); - assert_eq!( - once, - twice, - "{} normalization must settle", - args.action().as_str() - ); - } -} - -#[test] -fn reads_reject_a_bad_channel_thread_or_limit() { - assert!(ChannelReadArgs::channel("not-a-uuid").validated().is_err()); - assert!(ChannelReadArgs::channel(CHANNEL) - .in_thread("nothex") - .validated() - .is_err()); - assert!(ChannelReadArgs::channel(CHANNEL) - .limit(0) - .validated() - .is_err()); - assert!(ChannelReadArgs::channel(CHANNEL) - .limit(actions::MAX_PAGE_LIMIT) - .validated() - .is_ok()); - assert!(ChannelReadArgs::channel(CHANNEL) - .limit(actions::MAX_PAGE_LIMIT + 1) - .validated() - .is_err()); -} - -/// A bare channel read must not imply a thread or a mention filter the caller -/// never asked for. -#[test] -fn a_bare_channel_read_carries_no_unset_narrowing() { - let json = serde_json::to_value(ChannelReadArgs::channel(CHANNEL)).unwrap(); - let mut keys: Vec<&str> = json - .as_object() - .unwrap() - .keys() - .map(String::as_str) - .collect(); - keys.sort_unstable(); - assert_eq!(keys, vec!["channelId"]); -} - -#[test] -fn writes_reject_empty_oversized_and_over_mentioned_content() { - let post = |content: String, mentions: Vec| MessagePostArgs { - channel_id: CHANNEL.into(), - content, - mentions, +fn validators_accept_and_reject_at_their_boundaries() { + let read = |mutate: fn(&mut ChannelReadArgs)| { + let mut args = ChannelReadArgs::channel(CHANNEL); + mutate(&mut args); + args.validated().is_ok() }; - assert!(post(" ".into(), vec![]).validated().is_err()); + let post = |content: String, mentions: Vec| { + MessagePostArgs { + channel_id: CHANNEL.into(), + content, + mentions, + } + .validated() + }; + let react = |reaction: String| { + ReactionAddArgs { + channel_id: CHANNEL.into(), + target_event_id: EVENT.into(), + reaction, + } + .validated() + }; + let slug = |slug: &str| StorageAddressArgs { slug: slug.into() }.validated().is_ok(); + + // Channel UUID, thread id, limit, and opaque cursor. + assert!(ChannelReadArgs::channel("not-a-uuid").validated().is_err()); + assert!(!read(|a| a.root_event_id = Some("nothex".into()))); + assert!(read(|a| a.root_event_id = Some(EVENT.into()))); + assert!(!read(|a| a.limit = Some(0))); + assert!(read(|a| a.limit = Some(actions::MAX_PAGE_LIMIT))); + assert!(!read(|a| a.limit = Some(actions::MAX_PAGE_LIMIT + 1))); + assert!(!read(|a| a.cursor = Some(String::new()))); + assert!(!read(|a| a.cursor = Some("has space".into()))); + assert!(read( + |a| a.cursor = Some("a".repeat(actions::MAX_CURSOR_LEN)) + )); + assert!(!read( + |a| a.cursor = Some("a".repeat(actions::MAX_CURSOR_LEN + 1)) + )); + + // Content, mentions, reaction payload. + assert!(post(" ".into(), vec![]).is_err()); assert!(matches!( - post("x".repeat(actions::MAX_CONTENT_BYTES + 1), vec![]) - .validated() - .unwrap_err(), + post("x".repeat(actions::MAX_CONTENT_BYTES + 1), vec![]).unwrap_err(), SdkError::ContentTooLarge { .. } )); + assert!(post("hi".into(), vec![pubkey(); actions::MAX_MENTIONS]).is_ok()); assert!(matches!( - post("hi".into(), vec![pubkey(); actions::MAX_MENTIONS + 1]) - .validated() - .unwrap_err(), + post("hi".into(), vec![pubkey(); actions::MAX_MENTIONS + 1]).unwrap_err(), SdkError::TooManyMentions )); - assert!(post("hi".into(), vec![pubkey(); actions::MAX_MENTIONS]) - .validated() - .is_ok()); -} - -#[test] -fn reaction_rejects_empty_and_oversized_payloads() { - let react = |reaction: String| ReactionAddArgs { - channel_id: CHANNEL.into(), - target_event_id: EVENT.into(), - reaction, - }; - assert!(react(" ".into()).validated().is_err()); + assert!(react(" ".into()).is_err()); + assert!(react(":shipit:".into()).is_ok()); assert!(matches!( - react("a".repeat(actions::MAX_EMOJI_CHARS + 1)) - .validated() - .unwrap_err(), + react("a".repeat(actions::MAX_EMOJI_CHARS + 1)).unwrap_err(), SdkError::EmojiTooLong )); - assert!(react(":shipit:".into()).validated().is_ok()); -} -#[test] -fn profile_set_requires_a_change_and_names_no_subject() { - let empty = ProfileSetArgs { + // NIP-AE slug grammar for encrypted-memory addressing. + assert!(slug("core")); + assert!(slug("mem/broker-foundation")); + assert!(!slug("")); + assert!(!slug("Core")); + assert!(!slug("secrets")); + assert!(!slug("mem/Bad Slug")); + + // Patch-shaped writes must change something, and reject unknown modes. + let profile_error = ProfileSetArgs { display_name: None, about: None, picture: None, + } + .validated() + .unwrap_err() + .to_string(); + assert!(profile_error.contains("at least one"), "{profile_error}"); + let update = |respond_to: Option<&str>, name: Option<&str>| { + AgentsUpdateArgs { + target: AgentTarget::Pubkey(pubkey()), + display_name: name.map(str::to_string), + system_prompt: None, + runtime: None, + provider: None, + model: None, + respond_to: respond_to.map(str::to_string), + } + .validated() }; - let error = empty.validated().unwrap_err().to_string(); - assert!(error.contains("at least one"), "unexpected: {error}"); - - let with_subject = serde_json::json!({ "displayName": "A", "pubkey": PUBKEY }); - assert!(serde_json::from_value::(with_subject).is_err()); -} - -#[test] -fn storage_address_enforces_the_nip_ae_slug_grammar() { - let slug = |slug: &str| StorageAddressArgs { slug: slug.into() }.validated(); - assert!(slug("core").is_ok()); - assert!(slug("mem/broker-foundation").is_ok()); - assert!(slug("").is_err()); - assert!(slug("Core").is_err()); - assert!(slug("secrets").is_err()); - assert!(slug("mem/Bad Slug").is_err()); -} - -#[test] -fn agent_update_requires_a_change_and_delete_requires_a_target() { - let empty = AgentsUpdateArgs { - target: AgentTarget::Pubkey(pubkey()), - display_name: None, - system_prompt: None, - runtime: None, - provider: None, - model: None, - respond_to: None, - }; - let error = empty.validated().unwrap_err().to_string(); - assert!(error.contains("at least one field"), "unexpected: {error}"); - + assert!(update(None, None) + .unwrap_err() + .to_string() + .contains("at least one field")); + assert!(update(Some("anyone"), None).is_ok()); + assert!(update(Some("allowlist"), Some("A")).is_err()); assert!(AgentsDeleteArgs { target: AgentTarget::Name(" ".into()), } @@ -700,41 +675,70 @@ fn agent_update_requires_a_change_and_delete_requires_a_target() { .is_err()); } +// ── Reads carry verifiable provenance ─────────────────────────────────────── + +/// A read returns the signed event, so a keyless caller can check authorship +/// itself. A host that tampered with content fails verification locally, with no +/// relay involved — which is why this contract does not settle for a projection. #[test] -fn agents_create_rejects_an_unsupported_respond_to_mode() { - let args = AgentsCreateArgs { - channel_id: CHANNEL.into(), - display_name: "A".into(), - system_prompt: "B".into(), - runtime: None, - provider: None, - model: None, - respond_to: Some("allowlist".into()), - }; - assert!(args.validated().is_err()); +fn read_results_are_signed_events_a_keyless_caller_can_verify() { + let signer = Keys::generate(); + let message = signed_message(&signer); + message.verify().expect("a genuinely signed event verifies"); + assert_eq!( + message.author().unwrap().as_str(), + signer.public_key().to_hex() + ); + assert_eq!(message.thread().root.as_deref(), Some(EVENT)); + assert_eq!(message.mentions(), vec![PUBKEY.to_string()]); + + // Tamper with the content: the id no longer matches, so verification fails + // even though every other field is untouched. + let mut json = serde_json::to_value(&message).unwrap(); + json["content"] = serde_json::json!("a message the author never wrote"); + let tampered: BrokerMessage = + serde_json::from_value(json).expect("a tampered event still parses"); + assert!( + tampered.verify().is_err(), + "tampering must be locally detectable" + ); + + // The wire form is the event's own JSON — no wrapper of its own to disagree + // with the signed bytes. + let wire = serde_json::to_value(&message).unwrap(); + assert_eq!( + keys_of(&wire), + vec![ + "content", + "created_at", + "id", + "kind", + "pubkey", + "sig", + "tags" + ] + ); } -/// Update and delete carry no channel: channel appears only where the operation -/// needs it, which is the create attachment. #[test] -fn agent_update_and_delete_carry_no_channel() { - let update = serde_json::to_value(AgentsUpdateArgs { - target: AgentTarget::Pubkey(pubkey()), - display_name: Some("New".into()), - system_prompt: None, - runtime: None, - provider: None, - model: None, - respond_to: None, - }) - .unwrap(); - assert!(update.get("channelId").is_none()); - - let delete = serde_json::to_value(AgentsDeleteArgs { - target: AgentTarget::Name("Gone".into()), - }) - .unwrap(); - assert!(delete.get("channelId").is_none()); +fn a_page_is_bounded_and_its_cursor_opaque() { + let signer = Keys::generate(); + let page = |messages: Vec, next_cursor: Option<&str>| { + ActionOutcome::ChannelRead(MessagePage { + messages, + next_cursor: next_cursor.map(str::to_string), + }) + .validate() + }; + assert!(page(vec![], None).is_ok()); + assert!(page(vec![signed_message(&signer)], Some("c1")).is_ok()); + assert!(page(vec![], Some("")).is_err()); + assert!(page(vec![], Some("has space")).is_err()); + assert!(page( + vec![signed_message(&signer); actions::MAX_PAGE_LIMIT as usize + 1], + None + ) + .is_err()); } // ── Results ───────────────────────────────────────────────────────────────── @@ -764,6 +768,39 @@ fn failed_and_indeterminate_are_distinct_and_carry_no_outcome() { assert!(indeterminate.outcome().is_none()); } +/// A rejected credential is a host verdict with a known fate — the action did +/// not run — so it belongs in a `Failed` envelope, not in the transport error +/// type where it would tell the caller to reconcile something that never +/// happened. `OutcomeUnknown` is the mirror image: it may only ever be +/// `Indeterminate`. +#[test] +fn status_and_error_code_must_agree_about_side_effects() { + let auth_failure = BrokerResponse::new( + "req-1", + BrokerResult::failed(BrokerError::new( + BrokerErrorCode::Unauthenticated, + "credential rejected", + )), + ); + auth_failure + .validate() + .expect("a rejected credential is a valid Failed verdict"); + + let contradiction = BrokerResponse::new( + "req-1", + BrokerResult::failed(BrokerError::new(BrokerErrorCode::OutcomeUnknown, "?")), + ); + let error = contradiction.validate().unwrap_err().to_string(); + assert!(error.contains("outcome_unknown"), "unexpected: {error}"); + + BrokerResponse::new( + "req-1", + BrokerResult::indeterminate(BrokerError::new(BrokerErrorCode::OutcomeUnknown, "?")), + ) + .validate() + .expect("outcome_unknown is valid with indeterminate"); +} + #[test] fn replay_metadata_rides_the_response_not_the_result() { let result = BrokerResult::succeeded(ActionOutcome::AgentsDelete(AgentsDeleteOutcome { @@ -789,54 +826,167 @@ fn replay_metadata_rides_the_response_not_the_result() { .is_none()); } +/// A response that validates in isolation can still be the wrong answer. This is +/// the check that makes a mismatched outcome unusable rather than merely +/// surprising. #[test] -fn every_error_code_has_a_stable_wire_string() { - for (code, expected) in [ - (BrokerErrorCode::InvalidRequest, "invalid_request"), - ( - BrokerErrorCode::UnsupportedProtocolVersion, - "unsupported_protocol_version", - ), - (BrokerErrorCode::UnknownAction, "unknown_action"), - ( - BrokerErrorCode::UnsupportedActionVersion, - "unsupported_action_version", - ), - (BrokerErrorCode::Unsupported, "unsupported"), - (BrokerErrorCode::Unauthenticated, "unauthenticated"), - (BrokerErrorCode::Unauthorized, "unauthorized"), - (BrokerErrorCode::RequestIdConflict, "request_id_conflict"), - (BrokerErrorCode::ActionFailed, "action_failed"), - (BrokerErrorCode::OutcomeUnknown, "outcome_unknown"), - (BrokerErrorCode::Internal, "internal"), - ] { - assert_eq!(code.as_str(), expected); +fn response_validation_is_request_aware() { + let signer = Keys::generate(); + let request = prepared(ActionArgs::ChannelRead(ChannelReadArgs::channel(CHANNEL))); + let page = ActionOutcome::ChannelRead(MessagePage { + messages: vec![signed_message(&signer)], + next_cursor: None, + }); + + BrokerResponse::new(request.request_id(), BrokerResult::succeeded(page.clone())) + .validate_for(&request) + .expect("the right outcome for the right request"); + + // Wrong action: a post receipt is not an answer to a read. + let wrong_action = BrokerResponse::new( + request.request_id(), + BrokerResult::succeeded(ActionOutcome::MessagePost(EventPublished { + event_id: EVENT.into(), + kind: 9, + created_at: 1, + })), + ); + wrong_action + .validate() + .expect("it is well-formed on its own — that is the point"); + let error = wrong_action.validate_for(&request).unwrap_err().to_string(); + assert!(error.contains("message.post"), "unexpected: {error}"); + + // Wrong correlation id. + let error = BrokerResponse::new("req-other", BrokerResult::succeeded(page)) + .validate_for(&request) + .unwrap_err() + .to_string(); + assert!(error.contains("requestId"), "unexpected: {error}"); + + // Malformed identifiers inside an otherwise well-shaped outcome. + let bad_id = BrokerResponse::new( + request.request_id(), + BrokerResult::succeeded(ActionOutcome::ChannelRead(MessagePage { + messages: vec![], + next_cursor: Some("not a cursor".into()), + })), + ); + assert!(bad_id.validate_for(&request).is_err()); + + let post = prepared(ActionArgs::MessagePost(MessagePostArgs { + channel_id: CHANNEL.into(), + content: "hi".into(), + mentions: vec![], + })); + let bad_event_id = BrokerResponse::new( + post.request_id(), + BrokerResult::succeeded(ActionOutcome::MessagePost(EventPublished { + event_id: "nothex".into(), + kind: 9, + created_at: 1, + })), + ); + assert!(bad_event_id.validate_for(&post).is_err()); + + // A failure needs no outcome to match, only correlation. + BrokerResponse::new( + request.request_id(), + BrokerResult::failed(BrokerError::unauthorized("not your channel")), + ) + .validate_for(&request) + .expect("a refusal answers any action"); +} + +// ── Retry is identical bytes ──────────────────────────────────────────────── + +/// The retry contract is byte identity, so the client takes frozen bytes rather +/// than a typed value it would have to reserialize. Preparing once and reading +/// `body()` twice is the only way to send the same request twice. +#[test] +fn preparing_a_request_freezes_the_bytes_every_attempt_sends() { + let request = BrokerRequest::new( + "req-idem", + ActionArgs::MessagePost(MessagePostArgs { + channel_id: CHANNEL.into(), + content: "exactly once".into(), + mentions: vec![pubkey()], + }), + ) + .unwrap(); + let prepared = request.clone().prepare().expect("valid request prepares"); + + assert_eq!( + prepared.body(), + prepared.body(), + "body is frozen, not re-rendered" + ); + assert_eq!(prepared.request(), &request); + assert_eq!(prepared.request_id(), "req-idem"); + assert_eq!(prepared.action(), Action::MessagePost); + + // The frozen bytes are the envelope, and they parse back to the same value. + let parsed: BrokerRequest = + serde_json::from_slice(prepared.body()).expect("frozen body is the envelope"); + assert_eq!(parsed, request); + + // Preparing validates, so an invalid request never reaches a transport. + let invalid = BrokerRequest { + r#type: BROKER_REQUEST_TYPE.into(), + protocol_version: 99, + request_id: "req-bad".into(), + action_version: 1, + action: ActionArgs::AgentsDelete(AgentsDeleteArgs { + target: AgentTarget::Pubkey(pubkey()), + }), + }; + assert!(invalid.prepare().is_err()); +} + +/// The hand-written [`BrokerErrorCode::as_str`] and serde's derived name are two +/// encodings of one wire string, so each is pinned against the other and the +/// whole set is pinned against this literal — a rename in either fails here. +#[test] +fn error_codes_have_stable_wire_strings() { + use BrokerErrorCode as E; + let codes = [ + E::InvalidRequest, + E::UnsupportedProtocolVersion, + E::UnknownAction, + E::UnsupportedActionVersion, + E::Unsupported, + E::Unauthenticated, + E::Unauthorized, + E::RequestIdConflict, + E::ActionFailed, + E::OutcomeUnknown, + E::Internal, + ]; + for code in codes { assert_eq!( serde_json::to_value(code).unwrap(), - serde_json::json!(expected) + serde_json::json!(code.as_str()), + "as_str and the serde name must not drift" ); } + assert_eq!( + codes.map(E::as_str).join(","), + "invalid_request,unsupported_protocol_version,unknown_action,\ +unsupported_action_version,unsupported,unauthenticated,unauthorized,\ +request_id_conflict,action_failed,outcome_unknown,internal" + ); } /// Only housekeeping is best-effort. A host refusing a read or a write has /// broken the agent, and the agent must not treat that as routine. #[test] fn only_housekeeping_actions_are_best_effort() { - assert!(Action::ReactionAdd.is_best_effort()); - for essential in [ - Action::ChannelRead, - Action::MessagePost, - Action::MessageReply, - Action::ProfileSet, - Action::StorageAddress, - Action::AgentsCreate, - Action::AgentsUpdate, - Action::AgentsDelete, - ] { - assert!( - !essential.is_best_effort(), - "{} must not be best-effort", - essential.as_str() + for action in Action::ALL { + assert_eq!( + action.is_best_effort(), + action == Action::ReactionAdd, + "{} best-effort classification", + action.as_str() ); } } @@ -851,10 +1001,15 @@ struct DoubleBroker { } impl BrokerClient for DoubleBroker { - fn execute<'a>(&'a self, request: &'a BrokerRequest) -> BrokerFuture<'a> { - let response = self.response.clone().map(|mut response| { - response.request_id = request.request_id.clone(); + fn execute<'a>(&'a self, request: &'a PreparedRequest) -> BrokerFuture<'a> { + // A real implementation sends `request.body()` verbatim. The double + // stands in for a host that answers the request it was given. + let response = self.response.clone().and_then(|mut response| { + response.request_id = request.request_id().to_string(); response + .validate_for(request) + .map_err(|e| BrokerTransportError::MalformedResponse(e.to_string()))?; + Ok(response) }); Box::pin(async move { response }) } @@ -879,11 +1034,11 @@ fn block_on(future: F) -> F::Output { #[test] fn the_client_trait_is_object_safe_and_returns_a_host_verdict() { - let request = BrokerRequest::new( - "req-42", - ActionArgs::ChannelRead(ChannelReadArgs::channel(CHANNEL).mentions_only()), - ) - .unwrap(); + let request = prepared(ActionArgs::ChannelRead(ChannelReadArgs { + channel_id: CHANNEL.into(), + mentions_only: true, + ..ChannelReadArgs::default() + })); let succeeded: Box = Box::new(DoubleBroker { response: Ok(BrokerResponse::new( @@ -895,35 +1050,57 @@ fn the_client_trait_is_object_safe_and_returns_a_host_verdict() { )), }); let response = block_on(succeeded.execute(&request)).expect("double answers"); - response.validate().expect("double's response is valid"); - assert_eq!(response.request_id, "req-42"); + response + .validate_for(&request) + .expect("double's response answers the request"); + assert_eq!(response.request_id, "req-1"); assert!(response.result.outcome().is_some()); - // A refusal is still an answer: `Ok` with the verdict in the envelope. - let refused: Box = Box::new(DoubleBroker { + // A refusal — including a rejected credential — is still an answer: `Ok` + // with the verdict in the envelope. + for code in [ + BrokerErrorCode::Unauthorized, + BrokerErrorCode::Unauthenticated, + ] { + let refused: Box = Box::new(DoubleBroker { + response: Ok(BrokerResponse::new( + "placeholder", + BrokerResult::failed(BrokerError::new(code, "no")), + )), + }); + let response = + block_on(refused.execute(&request)).expect("a refusal is not a transport error"); + assert_eq!(response.result.error().map(|e| e.code), Some(code)); + } + + // No usable answer at all is a transport error, and says nothing about side + // effects. An intermediary's status is operator detail, not a verdict. + for error in [ + BrokerTransportError::Unreachable("connection reset".into()), + BrokerTransportError::NoEnvelope { + status: 401, + detail: "proxy denied".into(), + }, + BrokerTransportError::MalformedResponse("not json".into()), + ] { + let broken: Box = Box::new(DoubleBroker { + response: Err(error.clone()), + }); + assert_eq!(block_on(broken.execute(&request)).unwrap_err(), error); + } + + // A host answering the wrong action is rejected by the client, not passed on. + let confused: Box = Box::new(DoubleBroker { response: Ok(BrokerResponse::new( "placeholder", - BrokerResult::failed(BrokerError::unauthorized("not your channel")), + BrokerResult::succeeded(ActionOutcome::AgentsDelete(AgentsDeleteOutcome { + agent_pubkey: pubkey(), + display_name: "Gone".into(), + })), )), }); - let response = block_on(refused.execute(&request)).expect("a refusal is not a transport error"); - assert_eq!( - response.result.error().map(|e| e.code), - Some(BrokerErrorCode::Unauthorized) - ); - - // No answer at all is a transport error, and says nothing about side effects. - let unreachable: Box = Box::new(DoubleBroker { - response: Err(BrokerTransportError::CredentialRejected), - }); - assert_eq!( - block_on(unreachable.execute(&request)).unwrap_err(), - BrokerTransportError::CredentialRejected - ); -} - -#[test] -fn the_http_binding_is_a_single_path_with_a_bearer_credential() { - assert_eq!(BROKER_ACTION_PATH, "/v1/action"); - assert_eq!(BROKER_CREDENTIAL_HEADER, "authorization"); + assert!(matches!( + block_on(confused.execute(&request)).unwrap_err(), + BrokerTransportError::MalformedResponse(_) + )); }