feat: MCP writes judged through the shared guardrail rule core (ADR-0047 §5.4) - #510
Merged
Merged
Conversation
…047 §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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
EvaluateAction— the same rule-evaluation coreRequestGuardedAction(feat: the guardrail gains its public request-an-action entry (ADR-0047 §5) #508) and the workflow execution gate already share — instead of a flat enabled/required settings toggle with no rule concept at all. A rule authored via the ordinary Configure > Guardrail CRUD, scoped to kindmcp-write, now governs MCP writes: an explicit allow rule executes immediately with no park, an explicit deny rule blocks outright with no park, and the ask default (no matching rule, or no guardrail service wired) falls straight through to the existing durable park/poll mechanism, unchanged.MillMCPService.SetGuardrailServiceis a new late-bound setter (same shape asSetAuditResolver/SetExecutionService), nil-safe — no existing test construction needed to change.wiring.WireMillMCPServiceextracts the mcpsvc construction wiring out ofmain.goto stay under the 500-line cap.millmcpservice_guardrail_test.go) prove the leverage end-to-end: a deny rule blocks a write with zero park, an allow rule executes a write with zero park — both authored throughGuardrailService.CreateRule, the same CRUD a human uses.STOP — full mechanism deletion did not happen, and here's the concrete blocker
The brief asked for the MCP write path to call
RequestGuardedActioninstead of its bespoke park mechanism, deletingMCPWriteRecordonce parity is proven. I traced this against the actual test suite before attempting it and found a structural conflict:TestGateWrite_PersistFailureAtParkTime_ReturnsErrorAndLeavesNoRecordandTestResolveMCPWrite_PersistFailure_StillDeniesButReturnsError(millmcpservice_approval_test.go) constructMillMCPServicewith no guardrail dependency at all and assert directly on this package's ownstore.Setfailure path at park time.NewMillMCPService's constructor signature can't gain a required parameter without touching every test file in the package that constructs it (a dozen+), which the brief's own "every existing MCP-write test passes UNMODIFIED" bar forbids.RequestGuardedAction/PendingGuardedAction(guardrailsvc) are in-memory only — no durable persistence, no resolved-state retention window, no apply-on-approve payload, and its ownRequestGuardedActionunparks (deletes) the record the instant the blocking call returns, which is incompatible withMCPWriteRecord's restart-survival + 24h resolved-retention guarantees thatTestMCPWriteTools_RestartSurvival_PendingRecordSurvivesNewServiceInstanceandResolvedMCPWrites/check_write_statusdepend on.PendingGuardedActioninto a durable, applier-backed, retention-aware store from scratch and re-verifying every restart/courtesy-window/at-most-once-execution edge case this approval gate depends on for correctness — real risk of a double-execution or durability regression in security-critical code, not something to force through unreviewed.What I shipped instead: the actual payoff named in ADR-0047 §5.4 ("every agent action... flows through the one entry and the one rule plane") — MCP writes are judged by
EvaluateAction, the exact coreRequestGuardedActionitself calls first before ever parking. The durable park staysmcpsvc's ownMCPWriteRecord, called out explicitly in both the new file's header comment and the commit message as a named follow-up, not a silent gap.Unblocking full deletion would need, as its own reviewed slice:
PendingGuardedActiongaining durable persistence + a registered apply-on-approve payload + resolved-state retention, and a decision on how the two persist-failure tests migrate (their assertions would need to move to guardrailsvc, which is an "assertion change" the brief said to stop and report on rather than force through).Test plan
go test ./internal/services/mcpsvc/... ./internal/services/guardrailsvc/... ./internal/services/wiring/...— all pass, zero existing assertions touchedgo test ./internal/...— all green except a pre-existing, already-quarantined real-pasteboard flake (QUARANTINE.mdline 52,internal/adapters/clipboard, unrelated package, not touched by this change)golangci-lint run ./...,go vet ./...,scripts/check-loc.sh,scripts/check-comment-hygiene.sh— all cleanMillMCPServiceisn't a registered Wails Service — its RPCs live onSettingsService, unchanged) — no bindings regen needed🤖 Generated with Claude Code
https://claude.ai/code/session_012im1JxQQV2ahnXzZDdVmZq