refactor(mcp): address review feedback for read-only tool registration

aprimakina · claude · aprimakina · commit 8fca2dcfc3f2 · 2026-06-25T19:44:01.000+02:00
- Pass readOnly as a parameter through registerTools/registerServiceTools/
  registerDatabaseTools instead of storing it on the Server struct, so the
  stale-once-set value can't leak to tool handlers.
- Remove the deny-tiger-cli.sh hook and .claude/settings.json; this dev-only
  tooling shouldn't be checked in repo-wide.
- Update README read_only/TIGER_READ_ONLY docs to match spec_mcp.md: write
  MCP tools are not registered (absent from tools/list), not erroring.

Co-Authored-By: Claude Opus 4.8 &lt;noreply@anthropic.com&gt;
diff --git a/.claude/hooks/deny-tiger-cli.sh b/.claude/hooks/deny-tiger-cli.sh
diff --git a/.claude/settings.json b/.claude/settings.json
diff --git a/README.md b/README.md
@@ -248,7 +248,7 @@ All configuration options can be set via `tiger config set <key> <value>`:
 - `mcp_max_rows` - Maximum number of rows the `db_execute_query` MCP tool returns per result set before truncating, to limit how much data lands in an AI agent's context. Only applies to the MCP tool, not CLI commands. Default: `100`
 - `output` - Output format: `json`, `yaml`, or `table` (default: `table`)
 - `password_storage` - Password storage method: `keyring`, `pgpass`, or `none` (default: `keyring`)
-- `read_only` - When `true`, mutating operations are refused: the `tiger service create`/`fork`/`start`/`stop`/`resize`/`update-password`/`delete` CLI commands and their MCP equivalents return an error, and `tiger db connect`, `tiger db connection-string`, and the `db_execute_query` MCP tool open the database session in Tiger Cloud's immutable read-only mode (writes and DDL are rejected by the server). Read commands/tools are unaffected — `tiger db schema` and the `db_schema` MCP tool always open a read-only session regardless of this setting. Default: `false`.
+- `read_only` - When `true`, mutating operations are refused: the `tiger service create`/`fork`/`start`/`stop`/`resize`/`update-password`/`delete` CLI commands return an error, and their MCP equivalents are not registered, so they don't appear in `tools/list` and can't be called. `tiger db connect`, `tiger db connection-string`, and the `db_execute_query` MCP tool open the database session in Tiger Cloud's immutable read-only mode (writes and DDL are rejected by the server). Read commands/tools are unaffected — `tiger db schema` and the `db_schema` MCP tool always open a read-only session regardless of this setting. Default: `false`.
 - `service_id` - Default service ID
 - `version_check` - When `true`, the CLI checks for a newer version on each invocation (in an interactive terminal) and prints a notice if one is available. Set to `false` to disable. Default: `true`.
 
@@ -263,7 +263,7 @@ Environment variables override configuration file values. All variables use the
 - `TIGER_DOCS_MCP` - Enable/disable docs MCP proxy
 - `TIGER_OUTPUT` - Output format: `json`, `yaml`, or `table`
 - `TIGER_PASSWORD_STORAGE` - Password storage method: `keyring`, `pgpass`, or `none`
-- `TIGER_READ_ONLY` - When `true`, write/destructive CLI commands and Tiger MCP tools refuse to run, and `db_execute_query` runs against a read-only database connection
+- `TIGER_READ_ONLY` - When `true`, write/destructive CLI commands return an error, the corresponding Tiger MCP write tools are not registered, and `db_execute_query` runs against a read-only database connection
 - `TIGER_PUBLIC_KEY` - Public key to use for authentication (takes priority over stored credentials)
 - `TIGER_SECRET_KEY` - Secret key to use for authentication (takes priority over stored credentials)
 - `TIGER_SERVICE_ID` - Default service ID
diff --git a/internal/tiger/mcp/db_tools.go b/internal/tiger/mcp/db_tools.go
@@ -147,8 +147,8 @@ func (DBExecuteQueryOutput) Schema() *jsonschema.Schema {
 }
 
 // registerDatabaseTools registers database operation tools with comprehensive schemas and descriptions
