diff --git a/AGENTS.md b/AGENTS.md index 0e93a7a51d..c804c594f4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -260,7 +260,14 @@ That realignment fixed two real defects, both pinned by tests in `src/core/all_t - `harness()` claimed "agent + memory + threads + config + security" but silently dropped `agent::{agentbox, harness_init, artifacts, learning}`, `security::{credentials, devices}`, `config::{workspace, migration_helpers}`, `memory::people` and `skills::webhooks` into `Platform`. An agent harness that never registers `harness_init` is a latent bug. - `embedded()` had to set `platform: true` purely to reach credentials and config, which dragged the desktop and hosted-backend surfaces along with it. Those are `Desktop` / `Hosted` now and stay off. -**Adding a family directory means four edits, all compiler-enforced:** the `DomainGroup` variant (`src/core/all.rs`), the `DomainSet` field + `allows()` arm + every preset (`src/core/runtime/builder.rs`). Two more are *not* compiler-enforced and are the usual source of drift — `tool_group()` in `src/openhuman/tools/ops.rs` (a missing entry leaks a gated tool under a custom `DomainSet`; this is the #4808 review finding) and the `StoreInitPlan` / `DomainSubscriberPlan` keys (`runtime/context.rs`, `core/jsonrpc.rs`). Registering a controller whose store keys on a different group gives you a live RPC surface with no store behind it. +**Adding a family directory means four edits, all compiler-enforced:** the `DomainGroup` variant (`src/core/all.rs`), the `DomainSet` field + `allows()` arm + every preset (`src/core/runtime/builder.rs`). + +Three more consumers are *not* compiler-enforced — `tool_group()` (`tools/ops.rs`), `StoreInitPlan` (`runtime/context.rs`) and `DomainSubscriberPlan` (`core/jsonrpc.rs`) — so **drift guards** stand in for the compiler. Each forces every variant into exactly one of two lists (owns-a-store / storeless, registers-subscribers / none, owns-tools / tool-less), so adding a family cannot compile-and-forget: + +- `domain_group_all_lists_every_variant` is the root of trust. `DomainGroup::index()` is an exhaustive `match`, so a new variant is a compile error there first; this test then fails until `DomainGroup::ALL` and `COUNT` catch up. The other guards iterate `ALL`, so they are only as good as this one. +- `every_domain_group_is_accounted_for_in_tool_group` tests the *function*, not a built registry — which tools a registry contains depends on config flags, security tier and enabled integrations, so a registry-derived assertion passes or fails for unrelated reasons. `REPRESENTATIVE` holds one real tool name per family; `representative_tool_names_are_real` keeps that table from rotting into dead strings. + +These are not theoretical. Two bugs of exactly this shape shipped before the guards existed: `harness_init` sat in `Platform` so `DomainSet::harness()` never registered it, and the `Inference` rule matched `tokenjuice_` while the live tool is `tinyjuice_retrieve` (`tokenjuice_retrieve` is a migration alias), so CCR retrieval leaked to `Platform`. **Match tool names against the owning crate's constants, not a guessed prefix.** A controller whose store keys on a different group than its `push(...)` tag gives you a live RPC surface with no store behind it. ### Compile-time domain gates (Cargo `[features]`) diff --git a/src/core/all.rs b/src/core/all.rs index e3234a67f1..b95fb6860b 100644 --- a/src/core/all.rs +++ b/src/core/all.rs @@ -143,6 +143,75 @@ pub enum DomainGroup { Platform, } +impl DomainGroup { + /// Number of variants. Kept in sync by `domain_group_all_lists_every_variant`. + pub const COUNT: usize = 22; + + /// Every variant, for exhaustive iteration in drift guards. + /// + /// Hand-maintained, but not hand-*trusted*: [`DomainGroup::index`] below is + /// an exhaustive `match`, so adding a variant is a compile error until it is + /// given an index, and `domain_group_all_lists_every_variant` then fails + /// until it appears here and [`COUNT`](Self::COUNT) is bumped. That chain is + /// what makes the drift guards over `tool_group`, `StoreInitPlan` and + /// `DomainSubscriberPlan` trustworthy — those three consume `DomainGroup` + /// without the compiler checking coverage. + pub const ALL: &'static [DomainGroup] = &[ + DomainGroup::Agent, + DomainGroup::Memory, + DomainGroup::Threads, + DomainGroup::Config, + DomainGroup::Security, + DomainGroup::Flows, + DomainGroup::Skills, + DomainGroup::Mcp, + DomainGroup::Meet, + DomainGroup::Channels, + DomainGroup::Web3, + DomainGroup::Voice, + DomainGroup::Media, + DomainGroup::Medulla, + DomainGroup::Inference, + DomainGroup::Integrations, + DomainGroup::Automation, + DomainGroup::Runtimes, + DomainGroup::Desktop, + DomainGroup::Hosted, + DomainGroup::Relay, + DomainGroup::Platform, + ]; + + /// Dense index of this variant. Exhaustive by construction: the compiler + /// rejects a newly added variant here, which is the first link in the chain + /// described on [`ALL`](Self::ALL). + pub const fn index(self) -> usize { + match self { + DomainGroup::Agent => 0, + DomainGroup::Memory => 1, + DomainGroup::Threads => 2, + DomainGroup::Config => 3, + DomainGroup::Security => 4, + DomainGroup::Flows => 5, + DomainGroup::Skills => 6, + DomainGroup::Mcp => 7, + DomainGroup::Meet => 8, + DomainGroup::Channels => 9, + DomainGroup::Web3 => 10, + DomainGroup::Voice => 11, + DomainGroup::Media => 12, + DomainGroup::Medulla => 13, + DomainGroup::Inference => 14, + DomainGroup::Integrations => 15, + DomainGroup::Automation => 16, + DomainGroup::Runtimes => 17, + DomainGroup::Desktop => 18, + DomainGroup::Hosted => 19, + DomainGroup::Relay => 20, + DomainGroup::Platform => 21, + } + } +} + /// A [`RegisteredController`] tagged with the [`DomainGroup`] it belongs to. /// /// The registry stores these so the live surface can be filtered by the ambient diff --git a/src/core/all_tests.rs b/src/core/all_tests.rs index 98a3e5c9ec..8fc9d13fa6 100644 --- a/src/core/all_tests.rs +++ b/src/core/all_tests.rs @@ -1418,3 +1418,155 @@ fn embedded_preset_excludes_desktop_and_hosted() { assert!(e.inference, "embedded() needs inference"); assert!(e.integrations, "embedded() needs external integrations"); } + +// ---- DomainGroup drift guards --------------------------------------------- +// `DomainGroup` has three consumers the compiler does NOT check for coverage: +// `tool_group()` (tools/ops.rs), `StoreInitPlan` and `DomainSubscriberPlan`. +// Adding a variant compiles cleanly while leaving a tool ungated or a store +// unkeyed — both of which actually happened during the realignment (#5332): +// `harness_init` stayed in Platform, and `people`'s store keyed on a different +// group than its controllers, which would have served an RPC surface with no +// store behind it. These tests close that gap. + +/// First link in the chain: `ALL` really does list every variant. +/// +/// `DomainGroup::index` is an exhaustive match, so a new variant is a compile +/// error there first; this then fails until it is added to `ALL` and `COUNT` is +/// bumped. Every guard below iterates `ALL`, so they are only as trustworthy as +/// this test. +#[test] +fn domain_group_all_lists_every_variant() { + assert_eq!( + DomainGroup::ALL.len(), + DomainGroup::COUNT, + "DomainGroup::ALL and DomainGroup::COUNT disagree — a variant was added \ + to one but not the other" + ); + let mut seen = vec![false; DomainGroup::COUNT]; + for g in DomainGroup::ALL { + let i = g.index(); + assert!( + i < DomainGroup::COUNT, + "{g:?} has index {i} but COUNT is {} — bump COUNT", + DomainGroup::COUNT + ); + assert!(!seen[i], "two variants share index {i}"); + seen[i] = true; + } + let missing: Vec = seen + .iter() + .enumerate() + .filter(|(_, s)| !**s) + .map(|(i, _)| i) + .collect(); + assert!( + missing.is_empty(), + "DomainGroup::ALL is missing the variant(s) at index {missing:?} — \ + `index()` knows about them but `ALL` does not" + ); +} + +/// Every group must be a decision in `StoreInitPlan`: either it owns a store +/// field, or it is explicitly declared store-less here. A new family that owns +/// a store but is not keyed will fail this until it is listed. +#[test] +fn every_domain_group_is_accounted_for_in_store_init_plan() { + use crate::core::runtime::context::StoreInitPlan; + + // Groups that own a store field in StoreInitPlan. + const OWNS_STORE: &[DomainGroup] = + &[DomainGroup::Memory, DomainGroup::Agent, DomainGroup::Skills]; + // Groups with no store of their own. Adding a variant forces a choice + // between these two lists — that is the point. + const STORELESS: &[DomainGroup] = &[ + DomainGroup::Threads, + DomainGroup::Config, + DomainGroup::Security, + DomainGroup::Flows, + DomainGroup::Mcp, + DomainGroup::Meet, + DomainGroup::Channels, + DomainGroup::Web3, + DomainGroup::Voice, + DomainGroup::Media, + DomainGroup::Medulla, + DomainGroup::Inference, + DomainGroup::Integrations, + DomainGroup::Automation, + DomainGroup::Runtimes, + DomainGroup::Desktop, + DomainGroup::Hosted, + DomainGroup::Relay, + DomainGroup::Platform, + ]; + + for g in DomainGroup::ALL { + let owns = OWNS_STORE.contains(g); + let storeless = STORELESS.contains(g); + assert!( + owns ^ storeless, + "{g:?} is in neither (or both) of OWNS_STORE / STORELESS — decide \ + whether it needs a StoreInitPlan field and list it in exactly one" + ); + } + + // And the owning groups actually gate their field: turning the group off + // must turn the store off. + let mut only_memory = crate::core::runtime::DomainSet::none(); + only_memory.memory = true; + let plan = StoreInitPlan::for_domains(only_memory); + assert!(plan.memory, "Memory on ⇒ memory store initialized"); + assert!( + plan.people, + "Memory on ⇒ people store initialized (people lives under memory/)" + ); + assert!(!plan.agent_attachments, "Agent off ⇒ attachments store off"); + assert!(!plan.skills_prune, "Skills off ⇒ skills prune off"); +} + +/// Same contract for `DomainSubscriberPlan`: every group either registers +/// subscribers or is declared subscriber-less. +#[test] +fn every_domain_group_is_accounted_for_in_subscriber_plan() { + use crate::core::jsonrpc::DomainSubscriberPlan; + + const REGISTERS: &[DomainGroup] = &[ + DomainGroup::Platform, + DomainGroup::Channels, + DomainGroup::Flows, + DomainGroup::Memory, + DomainGroup::Meet, + DomainGroup::Agent, + DomainGroup::Mcp, + DomainGroup::Integrations, + DomainGroup::Security, + DomainGroup::Desktop, + DomainGroup::Skills, + ]; + const NO_SUBSCRIBERS: &[DomainGroup] = &[ + DomainGroup::Threads, + DomainGroup::Config, + DomainGroup::Web3, + DomainGroup::Voice, + DomainGroup::Media, + DomainGroup::Medulla, + DomainGroup::Inference, + DomainGroup::Automation, + DomainGroup::Runtimes, + DomainGroup::Hosted, + DomainGroup::Relay, + ]; + + for g in DomainGroup::ALL { + assert!( + REGISTERS.contains(g) ^ NO_SUBSCRIBERS.contains(g), + "{g:?} is in neither (or both) of REGISTERS / NO_SUBSCRIBERS — decide \ + whether it registers event-bus subscribers and list it in exactly one" + ); + } + + // full() must enable every registering group; none() must enable none. + let full = DomainSubscriberPlan::for_domains(crate::core::runtime::DomainSet::full()); + let none = DomainSubscriberPlan::for_domains(crate::core::runtime::DomainSet::none()); + assert_ne!(full, none, "full() and none() must differ"); +} diff --git a/src/openhuman/tools/ops.rs b/src/openhuman/tools/ops.rs index b42f4adaf9..4545445255 100644 --- a/src/openhuman/tools/ops.rs +++ b/src/openhuman/tools/ops.rs @@ -1503,8 +1503,11 @@ fn tool_group(name: &str) -> crate::core::all::DomainGroup { if name == "node_exec" || name == "npm_exec" || name == "python_exec" { return DomainGroup::Runtimes; } - // Inference: the token-compression retrieval surface. - if name.starts_with("tokenjuice_") { + // Inference: the CCR retrieval surface. Matched against the crate's own + // constant list rather than a name prefix — the live tool is + // `tinyjuice_retrieve`, and `tokenjuice_retrieve` / `retrieve_tool_output` + // are migration aliases, so a prefix rule silently missed the real one. + if crate::openhuman::inference::tokenjuice::RECOVERY_TOOL_NAMES.contains(&name) { return DomainGroup::Inference; } // Everything else — shell/file and other kernel utilities — is Platform: diff --git a/src/openhuman/tools/ops_tests.rs b/src/openhuman/tools/ops_tests.rs index df5ec36e2e..d7a5194778 100644 --- a/src/openhuman/tools/ops_tests.rs +++ b/src/openhuman/tools/ops_tests.rs @@ -2701,3 +2701,79 @@ fn default_tools_omits_flows_tools_when_feature_off() { ); } } + +// ---- tool_group() drift guard ---------------------------------------------- + +/// Every `DomainGroup` must be a deliberate decision in [`tool_group`]: either a +/// representative tool name maps to it, or it is declared tool-less. +/// +/// This is the guard that would have caught the #4808 leak by construction, and +/// it caught a live one on the way in: the `Inference` rule matched +/// `tokenjuice_` while the real tool is `tinyjuice_retrieve`, so CCR retrieval +/// was falling through to `Platform`. +/// +/// The failure it prevents is silent. A family whose tools have no `tool_group` +/// rule lands in `Platform`, so those tools stay in the list under a +/// `DomainSet { platform: true, : false }` — advertised to the model as +/// callable while the rest of the family is gated off — and conversely vanish +/// under `harness()`, which has `platform: false`. +/// +/// Deliberately tests the FUNCTION, not a built registry: which tools a registry +/// contains depends on config flags, security tier and enabled integrations, so +/// a registry-derived assertion passes or fails for reasons unrelated to group +/// mapping. `REPRESENTATIVE` names are asserted to be real tool names by +/// `representative_tool_names_are_real` below, so this cannot rot into testing +/// strings that no longer exist. +#[test] +fn every_domain_group_is_accounted_for_in_tool_group() { + use crate::core::all::DomainGroup; + + for g in DomainGroup::ALL { + let representative = REPRESENTATIVE.iter().find(|(_, group)| group == g); + let toolless = TOOL_LESS.contains(g); + assert!( + representative.is_some() ^ toolless, + "{g:?} is in neither (or both) of REPRESENTATIVE / TOOL_LESS — decide \ + whether the family owns agent tools and list it in exactly one" + ); + if let Some((name, want)) = representative { + assert_eq!( + tool_group(name), + *want, + "`{name}` must map to {want:?}; if it now maps elsewhere the \ + `tool_group` rule for this family has drifted" + ); + } + } +} + +/// One real tool name per family that owns tools. +const REPRESENTATIVE: &[(&str, crate::core::all::DomainGroup)] = { + use crate::core::all::DomainGroup as G; + &[ + ("delegate", G::Agent), + ("memory_search", G::Memory), + ("thread_list", G::Threads), + ("mcp_list_servers", G::Mcp), + ("wallet_get_address", G::Web3), + ("media_generate_image", G::Media), + ("whatsapp_data_list_chats", G::Channels), + ("audio_generate_podcast", G::Voice), + ("create_workflow", G::Flows), + ("run_workflow", G::Skills), + ("cron_add", G::Automation), + ("composio_execute", G::Integrations), + ("billing_top_up_credits", G::Hosted), + ("tinyplace_call", G::Relay), + ("dashboard_model_health", G::Desktop), + ("node_exec", G::Runtimes), + ("tinyjuice_retrieve", G::Inference), + ("shell", G::Platform), + ] +}; + +/// Families with no agent tools of their own. +const TOOL_LESS: &[crate::core::all::DomainGroup] = { + use crate::core::all::DomainGroup as G; + &[G::Config, G::Security, G::Meet, G::Medulla] +};