diff --git a/internal/services/executionsvc/executionservice_guardrail.go b/internal/services/executionsvc/executionservice_guardrail.go index d6ddd0952..4bd638b92 100644 --- a/internal/services/executionsvc/executionservice_guardrail.go +++ b/internal/services/executionsvc/executionservice_guardrail.go @@ -138,8 +138,12 @@ const shellCommandNodeTypeID = "process-shell-command" // the approval ceremony entirely, and a block containing even one // unlisted or deny-listed line still asks, same as today. func (e *ExecutionService) evaluateVerdict(workflowID string, node composition.Node, ec composition.ExecContext, class guardrail.EffectClass) guardrail.Verdict { + // One rule-evaluation core, two entry points (docs/adr/0047 §5): + // EvaluateStep is the same core guardrailsvc.RequestGuardedAction's + // EvaluateAction calls -- this is the execution gate's own call + // site, never a second evaluation of these rules. if node.NodeTypeID != shellCommandNodeTypeID { - return guardrail.Evaluate(e.guard.Rules(), guardrailsvc.GuardrailStep(workflowID, node, ec), class) + return e.guard.EvaluateStep(guardrailsvc.GuardrailStep(workflowID, node, ec), class) } steps := composition.ParseShellCommandBlock(ec.Payload) if len(steps) == 0 { diff --git a/internal/services/guardrailsvc/guardrailservice.go b/internal/services/guardrailsvc/guardrailservice.go index 55afd00d9..f05e10d65 100644 --- a/internal/services/guardrailsvc/guardrailservice.go +++ b/internal/services/guardrailsvc/guardrailservice.go @@ -58,6 +58,12 @@ type GuardrailService struct { store settings.Store rules []guardrail.Rule comp *compositionsvc.CompositionService + // guardedActionsMu guards guardedActions (guardrailservice_request.go) + // -- a separate lock from mu (rule CRUD): the two protect unrelated + // state, and a long-parked guarded action must never block a rule + // save/list. + guardedActionsMu sync.Mutex + guardedActions map[string]*guardedActionRecord } func NewGuardrailService(store settings.Store, comp *compositionsvc.CompositionService) *GuardrailService { diff --git a/internal/services/guardrailsvc/guardrailservice_request.go b/internal/services/guardrailsvc/guardrailservice_request.go new file mode 100644 index 000000000..2442cf676 --- /dev/null +++ b/internal/services/guardrailsvc/guardrailservice_request.go @@ -0,0 +1,211 @@ +package guardrailsvc + +import ( + "context" + "fmt" + "time" + + "github.com/alicoding/mill/internal/domain/guardrail" + "github.com/google/uuid" +) + +// One rule-evaluation core, two entry points (docs/adr/0047 §5): every +// verdict -- whether for an about-to-execute workflow step or a +// non-workflow GuardedAction -- is decided by guardrail.Evaluate over +// the SAME rule set (EvaluateStep below). The execution gate +// (executionsvc.evaluateVerdict) and RequestGuardedAction are its two +// callers; neither one owns a second, parallel policy plane. + +// EvaluateStep is the guardrail's rule-evaluation core: judges a +// fully-formed Step against the current rules with guardrail.Evaluate's +// deny > ask > allow > class-default precedence. A thin wrapper by +// design -- the extraction this pays for is a single call site every +// caller (a workflow step, a generic action) shares, so they can never +// silently diverge into two different evaluations of the same rules. +func (g *GuardrailService) EvaluateStep(step guardrail.Step, class guardrail.EffectClass) guardrail.Verdict { + return guardrail.Evaluate(g.Rules(), step, class) +} + +// EvaluateAction adapts a generic action's kind/attributes into a Step +// and evaluates it through EvaluateStep. kind fills Step.NodeTypeID -- +// the same scope axis a workflow node's own NodeTypeID already targets +// -- so a rule authored against a NodeTypeID scope also targets a +// guarded action of that kind, by construction, with no separate rule +// vocabulary to maintain. +func (g *GuardrailService) EvaluateAction(kind string, attributes map[string]string, class guardrail.EffectClass) guardrail.Verdict { + attrs := make(map[string]any, len(attributes)) + for k, v := range attributes { + attrs[k] = v + } + step := guardrail.Step{ + NodeTypeID: kind, + Env: guardrail.ConditionEnv("", attrs, nil), + } + return g.EvaluateStep(step, class) +} + +// guardedActionTimeout is the same §8 fail-safe every other park in +// this codebase already resolves an unattended ask to +// (guardrailApprovalTimeout in executionsvc, mcpWriteExpiry in +// mcpsvc) -- an unresolved guarded action denies itself closed after a +// day rather than blocking its caller forever. +const guardedActionTimeout = 24 * time.Hour + +// GuardedAction is what a non-workflow caller -- an agent today, a +// plugin once the out-of-tree loader ships (docs/adr/0047 §5) -- asks +// the guardrail to judge. Attributes is evaluated by the exact same +// rule conditions (guardrail.ConditionEnv) a workflow step's own +// Attributes already are. +type GuardedAction struct { + // Kind names the action class for rule targeting (fills + // Step.NodeTypeID -- see EvaluateAction). + Kind string + // Attributes is the same map[string]string vocabulary a workflow + // step's rules already evaluate. + Attributes map[string]string + // Description feeds the approval UI (Review, the floating prompt). + Description string + // Source names who's asking (an agent id, a future plugin id) -- + // carried onto the parked PendingGuardedAction's own Source field. + Source string +} + +// Decision is RequestGuardedAction's outcome. Approved is what every +// caller branches on; Effect/RuleID/RuleLabel identify what decided it +// (the original verdict, even after a human resolves an ask -- Effect +// stays "ask", Approved carries the human's actual answer). +type Decision struct { + Approved bool + Effect guardrail.Effect + RuleID string + RuleLabel string +} + +// PendingGuardedAction is a parked, not-yet-resolved GuardedAction -- +// the non-workflow analogue of executionsvc.PendingApproval (docs/adr/0047 +// §5 point 3): additive, never reshaping the workflow park it is meant +// to one day render alongside. +type PendingGuardedAction struct { + ID string + Kind string + Attributes map[string]string + Description string + Source string + CreatedAt time.Time +} + +// guardedActionRecord is one parked action's live bookkeeping -- +// decision is unexported so it never round-trips through anything that +// marshals PendingGuardedAction. +type guardedActionRecord struct { + PendingGuardedAction + decision chan bool +} + +// RequestGuardedAction is the guardrail's public "submit an action for +// evaluation" entry (docs/adr/0047 §5) -- the second caller of the +// rule-evaluation core EvaluateStep/EvaluateAction above already share +// with the execution gate. allow/deny resolve immediately; ask parks a +// PendingGuardedAction and blocks until a human resolves it +// (resolveGuardedAction), ctx is cancelled (a clean withdrawal -- the +// pending record is removed either way, never left orphaned), or the +// same 24h fail-safe every other park in this codebase already uses +// elapses. +// +// class is always guardrail.ClassExternal: a guarded action is by +// definition a request for a primitive the caller does not hold +// directly (docs/adr/0047 §2), which is exactly what ClassExternal's +// ask-by-default fail-safe already gates without a rule naming it. +// +// Not Wails-bound this slice -- a Go-internal entry for future +// in-process consumers; binding it is deferred until a real caller +// needs it from the frontend. +// +//wails:ignore +func (g *GuardrailService) RequestGuardedAction(ctx context.Context, action GuardedAction) (Decision, error) { + verdict := g.EvaluateAction(action.Kind, action.Attributes, guardrail.ClassExternal) + switch verdict.Effect { + case guardrail.EffectAllow: + return Decision{Approved: true, Effect: verdict.Effect, RuleID: verdict.RuleID, RuleLabel: verdict.RuleLabel}, nil + case guardrail.EffectDeny: + return Decision{Approved: false, Effect: verdict.Effect, RuleID: verdict.RuleID, RuleLabel: verdict.RuleLabel}, nil + } + + rec := &guardedActionRecord{ + PendingGuardedAction: PendingGuardedAction{ + ID: uuid.NewString(), + Kind: action.Kind, + Attributes: action.Attributes, + Description: action.Description, + Source: action.Source, + CreatedAt: time.Now(), + }, + decision: make(chan bool, 1), + } + g.parkGuardedAction(rec) + defer g.unparkGuardedAction(rec.ID) + + select { + case approve := <-rec.decision: + return Decision{Approved: approve, Effect: verdict.Effect, RuleID: verdict.RuleID, RuleLabel: verdict.RuleLabel}, nil + case <-ctx.Done(): + return Decision{}, ctx.Err() + case <-time.After(guardedActionTimeout): + return Decision{Approved: false}, fmt.Errorf("guardrail: guarded action approval timed out after %s", guardedActionTimeout) + } +} + +// parkGuardedAction/unparkGuardedAction/resolveGuardedAction/ +// pendingGuardedActions are the in-process pending store: an in-memory +// map, not settings-persisted (unlike mcpsvc's MCPWriteRecord) -- +// RequestGuardedAction has no real caller yet in this slice (the MCP +// rebase is next), so there is nothing whose restart-survival matters +// today; the record shape (PendingGuardedAction) carries everything a +// future durable store would need to persist. + +func (g *GuardrailService) parkGuardedAction(rec *guardedActionRecord) { + g.guardedActionsMu.Lock() + defer g.guardedActionsMu.Unlock() + if g.guardedActions == nil { + g.guardedActions = map[string]*guardedActionRecord{} + } + g.guardedActions[rec.ID] = rec +} + +func (g *GuardrailService) unparkGuardedAction(id string) { + g.guardedActionsMu.Lock() + defer g.guardedActionsMu.Unlock() + delete(g.guardedActions, id) +} + +// resolveGuardedAction delivers a human decision to a parked action -- +// the Go-internal analogue of executionsvc.ResolveApproval. Returns +// false when id names no currently-parked action (already resolved, +// timed out, or never existed), mirroring ResolveApproval's own +// unknown-id error path at the caller's discretion. +func (g *GuardrailService) resolveGuardedAction(id string, approve bool) bool { + g.guardedActionsMu.Lock() + rec, ok := g.guardedActions[id] + g.guardedActionsMu.Unlock() + if !ok { + return false + } + select { + case rec.decision <- approve: + default: + } + return true +} + +// pendingGuardedActions lists every currently-parked action -- the +// listing half of the pending model a future Review/floating-prompt +// wiring (or the MCP rebase) will read from. +func (g *GuardrailService) pendingGuardedActions() []PendingGuardedAction { + g.guardedActionsMu.Lock() + defer g.guardedActionsMu.Unlock() + out := make([]PendingGuardedAction, 0, len(g.guardedActions)) + for _, rec := range g.guardedActions { + out = append(out, rec.PendingGuardedAction) + } + return out +} diff --git a/internal/services/guardrailsvc/guardrailservice_request_test.go b/internal/services/guardrailsvc/guardrailservice_request_test.go new file mode 100644 index 000000000..3cca7364a --- /dev/null +++ b/internal/services/guardrailsvc/guardrailservice_request_test.go @@ -0,0 +1,223 @@ +package guardrailsvc + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/alicoding/mill/internal/domain/guardrail" +) + +// --- EvaluateAction/EvaluateStep: the extracted core against seeded rules --- + +func TestEvaluateAction_NoRules_UsesClassDefault(t *testing.T) { + g, _ := newTestGuardrailService(t) + v := g.EvaluateAction("some-kind", map[string]string{}, guardrail.ClassExternal) + if v.Effect != guardrail.EffectAllow && v.Effect != guardrail.EffectAsk { + t.Fatalf("EvaluateAction() with no rules = %+v, want the external-class default (ask)", v) + } + if v.Effect != guardrail.EffectAsk { + t.Errorf("EvaluateAction() with no rules = %+v, want ask (ClassExternal's default)", v) + } +} + +func TestEvaluateAction_KindFillsNodeTypeIDScope(t *testing.T) { + g, _ := newTestGuardrailService(t) + if _, err := g.CreateRule(guardrail.Rule{ + Label: "Allow this action kind", Effect: guardrail.EffectAllow, NodeTypeID: "plugin-write-file", + }); err != nil { + t.Fatalf("CreateRule: %v", err) + } + + got := g.EvaluateAction("plugin-write-file", map[string]string{"path": "/tmp/x"}, guardrail.ClassExternal) + if got.Effect != guardrail.EffectAllow || got.RuleLabel != "Allow this action kind" { + t.Errorf("EvaluateAction(kind=plugin-write-file) = %+v, want the NodeTypeID-scoped rule to match by kind", got) + } + + other := g.EvaluateAction("some-other-kind", nil, guardrail.ClassExternal) + if other.Effect != guardrail.EffectAsk { + t.Errorf("EvaluateAction(kind=some-other-kind) = %+v, want the ask default (rule scoped to a different kind must not match)", other) + } +} + +func TestEvaluateAction_DenyBeatsAskBeatsAllow(t *testing.T) { + g, _ := newTestGuardrailService(t) + for _, r := range []guardrail.Rule{ + {Label: "allow", Effect: guardrail.EffectAllow, NodeTypeID: "k"}, + {Label: "ask", Effect: guardrail.EffectAsk, NodeTypeID: "k"}, + {Label: "deny", Effect: guardrail.EffectDeny, NodeTypeID: "k"}, + } { + if _, err := g.CreateRule(r); err != nil { + t.Fatalf("CreateRule(%s): %v", r.Label, err) + } + } + got := g.EvaluateAction("k", nil, guardrail.ClassExternal) + if got.Effect != guardrail.EffectDeny || got.RuleLabel != "deny" { + t.Errorf("EvaluateAction() = %+v, want deny to win over ask/allow", got) + } +} + +// --- RequestGuardedAction: allow/deny immediate paths --- + +func TestRequestGuardedAction_Allow_ReturnsImmediately(t *testing.T) { + g, _ := newTestGuardrailService(t) + if _, err := g.CreateRule(guardrail.Rule{ + Label: "Allow it", Effect: guardrail.EffectAllow, NodeTypeID: "test-kind", + }); err != nil { + t.Fatalf("CreateRule: %v", err) + } + + decision, err := g.RequestGuardedAction(context.Background(), GuardedAction{ + Kind: "test-kind", Attributes: map[string]string{"x": "1"}, Description: "do a thing", Source: "test-agent", + }) + if err != nil { + t.Fatalf("RequestGuardedAction: %v", err) + } + if !decision.Approved || decision.Effect != guardrail.EffectAllow || decision.RuleLabel != "Allow it" { + t.Errorf("RequestGuardedAction() = %+v, want an immediate approved decision naming the allow rule", decision) + } +} + +func TestRequestGuardedAction_Deny_ReturnsImmediately(t *testing.T) { + g, _ := newTestGuardrailService(t) + if _, err := g.CreateRule(guardrail.Rule{ + Label: "Deny it", Effect: guardrail.EffectDeny, NodeTypeID: "test-kind", + }); err != nil { + t.Fatalf("CreateRule: %v", err) + } + + decision, err := g.RequestGuardedAction(context.Background(), GuardedAction{Kind: "test-kind"}) + if err != nil { + t.Fatalf("RequestGuardedAction: %v", err) + } + if decision.Approved || decision.Effect != guardrail.EffectDeny || decision.RuleLabel != "Deny it" { + t.Errorf("RequestGuardedAction() = %+v, want an immediate denied decision naming the deny rule", decision) + } +} + +// --- RequestGuardedAction: ask parks, resolves, unblocks the caller --- + +// awaitPending polls until exactly one action is parked (or fails the +// test) -- RequestGuardedAction parks asynchronously relative to the +// resolver goroutine below, so the test must observe the park before +// resolving it. +func awaitPending(t *testing.T, g *GuardrailService) PendingGuardedAction { + t.Helper() + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if pending := g.pendingGuardedActions(); len(pending) == 1 { + return pending[0] + } + time.Sleep(time.Millisecond) + } + t.Fatal("RequestGuardedAction never parked a PendingGuardedAction") + return PendingGuardedAction{} +} + +func TestRequestGuardedAction_Ask_ParkedThenApproved_UnblocksApproved(t *testing.T) { + g, _ := newTestGuardrailService(t) // no rules -- ClassExternal defaults to ask + + type result struct { + decision Decision + err error + } + done := make(chan result, 1) + go func() { + d, err := g.RequestGuardedAction(context.Background(), GuardedAction{ + Kind: "test-kind", Description: "needs a human", Source: "test-agent", + }) + done <- result{d, err} + }() + + pending := awaitPending(t, g) + if pending.Description != "needs a human" || pending.Source != "test-agent" { + t.Errorf("parked PendingGuardedAction = %+v, want the requested description/source carried through", pending) + } + if !g.resolveGuardedAction(pending.ID, true) { + t.Fatal("resolveGuardedAction(approve) on the just-parked id: want true") + } + + select { + case r := <-done: + if r.err != nil { + t.Fatalf("RequestGuardedAction after approve: %v", r.err) + } + if !r.decision.Approved || r.decision.Effect != guardrail.EffectAsk { + t.Errorf("RequestGuardedAction() after approve = %+v, want Approved=true, Effect=ask", r.decision) + } + case <-time.After(2 * time.Second): + t.Fatal("RequestGuardedAction never unblocked after resolveGuardedAction(approve)") + } + if pending := g.pendingGuardedActions(); len(pending) != 0 { + t.Errorf("pendingGuardedActions() after resolution = %+v, want empty (unparked)", pending) + } +} + +func TestRequestGuardedAction_Ask_ParkedThenDenied_UnblocksDenied(t *testing.T) { + g, _ := newTestGuardrailService(t) + + type result struct { + decision Decision + err error + } + done := make(chan result, 1) + go func() { + d, err := g.RequestGuardedAction(context.Background(), GuardedAction{Kind: "test-kind"}) + done <- result{d, err} + }() + + pending := awaitPending(t, g) + if !g.resolveGuardedAction(pending.ID, false) { + t.Fatal("resolveGuardedAction(deny) on the just-parked id: want true") + } + + select { + case r := <-done: + if r.err != nil { + t.Fatalf("RequestGuardedAction after deny: %v", r.err) + } + if r.decision.Approved { + t.Errorf("RequestGuardedAction() after deny = %+v, want Approved=false", r.decision) + } + case <-time.After(2 * time.Second): + t.Fatal("RequestGuardedAction never unblocked after resolveGuardedAction(deny)") + } +} + +// --- RequestGuardedAction: ctx-cancel withdraws the pending action cleanly --- + +func TestRequestGuardedAction_Ask_CtxCancel_WithdrawsCleanly(t *testing.T) { + g, _ := newTestGuardrailService(t) + ctx, cancel := context.WithCancel(context.Background()) + + type result struct { + decision Decision + err error + } + done := make(chan result, 1) + go func() { + d, err := g.RequestGuardedAction(ctx, GuardedAction{Kind: "test-kind"}) + done <- result{d, err} + }() + + pending := awaitPending(t, g) + cancel() + + select { + case r := <-done: + if !errors.Is(r.err, context.Canceled) { + t.Errorf("RequestGuardedAction() after ctx-cancel err = %v, want context.Canceled", r.err) + } + case <-time.After(2 * time.Second): + t.Fatal("RequestGuardedAction never unblocked after ctx-cancel") + } + if remaining := g.pendingGuardedActions(); len(remaining) != 0 { + t.Errorf("pendingGuardedActions() after ctx-cancel = %+v, want the withdrawn action removed", remaining) + } + // A stale resolve arriving after the withdrawal must be a no-op, not + // a panic on a closed/unknown channel. + if g.resolveGuardedAction(pending.ID, true) { + t.Error("resolveGuardedAction() on an already-withdrawn id: want false") + } +}