fix(guardrails): reorder PolicyDecision by severity; block-before-redact

Addresses initializ-mk's review on #221.

## Enum ordinal now maps to severity (was: latent aggregate bug)

Pre-fix constant order was Allow=0, Deny=1, Modify=2, so
`partA.Decision > partB.Decision` in CheckOutbound's aggregate
escalation was actually treating Modify as MORE severe than Deny —
opposite of the "Allow < Modify < Deny" comment. Worked only
because Deny short-circuits via the error return path.

Reordered to Allow < Modify < StepUp < Defer < Deny so the
ordinal comparison encodes restrictiveness. String() output and
audit-wire strings are unchanged (SIEM-stable). CheckOutbound's
aggregate escalation now correctly ranks any decision.

New TestPolicyDecision_SeverityOrdering pins the contract so a
future refactor can't silently break it. Also pins DecisionAllow
as the zero value so `PolicyResult{}` still implicitly means Allow.

## Block-before-redact in applyOutputPolicy (was: silent Deny→Modify)

Pre-fix, block detection ran against the progressively-redacted
string. A `redact` rule listed before a `block` rule could rewrite
the substring the block pattern would have caught, silently
downgrading Deny to Modify.

Post-fix, two-pass evaluation: pass 1 checks every block pattern
against the ORIGINAL content and short-circuits on match; pass 2
applies redacts cumulatively. Regression test
TestApplyOutputPolicy_RedactDoesNotHideEarlierBlock proves the
downgrade is closed.

## Docs + CHANGELOG (was: undocumented behavior change)

Dropping the cli_execute short-circuit is backward-incompatible:
existing deny_commands patterns now evaluate against the raw JSON
of every tool call. Added:

- CHANGELOG.md "Changed (potentially breaking)" section under
  Unreleased with migration guidance.
- docs/security/policy-decisions.md — new "Behavior change: match
  target asymmetry" table (cli_execute → reconstructed command
  line; other tools → raw JSON) and a "Block/redact ordering"
  section documenting the two-pass evaluation.

## Aggregate comment correction

CheckOutbound's docstring said aggregate.Modified "reflects the
last modified part" but strict `>` keeps the FIRST modified part
at each severity level. Docstring updated to match code.

## Dead code cleanup

Six pre-existing `result = nil` lines on gate-error paths where
the function immediately returns — carried over from an earlier
refactor. Removed while I'm here.

Author: naveen-kurra

CHANGELOG.md

@@ -2,6 +2,31 @@
 
 ## Unreleased
 
