controllers/mcp.go (2)

346-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Log errors from describeToolDescription instead of silently swallowing them.

When describeToolDescription fails for a table, the error is silently ignored with continue. This could hide real database issues or permission problems, making the tool list incomplete without any indication of why.

🔍 Proposed fix: log at debug level

 		desc, err := h.describeToolDescription(r, defaultDB, schema, table)
 		if err != nil {
+			slog.Debug("mcp: skipping tool for table", "schema", schema, "table", table, "error", err)
 			continue
 		}

🤖 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 `@controllers/mcp.go` around lines 346 - 348, The error from
describeToolDescription is being swallowed by the continue in the tool
enumeration flow, which can hide real failures. Update the logic around
describeToolDescription in the MCP tool listing path to log the returned err at
debug level before continuing, so failures are observable while preserving the
existing fallback behavior. Use the surrounding tool-building code in
controllers/mcp.go and the describeToolDescription call site as the anchor for
the fix.

---

25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Remove or document the unused builder field.

builder is set in NewMCPHandler (line 36) but never referenced anywhere in mcp.go. Either remove it or add a comment explaining its intended future use.

♻️ Proposed fix: remove the field

 type MCPHandler struct {
 	catalog  adapters.CatalogQuerier
-	builder  adapters.RequestQueryBuilder
 	executor adapters.QueryExecutor
 	db       adapters.DatabaseRegistry
 	singleDB bool
 	pgDB     string
 }

And in the constructor:

 func NewMCPHandler(deps Deps) *MCPHandler {
 	return &MCPHandler{
 		catalog:  deps.Catalog,
-		builder:  deps.Builder,
 		executor: deps.Executor,
 		db:       deps.DB,
 		singleDB: deps.SingleDB,
 		pgDB:     deps.PGDatabase,
 	}
 }

🤖 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 `@controllers/mcp.go` at line 25, The MCP handler currently carries an unused
builder field that is assigned in NewMCPHandler but never referenced anywhere in
mcp.go. Remove the builder field from the handler struct and update
NewMCPHandler to stop wiring it in; if it must stay for future use, add a brief
comment near the MCPHandler definition explaining its purpose, but the preferred
fix is to delete the dead field and constructor assignment.

integration/router/routes_test.go (1)

16-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider verifying response body content for the POST initialize path.

The where parameter ("MCPInitialize") is just a label in DoRequest — no body content is actually asserted since no variadic expectedBody strings are passed. The test only confirms HTTP 200. For the initialize JSON-RPC call, asserting that the response body contains expected fields (e.g., "serverInfo", "capabilities") would strengthen coverage and catch regressions in the response shape.

♻️ Optional: add body content assertions

 func TestMCPRoute(t *testing.T) {
 	base := helpers.ServerURL(t)
 	testutils.DoRequest(t, base+"/_mcp", nil, "GET", http.StatusOK, "MCPDiscovery")
 
 	payload := map[string]any{
 		"jsonrpc": "2.0",
 		"id":      1,
 		"method":  "initialize",
 	}
-	testutils.DoRequest(t, base+"/_mcp", payload, "POST", http.StatusOK, "MCPInitialize")
+	testutils.DoRequest(t, base+"/_mcp", payload, "POST", http.StatusOK, "MCPInitialize", "serverInfo", "capabilities")
 }

🤖 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 `@integration/router/routes_test.go` around lines 16 - 26, Update TestMCPRoute
so the POST initialize request actually asserts response body content instead of
only relying on the DoRequest label "MCPInitialize"; use the existing
testutils.DoRequest call site to add expectedBody checks for key JSON-RPC
response fields such as serverInfo and capabilities, ensuring the initialize
path validates the returned payload shape.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

.cursor/rules/agentic-loop.mdc (1)

83-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Align the adapter-test command with the canonical validation path.

This direct go test invocation drops the repo-standard -race, -count=1, and coverage flags that make test-unit already applies. Either point this section at make test-unit or mirror those flags here so race regressions in adapters/postgres don’t slip through.

🔧 Suggested replacement

-```sh
-go test -timeout 30s -parallel 8 ./adapters/postgres/...
-```
+```sh
+make test-unit
+# or, if you need a focused run:
+go test -timeout 30s -race -count=1 -tags prest_test_hooks -parallel 8 ./adapters/postgres/...
+```

🤖 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 @.cursor/rules/agentic-loop.mdc around lines 83 - 90, The Postgres adapter
test command in the agentic-loop rules is bypassing the repo’s canonical
validation flags, so update the guidance to use make test-unit or match its
behavior in the adapters/postgres test example. Keep the reference anchored
around the Postgres adapter unit tests section and the go test invocation, and
ensure the command includes the standard race, count, and test-hook flags used
by the main unit-test path so regressions are caught consistently.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes