From feaf7b9e40927e7f4f722d153a2968e11a84304e Mon Sep 17 00:00:00 2001 From: viettranx Date: Thu, 16 Apr 2026 08:01:12 +0700 Subject: [PATCH] feat(hooks/builtin): default_disabled flag + flip pii-redactor off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Input-mutation hooks should be opt-in. Adds an explicit YAML toggle so operators turn them on after verifying the regex / policy matches their content, instead of getting silent mutation on first install. - internal/hooks/builtin/loader.go: Spec.DefaultDisabled (yaml default_disabled). Reverse-named so zero value preserves prior default-on behavior for any spec that omits the field. - internal/hooks/builtin/seed.go: - Fresh insert path honors the flag → row created with enabled=false - Version-bump path: when an UPDATE crosses the boundary that newly introduces default_disabled=true, flip enabled→false once. Subsequent boots without a version bump leave the user's manual re-enable alone (the switch fall-through does nothing on equal versions). Logs hooks.builtin_default_off_applied for audit. - internal/hooks/builtin/builtins.yaml: pii-redactor v1 → v2 with default_disabled: true. Existing installs auto-flip to disabled on next boot via the version-bump branch above; users re-enable through the UI toggle when ready. - Tests: - TestSeed_DefaultDisabledOnFreshInsert (insert respects flag) - TestSeed_DefaultDisabledFlipsOnVersionBump (one-shot flip + user re-enable preserved on the next no-bump boot) --- internal/hooks/builtin/builtins.yaml | 10 +++- internal/hooks/builtin/loader.go | 31 ++++++++----- internal/hooks/builtin/seed.go | 18 +++++++- internal/hooks/builtin/seed_test.go | 69 ++++++++++++++++++++++++++++ 4 files changed, 114 insertions(+), 14 deletions(-) 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"))