Expose configuration#20
Merged
Merged
Conversation
InitializeResponse._meta
…_meta Adds GET /api/config, /api/models, /api/skills, /api/tools, /api/mcp-servers endpoints (replacing /models). MCP connection status is tracked at startup and surfaced per-server. The same data is included in InitializeResponse._meta alongside the existing models entry.
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR adds MCP server status tracking and REST API endpoints to expose configuration, models, skills, tools, and server metadata. The changes flow from new serializable data structures through status collection during tool loading, agent state initialization, and finally HTTP endpoint handlers, alongside documentation and Docker deployment updates. ChangesMCP Server Status Exposure & REST API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![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
🧹 Nitpick comments (3)
Dockerfile (1)
49-49: ⚡ Quick winSet explicit owner for copied config file.
At Line 49, add
--chown=openheim:openheimto ensure the copied config file is owned by the runtime user. Without this flag, Docker preserves host file ownership, which can cause permission issues if the application needs to modify the config.Proposed change
-COPY config.toml /home/openheim/.openheim/config.toml +COPY --chown=openheim:openheim config.toml /home/openheim/.openheim/config.toml🤖 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 `@Dockerfile` at line 49, Update the Dockerfile COPY instruction that copies the config file (the line containing "COPY config.toml /home/openheim/.openheim/config.toml") to set explicit ownership by adding the --chown=openheim:openheim flag so the file is owned by the runtime user; modify that COPY to use --chown=openheim:openheim before the source path.src/mcp/mod.rs (1)
13-25: MCP connection status is a startup-time snapshot — stale after process start.
McpServerStatusis populated once inload_mcp_toolsand stored immutably inAgentState::mcp_statuses. A server that was down at startup (connected: false) will never flip toconnected: true, and a server that was up but later disconnects will still show asconnected: true. The/api/mcp-serversendpoint andInitializeResponse._metawill both serve potentially stale data indefinitely.Consider whether a background health-check loop (re-running
connect_serverfor failed entries at startup) or a lazily-refreshed status would better serve consumers of the/api/mcp-serversendpoint.🤖 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/mcp/mod.rs` around lines 13 - 25, McpServerStatus is only set once in load_mcp_tools and stored in AgentState::mcp_statuses, so `/api/mcp-servers` and InitializeResponse._meta serve stale startup snapshots; fix by adding periodic or on-demand refresh logic: implement a background health-check task (spawned at startup) that re-runs connect_server for entries in AgentState::mcp_statuses (or specifically retries entries with connected == false) and updates the McpServerStatus entries atomically, or add lazy refresh when the /api/mcp-servers handler is called to re-check and refresh individual McpServerStatus before returning; update AgentState::mcp_statuses mutation logic accordingly and ensure synchronization (locks/async-safe updates) so the new connected/tool_count/error fields reflect runtime state.src/tools/mod.rs (1)
37-47: ⚖️ Poor tradeoff
buildreturn type change is untested.The existing test suite covers
new,register_builtins, and unknown-tool execution, but nothing exercises the updatedbuildpath or verifies thatmcp_statusesis correctly propagated. A minimal async test would guard against future regressions here.🤖 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/tools/mod.rs` around lines 37 - 47, The build function's changed return type (now returning (Self, Vec<crate::mcp::McpServerStatus>)) is untested; add a minimal async test that calls tools::build with a small mcp_configs map (exercising crate::mcp::load_mcp_tools path), asserts that the returned executor behaves like one created by tools::new/register_builtins (e.g., can execute a known builtin or unknown-tool behavior via executor.register/execute) and also verifies the Vec<McpServerStatus> is returned and has expected content or length; reference tools::build, tools::new, tools::register_builtins, crate::mcp::load_mcp_tools, executor.register and McpServerStatus when locating where to add the test.
🤖 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/acp/mod.rs`:
- Around line 91-93: The _meta["mcp_servers"] entry is being populated with
config-only data from mcp_servers_info(), which differs from the REST payload
shape; change the meta population to serialize the runtime status data in
state_init.mcp_statuses (or convert it into the same Vec<McpServerStatus> shape
used by GET /api/mcp-servers) instead of using mcp_servers_info(), so
InitializeResponse._meta["mcp_servers"] matches the runtime structure returned
by the API.
In `@src/config/types.rs`:
- Around line 102-114: mcp_servers_info currently sets transport = "http"
whenever cfg.command is None, which mislabels entries where both cfg.command and
cfg.url are None; change the logic in mcp_servers_info (function name) to branch
three-ways: if cfg.command.is_some() => transport "stdio" with command, else if
cfg.url.is_some() => transport "http" with url, else => set transport to
something like "unknown" (or empty) and leave both command and url None; apply
the identical three-way fix to the matching if/else in load_mcp_tools in
src/mcp/mod.rs so misconfigured entries are not reported as "http".
- Around line 81-100: The current to_public_json silently returns Value::Null on
serialization failure because it uses
serde_json::to_value(self).unwrap_or_default(), skipping all redaction logic;
change to propagate the serialization error instead by updating to_public_json
to return Result<serde_json::Value, serde_json::Error> (or another appropriate
error type), replace unwrap_or_default with a fallible call to
serde_json::to_value(self)? and return the Err on failure, keep the redaction
logic for "providers" -> "api_key" and "mcp_servers" -> "env" unchanged, and
update all callers to handle the Result (or log the error at call sites) so
serialization failures are not silently swallowed.
In `@src/transport/ws.rs`:
- Around line 59-63: The new REST routes in src/transport/ws.rs (/api/config,
/api/models, /api/skills, /api/tools, /api/mcp-servers) lack authentication and
CORS; add a middleware layer that enforces a simple bearer-token check (read
token from config/env) and return 401 for missing/invalid tokens, and apply it
to the Router that registers config_handler, models_handler, skills_handler,
tools_handler, and mcp_servers_handler; additionally add a CORS layer
(tower_http::cors::CorsLayer) configured to allow the intended browser origins,
methods (GET), and headers (Authorization), and attach it to the same Router so
browser fetch/XHR can succeed while requests from other origins remain
controlled.
---
Nitpick comments:
In `@Dockerfile`:
- Line 49: Update the Dockerfile COPY instruction that copies the config file
(the line containing "COPY config.toml /home/openheim/.openheim/config.toml") to
set explicit ownership by adding the --chown=openheim:openheim flag so the file
is owned by the runtime user; modify that COPY to use --chown=openheim:openheim
before the source path.
In `@src/mcp/mod.rs`:
- Around line 13-25: McpServerStatus is only set once in load_mcp_tools and
stored in AgentState::mcp_statuses, so `/api/mcp-servers` and
InitializeResponse._meta serve stale startup snapshots; fix by adding periodic
or on-demand refresh logic: implement a background health-check task (spawned at
startup) that re-runs connect_server for entries in AgentState::mcp_statuses (or
specifically retries entries with connected == false) and updates the
McpServerStatus entries atomically, or add lazy refresh when the
/api/mcp-servers handler is called to re-check and refresh individual
McpServerStatus before returning; update AgentState::mcp_statuses mutation logic
accordingly and ensure synchronization (locks/async-safe updates) so the new
connected/tool_count/error fields reflect runtime state.
In `@src/tools/mod.rs`:
- Around line 37-47: The build function's changed return type (now returning
(Self, Vec<crate::mcp::McpServerStatus>)) is untested; add a minimal async test
that calls tools::build with a small mcp_configs map (exercising
crate::mcp::load_mcp_tools path), asserts that the returned executor behaves
like one created by tools::new/register_builtins (e.g., can execute a known
builtin or unknown-tool behavior via executor.register/execute) and also
verifies the Vec<McpServerStatus> is returned and has expected content or
length; reference tools::build, tools::new, tools::register_builtins,
crate::mcp::load_mcp_tools, executor.register and McpServerStatus when locating
where to add the test.
🪄 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 Plus
Run ID: f90812e2-c9dc-4139-b294-73213942114d
📒 Files selected for processing (8)
DockerfileREADME.mdsrc/acp/mod.rssrc/config/mod.rssrc/config/types.rssrc/mcp/mod.rssrc/tools/mod.rssrc/transport/ws.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
/api/config,/api/models,/api/skills,/api/tools,/api/mcp-servers) to access server configuration and status information.Documentation