diff --git a/internal/hooks/builtin/builtins.yaml b/internal/hooks/builtin/builtins.yaml index 7128a6d9..de50c2de 100644 --- a/internal/hooks/builtin/builtins.yaml +++ b/internal/hooks/builtin/builtins.yaml @@ -27,7 +27,10 @@ # the _placeholder.js once at least one real source_file exists. builtins: - id: pii-redactor - version: 1 + # v2: default_disabled introduced — one-shot auto-flips existing enabled + # rows to disabled (input-mutation opt-in policy). Bump again for any + # future content change. + version: 2 events: [user_prompt_submit, pre_tool_use] scope: global matcher: "" @@ -46,3 +49,8 @@ builtins: - toolInput.content source_file: pii-redactor.js description: "Masks email addresses and E.164-ish phone numbers in user input and common tool-input fields." + # Off by default — input mutation should be an explicit opt-in. Operators + # turn on via the UI toggle once they verify the regex behavior matches + # their tenant's content. Existing rows in the DB keep their user-set + # toggle; this only affects fresh installs / new event-row inserts. + default_disabled: true diff --git a/internal/hooks/builtin/loader.go b/internal/hooks/builtin/loader.go index f7ee6382..31dbe19d 100644 --- a/internal/hooks/builtin/loader.go +++ b/internal/hooks/builtin/loader.go @@ -45,19 +45,26 @@ func BuiltinEventID(name, event string) uuid.UUID { // Spec mirrors one entry in builtins.yaml. All fields optional except id, // events, source_file; loader validates at parse time. +// +// DefaultDisabled flips the initial enabled value for fresh installs (true → +// row created with enabled=false). Reverse-named so the zero value preserves +// the old "default on" behaviour for any builtin that omits the field. Only +// affects INSERT — existing DB rows retain their user-set enabled toggle +// across version bumps. type Spec struct { - ID string `yaml:"id"` - Version int `yaml:"version"` - Events []string `yaml:"events"` - Scope string `yaml:"scope"` - Matcher string `yaml:"matcher"` - IfExpr string `yaml:"if_expr"` - TimeoutMS int `yaml:"timeout_ms"` - OnTimeout string `yaml:"on_timeout"` - Priority int `yaml:"priority"` - MutableFields []string `yaml:"mutable_fields"` - SourceFile string `yaml:"source_file"` - Description string `yaml:"description"` + ID string `yaml:"id"` + Version int `yaml:"version"` + Events []string `yaml:"events"` + Scope string `yaml:"scope"` + Matcher string `yaml:"matcher"` + IfExpr string `yaml:"if_expr"` + TimeoutMS int `yaml:"timeout_ms"` + OnTimeout string `yaml:"on_timeout"` + Priority int `yaml:"priority"` + MutableFields []string `yaml:"mutable_fields"` + SourceFile string `yaml:"source_file"` + Description string `yaml:"description"` + DefaultDisabled bool `yaml:"default_disabled"` } type catalog struct { diff --git a/internal/hooks/builtin/seed.go b/internal/hooks/builtin/seed.go index 1d72533b..94887896 100644 --- a/internal/hooks/builtin/seed.go +++ b/internal/hooks/builtin/seed.go @@ -86,6 +86,11 @@ func reconcile( } if current == nil { + // Fresh insert: honor both the operator force-disable list AND the + // spec's default_disabled flag. Spec-default off keeps high-impact + // builtins (e.g. PII redactor) opt-in so users opt them in + // deliberately rather than having mutation enabled silently. + enabled := !forceDisabled && !s.DefaultDisabled cfg := hooks.HookConfig{ ID: id, TenantID: hooks.SentinelTenantID, @@ -98,7 +103,7 @@ func reconcile( TimeoutMS: s.TimeoutMS, OnTimeout: hooks.Decision(s.OnTimeout), Priority: s.Priority, - Enabled: !forceDisabled, + Enabled: enabled, Source: hooks.SourceBuiltin, Metadata: metadata, } @@ -121,6 +126,17 @@ func reconcile( "priority": s.Priority, "metadata": metadata, } + // One-shot: when a version bump introduces default_disabled=true on + // a row that was previously enabled, flip it off too. Lets us roll + // out the "input mutation is opt-in" policy retroactively without + // stomping on a user who deliberately re-enables afterwards (the + // next boot finds dbVersion >= s.Version, skips this branch). + if s.DefaultDisabled && current.Enabled { + patch["enabled"] = false + slog.Info("hooks.builtin_default_off_applied", + "id", s.ID, "event", event, + "reason", "version bump introduced default_disabled=true") + } if err := hs.Update(ctx, id, patch); err != nil { return fmt.Errorf("update: %w", err) } diff --git a/internal/hooks/builtin/seed_test.go b/internal/hooks/builtin/seed_test.go index 5b39c76a..3b52b0d4 100644 --- a/internal/hooks/builtin/seed_test.go +++ b/internal/hooks/builtin/seed_test.go @@ -239,6 +239,75 @@ func TestSeed_DowngradeDetected(t *testing.T) { } } +// TestSeed_DefaultDisabledOnFreshInsert verifies a spec carrying +// default_disabled=true creates rows with enabled=false on first insert. +func TestSeed_DefaultDisabledOnFreshInsert(t *testing.T) { + spec := fixtureSpec(1) + spec.DefaultDisabled = true + withTestRegistry(t, spec, []byte("// v1")) + + fs := newFakeHookStore() + if err := Seed(context.Background(), fs, config.HooksConfig{}); err != nil { + t.Fatalf("Seed: %v", err) + } + for id, row := range fs.rows { + if row.Enabled { + t.Errorf("row %s enabled despite default_disabled spec", id) + } + } +} + +// TestSeed_DefaultDisabledFlipsOnVersionBump ensures the one-shot retroactive +// disable kicks in when a version bump introduces the policy on rows that +// were previously enabled. Subsequent boots (no version bump) leave the +// user's manual re-enable alone. +func TestSeed_DefaultDisabledFlipsOnVersionBump(t *testing.T) { + // Boot 1: spec v1, default_disabled NOT set → row created enabled. + withTestRegistry(t, fixtureSpec(1), []byte("// v1")) + fs := newFakeHookStore() + if err := Seed(context.Background(), fs, config.HooksConfig{}); err != nil { + t.Fatal(err) + } + for _, row := range fs.rows { + if !row.Enabled { + t.Fatal("baseline: row should be enabled before policy change") + } + } + + // Boot 2: spec v2 with default_disabled=true → flips existing enabled rows. + v2 := fixtureSpec(2) + v2.DefaultDisabled = true + withTestRegistry(t, v2, []byte("// v2")) + if err := Seed(context.Background(), fs, config.HooksConfig{}); err != nil { + t.Fatal(err) + } + for id, row := range fs.rows { + if row.Enabled { + t.Errorf("row %s still enabled after policy version bump", id) + } + } + + // User re-enables one row through the UI. + for id := range fs.rows { + fs.rows[id].Enabled = true + break + } + + // Boot 3: same spec v2 (no version bump) → user's choice is preserved. + if err := Seed(context.Background(), fs, config.HooksConfig{}); err != nil { + t.Fatal(err) + } + enabledAfter := 0 + for _, row := range fs.rows { + if row.Enabled { + enabledAfter++ + } + } + if enabledAfter != 1 { + t.Errorf("user re-enable lost: enabled=%d, want 1", enabledAfter) + } +} + // TestSeed_BuiltinDisableForcesOff applies the operator escape-hatch list. func TestSeed_BuiltinDisableForcesOff(t *testing.T) { withTestRegistry(t, fixtureSpec(1), []byte("// v1"))