feat(cli): add tool catalog and risk config#274
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a tool-level catalog and client connection configuration feature for MCP servers. It introduces a new ToolRiskLevel field, a filterable /api/runtime/tools API endpoint, and new CLI commands (mcp-runtime catalog and mcp-runtime server connect-config). The UI is also updated with a tool catalog table, search capabilities, and risk filtering. Feedback on these changes identifies an issue in the new tools API handler where a namespace authorization failure incorrectly returns a 500 Internal Server Error instead of a 403 Forbidden status.
| servers, err := s.visibleServers(ctx, control, p, strings.TrimSpace(r.URL.Query().Get("namespace"))) | ||
| if err != nil { | ||
| writeAPIError(w, http.StatusInternalServerError, "failed to list servers") | ||
| return | ||
| } |
There was a problem hiding this comment.
In HandleRuntimeTools, if visibleServers returns errForbiddenNamespace, the handler currently returns a 500 Internal Server Error with the message "failed to list servers". It should instead return a 403 Forbidden with the message "forbidden namespace" to match the behavior in handleRuntimeServerList and properly reflect the authorization failure.
servers, err := s.visibleServers(ctx, control, p, strings.TrimSpace(r.URL.Query().Get("namespace")))
if err == errForbiddenNamespace {
writeAPIError(w, http.StatusForbidden, "forbidden namespace")
return
}
if err != nil {
writeAPIError(w, http.StatusInternalServerError, "failed to list servers")
return
}![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: 13bd342ba5
ℹ️ 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".
|
|
||
| async function loadToolCatalog() { | ||
| try { | ||
| const data = await fetchJSON(scopedPath("/runtime/tools")); |
There was a problem hiding this comment.
In public-catalog mode, signed-out users load the dashboard through the UI proxy, but this new /runtime/tools fetch is not considered a public catalog request: isPublicCatalogAPIRequest still only matches /runtime/servers in services/ui/main.go:1117-1122. As a result the proxy returns 401 for the new tool table even though the API route itself allows public catalog reads, so anonymous public catalog pages always show an empty/failed tools catalog. Update the proxy allowlist/default namespace handling to include /runtime/tools as well.
Useful? React with 👍 / 👎.
| RequiredTrust: string(mcpv1alpha1.TrustLevelLow), | ||
| RiskLevel: string(mcpv1alpha1.ToolRiskLevelLow), |
There was a problem hiding this comment.
When live inventory reports a tool that is not declared in the MCPServer spec, the catalog marks it ungoverned but also assigns required_trust=low and risk_level=low. For servers with undeclared write/destructive tools, this makes the risk filter and badges present an unknown/ungoverned tool as low risk, which can mislead operators reviewing the catalog. Leave these fields empty/unknown (or compute them only from declared metadata) instead of defaulting ungoverned tools to low.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| writeAPIError(w, http.StatusInternalServerError, "failed to list servers") | ||
| return |
There was a problem hiding this comment.
If a non-admin calls the new tools catalog with a namespace they cannot read, visibleServers returns errForbiddenNamespace, but this handler converts every error into a 500. The existing server-list path handles the same error as 403, so catalog tools --namespace <other-team> now reports an internal server error for an authorization denial. Mirror the server-list handling here and return 403 forbidden namespace for errForbiddenNamespace.
Useful? React with 👍 / 👎.
Agent-Hellboy
force-pushed
the
feature/tool-catalog-risk-config
branch
from
5367466 to
3a10f6b
Compare
Remove docs/next-product-picks-plan.md and its mkdocs nav entry; it is an internal planning note unrelated to the tool catalog feature. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Validate riskLevel enum, honor catalog tool --query, and pick connect URLs deterministically with a non-empty guard. Revert unrelated AGENTS.md edits. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pushed review nits (also on `fix/pr-274-review`):
|
1 participant
Fixes #315