feat(core): compile-time mcp feature gate (#4799)#4914
Conversation
Empty feature list on purpose: there is no MCP SDK in this crate. The whole protocol stack is hand-rolled over tokio process stdio + reqwest + axum, all load-bearing for non-MCP domains, so the gate drops ~20k LOC and ~19 agent tools but ZERO dependencies. The comment records that fact so nobody "fixes" the empty list into `dep:` entries later.
…pes (tinyhumansai#4799) The always-compiled orchestrator prompt builder consumes this type, but it lived in connections.rs alongside the tokio/stdio connection map that the `mcp` feature gates. Relocating it to the inert serde-only types module lets both builds share ONE real definition instead of a hand-mirrored stub copy, so the struct's fields can never drift between them. connections re-exports it, so mcp_registry::connections::ConnectedServerOverview keeps resolving for every existing caller.
…umansai#4799) Facade + stub: types stays ungated (inert serde data consumed by the always-compiled orchestrator prompt), behaviour is gated. The stub's aggregator returns an empty Vec, which is what keeps src/core/all.rs — the epic's hottest multi-agent file — free of any #[cfg] for this gate. boot/bus/supervisor no-op, oauth::complete errors, connected_overview and all_connected_tools return empty; every always-on caller already tolerates those shapes with zero call-site edits.
…sai#4799) Same facade+stub+ungated-types shape as mcp_registry. The audit log records calls made through the MCP server, so with MCP compiled out there is no writer: list_writes returns Ok(empty) rather than Err — the query genuinely succeeded and the log is genuinely empty.
…st (tinyhumansai#4799) tools::types stays ungated so McpToolSpec — inert &'static str + Value data named by the always-compiled tool_registry — is the same real type in both builds. src/core/cli.rs is deliberately NOT touched. The naive gate is to delete the "mcp" | "mcp-server" match arm, but then `mcp` falls through to generic namespace resolution and dies with "unknown namespace: mcp" — reading like a user typo rather than a build fact. Instead the arm resolves to the stub, which bails with a message naming the cause and the fix, so an MCP host spawning `openhuman mcp` gets a non-zero exit + diagnostic instead of hanging on stdout that never speaks JSON-RPC.
…led (tinyhumansai#4799) mcp_client is NOT gated wholesale, because the directory does not match the real dependency graph. Two submodules stay always compiled: sanitize — orchestrator/prompt.rs runs *skill* descriptions through sanitize_for_llm. Nothing to do with MCP; stubbing it would corrupt the orchestrator prompt in slim builds. client — the gitbooks docs tool dials McpHttpClient + redact_endpoint directly (GitBook is modelled as a legacy MCP server); stubbing it would break a docs tool users still reach. Only registry/stdio/spawn_env/setup_agent are gated. The gate follows the real dependency graph, not the directory name. Keeping client compiled also keeps McpUnauthorizedError — and so the McpServerNeedsAuth classifier coupling test — always compiled, with no #[cfg] and no wording-drift leak.
…4799) Three clusters, both halves of the (confusingly named) MCP surface: mcp_registry_* × 11 — DYNAMIC, sqlite-installed servers mcp_setup_* × 5 — setup-agent surface mcp_* × 3 — STATIC, config-declared servers (mcp_client) The naming is inverted from intuition, so gating only one half would leave the gate half-applied. gitbooks deliberately stays — it dials McpHttpClient but is a docs tool, not MCP-subsystem code. network/{mcp,mcp_setup}.rs are leaf-gated: their only consumers are the gated blocks above, so no stub is needed.
…act (tinyhumansai#4799) Drops delegate_use_mcp_server from the orchestrator's synthesised belt. The orchestrator's agent.toml still lists mcp_agent in subagents — TOML is data, it cannot be #[cfg]'d, and forking it per-feature would invite exactly the drift this gate avoids. That dangling id is safe: collect_orchestrator_tools warns and skips unknown subagent ids, and validate_tier_hierarchy continues past them rather than failing the boot. Because the whole gate rests on that tolerance, pin it from both build configurations: orchestrator_tolerates_unresolvable_subagent_id and orchestrator_tolerates_absent_mcp_agent. A future "unknown subagent is a hard error" change now fails loudly here instead of silently breaking the slim build's boot — a failure mode CI's cargo check lane cannot catch.
…tinyhumansai#4799) all_tests: mcp_clients + mcp_audit registered when the gate is on, absent when off. An assert that only ever runs in one build configuration cannot prove a gate works. cli_tests: `openhuman mcp` must fail naming the BUILD as the cause and must NOT degrade into "unknown namespace" — the exact confusing failure the issue forbids. Covers the mcp-server alias too. These only run under --no-default-features, which CI never test-runs.
) Rather than gating whole tests away and losing their non-MCP coverage, gate only the MCP-specific assertions: - desktop tool tables: per-element #[cfg] on the mcp_registry_* rows - tool_registry: MCP-transport half gated, controller half keeps asserting; trim/list tests fall back to a controller-transport id - diagnostics: mcp_stdio_tools > 0 when on, == 0 when off Adds all_tools_omits_mcp_tools_when_gate_off, which asserts by PREFIX rather than naming the ~19 tools — a new MCP tool added later must not be able to leak into slim builds just because nobody extended a hardcoded list.
…cks (tinyhumansai#4799) legacy_aliases: the frontend RPC catalog and the alias table are DATA — they are authored against the full shipped surface and are built independently of the core's Cargo features, so in a slim build they legitimately name openhuman.mcp_clients_* methods whose controllers no longer exist. Ignore exactly the gated namespaces and keep asserting on everything else, so the drift signal survives instead of tempting someone to delete live frontend methods from the catalog. Same data-vs-code shape as the orchestrator TOML. definition_tests: gate the mcp_agent max-iterations row. mcp_setup stays — only its tools are gated, the agent definition still loads in both builds. tool_prep: use_mcp_server is mcp_agent's delegate_name; setup_mcp_server is mcp_setup's and stays.
…ansai#4799) default-features = false means the shell must opt in explicitly or the shipped app silently loses MCP. Multi-line array so sibling gates append cleanly.
…sions (tinyhumansai#4799) Records the three things most likely to be got wrong later: - the gate drops ZERO dependencies (there is no MCP SDK; the DoD line claiming it sheds one is superseded), so mcp = [] is empty on purpose; - static vs dynamic is INVERTED from intuition — mcp_client is the STATIC config-declared set, mcp_registry the DYNAMIC installed one; gating only one leaves the gate half-applied; - mcp_client is split-gated because the gate follows the real dependency graph, not the directory name. Plus the type carve-out, why cli.rs stays untouched, and why the orchestrator TOML's dangling mcp_agent reference is expected and safe.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds a default-on ChangesMCP compile-time gate
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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: a74f15ad79
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@AGENTS.md`:
- Line 247: Standardize the spelling of “sanitize” in the mcp_client
documentation paragraph by replacing the British “sanitises” variant with the
existing American spelling used elsewhere. Leave the dependency-gating
explanation and all other wording unchanged.
- Line 249: Update the scope note’s grep claim to inspect only Cargo.toml
dependency declarations, rather than all file contents, so the intentional mcp
feature definition and comments do not produce matches. Preserve the statement
that the only MCP-named dependency-related entry is the test-mcp-stub binary and
do not add dependencies to the mcp feature.
In `@app/src-tauri/Cargo.toml`:
- Around line 135-137: Update the openhuman_core dependency configuration to
preserve the root crate’s default features while enabling "mcp": remove
default-features = false, or explicitly include the existing
"tokenjuice-treesitter" and "voice" defaults alongside "mcp".
In `@src/openhuman/mcp_audit/stub.rs`:
- Around line 24-49: Add trace-level entry and exit diagnostics to all disabled
MCP facade flows: instrument all_mcp_audit_internal_controllers, record_write,
and list_writes in src/openhuman/mcp_audit/stub.rs (lines 24-49), and CLI
rejection, HTTP-server rejection, and empty tool-catalog generation in
src/openhuman/mcp_server/stub.rs (lines 25-85). Keep diagnostics verbose enough
to identify each operation and its outcome, while excluding secrets, full PII,
and audit payload contents.
In `@src/openhuman/tools/ops.rs`:
- Around line 866-871: Move the registration block for GitbooksSearchTool and
GitbooksGetPageTool under the same #[cfg(feature = "mcp")] gate as the
surrounding static MCP server registrations. Ensure both legacy GitBooks tools
are unavailable when the mcp feature is disabled, while preserving their
existing registration behavior when enabled.
🪄 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: 10b9f4c5-cd25-4ff8-bb57-09265e4d2b7d
📒 Files selected for processing (26)
AGENTS.mdCargo.tomlapp/src-tauri/Cargo.tomlsrc/core/all_tests.rssrc/core/cli_tests.rssrc/core/legacy_aliases.rssrc/openhuman/agent/harness/definition_tests.rssrc/openhuman/agent/harness/subagent_runner/tool_prep.rssrc/openhuman/agent_registry/agents/loader.rssrc/openhuman/agent_registry/agents/mod.rssrc/openhuman/mcp_audit/mod.rssrc/openhuman/mcp_audit/stub.rssrc/openhuman/mcp_client/mod.rssrc/openhuman/mcp_registry/connections.rssrc/openhuman/mcp_registry/mod.rssrc/openhuman/mcp_registry/stub.rssrc/openhuman/mcp_registry/types.rssrc/openhuman/mcp_server/mod.rssrc/openhuman/mcp_server/stub.rssrc/openhuman/mcp_server/tools/mod.rssrc/openhuman/tool_registry/ops_tests.rssrc/openhuman/tool_registry/schemas.rssrc/openhuman/tools/impl/network/mod.rssrc/openhuman/tools/mod.rssrc/openhuman/tools/ops.rssrc/openhuman/tools/ops_tests.rs
…re-gate # Conflicts: # AGENTS.md # Cargo.toml # app/src-tauri/Cargo.toml
…mansai#4799) - Use the American "sanitizes" consistently (the file already uses "sanitize" 4x); the lone British variant tripped LanguageTool. - The "grep -i mcp Cargo.toml matches only test-mcp-stub" claim went stale in this PR: the new mcp = [] feature definition, the default list and the gate comments all match it now. Scope the statement to dependency declarations, which is what it was actually asserting.
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/openhuman/tools/ops.rs (1)
851-852: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the MCP setup-agent gating comment.
#[cfg(feature = "mcp")]means this block is compiled in when MCP is enabled and omitted when disabled, but the surrounding text says it is “registered unconditionally” and “compiled out ... with” the feature. Update the comments to describe the actual behavior.🤖 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/openhuman/tools/ops.rs` around lines 851 - 852, Update the comments surrounding the MCP-gated setup-agent block to state that it is compiled and registered when the mcp feature is enabled, and omitted when the feature is disabled. Remove wording that says it is registered unconditionally or compiled out with the feature, while leaving the #[cfg(feature = "mcp")] gating unchanged.
♻️ Duplicate comments (1)
src/openhuman/tools/ops.rs (1)
866-895: 🎯 Functional Correctness | 🟠 MajorGate the legacy GitBooks MCP tools too.
The generic MCP block is now gated, but
GitbooksSearchToolandGitbooksGetPageToolremain registered unconditionally at Lines 833-842. They can therefore survive--no-default-features; because their names aregitbooks_*,tool_group()also does not apply the runtimeDomainSet::mcpgate. Put their registration behind#[cfg(feature = "mcp")]and classify thegitbooks_prefix asDomainGroup::Mcp.🤖 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/openhuman/tools/ops.rs` around lines 866 - 895, Gate the GitBooks tool registration containing GitbooksSearchTool and GitbooksGetPageTool with #[cfg(feature = "mcp")], and update tool_group() so the gitbooks_ name prefix maps to DomainGroup::Mcp. Ensure these legacy tools are excluded without the mcp feature and subject to the runtime MCP domain gate.
🤖 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/openhuman/tools/ops.rs`:
- Around line 851-852: Update the comments surrounding the MCP-gated setup-agent
block to state that it is compiled and registered when the mcp feature is
enabled, and omitted when the feature is disabled. Remove wording that says it
is registered unconditionally or compiled out with the feature, while leaving
the #[cfg(feature = "mcp")] gating unchanged.
---
Duplicate comments:
In `@src/openhuman/tools/ops.rs`:
- Around line 866-895: Gate the GitBooks tool registration containing
GitbooksSearchTool and GitbooksGetPageTool with #[cfg(feature = "mcp")], and
update tool_group() so the gitbooks_ name prefix maps to DomainGroup::Mcp.
Ensure these legacy tools are excluded without the mcp feature and subject to
the runtime MCP domain gate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 417a85f9-1c32-4268-b2ab-3fb3f9ef799b
📒 Files selected for processing (5)
AGENTS.mdCargo.tomlapp/src-tauri/Cargo.tomlsrc/openhuman/tools/ops.rssrc/openhuman/tools/ops_tests.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src-tauri/Cargo.toml
- AGENTS.md
- src/openhuman/tools/ops_tests.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc20a03b00
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…re-gate # Conflicts: # Cargo.toml # app/src-tauri/Cargo.toml
Maintainer review follow-ups for the mcp compile-time gate (tinyhumansai#4799): - Add `#![cfg(feature = "mcp")]` to the four MCP integration test files (mcp_registry_e2e, mcp_registry_multi_server, mcp_setup_e2e, mcp_stdio_integration). They import gated APIs (mcp_registry::{store, boot,ops,connections,setup}, mcp_client::McpStdioClient) unconditionally and are auto-discovered, so `cargo test --no-default-features --features tokenjuice-treesitter --tests` failed to compile in slim builds. Gating the whole file compiles them to nothing when mcp is off. - Add debug/warn diagnostics to the mcp_audit and mcp_server disabled stubs, matching the sibling mcp_registry stub and the repo's Rust-flow observability rule (no secrets/PII logged).
|
Maintainer review changes (pushed as 1. Gated the four MCP-only integration tests — 2. Instrumented the two silent disabled stubs — Both changes are cfg-off-path / test-only, so the default (shipped) build remains byte-identical. Declined items are answered in inline replies below. |
|
Maintainer sync: merged current main (includes #4949, the fix for the red-main typecheck failure tracked in #4948) into this branch. The "Frontend Checks (quality, i18n, docs, coverage)" failure on this PR was inherited from main — the two ToolTimelineEntry fixtures in Conversations.render.test.tsx were missing the required 'seq' property introduced by #4945 — not caused by this PR's Rust feature-gate changes. Clean merge, no conflicts to resolve. Re-running CI should clear that lane. |
|
@coderabbitai review The head has moved on since the standing review: every actionable comment has been addressed (fixed, or declined in-thread with reasoning), and the branch has since been re-synced onto current |
|
✅ Action performedReview finished.
|
|
Maintainer follow-up: my earlier stub-diagnostics commit referenced DISABLED_MSG in mcp_audit/stub.rs without defining it there (it is a module-private const in mcp_registry/stub.rs), which broke the gates-off build. Fixed by defining the const locally, mirroring the sibling stub. Apologies for the churn — the Feature-Gate Smoke lane caught it correctly. |
Resolve sibling-gate conflicts with tinyhumansai#4915 (meet gate, tinyhumansai#4800) by union: - Cargo.toml: keep both `meet` and `mcp` in default + both feature blocks - app/src-tauri/Cargo.toml: forward both `meet` and `mcp` - AGENTS.md: keep both gate-table rows and both gate sections - src/core/all_tests.rs: keep all four both-direction gate tests
|
Maintainer follow-up — two things in one push ( 1. Fixed my own gates-off regression. My earlier stub-diagnostics commit referenced 2. Re-synced onto current
No local cargo run (gates-off CI lane is the authority). Letting CI verify. |
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Approving — both conditions genuinely met.
CI: fully green. 14 pass, 4 skipped by changed-area detection (correct for a Rust-only change), zero pending, zero failing — including Rust Quality, Rust Core Coverage, Rust Tauri Coverage, Test Inventory, PR CI Gate, and critically Rust Feature-Gate Smoke (gates off).
That last lane deserves a note, because it caught a regression we introduced. An earlier maintainer commit on this PR added debug logging to mcp_audit/stub.rs referencing DISABLED_MSG without defining it there — that const is module-private in the sibling mcp_registry/stub.rs:24, so it was never in scope. Result: E0425 × 3 and a broken gates-off build. Fixed by defining the const locally (mirroring the sibling stub) rather than making the sibling's pub — each stub owning its own const keeps the modules independent. The sibling stubs were swept: mcp_server/stub.rs already defines its own, so no further defects. stub.rs compiles only in the gates-off lane, which is precisely why that lane is the authority on this class of change — it did its job here.
Review state: no CHANGES_REQUESTED. CodeRabbit's earlier block was stale; it has since re-reviewed and passed.
Verified beyond CI:
- The mcp gate uses a deliberate split facade with a single-definition type carve-out; default build byte-identical, with symmetric registered-when-on / absent-when-off tests.
- Re-synced onto current
main(includes #4915's meet gate). Four conflicts resolved to the union, including a genuinely structural one insrc/core/all_tests.rswhere both sides appended test fns sharing the final brace — all four tests retained, each verified to appear once and be well-braced.src/core/all.rsandtools/ops.rsauto-merged cleanly with both gates' content intact. - Bot items: fixed where legitimate; the two declined (gating GitBooks tools behind
mcp; the pre-existing Tauridefault-featuresgap) were declined with reasons — the first contradicts the documented split-facade design, the second is a known epic-level gap the PR body already documents.
Not merging — that is the maintainer's call.
…nflicts Both sibling gates append their feature to the default set, add a feature definition, a shell-forwarding entry, and docs. Kept both: - Cargo.toml [features].default: [..., "meet", "skills", "mcp"] - Cargo.toml: kept both skills = [] and mcp = [] definitions + comments - app/src-tauri/Cargo.toml: forward both skills and mcp; also deduped a doubled "tokenjuice-treesitter" entry left by the merge - AGENTS.md: kept both feature-table rows and both gate-pattern doc sections
|
Maintainer sync: merged current main to resolve conflicts introduced when sibling gate #4913 ( Conflicts resolved (kept both sides — the gates are additive, not competing):
All Rust registration sites ( |
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Approving — both conditions genuinely met, on the re-synced head.
CI: fully green — 15 pass, 4 skipped by changed-area detection (correct for a Rust-only change), zero pending, zero failing. Includes Rust Quality, Rust Core Coverage, Rust Tauri Coverage, Test Inventory, PR CI Gate, and critically Rust Feature-Gate Smoke (gates off).
That last lane earned its place on this PR. An earlier maintainer commit here added debug logging to mcp_audit/stub.rs referencing DISABLED_MSG without defining it in that module — the const is module-private in the sibling mcp_registry/stub.rs:24 — producing E0425 × 3 and a broken gates-off build. stub.rs files compile only in that lane, which is exactly why it is the authority for this class of change. Fixed by defining the const locally rather than making the sibling's pub, keeping each stub self-contained; mcp_server/stub.rs was swept and already had its own, so no further instances. To be clear: that regression was ours, not the author's.
Review state: no CHANGES_REQUESTED; CodeRabbit's earlier block is cleared.
Verified beyond CI:
- The mcp gate uses a deliberate split facade with a single-definition type carve-out; default build byte-identical, with symmetric registered-when-on / absent-when-off tests.
- Re-synced onto current
main(includes #4913 and #4915). Conflicts resolved to the union, including a genuinely structural one insrc/core/all_tests.rswhere both sides appended test fns sharing the closing brace — all four tests retained, each verified to appear once and be well-braced.src/core/all.rsandtools/ops.rsauto-merged with both gates' content intact. - Declined items were declined with reasons, not dropped: gating the GitBooks tools behind
mcpcontradicts the documented split-facade design (gitbooks is a docs tool, not MCP-subsystem code), and the Tauridefault-featuresgap is pre-existing and documented in the PR body. - The Codex
mcp_setup-agent finding is acknowledged and deferred with reason — fully gating it needs a coordinated pass across unconditional refs inawaiting_user.rs/dispatch.rs/core/all.rs; it is not a boot hazard (the loader tolerates it, pinned by test), only a degraded-UX edge in slim builds.
Not merging — that is the maintainer's call.
…ate conflicts tinyhumansai#4912 (flows) merged into main since the last sync, re-introducing sibling drift. Kept all gates (additive, not competing): - Cargo.toml [features].default: [..., "meet", "skills", "flows", "mcp"] - AGENTS.md: kept skills + flows + mcp feature-table rows and the flows leaf-gate doc block alongside this PR's mcp gate section - src/core/all_tests.rs: kept BOTH the mcp directional-gate tests (registered/absent) and main's flows directional-gate tests (registered/absent) — the conflict fused their shared trailing braces - app/src-tauri/Cargo.toml auto-merged: forwards all 8 gates incl. flows + mcp
|
Maintainer sync (2nd): merged current main again — #4912 ( Conflicts resolved (kept every gate — additive, not competing):
Verified against upstream/main: |
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Re-approving after the final conflict resolution — both conditions genuinely met on the new head.
CI: fully green — 15 pass, 4 skipped by changed-area detection (correct for a Rust-only change), zero pending, zero failing. Includes Rust Quality (fmt, clippy), Rust Core Coverage, Rust Tauri Coverage, Test Inventory, PR CI Gate, and critically Rust Feature-Gate Smoke (gates off).
Review state: no CHANGES_REQUESTED.
Why the re-approval: #4912 (flows gate) merged, which conflicted this branch — the fourth time this series has cascaded, since every gate in #4797-#4800 edits the same registration sites. Conflicts were resolved to the union across Cargo.toml, AGENTS.md, src/core/all.rs, src/openhuman/tools/ops.rs, and src/core/all_tests.rs; that push dismissed the prior approval, hence this one. This is the last gate in the series — the other three (#4913 skills, #4915 meet, #4912 flows) are merged.
Verified across the life of this PR:
- The mcp gate uses a deliberate split facade with a single-definition type carve-out; default build byte-identical, symmetric registered-when-on / absent-when-off tests.
src/core/all_tests.rshas repeatedly conflicted structurally — both sides appending test fns that share the closing brace. Re-verified after this resolution: every test fn appears exactly once and the braces balance.- A regression we introduced was caught and fixed here, not by the author. An earlier maintainer commit added debug logging to
mcp_audit/stub.rsreferencingDISABLED_MSGwithout defining it in that module (the const is module-private in the siblingmcp_registry/stub.rs:24), breaking the gates-off build withE0425x 3. Fixed by defining the const locally rather than making the sibling'spub.stub.rsfiles compile only in the gates-off lane — which is exactly why that lane is the authority for this class of change, and why it is green here that matters. - Declined items were declined with reasons, not dropped: gating the GitBooks tools behind
mcpcontradicts the documented split-facade design (gitbooks is a docs tool, not MCP-subsystem code); the Tauridefault-featuresgap is pre-existing and documented in the PR body. The Codexmcp_setup-agent finding is deferred with reason — fully gating it needs a coordinated pass across unconditional refs inawaiting_user.rs/dispatch.rs/core/all.rs, and it is not a boot hazard (the loader tolerates it, pinned by test), only a degraded-UX edge in slim builds.
Not merging — that is the maintainer's call.
…mansai#4914) Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai> Co-authored-by: M3gA-Mind <elvin@tinyhumans.ai> Co-authored-by: oxoxDev <oxoxdev@users.noreply.github.com>
3 participants
Summary
mcpfeature (default-ON) gatingmcp_client/mcp_server/mcp_registry/mcp_audit— ~20.1k LOC and ~19 agent tools — composing with the runtimeDomainSet::mcpflag from feat(core): DomainSet on CoreBuilder + DomainRegistration filter seam (feature-gate prerequisite) #4796.cargo build --no-default-features --features "<gates without mcp>".mcp = []).openhuman-core mcpandmcp-serverexit 1 with a build-fact error. Verified on the real binary.mcp_client— the gate follows the real dependency graph, not the directory name.Problem
#4795 wants one compile-time feature gate per subsystem family; #4796 shipped the runtime
DomainSetaxis. This PR adds the compile-time half for the MCP family. Three findings reshaped the scope:reqwest, andaxum— each load-bearing for non-MCP domains. Somcp = []and the gate's value is LOC, RPC surface, tool-belt size and attack surface, not binary size.mcp_clientis not all MCP.sanitizeandclient::{McpHttpClient, redact_endpoint}are mis-housed shared utilities: a GitBook docs tool dialsMcpHttpClient, and the orchestrator prompt runssanitize::sanitize_for_llmover skill descriptions. Gating the module wholesale breaks both.mcp_clientholds the static, config-declared server set;mcp_registryholds the dynamicmcp_clients.dbinstalls. Both must be gated, and the names actively mislead — now documented inAGENTS.md.Two epic DoD bullets also do not apply: "Add a
DomainSetflag onCoreBuilder" is already landed by #4796 (PR #4808); "sheds its exclusive dependencies" is boilerplate that is false here, as above.Solution
Cargo.toml:mcp = []added todefault— dep list intentionally empty, commented so nobody "fixes" it later.Split facade for
mcp_client. The real client/registry surface goes behind#[cfg(feature = "mcp")];sanitize+client::{McpHttpClient, redact_endpoint}stay always compiled, because the docs tool and the orchestrator prompt reach them from outside the MCP family. (Recommend a follow-up to relocate them — see## Related.)Facade+stub for
mcp_server,mcp_registry,mcp_audit:pub modstays compiled, real submodules gated,stub.rsmirrors the always-on caller surface with disabled-error / empty bodies.ConnectedServerOverviewrelocated into the dep-freetypes.rsrather than mirrored in a stub — an improvement on the plan: both builds now share one definition, so field drift between configs is structurally impossible.CLI fails loudly.
cli.rsis untouched; the stub bails. Verified on the real binary —openhuman-core mcpandmcp-serverbothexit=1with:Not "unknown namespace". No existing test covered this path — only running the binary proved it.
Dangling-subagent contract pinned. The gate rests on the agent loader tolerating a dangling
mcp_agentsubagent id (it warns and skips — a documented contract). Two new tests pin it, so a future "strict unknown subagent" change fails loudly instead of silently breaking slim builds.App forwarding (
app/src-tauri/Cargo.toml):openhuman_coreisdefault-features = false, somcpis added explicitly to keep the domain in the shipped desktop app. Verified viacargo tree -e features -i openhuman.CI: no change needed — the
rust-feature-gate-smokelane is a deny-by-default allow-list (--no-default-features --features tokenjuice-treesitter), so a new default-ON gate is excluded automatically and the disabled combination is already covered.When off: the
mcp_clients+mcp_auditnamespaces are absent from/schemaand their methods return a clean JSON-RPC unknown-method (-32000); the ~19 MCP tools are absent; the MCP CLI subcommands exit 1 with the build-fact error above.Submission Checklist
#[cfg]attrs, a Cargo feature, stub facades, a type relocation, docs);diff-coverreports no coverable lines. The directional gate tests cover the on/off behaviour. — Diff coverage ≥ 80%## Relatedmcp = [].docs/RELEASE-MANUAL-SMOKE.md)Closes #NNNin the## RelatedsectionImpact
mcpon and the build is byte-identical.openhuman-core mcpandmcp-serverexit 1 with an explicit build-fact error rather than appearing to work.Related
DomainSetseam (PR feat(core): runtime DomainSet composition axis — group-tagged registry + ambient filter (#4796) #4808); follows the feat(core): feature gate — voice #4803 voice pathfinder and feat(core): feature gate — media generation #4804 media leaf gate (PR feat(core): compile-time media feature gate (media_generation + image) (#4804) #4840). Sibling gates: feat(core): feature gate — flows/workflows #4797 (flows), feat(core): feature gate — skills #4798 (skills), feat(core): feature gate — meet #4800 (meet), feat(core): feature gate — web3 #4802 (web3, PR feat(web3): compile-time web3 feature gate for wallet/web3/x402 (#4802) #4855).mcp_client—sanitizeandclient::{McpHttpClient, redact_endpoint}are consumed by the GitBook docs tool and the orchestrator prompt, and do not belong in the MCP family. They are only always-compiled here because the gate follows the real dependency graph.voiceandtokenjuice-treesitterare currently not forwarded inapp/src-tauri/Cargo.toml(whoseopenhuman_coredep isdefault-features = false), so those two domains are missing from the shipped desktop app today. Pre-existing regression, out of scope here — being fixed epic-level.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
feat/4799-mcp-feature-gatea74f15ad79a098485c6470c25ba1e41f2b7d5222(13 commits, 26 files, +918/-59)Validation Run
pnpm --filter openhuman-app format:checkpnpm typecheckcargo fmtclean · enabledcargo check0 errors · disabledcargo check --no-default-features --features tokenjuice-treesitter0 errors · clippy — see Validation Blocked for 6 pre-existing errors.app/src-tauri/Cargo.toml(one dep line), no Tauri Rust source touched — forwarding verified viacargo tree -e features -i openhuman— Tauri fmt/check (if changed)Boot smoke (both directions, 2 runs for repeatability) — both configs built, linked, booted, and served
/rpc:mcp_clientsnamespace present in/schema; probe →{"result":{"installed":[]}}.mcp_clients+mcp_auditabsent; methods return a clean JSON-RPC unknown-method (-32000); the process survives. CLI subcommands exit 1 with the build-fact error — verified on the real binary, since no test covered it.Validation Blocked
command:cargo clippyerror:6 clippy errors.impact:Pre-existing — they reproduce identically on a clean base, so they are not introduced by this PR. Disclosed rather than worked around.command:cargo test --no-default-featureserror:2 pre-existing failures onmaintoday —group_mapping_smokeandharness_excludes_gated_namespaces, both asserting thevoicenamespace uncfg'd (fallout from the merged voice gate feat(core): feature gate — voice #4803). CI cannot see them: the feature-gate smoke lane runscargo check, which never compiles test code.impact:Not introduced by this PR and not fixed by it. This PR cfg'd its own asserts and left voice's alone; the voice asserts are a separate, epic-level fix that is pending. Scoped enabled-config tests and both-directioncargo checkare green.Behavior Changes
mcpcompile-time gate. Default build unchanged.Parity Contract
/rpc); gate OFF yields absence at every registration site, and the CLI fails loudly rather than degrading to "unknown namespace".ConnectedServerOverviewhas a single definition shared by both configs (relocated totypes.rs), so field drift is structurally impossible. The loader's dangling-mcp_agenttolerance is now pinned by tests, so the contract the gate rests on cannot silently regress. Composes with, and does not replace, the runtimeDomainSet::mcpguard from feat(core): DomainSet on CoreBuilder + DomainRegistration filter seam (feature-gate prerequisite) #4796.Duplicate / Superseded PR Handling
Summary by CodeRabbit
mcpbuild option enabled by default, bringing MCP connectivity, MCP tools, MCP registry behavior, and MCP audit support into standard builds.