feat(guardrails): generalize MODIFY policy decision beyond cli_execute

[naveen-kurra]

Closes #209. (Re-open of #219 which GitHub auto-closed after main was force-pushed; local branch rebased onto new main.)

Summary

• Introduce PolicyDecision (Allow / Deny / Modify / StepUp / Defer) + PolicyResult in forge-core/runtime — the governance R4a taxonomy.
• Migrate GuardrailChecker.CheckInbound / CheckOutbound to return (PolicyResult, error); noop + library engine + all 6 runner call sites updated.
• Drop the cli_execute-only short-circuit in SkillGuardrailEngine.CheckCommandInput / CheckCommandOutput. deny_commands now matches on the raw JSON of any tool; deny_output redact/block fires for output of ANY tool.
• Hoist 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.

Acceptance criteria (from #209)

• PolicyDecision + PolicyResult types introduced
• CheckInbound / CheckOutbound return PolicyResult
• MODIFY applies to LLM responses AND user prompts AND all tool outputs (not just cli_execute)
• Every guardrail audit event carries a decision field (unchanged — guardrail_check already emits decision)
• Tests: MODIFY case emitted for a non-cli_execute tool output

Test plan

• go test ./forge-core/... ./forge-cli/... — full sweep green
• gofmt -w + golangci-lint run all clean

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

[initializ-mk]

Reviewed the full diff and verified the critical path — the LibraryGuardrailEngine's in-place msg.Parts mutation still runs before the new return Modify(...), so masked content is still applied (no regression). This is a solid, well-tested refactor that closes a real gap: guardrail scanning of tool output was effectively cli_execute-scoped, and this generalizes MODIFY/DENY to every tool. No blocking correctness or security bug.

Requesting changes for a few things I'd like resolved before merge — none are large.

Docs / changelog (behavior change needs to be surfaced)

Dropping the cli_execute short-circuit is a backward-incompatible change: existing deny_commands patterns now evaluate against the raw JSON of every tool call, not just cli_execute command lines. A shell-oriented pattern (e.g. rm\s+-rf, or .*) that previously only affected cli_execute will now start denying web_search / http_request / MCP calls whose payload happens to contain that text. The test change makes it concrete: {"query":"kubectl get secrets"} flips from pass to blocked.

• Add a CHANGELOG.md entry calling out the behavior change (the PR doesn't touch it).
• In docs/security/policy-decisions.md, document that deny_commands now matches raw JSON for non-cli_execute tools, and note the asymmetry: deny_commands matches a reconstructed command line for cli_execute but raw JSON for every other tool — one rule list, two target shapes. Operators need this to calibrate patterns and avoid accidental matches on key names vs values / JSON escaping.

Should fix — enum ordering used as severity, but the enum isn't severity-ordered

CheckOutbound escalates with if partResult.Decision > aggregate.Decision, commenting "Allow < Modify < Deny." The constants are Allow=0, Deny=1, Modify=2, so numerically Modify > Deny — the opposite of the comment. It works today only because a Deny short-circuits via the error return (and warn-mode "deny" surfaces as Allow), so the > comparison only ever compares Allow vs Modify. It's latent: if StepUp/Defer or a non-erroring Deny ever flows through, the aggregate ranks them wrong.

• Don't overload enum order as severity — add an explicit severity helper (or reorder deliberately with a test), and fix the misleading comment.
• Nit in the same spot: the comment says the aggregate Modified "reflects the last modified part," but strict > keeps the first modified part. (No functional impact today since callers discard the result, but the comment should match the code.)

Should fix — redact can mask a later block pattern

In applyOutputPolicy, block detection matches against the progressively-redacted modified string, not the original. A redact filter earlier in the list that rewrites text a later block filter would have caught will suppress the block — silently downgrading a Deny to a Modify. This is pre-existing behavior, but it's now the canonical reusable helper, so it's the right moment to harden.

• Evaluate all block patterns against the original content before applying any redact (or run block checks first, redact second).
• Add a test for the ordering interaction (redact-before-block over overlapping content) — the current block+redact test doesn't hit it.

Minor

• The PolicyResult return values are discarded at all 6 runner call sites (_, err :=). MODIFY still works via in-place mutation, and the decision plumbing is forward-looking scaffolding for R4b/R4c — fine, but worth a one-line note that the returned decision isn't consumed by production callers yet (only unit-tested), so the new interface surface is a breaking change not yet exercised end-to-end.
• Pre-existing dead result = nil on the gate-error paths is carried along unchanged — cheap to drop while you're here.

What's good

Crypto/logic aside, the migration is complete and clean (all call sites, noop + library engine, tests threaded), and the tests that previously asserted the OPPOSITE (non-cli passes through) were correctly inverted with an explanatory comment. applyOutputPolicy is a sensible extraction for the upcoming MCP/RAG call sites. Once the docs/changelog and the two should-fix items land, this is good to merge.

[naveen-kurra]

Thanks for the review @initializ-mk — all four points addressed in b240550.

🟠 Enum ordinal as severity — reordered PolicyDecision to Allow < Modify < StepUp < Defer < Deny so the ordinal maps to restrictiveness. CheckOutbound's partResult.Decision > aggregate.Decision now correctly escalates when StepUp/Defer or non-erroring Deny flow through. Added TestPolicyDecision_SeverityOrdering to pin the contract + assert DecisionAllow == 0 so PolicyResult{} still implicitly means Allow. Audit-wire strings (allow/deny/modify/step_up/defer) unchanged, SIEM-stable.

🟠 applyOutputPolicy redact-hides-block — refactored to two-pass evaluation. Pass 1 runs every block pattern against the original content and short-circuits to Deny on match. Pass 2 applies redact substitutions cumulatively. Regression test TestApplyOutputPolicy_RedactDoesNotHideEarlierBlock constructs the exact "redact rewrites the substring a later block would have caught" scenario — passes now, would fail pre-fix.

Docs / CHANGELOG —

• New "Changed (potentially breaking)" section in CHANGELOG.md under Unreleased calling out the raw-JSON behavior change with migration guidance.
• New "Behavior change: match target asymmetry" section in docs/security/policy-decisions.md with a table (cli_execute → reconstructed command line; other tools → raw tool-input JSON) plus the "Block/redact ordering" description.

Aggregate comment correction — docstring on CheckOutbound said aggregate.Modified "reflects the last modified part" but strict > keeps the FIRST. Comment updated to match code; also documented that today's callers only inspect msg.Parts in-place, and aggregate.Modified is scaffolding for future R4b/R4c consumers.

🟡 Dead result = nil — six copies across the gate-error paths in guardrails_engine.go where the function immediately returns. Removed.

Full go test ./forge-core/... ./forge-cli/... sweep green, gofmt -w + golangci-lint run clean.

[initializ-mk]

Approving — the b240550f fix commit resolves everything from my earlier review, verified locally (full forge-core/runtime + forge-cli/runtime suites green on the branch).

• Enum severity ordering: fixed. Reordered to Allow(0) < Modify(1) < StepUp(2) < Defer(3) < Deny(4), so the ordinal encodes restrictiveness and CheckOutbound's > aggregate is correct for any decision (not just because Deny short-circuits). String() is name-based and the audit-wire strings are unchanged, so it stays SIEM-stable; DecisionAllow remains the zero value. TestPolicyDecision_SeverityOrdering pins the contract.
• Block-before-redact: fixed. applyOutputPolicy is now two-pass — every block pattern is checked against the ORIGINAL content first, then redacts apply cumulatively — so a redact can no longer downgrade a block to Modify. TestApplyOutputPolicy_RedactDoesNotHideEarlierBlock proves it. (Correct consequence: block now unconditionally precedes redact regardless of authoring order — documented.)
• Docs + CHANGELOG: added. CHANGELOG.md "Changed (potentially breaking)" with migration guidance and the match-target asymmetry; docs/security/policy-decisions.md with the cli_execute-vs-raw-JSON asymmetry table and the block/redact ordering section.
• Aggregate comment: corrected to "FIRST part at each severity level," matching the strict >.
• Dead-code cleanup: verified safe. The six result = nil removals sit before a deferred finishGuardrailSpan that early-returns on a nil result. I checked guardrails@v0.12.0: evaluateAtGate is fail-open and its reachable error returns are (nil, err), so result is already nil whenever the engine's err != nil branch runs — the removal is a genuine no-op.

No cross-PR collision here (unlike #220/#222), so this stands on its own.

Out-of-scope note for later, not a blocker for this PR: the guardrails engine itself is fail-open on internal errors (a policy-fetch failure yields Allow) — a pre-existing property of github.com/initializ/guardrails, unchanged here, but worth tracking separately if fail-closed is ever required.