feat(core): runtime DomainSet composition axis — group-tagged registry + ambient filter (#4796)#4808
Conversation
…filter (tinyhumansai#4796) Add a runtime DomainSet selector on CoreBuilder (sibling of ServiceSet) plus a DomainGroup family enum. Every controller is tagged with its group once at the single registration site via a GroupedController wrapper; the live surface (controllers/schema/dispatch, workspace stores, event subscribers) is filtered by the ambient CoreContext::domains(). full() is byte-identical to before; harness() keeps only agent/memory/threads/config/security. Gated methods dispatch as unknown (None); gated stores/subscribers are not initialized. These files form one atomic seam — the init/bootstrap signature changes cycle across all.rs, builder.rs, context.rs and jsonrpc.rs and only compile together.
…i#4796) Post-filter all_tools_with_runtime by the ambient CoreContext DomainSet: tools whose DomainGroup is disabled are dropped. Adds tool_group(name) classifying each tool by its name() into its family (web3/mcp/skills/flows/media/voice/meet gated; memory/threads mapped as harness-kept; everything else Platform). No ambient context or full() keeps every tool.
…i#4796) Build the headless embedder with the harness domain set so the example exercises the runtime seam. core.version + openhuman.ping (a built-in alias) stay valid regardless of the DomainSet.
…4796) full_registration_is_byte_identical, harness_excludes_gated_namespaces, dispatch_returns_none_for_gated_method, group_mapping_smoke (all_tests.rs); tool_group classifier + harness/full retention (ops_tests.rs). Adapt the validate_registry unit tests to the GroupedController signature via a grouped() helper.
AGENTS.md: ServiceSet + DomainSet runtime composition note. phase-2 plan: 2.d DomainSet section (Shape B filter seam; DomainRegistration struct collapse remains deferred) + verification entries.
…nsai#4796) Extract each workspace-store's DomainGroup guard into a pure StoreInitPlan that init_stores consumes, giving the store->group mapping a single source of truth. Assert full/none/harness plans directly — no process-global store state, no runtime boot. Closes the DoD item-3 store-gating test gap.
…nyhumansai#4796) Extract each event-bus subscriber's DomainGroup guard into a pure DomainSubscriberPlan that register_domain_subscribers consumes, giving the subscriber->group mapping a single source of truth. Assert full/none/harness plans directly — no real subscribers, no process-global bus mutation. Closes the DoD item-3 subscriber-gating test gap.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds ChangesDomainSet runtime composition
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CoreBuilder
participant CoreContext
participant ControllerRegistry
participant ToolRegistry
participant RuntimeBootstrap
CoreBuilder->>CoreContext: configure DomainSet
CoreContext->>ControllerRegistry: expose allowed controllers and schemas
CoreContext->>ToolRegistry: filter tools by DomainGroup
CoreContext->>RuntimeBootstrap: initialize selected stores and subscribers
Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
coderabbitai
Bot
added
feature
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70ca2c32ae
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| registry() | ||
| .iter() | ||
| .chain(internal_registry().iter()) | ||
| .find(|r| r.rpc_method_name() == method) | ||
| .map(|r| r.schema.clone()) | ||
| .find(|g| g.controller.rpc_method_name() == method) | ||
| .map(|g| g.controller.schema.clone()) |
There was a problem hiding this comment.
When an ambient DomainSet::harness()/none() is active, invoke_method_inner still uses this unfiltered schema lookup before try_invoke_registered_rpc enforces the domain gate. A disabled method such as openhuman.flows_* with missing or extra parameters will return the controller's validation error instead of method-not-found, which violates the documented contract that gated controllers are indistinguishable from absent and leaks the hidden RPC surface. Filter this lookup by group_allowed or skip validation for gated groups.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 7230922 — schema_for_rpc_method now filters by group_allowed exactly as try_invoke_registered_rpc does, so a gated method returns method-not-found at both validation and dispatch (no param-error leak). Proven by schema_lookup_is_gated_in_lockstep_with_dispatch + a transport-level test through invoke_method.
| // Everything else — shell/file/screen/config/security/agent/billing/… — is | ||
| // Platform: present under full(), absent under harness()/none(). | ||
| DomainGroup::Platform |
There was a problem hiding this comment.
For custom DomainSets that disable a gate family while leaving platform enabled, any existing tool not matched above falls through here as Platform. Current gate-domain tools such as audio_generate_podcast/audio_email_podcast (Voice) and whatsapp_data_* (Channels) are not classified, so agents can still call them even though the corresponding controllers/schema are gated off. Add classifications for the existing non-Platform tool names before this fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9f8f558 — added an explicit VOICE list (audio_generate_podcast / audio_email_podcast / audio_generate_and_email_podcast) and a whatsapp_data_ → Channels prefix, so these gate-family tools no longer fall through to Platform. Tests updated.
| static REGISTERED: Once = Once::new(); | ||
| REGISTERED.call_once(|| { |
There was a problem hiding this comment.
Because this registration is guarded by a single process-wide Once, the first runtime to call it fixes the subscriber set for the entire process. If an embedder or test builds DomainSet::harness()/none() first and later widens to DomainSet::full(), the channels/flows/MCP/platform subscribers skipped on the first call will never be installed, even though those domains' controllers are now exposed. Make subscriber registration idempotent per group or allow later calls to add newly enabled subscribers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in b40a06f — replaced the single process-wide Once with a per-DomainGroup registered-set: ungated infra runs once, and each gated group installs the first time it is enabled, so widening harness()→full() later installs the newly-enabled subscribers. DomainGroup now derives Hash for the set.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/jsonrpc.rs (2)
2082-2084: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the completion diagnostic reflect the actual registration plan.
The message claims webhook, channel, agent, and MCP subscribers were registered even when their branches were skipped.
Proposed diagnostic fix
log::info!( - "[event_bus] domain subscribers registered (webhook, channel, health, conversation, composio, restart, proactive, agent, session_expired, mcp_client)" + "[event_bus] domain subscriber registration complete: plan={plan:?}" );As per coding guidelines, new or changed flows require grep-friendly diagnostics for branches, state transitions, and exit outcomes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/jsonrpc.rs` around lines 2082 - 2084, Update the completion diagnostic in the domain subscriber registration flow near the existing log::info call to reflect only subscribers actually registered, rather than listing all possible subscriber types unconditionally. Track or construct the registered subscriber names as each conditional branch succeeds, then emit that collection in the final diagnostic while preserving grep-friendly branch and completion logging.Source: Coding guidelines
2096-2121: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply
DomainSetto domain initialization outside the subscriber registrar.The new argument is only used by
register_domain_subscribers. Lines 2187 and 2247 still initialize the Web3 x402 ledger and Skills triggered-workflow subscriber underharness()andnone(). Gate both by their owningDomainGroupand log skipped branches.This conflicts with the PR objective that excluded stores and subscribers remain uninitialized.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/jsonrpc.rs` around lines 2096 - 2121, Update bootstrap_core_runtime so DomainSet controls domain-owned initialization outside register_domain_subscribers. In the harness() path, initialize the Web3 x402 ledger only when its owning DomainGroup is enabled; in the none() path, initialize the Skills triggered-workflow subscriber only when its owning DomainGroup is enabled. Log each skipped initialization branch, ensuring excluded stores and subscribers remain uninitialized.
🧹 Nitpick comments (1)
src/core/all.rs (1)
1219-1235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd complete, grep-friendly diagnostics to both DomainSet gating flows.
src/core/all.rs#L1219-L1235: log lookup misses, allowed dispatch, and completion/error in addition to suppression.src/openhuman/tools/ops.rs#L1054-L1075: log the selected DomainSet, bypass branch, and before/after counts.As per coding guidelines, “New or changed flows must include verbose, grep-friendly diagnostics for entry/exit, branches, external calls, retries/timeouts, state transitions, and errors.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/all.rs` around lines 1219 - 1235, Expand diagnostics for both DomainSet gating flows: in src/core/all.rs lines 1219-1235, update the method lookup/dispatch flow around registry().iter(), internal_registry(), and group_allowed to log lookup misses, allowed dispatch, completion, and errors in addition to suppression; in src/openhuman/tools/ops.rs lines 1054-1075, update the corresponding DomainSet handling to log the selected DomainSet, bypass branch, and before/after counts. Use consistent verbose, grep-friendly messages for each branch and state transition.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/all_tests.rs`:
- Around line 660-684: Replace the self-referential registry comparison in
full_registration_is_byte_identical with an independently checked-in, ordered
pre-change RPC method snapshot, and assert the unfiltered
all_registered_controllers() output matches it in order and length. In
docs/plans/pluggable-core/phase-2-corecontext.md at line 153, mark the
verification complete only after this independent assertion is implemented.
- Around line 735-764: Extend dispatch_returns_none_for_gated_method to exercise
the JSON-RPC handler rather than only try_invoke_registered_rpc: send both valid
and malformed-parameter requests for the gated flows method through the
transport-level handler, and assert each returns the unknown-method error.
Retain the existing direct-dispatch and security.policy_info checks as needed,
ensuring the test proves gated methods are rejected after JSON-RPC schema
validation as well.
In `@src/core/runtime/builder.rs`:
- Around line 120-123: Update the documentation describing DomainSet::none to
clarify that it disables all optional domain families, not the entire runtime;
explicitly note that transport built-ins and always-on core infrastructure
remain active. Apply the same clarification to the corresponding repeated
documentation block.
In `@src/openhuman/tools/ops.rs`:
- Around line 1079-1195: Replace name-based classification in tool_group with
authoritative explicit DomainGroup metadata on every tool, ensuring Agent,
Config, Security, and Channels tools are correctly grouped without
disabled-family leakage. Update AGENTS.md at line 217 and
docs/plans/pluggable-core/phase-2-corecontext.md lines 135-140 to describe the
authoritative grouping mechanism. In src/core/all.rs lines 682-689, assign
TokenJuice to an ungated/core group; in src/core/all_tests.rs lines 687-733,
assert it remains visible under harness(); and in
src/openhuman/tools/ops_tests.rs lines 2197-2228, add coverage for Agent,
Config, Security, Channels, and TokenJuice classification.
---
Outside diff comments:
In `@src/core/jsonrpc.rs`:
- Around line 2082-2084: Update the completion diagnostic in the domain
subscriber registration flow near the existing log::info call to reflect only
subscribers actually registered, rather than listing all possible subscriber
types unconditionally. Track or construct the registered subscriber names as
each conditional branch succeeds, then emit that collection in the final
diagnostic while preserving grep-friendly branch and completion logging.
- Around line 2096-2121: Update bootstrap_core_runtime so DomainSet controls
domain-owned initialization outside register_domain_subscribers. In the
harness() path, initialize the Web3 x402 ledger only when its owning DomainGroup
is enabled; in the none() path, initialize the Skills triggered-workflow
subscriber only when its owning DomainGroup is enabled. Log each skipped
initialization branch, ensuring excluded stores and subscribers remain
uninitialized.
---
Nitpick comments:
In `@src/core/all.rs`:
- Around line 1219-1235: Expand diagnostics for both DomainSet gating flows: in
src/core/all.rs lines 1219-1235, update the method lookup/dispatch flow around
registry().iter(), internal_registry(), and group_allowed to log lookup misses,
allowed dispatch, completion, and errors in addition to suppression; in
src/openhuman/tools/ops.rs lines 1054-1075, update the corresponding DomainSet
handling to log the selected DomainSet, bypass branch, and before/after counts.
Use consistent verbose, grep-friendly messages for each branch and state
transition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 075e5cb0-6730-4fc1-9fec-84e18faa50e6
📒 Files selected for processing (13)
AGENTS.mddocs/plans/pluggable-core/phase-2-corecontext.mdexamples/embed_headless.rssrc/core/all.rssrc/core/all_tests.rssrc/core/jsonrpc.rssrc/core/jsonrpc_tests.rssrc/core/runtime/builder.rssrc/core/runtime/context.rssrc/core/runtime/mod.rssrc/lib.rssrc/openhuman/tools/ops.rssrc/openhuman/tools/ops_tests.rs
…humansai#4796) Codex review (tinyhumansai#4808): invoke_method_inner validated params against the FULL registry before the dispatch gate fired, so a gated method (e.g. openhuman.flows_*) with bad params returned the controller's validation error instead of method-not-found — leaking the hidden RPC surface and violating the 'gated == absent' contract. Filter schema_for_rpc_method by group_allowed exactly as try_invoke_registered_rpc does. Also strengthen the byte-identical test to assert full() registration ORDER (not just membership) per CodeRabbit, and derive Hash on DomainGroup for the subscriber-registry set.
…nyhumansai#4796) Codex review (tinyhumansai#4808): the tool_group classifier mapped Voice via voice_/tts_/ stt_ prefixes and had no Channels arm, so audio_generate_podcast / audio_email_podcast / audio_generate_and_email_podcast (Voice) and whatsapp_data_* (Channels) fell through to Platform — leaving them callable under a DomainSet that gates those families off while keeping Platform. Add an explicit VOICE list + a whatsapp_data_ Channels prefix.
…inyhumansai#4796) Codex review (tinyhumansai#4808): the single process-wide Once in register_domain_subscribers fixed the subscriber set to the FIRST caller's DomainSet — widening harness()-> full() later never installed the skipped subscribers. Split ungated infra into its own Once and track registered DomainGroups so a wider DomainSet installs exactly the newly-enabled groups. Fix the completion log to report the actual plan (CodeRabbit). Also gate the x402 payment ledger (Web3) and the triggered-workflow subscriber (Skills) in bootstrap_core_runtime — both still initialized under harness()/none() (CodeRabbit).
…inyhumansai#4796) CodeRabbit review (tinyhumansai#4808): 'nothing' / 'disables everything' misstated the contract — transport built-ins and always-on core infra still run under none().
tinyhumansai#4796) CodeRabbit review (tinyhumansai#4808): the direct-dispatch + schema-lookup tests don't exercise invoke_method (the transport path where validate-before-gate leaked). Send a gated flows method with both well-formed AND malformed params through invoke_method under harness() and assert unknown-method (never a param error).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/all.rs (1)
1213-1241: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard RPC-name uniqueness across both registries.
validate_registry()only checks thebuild_registered_controllers()slice;internal_registry()is never included in the duplicate-RPC check. Since bothschema_for_rpc_method()andtry_invoke_registered_rpc()search the combinedregistry()+internal_registry()set, an overlapping RPC name could still split schema lookup from dispatch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/all.rs` around lines 1213 - 1241, Update validate_registry() to include entries from internal_registry() when checking RPC-name uniqueness, matching the combined lookup used by schema_for_rpc_method() and try_invoke_registered_rpc(). Reject duplicate rpc_method_name() values across both registries while preserving the existing validation behavior for unique names.
🧹 Nitpick comments (1)
src/core/all.rs (1)
1063-1077: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a grep-friendly diagnostic for the gate branch.
try_invoke_registered_rpclogs (log::debug!) when it suppresses a call due togroup_allowedreturningfalse, butschema_for_rpc_method's identical gate has no logging at all — despite its own comment stating it filters "identically totry_invoke_registered_rpc". This makes it harder to grep logs for schema-lookup suppressions specifically (e.g. to debug a hidden-method report).🔍 Proposed diagnostic
pub fn schema_for_rpc_method(method: &str) -> Option<ControllerSchema> { registry() .iter() .chain(internal_registry().iter()) - .find(|g| g.controller.rpc_method_name() == method && group_allowed(g.group)) - .map(|g| g.controller.schema.clone()) + .find(|g| g.controller.rpc_method_name() == method) + .filter(|g| { + let allowed = group_allowed(g.group); + if !allowed { + log::debug!( + "[rpc][domain-gate] schema lookup for '{method}' suppressed — group {:?} disabled under active DomainSet", + g.group + ); + } + allowed + }) + .map(|g| g.controller.schema.clone()) }As per path instructions, "New or changed flows must include verbose, grep-friendly diagnostics for entry/exit, branches, external calls, retries/timeouts, state transitions, and errors" and "Use
logortracingatdebugortracelevels for Rust diagnostics."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/all.rs` around lines 1063 - 1077, Update schema_for_rpc_method to add a log::debug diagnostic when a matching method is rejected because group_allowed returns false, using a grep-friendly message that clearly identifies schema lookup suppression and the RPC method. Keep allowed matches and genuinely unregistered methods on their existing behavior.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/core/all.rs`:
- Around line 1213-1241: Update validate_registry() to include entries from
internal_registry() when checking RPC-name uniqueness, matching the combined
lookup used by schema_for_rpc_method() and try_invoke_registered_rpc(). Reject
duplicate rpc_method_name() values across both registries while preserving the
existing validation behavior for unique names.
---
Nitpick comments:
In `@src/core/all.rs`:
- Around line 1063-1077: Update schema_for_rpc_method to add a log::debug
diagnostic when a matching method is rejected because group_allowed returns
false, using a grep-friendly message that clearly identifies schema lookup
suppression and the RPC method. Keep allowed matches and genuinely unregistered
methods on their existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8f58f27-0ddc-41a8-8b61-0c40cb61d092
📒 Files selected for processing (7)
src/core/all.rssrc/core/all_tests.rssrc/core/jsonrpc.rssrc/core/jsonrpc_tests.rssrc/core/runtime/builder.rssrc/openhuman/tools/ops.rssrc/openhuman/tools/ops_tests.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/openhuman/tools/ops_tests.rs
- src/core/runtime/builder.rs
- src/core/jsonrpc.rs
- src/openhuman/tools/ops.rs
- src/core/all_tests.rs
…ansai#4764 Pre-existing main breakage surfaced by this PR's coverage run (unrelated to the DomainSet seam): tinyhumansai#4764 (383581b) gave the orchestrator direct memory_recall/ memory_store tools, but agent_retrieval_e2e still asserted memory_recall must be absent, so orchestrator_reaches_memory_agent_on_demand failed on main. Remove memory_recall from the superseded-tools list; the deep tree-walk tools stay delegated behind agent_memory.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/agent_retrieval_e2e.rs (1)
189-215: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that the retained memory tools are present.
Removing
memory_recallfrom the negative list prevents a false failure, but this test can now pass even ifmemory_recallandmemory_storeare accidentally absent. Add positive assertions for the retained first-class tools so the compatibility contract is covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/agent_retrieval_e2e.rs` around lines 189 - 215, Extend the test around the superseded-tool loop to positively assert that the orchestrator configuration contains both retained first-class tools, “memory_recall” and “memory_store”. Reuse the existing TOML line-matching approach and fail with a clear assertion if either tool is absent, while preserving the current negative assertions for superseded tools.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/agent_retrieval_e2e.rs`:
- Around line 189-215: Extend the test around the superseded-tool loop to
positively assert that the orchestrator configuration contains both retained
first-class tools, “memory_recall” and “memory_store”. Reuse the existing TOML
line-matching approach and fail with a clear assertion if either tool is absent,
while preserving the current negative assertions for superseded tools.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e699fc22-5af2-4716-9481-5b45afe6ee55
📒 Files selected for processing (1)
tests/agent_retrieval_e2e.rs
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
PR #4808 — feat(core): runtime DomainSet composition axis
Walkthrough
Adds DomainSet as a second runtime-composition axis on CoreBuilder (sibling to ServiceSet) that selects which domain families are live. Every controller is tagged with a DomainGroup once, at the single registration site (build_registered_controllers / build_internal_only_controllers) via a GroupedController wrapper, and five surfaces — /schema, dispatch, agent tools, workspace stores, and event-bus subscribers — filter by the ambient CoreContext::domains(). The default full() preset is a byte-identical no-op for desktop/CLI; harness() and none() are the slim opt-ins.
Overall this is a clean, well-tested seam. The design is sound: the ambient CoreContext::current() (task-local → DEFAULT_CONTEXT fallback) means the filter Just Works for both scoped multi-tenant dispatch and the headless boot path, and the gate is enforced at both schema-lookup and dispatch so a gated method can never leak a param-validation error (verified by gated_method_is_unknown_at_transport_even_with_malformed_params). All four CodeRabbit findings and the three Codex findings were addressed thoughtfully. My independent pass found no blockers — a couple of doc/consistency nits and one design question about the harness tool surface. Leaving this as a comment; approval is the maintainer's call.
Changes
| File | Summary |
|---|---|
AGENTS.md |
Documents the ServiceSet + DomainSet two-axis runtime composition. |
docs/plans/pluggable-core/phase-2-corecontext.md |
Adds §2.d describing the filter-seam mechanism + verification tests. |
examples/embed_headless.rs |
Switches the embed example to DomainSet::harness(). |
src/core/all.rs |
Adds DomainGroup, GroupedController, push, group_allowed; tags every controller; filters all_registered_controllers/all_controller_schemas/schema_for_rpc_method/try_invoke_registered_rpc. |
src/core/all_tests.rs |
Byte-identical-full, harness-exclusion, dispatch-gate, schema-lockstep, group-mapping tests. |
src/core/jsonrpc.rs |
DomainSubscriberPlan; per-DomainGroup idempotent subscriber registration; gates x402 ledger + skills subscriber. |
src/core/jsonrpc_tests.rs |
Subscriber-plan tests + transport-level malformed-param gate test. |
src/core/runtime/builder.rs |
DomainSet struct + full/harness/none presets + allows; threads .domains() through CoreBuilder. |
src/core/runtime/context.rs |
Stores domains on CoreContext; StoreInitPlan; gates init_stores; for_test ctor. |
src/core/runtime/mod.rs, src/lib.rs |
Re-export DomainSet. |
src/openhuman/tools/ops.rs |
tool_group name classifier + ambient post-filter in all_tools_with_runtime. |
src/openhuman/tools/ops_tests.rs |
tool_group classification + harness-drop tests. |
tests/agent_retrieval_e2e.rs |
Drops stale memory_recall negative assertion (unrelated #4764 fix, called out in PR body). |
Actionable comments (3)
💡 Refactor / suggestion
1. src/core/jsonrpc.rs:1805-1817 — Orphaned doc comment now mis-documents DomainSubscriberPlan
The two leading lines ("Registers all long-lived domain event-bus subscribers exactly once. / Guarded by std::sync::Once so repeated calls to bootstrap_core_runtime are safe and idempotent.") were the original doc for register_domain_subscribers. Because the new DomainSubscriberPlan struct doc was inserted directly after them without a blank line, they are now a single contiguous /// block attached to DomainSubscriberPlan — a pure decision struct that registers nothing and is not Once-guarded. The Once claim is also stale: the single Once was replaced by INFRA: Once + the per-group group_first_time set.
Suggested change:
// move these two lines down onto register_domain_subscribers, and correct the guard wording:
/// Registers all long-lived domain event-bus subscribers, each group at most
/// once per process. Ungated core infra runs once behind `INFRA: Once`; each
/// gated `DomainGroup` installs the first time it is enabled (`group_first_time`),
/// so widening `harness()` → `full()` later still installs the newly-enabled groups.
fn register_domain_subscribers(...) { ... }
/// Per-`DomainGroup` gating decision for each event-bus subscriber ...
pub struct DomainSubscriberPlan { ... }❓ Question
2. src/openhuman/tools/ops.rs:1091-1213 — harness() keeps Agent/Config/Security controllers live but drops their agent tools
Controller gating and tool gating use different groupings for the same domain. Under harness() the Agent/Config/Security controllers dispatch fine over RPC, but the corresponding agent tools (spawn_subagent, config/security tools, all agent-orchestration tools) fall through tool_group to Platform and are dropped. So the "embeddable agent core" agent surface is effectively memory + threads/todos only — it can't spawn sub-agents or read config even though those RPCs exist. That's a defensible strict-minimal choice for a seam PR (and you documented it), but it's worth confirming it's intended, since the doc string ("embeddable agent core") reads as more capable than the resulting tool surface. Consider a follow-up tracking issue for the per-Tool DomainGroup metadata so the two axes converge.
There's also no guard test that the tool classifier stays in sync with reality: a newly-added Web3/Voice/Meet tool that doesn't match a listed name/prefix silently becomes Platform, which would leak it under a custom DomainSet{ platform: true, web3: false }. A cheap regression guard:
#[test]
fn no_gate_family_tool_silently_defaults_to_platform() {
// Every tool whose name starts with a known gate prefix must NOT map to Platform.
for name in ["wallet_x", "web3_x", "x402_x", "mcp_x", "media_x"] {
assert_ne!(tool_group(name), DomainGroup::Platform, "{name} must not default to Platform");
}
}💡 Refactor / suggestion
3. src/core/runtime/builder.rs:157 / src/core/all.rs — DomainGroup::Media has zero tagged controllers
No controller in build_registered_controllers is tagged Media (grep confirms no media namespace); Media is referenced only by the media_generate_* tools in tool_group. So DomainSet.media gates tools but no controller/store/subscriber. That's harmless today but is a latent asymmetry (a custom set with media:false, platform:true drops the media tools while their backing controllers stay live). Worth a one-line comment on the media field noting it currently gates only the tool surface, or fold the media generation controller (when it lands) into the group.
Nitpicks (2)
src/core/jsonrpc.rs:2137-2138— thebootstrap_core_runtimecomment "Uses a Once guard so repeated calls … cannot double-subscribe" is now technically served by the per-group set rather than a function-levelOnce; the guarantee holds but the wording predates the refactor.src/core/all.rs:1058(rpc_method_from_parts) stays intentionally unfiltered (documented). It backscli.rs:436, so underharness()the local CLI can still resolve/print help for a gated method's schema — a mild surface leak, but CLI is a trusted local operator surface and this is out of scope for the seam. Fine to leave; flagging for awareness.
Verified / looks good
- Ambient filter is correct on both paths:
CoreContext::current()falls back toDEFAULT_CONTEXT, so/schema+ dispatch outside an explicit scope still filter by the runtime'sDomainSet(harness embeds) while desktopfull()is a true no-op. - Gate enforced at both
schema_for_rpc_methodandtry_invoke_registered_rpc, so a gated method with malformed params returns unknown-method, never a param-validation leak — proven at the transport layer. - Per-group
group_first_timecorrectly supports widening (harness()→full()installs newly-enabled groups; already-registered groups are not double-subscribed). full_registration_is_byte_identicalasserts order + membership against the raw registry (post sort-removal) — the right property for the filter-identity claim.StoreInitPlan/DomainSubscriberPlanextracted as pure values with direct unit tests — no process-global mutation needed to verify gating. Good pattern.- Convention-compliant: no new root-level
.rs, single registration site preserved,mod.rsuntouched,DomainSet/DomainGrouplive in the transport/registry layer, AGENTS.md + plan docs updated.
| @@ -1806,81 +1806,105 @@ async fn run_server_with_services( | |||
| /// | |||
| /// Guarded by `std::sync::Once` so repeated calls to `bootstrap_core_runtime` | |||
There was a problem hiding this comment.
Orphaned doc comment (minor). These two lines — "Registers all long-lived domain event-bus subscribers exactly once" / "Guarded by std::sync::Once…" — were the doc for register_domain_subscribers, but with no blank line before the new struct doc they now form one contiguous /// block attached to DomainSubscriberPlan, which registers nothing and isn't Once-guarded. The Once claim is also stale (replaced by INFRA: Once + the per-group group_first_time set). Move these two lines down onto register_domain_subscribers and correct the guard wording to describe the per-group idempotency.
There was a problem hiding this comment.
Addressed in 32de700 — moved the "registers subscribers … idempotent" doc back onto register_domain_subscribers and rewrote it for the real mechanism (ungated infra once behind INFRA: Once; each DomainGroup installs the first time it's enabled via group_first_time). Also fixed the stale Once wording at the bootstrap_core_runtime call site (nitpick 1).
| /// only memory + thread/todo tools remain. This is the strict #4796 harness | ||
| /// surface; an embedder that wants a broader tool set can widen its DomainSet. | ||
| /// (Names verified against each Tool impl's `fn name()` on 2026-07-13.) | ||
| fn tool_group(name: &str) -> crate::core::all::DomainGroup { |
There was a problem hiding this comment.
Harness keeps Agent/Config/Security controllers but drops their tools (question + guard-test suggestion). Controller gating and tool gating use different groupings here: under harness() the Agent/Config/Security controllers dispatch over RPC, but spawn_subagent, config/security, and all agent-orchestration tools fall through to Platform and are dropped — so the agent tool surface is effectively memory + threads only. Defensible as a strict minimal seam (and documented), but the "embeddable agent core" wording oversells the resulting agent capability; worth confirming intent + a follow-up for per-Tool DomainGroup metadata so the axes converge.\n\nSeparately, the name-based classifier has no sync guard: a future gate-family tool that matches no listed name/prefix silently becomes Platform and would leak under a custom DomainSet{ platform:true, web3:false }. Cheap regression:\n\nrust\n#[test]\nfn no_gate_family_tool_silently_defaults_to_platform() {\n for name in ["wallet_x","web3_x","x402_x","mcp_x","media_x"] {\n assert_ne!(tool_group(name), DomainGroup::Platform);\n }\n}\n
There was a problem hiding this comment.
Addressed in d06e24b. Guard test no_gate_family_tool_silently_defaults_to_platform added; and to actually close the leak I switched the prefix-owning gate families to prefix matching (Web3 = wallet_/web3_/x402_, Media = media_) so a new family tool auto-gates instead of exact-list-missing → Platform.
On the design question: the strict-minimal harness tool surface (memory + threads only; Agent/Config/Security tools drop to Platform) is intentional for this seam PR and documented. You're right the two axes should converge — opened #4821 to give every Tool authoritative DomainGroup metadata so tool gating matches controller gating (and the name heuristic + this guard can then be removed).
| /// Speech-to-text / text-to-speech, audio toolkit. | ||
| pub voice: bool, | ||
| /// Image/video media generation. | ||
| pub media: bool, |
There was a problem hiding this comment.
DomainGroup::Media gates only tools, no controllers (nit). No controller in build_registered_controllers is tagged Media (no media namespace exists); Media is referenced only by the media_generate_* tools in tool_group. So DomainSet.media gates the tool surface but no controller/store/subscriber — a custom set with media:false, platform:true would drop the media tools while their backing controllers stay live. Harmless today; worth a one-line comment on this field noting it currently gates only the tool surface.
There was a problem hiding this comment.
Review summary — DomainSet composition seamReviewed the full diff (registry/filter design, correctness, edge cases, security, repo conventions) and re-read the existing CodeRabbit + Codex threads. Left as a COMMENT review — approval is the maintainer's call. Verdict: looks-good / non-blocking. No blockers, no majors. Verified correct (checked against code, not just the reply threads)
Non-blocking items (3 inline comments)
One question for the maintainerUnder Note: this PR also bundles one unrelated pre-existing test fix ( |
…ult (tinyhumansai#4796) Maintainer review (tinyhumansai#4808): the name-based tool classifier used exact-name lists for Web3/Media, so a future tool in those families (no matching name) would silently fall through to Platform and leak under a custom DomainSet. Switch Web3 (wallet_/web3_/x402_) and Media (media_) to domain-exclusive prefixes, and add no_gate_family_tool_silently_defaults_to_platform as a regression guard. Axis-convergence via per-Tool metadata tracked in tinyhumansai#4821.
…inyhumansai#4796) Maintainer review (tinyhumansai#4808): (1) the original register_domain_subscribers doc ("registered exactly once / Once-guarded") had orphaned onto DomainSubscriberPlan with a stale Once claim — move it back to the fn and describe the per-group idempotency (INFRA: Once + group_first_time). (2) Update the bootstrap call-site comment likewise. (3) Note on DomainSet::media that it currently gates only the media_generate_* tool surface (no Media controller/store/subscriber yet).
|
Thanks for the thorough pass @M3gA-Mind — all 3 actionable items + nitpick 1 pushed in d06e24b/32de70009. On nitpick 2 ( |
Re-review — commits
|
…-filter-seam # Conflicts: # tests/agent_retrieval_e2e.rs
3 participants
Summary
DomainSet— a second runtime-composition axis onCoreBuilder(sibling to the existingServiceSet) that selects which domain families exist at runtime, one flag perDomainGroup.DomainGroupat the single registration site insrc/core/all.rs(via aGroupedControllerwrapper — the ~90 domain modules stay untouched), and filters the live surface by the ambientCoreContext::domains()./schema+ dispatch), agent tools, workspace stores, and event-bus subscribers. A gated domain becomes unknown-method, its tools absent, its stores/subscribers uninitialized.full()(default — byte-identical to pre-PR registration; the desktop app is unchanged),harness()(agent + memory + threads + config + security),none().[features]children (feat(core): feature gate — flows/workflows #4797–feat(core): feature gate — media generation #4804) compose with this runtime axis.Problem
Slim / harness builds of the core need a way to boot with only a subset of domain families live, without forking the registration path or risking any change to the shipped desktop runtime. Before this PR the domain surface was all-or-nothing: every controller, agent tool, store, and subscriber registered unconditionally.
Solution
DomainSet(src/core/runtime/builder.rs):Copystruct, one bool perDomainGroup,allows()accessor,full()/harness()/none()presets, threaded throughCoreBuilder::build()→CoreContext.DomainGroup+GroupedController(src/core/all.rs): registry entries carry their owning group;all_registered_controllers()/all_controller_schemas()/ dispatch filter by the ambientCoreContext::domains(). With no ambient context (⇒full, no filter) the public surface is byte-identical to the raw registry — proven byfull_registration_is_byte_identical.StoreInitPlan(context.rs) andDomainSubscriberPlan(jsonrpc.rs) — that the registrars consume and tests assert directly. This gives the store→group and subscriber→group mappings a single source of truth and lets the gating be verified without touching process-global store/bus state.tools/ops.rs) post-filtered by atool_group()classifier against the ambientDomainSet.examples/embed_headless.rsswitched toDomainSet::harness().Design note:
tokenjuiceis kept always-on / Platform (it is the token-compression content-router that runs on every agent tool output — agent-loop critical, not crypto), despite the epic listing it under the web3 gate; flagged in-code for re-scope at #4802.Submission Checklist
cargo llvm-cov+diff-covervs merge-base; a floor — boot-path lines injsonrpc/init_storesare integration-covered in CI, not the scoped unit subset). Every new surface has unit tests (registration, dispatch, schema-lookup, tools, stores, subscribers, presets). CIci-lite.ymlgate is authoritative.N/A: internal runtime-composition seam, no user-facing feature row## Related—N/A: no matrix rows affectedN/A: default full() path is byte-identical; desktop runtime unchangedCloses #NNNin the## RelatedsectionImpact
full()registration is byte-identical (asserted in test);examples/embed_headless.rsis the only caller opting intoharness().Noneambient ⇒ no filter ⇒ no added work on the desktop path.Related
upstream/main(stale base). Includes one unrelated pre-existing fix, bundled to unblock the coverage gate:test(agent): drop stale memory_recall assertion superseded by #4764— fix(agent): give the orchestrator direct memory tools so recall/store don't over-delegate #4764 (383581bd6) gave the orchestrator directmemory_recall/memory_storetools butagent_retrieval_e2estill forbadememory_recall, soorchestrator_reaches_memory_agent_on_demandfailed onmainitself (not caused by this PR). No other coupling to the feature-gate work.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
feat/4796-domainset-filter-seam49a872ce8271...(branch head; includes merge of upstream/main)Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheckcore::all::(49 passed, incl.full_registration_is_byte_identical,harness_excludes_gated_namespaces,dispatch_returns_none_for_gated_method,schema_lookup_is_gated_in_lockstep_with_dispatch),core::runtime::context::tests::store_init_plan_*(3),core::jsonrpc::tests::domain_subscriber_plan_*(3) +gated_method_is_unknown_at_transport_even_with_malformed_params,openhuman::tools::ops::tests::tool_group_*(2)cargo fmt --checkclean ·GGML_NATIVE=OFF cargo checkexit 0 ·cargo clippy --lib0 errors, no new warnings in changed files ·cargo check --example embed_headlessexit 0Validation Blocked
command:cargo test --lib(full)error:OOM on local machine (known; full lib suite exceeds local memory)impact:full matrix runs in CI; locally scoped to changed domains (core::all,core::runtime,tools::ops) — all greenBehavior Changes
DomainSetruntime axis; defaultfull()unchangedSummary by CodeRabbit
New Features
DomainSetaxis (independent of background services) withfull,harness, andnonepresets.DomainSet; gated methods behave like “unknown method.”Documentation
DomainSet-based runtime composition and verification expectations.Tests
DomainSetgating across dispatch, schema lookup, tool filtering, subscriber/store initialization, and updated a related e2e expectation.