diff --git a/internal/services/mcpsvc/millmcpservice.go b/internal/services/mcpsvc/millmcpservice.go index 3ed162ee3..ca325b12d 100644 --- a/internal/services/mcpsvc/millmcpservice.go +++ b/internal/services/mcpsvc/millmcpservice.go @@ -25,6 +25,7 @@ import ( "github.com/alicoding/mill/internal/services/atlassvc" "github.com/alicoding/mill/internal/services/compositionsvc" "github.com/alicoding/mill/internal/services/configuresvc" + "github.com/alicoding/mill/internal/services/guardrailsvc" "github.com/modelcontextprotocol/go-sdk/mcp" ) @@ -95,6 +96,15 @@ type MillMCPService struct { // until wired (every test that doesn't call SetAuditResolver), in // which case resolution calls are simply skipped. auditResolver func(writeID string, outcome mcpaudit.Outcome, errText string) + // guard is the shared guardrail rule-evaluation core (docs/adr/0047 + // §5.4) -- late-bound via SetGuardrailService, same injected-seam + // shape as auditResolver above (guardrailsvc.NewGuardrailService is + // constructed before this service in main.go, but wiring it as a + // constructor parameter would force every existing test in this + // package to construct one too; nil until wired, in which case + // gateWrite falls through to its unconditional-ask park, exactly the + // pre-rebase behavior). + guard *guardrailsvc.GuardrailService } // SetAuditResolver wires the audit trail's parked-write resolution @@ -107,6 +117,15 @@ func (m *MillMCPService) SetAuditResolver(fn func(writeID string, outcome mcpaud m.auditResolver = fn } +// SetGuardrailService wires the shared rule-evaluation core (main.go, +// after guardrailsvc.NewGuardrailService succeeds) so a gated write is +// judged by the SAME user-authored rules a workflow step's own guardrail +// gate already evaluates (docs/adr/0047 §5.4) -- see the guard field's +// own doc comment for why this is late-bound. +func (m *MillMCPService) SetGuardrailService(g *guardrailsvc.GuardrailService) { + m.guard = g +} + // NewMillMCPService builds the MCP server and registers every // resource/template up front -- registration is static (the resource // URIs themselves never change), only the data a read returns is diff --git a/internal/services/mcpsvc/millmcpservice_approval.go b/internal/services/mcpsvc/millmcpservice_approval.go index 2786da8a6..caf4a4b4b 100644 --- a/internal/services/mcpsvc/millmcpservice_approval.go +++ b/internal/services/mcpsvc/millmcpservice_approval.go @@ -12,16 +12,19 @@ import ( ) // Per-write MCP approval lifecycle: park-and-poll (docs/adr/0032, -// superseding the old bounded-120s-blocking-wait shape). A gated write -// tool call submits (description, toolName, argsJSON) to gateWrite, -// which parks the write itself -- not a live channel -- as a durable +// superseding the old bounded-120s-blocking-wait shape). gateWrite first +// judges the write through the shared guardrail rule-evaluation core +// (writeVerdictShortCircuit, millmcpservice_approval_guardrail.go -- +// docs/adr/0047 §5.4): an explicit allow/deny rule short-circuits the +// park entirely; the ask default (no rule, matching pre-rebase behavior) +// parks the write itself -- not a live channel -- as a durable // MCPWriteRecord persisted via the settings store, so an approval can // execute the write later even if the requester (or Mill itself) has -// since restarted. A short in-call courtesy window (10s) keeps the -// co-present-approver case a single round trip; past that, the call -// returns a SUCCESSFUL parked-pending text so the client polls -// check_write_status instead of the connection dying against a real -// host's own ~60s to-first-byte timer (ADR-0032's own research). +// since restarted. A short in-call courtesy window (10s) keeps a +// co-present approver to one round trip; past that, the call returns a +// SUCCESSFUL parked-pending text so the client polls check_write_status +// instead of the connection dying against a real host's own ~60s +// to-first-byte timer (ADR-0032's own research). // MCPWriteApprovalKey: when writes are enabled at all // (MCPWriteEnabledKey), this second toggle decides whether each write @@ -191,6 +194,13 @@ func (m *MillMCPService) gateWrite(toolName, description, argsJSON string) (*mcp return textResult(text), nil } + // docs/adr/0047 §5.4: an explicit allow/deny rule short-circuits the + // ask-every-time default here; "ask" (no rule, or no guardrail + // service wired) falls through to the unchanged park below. + if result, err, handled := m.writeVerdictShortCircuit(toolName, description, argsJSON); handled { + return result, err + } + rec := &MCPWriteRecord{ ID: uuid.NewString(), Description: description, diff --git a/internal/services/mcpsvc/millmcpservice_approval_guardrail.go b/internal/services/mcpsvc/millmcpservice_approval_guardrail.go new file mode 100644 index 000000000..80c7c5b14 --- /dev/null +++ b/internal/services/mcpsvc/millmcpservice_approval_guardrail.go @@ -0,0 +1,74 @@ +package mcpsvc + +// The rule-plane half of the park-and-poll approval lifecycle +// (millmcpservice_approval.go): a gated write is judged against the +// shared guardrail rule-evaluation core before it ever reaches the +// durable park (docs/adr/0047 §5.4) -- split out once the lifecycle file +// crossed the 500-line limit (CLAUDE.md/§1.3), same "split along a real +// seam" discipline millmcpservice_approval_query.go already established. +// +// The durable park itself stays this package's own MCPWriteRecord, not +// guardrailsvc's in-memory PendingGuardedAction: unifying the park too +// would need PendingGuardedAction to gain durable persistence and an +// apply-on-approve payload, which several existing tests in this +// package (constructed with no guardrail dependency at all, asserting +// directly on this package's own store-persistence failure paths) +// assume it never does -- tracked as a follow-up, not folded into this +// slice. + +import ( + "fmt" + + "github.com/alicoding/mill/internal/domain/guardrail" + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// mcpWriteGuardrailKind is the guardrail Kind a gated write is judged +// under (docs/adr/0047 §5.4) -- fills guardrail.Step.NodeTypeID via +// EvaluateAction, the same scope axis a workflow node's own NodeTypeID +// already targets, so a rule authored through the ordinary Guardrail +// CRUD governs every gated write tool uniformly, never a parallel rule +// vocabulary. +const mcpWriteGuardrailKind = "mcp-write" + +// evaluateWriteVerdict judges one gated write against the shared +// guardrail rule-evaluation core (docs/adr/0047 §5.4's "one entry, one +// rule plane") -- the exact EvaluateStep/EvaluateAction core +// RequestGuardedAction and the workflow execution gate already share, so +// a rule created via the normal Configure > Guardrail CRUD applies to +// MCP writes too. m.guard is nil in every test that never calls +// SetGuardrailService; the fail-safe there is the same one +// guardrail.ClassExternal's own DefaultEffect already returns for "no +// rule matched" -- ask, i.e. always park, exactly gateWrite's pre-rebase +// behavior. +func (m *MillMCPService) evaluateWriteVerdict(toolName, description string) guardrail.Verdict { + if m.guard == nil { + return guardrail.Verdict{Effect: guardrail.EffectAsk} + } + attrs := map[string]string{"toolName": toolName, "description": description} + return m.guard.EvaluateAction(mcpWriteGuardrailKind, attrs, guardrail.ClassExternal) +} + +// writeVerdictShortCircuit evaluates the write and, when the verdict is +// allow or deny, returns the FINAL result/error for gateWrite to hand +// back directly (handled=true) -- allow executes immediately, deny +// blocks with no park at all. handled=false means "ask" (no rule +// matched, or no guardrail service wired): gateWrite falls through to +// its existing durable park unchanged. +func (m *MillMCPService) writeVerdictShortCircuit(toolName, description, argsJSON string) (result *mcp.CallToolResult, err error, handled bool) { + switch verdict := m.evaluateWriteVerdict(toolName, description); verdict.Effect { + case guardrail.EffectDeny: + reason := verdict.RuleLabel + if reason == "" { + reason = verdict.RuleID + } + return nil, fmt.Errorf("denied by guardrail rule %q", reason), true + case guardrail.EffectAllow: + text, execErr := m.execute(toolName, argsJSON) + if execErr != nil { + return nil, execErr, true + } + return textResult(text), nil, true + } + return nil, nil, false +} diff --git a/internal/services/mcpsvc/millmcpservice_guardrail_test.go b/internal/services/mcpsvc/millmcpservice_guardrail_test.go new file mode 100644 index 000000000..9737df658 --- /dev/null +++ b/internal/services/mcpsvc/millmcpservice_guardrail_test.go @@ -0,0 +1,112 @@ +package mcpsvc + +// The new rules-leverage payoff (docs/adr/0047 §5.4): a guardrail rule +// authored through the ordinary Configure > Guardrail CRUD -- the exact +// same guardrailsvc.GuardrailService.CreateRule a human uses from the +// Review "Rules" audit view -- now governs a gated MCP write, because +// gateWrite's evaluateWriteVerdict judges it through EvaluateAction, the +// same rule-evaluation core the workflow execution gate and +// RequestGuardedAction already share (guardrailservice_request.go). + +import ( + "testing" + + "github.com/alicoding/mill/internal/domain/composition" + "github.com/alicoding/mill/internal/domain/guardrail" + "github.com/alicoding/mill/internal/services/compositionsvc" + "github.com/alicoding/mill/internal/services/configuresvc" + "github.com/alicoding/mill/internal/services/guardrailsvc" + "github.com/alicoding/mill/internal/services/servicetest" +) + +// newGuardrailWiredHarness builds a MillMCPService with a real, +// store-backed GuardrailService wired via SetGuardrailService -- the +// main.go wiring this test proves matters, unlike every other harness in +// this package which leaves m.guard nil deliberately. +func newGuardrailWiredHarness(t *testing.T) (*MillMCPService, *guardrailsvc.GuardrailService, *compositionsvc.CompositionService) { + t.Helper() + store := servicetest.NewFakeStore() + comp := compositionsvc.NewCompositionService(store) + cfg := configuresvc.NewConfigureService(store, comp, servicetest.FakeCredentialStore{}) + guard := guardrailsvc.NewGuardrailService(store, comp) + + m := NewMillMCPService("0.0.0-test", comp, cfg, store, nil) + m.SetGuardrailService(guard) + if err := store.Set(MCPWriteEnabledKey, "true"); err != nil { + t.Fatalf("set write key: %v", err) + } + // approval key left unset: required is the default -- a rule must be + // what changes the outcome here, not a relaxed toggle. + return m, guard, comp +} + +// TestGateWrite_DenyRuleFromNormalCRUD_BlocksTheWriteWithNoPark proves a +// guardrail rule created through CreateRule (not a bespoke MCP-only +// concept) denies a gated write outright -- no park, no Activity/audit +// ceremony a plain policy deny never gets, and nothing written. +func TestGateWrite_DenyRuleFromNormalCRUD_BlocksTheWriteWithNoPark(t *testing.T) { + m, guard, comp := newGuardrailWiredHarness(t) + before := len(comp.Workflows()) + + if _, err := guard.CreateRule(guardrail.Rule{ + Label: "Block all MCP writes", Effect: guardrail.EffectDeny, NodeTypeID: mcpWriteGuardrailKind, + }); err != nil { + t.Fatalf("CreateRule: %v", err) + } + + res, err := m.gateWrite("import_workflow", "denied by rule", "{}") + if err == nil { + t.Fatalf("gateWrite() with a deny rule in place: want an error, got a result (res=%+v)", res) + } + if got := len(comp.Workflows()); got != before { + t.Errorf("workflow count = %d, want %d -- a denied write must write nothing", got, before) + } + if pending := m.PendingMCPWrites(); len(pending) != 0 { + t.Errorf("PendingMCPWrites() = %+v, want empty -- a deny-rule verdict must never park", pending) + } +} + +// TestGateWrite_AllowRuleFromNormalCRUD_SkipsTheParkAndExecutes proves +// the allow side of the same leverage: a matching allow rule executes +// the write immediately even though MCPWriteApprovalKey defaults to +// required, since the rule now short-circuits the ask-every-time +// default that governed every MCP write before this rebase. +func TestGateWrite_AllowRuleFromNormalCRUD_SkipsTheParkAndExecutes(t *testing.T) { + m, guard, comp := newGuardrailWiredHarness(t) + + wf, err := comp.CreateWorkflow("Allow-rule source workflow", "", + []composition.Node{{ID: "t", NodeTypeID: "trigger-manual"}, {ID: "c", NodeTypeID: "capture-clipboard-html"}}, + []composition.Edge{{ID: "e1", Source: "t", Target: "c"}}) + if err != nil { + t.Fatalf("CreateWorkflow: %v", err) + } + exported, err := comp.ExportWorkflow(wf.ID) + if err != nil { + t.Fatalf("ExportWorkflow: %v", err) + } + before := len(comp.Workflows()) + + if _, err := guard.CreateRule(guardrail.Rule{ + Label: "Allow all MCP writes", Effect: guardrail.EffectAllow, NodeTypeID: mcpWriteGuardrailKind, + }); err != nil { + t.Fatalf("CreateRule: %v", err) + } + + argsJSON, err := marshalArgs(importToolArgs{JSON: stripJSONIDField(t, exported)}) + if err != nil { + t.Fatalf("marshalArgs: %v", err) + } + res, err := m.gateWrite("import_workflow", "allowed by rule", argsJSON) + if err != nil { + t.Fatalf("gateWrite() with an allow rule in place: %v", err) + } + if res == nil || res.IsError { + t.Fatalf("gateWrite() result = %+v, want a successful (non-error) result", res) + } + if got := len(comp.Workflows()); got != before+1 { + t.Errorf("workflow count = %d, want %d -- an allow-rule verdict must execute immediately, no park needed", got, before+1) + } + if pending := m.PendingMCPWrites(); len(pending) != 0 { + t.Errorf("PendingMCPWrites() = %+v, want empty -- an allow-rule verdict must never park", pending) + } +} diff --git a/internal/services/wiring/wiring.go b/internal/services/wiring/wiring.go index f01e8560c..9ea30900d 100644 --- a/internal/services/wiring/wiring.go +++ b/internal/services/wiring/wiring.go @@ -378,3 +378,21 @@ func publishSystemEventNotification(notif *notificationsvc.NotificationService, slog.Warn("publish system-event notification", "event", ev.Event, "error", err) } } + +// WireMillMCPService late-binds every cross-service seam MillMCPService +// needs (docs/adr/0047 §5.4's guardrail wiring included) and starts its +// HTTP listener -- a bind failure is logged, not fatal, since this is +// additive local tooling the rest of the app doesn't depend on to +// function. +func WireMillMCPService(mill *mcpsvc.MillMCPService, settingsService *settingssvc.SettingsService, exec *executionsvc.ExecutionService, atlas *atlassvc.AtlasService, audit *mcpauditsvc.MCPAuditService, guard *guardrailsvc.GuardrailService, addr string, logger *slog.Logger) { + settingsService.SetMCPService(mill) + mill.SetExecutionService(exec) + mill.SetAtlasService(atlas) + mill.SetAuditResolver(audit.ResolveParkedWrite) + mill.SetGuardrailService(guard) + if err := mill.Start(addr); err != nil { + logger.Error("mill MCP server", "error", err) + } else { + logger.Info("mill MCP server listening", "addr", addr) + } +} diff --git a/main.go b/main.go index 5ab4515f6..fc9bfcca6 100644 --- a/main.go +++ b/main.go @@ -291,15 +291,7 @@ func main() { // something the rest of the app depends on to function. millMCPAddr, _ := settingssvc.ResolveMCPAddr(os.Getenv("MILL_MCP_ADDR"), settingsService.MCPAccessAddress()) millMCPService := mcpsvc.NewMillMCPService(millVersion, compositionService, configureService, settingsStore, userdocsFS, mcpAuditService.ServerMiddleware()) - settingsService.SetMCPService(millMCPService) - millMCPService.SetExecutionService(executionService) - millMCPService.SetAtlasService(atlasService) - millMCPService.SetAuditResolver(mcpAuditService.ResolveParkedWrite) - if err := millMCPService.Start(millMCPAddr); err != nil { - logger.Error("mill MCP server", "error", err) - } else { - logger.Info("mill MCP server listening", "addr", millMCPAddr) - } + wiring.WireMillMCPService(millMCPService, settingsService, executionService, atlasService, mcpAuditService, guardrailService, millMCPAddr, logger) agentLoopService := agentloopsvc.NewAgentLoopService(millMCPService) // an MCP client of it, ADR-0035