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: 26 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 (4)
📝 WalkthroughWalkthroughAdds a default-on ChangesMCP feature contract
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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".
| // these via its `[tools] named = [...]` allowlist, and the host agent's | ||
| // own tool list is wide enough that the extra five entries are negligible. | ||
| // Compiled out entirely with the `mcp` feature. | ||
| #[cfg(feature = "mcp")] |
There was a problem hiding this comment.
In --no-default-features builds this #[cfg(feature = "mcp")] removes all five mcp_setup_* tools, but the mcp_setup built-in agent remains registered and the orchestrator still synthesizes its setup_mcp_server delegate from mcp_setup/agent.toml. A user who asks to install an MCP server in a slim build can therefore be routed into an agent whose prompt and allowlist require tools that are not in the runtime registry, leading to unknown-tool failures instead of the clean “MCP disabled” behavior used for mcp_agent/openhuman mcp.
Useful? React with 👍 / 👎.
![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
| Follows the voice facade+stub pattern for `mcp_server` / `mcp_registry` / `mcp_audit` (`stub.rs` in each), with two refinements worth copying: | ||
|
|
||
| - **Type carve-out.** Inert, dependency-free type modules stay **ungated**: `mcp_registry::types`, `mcp_audit::types`, `mcp_server::tools::types` (`McpToolSpec`). They are `serde`/`serde_json`-only data consumed by always-compiled callers (the orchestrator prompt builder, `tool_registry`). Both builds therefore share the **one real type definition** — the stubs carry behaviour only, so struct fields can never drift between the enabled and disabled builds. `ConnectedServerOverview` was moved from `connections.rs` into `types.rs` for exactly this reason and is re-exported from `connections` so existing paths still resolve. | ||
| - **Split facade (`mcp_client`).** `mcp_client` is **not** gated wholesale, because the directory does not match the real dependency graph. `mcp_client::sanitize` and `mcp_client::client` (`McpHttpClient`, `redact_endpoint`, `McpUnauthorizedError`) stay **always compiled**: they are mis-housed shared utilities. The `gitbooks` docs tool dials `McpHttpClient` directly (GitBook is modelled as a legacy MCP server), and the orchestrator prompt sanitises **skill** descriptions through `sanitize::sanitize_for_llm` — neither has anything to do with MCP, and stubbing them would silently break a docs tool and corrupt the orchestrator prompt in slim builds. Only `registry`, `stdio`, `spawn_env`, `setup_agent` are gated. **The gate follows the real dependency graph, not the directory name.** (Relocating `sanitize` + `client` out of `mcp_client` is worthwhile follow-up.) A bonus of keeping `client` compiled: the `McpServerNeedsAuth` classifier coupling test in `core::observability` stays always-compiled — no `#[cfg]`, no wording-drift leak. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use one spelling variant for “sanitize”.
This sentence mixes British sanitises with American sanitize; standardize the wording to avoid inconsistent documentation and the reported LanguageTool warning.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~247-~247: Do not mix variants of the same word (‘sanitise’ and ‘sanitize’) within a single text.
Context: ...CP server), and the orchestrator prompt sanitises skill descriptions through `sanitiz...
(EN_WORD_COHERENCY)
🤖 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 `@AGENTS.md` at 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.
Source: Linters/SAST tools
| /// No controllers: the internal `mcp_audit` list method is unregistered, so | ||
| /// the desktop UI/CLI sees an unknown method rather than an empty history. | ||
| /// | ||
| /// `src/core/all.rs` pushes this straight into its internal-controller vec | ||
| /// with no `#[cfg]` of its own — the empty vec keeps that file untouched. | ||
| pub fn all_mcp_audit_internal_controllers() -> Vec<crate::core::all::RegisteredController> { | ||
| Vec::new() | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Store surface (mirrors `store::{record_write, list_writes}`) | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// No-op that reports a synthetic row id: nothing can call an MCP write tool | ||
| /// in this build, so this is unreachable in practice. Returns `Ok` rather than | ||
| /// `Err` because the real callers treat a failure here as a logged anomaly — | ||
| /// an "audit write failed" warning would be actively misleading when the | ||
| /// audited subsystem does not exist. | ||
| pub fn record_write(_config: &Config, _record: NewMcpWriteRecord) -> Result<i64> { | ||
| Ok(0) | ||
| } | ||
|
|
||
| /// Empty history: no MCP writes can have occurred in this build. `Ok(vec![])` | ||
| /// (not `Err`) keeps the shape honest — the query succeeded, the log is empty. | ||
| pub fn list_writes(_config: &Config, _query: &McpWriteListQuery) -> Result<Vec<McpWriteRecord>> { | ||
| Ok(Vec::new()) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add trace diagnostics to both disabled MCP facades.
The new stub flows are silent, contrary to the repository’s Rust-flow observability rule. Add trace-level entry/exit markers without recording secrets, full PII, or audit payload contents.
src/openhuman/mcp_audit/stub.rs#L24-L49: instrument controller discovery,record_write, andlist_writes.src/openhuman/mcp_server/stub.rs#L25-L85: instrument CLI rejection, HTTP-server rejection, and empty tool-catalog generation.
As per coding guidelines, new or changed Rust flows must include verbose diagnostics while never logging secrets or full PII.
📍 Affects 2 files
src/openhuman/mcp_audit/stub.rs#L24-L49(this comment)src/openhuman/mcp_server/stub.rs#L25-L85
🤖 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/mcp_audit/stub.rs` around lines 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.
Source: Coding guidelines
| // | ||
| // Backed by the STATIC, config-declared server set (`[[mcp_client.servers]]` | ||
| // in TOML) — despite the local binding's name, this is NOT the dynamic | ||
| // `mcp_registry` domain gated above. Both are compiled out by the `mcp` | ||
| // feature; see the static-vs-dynamic note in AGENTS.md. | ||
| #[cfg(feature = "mcp")] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant region with line numbers
sed -n '780,900p' src/openhuman/tools/ops.rs | cat -n
# Find GitBooks-related symbols and cfg gates in the file
rg -n "Gitbooks|gitbooks|cfg\\(feature = \"mcp\"\\)|mcp_registry|mcp_client" src/openhuman/tools/ops.rsRepository: tinyhumansai/openhuman
Length of output: 8880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba src/openhuman/tools/ops.rs | sed -n '810,880p'
printf '\n--- search ---\n'
rg -n -C 3 'GitbooksSearchTool|GitbooksGetPageTool|cfg\(feature = "mcp"\)|cfg\(feature = "gitbooks"\)|mcp_registry|mcp_client' src/openhuman/tools/ops.rsRepository: tinyhumansai/openhuman
Length of output: 200
Gate the legacy GitBooks MCP tool registrations too. GitbooksSearchTool and GitbooksGetPageTool are registered before the #[cfg(feature = "mcp")] block, so gitbooks.enabled still exposes MCP-backed tools in an mcp-disabled build. Put that registration block behind the same gate.
🤖 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 - 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.
…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".
| #[cfg(feature = "mcp")] | ||
| pub mod connections; |
There was a problem hiding this comment.
In an mcp-off build, this cfg and the sibling cfgs for setup, store, and mcp_client::McpStdioClient remove APIs that several integration tests import unconditionally (tests/mcp_registry_e2e.rs, tests/mcp_registry_multi_server.rs, tests/mcp_setup_e2e.rs, and tests/mcp_stdio_integration.rs). As a result, cargo test --no-default-features --features tokenjuice-treesitter --tests will fail to compile before it can exercise the slim build; please mark these MCP-only integration tests with required-features = ["mcp"] or gate the test crates themselves.
Useful? React with 👍 / 👎.
2 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