feat: add connection_type, auth_type, state, and virtual_key filters to MCP clients list - #4839
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR expands MCP client listing filters, adds stricter HTTP query parsing, and applies the new filters in the config store query path. ChangesMCP Client Filtering
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
impoiler
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
framework/configstore/store.go (1)
71-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ConnectionTypesdoc comment omitsinprocess.The comment documents connection types as
(http | sse | stdio), but the schema (transports/config.schema.json) constrainsconnection_typetostdio|http|sse|inprocess. The missinginprocessvalue could mislead callers wiring this filter.📝 Suggested fix
- ConnectionTypes []string // exact connection_type filter(s), OR semantics (http | sse | stdio) + ConnectionTypes []string // exact connection_type filter(s), OR semantics (http | sse | stdio | inprocess)As per path instructions, "
mcp.client_configs[].connection_typeis constrained tostdio|http|sse|inprocess... use these exact values when wiring request query params into DB filters."🤖 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 `@framework/configstore/store.go` at line 71, Update the `ConnectionTypes` field comment in `store.go` so it lists all valid `connection_type` values, including `inprocess`, instead of only `(http | sse | stdio)`. Keep the comment aligned with the schema used by `configstore` and `mcp.client_configs[].connection_type`, and make sure any filter wiring references the exact allowed set: `stdio`, `http`, `sse`, and `inprocess`.Source: Path instructions
framework/configstore/rdb.go (1)
1619-1639: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid subquery/switch logic — consider adding table-driven tests.
The three-way switch correctly covers all reachable combinations (
OnlyAllVirtualKeysalone, VK-subset alone, or both combined), andassignedSubis never nil where dereferenced. Given the non-trivial combinatorics (state in/out, code-mode/disabled tri-state, VK-access OR logic), consider adding table-driven test cases inrdb_test.gocovering each filter combination to lock in behavior.As per coding guidelines,
**/*.gochanges should apply "deterministic tests, and table-driven coverage for behavior changes."🤖 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 `@framework/configstore/rdb.go` around lines 1619 - 1639, The VK access filter logic in rdb.go is correct but lacks deterministic coverage for its combinatorial cases. Add table-driven tests in rdb_test.go for the filter behavior around OnlyAllVirtualKeys and VirtualKeyIDs, including each reachable switch branch (all VKs only, subset only, and combined OR case) and any related state/code-mode interactions. Use the existing query/filter entry points in rdb.go and the VK access logic symbols to keep the tests aligned with the implementation.Source: Coding guidelines
transports/bifrost-http/handlers/mcp.go (1)
115-179: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInconsistent validation for new filter facets vs.
limit/offset.
limit/offsetreturn 400 on invalid input, but the new facets don't:
parseBoolQueryArg(Lines 169-179) silently treats a malformed boolean (e.g.,?disabled=maybe) as "no filter" rather than erroring.connection_type/auth_type(Lines 118-119) are passed straight into the SQLINlist with no validation against the schema-defined enum values, so a typo silently yields zero matching rows instead of a clear error.state(Line 128) is similarly unvalidated — an unrecognized value silently no-ops the state filter.Consider validating these against their known enum values (or at least returning 400 for unparseable booleans) for a clearer API contract, consistent with how
limit/offsetare already handled in this same function.As per path instructions, "
mcp.client_configs[].connection_typeis constrained tostdio|http|sse|inprocessandauth_typetonone|headers|oauth|per_user_oauth|per_user_headers; use these exact values when wiring request query params into DB filters."🤖 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 `@transports/bifrost-http/handlers/mcp.go` around lines 115 - 179, The filter parsing in getMCPClientsPaginated is too lenient compared with the existing limit/offset handling. Update parseBoolQueryArg so malformed booleans for code_mode/disabled return a 400 instead of silently becoming “no filter”, and validate connection_type, auth_type, and state against their allowed enum values before building MCPClientsQueryParams. Use the known enum sets in mcp.client_configs and keep the query-to-filter mapping in handler logic so typos surface as explicit bad-request errors rather than empty result sets.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@framework/configstore/rdb.go`:
- Around line 1619-1639: The VK access filter logic in rdb.go is correct but
lacks deterministic coverage for its combinatorial cases. Add table-driven tests
in rdb_test.go for the filter behavior around OnlyAllVirtualKeys and
VirtualKeyIDs, including each reachable switch branch (all VKs only, subset
only, and combined OR case) and any related state/code-mode interactions. Use
the existing query/filter entry points in rdb.go and the VK access logic symbols
to keep the tests aligned with the implementation.
In `@framework/configstore/store.go`:
- Line 71: Update the `ConnectionTypes` field comment in `store.go` so it lists
all valid `connection_type` values, including `inprocess`, instead of only
`(http | sse | stdio)`. Keep the comment aligned with the schema used by
`configstore` and `mcp.client_configs[].connection_type`, and make sure any
filter wiring references the exact allowed set: `stdio`, `http`, `sse`, and
`inprocess`.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 115-179: The filter parsing in getMCPClientsPaginated is too
lenient compared with the existing limit/offset handling. Update
parseBoolQueryArg so malformed booleans for code_mode/disabled return a 400
instead of silently becoming “no filter”, and validate connection_type,
auth_type, and state against their allowed enum values before building
MCPClientsQueryParams. Use the known enum sets in mcp.client_configs and keep
the query-to-filter mapping in handler logic so typos surface as explicit
bad-request errors rather than empty result sets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ceba6700-6bf9-4a45-b0af-de19b6a2385a
📒 Files selected for processing (3)
framework/configstore/rdb.goframework/configstore/store.gotransports/bifrost-http/handlers/mcp.go
1745ae6 to
52f2fa1
Compare
52f2fa1 to
7cdb87a
Compare
7cdb87a to
af49b62
Compare
Merge activity
|
* upstream/dev: feat: add pagination and filtering to MCP clients list endpoint (maximhq#4841) feat: add filter sidebar with faceted filtering to MCP clients page (maximhq#4840) feat: add connection_type, auth_type, state, and virtual_key filters to MCP clients list (maximhq#4839) # Conflicts: # ui/app/workspace/mcp-registry/page.tsx # ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
2 participants
Summary
Extends the
GET /api/mcp/clientsendpoint with richer filtering capabilities, allowing callers to narrow MCP client results by connection type, auth type, code-mode flag, disabled state, runtime connection state (connected/disconnected), and virtual-key access — all within the same paginated database query.Changes
ConnectionTypes,AuthTypes,IsCodeModeClient,Disabled,StateClientIDs,StateInclude,OnlyAllVirtualKeys, andVirtualKeyIDsfields toMCPClientsQueryParams.GetMCPClientsPaginated: IN-list filters for connection/auth types, boolean column filters for code-mode and disabled, a runtime state filter that translates a connected-ID set intoIN/NOT INclauses, and a virtual-key access filter that ORs theallow_on_all_virtual_keysflag with an explicit-assignment subquery over the VK⇄MCP join table.GetMCPClients) to the top ofgetMCPClientsPaginatedso the live connected-ID set is available before the DB query executes, enabling the state filter to be pushed into the paginated query rather than applied post-fetch.connection_type,auth_type,virtual_keys,all_virtual_keys,state,code_mode,disabled) ingetMCPClients, with validation forlimit/offsetand aparseBoolQueryArghelper for optional boolean facets.connectedanddisconnectedstates simultaneously (or neither) is treated as a no-op, avoiding unnecessary constraints.Type of change
Affected areas
How to test
Breaking changes
Related issues
Security considerations
The virtual-key access filter ensures clients are only surfaced when they are either open to all virtual keys or explicitly assigned to one of the requested VK IDs, preventing unintended cross-tenant visibility when filtering by VK.
Checklist
docs/contributing/README.mdand followed the guidelines