feat(guardrails): generalize MODIFY policy decision beyond cli_execute (closes #209)

Governance R4a requires MODIFY to be a first-class policy decision
available to any evaluated content — not a special-cased
cli_execute-only path. This change:

- Introduces PolicyDecision (Allow/Deny/Modify/StepUp/Defer) +
  PolicyResult in forge-core/runtime. StepUp and Defer are reserved
  for R4b/R4c (#210/#211) and not yet emitted.
- Migrates GuardrailChecker.CheckInbound / CheckOutbound to return
  (PolicyResult, error). NoopGuardrailChecker + LibraryGuardrail
  Engine + all runner call sites updated.
- Drops the `if toolName != "cli_execute"` short-circuits in
  SkillGuardrailEngine.CheckCommandInput / CheckCommandOutput.
  deny_commands now matches on the raw JSON of non-cli tools;
  deny_output redact/block fires for output of ANY tool.
- Hoists the redact/block loop into a package-level
  applyOutputPolicy so future call sites (MCP tool result hook, RAG
  ingest) can reuse the same MODIFY semantics.

Docs at docs/security/policy-decisions.md.

Author: naveen-kurra

docs/security/policy-decisions.md

@@ -0,0 +1,52 @@
+# 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.
+
+## 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
@@ -187,7 +190,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 +205,58 @@ 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-severe part's outcome:
+// any Deny → Deny; else any Modify → Modify (Modified reflects the
+// last modified part's content); else Allow.
+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 if this part is more severe than what
+		// we've seen so far. Allow < Modify < Deny.
+		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
@@ -264,7 +278,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 +289,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

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

forge-cli/runtime/runner.go

@@ -1270,7 +1270,7 @@ func (r *Runner) registerHandlers(srv *server.Server, executor coreruntime.Agent
 		server.WriteSSEEvent(w, flusher, "status", task) //nolint:errcheck
 
 		// Guardrail check inbound
-		if err := guardrails.CheckInbound(ctx, &params.Message); err != nil {
+		if _, err := guardrails.CheckInbound(ctx, &params.Message); err != nil {
 			task.Status = a2a.TaskStatus{
 				State: a2a.TaskStateFailed,
 				Message: &a2a.Message{
@@ -1340,7 +1340,7 @@ func (r *Runner) registerHandlers(srv *server.Server, executor coreruntime.Agent
 		var finalState a2a.TaskState
 		for respMsg := range ch {
 			// Guardrail check outbound
-			if grErr := guardrails.CheckOutbound(ctx, respMsg); grErr != nil {
+			if _, grErr := guardrails.CheckOutbound(ctx, respMsg); grErr != nil {
 				task.Status = a2a.TaskStatus{
 					State: a2a.TaskStateFailed,
 					Message: &a2a.Message{
@@ -1511,7 +1511,7 @@ func (r *Runner) executeTask(
 		auditLogger.EmitInvocationComplete(ctx, snap.InvocationDuration, fields)
 	}
 
-	if err := guardrails.CheckInbound(ctx, &params.Message); err != nil {
+	if _, err := guardrails.CheckInbound(ctx, &params.Message); err != nil {
 		task.Status = a2a.TaskStatus{
 			State: a2a.TaskStateFailed,
 			Message: &a2a.Message{
@@ -1582,7 +1582,7 @@ func (r *Runner) executeTask(
 	}
 
 	if respMsg != nil {
-		if err := guardrails.CheckOutbound(ctx, respMsg); err != nil {
+		if _, err := guardrails.CheckOutbound(ctx, respMsg); err != nil {
 			task.Status = a2a.TaskStatus{
 				State: a2a.TaskStateFailed,
 				Message: &a2a.Message{
@@ -1781,7 +1781,7 @@ func (r *Runner) registerRESTHandlers(srv *server.Server, executor coreruntime.A
 		store.Put(task)
 		server.WriteSSEEvent(w, flusher, "status", task) //nolint:errcheck
 
-		if err := guardrails.CheckInbound(ctx, &params.Message); err != nil {
+		if _, err := guardrails.CheckInbound(ctx, &params.Message); err != nil {
 			task.Status = a2a.TaskStatus{
 				State: a2a.TaskStateFailed,
 				Message: &a2a.Message{
@@ -1845,7 +1845,7 @@ func (r *Runner) registerRESTHandlers(srv *server.Server, executor coreruntime.A
 
 		var finalState a2a.TaskState
 		for respMsg := range ch {
-			if grErr := guardrails.CheckOutbound(ctx, respMsg); grErr != nil {
+			if _, grErr := guardrails.CheckOutbound(ctx, respMsg); grErr != nil {
 				task.Status = a2a.TaskStatus{
 					State: a2a.TaskStateFailed,
 					Message: &a2a.Message{