From 7c04e4bc964d84a7ff100c1bbf42d90b06ca0dd3 Mon Sep 17 00:00:00 2001 From: Alexandra Primakina Date: Fri, 12 Jun 2026 20:07:28 +0200 Subject: [PATCH] feat(config): enable read-only mode by default for new installations Flip the read_only default to true so fresh installs refuse mutating operations (CLI service commands, MCP write tools) and open database sessions read-only until the user opts in with `tiger config set read_only false`. Existing installations are grandfathered: older CLI versions only wrote read_only to the config file when set explicitly, so a config file without the key predates the default flip and MigrateReadOnly (same in-memory-shim pattern as MigrateVersionCheck) resolves it to false. To keep that detection sound, every config file written by Set/Unset/Reset now records read_only explicitly; unsetting or resetting restores the current default (true). The integration test suite sets TIGER_READ_ONLY=false since it exercises real mutating commands, and ErrReadOnly now tells users how to disable read-only mode. Co-Authored-By: Claude Fable 5 --- CLAUDE.md | 2 + README.md | 4 +- internal/tiger/cmd/config.go | 5 + internal/tiger/cmd/integration_test.go | 4 + internal/tiger/config/config.go | 44 ++++++- internal/tiger/config/config_test.go | 155 ++++++++++++++++++++++++- specs/spec.md | 2 +- specs/spec_mcp.md | 2 +- 8 files changed, 211 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b9be8242..cd7fa9eb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -322,6 +322,8 @@ The Tiger MCP server provides AI assistants with programmatic access to Tiger re Write/destructive MCP tool handlers and CLI command `RunE` functions must call `common.CheckReadOnly(cfg.Config)` (defined in `internal/tiger/common/errors.go`) immediately after `common.LoadConfig(ctx)`. When `cfg.ReadOnly` is `true`, the call returns `common.ErrReadOnly` and the API client is never invoked. The gated CLI commands today are `service create`, `service fork`, `service start`, `service stop`, `service resize`, `service update-password`, and `service delete`. +`read_only` defaults to `true` for new installations only: a config file without the key predates the default flip and is grandfathered to `false` by `MigrateReadOnly` (same in-memory-shim pattern as `MigrateVersionCheck`). To keep that detection sound, every config file written by `Set`/`Unset`/`Reset` records `read_only` explicitly (see `materializeReadOnly`) — preserve this invariant in any new config-file write path. In tests, a config file written via `config.UseTestConfig` without `read_only` therefore behaves like a grandfathered install (`read_only=false`). + **Tool Definition Pattern:** When defining MCP tools, we use a pattern that balances type safety with schema flexibility: diff --git a/README.md b/README.md index 5ae75516..debdcec0 100644 --- a/README.md +++ b/README.md @@ -245,7 +245,7 @@ All configuration options can be set via `tiger config set `: - `docs_mcp` - Enable/disable docs MCP proxy (default: `true`) - `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. Default: `false`. +- `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. Default: `true` for new installations; config files created by older CLI versions keep the previous default of `false`. Set `read_only` to `false` to enable mutating operations. - `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`. @@ -260,7 +260,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` (the default for new installations), write/destructive CLI commands and Tiger MCP tools refuse to run, 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/cmd/config.go b/internal/tiger/cmd/config.go index afff70b8..1c8f53af 100644 --- a/internal/tiger/cmd/config.go +++ b/internal/tiger/cmd/config.go @@ -52,6 +52,11 @@ func buildConfigShowCmd() *cobra.Command { return err } config.MigrateVersionCheck(v) + if !noDefaults { + // Grandfathered read_only is effectively a default, so hide + // it with the other defaults. + config.MigrateReadOnly(v) + } cfgOut, err := config.ForOutputFromViper(v) if err != nil { diff --git a/internal/tiger/cmd/integration_test.go b/internal/tiger/cmd/integration_test.go index edb57415..3ee75dd6 100644 --- a/internal/tiger/cmd/integration_test.go +++ b/internal/tiger/cmd/integration_test.go @@ -34,6 +34,9 @@ func setupIntegrationTest(t *testing.T) string { // Disable analytics for integration tests to avoid tracking test events os.Setenv("TIGER_ANALYTICS", "false") + // Disable read-only mode (default: true) so mutating commands can run + os.Setenv("TIGER_READ_ONLY", "false") + // Reset global config and viper to ensure test isolation config.ResetGlobalConfig() @@ -61,6 +64,7 @@ func setupIntegrationTest(t *testing.T) string { // Clean up environment variables BEFORE cleaning up file system os.Unsetenv("TIGER_CONFIG_DIR") os.Unsetenv("TIGER_ANALYTICS") + os.Unsetenv("TIGER_READ_ONLY") // Then clean up file system os.RemoveAll(tmpDir) }) diff --git a/internal/tiger/config/config.go b/internal/tiger/config/config.go index 9bbcad1f..38f14bd2 100644 --- a/internal/tiger/config/config.go +++ b/internal/tiger/config/config.go @@ -65,7 +65,7 @@ const ( DefaultGatewayURL = "https://console.cloud.tigerdata.com/api" DefaultOutput = "table" DefaultPasswordStorage = "keyring" - DefaultReadOnly = false + DefaultReadOnly = true DefaultReleasesURL = "https://cli.tigerdata.com" DefaultVersionCheck = true @@ -137,6 +137,7 @@ func SetupViper(configDir string) error { } MigrateVersionCheck(v) + MigrateReadOnly(v) return nil } @@ -159,6 +160,36 @@ func MigrateVersionCheck(v *viper.Viper) { } } +// MigrateReadOnly grandfathers installations that predate read-only-by-default. +// Older CLI versions only wrote read_only to the config file when the user set +// it explicitly (current versions always record it; see materializeReadOnly), +// so a config file without the key predates the default flip and keeps its old +// effective value (false) rather than breaking mutating commands on upgrade. +// Fresh installations have no config file and get the read-only default. +// +// Applied via SetDefault, so an explicit config file value or TIGER_READ_ONLY +// env var still wins. Like MigrateVersionCheck, this is an in-memory shim. +func MigrateReadOnly(v *viper.Viper) { + if _, err := os.Stat(v.ConfigFileUsed()); err == nil && !v.InConfig("read_only") { + v.SetDefault("read_only", false) + } +} + +// materializeReadOnly pins read_only in every config file this version writes, +// so a freshly created file isn't mistaken for a pre-upgrade install and +// grandfathered to false by MigrateReadOnly on the next load. Must be called +// before the file is rewritten. +func materializeReadOnly(v *viper.Viper) { + if v.IsSet("read_only") { + return + } + if _, err := os.Stat(v.ConfigFileUsed()); err == nil { + v.Set("read_only", false) + } else { + v.Set("read_only", DefaultReadOnly) + } +} + func FromViper(v *viper.Viper) (*Config, error) { cfg := &Config{ ConfigDir: filepath.Dir(v.ConfigFileUsed()), @@ -267,6 +298,7 @@ func (c *Config) Set(key, value string) error { v.ReadInConfig() v.Set(key, validated) + materializeReadOnly(v) if err := v.WriteConfigAs(configFile); err != nil { return fmt.Errorf("error writing config file: %w", err) @@ -493,6 +525,14 @@ func (c *Config) Unset(key string) error { return fmt.Errorf("unknown configuration key: %s", key) } + // Keep read_only materialized (see materializeReadOnly); unsetting it + // restores the current default. + if key == "read_only" { + vNew.Set("read_only", DefaultReadOnly) + } else { + materializeReadOnly(vNew) + } + // Apply the default to the current global viper state if def, ok := defaultValues[key]; ok { if _, err := c.UpdateField(key, def); err != nil { @@ -515,6 +555,8 @@ func (c *Config) Reset() error { v := viper.New() v.SetConfigFile(configFile) + v.Set("read_only", DefaultReadOnly) + if err := v.WriteConfigAs(configFile); err != nil { return fmt.Errorf("error writing config file: %w", err) } diff --git a/internal/tiger/config/config_test.go b/internal/tiger/config/config_test.go index 594a6a5b..6eaf4a53 100644 --- a/internal/tiger/config/config_test.go +++ b/internal/tiger/config/config_test.go @@ -8,6 +8,8 @@ import ( "testing" "github.com/spf13/pflag" + "github.com/spf13/viper" + "github.com/timescale/tiger-cli/internal/tiger/util" ) @@ -70,8 +72,10 @@ func TestLoad_DefaultValues(t *testing.T) { if cfg.Analytics != DefaultAnalytics { t.Errorf("Expected Analytics %t, got %t", DefaultAnalytics, cfg.Analytics) } - if cfg.ReadOnly != DefaultReadOnly { - t.Errorf("Expected ReadOnly %t, got %t", DefaultReadOnly, cfg.ReadOnly) + // setupTestConfig writes a config file without read_only, so it's + // grandfathered to false (see MigrateReadOnly). + if cfg.ReadOnly { + t.Errorf("Expected ReadOnly false for pre-existing config file, got true") } if cfg.ConfigDir != tmpDir { t.Errorf("Expected ConfigDir %s, got %s", tmpDir, cfg.ConfigDir) @@ -176,6 +180,153 @@ func TestLoad_MigrateVersionCheck(t *testing.T) { } } +func TestLoad_MigrateReadOnly(t *testing.T) { + tests := []struct { + name string + fileBody *string // nil means no config file + env map[string]string + want bool + }{ + { + name: "no config file gets read-only default", + fileBody: nil, + want: DefaultReadOnly, + }, + { + name: "pre-existing config file without key is grandfathered", + fileBody: util.Ptr("output: json\n"), + want: false, + }, + { + name: "explicit read_only in config file is respected", + fileBody: util.Ptr("read_only: true\n"), + want: true, + }, + { + name: "env var overrides grandfathered value", + fileBody: util.Ptr("output: json\n"), + env: map[string]string{"TIGER_READ_ONLY": "true"}, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := t.TempDir() + t.Cleanup(ResetGlobalConfig) + + if tt.fileBody != nil { + configFile := GetConfigFile(tmpDir) + if err := os.WriteFile(configFile, []byte(*tt.fileBody), 0644); err != nil { + t.Fatalf("Failed to write config file: %v", err) + } + } + + os.Setenv("TIGER_CONFIG_DIR", tmpDir) + t.Cleanup(func() { os.Unsetenv("TIGER_CONFIG_DIR") }) + for k, val := range tt.env { + os.Setenv(k, val) + t.Cleanup(func() { os.Unsetenv(k) }) + } + + setupViper(t, tmpDir) + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() failed: %v", err) + } + if cfg.ReadOnly != tt.want { + t.Errorf("ReadOnly = %t, want %t", cfg.ReadOnly, tt.want) + } + }) + } +} + +// TestReadOnlyMaterializedOnWrite verifies that Set/Unset/Reset record +// read_only explicitly, so rewritten files aren't grandfathered to false. +func TestReadOnlyMaterializedOnWrite(t *testing.T) { + loadReadOnly := func(t *testing.T, tmpDir string) bool { + t.Helper() + ResetGlobalConfig() + setupViper(t, tmpDir) + cfg, err := Load() + if err != nil { + t.Fatalf("Load() failed: %v", err) + } + return cfg.ReadOnly + } + + setup := func(t *testing.T, fileBody *string) (string, *Config) { + t.Helper() + tmpDir := t.TempDir() + t.Cleanup(ResetGlobalConfig) + os.Setenv("TIGER_CONFIG_DIR", tmpDir) + t.Cleanup(func() { os.Unsetenv("TIGER_CONFIG_DIR") }) + if fileBody != nil { + if err := os.WriteFile(GetConfigFile(tmpDir), []byte(*fileBody), 0644); err != nil { + t.Fatalf("Failed to write config file: %v", err) + } + } + setupViper(t, tmpDir) + cfg, err := Load() + if err != nil { + t.Fatalf("Load() failed: %v", err) + } + return tmpDir, cfg + } + + t.Run("Set on fresh install pins read-only default", func(t *testing.T) { + tmpDir, cfg := setup(t, nil) + if err := cfg.Set("output", "json"); err != nil { + t.Fatalf("Set() failed: %v", err) + } + if got := loadReadOnly(t, tmpDir); got != DefaultReadOnly { + t.Errorf("ReadOnly = %t after Set on fresh install, want %t", got, DefaultReadOnly) + } + }) + + t.Run("Set on grandfathered config pins false", func(t *testing.T) { + tmpDir, cfg := setup(t, util.Ptr("service_id: svc-123\n")) + if err := cfg.Set("output", "json"); err != nil { + t.Fatalf("Set() failed: %v", err) + } + // Assert on the file itself rather than the loaded value: the + // grandfathering shim would yield an effective false even if Set + // failed to write the key. + v := viper.New() + v.SetConfigFile(GetConfigFile(tmpDir)) + if err := v.ReadInConfig(); err != nil { + t.Fatalf("Failed to read config file: %v", err) + } + if !v.InConfig("read_only") { + t.Fatal("read_only not written to config file by Set") + } + if v.GetBool("read_only") { + t.Error("read_only = true in config file after Set on grandfathered config, want false") + } + }) + + t.Run("Unset read_only restores current default", func(t *testing.T) { + tmpDir, cfg := setup(t, util.Ptr("read_only: false\n")) + if err := cfg.Unset("read_only"); err != nil { + t.Fatalf("Unset() failed: %v", err) + } + if got := loadReadOnly(t, tmpDir); got != DefaultReadOnly { + t.Errorf("ReadOnly = %t after Unset, want %t", got, DefaultReadOnly) + } + }) + + t.Run("Reset restores current default", func(t *testing.T) { + tmpDir, cfg := setup(t, util.Ptr("read_only: false\noutput: json\n")) + if err := cfg.Reset(); err != nil { + t.Fatalf("Reset() failed: %v", err) + } + if got := loadReadOnly(t, tmpDir); got != DefaultReadOnly { + t.Errorf("ReadOnly = %t after Reset, want %t", got, DefaultReadOnly) + } + }) +} + func TestLoad_FromEnvironmentVariables(t *testing.T) { tmpDir := setupTestConfig(t) diff --git a/specs/spec.md b/specs/spec.md index 95892c85..244fab79 100644 --- a/specs/spec.md +++ b/specs/spec.md @@ -48,7 +48,7 @@ All configuration options can be set via `tiger config set `: - `docs_mcp` - Enable/disable docs MCP proxy (default: true) - `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: `tiger service create`/`fork`/`start`/`stop`/`resize`/`update-password`/`delete` and their MCP equivalents return an error, and `tiger db connect`/`connection-string`/`db_execute_query` open against an immutable read-only database connection regardless of `--read-only` (default: false). See `specs/spec_mcp.md` for details. +- `read_only` - When `true`, mutating operations are refused: `tiger service create`/`fork`/`start`/`stop`/`resize`/`update-password`/`delete` and their MCP equivalents return an error, and `tiger db connect`/`connection-string`/`db_execute_query` open against an immutable read-only database connection regardless of `--read-only` (default: true for new installations). See `specs/spec_mcp.md` for details. - `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; false to disable (default: true) diff --git a/specs/spec_mcp.md b/specs/spec_mcp.md index 4e31c6d2..37e3151d 100644 --- a/specs/spec_mcp.md +++ b/specs/spec_mcp.md @@ -69,7 +69,7 @@ When `read_only` is `true` (or `TIGER_READ_ONLY=true`), Tiger refuses any mutati Intended for AI agents that should be able to read Tiger Cloud resources without risk of mutation. `tsdb_admin.read_only_connection` is a Tiger Cloud GUC injected as a startup `options` parameter; it activates an immutable read-only connection so writes and DDL are rejected by the server itself and cannot be re-enabled with a `SET` statement. -To toggle: `tiger config set read_only true` / `tiger config unset read_only`. +Read-only mode is enabled by default for new installations. Config files created by older CLI versions don't contain the `read_only` key and are grandfathered to `false` (see `MigrateReadOnly` in `internal/tiger/config/config.go`), so upgrading doesn't break existing workflows. To toggle: `tiger config set read_only false` / `tiger config set read_only true`. When read-only mode is enabled, the MCP server includes a warning in its `initialize` response `instructions` field listing the gated tools and asking the LLM to inform the user before gathering inputs for them. The instructions are read at server start; if the user toggles `read_only` mid-session, the warning is stale until the MCP client restarts (the gate and the GUC injection are both unaffected because handlers reload config per call).