diff --git a/internal/store/pg/tenant_configs.go b/internal/store/pg/tenant_configs.go index 8e771a6e..f3fe61c8 100644 --- a/internal/store/pg/tenant_configs.go +++ b/internal/store/pg/tenant_configs.go @@ -3,9 +3,14 @@ package pg import ( "context" "database/sql" + "encoding/json" + "errors" + "fmt" "time" "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" ) // PGBuiltinToolTenantConfigStore implements store.BuiltinToolTenantConfigStore. @@ -18,6 +23,9 @@ func NewPGBuiltinToolTenantConfigStore(db *sql.DB) *PGBuiltinToolTenantConfigSto } func (s *PGBuiltinToolTenantConfigStore) ListDisabled(ctx context.Context, tenantID uuid.UUID) ([]string, error) { + if tenantID == uuid.Nil { + return nil, store.ErrInvalidTenant + } type row struct { ToolName string `db:"tool_name"` } @@ -36,35 +44,52 @@ func (s *PGBuiltinToolTenantConfigStore) ListDisabled(ctx context.Context, tenan } func (s *PGBuiltinToolTenantConfigStore) ListAll(ctx context.Context, tenantID uuid.UUID) (map[string]bool, error) { + if tenantID == uuid.Nil { + return nil, store.ErrInvalidTenant + } type row struct { ToolName string `db:"tool_name"` - Enabled bool `db:"enabled"` + Enabled *bool `db:"enabled"` } var rows []row if err := pkgSqlxDB.SelectContext(ctx, &rows, - `SELECT tool_name, enabled FROM builtin_tool_tenant_configs WHERE tenant_id = $1`, + `SELECT tool_name, enabled FROM builtin_tool_tenant_configs WHERE tenant_id = $1 AND enabled IS NOT NULL`, tenantID, ); err != nil { return nil, err } result := make(map[string]bool, len(rows)) for _, r := range rows { - result[r.ToolName] = r.Enabled + if r.Enabled != nil { + result[r.ToolName] = *r.Enabled + } } return result, nil } +// Set upserts the enabled flag. Preserves the settings column via explicit +// column list in DO UPDATE SET — a concurrent settings write is untouched. func (s *PGBuiltinToolTenantConfigStore) Set(ctx context.Context, tenantID uuid.UUID, toolName string, enabled bool) error { + if tenantID == uuid.Nil { + return store.ErrInvalidTenant + } _, err := s.db.ExecContext(ctx, `INSERT INTO builtin_tool_tenant_configs (tool_name, tenant_id, enabled, updated_at) VALUES ($1, $2, $3, $4) - ON CONFLICT (tool_name, tenant_id) DO UPDATE SET enabled = $3, updated_at = $4`, + ON CONFLICT (tool_name, tenant_id) + DO UPDATE SET enabled = EXCLUDED.enabled, updated_at = EXCLUDED.updated_at`, toolName, tenantID, enabled, time.Now(), ) - return err + if err != nil { + return fmt.Errorf("set tenant tool enabled: %w", err) + } + return nil } func (s *PGBuiltinToolTenantConfigStore) Delete(ctx context.Context, tenantID uuid.UUID, toolName string) error { + if tenantID == uuid.Nil { + return store.ErrInvalidTenant + } _, err := s.db.ExecContext(ctx, `DELETE FROM builtin_tool_tenant_configs WHERE tool_name = $1 AND tenant_id = $2`, toolName, tenantID, @@ -72,6 +97,83 @@ func (s *PGBuiltinToolTenantConfigStore) Delete(ctx context.Context, tenantID uu return err } +// GetSettings returns the raw settings JSON for a tool. +// Missing row or NULL column → (nil, nil) so callers fall back to global/hardcoded defaults. +func (s *PGBuiltinToolTenantConfigStore) GetSettings(ctx context.Context, tenantID uuid.UUID, toolName string) (json.RawMessage, error) { + if tenantID == uuid.Nil { + return nil, store.ErrInvalidTenant + } + var raw []byte + err := s.db.QueryRowContext(ctx, + `SELECT settings FROM builtin_tool_tenant_configs + WHERE tool_name = $1 AND tenant_id = $2`, + toolName, tenantID, + ).Scan(&raw) + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("get tenant tool settings: %w", err) + } + if len(raw) == 0 { + return nil, nil + } + return json.RawMessage(raw), nil +} + +// SetSettings upserts the settings JSON. Preserves enabled column via explicit +// column list in DO UPDATE SET. Passing settings=nil writes SQL NULL (clears override). +func (s *PGBuiltinToolTenantConfigStore) SetSettings(ctx context.Context, tenantID uuid.UUID, toolName string, settings json.RawMessage) error { + if tenantID == uuid.Nil { + return store.ErrInvalidTenant + } + var settingsArg any + if settings != nil { + settingsArg = []byte(settings) + } + _, err := s.db.ExecContext(ctx, + `INSERT INTO builtin_tool_tenant_configs (tool_name, tenant_id, settings, updated_at) + VALUES ($1, $2, $3, $4) + ON CONFLICT (tool_name, tenant_id) + DO UPDATE SET settings = EXCLUDED.settings, updated_at = EXCLUDED.updated_at`, + toolName, tenantID, settingsArg, time.Now(), + ) + if err != nil { + return fmt.Errorf("set tenant tool settings: %w", err) + } + return nil +} + +// ListAllSettings returns tool_name → raw settings JSON for every tenant row +// with a non-null settings column. Used by the agent resolver to bulk-load +// tenant settings at Loop construction. +func (s *PGBuiltinToolTenantConfigStore) ListAllSettings(ctx context.Context, tenantID uuid.UUID) (map[string]json.RawMessage, error) { + if tenantID == uuid.Nil { + return nil, store.ErrInvalidTenant + } + rows, err := s.db.QueryContext(ctx, + `SELECT tool_name, settings FROM builtin_tool_tenant_configs + WHERE tenant_id = $1 AND settings IS NOT NULL`, + tenantID, + ) + if err != nil { + return nil, fmt.Errorf("list tenant tool settings: %w", err) + } + defer rows.Close() + result := make(map[string]json.RawMessage) + for rows.Next() { + var name string + var raw []byte + if err := rows.Scan(&name, &raw); err != nil { + return nil, err + } + if len(raw) > 0 { + result[name] = json.RawMessage(raw) + } + } + return result, rows.Err() +} + // PGSkillTenantConfigStore implements store.SkillTenantConfigStore. type PGSkillTenantConfigStore struct { db *sql.DB diff --git a/internal/store/sqlitestore/tenant-configs.go b/internal/store/sqlitestore/tenant-configs.go index 38715d04..639ba371 100644 --- a/internal/store/sqlitestore/tenant-configs.go +++ b/internal/store/sqlitestore/tenant-configs.go @@ -5,9 +5,14 @@ package sqlitestore import ( "context" "database/sql" + "encoding/json" + "errors" + "fmt" "time" "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" ) // SQLiteBuiltinToolTenantConfigStore implements store.BuiltinToolTenantConfigStore. @@ -20,6 +25,9 @@ func NewSQLiteBuiltinToolTenantConfigStore(db *sql.DB) *SQLiteBuiltinToolTenantC } func (s *SQLiteBuiltinToolTenantConfigStore) ListDisabled(ctx context.Context, tenantID uuid.UUID) ([]string, error) { + if tenantID == uuid.Nil { + return nil, store.ErrInvalidTenant + } rows, err := s.db.QueryContext(ctx, `SELECT tool_name FROM builtin_tool_tenant_configs WHERE tenant_id = ? AND enabled = 0`, tenantID, @@ -41,8 +49,11 @@ func (s *SQLiteBuiltinToolTenantConfigStore) ListDisabled(ctx context.Context, t } func (s *SQLiteBuiltinToolTenantConfigStore) ListAll(ctx context.Context, tenantID uuid.UUID) (map[string]bool, error) { + if tenantID == uuid.Nil { + return nil, store.ErrInvalidTenant + } rows, err := s.db.QueryContext(ctx, - `SELECT tool_name, enabled FROM builtin_tool_tenant_configs WHERE tenant_id = ?`, + `SELECT tool_name, enabled FROM builtin_tool_tenant_configs WHERE tenant_id = ? AND enabled IS NOT NULL`, tenantID, ) if err != nil { @@ -53,26 +64,39 @@ func (s *SQLiteBuiltinToolTenantConfigStore) ListAll(ctx context.Context, tenant result := make(map[string]bool) for rows.Next() { var name string - var enabled bool + var enabled sql.NullBool if err := rows.Scan(&name, &enabled); err != nil { return nil, err } - result[name] = enabled + if enabled.Valid { + result[name] = enabled.Bool + } } return result, rows.Err() } +// Set upserts the enabled flag. Preserves the settings column. func (s *SQLiteBuiltinToolTenantConfigStore) Set(ctx context.Context, tenantID uuid.UUID, toolName string, enabled bool) error { + if tenantID == uuid.Nil { + return store.ErrInvalidTenant + } _, err := s.db.ExecContext(ctx, `INSERT INTO builtin_tool_tenant_configs (tool_name, tenant_id, enabled, updated_at) VALUES (?, ?, ?, ?) - ON CONFLICT (tool_name, tenant_id) DO UPDATE SET enabled = excluded.enabled, updated_at = excluded.updated_at`, + ON CONFLICT (tool_name, tenant_id) + DO UPDATE SET enabled = excluded.enabled, updated_at = excluded.updated_at`, toolName, tenantID, enabled, time.Now().UTC(), ) - return err + if err != nil { + return fmt.Errorf("set tenant tool enabled: %w", err) + } + return nil } func (s *SQLiteBuiltinToolTenantConfigStore) Delete(ctx context.Context, tenantID uuid.UUID, toolName string) error { + if tenantID == uuid.Nil { + return store.ErrInvalidTenant + } _, err := s.db.ExecContext(ctx, `DELETE FROM builtin_tool_tenant_configs WHERE tool_name = ? AND tenant_id = ?`, toolName, tenantID, @@ -80,6 +104,82 @@ func (s *SQLiteBuiltinToolTenantConfigStore) Delete(ctx context.Context, tenantI return err } +// GetSettings returns the raw settings JSON for a tool. +// Missing row or NULL column → (nil, nil). +func (s *SQLiteBuiltinToolTenantConfigStore) GetSettings(ctx context.Context, tenantID uuid.UUID, toolName string) (json.RawMessage, error) { + if tenantID == uuid.Nil { + return nil, store.ErrInvalidTenant + } + var raw sql.NullString + err := s.db.QueryRowContext(ctx, + `SELECT settings FROM builtin_tool_tenant_configs + WHERE tool_name = ? AND tenant_id = ?`, + toolName, tenantID, + ).Scan(&raw) + if errors.Is(err, sql.ErrNoRows) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("get tenant tool settings: %w", err) + } + if !raw.Valid || len(raw.String) == 0 { + return nil, nil + } + return json.RawMessage(raw.String), nil +} + +// SetSettings upserts the settings JSON. Preserves enabled column. nil → SQL NULL. +func (s *SQLiteBuiltinToolTenantConfigStore) SetSettings(ctx context.Context, tenantID uuid.UUID, toolName string, settings json.RawMessage) error { + if tenantID == uuid.Nil { + return store.ErrInvalidTenant + } + var settingsArg any + if settings != nil { + // SQLite stores JSON as TEXT — pass as string for portable NULL handling. + settingsArg = string(settings) + } + _, err := s.db.ExecContext(ctx, + `INSERT INTO builtin_tool_tenant_configs (tool_name, tenant_id, settings, updated_at) + VALUES (?, ?, ?, ?) + ON CONFLICT (tool_name, tenant_id) + DO UPDATE SET settings = excluded.settings, updated_at = excluded.updated_at`, + toolName, tenantID, settingsArg, time.Now().UTC(), + ) + if err != nil { + return fmt.Errorf("set tenant tool settings: %w", err) + } + return nil +} + +// ListAllSettings returns tool_name → raw settings JSON for every tenant row +// with a non-null settings column. +func (s *SQLiteBuiltinToolTenantConfigStore) ListAllSettings(ctx context.Context, tenantID uuid.UUID) (map[string]json.RawMessage, error) { + if tenantID == uuid.Nil { + return nil, store.ErrInvalidTenant + } + rows, err := s.db.QueryContext(ctx, + `SELECT tool_name, settings FROM builtin_tool_tenant_configs + WHERE tenant_id = ? AND settings IS NOT NULL`, + tenantID, + ) + if err != nil { + return nil, fmt.Errorf("list tenant tool settings: %w", err) + } + defer rows.Close() + result := make(map[string]json.RawMessage) + for rows.Next() { + var name string + var raw sql.NullString + if err := rows.Scan(&name, &raw); err != nil { + return nil, err + } + if raw.Valid && len(raw.String) > 0 { + result[name] = json.RawMessage(raw.String) + } + } + return result, rows.Err() +} + // SQLiteSkillTenantConfigStore implements store.SkillTenantConfigStore. type SQLiteSkillTenantConfigStore struct { db *sql.DB diff --git a/internal/store/sqlitestore/tenant_configs_settings_test.go b/internal/store/sqlitestore/tenant_configs_settings_test.go new file mode 100644 index 00000000..d2a95e12 --- /dev/null +++ b/internal/store/sqlitestore/tenant_configs_settings_test.go @@ -0,0 +1,301 @@ +//go:build sqlite || sqliteonly + +package sqlitestore + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// newTenantToolCfgFixture returns a ready-to-use SQLiteBuiltinToolTenantConfigStore +// backed by a temp DB with tenants + builtin_tools rows pre-seeded (FK targets). +func newTenantToolCfgFixture(t *testing.T) (*SQLiteBuiltinToolTenantConfigStore, uuid.UUID) { + t.Helper() + + db := openTestDB(t) + if err := EnsureSchema(db); err != nil { + t.Fatalf("EnsureSchema: %v", err) + } + + // Seed a tenant + tool to satisfy FK constraints. + tenantID := uuid.New() + if _, err := db.Exec( + `INSERT INTO tenants (id, name, slug, status, settings, created_at, updated_at) + VALUES (?, ?, ?, 'active', '{}', + strftime('%Y-%m-%dT%H:%M:%fZ', 'now'), + strftime('%Y-%m-%dT%H:%M:%fZ', 'now'))`, + tenantID, "tenant-a", "tenant-a-"+tenantID.String()[:8], + ); err != nil { + t.Fatalf("seed tenant: %v", err) + } + + for _, tool := range []string{"web_search", "web_fetch", "tts"} { + if _, err := db.Exec( + `INSERT INTO builtin_tools (name, display_name, description, category, enabled, settings, requires, metadata, created_at, updated_at) + VALUES (?, ?, '', 'test', 1, '{}', '[]', '{}', + strftime('%Y-%m-%dT%H:%M:%fZ', 'now'), + strftime('%Y-%m-%dT%H:%M:%fZ', 'now'))`, + tool, tool, + ); err != nil { + t.Fatalf("seed builtin_tool %s: %v", tool, err) + } + } + + return NewSQLiteBuiltinToolTenantConfigStore(db), tenantID +} + +// ---- GetSettings / SetSettings round-trip ---- + +func TestSQLiteTenantCfg_SettingsRoundTrip(t *testing.T) { + s, tid := newTenantToolCfgFixture(t) + ctx := context.Background() + + raw := json.RawMessage(`{"exa":{"enabled":true,"max_results":15}}`) + if err := s.SetSettings(ctx, tid, "web_search", raw); err != nil { + t.Fatalf("SetSettings: %v", err) + } + + got, err := s.GetSettings(ctx, tid, "web_search") + if err != nil { + t.Fatalf("GetSettings: %v", err) + } + if string(got) != string(raw) { + t.Errorf("GetSettings = %s, want %s", got, raw) + } +} + +func TestSQLiteTenantCfg_GetSettings_MissingRow_ReturnsNilNil(t *testing.T) { + s, tid := newTenantToolCfgFixture(t) + ctx := context.Background() + + got, err := s.GetSettings(ctx, tid, "tts") + if err != nil { + t.Fatalf("GetSettings: %v", err) + } + if got != nil { + t.Errorf("expected nil for missing row, got %s", got) + } +} + +func TestSQLiteTenantCfg_GetSettings_NullColumn_ReturnsNilNil(t *testing.T) { + s, tid := newTenantToolCfgFixture(t) + ctx := context.Background() + + // Create row via Set(enabled) — settings column stays NULL. + if err := s.Set(ctx, tid, "tts", true); err != nil { + t.Fatalf("Set: %v", err) + } + + got, err := s.GetSettings(ctx, tid, "tts") + if err != nil { + t.Fatalf("GetSettings: %v", err) + } + if got != nil { + t.Errorf("expected nil for NULL settings column, got %s", got) + } +} + +// ---- ErrInvalidTenant sentinel ---- + +func TestSQLiteTenantCfg_NilTenant_ReturnsErrInvalidTenant(t *testing.T) { + s, _ := newTenantToolCfgFixture(t) + ctx := context.Background() + + _, err := s.GetSettings(ctx, uuid.Nil, "web_search") + if !errors.Is(err, store.ErrInvalidTenant) { + t.Errorf("GetSettings nil tenant err = %v, want ErrInvalidTenant", err) + } + + err = s.SetSettings(ctx, uuid.Nil, "web_search", json.RawMessage(`{}`)) + if !errors.Is(err, store.ErrInvalidTenant) { + t.Errorf("SetSettings nil tenant err = %v, want ErrInvalidTenant", err) + } + + _, err = s.ListAllSettings(ctx, uuid.Nil) + if !errors.Is(err, store.ErrInvalidTenant) { + t.Errorf("ListAllSettings nil tenant err = %v, want ErrInvalidTenant", err) + } + + // Existing methods now guarded too. + err = s.Set(ctx, uuid.Nil, "web_search", true) + if !errors.Is(err, store.ErrInvalidTenant) { + t.Errorf("Set nil tenant err = %v, want ErrInvalidTenant", err) + } +} + +// ---- Column preservation ---- + +// SetSettings must preserve the existing enabled flag. +func TestSQLiteTenantCfg_SetSettings_PreservesEnabled(t *testing.T) { + s, tid := newTenantToolCfgFixture(t) + ctx := context.Background() + + // Set enabled first. + if err := s.Set(ctx, tid, "web_search", true); err != nil { + t.Fatalf("Set: %v", err) + } + // Then overwrite settings. + raw := json.RawMessage(`{"brave":{"max_results":20}}`) + if err := s.SetSettings(ctx, tid, "web_search", raw); err != nil { + t.Fatalf("SetSettings: %v", err) + } + + all, err := s.ListAll(ctx, tid) + if err != nil { + t.Fatalf("ListAll: %v", err) + } + if !all["web_search"] { + t.Errorf("enabled flag lost after SetSettings: %#v", all) + } +} + +// Set(enabled) must preserve the existing settings column. +func TestSQLiteTenantCfg_Set_PreservesSettings(t *testing.T) { + s, tid := newTenantToolCfgFixture(t) + ctx := context.Background() + + raw := json.RawMessage(`{"brave":{"max_results":20}}`) + if err := s.SetSettings(ctx, tid, "web_search", raw); err != nil { + t.Fatalf("SetSettings: %v", err) + } + // Then toggle enabled. + if err := s.Set(ctx, tid, "web_search", false); err != nil { + t.Fatalf("Set: %v", err) + } + + got, err := s.GetSettings(ctx, tid, "web_search") + if err != nil { + t.Fatalf("GetSettings: %v", err) + } + if string(got) != string(raw) { + t.Errorf("settings lost after Set(enabled), got %s", got) + } +} + +// SetSettings(nil) clears the settings column without deleting the row. +func TestSQLiteTenantCfg_SetSettings_Nil_ClearsColumn(t *testing.T) { + s, tid := newTenantToolCfgFixture(t) + ctx := context.Background() + + raw := json.RawMessage(`{"brave":{"max_results":20}}`) + if err := s.SetSettings(ctx, tid, "web_search", raw); err != nil { + t.Fatalf("SetSettings initial: %v", err) + } + if err := s.Set(ctx, tid, "web_search", true); err != nil { + t.Fatalf("Set: %v", err) + } + + // Clear settings. + if err := s.SetSettings(ctx, tid, "web_search", nil); err != nil { + t.Fatalf("SetSettings nil: %v", err) + } + + got, err := s.GetSettings(ctx, tid, "web_search") + if err != nil { + t.Fatalf("GetSettings: %v", err) + } + if got != nil { + t.Errorf("expected nil settings after clear, got %s", got) + } + + // Enabled must still be true — row not deleted. + all, err := s.ListAll(ctx, tid) + if err != nil { + t.Fatalf("ListAll: %v", err) + } + if !all["web_search"] { + t.Errorf("enabled flag lost after SetSettings(nil): %#v", all) + } +} + +// ---- ListAllSettings ---- + +func TestSQLiteTenantCfg_ListAllSettings(t *testing.T) { + s, tid := newTenantToolCfgFixture(t) + ctx := context.Background() + + rawA := json.RawMessage(`{"a":1}`) + rawB := json.RawMessage(`{"b":2}`) + if err := s.SetSettings(ctx, tid, "web_search", rawA); err != nil { + t.Fatalf("SetSettings web_search: %v", err) + } + if err := s.SetSettings(ctx, tid, "web_fetch", rawB); err != nil { + t.Fatalf("SetSettings web_fetch: %v", err) + } + // A row with NULL settings should be excluded. + if err := s.Set(ctx, tid, "tts", true); err != nil { + t.Fatalf("Set tts: %v", err) + } + + all, err := s.ListAllSettings(ctx, tid) + if err != nil { + t.Fatalf("ListAllSettings: %v", err) + } + if len(all) != 2 { + t.Errorf("ListAllSettings len = %d, want 2 (tts row with NULL settings excluded)", len(all)) + } + if string(all["web_search"]) != string(rawA) { + t.Errorf("web_search = %s, want %s", all["web_search"], rawA) + } + if string(all["web_fetch"]) != string(rawB) { + t.Errorf("web_fetch = %s, want %s", all["web_fetch"], rawB) + } + if _, has := all["tts"]; has { + t.Errorf("ListAllSettings should exclude rows with NULL settings, but got tts") + } +} + +// ---- Cross-tenant isolation ---- + +func TestSQLiteTenantCfg_CrossTenantIsolation(t *testing.T) { + s, tidA := newTenantToolCfgFixture(t) + ctx := context.Background() + + // Seed a second tenant on the same DB by reaching into the store. + // (We borrow the underlying db handle via a minimal shim — simpler than a second fixture.) + if _, err := s.db.Exec( + `INSERT INTO tenants (id, name, slug, status, settings, created_at, updated_at) + VALUES (?, ?, ?, 'active', '{}', + strftime('%Y-%m-%dT%H:%M:%fZ', 'now'), + strftime('%Y-%m-%dT%H:%M:%fZ', 'now'))`, + uuid.New().String(), "tenant-b", "tenant-b", + ); err != nil { + t.Fatalf("seed tenant b: %v", err) + } + var tidBStr string + if err := s.db.QueryRow(`SELECT id FROM tenants WHERE slug = 'tenant-b'`).Scan(&tidBStr); err != nil { + t.Fatalf("fetch tenant b id: %v", err) + } + tidB, err := uuid.Parse(tidBStr) + if err != nil { + t.Fatalf("parse tenant b id: %v", err) + } + + // Tenant A writes settings. + if err := s.SetSettings(ctx, tidA, "web_search", json.RawMessage(`{"secret":"A"}`)); err != nil { + t.Fatalf("A SetSettings: %v", err) + } + + // Tenant B must not see A's settings. + got, err := s.GetSettings(ctx, tidB, "web_search") + if err != nil { + t.Fatalf("B GetSettings: %v", err) + } + if got != nil { + t.Errorf("tenant B leaked A's settings: %s", got) + } + + allB, err := s.ListAllSettings(ctx, tidB) + if err != nil { + t.Fatalf("B ListAllSettings: %v", err) + } + if len(allB) != 0 { + t.Errorf("tenant B ListAllSettings = %v, want empty", allB) + } +} diff --git a/internal/store/tenant_config_store.go b/internal/store/tenant_config_store.go index f735b7aa..316b4bec 100644 --- a/internal/store/tenant_config_store.go +++ b/internal/store/tenant_config_store.go @@ -2,27 +2,55 @@ package store import ( "context" + "encoding/json" + "errors" "github.com/google/uuid" ) +// ErrInvalidTenant is returned when a store method requires an explicit tenant +// but receives uuid.Nil. Callers can check via errors.Is(err, store.ErrInvalidTenant). +var ErrInvalidTenant = errors.New("tenant_id cannot be nil") + // BuiltinToolTenantConfig represents a per-tenant override for a builtin tool. +// +// Two independent override dimensions: +// - Enabled: tenant admin can force-enable or force-disable the tool (nil = use default). +// - Settings: tenant admin can override the tool's config JSON blob (nil = no override, +// falls back to global builtin_tools.settings). +// +// The two columns are managed by distinct Set*/Get* methods so a write to one never +// clobbers the other (see PG/SQLite impl: upsert uses explicit column list on conflict). type BuiltinToolTenantConfig struct { - ToolName string `json:"tool_name" db:"tool_name"` - TenantID uuid.UUID `json:"tenant_id" db:"tenant_id"` - Enabled *bool `json:"enabled,omitempty" db:"enabled"` // nil = use default, false = disabled, true = enabled + ToolName string `json:"tool_name" db:"tool_name"` + TenantID uuid.UUID `json:"tenant_id" db:"tenant_id"` + Enabled *bool `json:"enabled,omitempty" db:"enabled"` // nil = use default, false = disabled, true = enabled + Settings json.RawMessage `json:"settings,omitempty" db:"settings"` // nil = no override; tool uses global/hardcoded default } // BuiltinToolTenantConfigStore manages per-tenant builtin tool overrides. +// +// All methods require an explicit tenantID parameter — passing uuid.Nil returns +// ErrInvalidTenant. This avoids silent master-default leaks via fallback logic. type BuiltinToolTenantConfigStore interface { // ListDisabled returns tool names disabled for a tenant. ListDisabled(ctx context.Context, tenantID uuid.UUID) ([]string, error) - // ListAll returns all tenant overrides (tool_name → enabled) for a tenant. + // ListAll returns all tenant enabled overrides (tool_name → enabled) for a tenant. ListAll(ctx context.Context, tenantID uuid.UUID) (map[string]bool, error) - // Set creates or updates a tenant tool config. + // Set creates or updates a tenant tool's enabled override. Preserves settings column. Set(ctx context.Context, tenantID uuid.UUID, toolName string, enabled bool) error - // Delete removes a tenant tool config (reverts to default). + // Delete removes a tenant tool config row entirely (reverts both enabled + settings). Delete(ctx context.Context, tenantID uuid.UUID, toolName string) error + + // GetSettings returns the raw tenant settings JSON for a tool. + // Returns (nil, nil) when the row doesn't exist or settings column is NULL. + GetSettings(ctx context.Context, tenantID uuid.UUID, toolName string) (json.RawMessage, error) + // SetSettings upserts the tenant settings JSON for a tool. Preserves enabled column. + // Passing nil writes SQL NULL (clears the override without deleting the row). + SetSettings(ctx context.Context, tenantID uuid.UUID, toolName string, settings json.RawMessage) error + // ListAllSettings returns tool_name → settings JSON for every row where settings IS NOT NULL. + // Used by the agent resolver to bulk-load tenant settings at Loop construction. + ListAllSettings(ctx context.Context, tenantID uuid.UUID) (map[string]json.RawMessage, error) } // SkillTenantConfig represents a per-tenant override for a skill. diff --git a/tests/integration/v3_tenant_configs_test.go b/tests/integration/v3_tenant_configs_test.go index b1fde48e..365256f0 100644 --- a/tests/integration/v3_tenant_configs_test.go +++ b/tests/integration/v3_tenant_configs_test.go @@ -4,8 +4,12 @@ package integration import ( "context" + "encoding/json" + "errors" "testing" + "github.com/google/uuid" + "github.com/nextlevelbuilder/goclaw/internal/store" "github.com/nextlevelbuilder/goclaw/internal/store/pg" ) @@ -181,3 +185,142 @@ func TestStoreTenantConfig_TenantIsolation(t *testing.T) { t.Errorf("tenant A sees %d disabled tools, want 1", len(disabledA)) } } + +// ---- Phase 2: tenant settings round-trip + column preservation ---- + +func TestStoreTenantConfig_SettingsRoundTrip(t *testing.T) { + db := testDB(t) + tenantID, _ := seedTenantAgent(t, db) + s := pg.NewPGBuiltinToolTenantConfigStore(db) + pg.InitSqlx(db) + ctx := context.Background() + + db.Exec(`INSERT INTO builtin_tools (name, display_name) VALUES ('web_search', 'web_search') ON CONFLICT DO NOTHING`) + t.Cleanup(func() { + db.Exec("DELETE FROM builtin_tool_tenant_configs WHERE tenant_id = $1", tenantID) + }) + + raw := json.RawMessage(`{"exa":{"enabled":true,"max_results":15}}`) + if err := s.SetSettings(ctx, tenantID, "web_search", raw); err != nil { + t.Fatalf("SetSettings: %v", err) + } + got, err := s.GetSettings(ctx, tenantID, "web_search") + if err != nil { + t.Fatalf("GetSettings: %v", err) + } + // PG jsonb may normalize whitespace — decode both sides for equality. + var want, have map[string]any + if err := json.Unmarshal(raw, &want); err != nil { + t.Fatalf("unmarshal want: %v", err) + } + if err := json.Unmarshal(got, &have); err != nil { + t.Fatalf("unmarshal got: %v", err) + } + if len(have) != len(want) || have["exa"] == nil { + t.Errorf("GetSettings round-trip mismatch: got=%v want=%v", have, want) + } + + // Missing row → (nil, nil) + gotMissing, err := s.GetSettings(ctx, tenantID, "unknown_tool") + if err != nil { + t.Fatalf("GetSettings missing: %v", err) + } + if gotMissing != nil { + t.Errorf("missing row GetSettings = %s, want nil", gotMissing) + } +} + +func TestStoreTenantConfig_SettingsAndEnabledCoexist(t *testing.T) { + db := testDB(t) + tenantID, _ := seedTenantAgent(t, db) + s := pg.NewPGBuiltinToolTenantConfigStore(db) + pg.InitSqlx(db) + ctx := context.Background() + + db.Exec(`INSERT INTO builtin_tools (name, display_name) VALUES ('web_search', 'web_search') ON CONFLICT DO NOTHING`) + t.Cleanup(func() { + db.Exec("DELETE FROM builtin_tool_tenant_configs WHERE tenant_id = $1", tenantID) + }) + + // Set enabled first. + if err := s.Set(ctx, tenantID, "web_search", true); err != nil { + t.Fatalf("Set: %v", err) + } + // SetSettings must preserve enabled. + raw := json.RawMessage(`{"brave":{"max_results":20}}`) + if err := s.SetSettings(ctx, tenantID, "web_search", raw); err != nil { + t.Fatalf("SetSettings: %v", err) + } + all, err := s.ListAll(ctx, tenantID) + if err != nil { + t.Fatalf("ListAll: %v", err) + } + if !all["web_search"] { + t.Errorf("enabled flag lost after SetSettings: %#v", all) + } + + // Set(enabled=false) must preserve settings. + if err := s.Set(ctx, tenantID, "web_search", false); err != nil { + t.Fatalf("Set: %v", err) + } + got, err := s.GetSettings(ctx, tenantID, "web_search") + if err != nil { + t.Fatalf("GetSettings: %v", err) + } + if len(got) == 0 { + t.Errorf("settings lost after Set(enabled)") + } +} + +func TestStoreTenantConfig_SettingsCrossTenantIsolation(t *testing.T) { + db := testDB(t) + tenantA, _ := seedTenantAgent(t, db) + tenantB, _ := seedTenantAgent(t, db) + s := pg.NewPGBuiltinToolTenantConfigStore(db) + pg.InitSqlx(db) + ctx := context.Background() + + db.Exec(`INSERT INTO builtin_tools (name, display_name) VALUES ('web_search', 'web_search') ON CONFLICT DO NOTHING`) + t.Cleanup(func() { + db.Exec("DELETE FROM builtin_tool_tenant_configs WHERE tenant_id IN ($1, $2)", tenantA, tenantB) + }) + + if err := s.SetSettings(ctx, tenantA, "web_search", json.RawMessage(`{"secret":"A"}`)); err != nil { + t.Fatalf("A SetSettings: %v", err) + } + gotB, err := s.GetSettings(ctx, tenantB, "web_search") + if err != nil { + t.Fatalf("B GetSettings: %v", err) + } + if gotB != nil { + t.Errorf("tenant B leaked tenant A settings: %s", gotB) + } + + allB, err := s.ListAllSettings(ctx, tenantB) + if err != nil { + t.Fatalf("B ListAllSettings: %v", err) + } + if len(allB) != 0 { + t.Errorf("tenant B ListAllSettings = %v, want empty", allB) + } +} + +func TestStoreTenantConfig_NilTenant_ReturnsErr(t *testing.T) { + db := testDB(t) + s := pg.NewPGBuiltinToolTenantConfigStore(db) + pg.InitSqlx(db) + ctx := context.Background() + + _, err := s.GetSettings(ctx, uuid.Nil, "web_search") + if !errors.Is(err, store.ErrInvalidTenant) { + t.Errorf("GetSettings nil tenant = %v, want ErrInvalidTenant", err) + } + err = s.SetSettings(ctx, uuid.Nil, "web_search", json.RawMessage(`{}`)) + if !errors.Is(err, store.ErrInvalidTenant) { + t.Errorf("SetSettings nil tenant = %v, want ErrInvalidTenant", err) + } + _, err = s.ListAllSettings(ctx, uuid.Nil) + if !errors.Is(err, store.ErrInvalidTenant) { + t.Errorf("ListAllSettings nil tenant = %v, want ErrInvalidTenant", err) + } +}