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

A gated MCP write's approve/deny/ask verdict now comes from
EvaluateAction -- the same rule-evaluation core RequestGuardedAction and
the workflow execution gate already share -- instead of a flat
enabled/required settings toggle with no rule concept at all. A user
authoring a guardrail rule scoped to kind "mcp-write" through the
ordinary Configure > Guardrail CRUD now governs MCP writes: an explicit
allow rule executes immediately with no park, an explicit deny rule
blocks outright, and the "ask" default (no matching rule, or no
guardrail service wired) falls straight through to the existing durable
park/poll mechanism unchanged.

The durable park itself (MCPWriteRecord, its restart-survival, courtesy
window, sweep/expiry, audit, and Activity emission) stays this package's
own -- unifying it onto guardrailsvc's PendingGuardedAction would need
that store to gain durable persistence and an apply-on-approve payload,
which several existing tests here (constructed with no guardrail
dependency at all, asserting directly on this package's own
store-persistence failure paths) assume it never does. Full mechanism
deletion is a follow-up, not folded into this slice.

Wiring: SetGuardrailService is a new late-bound setter (same shape as
SetAuditResolver/SetExecutionService), nil-safe so no existing test
construction changes. The gateWrite-adjacent wiring call in main.go was
extracted into wiring.WireMillMCPService to stay under the 500-line
file cap.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012im1JxQQV2ahnXzZDdVmZq

Author: alicoding

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