+### Changed (potentially breaking)
+
+- **Guardrail MODIFY / DENY now apply to every tool, not just
+  `cli_execute` (#209 / governance R4a).** Pre-#209 the
+  `SkillGuardrailEngine` short-circuited on any tool whose name
+  wasn't `cli_execute`, so `deny_commands` / `deny_output` patterns
+  silently no-op'd for `web_search`, `http_request`, MCP calls, and
+  every custom tool. The short-circuit is removed.
+  - **Match-target asymmetry to be aware of**: for `cli_execute`
+    the match target is still the reconstructed shell command line
+    (`binary arg1 arg2 …`) so existing shell-style patterns keep
+    working. For every other tool the match target is the raw
+    tool-input JSON as the LLM produced it. See
+    `docs/security/policy-decisions.md` for migration guidance.
+  - **`GuardrailChecker.CheckInbound` / `CheckOutbound` signatures
+    change** to return `(PolicyResult, error)` — the new
+    `PolicyDecision` enum (`Allow` < `Modify` < `StepUp` < `Defer`
+    < `Deny`, ordered by restrictiveness) is the R4 taxonomy from
+    the governance framework. `StepUp`/`Defer` are reserved for R4b
+    (#210) / R4c (#211) and not yet emitted; callers today still
+    read only Allow / Modify / Deny.
+  - Every `guardrail_check` audit event already carried a
+    `fields.decision` string; nothing on the audit wire shape
+    changes.
+
 ### Added
 
 - **Anthropic-format custom URLs + AWS Bedrock SigV4 outbound auth

docs/security/policy-decisions.md

@@ -31,6 +31,51 @@ removes the `cli_execute` short-circuit so:
   `applyOutputPolicy` so future call sites (MCP tool result hook,
   RAG context ingestion) can reuse the same MODIFY semantics.
 
+### ⚠️ Behavior change: match target asymmetry
+
+`deny_commands` patterns match against **different target strings**
+depending on the tool:
+
+| Tool             | Match target                                            |
+|------------------|---------------------------------------------------------|
+| `cli_execute`    | Reconstructed shell command line: `binary arg1 arg2 …`  |
+| any other tool   | The raw tool-input JSON verbatim                        |
+
+For `cli_execute` this preserves the pre-#209 semantics — patterns
+authored as shell-style regexes (`\bget\s+secrets?\b`, `rm\s+-rf`)
+still fire the same way against the parsed `binary`+`args`.
+
+For every other tool, the match runs against the JSON payload as
+the LLM produced it. A pattern like `"query":"kubectl get secrets"`
+will match a `web_search` invocation whose LLM-provided arguments
+happen to contain that substring; a pattern like `rm\s+-rf` will
+NOT (there's no reconstructed command line for `web_search`).
+
+**Migration**: operators upgrading a pre-#209 skill should audit
+their `deny_commands` list. A permissive shell-style pattern that
+previously affected only `cli_execute` may now start denying
+`web_search` / `http_request` / MCP calls whose JSON body happens
+to contain that text. Split rules that were meant for one tool
+family into per-tool patterns anchored on JSON structure (e.g.
+`"binary":"kubectl"` for cli_execute-only matches) if the wider
+scope is undesirable.
+
+### Block/redact ordering
+
+`applyOutputPolicy` evaluates `deny_output` filters in two passes:
+
+1. **Block pass** — every `block` pattern is checked against the
+   **original** content. A single match short-circuits the whole
+   call to `Deny`.
+2. **Redact pass** — every `redact` pattern is applied cumulatively
+   to the (possibly already-partially-redacted) content, returning
+   `Modify` if any substitution happened.
+
+The two-pass design guarantees an earlier `redact` cannot suppress a
+later `block` match by rewriting the substring the block pattern
+would have caught — a `Deny`-worthy string is never silently
+downgraded to `Modify`.
+
 ## Audit event mapping
 
 Every `guardrail_check` event carries a `fields.decision` string:

forge-cli/runtime/guardrails_engine.go

@@ -189,7 +189,6 @@ func (e *LibraryGuardrailEngine) CheckInbound(ctx context.Context, msg *a2a.Mess
 	})
 	if err != nil {
 		e.logger.Warn("guardrail input gate error", map[string]any{"error": err.Error()})
-		result = nil
 		return coreruntime.Allow(), nil
 	}
 
@@ -228,9 +227,14 @@ func (e *LibraryGuardrailEngine) CheckInbound(ctx context.Context, msg *a2a.Mess
 // only in enforce mode. One guardrail.output span per text part — the
 // trace tree mirrors the part-level iteration.
 //
-// The aggregated PolicyResult follows the most-severe part's outcome:
-// any Deny → Deny; else any Modify → Modify (Modified reflects the
-// last modified part's content); else Allow.
+// The aggregated PolicyResult follows the MOST-RESTRICTIVE part's
+// outcome per the PolicyDecision ordering (Allow < Modify < StepUp <
+// Defer < Deny). The strict `>` comparison keeps the FIRST part at
+// each severity level — subsequent equal-severity parts do not
+// overwrite Modified. In practice callers today only inspect the
+// mutated msg.Parts (each part carries its own redaction) — the
+// aggregate.Modified string is scaffolding for future R4b/R4c
+// callers that need a single-string projection.
 func (e *LibraryGuardrailEngine) CheckOutbound(ctx context.Context, msg *a2a.Message) (coreruntime.PolicyResult, error) {
 	aggregate := coreruntime.Allow()
 	for i, p := range msg.Parts {
@@ -243,8 +247,11 @@ func (e *LibraryGuardrailEngine) CheckOutbound(ctx context.Context, msg *a2a.Mes
 		if blockErr != nil {
 			return partResult, blockErr
 		}
-		// Escalate aggregate if this part is more severe than what
-		// we've seen so far. Allow < Modify < Deny.
+		// Escalate aggregate when this part is more restrictive. Uses
+		// the PolicyDecision ordinal-as-severity contract established
+		// in forge-core/runtime/guardrails.go: constants are ordered
+		// Allow < Modify < StepUp < Defer < Deny, so a numeric > is
+		// safe here even when StepUp/Defer flow through in future.
 		if partResult.Decision > aggregate.Decision {
 			aggregate = partResult
 		}
@@ -277,7 +284,6 @@ func (e *LibraryGuardrailEngine) checkOneOutboundPart(ctx context.Context, msg *
 	})
 	if err != nil {
 		e.logger.Warn("guardrail output gate error", map[string]any{"error": err.Error()})
-		result = nil
 		return coreruntime.Allow(), nil
 	}
 
@@ -339,7 +345,6 @@ func (e *LibraryGuardrailEngine) CheckToolCall(ctx context.Context, toolName, ar
 			"tool":  toolName,
 			"error": err.Error(),
 		})
-		result = nil
 		return args, nil
 	}
 
@@ -406,7 +411,6 @@ func (e *LibraryGuardrailEngine) CheckToolOutput(ctx context.Context, toolName,
 			"tool":  toolName,
 			"error": err.Error(),
 		})
-		result = nil
 		return text, nil
 	}
 
@@ -468,7 +472,6 @@ func (e *LibraryGuardrailEngine) CheckContext(ctx context.Context, content strin
 	})
 	if err != nil {
 		e.logger.Warn("guardrail context gate error", map[string]any{"error": err.Error()})
-		result = nil
 		return content, nil
 	}
 
