Merge pull request #221 from initializ/feat/gov-r4a-modify-generalization-v2

feat(guardrails): generalize MODIFY policy decision beyond cli_execute

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

@@ -0,0 +1,97 @@
+# Policy decisions
+
+Forge's guardrail engine can emit one of five decisions for any
+evaluated piece of content:
+
+| Decision  | Meaning                                                    | Where it fires today                                      |
+|-----------|------------------------------------------------------------|-----------------------------------------------------------|
+| `allow`   | Content passes through unmodified                          | Every gate, default                                       |
+| `deny`    | Content is rejected — the caller must not admit it         | InputGate, OutputGate, ToolCallGate, ToolOutputGate       |
+| `modify`  | Content is admissible but must be rewritten first          | InputGate (masking), OutputGate (masking), ToolOutputGate (redact) |
+| `step_up` | Reserved for R4b (#210) — additional user interaction     | not yet emitted                                           |
+| `defer`   | Reserved for R4c (#211) — out-of-band lookup required      | not yet emitted                                           |
+
+The `PolicyDecision` type and `PolicyResult` struct live in
+`forge-core/runtime/guardrails.go`. See the interface docstrings on
+`GuardrailChecker` for the exact contract.
+
+## MODIFY, generalized (#209)
+
+Before #209, `modify` was implemented for exactly one path —
+`cli_execute` output redaction. Skill-authored `deny_output` /
+`deny_commands` patterns silently no-op'd for every other tool. #209
+removes the `cli_execute` short-circuit so:
+
+- **User prompts** — the LibraryGuardrailEngine's InputGate mask
+  decision returns `DecisionModify` from `CheckInbound`.
+- **LLM responses** — same via OutputGate on `CheckOutbound`.
+- **Any tool output** — `SkillGuardrailEngine.CheckCommandOutput`
+  now applies the redact/block loop to output of ANY tool, not just
+  cli_execute. The loop is hoisted into a package-level
+  `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:
+
+- `allowed` — Allow
+- `masked` — Modify
+- `blocked` — Deny (enforce mode)
+- `warned` — Deny (warn mode)
+
+These strings predate the `PolicyDecision` enum by several sprints —
+they stay for SIEM stability. Consumers building on the enum should
+map `allowed ↔ allow`, `masked ↔ modify`, `blocked ↔ deny`.
+
+## Test surface
+
+- `forge-core/runtime/policy_decision_test.go` — enum semantics +
+  `applyOutputPolicy` behavior across tools.
+- `forge-core/runtime/skill_guardrails_test.go` — `deny_commands`
+  and `deny_output` MODIFY paths fire for non-cli_execute tools.

forge-cli/runtime/guardrails_engine.go

@@ -154,10 +154,13 @@ func (e *LibraryGuardrailEngine) structuredIfFileMode() *models.StructuredGuardr
 }
 
 // CheckInbound validates an inbound (user) message via InputGate.
-func (e *LibraryGuardrailEngine) CheckInbound(ctx context.Context, msg *a2a.Message) error {
+// The returned PolicyResult carries the engine's Allow/Deny/Modify
+// decision (see #209). On Modify, msg.Parts is mutated in place so
+// downstream reads see the redacted text.
+func (e *LibraryGuardrailEngine) CheckInbound(ctx context.Context, msg *a2a.Message) (coreruntime.PolicyResult, error) {
 	text := coreruntime.ExtractText(msg)
 	if text == "" {
-		return nil
+		return coreruntime.Allow(), nil
 	}
 
 	// Span lifecycle (issue #161): open before the library call so the
@@ -186,8 +189,7 @@ 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 nil
+		return coreruntime.Allow(), nil
 	}
 
 	switch result.Decision {
@@ -202,47 +204,66 @@ func (e *LibraryGuardrailEngine) CheckInbound(ctx context.Context, msg *a2a.Mess
 			e.emitGuardrailEvent(ctx, "", result.MaskedContent, guardrailResultMasked, result)
 			evidenceContent = result.MaskedContent
 			decisionString = guardrailResultMasked
+			return coreruntime.Modify(result.MaskedContent, violationSummary(result)), nil
 		}
 	case guardrails.DecisionBlock:
 		desc := violationSummary(result)
 		if e.enforce {
 			e.emitGuardrailEvent(ctx, "", text, guardrailResultBlocked, result)
 			evidenceContent = text
 			decisionString = guardrailResultBlocked
-			return fmt.Errorf("input blocked: %s", desc)
+			return coreruntime.Deny(desc), fmt.Errorf("input blocked: %s", desc)
 		}
 		e.logger.Warn("guardrail input violation (warn mode)", map[string]any{"detail": desc})
 		e.emitGuardrailEvent(ctx, "", text, guardrailResultWarned, result)
 		evidenceContent = text
 		decisionString = guardrailResultWarned
 	}
-	return nil
+	return coreruntime.Allow(), nil
 }
 
 // CheckOutbound validates an outbound (agent) message via OutputGate.
 // Masked content is applied in-place; blocked content returns an error
 // only in enforce mode. One guardrail.output span per text part — the
 // trace tree mirrors the part-level iteration.
-func (e *LibraryGuardrailEngine) CheckOutbound(ctx context.Context, msg *a2a.Message) error {
+//
+// 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 {
 		if p.Kind != a2a.PartKindText || p.Text == "" {
 			continue
 		}
 
 		original := p.Text
-		blockErr := e.checkOneOutboundPart(ctx, msg, i, original)
+		partResult, blockErr := e.checkOneOutboundPart(ctx, msg, i, original)
 		if blockErr != nil {
-			return blockErr
+			return partResult, blockErr
+		}
+		// 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
 		}
 	}
-	return nil
+	return aggregate, nil
 }
 
 // checkOneOutboundPart runs OutputGate over a single text part. Split
 // from CheckOutbound so the per-part span has a clean lifetime (open
 // at function entry, deferred close at exit) without juggling state
 // across iterations.
-func (e *LibraryGuardrailEngine) checkOneOutboundPart(ctx context.Context, msg *a2a.Message, i int, original string) error {
+func (e *LibraryGuardrailEngine) checkOneOutboundPart(ctx context.Context, msg *a2a.Message, i int, original string) (coreruntime.PolicyResult, error) {
 	ctx, span := startGuardrailSpan(ctx, "output", "")
 	var (
 		evidenceContent string
@@ -263,8 +284,7 @@ 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 nil
+		return coreruntime.Allow(), nil
 	}
 
 	switch result.Decision {
@@ -275,21 +295,22 @@ func (e *LibraryGuardrailEngine) checkOneOutboundPart(ctx context.Context, msg *
 			e.emitGuardrailEvent(ctx, "", result.MaskedContent, guardrailResultMasked, result)
 			evidenceContent = result.MaskedContent
 			decisionString = guardrailResultMasked
+			return coreruntime.Modify(result.MaskedContent, violationSummary(result)), nil
 		}
 	case guardrails.DecisionBlock:
 		desc := violationSummary(result)
 		if e.enforce {
 			e.emitGuardrailEvent(ctx, "", original, guardrailResultBlocked, result)
 			evidenceContent = original
 			decisionString = guardrailResultBlocked
-			return fmt.Errorf("output blocked: %s", desc)
+			return coreruntime.Deny(desc), fmt.Errorf("output blocked: %s", desc)
 		}
 		e.logger.Warn("guardrail output violation (warn mode)", map[string]any{"detail": desc})
 		e.emitGuardrailEvent(ctx, "", original, guardrailResultWarned, result)
 		evidenceContent = original
 		decisionString = guardrailResultWarned
 	}
-	return nil
+	return coreruntime.Allow(), nil
 }
 
 // CheckToolCall validates the arguments the agent is about to pass to
@@ -324,7 +345,6 @@ func (e *LibraryGuardrailEngine) CheckToolCall(ctx context.Context, toolName, ar
 			"tool":  toolName,
 			"error": err.Error(),
 		})
-		result = nil
 		return args, nil
 	}
 
@@ -391,7 +411,6 @@ func (e *LibraryGuardrailEngine) CheckToolOutput(ctx context.Context, toolName,
 			"tool":  toolName,
 			"error": err.Error(),
 		})
-		result = nil
 		return text, nil
 	}
 
@@ -453,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
 	}
 
@@ -514,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-cli/runtime/guardrails_engine_test.go

@@ -49,13 +49,13 @@ func TestFileGuardrailEngine_CheckInbound(t *testing.T) {
 		Parts: []a2a.Part{{Kind: a2a.PartKindText, Text: "Hello, how are you?"}},
 	}
 	ctx := context.Background()
-	if err := engine.CheckInbound(ctx, msg); err != nil {
+	if _, err := engine.CheckInbound(ctx, msg); err != nil {
 		t.Errorf("normal message should pass inbound check: %v", err)
 	}
 
 	// Empty message should pass
 	emptyMsg := &a2a.Message{Role: "user"}
-	if err := engine.CheckInbound(ctx, emptyMsg); err != nil {
+	if _, err := engine.CheckInbound(ctx, emptyMsg); err != nil {
 		t.Errorf("empty message should pass inbound check: %v", err)
 	}
 }
@@ -73,7 +73,7 @@ func TestFileGuardrailEngine_CheckOutbound(t *testing.T) {
 		Role:  "agent",
 		Parts: []a2a.Part{{Kind: a2a.PartKindText, Text: "Here is the result."}},
 	}
-	if err := engine.CheckOutbound(context.Background(), msg); err != nil {
+	if _, err := engine.CheckOutbound(context.Background(), msg); err != nil {
 		t.Errorf("normal message should pass outbound check: %v", err)
 	}
 }
@@ -122,7 +122,7 @@ func TestBuildGuardrailChecker_FileMode(t *testing.T) {
 		Role:  "user",
 		Parts: []a2a.Part{{Kind: a2a.PartKindText, Text: "hello"}},
 	}
-	if err := checker.CheckInbound(context.Background(), msg); err != nil {
+	if _, err := checker.CheckInbound(context.Background(), msg); err != nil {
 		t.Errorf("default checker should pass normal message: %v", err)
 	}
 }
@@ -148,7 +148,7 @@ func TestLibraryGuardrailEngine_EmitsAuditOnInboundMask(t *testing.T) {
 		Role:  "user",
 		Parts: []a2a.Part{{Kind: a2a.PartKindText, Text: "my email is foo@example.com please verify"}},
 	}
-	if err := engine.CheckInbound(context.Background(), msg); err != nil {
+	if _, err := engine.CheckInbound(context.Background(), msg); err != nil {
 		t.Fatalf("CheckInbound: %v", err)
 	}
 
@@ -264,7 +264,7 @@ func TestLibraryGuardrailEngine_OmitsEvidenceByDefault(t *testing.T) {
 		Role:  "user",
 		Parts: []a2a.Part{{Kind: a2a.PartKindText, Text: "my email is foo@example.com"}},
 	}
-	if err := engine.CheckInbound(context.Background(), msg); err != nil {
+	if _, err := engine.CheckInbound(context.Background(), msg); err != nil {
 		t.Fatalf("CheckInbound: %v", err)
 	}
 

forge-cli/runtime/guardrails_tracing_test.go

@@ -67,7 +67,7 @@ func TestCheckInbound_OpensInputSpanWithGateAttributes(t *testing.T) {
 		Role:  "user",
 		Parts: []a2a.Part{{Kind: a2a.PartKindText, Text: "my email is foo@example.com"}},
 	}
-	if err := engine.CheckInbound(context.Background(), msg); err != nil {
+	if _, err := engine.CheckInbound(context.Background(), msg); err != nil {
 		t.Fatalf("CheckInbound: %v", err)
 	}
 
@@ -102,7 +102,7 @@ func TestCheckInbound_CaptureContent_StampsRedactedEvidence(t *testing.T) {
 		Role:  "user",
 		Parts: []a2a.Part{{Kind: a2a.PartKindText, Text: "my email is foo@example.com"}},
 	}
-	if err := engine.CheckInbound(context.Background(), msg); err != nil {
+	if _, err := engine.CheckInbound(context.Background(), msg); err != nil {
 		t.Fatalf("CheckInbound: %v", err)
 	}
 
@@ -152,7 +152,7 @@ func TestCheckOutbound_OpensOutputSpan_NoToolAttribute(t *testing.T) {
 		Role:  "agent",
 		Parts: []a2a.Part{{Kind: a2a.PartKindText, Text: "Here is your answer."}},
 	}
-	if err := engine.CheckOutbound(context.Background(), msg); err != nil {
+	if _, err := engine.CheckOutbound(context.Background(), msg); err != nil {
 		t.Fatalf("CheckOutbound: %v", err)
 	}
 
@@ -212,7 +212,7 @@ func TestCheckInbound_NoTracing_NoSpansRecorded(t *testing.T) {
 		Role:  "user",
 		Parts: []a2a.Part{{Kind: a2a.PartKindText, Text: "hello"}},
 	}
-	if err := engine.CheckInbound(context.Background(), msg); err != nil {
+	if _, err := engine.CheckInbound(context.Background(), msg); err != nil {
 		t.Fatalf("CheckInbound: %v", err)
 	}