feat: add MCP server for AI agent tool access#248
Merged
Conversation
Add Model Context Protocol (MCP) server integration to Compass, enabling AI coding tools to discover and query assets via the /mcp endpoint. Also adds .mcp.json to .gitignore and updates docker-compose postgres port.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis change adds a new Model Context Protocol (MCP) server implementation to expose the catalog as AI-agent tools. A new Sequence DiagramsequenceDiagram
participant Client as MCP Client
participant Server as MCP Server
participant Handler as Tool Handler
participant Service as Asset Service
participant Formatter as Format Helper
Client->>Server: CallTool(search_assets, {text, types, services})
Server->>Handler: handleSearchAssets(request)
Handler->>Handler: Parse & validate parameters
Handler->>Service: SearchAssets(ctx, SearchConfig)
Service-->>Handler: []SearchResult or error
alt Service Success
Handler->>Formatter: formatSearchResults(results)
Formatter-->>Handler: Formatted markdown text
Handler-->>Server: ToolResultText(content)
else Service Error
Handler-->>Server: ToolResultError(error message)
end
Server-->>Client: CallToolResult
Possibly Related Issues
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🧹 Nitpick comments (6)
docker-compose.yaml (1)
18-18: Port change may require documentation update.Changing the host port from
5432to5433will require existing developers to update their local configuration. Consider adding a note in the PR description or README about this change to prevent confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yaml` at line 18, You changed the Docker Compose port mapping from "5432" to "5433" (line "- 5433:5432") which affects local developer setups; update the README/CONTRIBUTING and the PR description to note the new host port, include the exact mapping "5433:5432", list steps to adjust local configs (e.g., DB clients or env vars pointing to localhost:5432 -> localhost:5433), and mention how to revert or run on the old port if needed.internal/mcp/handlers.go (1)
98-105: Consider adding an upper bound forsizeparameter.The handler accepts any positive value for
sizewithout an upper limit. A malicious or mistaken request with a very largesizecould strain backend resources. Consider capping it to a reasonable maximum (e.g., 100 or 500).Proposed fix
func (s *Server) handleGetAllAssets(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { size := mcp.ParseInt(req, "size", 20) + if size > 500 { + size = 500 + } offset := mcp.ParseInt(req, "offset", 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/handlers.go` around lines 98 - 105, The handler handleGetAllAssets allows an unbounded size from mcp.ParseInt which can be abused; update the code that computes size (variable size in handleGetAllAssets) to enforce a maximum (e.g., const maxSize = 100) and clamp/validate the parsed value (ensure non-negative and if size > maxSize set size = maxSize) before passing it into asset.NewFilterBuilder().Size(size); keep the default behavior when ParseInt yields no value and add a brief comment about the cap.internal/mcp/format.go (1)
59-68: Consider handling empty column names more gracefully.When
nameordata_typeare missing or not strings, the output could produce entries like"- ()". While this defensive approach is acceptable for unstructured data, you might consider skipping columns without a name.Optional improvement
for _, col := range columns { name, _ := col["name"].(string) + if name == "" { + continue + } dataType, _ := col["data_type"].(string) desc, _ := col["description"].(string)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/format.go` around lines 59 - 68, Skip rendering columns that lack a valid non-empty name: inside the loop that iterates over columns (variables name, dataType, desc and uses fmt.Fprintf to write to buffer b), first validate that name is a non-empty string and if not continue to the next column; for dataType you can default to a placeholder like "unknown" when it's missing or not a string before calling fmt.Fprintf so you don't emit entries like "- ()".internal/mcp/handlers_test.go (2)
112-117: Avoid brittle direct indexing in filter assertions.Lines 112-117 directly index map/slice entries; when mapping breaks, this can panic instead of producing clear assertion output.
🔧 Safer assertions
- if cfg.Filters["type"][0] != "table" { - t.Errorf("expected type filter 'table', got %v", cfg.Filters["type"]) - } - if cfg.Filters["service"][0] != "bigquery" { - t.Errorf("expected service filter 'bigquery', got %v", cfg.Filters["service"]) - } + gotTypes := cfg.Filters["type"] + if len(gotTypes) != 1 || gotTypes[0] != "table" { + t.Errorf("expected type filter ['table'], got %v", gotTypes) + } + gotServices := cfg.Filters["service"] + if len(gotServices) != 1 || gotServices[0] != "bigquery" { + t.Errorf("expected service filter ['bigquery'], got %v", gotServices) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/handlers_test.go` around lines 112 - 117, The assertions in the test use brittle direct indexing (cfg.Filters["type"][0] and cfg.Filters["service"][0]) which can panic; update the checks on cfg and its Filters to first assert the key exists and the slice has expected length, then assert the slice contains the expected value (e.g., check ok := cfg.Filters["type"]; if !ok || len(ok) == 0 { t.Fatalf(...) } else if ok[0] != "table" { t.Errorf(...) }) or use a safe helper/assertion (assert.Contains or reflect.DeepEqual) to compare the slices without direct indexing; apply the same pattern for "service" to avoid panics and provide clear failure messages.
165-166: Assert handler argument passthrough in mocks.Line 165 (
id) and Line 205 (urn) are not validated in the service doubles. Adding explicit checks will catch request parsing/mapping regressions earlier.Also applies to: 205-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/handlers_test.go` around lines 165 - 166, In the test doubles (e.g., getAssetByIDFunc and the analogous getAssetByURNFunc/mock), add explicit assertions on the incoming handler arguments by comparing the received id/urn to the expected test value and returning an error if they don't match; update the mock implementations (getAssetByIDFunc and the mock for URN lookup) to validate the argument before constructing the asset so request parsing/mapping regressions are caught early.internal/mcp/server.go (1)
30-34: Fail fast on invalid constructor dependencies.Line 30 currently accepts nil dependencies silently; a miswired bootstrap can fail later at request time. Add early validation in
Newfor clearer startup-time failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcp/server.go` around lines 30 - 34, Modify the constructor New to fail fast by changing its signature to return (*Server, error), validate that the incoming assetSvc (AssetService) and ns (*namespace.Namespace) are non-nil, and return a descriptive error (e.g., fmt.Errorf("New: nil AssetService") or fmt.Errorf("New: nil namespace.Namespace")) if either is nil; on success return the constructed *Server and nil error. Update all call sites to handle the new (*Server, error) return and propagate or handle the error at bootstrap time so miswired dependencies fail at startup rather than at request time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/mcp/server_test.go`:
- Around line 25-42: The mock getAssetByIDFunc currently ignores its id argument
which hides request-mapping regressions; update getAssetByIDFunc (and the
similar mock used around lines 138-143) to explicitly assert the incoming id
equals the expected URN (e.g., "urn:bq:dataset.orders" or the test's expectedID
variable) and make the mock fail the test (or return a distinct error) if it
doesn't match so the e2e test validates that GetAssetByID is called with the
URN, not some other identifier.
---
Nitpick comments:
In `@docker-compose.yaml`:
- Line 18: You changed the Docker Compose port mapping from "5432" to "5433"
(line "- 5433:5432") which affects local developer setups; update the
README/CONTRIBUTING and the PR description to note the new host port, include
the exact mapping "5433:5432", list steps to adjust local configs (e.g., DB
clients or env vars pointing to localhost:5432 -> localhost:5433), and mention
how to revert or run on the old port if needed.
In `@internal/mcp/format.go`:
- Around line 59-68: Skip rendering columns that lack a valid non-empty name:
inside the loop that iterates over columns (variables name, dataType, desc and
uses fmt.Fprintf to write to buffer b), first validate that name is a non-empty
string and if not continue to the next column; for dataType you can default to a
placeholder like "unknown" when it's missing or not a string before calling
fmt.Fprintf so you don't emit entries like "- ()".
In `@internal/mcp/handlers_test.go`:
- Around line 112-117: The assertions in the test use brittle direct indexing
(cfg.Filters["type"][0] and cfg.Filters["service"][0]) which can panic; update
the checks on cfg and its Filters to first assert the key exists and the slice
has expected length, then assert the slice contains the expected value (e.g.,
check ok := cfg.Filters["type"]; if !ok || len(ok) == 0 { t.Fatalf(...) } else
if ok[0] != "table" { t.Errorf(...) }) or use a safe helper/assertion
(assert.Contains or reflect.DeepEqual) to compare the slices without direct
indexing; apply the same pattern for "service" to avoid panics and provide clear
failure messages.
- Around line 165-166: In the test doubles (e.g., getAssetByIDFunc and the
analogous getAssetByURNFunc/mock), add explicit assertions on the incoming
handler arguments by comparing the received id/urn to the expected test value
and returning an error if they don't match; update the mock implementations
(getAssetByIDFunc and the mock for URN lookup) to validate the argument before
constructing the asset so request parsing/mapping regressions are caught early.
In `@internal/mcp/handlers.go`:
- Around line 98-105: The handler handleGetAllAssets allows an unbounded size
from mcp.ParseInt which can be abused; update the code that computes size
(variable size in handleGetAllAssets) to enforce a maximum (e.g., const maxSize
= 100) and clamp/validate the parsed value (ensure non-negative and if size >
maxSize set size = maxSize) before passing it into
asset.NewFilterBuilder().Size(size); keep the default behavior when ParseInt
yields no value and add a brief comment about the cap.
In `@internal/mcp/server.go`:
- Around line 30-34: Modify the constructor New to fail fast by changing its
signature to return (*Server, error), validate that the incoming assetSvc
(AssetService) and ns (*namespace.Namespace) are non-nil, and return a
descriptive error (e.g., fmt.Errorf("New: nil AssetService") or fmt.Errorf("New:
nil namespace.Namespace")) if either is nil; on success return the constructed
*Server and nil error. Update all call sites to handle the new (*Server, error)
return and propagate or handle the error at bootstrap time so miswired
dependencies fail at startup rather than at request time.
🪄 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: 91b10449-e6b8-4af1-85f2-ca9a6ffc50cc
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
.gitignoredocker-compose.yamlgo.modinternal/mcp/format.gointernal/mcp/handlers.gointernal/mcp/handlers_test.gointernal/mcp/server.gointernal/mcp/server_test.gointernal/mcp/tools.gointernal/server/bootstrap.gointernal/server/server.go
Comment on lines
+25
to
+42
| getAssetByIDFunc: func(_ context.Context, id string) (asset.Asset, error) { | ||
| return asset.Asset{ | ||
| ID: "1", | ||
| URN: "urn:bq:dataset.orders", | ||
| Name: "orders", | ||
| Type: "table", | ||
| Service: "bigquery", | ||
| Description: "Main orders table with all customer transactions", | ||
| Owners: []user.User{{Email: "alice@company.com"}, {Email: "bob@company.com"}}, | ||
| Data: map[string]interface{}{ | ||
| "columns": []interface{}{ | ||
| map[string]interface{}{"name": "order_id", "data_type": "INTEGER", "description": "Primary key"}, | ||
| map[string]interface{}{"name": "customer_id", "data_type": "INTEGER", "description": "FK to customers"}, | ||
| map[string]interface{}{"name": "amount", "data_type": "FLOAT", "description": "Order total in USD"}, | ||
| }, | ||
| }, | ||
| }, nil | ||
| }, |
There was a problem hiding this comment.
Tighten get_asset identifier contract in the e2e test.
Line 142 sends a URN in the id argument, and Lines 25-42 don’t assert what was passed into GetAssetByID. This can hide request-mapping regressions.
🔧 Proposed test hardening
getAssetByIDFunc: func(_ context.Context, id string) (asset.Asset, error) {
+ if id != "1" {
+ t.Errorf("expected id '1', got '%s'", id)
+ }
return asset.Asset{
ID: "1",
URN: "urn:bq:dataset.orders",
@@
t.Run("get_asset", func(t *testing.T) {
result, err := client.CallTool(ctx, mcp.CallToolRequest{
Params: mcp.CallToolParams{
Name: "get_asset",
- Arguments: map[string]any{"id": "urn:bq:dataset.orders"},
+ Arguments: map[string]any{"id": "1"},
},
})Also applies to: 138-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/mcp/server_test.go` around lines 25 - 42, The mock getAssetByIDFunc
currently ignores its id argument which hides request-mapping regressions;
update getAssetByIDFunc (and the similar mock used around lines 138-143) to
explicitly assert the incoming id equals the expected URN (e.g.,
"urn:bq:dataset.orders" or the test's expectedID variable) and make the mock
fail the test (or return a distinct error) if it doesn't match so the e2e test
validates that GetAssetByID is called with the URN, not some other identifier.
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
/mcpendpoint.mcp.jsonto.gitignoreand update docker-compose postgres port to avoid conflictsTest plan
/mcpwhen Compass server is runninginternal/mcp/