feat: MCP writes judged through the shared guardrail rule core (ADR-0047 §5.4)

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

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,

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
+}

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)
+	}
+}

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)
+	}
+}

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