From b7592bcacfd340382e4b2a3e780476d513dad591 Mon Sep 17 00:00:00 2001 From: viettranx Date: Wed, 15 Apr 2026 23:03:11 +0700 Subject: [PATCH] =?UTF-8?q?feat(hooks/script):=20integrate=20script=20hand?= =?UTF-8?q?ler=20=E2=80=94=20FireResult=20+=20source-tier=20gate=20+=20mig?= =?UTF-8?q?ration?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wave 1 Phase 03. Wires the Phase 02 Goja handler into the dispatcher, widens the DB schema for script + builtin sources, and refactors Dispatcher.Fire to return a FireResult so builtin-source hooks can mutate event input. Dispatcher: - FireResult{Decision, UpdatedToolInput, UpdatedRawInput} replaces the plain Decision return. stdDispatcher.runSync keeps a local evMut copy so a builtin-source hook's updatedInput flows to downstream hooks in the chain and to the caller; non-builtin (source=ui) script mutations are dropped + WARN-logged (defense-in-depth; source tier enforced at the dispatcher, not only at the handler). - applyBuiltinMutation + placeholder builtinAllowlistFor (rawInput, toolInput) — Phase 04 overrides the allowlist from builtins.yaml. - noopDispatcher returns FireResult{DecisionAllow}. - Script mutation tests: TestDispatcher_ScriptMutation_BuiltinSourceApplies, TestDispatcher_ScriptMutation_UISourceDenied. Pipeline + callers (14 Fire sites refactored): - FireHook wrapper returns FireResult; context_stage.go:52 applies UpdatedRawInput → state.Input.Message; tool_stage.go:52 applies UpdatedToolInput → tc.Arguments before ExecuteToolCall. - delegate_bridge + delegate_tool keep the Decision branch; Updated* ignored. - dispatcher_test / delegate_bridge_test / delegate_tool_hooks_test / integration test helpers updated for the new shape. Validation: - edition_gate allows HandlerScript on every edition (sandboxed, no shell escape surface). - config.validateHandler rejects empty source, source > 32 KiB, goja compile error; validateTimeout rejects on_timeout=ask|defer (reserved). - Six new config tests cover the script path. Migrations: - PG migration 000053 relaxes handler_type + source CHECKs and drops uq_hooks_{global,tenant,agent} — scripts routinely want many small hooks per event. RequiredSchemaVersion → 53. - SQLite SchemaVersion → 21; patch 20 is a SELECT 1 placeholder because SQLite can't ALTER a CHECK — rebuildAgentHooksV21 runs outside the migration tx (parallel to backfillV16) to rename/recreate the table with widened CHECKs. schema.sql fresh-DB path matches. - H9 fix: PGHookStore.Create + SqliteHookStore.Create honor caller-provided cfg.ID (uuid.Nil falls back to UUIDv7), unblocking Phase 04 idempotent UUIDv5 seed. TestCreateHonorsFixedID on both stores. Config: - config.HooksConfig{ScriptConcurrency, ScriptPerTenantConcurrency, ScriptCacheSize, BuiltinDisable}; buildHookHandlers wires the script handler with the caps from appCfg.Hooks. Gates green: go build ./... + sqliteonly, go test -race -count=1 across hooks/pipeline/sqlitestore. --- cmd/gateway_hooks.go | 13 +- cmd/gateway_managed.go | 2 +- internal/config/config.go | 18 +++ internal/hooks/config.go | 30 +++- internal/hooks/config_test.go | 63 ++++++++ internal/hooks/delegate_bridge.go | 2 + internal/hooks/delegate_bridge_test.go | 4 +- internal/hooks/dispatcher.go | 150 +++++++++++++++--- internal/hooks/dispatcher_test.go | 127 ++++++++++++--- internal/hooks/edition_gate.go | 4 +- internal/hooks/types.go | 19 +++ internal/pipeline/context_stage.go | 10 +- internal/pipeline/deps.go | 10 +- internal/pipeline/tool_stage.go | 10 +- internal/store/pg/hooks.go | 9 +- internal/store/pg/hooks_test.go | 35 ++++ internal/store/sqlitestore/hooks.go | 8 +- internal/store/sqlitestore/hooks_test.go | 29 +++- internal/store/sqlitestore/schema.go | 20 ++- internal/store/sqlitestore/schema.sql | 21 +-- .../schema_rebuild_agent_hooks_v21.go | 84 ++++++++++ internal/tools/delegate_tool.go | 6 +- internal/tools/delegate_tool_hooks_test.go | 4 +- internal/upgrade/version.go | 2 +- ...53_agent_hooks_script_and_builtin.down.sql | 21 +++ ...0053_agent_hooks_script_and_builtin.up.sql | 21 +++ tests/integration/hooks_chaos_test.go | 24 +-- tests/integration/hooks_e2e_test.go | 24 +-- tests/integration/hooks_pipeline_test.go | 18 +-- 29 files changed, 663 insertions(+), 125 deletions(-) create mode 100644 internal/store/sqlitestore/schema_rebuild_agent_hooks_v21.go create mode 100644 migrations/000053_agent_hooks_script_and_builtin.down.sql create mode 100644 migrations/000053_agent_hooks_script_and_builtin.up.sql diff --git a/cmd/gateway_hooks.go b/cmd/gateway_hooks.go index 68e9774b..26b35887 100644 --- a/cmd/gateway_hooks.go +++ b/cmd/gateway_hooks.go @@ -4,6 +4,7 @@ import ( "os" "time" + "github.com/nextlevelbuilder/goclaw/internal/config" "github.com/nextlevelbuilder/goclaw/internal/edition" "github.com/nextlevelbuilder/goclaw/internal/hooks" hookhandlers "github.com/nextlevelbuilder/goclaw/internal/hooks/handlers" @@ -26,7 +27,7 @@ var sharedHookHandlers map[hooks.HandlerType]hooks.Handler // Budget wiring (C1 fix): the PromptHandler receives a budget.Store bound // to pg.NewPGHookBudget so token spend is atomically deducted per tenant. // When the DB handle is unavailable, budget falls back to nil (Lite desktop). -func buildHookHandlers(stores *store.Stores, providerReg *providers.Registry) map[hooks.HandlerType]hooks.Handler { +func buildHookHandlers(stores *store.Stores, providerReg *providers.Registry, hooksCfg config.HooksConfig) map[hooks.HandlerType]hooks.Handler { encryptKey := os.Getenv("GOCLAW_ENCRYPTION_KEY") var budgetStore *budget.Store @@ -40,6 +41,15 @@ func buildHookHandlers(stores *store.Stores, providerReg *providers.Registry) ma DefaultModel: "haiku", } + // ScriptHandler: bounded by cfg.Hooks caps; zero values fall back to + // handler defaults (10 / 3 / 500). Safe for concurrent reuse across + // dispatcher + hooks.test runner (each Execute allocates its own runtime). + scriptHandler := hookhandlers.NewScriptHandler( + hooksCfg.ScriptConcurrency, + hooksCfg.ScriptPerTenantConcurrency, + hooksCfg.ScriptCacheSize, + ) + return map[hooks.HandlerType]hooks.Handler{ hooks.HandlerCommand: &hookhandlers.CommandHandler{Edition: edition.Current()}, hooks.HandlerHTTP: &hookhandlers.HTTPHandler{ @@ -47,5 +57,6 @@ func buildHookHandlers(stores *store.Stores, providerReg *providers.Registry) ma Client: security.NewSafeClient(10 * time.Second), }, hooks.HandlerPrompt: promptHandler, + hooks.HandlerScript: scriptHandler, } } diff --git a/cmd/gateway_managed.go b/cmd/gateway_managed.go index d73f00cf..66b72e5d 100644 --- a/cmd/gateway_managed.go +++ b/cmd/gateway_managed.go @@ -148,7 +148,7 @@ 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 { - handlers := buildHookHandlers(stores, providerReg) + handlers := buildHookHandlers(stores, providerReg, appCfg.Hooks) stdOpts := hooks.StdDispatcherOpts{ Store: hs, Audit: hooks.NewAuditWriter(hs, ""), diff --git a/internal/config/config.go b/internal/config/config.go index 1bd978dc..42791962 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -55,9 +55,27 @@ type Config struct { Telemetry TelemetryConfig `json:"telemetry"` Tailscale TailscaleConfig `json:"tailscale"` Bindings []AgentBinding `json:"bindings,omitempty"` + Hooks HooksConfig `json:"hooks,omitempty"` mu sync.RWMutex } +// HooksConfig tunes the script-hook runtime caps. All zero-valued fields fall +// back to the handlers package defaults (see handlers.NewScriptHandler). +// +// ScriptConcurrency bounds the total concurrent script executions per process. +// ScriptPerTenantConcurrency bounds a single tenant's share of that pool so a +// runaway tenant cannot starve global slots. ScriptCacheSize caps the LRU of +// compiled goja programs keyed by (hookID, version). +// +// BuiltinDisable names builtin hook IDs (from Phase 04's builtins.yaml) that +// the operator wants force-disabled at startup — per-deployment escape hatch. +type HooksConfig struct { + ScriptConcurrency int `json:"script_concurrency,omitempty"` + ScriptPerTenantConcurrency int `json:"script_per_tenant_concurrency,omitempty"` + ScriptCacheSize int `json:"script_cache_size,omitempty"` + BuiltinDisable []string `json:"builtin_disable,omitempty"` +} + // TailscaleConfig configures the optional Tailscale tsnet listener. // Requires building with -tags tsnet. Auth key from env only (never persisted). type TailscaleConfig struct { diff --git a/internal/hooks/config.go b/internal/hooks/config.go index aceaafa0..187de8e9 100644 --- a/internal/hooks/config.go +++ b/internal/hooks/config.go @@ -3,6 +3,7 @@ package hooks import ( "fmt" + "github.com/dop251/goja" "github.com/google/uuid" "github.com/nextlevelbuilder/goclaw/internal/edition" @@ -121,6 +122,19 @@ func (h *HookConfig) validateHandler(ed edition.Edition) error { if tmpl, _ := h.Config["prompt_template"].(string); tmpl == "" { return fmt.Errorf("hook: prompt handler requires non-empty prompt_template") } + case HandlerScript: + source, _ := h.Config["source"].(string) + if source == "" { + return fmt.Errorf("hook: script handler requires non-empty config.source") + } + if len(source) > MaxScriptSourceBytes { + return fmt.Errorf("hook: script source exceeds %d bytes (got %d)", MaxScriptSourceBytes, len(source)) + } + // Strict=true parallels the handler's compile; surfaces `Line N:M` + // errors that the UI editor can display inline. + if _, err := goja.Compile(h.ID.String(), source, true); err != nil { + return fmt.Errorf("hook: script compile error: %w", err) + } case HandlerCommand, HandlerHTTP: // No extra filter required — matcher/if_expr are optional. default: @@ -129,7 +143,14 @@ func (h *HookConfig) validateHandler(ed edition.Edition) error { return nil } -// validateTimeout bounds the per-hook timeout within [0, MaxTimeoutMS]. +// MaxScriptSourceBytes mirrors handlers.MaxScriptSourceBytes (32 KiB). Declared +// here to avoid importing the handlers package (which imports goja) just for +// this constant — the size cap is part of the config contract and should be +// enforceable without the runtime dep elsewhere. +const MaxScriptSourceBytes = 32 * 1024 + +// validateTimeout bounds the per-hook timeout within [0, MaxTimeoutMS] and +// rejects on_timeout values reserved for future phases. // Zero is normalized to DefaultTimeoutMS in applyDefaults. func (h *HookConfig) validateTimeout() error { if h.TimeoutMS < 0 { @@ -138,6 +159,13 @@ func (h *HookConfig) validateTimeout() error { if h.TimeoutMS > MaxTimeoutMS { return fmt.Errorf("hook: timeout_ms %d exceeds max %d", h.TimeoutMS, MaxTimeoutMS) } + // ask/defer are reserved for Wave 3 human-approval + external-arbitration + // flows. Until the dispatcher wires a `request_user_input` path, accepting + // these as timeout-fallback would let a misconfigured hook hang blocking + // events indefinitely. + if h.OnTimeout == DecisionAsk || h.OnTimeout == DecisionDefer { + return fmt.Errorf("hook: on_timeout=%q not supported in Wave 1 (reserved for future)", h.OnTimeout) + } return nil } diff --git a/internal/hooks/config_test.go b/internal/hooks/config_test.go index fe3bbbe8..d364efc1 100644 --- a/internal/hooks/config_test.go +++ b/internal/hooks/config_test.go @@ -254,3 +254,66 @@ func TestValidate_CapsTimeoutToMax(t *testing.T) { t.Fatal("expected error for timeout > MaxTimeoutMS") } } + +// ─── Script handler validation (Phase 03) ──────────────────────────────────── + +func baseValidScriptHook() hooks.HookConfig { + h := baseValidCommandHook() + h.HandlerType = hooks.HandlerScript + h.Config = map[string]any{"source": `function handle(e){return {decision:"allow"}}`} + return h +} + +func TestValidate_AcceptsValidScriptHook(t *testing.T) { + h := baseValidScriptHook() + if err := h.Validate(edition.Standard); err != nil { + t.Fatalf("expected nil error; got %v", err) + } +} + +func TestValidate_RejectsEmptyScriptSource(t *testing.T) { + h := baseValidScriptHook() + h.Config = map[string]any{"source": ""} + err := h.Validate(edition.Standard) + if err == nil || !strings.Contains(err.Error(), "non-empty") { + t.Fatalf("expected non-empty source error; got %v", err) + } +} + +func TestValidate_RejectsOversizedScriptSource(t *testing.T) { + h := baseValidScriptHook() + big := strings.Repeat("// pad\n", 6000) // >32 KiB + h.Config = map[string]any{"source": big} + err := h.Validate(edition.Standard) + if err == nil || !strings.Contains(err.Error(), "exceeds") { + t.Fatalf("expected exceeds-bytes error; got %v", err) + } +} + +func TestValidate_RejectsScriptCompileError(t *testing.T) { + h := baseValidScriptHook() + // Missing closing brace → parser error with line:col. + h.Config = map[string]any{"source": `function handle(e) { return {`} + err := h.Validate(edition.Standard) + if err == nil || !strings.Contains(err.Error(), "compile error") { + t.Fatalf("expected compile error; got %v", err) + } +} + +func TestValidate_RejectsOnTimeoutAsk(t *testing.T) { + h := baseValidScriptHook() + h.OnTimeout = hooks.DecisionAsk + err := h.Validate(edition.Standard) + if err == nil || !strings.Contains(err.Error(), "on_timeout") { + t.Fatalf("expected on_timeout rejection; got %v", err) + } +} + +func TestValidate_RejectsOnTimeoutDefer(t *testing.T) { + h := baseValidScriptHook() + h.OnTimeout = hooks.DecisionDefer + err := h.Validate(edition.Standard) + if err == nil || !strings.Contains(err.Error(), "on_timeout") { + t.Fatalf("expected on_timeout rejection; got %v", err) + } +} diff --git a/internal/hooks/delegate_bridge.go b/internal/hooks/delegate_bridge.go index 16ae5e9d..0a2c43fe 100644 --- a/internal/hooks/delegate_bridge.go +++ b/internal/hooks/delegate_bridge.go @@ -45,6 +45,8 @@ func SubscribeDelegateEvents(bus eventbus.DomainEventBus, d Dispatcher) { "err", err, ) } + // SubagentStop is non-blocking; Updated* from FireResult is ignored + // (delegate bridge has no mutation path in Wave 1). return nil } diff --git a/internal/hooks/delegate_bridge_test.go b/internal/hooks/delegate_bridge_test.go index bcce6e47..53347a14 100644 --- a/internal/hooks/delegate_bridge_test.go +++ b/internal/hooks/delegate_bridge_test.go @@ -40,9 +40,9 @@ type fakeCapturingDispatcher struct { events []Event } -func (f *fakeCapturingDispatcher) Fire(_ context.Context, ev Event) (Decision, error) { +func (f *fakeCapturingDispatcher) Fire(_ context.Context, ev Event) (FireResult, error) { f.events = append(f.events, ev) - return DecisionAllow, nil + return FireResult{Decision: DecisionAllow}, nil } // --- tests --- diff --git a/internal/hooks/dispatcher.go b/internal/hooks/dispatcher.go index 1e6bd8a4..6e96531c 100644 --- a/internal/hooks/dispatcher.go +++ b/internal/hooks/dispatcher.go @@ -20,9 +20,11 @@ type Handler interface { } // Dispatcher is the pipeline integration surface. Stages call Fire and act on -// the Decision (allow → continue; block → abort with i18n error). +// FireResult.Decision (allow → continue; block → abort with i18n error). +// FireResult.Updated* carry mutations authored by builtin-source hooks — +// callers apply them to their own tc.Arguments / state.Input when non-nil. type Dispatcher interface { - Fire(ctx context.Context, ev Event) (Decision, error) + Fire(ctx context.Context, ev Event) (FireResult, error) } // MaxLoopDepth caps nested hook invocation (M5). Depth increments when a hook @@ -130,8 +132,8 @@ func NewNoopDispatcher() Dispatcher { type noopDispatcher struct{} -func (noopDispatcher) Fire(context.Context, Event) (Decision, error) { - return DecisionAllow, nil +func (noopDispatcher) Fire(context.Context, Event) (FireResult, error) { + return FireResult{Decision: DecisionAllow}, nil } // ── stdDispatcher ──────────────────────────────────────────────────────────── @@ -146,13 +148,13 @@ type stdDispatcher struct { cb *circuitBreaker } -func (d *stdDispatcher) Fire(ctx context.Context, ev Event) (Decision, error) { +func (d *stdDispatcher) Fire(ctx context.Context, ev Event) (FireResult, error) { if depthFromCtx(ctx) > MaxLoopDepth { slog.Warn("security.hook.loop_depth_exceeded", "event_id", ev.EventID, "hook_event", ev.HookEvent, ) - return DecisionError, ErrLoopDepthExceeded + return FireResult{Decision: DecisionError}, ErrLoopDepthExceeded } hooks, err := d.store.ResolveForEvent(ctx, ev) @@ -160,68 +162,174 @@ func (d *stdDispatcher) Fire(ctx context.Context, ev Event) (Decision, error) { // Fail-closed: a DB blip must not let a pre-tool gate open silently. slog.Warn("security.hook.resolve_error", "err", err, "event_id", ev.EventID) if ev.HookEvent.IsBlocking() { - return DecisionBlock, err + return FireResult{Decision: DecisionBlock}, err } - return DecisionAllow, err + return FireResult{Decision: DecisionAllow}, err } if len(hooks) == 0 { - return DecisionAllow, nil + return FireResult{Decision: DecisionAllow}, nil } if ev.HookEvent.IsBlocking() { return d.runSync(ctx, ev, hooks) } d.runAsync(ctx, ev, hooks) - return DecisionAllow, nil + return FireResult{Decision: DecisionAllow}, nil } // runSync executes the blocking chain with a wall-time budget and per-hook // timeouts. First block wins; any fail-closed condition aborts to Block. -func (d *stdDispatcher) runSync(ctx context.Context, ev Event, chain []HookConfig) (Decision, error) { +// +// Mutation propagation (Phase 03): the dispatcher keeps a local evMut copy +// with its own ToolInput map so a builtin-source hook's updatedInput becomes +// visible to downstream hooks in the chain (and to the caller via FireResult) +// without leaking mutations back into the caller's original event on error +// paths. Non-builtin scripts returning updatedInput get their mutation +// dropped + logged — source tier enforced at the dispatcher. +func (d *stdDispatcher) runSync(ctx context.Context, ev Event, chain []HookConfig) (FireResult, error) { chainCtx, cancel := context.WithTimeout(ctx, d.chainBudget) defer cancel() + evMut := ev + if ev.ToolInput != nil { + evMut.ToolInput = cloneMap(ev.ToolInput) + } + mutated := false + for _, cfg := range chain { if !cfg.Enabled { continue } if d.cb.isTripped(cfg.ID, d.now()) { - d.writeExec(ctx, cfg, ev, DecisionBlock, 0, "circuit breaker open") - return DecisionBlock, nil + d.writeExec(ctx, cfg, evMut, DecisionBlock, 0, "circuit breaker open") + return FireResult{Decision: DecisionBlock}, nil } - if !d.prefilter(cfg, ev) { + if !d.prefilter(cfg, evMut) { continue } - dec, execErr, duration := d.runOne(chainCtx, cfg, ev) + + // Attach fresh ScriptResult carrier per-hook so the script handler can + // report its non-decision outputs (reason/updatedInput/stdout). Other + // handler types ignore the ctx value — zero-cost. + scriptRes := &ScriptResult{} + hctx := WithScriptResult(chainCtx, scriptRes) + + dec, execErr, duration := d.runOne(hctx, cfg, evMut) errMsg := "" if execErr != nil { errMsg = execErr.Error() } - d.writeExec(ctx, cfg, ev, dec, duration, errMsg) + d.writeExec(ctx, cfg, evMut, dec, duration, errMsg) + + // Apply mutation to the local copy only when the hook is builtin-source. + // Non-builtin scripts get their updatedInput stripped + warned. + if cfg.HandlerType == HandlerScript && dec == DecisionAllow && scriptRes.UpdatedInput != nil { + if cfg.Source == SourceBuiltin { + applyBuiltinMutation(&evMut, scriptRes.UpdatedInput, builtinAllowlistFor(cfg.ID)) + mutated = true + } else { + slog.Warn("hooks.script_mutation_denied", + "hook_id", cfg.ID, + "source", cfg.Source, + "field_count", len(scriptRes.UpdatedInput), + ) + } + } switch dec { case DecisionBlock: d.cb.record(cfg.ID, d.now(), d.store) - return DecisionBlock, nil + return FireResult{Decision: DecisionBlock}, nil case DecisionTimeout: d.cb.record(cfg.ID, d.now(), d.store) if cfg.OnTimeout == DecisionBlock { - return DecisionBlock, nil + return FireResult{Decision: DecisionBlock}, nil } // OnTimeout=allow: degrade gracefully but keep scanning. case DecisionError: // Unexpected error in a blocking chain → fail-closed. - return DecisionBlock, nil + return FireResult{Decision: DecisionBlock}, nil } if chainCtx.Err() != nil { // Chain wall-time budget exhausted (H3): fail-closed. - return DecisionBlock, nil + return FireResult{Decision: DecisionBlock}, nil } } - return DecisionAllow, nil + + result := FireResult{Decision: DecisionAllow} + if mutated { + if evMut.ToolInput != nil { + result.UpdatedToolInput = evMut.ToolInput + } + if evMut.RawInput != ev.RawInput { + s := evMut.RawInput + result.UpdatedRawInput = &s + } + } + return result, nil } +// cloneMap returns a shallow copy of m. Used so runSync's mutations on the +// local evMut don't leak back to the caller's event when a downstream hook +// blocks or the chain aborts. +func cloneMap(m map[string]any) map[string]any { + out := make(map[string]any, len(m)) + for k, v := range m { + out[k] = v + } + return out +} + +// applyBuiltinMutation applies the builtin hook's updatedInput fields into ev, +// gated by the per-builtin allowlist. The allowlist is loaded in Phase 04 from +// builtins.yaml; Phase 03 uses a permissive default (rawInput + toolInput). +func applyBuiltinMutation(ev *Event, updated map[string]any, allowlist []string) { + if updated == nil { + return + } + allowSet := make(map[string]struct{}, len(allowlist)) + for _, f := range allowlist { + allowSet[f] = struct{}{} + } + if _, ok := allowSet["rawInput"]; ok { + if s, ok2 := updated["rawInput"].(string); ok2 { + ev.RawInput = s + } + } + if m, ok := updated["toolInput"].(map[string]any); ok { + if _, wildcard := allowSet["toolInput"]; wildcard { + if ev.ToolInput == nil { + ev.ToolInput = map[string]any{} + } + for k, v := range m { + ev.ToolInput[k] = v + } + } else { + for k, v := range m { + if _, ok := allowSet["toolInput."+k]; ok { + if ev.ToolInput == nil { + ev.ToolInput = map[string]any{} + } + ev.ToolInput[k] = v + } + } + } + } +} + +// 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 { + return []string{"rawInput", "toolInput"} +} + +// SourceBuiltin is the `source` value marking a hook seeded by the builtin +// infrastructure (Phase 04 loader). Only builtin-source hooks are allowed to +// mutate event input through the script handler's updatedInput field. +const SourceBuiltin = "builtin" + // runAsync fires non-blocking hooks concurrently. Phase 1 uses a simple // goroutine-per-hook; Phase 2 will route through the eventbus worker pool. func (d *stdDispatcher) runAsync(ctx context.Context, ev Event, chain []HookConfig) { diff --git a/internal/hooks/dispatcher_test.go b/internal/hooks/dispatcher_test.go index 126aa4e0..10b4914f 100644 --- a/internal/hooks/dispatcher_test.go +++ b/internal/hooks/dispatcher_test.go @@ -122,15 +122,15 @@ func TestDispatcher_NoHooks_ReturnsAllow(t *testing.T) { Store: fs, Audit: hooks.NewAuditWriter(fs, ""), }) - dec, err := d.Fire(context.Background(), hooks.Event{ + r, err := d.Fire(context.Background(), hooks.Event{ EventID: "e1", HookEvent: hooks.EventPreToolUse, }) if err != nil { t.Fatalf("Fire: %v", err) } - if dec != hooks.DecisionAllow { - t.Errorf("decision=%q, want allow", dec) + if r.Decision != hooks.DecisionAllow { + t.Errorf("decision=%q, want allow", r.Decision) } } @@ -157,15 +157,15 @@ func TestDispatcher_SyncChain_FirstBlockWins(t *testing.T) { }, }, }) - dec, err := d.Fire(context.Background(), hooks.Event{ + r, err := d.Fire(context.Background(), hooks.Event{ EventID: "e-block", HookEvent: hooks.EventPreToolUse, }) if err != nil { t.Fatalf("Fire: %v", err) } - if dec != hooks.DecisionBlock { - t.Errorf("decision=%q, want block", dec) + if r.Decision != hooks.DecisionBlock { + t.Errorf("decision=%q, want block", r.Decision) } if atomic.LoadInt32(&lateHandler.calls) != 0 { t.Errorf("post-block handler ran %d times, want 0", lateHandler.calls) @@ -198,15 +198,15 @@ func TestDispatcher_PerHookTimeout_FailsClosed(t *testing.T) { PerHookTimeout: 20 * time.Millisecond, ChainBudget: 5 * time.Second, }) - dec, err := d.Fire(context.Background(), hooks.Event{ + r, err := d.Fire(context.Background(), hooks.Event{ EventID: "e-timeout", HookEvent: hooks.EventPreToolUse, }) if err != nil { t.Fatalf("Fire: %v", err) } - if dec != hooks.DecisionBlock { - t.Errorf("decision=%q, want block (fail-closed on timeout)", dec) + if r.Decision != hooks.DecisionBlock { + t.Errorf("decision=%q, want block (fail-closed on timeout)", r.Decision) } // Exactly one execution row recorded with timeout decision. execs := fs.snapshotExecs() @@ -281,15 +281,15 @@ func TestDispatcher_NonBlockingEvent_AsyncNoDecision(t *testing.T) { }), }, }) - dec, err := d.Fire(context.Background(), hooks.Event{ + r, err := d.Fire(context.Background(), hooks.Event{ EventID: "e-async", HookEvent: hooks.EventPostToolUse, }) if err != nil { t.Fatalf("Fire: %v", err) } - if dec != hooks.DecisionAllow { - t.Errorf("decision=%q, want allow (non-blocking path never blocks)", dec) + if r.Decision != hooks.DecisionAllow { + t.Errorf("decision=%q, want allow (non-blocking path never blocks)", r.Decision) } select { case <-done: @@ -314,15 +314,15 @@ func TestDispatcher_AllAllow_ReturnsAllow(t *testing.T) { Audit: hooks.NewAuditWriter(fs, ""), Handlers: map[hooks.HandlerType]hooks.Handler{hooks.HandlerHTTP: &fakeHandler{decision: hooks.DecisionAllow}}, }) - dec, err := d.Fire(context.Background(), hooks.Event{ + r, err := d.Fire(context.Background(), hooks.Event{ EventID: "e-all-allow", HookEvent: hooks.EventPreToolUse, }) if err != nil { t.Fatalf("Fire: %v", err) } - if dec != hooks.DecisionAllow { - t.Errorf("decision=%q, want allow", dec) + if r.Decision != hooks.DecisionAllow { + t.Errorf("decision=%q, want allow", r.Decision) } if got := len(fs.snapshotExecs()); got != 2 { t.Errorf("exec rows=%d, want 2 (one per hook)", got) @@ -335,15 +335,15 @@ func TestDispatcher_ResolveError_FailsClosed(t *testing.T) { Store: fs, Audit: hooks.NewAuditWriter(fs, ""), }) - dec, err := d.Fire(context.Background(), hooks.Event{ + r, err := d.Fire(context.Background(), hooks.Event{ EventID: "e-resolve-err", HookEvent: hooks.EventPreToolUse, }) if err == nil { t.Fatal("Fire err=nil, want non-nil on resolve failure") } - if dec != hooks.DecisionBlock { - t.Errorf("decision=%q, want block (fail-closed)", dec) + if r.Decision != hooks.DecisionBlock { + t.Errorf("decision=%q, want block (fail-closed)", r.Decision) } } @@ -356,12 +356,12 @@ func TestDispatcher_MissingHandler_BlocksBlockingEvent(t *testing.T) { Store: fs, Audit: hooks.NewAuditWriter(fs, ""), }) - dec, _ := d.Fire(context.Background(), hooks.Event{ + r, _ := d.Fire(context.Background(), hooks.Event{ EventID: "e-no-handler", HookEvent: hooks.EventPreToolUse, }) - if dec != hooks.DecisionBlock { - t.Errorf("decision=%q, want block (missing handler → fail-closed)", dec) + if r.Decision != hooks.DecisionBlock { + t.Errorf("decision=%q, want block (missing handler → fail-closed)", r.Decision) } } @@ -388,14 +388,93 @@ func TestDispatcher_DedupKey_IncludesEventID(t *testing.T) { func TestNoopDispatcher_AlwaysAllows(t *testing.T) { d := hooks.NewNoopDispatcher() - dec, err := d.Fire(context.Background(), hooks.Event{ + r, err := d.Fire(context.Background(), hooks.Event{ EventID: "e", HookEvent: hooks.EventPreToolUse, }) if err != nil { t.Fatalf("Fire: %v", err) } - if dec != hooks.DecisionAllow { - t.Errorf("decision=%q, want allow", dec) + if r.Decision != hooks.DecisionAllow { + t.Errorf("decision=%q, want allow", r.Decision) + } + if r.UpdatedToolInput != nil || r.UpdatedRawInput != nil { + t.Errorf("noop must return zero Updated* (got ti=%v ri=%v)", r.UpdatedToolInput, r.UpdatedRawInput) + } +} + +// mutatingHandler writes an UpdatedInput to the ctx-carried ScriptResult so +// the dispatcher's source-tier gate can be exercised without invoking the +// real goja runtime. +type mutatingHandler struct { + updated map[string]any +} + +func (h *mutatingHandler) Execute(ctx context.Context, _ hooks.HookConfig, _ hooks.Event) (hooks.Decision, error) { + if r := hooks.ScriptResultFrom(ctx); r != nil { + r.UpdatedInput = h.updated + } + return hooks.DecisionAllow, nil +} + +// TestDispatcher_ScriptMutation_BuiltinSourceApplies verifies the source-tier +// capability: a script hook with source="builtin" has its UpdatedInput applied +// to FireResult.Updated*; a script hook with source="ui" does NOT. +func TestDispatcher_ScriptMutation_BuiltinSourceApplies(t *testing.T) { + cfg := newBaseHook(hooks.HandlerScript, hooks.EventUserPromptSubmit) + cfg.Source = hooks.SourceBuiltin + fs := &fakeStore{hooks: []hooks.HookConfig{cfg}} + + h := &mutatingHandler{updated: map[string]any{"rawInput": "REDACTED"}} + d := hooks.NewStdDispatcher(hooks.StdDispatcherOpts{ + Store: fs, + Audit: hooks.NewAuditWriter(fs, ""), + Handlers: map[hooks.HandlerType]hooks.Handler{hooks.HandlerScript: h}, + }) + + r, err := d.Fire(context.Background(), hooks.Event{ + EventID: "e-builtin", + HookEvent: hooks.EventUserPromptSubmit, + RawInput: "ssn is 123-45-6789", + }) + if err != nil { + t.Fatalf("Fire: %v", err) + } + if r.Decision != hooks.DecisionAllow { + t.Fatalf("decision=%q, want allow", r.Decision) + } + if r.UpdatedRawInput == nil { + t.Fatal("UpdatedRawInput nil; expected redacted rawInput from builtin hook") + } + if *r.UpdatedRawInput != "REDACTED" { + t.Errorf("UpdatedRawInput=%q, want REDACTED", *r.UpdatedRawInput) + } +} + +func TestDispatcher_ScriptMutation_UISourceDenied(t *testing.T) { + cfg := newBaseHook(hooks.HandlerScript, hooks.EventUserPromptSubmit) + cfg.Source = "ui" + fs := &fakeStore{hooks: []hooks.HookConfig{cfg}} + + h := &mutatingHandler{updated: map[string]any{"rawInput": "pwn"}} + d := hooks.NewStdDispatcher(hooks.StdDispatcherOpts{ + Store: fs, + Audit: hooks.NewAuditWriter(fs, ""), + Handlers: map[hooks.HandlerType]hooks.Handler{hooks.HandlerScript: h}, + }) + + r, err := d.Fire(context.Background(), hooks.Event{ + EventID: "e-ui", + HookEvent: hooks.EventUserPromptSubmit, + RawInput: "original", + }) + if err != nil { + t.Fatalf("Fire: %v", err) + } + if r.Decision != hooks.DecisionAllow { + t.Fatalf("decision=%q, want allow", r.Decision) + } + if r.UpdatedRawInput != nil { + t.Fatalf("non-builtin mutation leaked: UpdatedRawInput=%v", *r.UpdatedRawInput) } } diff --git a/internal/hooks/edition_gate.go b/internal/hooks/edition_gate.go index 90ad81c7..12ed35b5 100644 --- a/internal/hooks/edition_gate.go +++ b/internal/hooks/edition_gate.go @@ -15,7 +15,9 @@ type HookEditionPolicy struct{} // reason is a stable string suitable for audit rows and i18n key lookup. func (HookEditionPolicy) Allow(ht HandlerType, _ Scope, ed edition.Edition) (bool, string) { switch ht { - case HandlerHTTP, HandlerPrompt: + case HandlerHTTP, HandlerPrompt, HandlerScript: + // Script hooks run in a sandboxed goja runtime — no shell escape surface. + // Allowed on every edition for tenant admin + operator authors alike. return true, "" case HandlerCommand: if ed.Name == edition.Lite.Name { diff --git a/internal/hooks/types.go b/internal/hooks/types.go index 4d347996..6fe51da5 100644 --- a/internal/hooks/types.go +++ b/internal/hooks/types.go @@ -134,6 +134,25 @@ func (d Decision) IsBlock() bool { return d == DecisionBlock } +// FireResult is the return value of Dispatcher.Fire. Callers read Decision to +// branch on allow/block and apply Updated* when a builtin hook mutated input. +// +// UpdatedToolInput is non-nil only when at least one builtin-source hook in +// the chain returned updatedInput AND the dispatcher applied allow-listed +// fields. Callers overwrite their own tc.Arguments / state.Input with it. +// +// UpdatedRawInput points to a string only when a builtin hook mutated +// rawInput. Callers replace state.Input.Message with the dereferenced value. +// +// For non-builtin scripts returning updatedInput the dispatcher strips the +// mutation + logs a WARN; Updated* stay nil (defense-in-depth against a +// tenant-authored script escalating its capability tier). +type FireResult struct { + Decision Decision + UpdatedToolInput map[string]any + UpdatedRawInput *string +} + // ─── Config & execution structs ────────────────────────────────────────────── // HookConfig mirrors the agent_hooks DB row. All pointer fields correspond to diff --git a/internal/pipeline/context_stage.go b/internal/pipeline/context_stage.go index 5a49b2b0..3377fffd 100644 --- a/internal/pipeline/context_stage.go +++ b/internal/pipeline/context_stage.go @@ -48,16 +48,20 @@ func (s *ContextStage) Execute(ctx context.Context, state *RunState) error { }) } - // Hook: sync UserPromptSubmit — blocking; abort if blocked. - if dec, _ := s.deps.FireHook(ctx, hooks.Event{ + // Hook: sync UserPromptSubmit — blocking; abort if blocked. A builtin- + // source hook may rewrite RawInput (e.g. PII redactor); apply the update + // before any downstream stage reads state.Input.Message. + if r, _ := s.deps.FireHook(ctx, hooks.Event{ EventID: uuid.NewString(), SessionID: state.Input.SessionKey, TenantID: store.TenantIDFromContext(ctx), AgentID: store.AgentIDFromContext(ctx), RawInput: state.Input.Message, HookEvent: hooks.EventUserPromptSubmit, - }); dec == hooks.DecisionBlock { + }); r.Decision == hooks.DecisionBlock { return fmt.Errorf("hook blocked user_prompt_submit") + } else if r.UpdatedRawInput != nil { + state.Input.Message = *r.UpdatedRawInput } // 0. Inject context values (agent/tenant/user/workspace scoping, input guard, truncation). diff --git a/internal/pipeline/deps.go b/internal/pipeline/deps.go index 4a4103a9..e941d4a8 100644 --- a/internal/pipeline/deps.go +++ b/internal/pipeline/deps.go @@ -110,11 +110,13 @@ type PipelineDeps struct { MaybeSummarize func(ctx context.Context, sessionKey string) } -// FireHook is nil-safe. Returns DecisionAllow when no dispatcher is wired. -// Callers on blocking events should abort the stage on DecisionBlock. -func (d *PipelineDeps) FireHook(ctx context.Context, ev hooks.Event) (hooks.Decision, error) { +// FireHook is nil-safe. Returns FireResult{Decision: DecisionAllow} when no +// dispatcher is wired. Callers on blocking events should abort the stage on +// result.Decision == DecisionBlock and apply result.UpdatedToolInput / +// UpdatedRawInput to their local state when non-nil (builtin-hook mutations). +func (d *PipelineDeps) FireHook(ctx context.Context, ev hooks.Event) (hooks.FireResult, error) { if d == nil || d.Hooks == nil { - return hooks.DecisionAllow, nil + return hooks.FireResult{Decision: hooks.DecisionAllow}, nil } return d.Hooks.Fire(ctx, ev) } diff --git a/internal/pipeline/tool_stage.go b/internal/pipeline/tool_stage.go index 39d3eaef..8779822b 100644 --- a/internal/pipeline/tool_stage.go +++ b/internal/pipeline/tool_stage.go @@ -48,8 +48,10 @@ func (s *ToolStage) Execute(ctx context.Context, state *RunState) error { // Sequential fallback: ExecuteToolCall handles both I/O and state mutation. for _, tc := range toolCalls { - // Hook: sync PreToolUse — block if hook denies. - if dec, _ := s.deps.FireHook(ctx, hooks.Event{ + // Hook: sync PreToolUse — block if hook denies. Builtin-source hooks may + // rewrite tc.Arguments via UpdatedToolInput (e.g. path-sanitizer); apply + // before ExecuteToolCall so the rewrite is authoritative. + if r, _ := s.deps.FireHook(ctx, hooks.Event{ EventID: uuid.NewString(), SessionID: state.Input.SessionKey, TenantID: store.TenantIDFromContext(ctx), @@ -57,7 +59,7 @@ func (s *ToolStage) Execute(ctx context.Context, state *RunState) error { ToolName: tc.Name, ToolInput: tc.Arguments, HookEvent: hooks.EventPreToolUse, - }); dec == hooks.DecisionBlock { + }); r.Decision == hooks.DecisionBlock { // Inject synthetic blocked tool message and skip actual execution. state.Messages.AppendPending(providers.Message{ Role: "tool", @@ -66,6 +68,8 @@ func (s *ToolStage) Execute(ctx context.Context, state *RunState) error { }) state.Tool.TotalToolCalls++ continue + } else if r.UpdatedToolInput != nil { + tc.Arguments = r.UpdatedToolInput } msgs, err := s.deps.ExecuteToolCall(ctx, state, tc) diff --git a/internal/store/pg/hooks.go b/internal/store/pg/hooks.go index f775439a..18712687 100644 --- a/internal/store/pg/hooks.go +++ b/internal/store/pg/hooks.go @@ -43,7 +43,14 @@ func NewPGHookStore(db *sql.DB) *PGHookStore { // ─── Create ───────────────────────────────────────────────────────────────── func (s *PGHookStore) Create(ctx context.Context, cfg hooks.HookConfig) (uuid.UUID, error) { - id := uuid.Must(uuid.NewV7()) + // Honor a caller-provided fixed ID when non-nil (H9 fix) — the Phase 04 + // builtin seeder needs idempotent UPSERTs keyed by UUIDv5, and tests need + // deterministic IDs. Fall back to UUIDv7 only when the caller did not + // supply one. + id := cfg.ID + if id == uuid.Nil { + id = uuid.Must(uuid.NewV7()) + } now := time.Now().UTC() cfgJSON, err := json.Marshal(cfg.Config) diff --git a/internal/store/pg/hooks_test.go b/internal/store/pg/hooks_test.go index e7fca2f9..b7694e93 100644 --- a/internal/store/pg/hooks_test.go +++ b/internal/store/pg/hooks_test.go @@ -381,3 +381,38 @@ func TestPGHookStore_CacheInvalidatedOnWrite(t *testing.T) { t.Errorf("expected more hooks after create: before=%d after=%d", beforeCount, len(after)) } } + +// H9 (Phase 03): Create must honor a caller-supplied cfg.ID so the builtin +// seeder's idempotent UUIDv5 keys survive restarts and tests can use +// deterministic IDs. Verified on both Create paths (fixed + auto). +func TestPGHookStore_CreateHonorsFixedID(t *testing.T) { + db := hooksTestDB(t) + tenantID, _ := seedTenantAndAgent(t, db) + s := NewPGHookStore(db) + ctx := tenantScopedCtx(tenantID) + + fixed := uuid.MustParse("aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee") + cfg := minimalHook(tenantID, hooks.EventUserPromptSubmit) + cfg.ID = fixed + + got, err := s.Create(ctx, cfg) + if err != nil { + t.Fatalf("Create fixed id: %v", err) + } + t.Cleanup(func() { s.Delete(masterCtx(), got) }) + if got != fixed { + t.Fatalf("returned id=%s, want %s (H9: caller id must be honored)", got, fixed) + } + + // Nil cfg.ID still auto-generates a UUIDv7. + cfg2 := minimalHook(tenantID, hooks.EventPreToolUse) + cfg2.ID = uuid.Nil + auto, err := s.Create(ctx, cfg2) + if err != nil { + t.Fatalf("Create auto id: %v", err) + } + t.Cleanup(func() { s.Delete(masterCtx(), auto) }) + if auto == uuid.Nil { + t.Fatal("Create returned nil id for cfg.ID=uuid.Nil path") + } +} diff --git a/internal/store/sqlitestore/hooks.go b/internal/store/sqlitestore/hooks.go index c452325f..ef0540f3 100644 --- a/internal/store/sqlitestore/hooks.go +++ b/internal/store/sqlitestore/hooks.go @@ -45,7 +45,13 @@ func NewSQLiteHookStore(db *sql.DB) *SqliteHookStore { // ─── Create ───────────────────────────────────────────────────────────────── func (s *SqliteHookStore) Create(ctx context.Context, cfg hooks.HookConfig) (uuid.UUID, error) { - id := uuid.Must(uuid.NewV7()) + // Honor caller-provided fixed ID (H9) — Phase 04 builtin seeder uses + // UUIDv5 for idempotent UPSERT; tests need deterministic IDs. Only + // auto-generate when the caller left ID unset. + id := cfg.ID + if id == uuid.Nil { + id = uuid.Must(uuid.NewV7()) + } now := time.Now().UTC().Format(time.RFC3339Nano) cfgJSON, err := json.Marshal(cfg.Config) diff --git a/internal/store/sqlitestore/hooks_test.go b/internal/store/sqlitestore/hooks_test.go index 14b7ade2..1e8c27e2 100644 --- a/internal/store/sqlitestore/hooks_test.go +++ b/internal/store/sqlitestore/hooks_test.go @@ -212,25 +212,38 @@ func TestSQLiteHookStore_TenantIsolation(t *testing.T) { // ─── Partial unique indexes ─────────────────────────────────────────────────── -func TestSQLiteHookStore_UniqueIndexPreventsScopedDuplicates(t *testing.T) { +// H9 (Phase 03): Create honors caller-supplied cfg.ID so the builtin seeder's +// idempotent UUIDv5 keys survive restarts and tests get deterministic IDs. +func TestSQLiteHookStore_CreateHonorsFixedID(t *testing.T) { db := newHookTestDB(t) tenantID, _ := seedHookTenantAgent(t, db) s := NewSQLiteHookStore(db) ctx := sqliteTenantCtx(tenantID) + fixed := uuid.MustParse("11111111-2222-3333-4444-555555555555") cfg := sqliteMinimalHook(tenantID, hooks.EventUserPromptSubmit) + cfg.ID = fixed cfg.Scope = hooks.ScopeTenant - id1, err := s.Create(ctx, cfg) + got, err := s.Create(ctx, cfg) if err != nil { - t.Fatalf("first Create: %v", err) + t.Fatalf("Create: %v", err) + } + t.Cleanup(func() { s.Delete(sqliteMasterCtx(), got) }) + if got != fixed { + t.Fatalf("Create returned id=%s, want %s (H9: caller id must be honored)", got, fixed) } - t.Cleanup(func() { s.Delete(sqliteMasterCtx(), id1) }) - // Same (tenant_id, event, handler_type) with ScopeTenant → should fail. - _, err = s.Create(ctx, cfg) - if err == nil { - t.Error("expected unique constraint error on duplicate scoped hook, got nil") + // A nil cfg.ID still auto-generates. + cfg2 := sqliteMinimalHook(tenantID, hooks.EventPreToolUse) + cfg2.ID = uuid.Nil + auto, err := s.Create(ctx, cfg2) + if err != nil { + t.Fatalf("Create auto: %v", err) + } + t.Cleanup(func() { s.Delete(sqliteMasterCtx(), auto) }) + if auto == uuid.Nil { + t.Fatal("Create returned nil id for cfg.ID=uuid.Nil path") } } diff --git a/internal/store/sqlitestore/schema.go b/internal/store/sqlitestore/schema.go index 9e185800..323434f2 100644 --- a/internal/store/sqlitestore/schema.go +++ b/internal/store/sqlitestore/schema.go @@ -16,7 +16,7 @@ var schemaSQL string // SchemaVersion is the current SQLite schema version. // Bump this when adding new migration steps below. -const SchemaVersion = 20 +const SchemaVersion = 21 // migrations maps version → SQL to apply when upgrading FROM that version. // schema.sql always represents the LATEST full schema (for fresh DBs). @@ -460,6 +460,14 @@ WHERE context_pruning IS NOT NULL // Creates agent_hooks, hook_executions, tenant_hook_budget tables. // All idempotent (CREATE TABLE IF NOT EXISTS / CREATE INDEX IF NOT EXISTS). 19: addAgentHooksTables, + + // Version 20 → 21: relax agent_hooks CHECK constraints for script handler + // + builtin source. The inline patch is a no-op (`SELECT 1`) because SQLite + // can't ALTER a CHECK constraint — the table must be rebuilt, which + // requires `PRAGMA foreign_keys = OFF` outside any transaction. The actual + // rebuild runs in rebuildAgentHooksV21 triggered after the v20→21 migration + // tx commits (parallel to backfillV16 at EnsureSchema:677). + 20: `SELECT 1;`, } // addAgentHooksTables is the SQLite incremental migration for schema v19 → v20. @@ -679,6 +687,16 @@ func EnsureSchema(db *sql.DB) error { return fmt.Errorf("v16 backfill: %w", err) } } + // v20 → v21: rebuild agent_hooks table to widen the + // handler_type + source CHECK constraints. SQLite forbids + // ALTER TABLE ... ADD/DROP CONSTRAINT, so the table is recreated + // with the new CHECKs. Runs outside the migration tx because + // PRAGMA foreign_keys is silently ignored inside a transaction. + if v == 20 { + if err := rebuildAgentHooksV21(context.Background(), db); err != nil { + return fmt.Errorf("v21 rebuild: %w", err) + } + } slog.Info("sqlite: applied migration", "version", v+1) } } diff --git a/internal/store/sqlitestore/schema.sql b/internal/store/sqlitestore/schema.sql index b11d28af..9fffd5bf 100644 --- a/internal/store/sqlitestore/schema.sql +++ b/internal/store/sqlitestore/schema.sql @@ -1570,9 +1570,12 @@ CREATE INDEX IF NOT EXISTS idx_vault_links_source WHERE json_extract(metadata, '$.source') IS NOT NULL; -- ============================================================ --- Table: agent_hooks (migration 000052) +-- Table: agent_hooks (migrations 000052 + 000053) -- SQLite translation: JSONB→TEXT, TIMESTAMPTZ→TEXT, UUID→TEXT, -- BYTEA→BLOB, DATE→TEXT (ISO8601), CHECK for enums. +-- v21 (migration 000053): handler_type adds 'script', source adds 'builtin', +-- old (event, handler_type) uniqueness indexes dropped — scripts routinely +-- want many small hooks per event. -- ============================================================ CREATE TABLE IF NOT EXISTS agent_hooks ( @@ -1581,7 +1584,7 @@ CREATE TABLE IF NOT EXISTS agent_hooks ( agent_id TEXT REFERENCES agents(id) ON DELETE CASCADE, scope TEXT NOT NULL CHECK (scope IN ('global', 'tenant', 'agent')), event TEXT NOT NULL, - handler_type TEXT NOT NULL CHECK (handler_type IN ('command', 'http', 'prompt')), + handler_type TEXT NOT NULL CHECK (handler_type IN ('command', 'http', 'prompt', 'script')), config TEXT NOT NULL DEFAULT '{}', matcher TEXT, if_expr TEXT, @@ -1590,25 +1593,13 @@ CREATE TABLE IF NOT EXISTS agent_hooks ( priority INTEGER NOT NULL DEFAULT 0, enabled INTEGER NOT NULL DEFAULT 1, version INTEGER NOT NULL DEFAULT 1, - source TEXT NOT NULL DEFAULT 'ui' CHECK (source IN ('ui', 'api', 'seed')), + source TEXT NOT NULL DEFAULT 'ui' CHECK (source IN ('ui', 'api', 'seed', 'builtin')), metadata TEXT NOT NULL DEFAULT '{}', created_by TEXT, created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), updated_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) ); -CREATE UNIQUE INDEX IF NOT EXISTS uq_hooks_global - ON agent_hooks (event, handler_type) - WHERE scope = 'global'; - -CREATE UNIQUE INDEX IF NOT EXISTS uq_hooks_tenant - ON agent_hooks (tenant_id, event, handler_type) - WHERE scope = 'tenant'; - -CREATE UNIQUE INDEX IF NOT EXISTS uq_hooks_agent - ON agent_hooks (tenant_id, agent_id, event, handler_type) - WHERE scope = 'agent'; - CREATE INDEX IF NOT EXISTS idx_hooks_lookup ON agent_hooks (tenant_id, agent_id, event) WHERE enabled = 1; diff --git a/internal/store/sqlitestore/schema_rebuild_agent_hooks_v21.go b/internal/store/sqlitestore/schema_rebuild_agent_hooks_v21.go new file mode 100644 index 00000000..9e192fc0 --- /dev/null +++ b/internal/store/sqlitestore/schema_rebuild_agent_hooks_v21.go @@ -0,0 +1,84 @@ +//go:build sqlite || sqliteonly + +package sqlitestore + +import ( + "context" + "database/sql" + "fmt" +) + +// rebuildAgentHooksV21 recreates the agent_hooks table with relaxed CHECK +// constraints — handler_type accepts `script` and source accepts `builtin`. +// Uniqueness indexes on (event, handler_type) per scope are dropped because +// a tenant may want many small script hooks per event (e.g. one redactor per +// PII class). +// +// SQLite cannot ALTER a CHECK constraint: the table must be dropped and +// recreated, then data copied. PRAGMA foreign_keys is a no-op inside a +// transaction, so the sequence runs OFF outside any tx, then wraps the rename +// + recreate + copy in one transaction for atomicity: +// +// 1. PRAGMA foreign_keys = OFF (outside tx) +// 2. BEGIN +// 3. ALTER TABLE agent_hooks RENAME TO agent_hooks_old +// 4. CREATE TABLE agent_hooks (...new CHECKs) +// 5. INSERT INTO agent_hooks SELECT * FROM agent_hooks_old +// 6. DROP TABLE agent_hooks_old +// 7. recreate the post-rename index that survives the rebuild +// 8. COMMIT +// 9. PRAGMA foreign_keys = ON +// +// Runs after the v20→v21 migration tx commits (parallel to backfillV16 at +// EnsureSchema:677) so the caller's outer migration loop sees v21 only when +// the rebuild also succeeded. +func rebuildAgentHooksV21(ctx context.Context, db *sql.DB) error { + if _, err := db.ExecContext(ctx, "PRAGMA foreign_keys = OFF"); err != nil { + return fmt.Errorf("v21 fk off: %w", err) + } + defer func() { _, _ = db.ExecContext(ctx, "PRAGMA foreign_keys = ON") }() + + tx, err := db.BeginTx(ctx, nil) + if err != nil { + return fmt.Errorf("v21 begin tx: %w", err) + } + + steps := []string{ + `ALTER TABLE agent_hooks RENAME TO agent_hooks_old`, + `CREATE TABLE agent_hooks ( + id TEXT NOT NULL PRIMARY KEY, + tenant_id TEXT NOT NULL DEFAULT '0193a5b0-7000-7000-8000-000000000001', + agent_id TEXT REFERENCES agents(id) ON DELETE CASCADE, + scope TEXT NOT NULL CHECK (scope IN ('global', 'tenant', 'agent')), + event TEXT NOT NULL, + handler_type TEXT NOT NULL CHECK (handler_type IN ('command', 'http', 'prompt', 'script')), + config TEXT NOT NULL DEFAULT '{}', + matcher TEXT, + if_expr TEXT, + timeout_ms INTEGER NOT NULL DEFAULT 5000, + on_timeout TEXT NOT NULL DEFAULT 'block' CHECK (on_timeout IN ('block', 'allow')), + priority INTEGER NOT NULL DEFAULT 0, + enabled INTEGER NOT NULL DEFAULT 1, + version INTEGER NOT NULL DEFAULT 1, + source TEXT NOT NULL DEFAULT 'ui' CHECK (source IN ('ui', 'api', 'seed', 'builtin')), + metadata TEXT NOT NULL DEFAULT '{}', + created_by TEXT, + created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), + updated_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) + )`, + `INSERT INTO agent_hooks SELECT * FROM agent_hooks_old`, + `DROP TABLE agent_hooks_old`, + `CREATE INDEX IF NOT EXISTS idx_agent_hooks_lookup ON agent_hooks (tenant_id, agent_id, event) WHERE enabled = 1`, + } + for _, s := range steps { + if _, err := tx.ExecContext(ctx, s); err != nil { + _ = tx.Rollback() + head := s + if len(head) > 60 { + head = head[:60] + } + return fmt.Errorf("v21 step failed (%q): %w", head, err) + } + } + return tx.Commit() +} diff --git a/internal/tools/delegate_tool.go b/internal/tools/delegate_tool.go index 518fa791..704d9797 100644 --- a/internal/tools/delegate_tool.go +++ b/internal/tools/delegate_tool.go @@ -172,7 +172,7 @@ func (t *DelegateTool) Execute(ctx context.Context, args map[string]any) *Result HookEvent: hooks.EventSubagentStart, Depth: hooks.DepthFrom(ctx), } - dec, err := t.hookDispatcher.Fire(ctx, evt) + r, err := t.hookDispatcher.Fire(ctx, evt) if err != nil { t.emitEvent(ctx, eventbus.EventDelegateFailed, eventbus.DelegateFailedPayload{ DelegationID: req.DelegationID, @@ -182,7 +182,9 @@ func (t *DelegateTool) Execute(ctx context.Context, args map[string]any) *Result }) return ErrorResult(fmt.Sprintf("subagent_start hook error: %v", err)) } - if dec == hooks.DecisionBlock { + // Updated* from FireResult intentionally unused — delegate has no + // mutation need in Wave 1. + if r.Decision == hooks.DecisionBlock { t.emitEvent(ctx, eventbus.EventDelegateFailed, eventbus.DelegateFailedPayload{ DelegationID: req.DelegationID, FromAgent: req.FromAgentKey, diff --git a/internal/tools/delegate_tool_hooks_test.go b/internal/tools/delegate_tool_hooks_test.go index e8139aac..dafb3fc3 100644 --- a/internal/tools/delegate_tool_hooks_test.go +++ b/internal/tools/delegate_tool_hooks_test.go @@ -16,9 +16,9 @@ type fakeDispatcher struct { calls int } -func (f *fakeDispatcher) Fire(_ context.Context, _ hooks.Event) (hooks.Decision, error) { +func (f *fakeDispatcher) Fire(_ context.Context, _ hooks.Event) (hooks.FireResult, error) { f.calls++ - return f.decision, nil + return hooks.FireResult{Decision: f.decision}, nil } // --- minimal noop stores to satisfy AgentLinkStore / AgentCRUDStore --- diff --git a/internal/upgrade/version.go b/internal/upgrade/version.go index 7fa47770..2ff3bd42 100644 --- a/internal/upgrade/version.go +++ b/internal/upgrade/version.go @@ -2,4 +2,4 @@ package upgrade // RequiredSchemaVersion is the schema migration version this binary requires. // Bump this whenever adding a new SQL migration file. -const RequiredSchemaVersion uint = 52 +const RequiredSchemaVersion uint = 53 diff --git a/migrations/000053_agent_hooks_script_and_builtin.down.sql b/migrations/000053_agent_hooks_script_and_builtin.down.sql new file mode 100644 index 00000000..f9576131 --- /dev/null +++ b/migrations/000053_agent_hooks_script_and_builtin.down.sql @@ -0,0 +1,21 @@ +-- 000053 down — restore the pre-script constraints. Any rows with the new +-- values are deleted first so the restored CHECK won't reject valid historical +-- rows. Uniqueness indexes are recreated in their original form. + +DELETE FROM agent_hooks WHERE handler_type = 'script' OR source = 'builtin'; + +ALTER TABLE agent_hooks DROP CONSTRAINT IF EXISTS agent_hooks_handler_type_check; +ALTER TABLE agent_hooks ADD CONSTRAINT agent_hooks_handler_type_check + CHECK (handler_type IN ('command', 'http', 'prompt')); + +ALTER TABLE agent_hooks DROP CONSTRAINT IF EXISTS agent_hooks_source_check; +ALTER TABLE agent_hooks ADD CONSTRAINT agent_hooks_source_check + CHECK (source IN ('ui', 'api', 'seed')); +ALTER TABLE agent_hooks ALTER COLUMN source TYPE VARCHAR(8); + +CREATE UNIQUE INDEX IF NOT EXISTS uq_hooks_global + ON agent_hooks (event, handler_type) WHERE scope = 'global'; +CREATE UNIQUE INDEX IF NOT EXISTS uq_hooks_tenant + ON agent_hooks (tenant_id, event, handler_type) WHERE scope = 'tenant'; +CREATE UNIQUE INDEX IF NOT EXISTS uq_hooks_agent + ON agent_hooks (tenant_id, agent_id, event, handler_type) WHERE scope = 'agent'; diff --git a/migrations/000053_agent_hooks_script_and_builtin.up.sql b/migrations/000053_agent_hooks_script_and_builtin.up.sql new file mode 100644 index 00000000..516a9edf --- /dev/null +++ b/migrations/000053_agent_hooks_script_and_builtin.up.sql @@ -0,0 +1,21 @@ +-- 000053 — Relax CHECK constraints for script handler + builtin source. +-- +-- Phase 03 of the Wave 1 hooks plan. Before this migration the hook system +-- only accepts command/http/prompt handlers and ui/api/seed sources. Phase 02 +-- landed a goja-backed script handler; Phase 04 ships builtin hooks whose +-- source marker must be persisted as 'builtin'. Uniqueness indexes on +-- (event, handler_type) per scope are also dropped — scripts routinely want +-- many small hooks per event (e.g. a redactor per PII type). + +ALTER TABLE agent_hooks DROP CONSTRAINT IF EXISTS agent_hooks_handler_type_check; +ALTER TABLE agent_hooks ADD CONSTRAINT agent_hooks_handler_type_check + CHECK (handler_type IN ('command', 'http', 'prompt', 'script')); + +ALTER TABLE agent_hooks ALTER COLUMN source TYPE VARCHAR(16); +ALTER TABLE agent_hooks DROP CONSTRAINT IF EXISTS agent_hooks_source_check; +ALTER TABLE agent_hooks ADD CONSTRAINT agent_hooks_source_check + CHECK (source IN ('ui', 'api', 'seed', 'builtin')); + +DROP INDEX IF EXISTS uq_hooks_global; +DROP INDEX IF EXISTS uq_hooks_tenant; +DROP INDEX IF EXISTS uq_hooks_agent; diff --git a/tests/integration/hooks_chaos_test.go b/tests/integration/hooks_chaos_test.go index ce02f840..b4a9ad0d 100644 --- a/tests/integration/hooks_chaos_test.go +++ b/tests/integration/hooks_chaos_test.go @@ -56,13 +56,13 @@ func TestHooksChaos_HTTPHandler_ProviderDown(t *testing.T) { d := newDispatcher(t, hs, map[hooks.HandlerType]hooks.Handler{ hooks.HandlerHTTP: &hookhandlers.HTTPHandler{Client: &http.Client{Timeout: 1 * time.Second}}, }) - dec, _ := d.Fire(ctx, hooks.Event{ + r, _ := d.Fire(ctx, hooks.Event{ EventID: uuid.NewString(), TenantID: tenantID, AgentID: agentID, HookEvent: hooks.EventPreToolUse, }) // Blocking event + unexpected error → fail-closed Block. - if dec != hooks.DecisionBlock { - t.Errorf("decision=%q, want block (fail-closed on provider down)", dec) + if r.Decision != hooks.DecisionBlock { + t.Errorf("decision=%q, want block (fail-closed on provider down)", r.Decision) } } @@ -104,14 +104,14 @@ func TestHooksChaos_PerHookTimeout(t *testing.T) { hooks.HandlerHTTP: &hookhandlers.HTTPHandler{Client: srv.Client()}, }) start := time.Now() - dec, _ := d.Fire(ctx, hooks.Event{ + r, _ := d.Fire(ctx, hooks.Event{ EventID: uuid.NewString(), TenantID: tenantID, AgentID: agentID, HookEvent: hooks.EventPreToolUse, }) elapsed := time.Since(start) - if dec != hooks.DecisionBlock { - t.Errorf("decision=%q, want block (OnTimeout=block)", dec) + if r.Decision != hooks.DecisionBlock { + t.Errorf("decision=%q, want block (OnTimeout=block)", r.Decision) } // Must NOT have waited the full 2s; per-hook timeout caps at ~300ms+retry. if elapsed > 4*time.Second { @@ -193,15 +193,15 @@ func TestHooksChaos_LoopDepthExceeded(t *testing.T) { // Wrap ctx with depth > MaxLoopDepth. ctx := hooks.WithDepth(tenantCtx(tenantID), hooks.MaxLoopDepth+1) - dec, err := d.Fire(ctx, hooks.Event{ + r, err := d.Fire(ctx, hooks.Event{ EventID: uuid.NewString(), TenantID: tenantID, AgentID: agentID, HookEvent: hooks.EventPreToolUse, }) if !errors.Is(err, hooks.ErrLoopDepthExceeded) { t.Errorf("err=%v, want ErrLoopDepthExceeded", err) } - if dec != hooks.DecisionError { - t.Errorf("decision=%q, want error", dec) + if r.Decision != hooks.DecisionError { + t.Errorf("decision=%q, want error", r.Decision) } } @@ -295,13 +295,13 @@ func TestHooksChaos_HTTPHandler_5xxRetriesThenErrors(t *testing.T) { d := newDispatcher(t, hs, map[hooks.HandlerType]hooks.Handler{ hooks.HandlerHTTP: &hookhandlers.HTTPHandler{Client: srv.Client()}, }) - dec, _ := d.Fire(ctx, hooks.Event{ + r, _ := d.Fire(ctx, hooks.Event{ EventID: uuid.NewString(), TenantID: tenantID, AgentID: agentID, HookEvent: hooks.EventPreToolUse, }) // Blocking event + repeated 5xx → fail-closed Block. - if dec != hooks.DecisionBlock { - t.Errorf("decision=%q, want block (fail-closed on 5xx)", dec) + if r.Decision != hooks.DecisionBlock { + t.Errorf("decision=%q, want block (fail-closed on 5xx)", r.Decision) } // Verify the handler did perform its single retry (hits >= 2). if hits.Load() < 2 { diff --git a/tests/integration/hooks_e2e_test.go b/tests/integration/hooks_e2e_test.go index 9dc4ebd7..b1ff549d 100644 --- a/tests/integration/hooks_e2e_test.go +++ b/tests/integration/hooks_e2e_test.go @@ -73,14 +73,14 @@ func TestHooksE2E_AllEvents_AllowsDefault(t *testing.T) { db.Exec("DELETE FROM agent_hooks WHERE id = $1", hookID) }) - dec, err := d.Fire(ctx, hooks.Event{ + r, err := d.Fire(ctx, hooks.Event{ EventID: uuid.NewString(), TenantID: tenantID, AgentID: agentID, HookEvent: ev, }) if err != nil { t.Fatalf("Fire %s: %v", ev, err) } - if dec != hooks.DecisionAllow { - t.Errorf("%s: decision=%q, want allow", ev, dec) + if r.Decision != hooks.DecisionAllow { + t.Errorf("%s: decision=%q, want allow", ev, r.Decision) } // Non-blocking events write audit from a goroutine; poll briefly @@ -122,15 +122,15 @@ func TestHooksE2E_CommandHandler_BlockOnExitTwo(t *testing.T) { hooks.HandlerCommand: &hookhandlers.CommandHandler{Edition: edition.Lite}, }) - dec, err := d.Fire(ctx, hooks.Event{ + r, err := d.Fire(ctx, hooks.Event{ EventID: uuid.NewString(), TenantID: tenantID, AgentID: agentID, HookEvent: hooks.EventPreToolUse, }) if err != nil { t.Fatalf("Fire: %v", err) } - if dec != hooks.DecisionBlock { - t.Errorf("decision=%q, want block (exit 2)", dec) + if r.Decision != hooks.DecisionBlock { + t.Errorf("decision=%q, want block (exit 2)", r.Decision) } assertHookAuditCount(t, db, hookID, "block", 1) } @@ -174,12 +174,12 @@ func TestHooksE2E_ContextUpdateInjection(t *testing.T) { d := newDispatcher(t, hs, map[hooks.HandlerType]hooks.Handler{ hooks.HandlerHTTP: &hookhandlers.HTTPHandler{Client: srv.Client()}, }) - dec, err := d.Fire(ctx, hooks.Event{ + r, err := d.Fire(ctx, hooks.Event{ EventID: uuid.NewString(), TenantID: tenantID, AgentID: agentID, HookEvent: hooks.EventUserPromptSubmit, }) - if err != nil || dec != hooks.DecisionAllow { - t.Errorf("decision=%q err=%v, want allow+nil", dec, err) + if err != nil || r.Decision != hooks.DecisionAllow { + t.Errorf("decision=%q err=%v, want allow+nil", r.Decision, err) } assertHookAuditCount(t, db, hookID, "allow", 1) } @@ -276,13 +276,13 @@ func TestHooksE2E_EditionGate_CommandOnStandard(t *testing.T) { d := newDispatcher(t, hs, map[hooks.HandlerType]hooks.Handler{ hooks.HandlerCommand: &hookhandlers.CommandHandler{Edition: edition.Standard}, }) - dec, _ := d.Fire(ctx, hooks.Event{ + r, _ := d.Fire(ctx, hooks.Event{ EventID: uuid.NewString(), TenantID: tenantID, AgentID: agentID, HookEvent: hooks.EventPreToolUse, }) // Blocking event + handler error → fail-closed Block. - if dec != hooks.DecisionBlock { - t.Errorf("decision=%q, want block (edition gate)", dec) + if r.Decision != hooks.DecisionBlock { + t.Errorf("decision=%q, want block (edition gate)", r.Decision) } } diff --git a/tests/integration/hooks_pipeline_test.go b/tests/integration/hooks_pipeline_test.go index a1fe3f02..6d330ff2 100644 --- a/tests/integration/hooks_pipeline_test.go +++ b/tests/integration/hooks_pipeline_test.go @@ -88,12 +88,12 @@ func TestHooksIntegration_HTTPHandler_AllowWritesAudit(t *testing.T) { AgentID: agentID, HookEvent: hooks.EventUserPromptSubmit, } - dec, err := d.Fire(ctx, ev) + r, err := d.Fire(ctx, ev) if err != nil { t.Fatalf("Fire: %v", err) } - if dec != hooks.DecisionAllow { - t.Errorf("decision=%q, want allow", dec) + if r.Decision != hooks.DecisionAllow { + t.Errorf("decision=%q, want allow", r.Decision) } // Give the audit writer a moment (sync path, but defensive). @@ -159,12 +159,12 @@ func TestHooksIntegration_HTTPHandler_Block(t *testing.T) { AgentID: agentID, HookEvent: hooks.EventUserPromptSubmit, } - dec, err := d.Fire(ctx, ev) + r, err := d.Fire(ctx, ev) if err != nil { t.Fatalf("Fire: %v", err) } - if dec != hooks.DecisionBlock { - t.Errorf("decision=%q, want block", dec) + if r.Decision != hooks.DecisionBlock { + t.Errorf("decision=%q, want block", r.Decision) } var count int @@ -221,12 +221,12 @@ func TestHooksIntegration_CommandHandler_LiteEdition(t *testing.T) { AgentID: agentID, HookEvent: hooks.EventUserPromptSubmit, } - dec, err := d.Fire(ctx, ev) + r, err := d.Fire(ctx, ev) if err != nil { t.Fatalf("Fire: %v", err) } - if dec != hooks.DecisionAllow { - t.Errorf("decision=%q, want allow", dec) + if r.Decision != hooks.DecisionAllow { + t.Errorf("decision=%q, want allow", r.Decision) } var count int