internal/trino/client.go (1)

300-322: Optional hardening: quote identifiers in SHOW STATS table reference.

Currently table identifiers are interpolated directly. While we restrict to read-only statements, defensive quoting of identifiers (catalog/schema/table parts) avoids malformed identifiers or accidental token injection.

Consider a helper like:

• Split the fully-qualified name into parts, escape embedded double quotes by doubling them, wrap each with ".

Example inside this method:

• For 3 parts: SHOW STATS FOR "cat"."sch"."tbl"
• For 2 parts: SHOW STATS FOR "cat".<parts[0]>."<parts[1]>"
• For 1 part: SHOW STATS FOR "cat"."sch"."tbl"

I can draft a small quoteIdent helper if you want to proceed.

README.md (1)

586-675: Doc-response shape likely does not match actual server output for EXPLAIN.

Handlers/client return raw rows/columns from Trino (marshaled JSON), typically a single text column "Query Plan" for EXPLAIN/EXPLAIN (TYPE DISTRIBUTED). The documented response under explain_query shows a structured execution_plan array with stages/operations, which the code does not produce.

Please either:

• Update the docs to reflect the actual raw EXPLAIN output (e.g., {"columns":["Query Plan"], "data":[["Fragment 0 ...\n..."]]}), or
• Enhance the handler to parse Trino’s plan text into the documented structured execution_plan.

Additionally, the parameter description says “DISTRIBUTED (default)”. With current code, default is Trino’s LOGICAL plan unless a format is provided. Align the description after deciding on code behavior. I can prepare a doc update patch once you decide.

internal/mcp/handlers.go (2)

197-236: Minor validation hardening: reject empty query strings.

The handler checks type but not emptiness. Empty/whitespace-only queries will produce avoidable errors downstream.

Add a trim/emptiness check after extracting query:

• If empty, return a tool error “query parameter must not be empty”.

I can provide a small patch (will require adding strings import) if you want it in this PR.

---

238-282: Minor validation hardening: reject empty table names.

Similarly, validate the required table parameter for emptiness after type assertion to avoid downstream SQL errors.

Add a whitespace trim and ensure non-empty before proceeding. I can send a small patch (will require strings import).

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to internal/handlers/trino_handlers.go : The handler layer in internal/handlers/trino_handlers.go must implement MCP tools with JSON response formatting, parameter validation, error handling, consistent logging, and tool result standardization.

Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to internal/handlers/trino_handlers.go : The handler layer in internal/handlers/trino_handlers.go must implement MCP tools with JSON response formatting, parameter validation, error handling, consistent logging, and tool result standardization.

Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tool endpoints (execute_query, list_catalogs, list_schemas, list_tables, get_table_schema) must return JSON-formatted responses and handle parameter validation.

Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tool endpoints must handle parameter validation and error handling consistently.

Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to internal/handlers/trino_handlers.go : Tool result standardization must be enforced in handler responses.

internal/mcp/handlers.go (1)

308-311: Resolved: Schema-browsing description now matches behavior (single catalog).

This addresses the earlier review about “all catalogs” wording. Thanks for fixing.

internal/mcp/handlers.go (5)

197-236: Validate format against allowed values in ExplainQuery.

Prevent avoidable round-trips by rejecting invalid plan types early.

Apply this diff inside ExplainQuery to validate format:

   // Extract optional format parameter
   var format string
   if formatParam, ok := args["format"].(string); ok {
     format = formatParam
   }
 
+  // Validate format value (if provided)
+  if format != "" {
+    switch format {
+    case "LOGICAL", "DISTRIBUTED", "VALIDATE", "IO":
+      // ok
+    default:
+      mcpErr := fmt.Errorf("invalid format %q; must be one of LOGICAL, DISTRIBUTED, VALIDATE, IO", format)
+      return mcp.NewToolResultErrorFromErr(mcpErr.Error(), mcpErr), nil
+    }
+  }
 
   // Execute the explain query
   results, err := h.TrinoClient.ExplainQuery(query, format)

---

228-236: Optional: Standardize tool responses with a simple envelope.

You currently return raw JSON for each tool. For consistency across tools and to make future evolution easier, consider a small envelope like {"data": ...}. This aligns with the “tool result standardization” preference in prior guidance.

Outside this block, you could add:

type toolResponse struct {
  Data any `json:"data"`
}

And replace:

• json.MarshalIndent(results, "", " ")
  with:
• json.MarshalIndent(toolResponse{Data: results}, "", " ")

---

241-246: Minor: Factor out repeated argument parsing.

The map assertion + error pattern is repeated across handlers. A tiny helper reduces duplication and keeps error messages consistent.

Example helper (place at package level):

func parseArgs(req mcp.CallToolRequest) (map[string]interface{}, error) {
  args, ok := req.Params.Arguments.(map[string]interface{})
  if !ok {
    return nil, fmt.Errorf("invalid arguments format: got %T", req.Params.Arguments)
  }
  return args, nil
}

Then replace this block with:

args, err := parseArgs(request)
if err != nil {
  return mcp.NewToolResultErrorFromErr(err.Error(), err), nil
}

---

300-304: Nit: Clarify overlap between execute_query and explain_query.

Since a dedicated explain_query tool now exists, hint users to prefer it for plan analysis.

-    mcp.WithDescription("Execute SQL queries on Trino's fast distributed query engine for big data analytics. Run SELECT, SHOW, DESCRIBE, EXPLAIN statements across multiple data sources simultaneously. Perfect for complex analytics, aggregations, joins, and cross-system data exploration on large datasets."),
+    mcp.WithDescription("Execute SQL queries on Trino's fast distributed query engine for big data analytics. Run SELECT, SHOW, DESCRIBE, EXPLAIN statements across multiple data sources simultaneously. Perfect for complex analytics, aggregations, joins, and cross-system data exploration on large datasets. For plan analysis without execution, prefer the `explain_query` tool."),

---

314-317: Align list_tables description with actual scoping behavior.

“Browse all available data across the distributed system” over-promises; the handler lists tables for one catalog/schema (with optional defaults).

-    mcp.WithDescription("Discover tables and views available for querying in Trino schemas. Essential for finding datasets to analyze. Can scope to specific catalog/schema or browse all available data across the distributed system."),
-    mcp.WithString("catalog", mcp.Description("Trino catalog name (optional)")),
-    mcp.WithString("schema", mcp.Description("Schema name within catalog (optional)"))),
+    mcp.WithDescription("Discover tables and views available for querying in Trino schemas. Essential for finding datasets to analyze. Scope to a specific catalog/schema; if omitted, the server default(s) are used."),
+    mcp.WithString("catalog", mcp.Description("Trino catalog name (optional; defaults to server configuration if omitted)")),
+    mcp.WithString("schema", mcp.Description("Schema name within catalog (optional; defaults to server configuration if omitted)"))),

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to internal/handlers/trino_handlers.go : The handler layer in internal/handlers/trino_handlers.go must implement MCP tools with JSON response formatting, parameter validation, error handling, consistent logging, and tool result standardization.

Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to internal/handlers/trino_handlers.go : The handler layer in internal/handlers/trino_handlers.go must implement MCP tools with JSON response formatting, parameter validation, error handling, consistent logging, and tool result standardization.

Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tool endpoints (execute_query, list_catalogs, list_schemas, list_tables, get_table_schema) must return JSON-formatted responses and handle parameter validation.

Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-25T23:51:10.585Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tool endpoints must handle parameter validation and error handling consistently.