-func (s *Server) registerDatabaseTools() {
-	addTool(s, &mcp.Tool{
+func (s *Server) registerDatabaseTools(readOnly bool) {
+	addTool(s, readOnly, &mcp.Tool{
 		Name:  toolDBExecuteQuery,
 		Title: "Execute SQL Query",
 		Description: `Execute SQL queries against a service database.
diff --git a/internal/tiger/mcp/server.go b/internal/tiger/mcp/server.go
@@ -27,15 +27,14 @@ const (
 type Server struct {
 	mcpServer       *mcp.Server
 	docsProxyClient *ProxyClient
-
-	// readOnly is captured from config at construction time. Handlers still call
-	// common.CheckReadOnly in case read_only is toggled on mid-session.
-	readOnly bool
 }
 
 // addTool registers an MCP tool, skipping readOnlyGatedTools in read-only mode.
-func addTool[In, Out any](s *Server, t *mcp.Tool, h mcp.ToolHandlerFor[In, Out]) {
-	if s.readOnly && slices.Contains(readOnlyGatedTools, t.Name) {
+// readOnly is passed in (rather than stored on Server) so that this stale-once-set
+// value never leaks to tool handlers; handlers re-check the live config via
+// common.CheckReadOnly in case read_only is toggled on mid-session.
+func addTool[In, Out any](s *Server, readOnly bool, t *mcp.Tool, h mcp.ToolHandlerFor[In, Out]) {
+	if readOnly && slices.Contains(readOnlyGatedTools, t.Name) {
 		logging.Debug("Skipping write tool in read-only mode", zap.String("tool", t.Name))
 		return
 	}
@@ -69,11 +68,12 @@ func NewServer(ctx context.Context, cfg *config.Config) (*Server, error) {
 
 	server := &Server{
 		mcpServer: mcpServer,
-		readOnly:  cfg != nil && cfg.ReadOnly,
 	}
 
-	// Register all tools (including proxied docs tools)
-	server.registerTools(ctx)
+	// Register all tools (including proxied docs tools). readOnly is captured
+	// here and threaded through registration only.
+	readOnly := cfg != nil && cfg.ReadOnly
+	server.registerTools(ctx, readOnly)
 
 	// Add analytics tracking middleware
 	server.mcpServer.AddReceivingMiddleware(server.analyticsMiddleware)
@@ -96,12 +96,12 @@ func (s *Server) HTTPHandler() http.Handler {
 }
 
 // registerTools registers all available MCP tools
-func (s *Server) registerTools(ctx context.Context) {
+func (s *Server) registerTools(ctx context.Context, readOnly bool) {
 	// Service management tools
-	s.registerServiceTools()
+	s.registerServiceTools(readOnly)
 
 	// Database operation tools
-	s.registerDatabaseTools()
+	s.registerDatabaseTools(readOnly)
 
 	// TODO: Register more tool groups
 
diff --git a/internal/tiger/mcp/server_test.go b/internal/tiger/mcp/server_test.go
@@ -33,10 +33,9 @@ func registeredToolNames(t *testing.T, readOnly bool) []string {
 			Title:   serverTitle,
 			Version: config.Version,
 		}, nil),
-		readOnly: readOnly,
 	}
-	s.registerServiceTools()
-	s.registerDatabaseTools()
+	s.registerServiceTools(readOnly)
+	s.registerDatabaseTools(readOnly)
 
 	clientTransport, serverTransport := mcp.NewInMemoryTransports()
 
diff --git a/internal/tiger/mcp/service_tools.go b/internal/tiger/mcp/service_tools.go
@@ -418,9 +418,9 @@ func (ServiceLogsOutput) Schema() *jsonschema.Schema {
 }
 
 // registerServiceTools registers service management tools with comprehensive schemas and descriptions
-func (s *Server) registerServiceTools() {
+func (s *Server) registerServiceTools(readOnly bool) {
 	// service_list
-	addTool(s, &mcp.Tool{
+	addTool(s, readOnly, &mcp.Tool{
 		Name:  toolServiceList,
 		Title: "List Database Services",
 		Description: "List all database services in your Tiger Cloud project. " +
@@ -435,7 +435,7 @@ func (s *Server) registerServiceTools() {
 	}, s.handleServiceList)
 
 	// service_get
-	addTool(s, &mcp.Tool{
+	addTool(s, readOnly, &mcp.Tool{
 		Name:  toolServiceGet,
 		Title: "Get Service Details",
 		Description: "Get detailed information for a specific database service. " +
@@ -450,7 +450,7 @@ func (s *Server) registerServiceTools() {
 	}, s.handleServiceGet)
 
 	// service_create
-	addTool(s, &mcp.Tool{
+	addTool(s, readOnly, &mcp.Tool{
 		Name:  toolServiceCreate,
 		Title: "Create Database Service",
 		Description: `Create a new database service in Tiger Cloud with specified type, compute resources, region, and HA options.
@@ -472,7 +472,7 @@ WARNING: Creates billable resources.`,
 	}, s.handleServiceCreate)
 
 	// service_fork
-	addTool(s, &mcp.Tool{
+	addTool(s, readOnly, &mcp.Tool{
 		Name:  toolServiceFork,
 		Title: "Fork Database Service",
 		Description: `Fork an existing database service to create a new independent copy.
@@ -500,7 +500,7 @@ WARNING: Creates billable resources.`,
 	}, s.handleServiceFork)
 
 	// service_update_password
-	addTool(s, &mcp.Tool{
+	addTool(s, readOnly, &mcp.Tool{
 		Name:  toolServiceUpdatePassword,
 		Title: "Update Service Password",
 		Description: "Update master password for 'tsdbadmin' user of a database service. " +
@@ -517,7 +517,7 @@ WARNING: Creates billable resources.`,
 	}, s.handleServiceUpdatePassword)
 
 	// service_start
-	addTool(s, &mcp.Tool{
+	addTool(s, readOnly, &mcp.Tool{
 		Name:  toolServiceStart,
 		Title: "Start Database Service",
 		Description: `Start a stopped database service.
@@ -535,7 +535,7 @@ This operation starts a service that is currently in a stopped/paused state. The
 	}, s.handleServiceStart)
 
 	// service_stop
-	addTool(s, &mcp.Tool{
+	addTool(s, readOnly, &mcp.Tool{
 		Name:  toolServiceStop,
 		Title: "Stop Database Service",
 		Description: `Stop a running database service.
@@ -553,7 +553,7 @@ This operation stops a service that is currently running. The service will trans
 	}, s.handleServiceStop)
 
 	// service_resize
-	addTool(s, &mcp.Tool{
+	addTool(s, readOnly, &mcp.Tool{
 		Name:  toolServiceResize,
 		Title: "Resize Database Service",
 		Description: `Resize a database service by changing its CPU and memory allocation.
@@ -574,7 +574,7 @@ WARNING: Creates billable resource changes. Increasing resources will increase c
 	}, s.handleServiceResize)
 
 	// service_logs
-	addTool(s, &mcp.Tool{
+	addTool(s, readOnly, &mcp.Tool{
 		Name:  toolServiceLogs,
 		Title: "Get Service Logs",
 		Description: `View logs for a database service.