@@ -529,7 +532,6 @@ func (e *LibraryGuardrailEngine) CheckStream(ctx context.Context, chunk string)
 	})
 	if err != nil {
 		e.logger.Warn("guardrail stream gate error", map[string]any{"error": err.Error()})
-		result = nil
 		return chunk, nil
 	}
 

forge-core/runtime/guardrails.go

@@ -12,16 +12,23 @@ import (
 // piece of content. Governance R4 requires the engine to be capable
 // of expressing each of these — even if a particular gate only
 // exercises a subset today. See docs/security/policy-decisions.md.
+//
+// Constants are ordered by RESTRICTIVENESS — the ordinal value maps
+// to severity so `partA.Decision > partB.Decision` selects the more
+// restrictive decision when aggregating across multiple parts:
+//
+//	Allow < Modify < StepUp < Defer < Deny
+//
+// Callers comparing severity SHOULD use the ordinal directly. Do NOT
+// reorder without updating every aggregate site (see
+// LibraryGuardrailEngine.CheckOutbound and its per-part escalation).
 type PolicyDecision int
 
 const (
 	// DecisionAllow — content passes through unmodified. Zero value.
+	// Least restrictive.
 	DecisionAllow PolicyDecision = iota
 
-	// DecisionDeny — content is rejected. Caller MUST propagate the
-	// error and MUST NOT let the content proceed.
-	DecisionDeny
-
 	// DecisionModify — content is admissible but must be rewritten
 	// (redacted, truncated, tagged) before it moves forward.
 	// PolicyResult.Modified carries the replacement.
@@ -36,6 +43,10 @@ const (
 	// (platform API, human queue) before the caller can proceed.
 	// Reserved for R4c (#211).
 	DecisionDefer
+
+	// DecisionDeny — content is rejected. Caller MUST propagate the
+	// error and MUST NOT let the content proceed. Most restrictive.
+	DecisionDeny
 )
 
 // String returns the audit-safe decision token. Matches the strings

forge-core/runtime/policy_decision_test.go

@@ -5,6 +5,32 @@ import (
 	"testing"
 )
 
+// TestPolicyDecision_SeverityOrdering pins the ordinal-as-severity
+// contract callers rely on (LibraryGuardrailEngine.CheckOutbound
+// uses `>` to escalate the aggregate result). Reordering these
+// constants without updating aggregate sites is exactly the class
+// of silent breakage this test guards against.
+func TestPolicyDecision_SeverityOrdering(t *testing.T) {
+	order := []PolicyDecision{
+		DecisionAllow,
+		DecisionModify,
+		DecisionStepUp,
+		DecisionDefer,
+		DecisionDeny,
+	}
+	for i := 1; i < len(order); i++ {
+		if order[i-1] >= order[i] {
+			t.Errorf("severity broken: %s(%d) should be < %s(%d)",
+				order[i-1], order[i-1], order[i], order[i])
+		}
+	}
+	// Also pin the zero-value expectation — deployments that
+	// return `PolicyResult{}` implicitly mean Allow.
+	if DecisionAllow != 0 {
+		t.Errorf("DecisionAllow must remain the zero value; got %d", DecisionAllow)
+	}
+}
+
 func TestPolicyDecision_StringMapping(t *testing.T) {
 	cases := []struct {
 		d    PolicyDecision
@@ -65,6 +91,31 @@ func TestApplyOutputPolicy_BlockShortCircuits(t *testing.T) {
 	}
 }
 
+// TestApplyOutputPolicy_RedactDoesNotHideEarlierBlock is a regression
+// test for reviewer initializ-mk's #221 point: a "redact" pattern
+// listed before a "block" pattern MUST NOT rewrite text in a way
+// that suppresses the block match. Pre-fix, the loop matched
+// blocks against the progressively-redacted string, so a
+// well-crafted redact-then-block config silently downgraded Deny
+// to Modify. Post-fix, block checks run against the ORIGINAL
+// content first (two-pass evaluation).
+func TestApplyOutputPolicy_RedactDoesNotHideEarlierBlock(t *testing.T) {
+	// The redact rule matches "secret " and rewrites it away.
+	// The block rule wants to match "secret key" — with the pre-fix
+	// single-pass loop it never fires because "secret " was already
+	// redacted to "[BLOCKED BY POLICY]". Post-fix, block runs first
+	// against original content and returns Deny.
+	filters := []compiledOutputFilter{
+		{re: regexp.MustCompile(`secret `), action: "redact"},
+		{re: regexp.MustCompile(`secret key`), action: "block"},
+	}
+	res := applyOutputPolicy("leaked secret key: abc", filters, &testLogger{}, "any_tool")
+	if res.Decision != DecisionDeny {
+		t.Errorf("block must beat earlier redact — got %s (modified=%q)",
+			res.Decision, res.Modified)
+	}
+}
+
 func TestApplyOutputPolicy_AllowWhenNoMatch(t *testing.T) {
 	filters := []compiledOutputFilter{{
 		re:     regexp.MustCompile(`nope`),

forge-core/runtime/skill_guardrails.go

@@ -173,35 +173,40 @@ func (s *SkillGuardrailEngine) CheckCommandOutput(toolName, toolOutput string) (
 // scanning, RAG-context scanning, MCP tool-result scanning) can share
 // the same MODIFY semantics.
 //
-// Precedence: any "block" match short-circuits the loop and returns
-// DecisionDeny. Otherwise, every "redact" match is applied
-// cumulatively and the aggregated redacted string is returned as
-// DecisionModify (if any pattern matched) or DecisionAllow (no matches).
+// Two-pass evaluation (fixes the redact-hides-block downgrade
+// reviewer initializ-mk flagged): pass 1 checks every "block" pattern
+// against the ORIGINAL content and short-circuits to Deny on any
+// match. Only after all blocks pass does pass 2 apply "redact"
+// substitutions cumulatively. This guarantees a block-worthy string
+// isn't silently downgraded to Modify by an earlier redact that
+// rewrote the substring the block pattern would have caught.
 //
 // `logger` and `tool` are for the redaction log line — pass "" for
 // tool when the caller isn't tool-scoped.
 func applyOutputPolicy(content string, filters []compiledOutputFilter, logger Logger, tool string) PolicyResult {
+	// Pass 1: block checks against ORIGINAL content.
+	for _, f := range filters {
+		if f.action == "block" && f.re.MatchString(content) {
+			return Deny("output matched deny_output block pattern")
+		}
+	}
+	// Pass 2: cumulative redacts.
 	modified := content
 	changed := false
 	for _, f := range filters {
-		if !f.re.MatchString(modified) {
+		if f.action != "redact" || !f.re.MatchString(modified) {
 			continue
 		}
-		switch f.action {
-		case "block":
-			return Deny("output matched deny_output block pattern")
-		case "redact":
-			modified = f.re.ReplaceAllString(modified, "[BLOCKED BY POLICY]")
-			changed = true
-			fields := map[string]any{
-				"pattern": f.re.String(),
-				"action":  "redact",
-			}
-			if tool != "" {
-				fields["tool"] = tool
-			}
-			logger.Warn("skill guardrail output redaction", fields)
+		modified = f.re.ReplaceAllString(modified, "[BLOCKED BY POLICY]")
+		changed = true
+		fields := map[string]any{
+			"pattern": f.re.String(),
+			"action":  "redact",
+		}
+		if tool != "" {
+			fields["tool"] = tool
 		}
+		logger.Warn("skill guardrail output redaction", fields)
 	}
 	if changed {
 		return Modify(modified, "output matched deny_output redact pattern")