diff --git a/cmd/gateway_managed.go b/cmd/gateway_managed.go index 66b72e5d..b04f5eb7 100644 --- a/cmd/gateway_managed.go +++ b/cmd/gateway_managed.go @@ -17,6 +17,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/edition" "github.com/nextlevelbuilder/goclaw/internal/eventbus" "github.com/nextlevelbuilder/goclaw/internal/hooks" + hookbuiltin "github.com/nextlevelbuilder/goclaw/internal/hooks/builtin" "github.com/nextlevelbuilder/goclaw/internal/orchestration" httpapi "github.com/nextlevelbuilder/goclaw/internal/http" kg "github.com/nextlevelbuilder/goclaw/internal/knowledgegraph" @@ -148,6 +149,21 @@ func wireExtras( // Agent Hooks (Issue #875) — lifecycle dispatcher + handlers. var hookDispatcher hooks.Dispatcher = hooks.NewNoopDispatcher() if hs, ok := stores.Hooks.(hooks.HookStore); ok && hs != nil { + // Phase 04: wire builtin registry. Install a strip-all lookup FIRST so a + // Load() failure leaves the dispatcher failing closed (no wide fallback + // via the Phase 03 permissive default). On successful Load we swap in the + // real per-id allowlist, then UPSERT canonical rows with stable UUIDv5s. + // Seed failures log but never block startup. + hooks.SetBuiltinAllowlistLookup(func(uuid.UUID) []string { return nil }) + if err := hookbuiltin.Load(); err != nil { + slog.Warn("hooks.builtin_load_failed", "err", err) + } else { + hooks.SetBuiltinAllowlistLookup(hookbuiltin.AllowlistFor) + if err := hookbuiltin.Seed(context.Background(), hs, appCfg.Hooks); err != nil { + slog.Warn("hooks.builtin_seed_failed", "err", err) + } + } + handlers := buildHookHandlers(stores, providerReg, appCfg.Hooks) stdOpts := hooks.StdDispatcherOpts{ Store: hs, diff --git a/go.mod b/go.mod index ae71b416..2779b944 100644 --- a/go.mod +++ b/go.mod @@ -43,6 +43,7 @@ require ( go.opentelemetry.io/otel/trace v1.40.0 golang.org/x/image v0.27.0 golang.org/x/time v0.14.0 + gopkg.in/yaml.v3 v3.0.1 modernc.org/sqlite v1.47.0 tailscale.com v1.94.2 ) @@ -156,7 +157,6 @@ require ( golang.org/x/term v0.40.0 // indirect golang.zx2c4.com/wintun v0.0.0-20230126152724-0fa3db229ce2 // indirect golang.zx2c4.com/wireguard/windows v0.5.3 // indirect - gopkg.in/yaml.v3 v3.0.1 // indirect gvisor.dev/gvisor v0.0.0-20250205023644-9414b50a5633 // indirect modernc.org/libc v1.70.0 // indirect modernc.org/mathutil v1.7.1 // indirect diff --git a/internal/hooks/builtin/README.md b/internal/hooks/builtin/README.md new file mode 100644 index 00000000..934b8d0b --- /dev/null +++ b/internal/hooks/builtin/README.md @@ -0,0 +1,59 @@ +# Builtin Hooks + +Canonical, embed-shipped hook rows. Users may toggle `enabled` but cannot +edit content — the embedded `.js` is overwritten on every boot. + +## Adding a new builtin + +1. Pick a stable kebab identifier (`pii-redactor`, `sql-guard`). Never rename: + the DB primary key is `UUIDv5(namespace, id + "/" + event)` and renaming + would orphan any existing rows. +2. Drop `my-feature.js` alongside `builtins.yaml`. The script sees the + [goja](https://github.com/dop251/goja) sandboxed globals `event` and + `result`, plus the usual `JSON.*`, `Math.*`. No network, no I/O, no timers. +3. Add an entry to `builtins.yaml`: + + ```yaml + - id: my-feature + version: 1 + events: [pre_tool_use] + scope: global + timeout_ms: 200 + on_timeout: allow + priority: 500 + mutable_fields: [toolInput.command] + source_file: my-feature.js + description: "One-line user-facing summary." + ``` + +4. Bump `version` whenever the JS content changes. On next boot, matching + rows get UPDATEd in-place (the user's `enabled` toggle is preserved). +5. Remove `_placeholder.js` once this is the first real `.js` in the package. + +## Operator escape hatch + +Operators can force-disable any builtin by id via `hooks.builtin_disable` in +`config.json` — useful when a builtin misbehaves before the next release: + +```json +{ + "hooks": { + "builtin_disable": ["pii-redactor"] + } +} +``` + +The row stays in the DB (history preserved); only `enabled=false` is applied. + +## Version reconciliation rules + +| Embed version | DB version | Outcome | +|---------------|------------|------------------------| +| missing row | — | INSERT, `enabled=true` | +| newer | older | UPDATE content; keep user's `enabled` toggle | +| equal | equal | no-op | +| older | newer | WARN log; keep DB (no rollback) | + +A downgrade happens when someone rolls back the binary but the DB has already +been written by a newer version. We never destructively rewrite; operators +must resolve manually. diff --git a/internal/hooks/builtin/_placeholder.js b/internal/hooks/builtin/_placeholder.js new file mode 100644 index 00000000..bf490386 --- /dev/null +++ b/internal/hooks/builtin/_placeholder.js @@ -0,0 +1,2 @@ +// Placeholder to satisfy `//go:embed *.js` until the first real builtin +// ships. Phase 05 adds pii-redactor.js and this file is removed. diff --git a/internal/hooks/builtin/allowlist.go b/internal/hooks/builtin/allowlist.go new file mode 100644 index 00000000..5fd38a0a --- /dev/null +++ b/internal/hooks/builtin/allowlist.go @@ -0,0 +1,24 @@ +package builtin + +import "github.com/google/uuid" + +// AllowlistFor returns the `mutable_fields` list for a registered builtin +// hook row keyed by the per-event UUID (see BuiltinEventID). Returns nil when +// the id is not a registered builtin — caller (dispatcher) treats nil as +// "strip all mutations" for defense-in-depth. +func AllowlistFor(id uuid.UUID) []string { + regMu.RLock() + defer regMu.RUnlock() + if s, ok := eventIDSpec[id]; ok { + return s.MutableFields + } + return nil +} + +// IsBuiltin reports whether id belongs to a registered builtin row. +func IsBuiltin(id uuid.UUID) bool { + regMu.RLock() + defer regMu.RUnlock() + _, ok := eventIDSpec[id] + return ok +} diff --git a/internal/hooks/builtin/builtins.yaml b/internal/hooks/builtin/builtins.yaml new file mode 100644 index 00000000..bf1a13aa --- /dev/null +++ b/internal/hooks/builtin/builtins.yaml @@ -0,0 +1,28 @@ +# GoClaw builtin hooks registry. +# +# Each entry is shipped with the binary (`//go:embed *.js`) and seeded into +# agent_hooks at startup with source='builtin' and a stable UUIDv5. Users may +# toggle `enabled` on these rows but cannot edit content — the embedded JS is +# the canonical source at every boot. +# +# Schema: +# id kebab-case stable identifier (never rename — breaks UUIDv5) +# version integer; bump to force startup overwrite of matching rows +# events list of HookEvent values (session_start, user_prompt_submit, +# pre_tool_use, post_tool_use, stop, subagent_start, +# subagent_stop). One DB row is created per event. +# scope global | tenant | agent (builtins default to global) +# matcher optional regex applied to tool_name (pre/post_tool_use only) +# if_expr optional CEL expression pre-filter +# timeout_ms per-hook wall-clock budget +# on_timeout allow | block (fail-closed for blocking events) +# priority higher = runs earlier in the chain +# mutable_fields per-hook allowlist of dotted paths the script may mutate +# via updatedInput (e.g. rawInput, toolInput.command) +# source_file name of the .js file sitting next to this YAML +# description short human summary; surfaced in docs/UI +# +# Phase 04 ships this registry empty. Phase 05 adds the first exemplar +# (pii-redactor). When adding a builtin, also add the .js file + remove +# the _placeholder.js once at least one real source_file exists. +builtins: [] diff --git a/internal/hooks/builtin/loader.go b/internal/hooks/builtin/loader.go new file mode 100644 index 00000000..c689fd98 --- /dev/null +++ b/internal/hooks/builtin/loader.go @@ -0,0 +1,130 @@ +// Package builtin discovers, parses, and seeds builtin hook rows. +// Builtins are shipped with the binary via //go:embed and are the only hooks +// allowed to mutate event input (source-tier gate in dispatcher). +// +// Lifecycle: cmd/gateway_managed.go calls Load() once at startup (before +// dispatcher wiring), then Seed(ctx, hookStore, cfg.Hooks) after migrations. +// Seed is idempotent via stable UUIDv5 IDs derived from the builtin's kebab +// name; safe to run on every boot. +package builtin + +import ( + "embed" + "fmt" + "sync" + + "github.com/google/uuid" + "gopkg.in/yaml.v3" +) + +// fs carries the registry YAML and one .js per builtin. A `//go:embed *.js` +// directive errors at build time if zero .js files exist, so Phase 04 ships a +// tiny `_placeholder.js` alongside an empty `builtins:` list. Phase 05 adds +// pii-redactor.js and removes the placeholder. +// +//go:embed *.yaml *.js +var fs embed.FS + +// BuiltinNamespace is the stable UUIDv5 namespace for all builtin hook IDs. +// Precomputed via uuid.NewSHA1(uuid.NameSpaceDNS, []byte("goclaw.hooks.builtin")) +// so every GoClaw build produces the same hash and the seeded rows persist +// across restarts / migrations. +var BuiltinNamespace = uuid.MustParse("082ab084-a25f-52b4-a4a4-eb8a816bd9a8") + +// BuiltinID returns the stable UUIDv5 derived from a builtin's kebab name. +// Used by Seed + dispatcher AllowlistFor lookups. +func BuiltinID(name string) uuid.UUID { + return uuid.NewSHA1(BuiltinNamespace, []byte(name)) +} + +// BuiltinEventID returns the stable UUIDv5 for a single (builtin, event) +// pair. Each YAML spec lists N events, each materialized as one DB row. +func BuiltinEventID(name, event string) uuid.UUID { + return uuid.NewSHA1(BuiltinNamespace, []byte(name+"/"+event)) +} + +// Spec mirrors one entry in builtins.yaml. All fields optional except id, +// events, source_file; loader validates at parse time. +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"` +} + +type catalog struct { + Builtins []Spec `yaml:"builtins"` +} + +// Registry holds the parsed view of builtins.yaml plus fast allowlist lookup. +// Populated once at Load(); read concurrently afterwards under regMu. +var ( + regMu sync.RWMutex + specs []Spec // ordered list for deterministic seeding + eventIDSpec map[uuid.UUID]*Spec // per-event UUID → owning Spec + sourceCache map[string][]byte // source_file → file bytes +) + +// Load parses builtins.yaml and caches the registry in memory. Call once at +// startup before Seed/AllowlistFor. Idempotent — subsequent calls replace the +// cache so tests can reload after fixture edits. +func Load() error { + raw, err := fs.ReadFile("builtins.yaml") + if err != nil { + return fmt.Errorf("builtin: read yaml: %w", err) + } + var cat catalog + if err := yaml.Unmarshal(raw, &cat); err != nil { + return fmt.Errorf("builtin: parse yaml: %w", err) + } + + idx := make(map[uuid.UUID]*Spec, len(cat.Builtins)*2) + srcs := make(map[string][]byte, len(cat.Builtins)) + for i := range cat.Builtins { + s := &cat.Builtins[i] + if s.ID == "" || s.SourceFile == "" || len(s.Events) == 0 { + return fmt.Errorf("builtin: incomplete spec: %+v", *s) + } + src, err := fs.ReadFile(s.SourceFile) + if err != nil { + return fmt.Errorf("builtin: missing source file %q: %w", s.SourceFile, err) + } + srcs[s.SourceFile] = src + for _, ev := range s.Events { + idx[BuiltinEventID(s.ID, ev)] = s + } + } + + regMu.Lock() + specs = cat.Builtins + eventIDSpec = idx + sourceCache = srcs + regMu.Unlock() + return nil +} + +// RegisteredSpecs returns a copy of the loaded specs for testing / callers +// that need to iterate independently of Load()'s internal state. +func RegisteredSpecs() []Spec { + regMu.RLock() + defer regMu.RUnlock() + out := make([]Spec, len(specs)) + copy(out, specs) + return out +} + +// source looks up the embedded JS bytes for a SourceFile key. +func source(file string) ([]byte, bool) { + regMu.RLock() + defer regMu.RUnlock() + b, ok := sourceCache[file] + return b, ok +} diff --git a/internal/hooks/builtin/loader_test.go b/internal/hooks/builtin/loader_test.go new file mode 100644 index 00000000..6f191976 --- /dev/null +++ b/internal/hooks/builtin/loader_test.go @@ -0,0 +1,96 @@ +package builtin + +import ( + "testing" + + "github.com/google/uuid" +) + +// TestLoad_EmptyYaml ensures the Phase 04 ship state (empty builtins list + +// _placeholder.js) loads without error and leaves the registry empty. +func TestLoad_EmptyYaml(t *testing.T) { + if err := Load(); err != nil { + t.Fatalf("Load: %v", err) + } + if got := RegisteredSpecs(); len(got) != 0 { + t.Errorf("registered=%d, want 0 (empty builtins.yaml)", len(got)) + } +} + +// TestBuiltinNamespace_Stable guards against a silent namespace change that +// would orphan every already-seeded row at the next boot. +func TestBuiltinNamespace_Stable(t *testing.T) { + want := uuid.MustParse("082ab084-a25f-52b4-a4a4-eb8a816bd9a8") + if BuiltinNamespace != want { + t.Fatalf("namespace drifted: have %s, want %s", BuiltinNamespace, want) + } +} + +// TestBuiltinID_Deterministic locks in the per-id hash so that a renamed id +// gets caught in review — downgrade-safety depends on stable IDs. +func TestBuiltinID_Deterministic(t *testing.T) { + a := BuiltinID("pii-redactor") + b := BuiltinID("pii-redactor") + if a != b { + t.Fatalf("BuiltinID non-deterministic: %s vs %s", a, b) + } + c := BuiltinID("sql-guard") + if a == c { + t.Fatal("distinct ids produced equal UUID — namespace collision") + } +} + +// TestBuiltinEventID_Distinct ensures each event of a multi-event spec gets +// its own row id so per-event enabled toggles don't clobber siblings. +func TestBuiltinEventID_Distinct(t *testing.T) { + a := BuiltinEventID("pii-redactor", "user_prompt_submit") + b := BuiltinEventID("pii-redactor", "pre_tool_use") + if a == b { + t.Fatal("event UUIDs collided for different events") + } + if a == BuiltinID("pii-redactor") { + t.Fatal("per-event UUID must differ from bare spec id") + } +} + +// TestAllowlistFor_Unknown returns nil for ids that aren't registered. +// Dispatcher treats nil as strip → defense-in-depth against scripts with +// forged source='builtin' on a random UUID. +func TestAllowlistFor_Unknown(t *testing.T) { + if err := Load(); err != nil { + t.Fatalf("Load: %v", err) + } + if got := AllowlistFor(uuid.New()); got != nil { + t.Errorf("unknown id → allowlist=%v, want nil", got) + } +} + +func TestIsBuiltin_Unknown(t *testing.T) { + if err := Load(); err != nil { + t.Fatalf("Load: %v", err) + } + if IsBuiltin(uuid.New()) { + t.Error("random UUID reported as builtin") + } +} + +// TestMetadataVersion covers the JSONB float64 decode path plus int fast path. +func TestMetadataVersion(t *testing.T) { + cases := []struct { + name string + meta map[string]any + want int + }{ + {"nil", nil, 0}, + {"missing", map[string]any{}, 0}, + {"int", map[string]any{"version": 3}, 3}, + {"int64", map[string]any{"version": int64(4)}, 4}, + {"float64", map[string]any{"version": float64(5)}, 5}, + {"string-ignored", map[string]any{"version": "oops"}, 0}, + } + for _, tc := range cases { + if got := metadataVersion(tc.meta); got != tc.want { + t.Errorf("%s: got %d want %d", tc.name, got, tc.want) + } + } +} diff --git a/internal/hooks/builtin/seed.go b/internal/hooks/builtin/seed.go new file mode 100644 index 00000000..1d72533b --- /dev/null +++ b/internal/hooks/builtin/seed.go @@ -0,0 +1,160 @@ +package builtin + +import ( + "context" + "fmt" + "log/slog" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/hooks" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// Seed UPSERTs every registered builtin into hookStore following the +// version-reconciliation rules in phase-04: +// +// - DB row missing → INSERT one row per event with source='builtin'. +// - DB version < embed version → UPDATE content (preserve enabled toggle). +// - DB version == embed version → no-op. +// - DB version > embed version → WARN + keep DB (no rollback). +// +// Runs AFTER migrations and BEFORE dispatcher handler registration. Uses +// store.RoleOwner + hooks.WithSeedBypass to traverse the tenant-scope guard +// and the builtin-readonly Update protection; both markers are process-local. +func Seed(ctx context.Context, hookStore hooks.HookStore, cfg config.HooksConfig) error { + regMu.RLock() + all := make([]Spec, len(specs)) + copy(all, specs) + regMu.RUnlock() + + disabled := make(map[string]struct{}, len(cfg.BuiltinDisable)) + for _, d := range cfg.BuiltinDisable { + disabled[d] = struct{}{} + } + + ctx = store.WithRole(ctx, store.RoleOwner) + ctx = hooks.WithSeedBypass(ctx) + + for i := range all { + s := all[i] + _, forceDisabled := disabled[s.ID] + if err := seedOne(ctx, hookStore, s, forceDisabled); err != nil { + slog.Warn("hooks.builtin_seed_failed", "id", s.ID, "err", err) + } + } + return nil +} + +// seedOne reconciles one spec (N events → N rows) against the store. +func seedOne(ctx context.Context, hs hooks.HookStore, s Spec, forceDisabled bool) error { + src, ok := source(s.SourceFile) + if !ok { + return fmt.Errorf("source not cached: %s", s.SourceFile) + } + + cfgMap := map[string]any{"source": string(src)} + metadata := map[string]any{ + "builtin": true, + "version": s.Version, + "description": s.Description, + } + + for _, ev := range s.Events { + id := BuiltinEventID(s.ID, ev) + if err := reconcile(ctx, hs, id, s, ev, cfgMap, metadata, forceDisabled); err != nil { + return fmt.Errorf("%s/%s: %w", s.ID, ev, err) + } + } + return nil +} + +// reconcile handles insert/update/noop for a single builtin-event row. +func reconcile( + ctx context.Context, + hs hooks.HookStore, + id uuid.UUID, + s Spec, + event string, + cfgMap, metadata map[string]any, + forceDisabled bool, +) error { + current, err := hs.GetByID(ctx, id) + if err != nil { + return fmt.Errorf("get: %w", err) + } + + if current == nil { + cfg := hooks.HookConfig{ + ID: id, + TenantID: hooks.SentinelTenantID, + Event: hooks.HookEvent(event), + HandlerType: hooks.HandlerScript, + Scope: hooks.Scope(s.Scope), + Config: cfgMap, + Matcher: s.Matcher, + IfExpr: s.IfExpr, + TimeoutMS: s.TimeoutMS, + OnTimeout: hooks.Decision(s.OnTimeout), + Priority: s.Priority, + Enabled: !forceDisabled, + Source: hooks.SourceBuiltin, + Metadata: metadata, + } + if _, err := hs.Create(ctx, cfg); err != nil { + return fmt.Errorf("create: %w", err) + } + slog.Info("hooks.builtin_seeded", "id", s.ID, "event", event, "version", s.Version) + return nil + } + + dbVersion := metadataVersion(current.Metadata) + switch { + case s.Version > dbVersion: + patch := map[string]any{ + "config": cfgMap, + "matcher": s.Matcher, + "if_expr": s.IfExpr, + "timeout_ms": s.TimeoutMS, + "on_timeout": string(hooks.Decision(s.OnTimeout)), + "priority": s.Priority, + "metadata": metadata, + } + if err := hs.Update(ctx, id, patch); err != nil { + return fmt.Errorf("update: %w", err) + } + slog.Info("hooks.builtin_updated", "id", s.ID, "event", event, + "from_version", dbVersion, "to_version", s.Version) + case s.Version < dbVersion: + slog.Warn("hooks.builtin_downgrade_detected", + "id", s.ID, "event", event, + "db_version", dbVersion, "embed_version", s.Version, + "note", "DB content kept; no rollback performed") + } + + if forceDisabled && current.Enabled { + if err := hs.Update(ctx, id, map[string]any{"enabled": false}); err != nil { + return fmt.Errorf("force-disable: %w", err) + } + slog.Info("hooks.builtin_disabled_by_config", "id", s.ID, "event", event) + } + return nil +} + +// metadataVersion reads metadata["version"] tolerating JSONB float64 decoding. +func metadataVersion(m map[string]any) int { + if m == nil { + return 0 + } + switch v := m["version"].(type) { + case int: + return v + case int64: + return int(v) + case float64: + return int(v) + default: + return 0 + } +} diff --git a/internal/hooks/builtin/seed_test.go b/internal/hooks/builtin/seed_test.go new file mode 100644 index 00000000..5b39c76a --- /dev/null +++ b/internal/hooks/builtin/seed_test.go @@ -0,0 +1,255 @@ +package builtin + +import ( + "context" + "testing" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/hooks" +) + +// fakeHookStore is an in-memory HookStore just rich enough to test seed +// reconciliation. Implements only the methods the seeder touches (Create, +// GetByID, Update); others return zero values / nil. +type fakeHookStore struct { + rows map[uuid.UUID]*hooks.HookConfig +} + +func newFakeHookStore() *fakeHookStore { + return &fakeHookStore{rows: map[uuid.UUID]*hooks.HookConfig{}} +} + +func (f *fakeHookStore) Create(_ context.Context, cfg hooks.HookConfig) (uuid.UUID, error) { + if cfg.ID == uuid.Nil { + cfg.ID = uuid.Must(uuid.NewV7()) + } + if _, exists := f.rows[cfg.ID]; exists { + // Mimic DB primary-key conflict to catch non-idempotent seeds. + return uuid.Nil, errDuplicate + } + c := cfg + f.rows[cfg.ID] = &c + return cfg.ID, nil +} + +func (f *fakeHookStore) GetByID(_ context.Context, id uuid.UUID) (*hooks.HookConfig, error) { + if r, ok := f.rows[id]; ok { + c := *r + return &c, nil + } + return nil, nil +} + +func (f *fakeHookStore) List(context.Context, hooks.ListFilter) ([]hooks.HookConfig, error) { + return nil, nil +} + +func (f *fakeHookStore) Update(_ context.Context, id uuid.UUID, updates map[string]any) error { + r, ok := f.rows[id] + if !ok { + return errNotFound + } + for k, v := range updates { + switch k { + case "enabled": + if b, ok := v.(bool); ok { + r.Enabled = b + } + case "metadata": + if m, ok := v.(map[string]any); ok { + r.Metadata = m + } + case "config": + if m, ok := v.(map[string]any); ok { + r.Config = m + } + case "matcher": + if s, ok := v.(string); ok { + r.Matcher = s + } + case "if_expr": + if s, ok := v.(string); ok { + r.IfExpr = s + } + case "priority": + if n, ok := v.(int); ok { + r.Priority = n + } + case "timeout_ms": + if n, ok := v.(int); ok { + r.TimeoutMS = n + } + } + } + r.Version++ + return nil +} + +func (f *fakeHookStore) Delete(_ context.Context, id uuid.UUID) error { + delete(f.rows, id) + return nil +} + +func (f *fakeHookStore) ResolveForEvent(context.Context, hooks.Event) ([]hooks.HookConfig, error) { + return nil, nil +} + +func (f *fakeHookStore) WriteExecution(context.Context, hooks.HookExecution) error { return nil } + +type errStr string + +func (e errStr) Error() string { return string(e) } + +const ( + errDuplicate errStr = "fake: duplicate key" + errNotFound errStr = "fake: not found" +) + +// withTestRegistry installs a one-spec registry for the test and restores the +// prior state on cleanup. Serial access only — tests must not run in parallel. +func withTestRegistry(t *testing.T, s Spec, src []byte) { + t.Helper() + regMu.Lock() + prevSpecs := specs + prevIdx := eventIDSpec + prevSrc := sourceCache + specs = []Spec{s} + eventIDSpec = map[uuid.UUID]*Spec{} + for _, ev := range s.Events { + eventIDSpec[BuiltinEventID(s.ID, ev)] = &specs[0] + } + sourceCache = map[string][]byte{s.SourceFile: src} + regMu.Unlock() + t.Cleanup(func() { + regMu.Lock() + specs = prevSpecs + eventIDSpec = prevIdx + sourceCache = prevSrc + regMu.Unlock() + }) +} + +func fixtureSpec(version int) Spec { + return Spec{ + ID: "test-redactor", + Version: version, + Events: []string{"user_prompt_submit", "pre_tool_use"}, + Scope: "global", + TimeoutMS: 500, + OnTimeout: "allow", + Priority: 900, + MutableFields: []string{"rawInput"}, + SourceFile: "test-redactor.js", + Description: "test", + } +} + +// TestSeed_InsertsOneRowPerEvent verifies seed creates N rows for a spec with +// N events using stable UUIDv5s — this is the H9 idempotency check. +func TestSeed_InsertsOneRowPerEvent(t *testing.T) { + spec := fixtureSpec(1) + withTestRegistry(t, spec, []byte("// v1")) + + fs := newFakeHookStore() + if err := Seed(context.Background(), fs, config.HooksConfig{}); err != nil { + t.Fatalf("Seed: %v", err) + } + if got := len(fs.rows); got != len(spec.Events) { + t.Fatalf("rows=%d, want %d", got, len(spec.Events)) + } + for _, ev := range spec.Events { + id := BuiltinEventID(spec.ID, ev) + row, ok := fs.rows[id] + if !ok { + t.Fatalf("missing row for event %q", ev) + } + if row.Source != hooks.SourceBuiltin { + t.Errorf("source=%q, want builtin", row.Source) + } + if !row.Enabled { + t.Error("row disabled; expected enabled=true on fresh seed") + } + } +} + +// TestSeed_Idempotent validates H9 — re-running seed against the same registry +// creates zero additional rows. A regression here would manifest as duplicate +// rows every boot once the unique index is dropped by Phase 03. +func TestSeed_Idempotent(t *testing.T) { + spec := fixtureSpec(1) + withTestRegistry(t, spec, []byte("// v1")) + + fs := newFakeHookStore() + for i := range 3 { + if err := Seed(context.Background(), fs, config.HooksConfig{}); err != nil { + t.Fatalf("Seed iter=%d: %v", i, err) + } + } + if got := len(fs.rows); got != len(spec.Events) { + t.Fatalf("after 3 seeds rows=%d, want %d (duplicates!)", got, len(spec.Events)) + } +} + +// TestSeed_VersionBump updates existing rows when the embed version grows. +// Preserves the user's enabled toggle. +func TestSeed_VersionBump(t *testing.T) { + spec := fixtureSpec(1) + withTestRegistry(t, spec, []byte("// v1")) + fs := newFakeHookStore() + if err := Seed(context.Background(), fs, config.HooksConfig{}); err != nil { + t.Fatal(err) + } + // Simulate user disabling a row. + id := BuiltinEventID(spec.ID, "pre_tool_use") + fs.rows[id].Enabled = false + + // Replace registry with v2. + withTestRegistry(t, fixtureSpec(2), []byte("// v2")) + if err := Seed(context.Background(), fs, config.HooksConfig{}); err != nil { + t.Fatal(err) + } + if ver := fs.rows[id].Metadata["version"]; ver != 2 { + t.Errorf("version not bumped: %v", ver) + } + if fs.rows[id].Enabled { + t.Error("seed overwrote user's disabled toggle") + } +} + +// TestSeed_DowngradeDetected keeps the DB row intact when the embed version +// is older than the DB version (rollback scenario). +func TestSeed_DowngradeDetected(t *testing.T) { + withTestRegistry(t, fixtureSpec(3), []byte("// v3")) + fs := newFakeHookStore() + if err := Seed(context.Background(), fs, config.HooksConfig{}); err != nil { + t.Fatal(err) + } + id := BuiltinEventID("test-redactor", "user_prompt_submit") + gotV := fs.rows[id].Metadata["version"] + + // Now simulate rollback to v2 embed. + withTestRegistry(t, fixtureSpec(2), []byte("// v2")) + if err := Seed(context.Background(), fs, config.HooksConfig{}); err != nil { + t.Fatal(err) + } + if gotV2 := fs.rows[id].Metadata["version"]; gotV2 != gotV { + t.Errorf("downgrade mutated row: %v → %v", gotV, gotV2) + } +} + +// TestSeed_BuiltinDisableForcesOff applies the operator escape-hatch list. +func TestSeed_BuiltinDisableForcesOff(t *testing.T) { + withTestRegistry(t, fixtureSpec(1), []byte("// v1")) + fs := newFakeHookStore() + cfg := config.HooksConfig{BuiltinDisable: []string{"test-redactor"}} + if err := Seed(context.Background(), fs, cfg); err != nil { + t.Fatal(err) + } + for id, row := range fs.rows { + if row.Enabled { + t.Errorf("row %s enabled despite builtin_disable", id) + } + } +} diff --git a/internal/hooks/dispatcher.go b/internal/hooks/dispatcher.go index 6e96531c..a80b4703 100644 --- a/internal/hooks/dispatcher.go +++ b/internal/hooks/dispatcher.go @@ -318,10 +318,27 @@ func applyBuiltinMutation(ev *Event, updated map[string]any, allowlist []string) } } -// builtinAllowlistFor returns the field allowlist for a builtin hook. -// Phase 03 ships a permissive default; Phase 04 overrides with a per-hook -// allowlist loaded from builtins.yaml at startup. -func builtinAllowlistFor(_ uuid.UUID) []string { +// builtinAllowlistLookup is set at startup by the builtin package wiring +// (cmd/gateway_managed.go calls SetBuiltinAllowlistLookup(builtin.AllowlistFor)). +// When nil (tests that don't wire the registry) we fall back to the Phase 03 +// permissive default so legacy mutation tests stay green. +var builtinAllowlistLookup func(id uuid.UUID) []string + +// SetBuiltinAllowlistLookup installs the per-id allowlist function. Called +// once at startup from cmd/gateway_managed.go. Safe to leave unset in tests — +// callers get the permissive default (rawInput + toolInput) below. +func SetBuiltinAllowlistLookup(f func(uuid.UUID) []string) { + builtinAllowlistLookup = f +} + +// builtinAllowlistFor returns the field allowlist for a builtin hook row. +// Registered builtin → YAML mutable_fields. Unknown id under a wired lookup +// → empty allowlist (mutations stripped, defense-in-depth). Unset lookup +// (unit tests) → Phase 03 permissive default. +func builtinAllowlistFor(id uuid.UUID) []string { + if builtinAllowlistLookup != nil { + return builtinAllowlistLookup(id) + } return []string{"rawInput", "toolInput"} } diff --git a/internal/hooks/store.go b/internal/hooks/store.go index 65b3866e..7d067967 100644 --- a/internal/hooks/store.go +++ b/internal/hooks/store.go @@ -2,10 +2,36 @@ package hooks import ( "context" + "errors" "github.com/google/uuid" ) +// ErrBuiltinReadOnly is returned by Update/Delete when a caller attempts to +// modify a row with source='builtin' beyond toggling `enabled`. Builtin rows +// are canonical — content comes from embedded JS at every startup (Phase 04 +// seed). UI should render them read-only except for the enabled toggle. +// Message is surfaced via i18n key hook.builtin_readonly. +var ErrBuiltinReadOnly = errors.New("hook: builtin row is read-only except enabled toggle") + +// seedBypassKey is the private context key used by Phase 04 builtin seeder to +// bypass the Update/Delete builtin-readonly guard. Process-private: only the +// seeder sets it. Ensures user-facing writes still fail ErrBuiltinReadOnly. +type seedBypassKey struct{} + +// WithSeedBypass marks ctx as originating from the builtin seeder. Store +// implementations MUST consult IsSeedBypass(ctx) before rejecting writes on +// builtin rows. Any other caller is denied. +func WithSeedBypass(ctx context.Context) context.Context { + return context.WithValue(ctx, seedBypassKey{}, true) +} + +// IsSeedBypass reports whether ctx carries the builtin-seeder bypass marker. +func IsSeedBypass(ctx context.Context) bool { + v, _ := ctx.Value(seedBypassKey{}).(bool) + return v +} + // ListFilter narrows results from HookStore.List. type ListFilter struct { TenantID *uuid.UUID diff --git a/internal/i18n/catalog_en.go b/internal/i18n/catalog_en.go index f38d9cec..426d6328 100644 --- a/internal/i18n/catalog_en.go +++ b/internal/i18n/catalog_en.go @@ -210,5 +210,6 @@ func init() { MsgHookCircuitBreakerTripped: "hook auto-disabled after repeated failures", MsgHookBudgetExceeded: "tenant hook token budget exceeded", MsgHookPerTurnCapReached: "hook invocation per-turn cap reached", + MsgHookBuiltinReadOnly: "builtin hooks are read-only except for the enabled toggle", }) } diff --git a/internal/i18n/catalog_vi.go b/internal/i18n/catalog_vi.go index 6c46b1fe..86dbfe0d 100644 --- a/internal/i18n/catalog_vi.go +++ b/internal/i18n/catalog_vi.go @@ -210,5 +210,6 @@ func init() { MsgHookCircuitBreakerTripped: "hook đã tự tắt sau nhiều lần thất bại liên tiếp", MsgHookBudgetExceeded: "tenant đã vượt ngân sách token cho hook", MsgHookPerTurnCapReached: "đã đạt giới hạn số lần gọi hook trong một lượt", + MsgHookBuiltinReadOnly: "hook dựng sẵn chỉ cho phép bật/tắt, không thể chỉnh sửa", }) } diff --git a/internal/i18n/catalog_zh.go b/internal/i18n/catalog_zh.go index eb4451f4..f8f2548f 100644 --- a/internal/i18n/catalog_zh.go +++ b/internal/i18n/catalog_zh.go @@ -210,5 +210,6 @@ func init() { MsgHookCircuitBreakerTripped: "钩子在多次失败后已自动禁用", MsgHookBudgetExceeded: "租户钩子令牌预算已耗尽", MsgHookPerTurnCapReached: "单轮钩子调用次数已达上限", + MsgHookBuiltinReadOnly: "内置钩子只读,仅允许切换启用状态", }) } diff --git a/internal/i18n/keys.go b/internal/i18n/keys.go index eeb92464..4ad20a2b 100644 --- a/internal/i18n/keys.go +++ b/internal/i18n/keys.go @@ -214,4 +214,5 @@ const ( MsgHookCircuitBreakerTripped = "hook.circuit_breaker_tripped" // "hook auto-disabled after repeated failures" MsgHookBudgetExceeded = "hook.budget_exceeded" // "tenant hook token budget exceeded" MsgHookPerTurnCapReached = "hook.per_turn_cap_reached" // "hook invocation per-turn cap reached" + MsgHookBuiltinReadOnly = "hook.builtin_readonly" // "builtin hooks are read-only except for the enabled toggle" ) diff --git a/internal/store/pg/hooks.go b/internal/store/pg/hooks.go index 18712687..871f8153 100644 --- a/internal/store/pg/hooks.go +++ b/internal/store/pg/hooks.go @@ -199,6 +199,25 @@ func (s *PGHookStore) Update(ctx context.Context, id uuid.UUID, updates map[stri return fmt.Errorf("callers must not include 'version' in update map") } + // Builtin-row protection (Phase 04). Rows with source='builtin' are canonical + // embed-seeded artifacts: UI/user writes may only toggle `enabled`. The + // builtin seeder itself passes through via hooks.WithSeedBypass ctx. + // Fail-closed: a transient GetByID error must not let a non-enabled patch + // slip through against a builtin row. + if !hooks.IsSeedBypass(ctx) { + current, err := s.GetByID(ctx, id) + if err != nil { + return fmt.Errorf("builtin guard: %w", err) + } + if current != nil && current.Source == hooks.SourceBuiltin { + for k := range updates { + if k != "enabled" { + return hooks.ErrBuiltinReadOnly + } + } + } + } + // Marshal map/slice values to JSON for JSONB columns. for k, v := range updates { switch k { @@ -256,6 +275,18 @@ func (s *PGHookStore) Update(ctx context.Context, id uuid.UUID, updates map[stri // ─── Delete ────────────────────────────────────────────────────────────────── func (s *PGHookStore) Delete(ctx context.Context, id uuid.UUID) error { + // Builtin-row protection (Phase 04): reject unless caller marked seed-bypass. + // Fail-closed on GetByID errors so we don't accidentally delete a builtin. + if !hooks.IsSeedBypass(ctx) { + current, err := s.GetByID(ctx, id) + if err != nil { + return fmt.Errorf("builtin guard: %w", err) + } + if current != nil && current.Source == hooks.SourceBuiltin { + return hooks.ErrBuiltinReadOnly + } + } + q := "DELETE FROM agent_hooks WHERE id = $1" args := []any{id} diff --git a/internal/store/pg/hooks_test.go b/internal/store/pg/hooks_test.go index a3145f92..e1ac3792 100644 --- a/internal/store/pg/hooks_test.go +++ b/internal/store/pg/hooks_test.go @@ -3,6 +3,7 @@ package pg import ( "context" "database/sql" + "errors" "os" "testing" "time" @@ -421,3 +422,52 @@ func TestPGHookStore_CreateHonorsFixedID(t *testing.T) { t.Fatal("Create returned nil id for cfg.ID=uuid.Nil path") } } + +// ─── Phase 04: builtin-row readonly protection ─────────────────────────────── + +// TestPGHookStore_BuiltinReadOnly exercises the Phase 04 guard: user-facing +// writes on a source='builtin' row may only toggle enabled; every other patch +// must surface ErrBuiltinReadOnly. The seed bypass marker unlocks full writes +// for the loader package only. +func TestPGHookStore_BuiltinReadOnly(t *testing.T) { + db := hooksTestDB(t) + tenantID, _ := seedTenantAndAgent(t, db) + s := NewPGHookStore(db) + + // Seed a builtin row via WithSeedBypass (the only authorized path). + seedCtx := hooks.WithSeedBypass(store.WithRole(masterCtx(), store.RoleOwner)) + cfg := minimalHook(hooks.SentinelTenantID, hooks.EventUserPromptSubmit) + cfg.Source = hooks.SourceBuiltin + cfg.Scope = hooks.ScopeGlobal + cfg.HandlerType = hooks.HandlerScript + cfg.Config = map[string]any{"source": "// v1"} + fixedID := uuid.MustParse("11111111-2222-3333-4444-555555555555") + cfg.ID = fixedID + id, err := s.Create(seedCtx, cfg) + if err != nil { + t.Fatalf("seed create: %v", err) + } + t.Cleanup(func() { s.Delete(seedCtx, id) }) + + // User ctx: tenant-scoped. Non-enabled patch must be rejected. + userCtx := tenantScopedCtx(tenantID) + err = s.Update(userCtx, id, map[string]any{"matcher": "evil"}) + if !errors.Is(err, hooks.ErrBuiltinReadOnly) { + t.Fatalf("Update(matcher) err=%v, want ErrBuiltinReadOnly", err) + } + + // Enabled toggle through master context is allowed. + if err := s.Update(masterCtx(), id, map[string]any{"enabled": false}); err != nil { + t.Fatalf("Update(enabled) should succeed on builtin: %v", err) + } + + // Delete blocked for users. + if err := s.Delete(userCtx, id); !errors.Is(err, hooks.ErrBuiltinReadOnly) { + t.Fatalf("Delete user err=%v, want ErrBuiltinReadOnly", err) + } + + // Seed bypass unlocks full writes (round-trip to prove). + if err := s.Update(seedCtx, id, map[string]any{"matcher": "ok"}); err != nil { + t.Fatalf("seed-bypass Update should succeed: %v", err) + } +} diff --git a/internal/store/sqlitestore/hooks.go b/internal/store/sqlitestore/hooks.go index ef0540f3..f99da3fc 100644 --- a/internal/store/sqlitestore/hooks.go +++ b/internal/store/sqlitestore/hooks.go @@ -205,6 +205,24 @@ func (s *SqliteHookStore) Update(ctx context.Context, id uuid.UUID, updates map[ return fmt.Errorf("callers must not include 'version' in update map") } + // Builtin-row protection (Phase 04). User/UI writes may only toggle + // `enabled`; the builtin seeder itself bypasses this via hooks.WithSeedBypass. + // Fail-closed on GetByID errors — silently passing here could let wide + // patches through on a transient DB read failure. + if !hooks.IsSeedBypass(ctx) { + current, err := s.GetByID(ctx, id) + if err != nil { + return fmt.Errorf("builtin guard: %w", err) + } + if current != nil && current.Source == hooks.SourceBuiltin { + for k := range updates { + if k != "enabled" { + return hooks.ErrBuiltinReadOnly + } + } + } + } + // Marshal map/slice values to JSON strings for SQLite TEXT columns. for k, v := range updates { switch k { @@ -258,6 +276,18 @@ func (s *SqliteHookStore) Update(ctx context.Context, id uuid.UUID, updates map[ // ─── Delete ────────────────────────────────────────────────────────────────── func (s *SqliteHookStore) Delete(ctx context.Context, id uuid.UUID) error { + // Builtin-row protection (Phase 04): reject unless caller marked seed-bypass. + // Fail-closed on GetByID errors. + if !hooks.IsSeedBypass(ctx) { + current, err := s.GetByID(ctx, id) + if err != nil { + return fmt.Errorf("builtin guard: %w", err) + } + if current != nil && current.Source == hooks.SourceBuiltin { + return hooks.ErrBuiltinReadOnly + } + } + q := "DELETE FROM agent_hooks WHERE id = ?" args := []any{id.String()} diff --git a/internal/store/sqlitestore/hooks_test.go b/internal/store/sqlitestore/hooks_test.go index 1e8c27e2..4aebf4a6 100644 --- a/internal/store/sqlitestore/hooks_test.go +++ b/internal/store/sqlitestore/hooks_test.go @@ -5,6 +5,7 @@ package sqlitestore import ( "context" "database/sql" + "errors" "path/filepath" "testing" "time" @@ -438,3 +439,50 @@ func TestSQLiteHookStore_GlobalScopeVisibleToTenant(t *testing.T) { t.Error("global hook not visible in ResolveForEvent from tenant scope") } } + +// TestSQLiteHookStore_BuiltinReadOnly mirrors the PG test: user-facing writes +// on source='builtin' rows may only toggle enabled; WithSeedBypass unlocks. +func TestSQLiteHookStore_BuiltinReadOnly(t *testing.T) { + db := newHookTestDB(t) + tenantID, _ := seedHookTenantAgent(t, db) + s := NewSQLiteHookStore(db) + + seedCtx := hooks.WithSeedBypass(store.WithRole(sqliteMasterCtx(), store.RoleOwner)) + + cfg := hooks.HookConfig{ + ID: uuid.MustParse("11111111-2222-3333-4444-555555555555"), + TenantID: hooks.SentinelTenantID, + Event: hooks.EventUserPromptSubmit, + HandlerType: hooks.HandlerScript, + Scope: hooks.ScopeGlobal, + Config: map[string]any{"source": "// v1"}, + Metadata: map[string]any{"builtin": true, "version": 1}, + TimeoutMS: 500, + OnTimeout: hooks.DecisionAllow, + Source: hooks.SourceBuiltin, + Enabled: true, + } + id, err := s.Create(seedCtx, cfg) + if err != nil { + t.Fatalf("seed create: %v", err) + } + t.Cleanup(func() { s.Delete(seedCtx, id) }) + + userCtx := sqliteTenantCtx(tenantID) + + if err := s.Update(userCtx, id, map[string]any{"matcher": "evil"}); !errors.Is(err, hooks.ErrBuiltinReadOnly) { + t.Fatalf("Update(matcher) err=%v, want ErrBuiltinReadOnly", err) + } + + if err := s.Update(sqliteMasterCtx(), id, map[string]any{"enabled": false}); err != nil { + t.Fatalf("Update(enabled) should succeed: %v", err) + } + + if err := s.Delete(userCtx, id); !errors.Is(err, hooks.ErrBuiltinReadOnly) { + t.Fatalf("Delete user err=%v, want ErrBuiltinReadOnly", err) + } + + if err := s.Update(seedCtx, id, map[string]any{"matcher": "ok"}); err != nil { + t.Fatalf("seed-bypass Update should succeed: %v", err) + } +}