mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-16 16:15:40 +00:00
feat(hooks/builtin): default_disabled flag + flip pii-redactor off
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)
This commit is contained in:
1 parent
04354e833f
commit
feaf7b9e40
4 files changed
+114
-14
No files matched your search
@@ -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
|
||||
@@ -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 {
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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"))
|
||||
|
||||
Reference in new issue
Block a user