fix(mcp): tighten stdio server logging and error semantics

[justinhsu1477]

Summary

• Always install the tracing subscriber when running openhuman-core mcp, defaulting to warn, so production errors reach stderr without --verbose.
• Route print_help through eprintln! to keep stdout protocol-only, matching the banner-suppression contract.
• Add ToolCallError::Internal so config-load failures surface as JSON-RPC -32603 Internal error instead of being mis-labelled -32602 Invalid params.
• Reject k > MAX_LIMIT in memory.search / memory.recall instead of silently clamping, so the LLM can self-correct on the next call.

Problem

Four follow-ups on the Phase 1 MCP server (#1760) noticed during review:

1. Silent errors in production. init_for_cli_run ran only when --verbose was passed, so log::error! / log::warn! events were dropped when the server ran as a subprocess of Claude Desktop / Cursor — exactly the contexts where field-debugging requires those events.
2. Help output collides with the protocol stream. print_help used println! while the banner suppression in core/cli.rs keeps stdout clean for JSON-RPC frames. Inconsistent, and a footgun if anyone later wires help into a non-exit path.
3. Wrong error code for internal failures. Config-load failures inside enforce_read_policy were mapped to ToolCallError::InvalidParams, surfacing as -32602 Invalid params. Clients then mis-attribute server-side problems to bad caller arguments.
4. Silent argument clamping. optional_limit clamped k > 50 to 50 without telling the caller. The schema advertises maximum: 50, so a higher value is a client bug; silent clamping prevents the LLM's corrective feedback loop.

Solution

• init_mcp_logging always installs the subscriber; default level is warn, --verbose promotes to debug, and a user-set RUST_LOG always wins.
• print_help now uses eprintln!.
• ToolCallError::Internal(String) added with code() + jsonrpc_message() dispatchers. enforce_read_policy uses Internal for config-load failures and keeps InvalidParams for policy denials (which are actionable to the caller).
• optional_limit returns InvalidParams("argument k must not exceed N (got M)") for k > MAX_LIMIT.

Submission Checklist

• Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy — 5 new unit tests in tools.rs; the replaced memory_search_params_default_and_clamp_k now becomes memory_search_params_trim_query_and_use_default_k + memory_search_rejects_k_above_max + memory_search_accepts_k_at_max (boundary).
• Diff coverage ≥ 80% — local cargo test --lib mcp_server:: runs 20 tests (0 failed). New error/clamp paths each have dedicated coverage. CI will confirm the gate.
• Coverage matrix updated — N/A: behaviour-only polish on existing row 11.1.4
• All affected feature IDs from the matrix are listed in the PR description under ## Related
• No new external network dependencies introduced
• Manual smoke checklist updated if this touches release-cut surfaces — N/A: opt-in MCP server, not on release-cut path
• Linked issue closed via Closes #NNN in the ## Related section — N/A: phase 1 (#1760) is the headline implementation; this PR is polish, not the closing PR for #1586

Impact

• Runtime: openhuman-core mcp subcommand only. No HTTP RPC, web, or mobile impact.
• Compatibility: MCP clients (Claude Desktop / Cursor / Inspector) see more useful stderr and stricter k validation. The schema-advertised maximum: 50 was already public, so well-behaved clients are unaffected.
• Performance: negligible — one comparison added in optional_limit, one subscriber installed earlier in startup.
• Security: SecurityPolicy gate untouched.

Related

• Feature IDs: 11.1.4 (MCP stdio server)
• Follows: feat(mcp): add stdio memory server #1760
• Refs: Expose OpenHuman as a Model Context Protocol (MCP) server #1586

---

AI Authored PR Metadata (required for Codex/Linear PRs)

Keep this section for AI-authored PRs. For human-only PRs, mark each field N/A.

Linear Issue

• Key: N/A
• URL: N/A

Commit & Branch

• Branch: N/A
• Commit SHA: N/A

Validation Run

• pnpm --filter openhuman-app format:check — N/A (no JS/TS changes)
• pnpm typecheck — N/A (no JS/TS changes)
• Focused tests: cargo test --lib mcp_server:: — 20 passed, 0 failed
• Rust fmt/check (if changed): cargo fmt --check clean; cargo check --lib compiles; cargo clippy --lib --no-deps no mcp_server warnings
• Tauri fmt/check (if changed): N/A (no app/src-tauri changes)

Validation Blocked

• command: N/A
• error: N/A
• impact: N/A

Behavior Changes

• Intended behavior change: error / warn logs reach stderr by default; k > 50 returns an error instead of silent clamp; config-load failures surface as -32603.
• User-visible effect: MCP clients see more diagnostic info on stderr and stricter argument validation. No change to the protocol surface.

Parity Contract

• Legacy behavior preserved: schema, tool list, initialize, ping, tools/list, tools/call unchanged. All existing Phase 1 tests still pass.
• Guard/fallback/dispatch parity checks: --verbose still bumps to debug; ToolCallError::InvalidParams callers unchanged.

Duplicate / Superseded PR Handling

• Duplicate PR(s): None
• Canonical PR: this PR
• Resolution: N/A

Summary by CodeRabbit

• Bug Fixes

  • Tool call errors now return accurate JSON-RPC codes and detailed messages and are logged with more context
  • Parameter validation for memory/search now rejects values above limits (with informative feedback) instead of silently clamping

• Improvements

  • CLI logging initialization made more robust and respects existing configuration
  • Help text output now goes to stderr to avoid interfering with protocol streams

• Tests

  • Unit tests updated/added to cover parameter handling and error-code/message mapping

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f5f25fd0-f0f4-4daf-9f90-b02af86514dc

📥 Commits

Reviewing files that changed from the base of the PR and between 636805b and 3838430.

📒 Files selected for processing (1)

• src/openhuman/mcp_server/tools.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/openhuman/mcp_server/tools.rs

---

📝 Walkthrough

Walkthrough

The PR refactors MCP server error handling to distinguish invalid client parameters from server-side failures. ToolCallError semantics are updated, parameter validation rejects oversized inputs instead of clamping, config-load failures are reclassified as internal errors, and the protocol layer now uses error-mapped JSON-RPC codes rather than hardcoded responses. Logging infrastructure is refactored and help output redirected to stderr.

Changes

MCP Error Classification and Validation

Layer / File(s)	Summary
Error Type and JSON-RPC Mapping src/openhuman/mcp_server/tools.rs	ToolCallError now provides message() returning the detail string consistently for both InvalidParams and Internal variants, while code() and jsonrpc_message() preserve distinct JSON-RPC -32602 vs -32603 codes; documentation reflects the client vs. server failure separation.
Parameter Validation and Error Classification src/openhuman/mcp_server/tools.rs	optional_limit rejects k > MAX_LIMIT with detailed InvalidParams error instead of silently clamping; enforce_read_policy reclassifies config-load failures from InvalidParams to Internal to correctly reflect server-side issues.
Protocol Error Response Handling src/openhuman/mcp_server/protocol.rs	tools/call error handler now derives the JSON-RPC code and message from err.code(), err.jsonrpc_message(), and err.message() instead of returning fixed -32602 "Invalid params" for all tool errors.
Logging Initialization and Output Configuration src/openhuman/mcp_server/stdio.rs	MCP server logging initialization is extracted into init_mcp_logging() helper that conditionally sets RUST_LOG based on --verbose flag; help text is output to eprintln! to prevent stdout-based protocol-stream collisions.
Test Coverage for Error Handling and Validation src/openhuman/mcp_server/tools.rs	Tests verify query trimming and default k behavior, rejection of k > MAX_LIMIT with descriptive errors, boundary acceptance at k == MAX_LIMIT, and correct JSON-RPC -32602/-32603 code and message mapping for error variants.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• tinyhumansai/openhuman#1760: This PR's error mapping and validation changes directly build upon and refine the MCP stdio server implementation introduced in PR #1760.

Suggested reviewers

• senamakel

Poem

🐰 I hopped through code and trimmed the space,
Replaced the clamps with clearer case,
Errors now map true, not all the same,
stderr for help, and logs to tame. 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main changes: improving logging initialization and error categorization in the MCP stdio server.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

• Generate code and open pull requests
• Plan features and break down work
• Investigate incidents and troubleshoot customer tickets together
• Automate recurring tasks and respond to alerts with triggers
• Summarize progress and report instantly

Built for teams:

• Shared memory across your entire org—no repeating context
• Per-thread sandboxes to safely plan and execute work
• Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/openhuman/mcp_server/tools.rs (1)

288-302: ⚡ Quick win

Add debug logging when config load fails.

The coding guidelines require verbose diagnostics logging for external calls and error handling paths. The load_config_with_timeout() call at line 292 is an external call, and the new error classification (Internal vs InvalidParams) is a behavior change that should be logged for debugging and operational visibility.

📊 Suggested logging addition

 async fn enforce_read_policy(tool_name: &str) -> Result<(), ToolCallError> {
     // Config-load failure is an internal/server issue (disk error, corrupt
     // config), not bad client input — report it as `-32603 Internal error`
     // rather than `-32602 Invalid params`.
-    let config = config_rpc::load_config_with_timeout()
-        .await
-        .map_err(|err| ToolCallError::Internal(format!("failed to load config: {err}")))?;
+    let config = match config_rpc::load_config_with_timeout().await {
+        Ok(config) => config,
+        Err(err) => {
+            log::warn!(
+                "[mcp_server] enforce_read_policy config load failed tool={} error={}",
+                tool_name,
+                err
+            );
+            return Err(ToolCallError::Internal(format!("failed to load config: {err}")));
+        }
+    };
     let policy = SecurityPolicy::from_config(&config.autonomy, &config.workspace_dir);

As per coding guidelines: "In Rust, use log / tracing at debug or trace level for development-oriented diagnostics on new/changed flows, including logs at entry/exit points, branch decisions, external calls, retries/timeouts, state transitions, and error handling paths."

🤖 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_server/tools.rs` around lines 288 - 302,
enforce_read_policy currently classifies a config-load failure as
ToolCallError::Internal but does not emit diagnostic logs; add debug-level
tracing around the external call to config_rpc::load_config_with_timeout()
(e.g., a trace/debug before the call and a debug on error) so the failure and
the decision to map it to Internal are visible; specifically, inside
enforce_read_policy log the call attempt, and in the map_err closure log the
error details and the fact you are returning ToolCallError::Internal (use
tracing::debug or log::debug and include err and tool_name to aid debugging).

🤖 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.

Nitpick comments:
In `@src/openhuman/mcp_server/tools.rs`:
- Around line 288-302: enforce_read_policy currently classifies a config-load
failure as ToolCallError::Internal but does not emit diagnostic logs; add
debug-level tracing around the external call to
config_rpc::load_config_with_timeout() (e.g., a trace/debug before the call and
a debug on error) so the failure and the decision to map it to Internal are
visible; specifically, inside enforce_read_policy log the call attempt, and in
the map_err closure log the error details and the fact you are returning
ToolCallError::Internal (use tracing::debug or log::debug and include err and
tool_name to aid debugging).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: cd3b3c89-e35e-4eb9-b995-a765003186e4

📥 Commits

Reviewing files that changed from the base of the PR and between a6def97 and 636805b.

📒 Files selected for processing (3)

• src/openhuman/mcp_server/protocol.rs
• src/openhuman/mcp_server/stdio.rs
• src/openhuman/mcp_server/tools.rs

[justinhsu1477]

Heads-up: the two red rust checks are blocked on what looks like a regression
on main that's affecting every PR right now, not anything this PR touches.

The failing test:

openhuman::composio::auth_retry::tests::retries_once_only_even_when_second_call_still_errors
src/openhuman/composio/auth_retry_tests.rs:187
assertion left == right failed: must retry exactly once, never a third time
left: 4, right: 2

Recent CI runs on main are failing in the same place (e.g. #1792 README
edit, #1743, #1717, #1783). This PR's diff lives entirely under
src/openhuman/mcp_server/; composio/ isn't touched.

The third red is Build & smoke-test core image, which failed at docker buildx — looks like an infrastructure flake, unrelated.

Happy to re-trigger CI once main is green, or to wait for a maintainer
to weigh in on the composio side.

[justinhsu1477]

Thanks for the merge