feat: Guardrail prerequisites — outbound deny recording + on_error policy#395
Conversation
Close a symmetry gap in session-event recording: inbound rejects in
the extproc listener were already emitted as SessionDenied events,
but outbound rejects in extproc, outbound rejects in forwardproxy,
and inbound rejects in reverseproxy all returned an HTTP error
without appending any session event.
Consequences: a guardrail plugin blocks an outbound MCP/inference
call → 403 lands on the agent, but /v1/sessions and abctl show
nothing. The observability surface operators use to understand what
was blocked and why is blind to exactly the events they care about
most.
Fix: add a reject-recording helper in each listener, called from
the reject path before the HTTP error is rendered. Symmetric shape
to the existing extproc recordInboundReject:
Phase: SessionDenied
Invocations: phase-filtered from pctx.Extensions.Invocations
Host: pctx.Host
StatusCode + Error: from action.Violation
Bucketing:
- extproc outbound: ActiveSession() → default
- forwardproxy: ActiveSession() → default
- reverseproxy: A2A.SessionID → ActiveSession() → default
(mirrors the accept path's fallback chain)
Skip rule: denials with no Invocations are not recorded — a
content-free SessionDenied event would be noise without
attribution, and non-plugin-originated denials (body-too-large,
etc.) belong in /stats counters, not the session stream.
Response-phase rejects (RunResponse denying after the upstream
returned) are out of scope — no in-tree plugin rejects in that
path today; wiring it adds listener complexity without a current
consumer.
Enables IBAC-style outbound guardrails to surface their blocks to
operators in abctl and /v1/sessions alongside the allow events.
Tests: 8 new tests across the three listeners covering the
happy-path recording, bucketing fallbacks, and skip-without-
invocations rule. All existing tests pass.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Per-plugin on_error knob that operators set in YAML to wrap the
plugin's behavior without changing the plugin code. Three values:
enforce (default) — Reject becomes the HTTP error, pipeline stops.
Existing behavior unchanged.
observe — plugin runs normally; a Reject return is
converted to Continue with Shadow=true on the
Invocation. Body mutations under observe are
also suppressed.
off — plugin is not dispatched at all. Kill-switch
without a redeploy.
Ships the rollout story for guardrail plugins: a new guardrail that
could false-positive (PII detection, prompt-injection scoring,
jailbreak classifiers) goes live in observe first, operators watch
the shadow-deny rate, then flip to enforce.
Scope of this commit:
pipeline/policy.go NEW — ErrorPolicy enum + Valid/Resolved
pipeline/pipeline.go WithPolicies option, policies slice on
Pipeline, off/observe dispatch in
Run/RunResponse, policyAt, markShadowAndLog
pipeline/context.go currentPolicy field, setCurrent/clearCurrent
(unexported; SetCurrentPlugin/ClearCurrent
kept for back-compat), SetBody/SetResponseBody
no-op under observe with Shadow=true
telemetry, recordShadowBodyMutation helper,
markLastInvocationShadow helper
pipeline/extensions.go Shadow bool on Invocation
config/config.go OnError field on PluginEntry, YAML decode,
valid-value check at parse time
plugins/registry.go Build skips off-policy entries, passes
policies through to pipeline.New via
WithPolicies; non-off plugins resolve
empty→enforce
Not in this commit (deliberately deferred from the parent
feat/plugin-on-error branch): sealing the built-in registry
(reservedBuiltinNames, RegisterBuiltin, IsReservedBuiltin,
ChainDirection, BuildChain, chain-placement validation, switching
built-ins to RegisterBuiltin). The seal is a different concern — it
prevents custom plugins from replacing or shadowing jwt-validation /
token-exchange — and can land separately without blocking the
rollout mechanism. on_error is the piece guardrail plugins need.
Under observe, the plugin's own code is unchanged: Reject intentions
are converted to Continue with Shadow=true on the Invocation record,
and body mutations are suppressed so the wire sees the original bytes.
Dashboards partition on Invocation.Shadow to count rollout candidates
(shadow=true) vs enforced outcomes (shadow=false).
Tests: 7 new pipeline tests (observe conversion, enforce default, off
skipping, response-phase observe, synthesized record when plugin
doesn't Record, SetBody observe-is-noop, WithPolicies overflow
rejection) + 2 new config tests (on_error YAML parse valid/invalid,
omitted-defaults-to-enforce). All existing tests pass.
Docs: plugin-reference.md gets an `on_error policy` section with the
policy table, observe semantics, shadow timeline query patterns, a
note that auth gates shouldn't be shadowed, and what on_error does
NOT do (not a circuit breaker, not sampling, not a timeout, not for
panics). plugin-tutorial.md gets a "Ship in observe mode first"
callout next to the Reject helper.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Five small fixes from review:
1. Promote filterInvocationsByPhase to (*Invocations).FilteredByPhase
in pipeline/. Forwardproxy and reverseproxy had byte-for-byte
duplicate helpers; now both call the method and the local helpers
are gone. Extproc's snapshotInvocations collapses to a one-line
delegation that preserves the snapshotXxx naming at call sites.
Aligned the semantics: strict phase match (was inconsistent -
forwardproxy/reverseproxy accepted empty Phase, extproc rejected).
Strict is correct: the framework always populates Phase via
Context.Record.
2. Synthesized shadow Invocation now carries action.Violation.Code
as Reason, so dashboards grouping denials by Reason see the
plugin's actual deny code across both recorded and synthesized
paths. The "synthesized" marker moves to Details["synthesized"]
= "true" so debugging can still distinguish "plugin Recorded
then Deny'd" from "plugin Deny'd without Recording". Test
updated to match.
3. Doc note in plugin-reference.md about NOT echoing matched
content into Violation.Reason: the field flows to the session
store (unauthenticated) and WARN logs (potentially different
retention). Leading example shows a GOOD vs BAD pair and points
at Details["match_sha256"] as the place to capture
fingerprints when debugging is needed.
4. Lead the on_error section with the naming caveat ("despite the
name, this handles intentional Deny, not panics/errors") so
operators don't reach for it for the wrong reason. The same
point was previously buried under "What on_error does NOT do" -
promoted to a blockquote at the top.
5. abctl actionCell appends "*" to the action string when
Invocation.Shadow is true (e.g., "deny*"). Operators scanning
the timeline can spot would-have-blocked rows at a glance.
Detail pane picks up Invocation.Shadow automatically via the
JSON marshal tag added in the parent commit. New test case
covers the shadow rendering.
Not addressed (observation-only in the review):
6. "Missing test: policy references plugin that doesn't exist."
The schema can't express this case: PluginEntry.OnError is
inline with the plugin entry in YAML, and WithPolicies takes
a positional slice indexed parallel to plugins. There's no
name-keyed policy map where a missing-plugin reference could
arise. Noted in reply thread rather than inventing a test for
a non-existent case.
Tests: existing pass; 1 new case added to events_pane_test.go for
the shadow action cell.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
|
Addressing review in #1 Reason content leaking to logs / session store — Added a "Don't put matched content in #2 Duplicate #3 Synthesized Invocation Reason obscures the deny code — Fixed. The synthesized #4 #5 abctl not updated to surface Shadow — #6 Missing test: policy references plugin that doesn't exist — No change; documenting why. The schema can't express this case:
If a future schema adds a name-keyed policy-override section (e.g., |
pdettori
left a comment
There was a problem hiding this comment.
Well-structured PR with two cleanly separable features bundled together. The deny-recording gap closure (commit 1) and the on_error policy (commit 2) are both well-implemented with thorough tests (17 new tests total) and clear documentation. The FilteredByPhase consolidation in commit 3 is a nice cleanup.
One blocking issue: a code comment claims auth plugins are validated against shadowing at startup, when that validation was intentionally deferred to the built-in registry sealing follow-up. The claim could mislead operators or auditors.
Areas reviewed: Go (pipeline, config, listeners, plugins, abctl), Docs
Commits: 3 commits, all signed-off
CI status: 16/16 passing (1 skip: Spellcheck)
ErrorPolicy godoc said "Reserved built-in gates are locked to
ErrorPolicyEnforce" and "plugins.BuildChain validates this at
startup." Both were leftovers from the parent feat/plugin-on-error
branch, which included a built-in-registry seal pass. That seal
was intentionally dropped when extracting the on_error piece for
this PR (see the Gap 3 commit body).
The stale claim misleads: an operator or auditor reading the godoc
would trust that on_error: observe on jwt-validation cannot be
configured, when in fact nothing prevents it today.
Rewrite the paragraph as advisory + explicit "not enforced at
startup" + pointer to the sealing follow-up. Matches the tone of
the §Applicability-to-auth-gates doc section, which was already
correctly advisory ("should stay on enforce").
No behavior change; godoc only.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
|
Addressed in // Shadowing or disabling auth gates (jwt-validation, token-exchange)
// is an authentication bypass dressed as a feature — operators SHOULD
// leave those plugins on ErrorPolicyEnforce (the default).
//
// The framework does NOT enforce this at startup: nothing prevents
// on_error: observe on jwt-validation today. A built-in-registry
// sealing pass is planned as a follow-up to reject a non-enforce
// policy on reserved gates at build time.Also verified the matching doc section in |
pdettori
left a comment
There was a problem hiding this comment.
Previous blocking issue (stale godoc claiming startup enforcement of auth-gate policies) is resolved — the fixup commit correctly rewrites the paragraph as advisory with an honest "not enforced at startup" statement.
The two features are well-implemented: deny-recording helpers are symmetric across all three listeners, the observe/enforce/off dispatch logic is clean with proper bounds checking, and body mutation suppression under observe is correct (both in-memory state and wire bytes preserved). 17 new tests cover happy paths, edge cases, and both request/response phases. Documentation is accurate and includes appropriate caveats.
Areas reviewed: Go (pipeline, config, listeners, plugins, abctl), Docs
Commits: 4 commits, all signed-off
CI status: 16/16 passing (1 skip: Spellcheck)
| // mode. Mirrors emitBodyMutation's telemetry (length + sha256 delta) | ||
| // so dashboards get the same shape they see under enforce, just with | ||
| // Shadow=true and no wire-level effect. | ||
| func (c *Context) recordShadowBodyMutation(phase string, oldBody, newBody []byte) { |
There was a problem hiding this comment.
nit: recordShadowBodyMutation fires once per SetBody call under observe. If a plugin calls SetBody in a loop (unlikely but possible), this creates N shadow invocations. Not a bug — just something to be aware of if shadow invocation counts look inflated during canary analysis.
Five small fixes from review:
1. Promote filterInvocationsByPhase to (*Invocations).FilteredByPhase
in pipeline/. Forwardproxy and reverseproxy had byte-for-byte
duplicate helpers; now both call the method and the local helpers
are gone. Extproc's snapshotInvocations collapses to a one-line
delegation that preserves the snapshotXxx naming at call sites.
Aligned the semantics: strict phase match (was inconsistent -
forwardproxy/reverseproxy accepted empty Phase, extproc rejected).
Strict is correct: the framework always populates Phase via
Context.Record.
2. Synthesized shadow Invocation now carries action.Violation.Code
as Reason, so dashboards grouping denials by Reason see the
plugin's actual deny code across both recorded and synthesized
paths. The "synthesized" marker moves to Details["synthesized"]
= "true" so debugging can still distinguish "plugin Recorded
then Deny'd" from "plugin Deny'd without Recording". Test
updated to match.
3. Doc note in plugin-reference.md about NOT echoing matched
content into Violation.Reason: the field flows to the session
store (unauthenticated) and WARN logs (potentially different
retention). Leading example shows a GOOD vs BAD pair and points
at Details["match_sha256"] as the place to capture
fingerprints when debugging is needed.
4. Lead the on_error section with the naming caveat ("despite the
name, this handles intentional Deny, not panics/errors") so
operators don't reach for it for the wrong reason. The same
point was previously buried under "What on_error does NOT do" -
promoted to a blockquote at the top.
5. abctl actionCell appends "*" to the action string when
Invocation.Shadow is true (e.g., "deny*"). Operators scanning
the timeline can spot would-have-blocked rows at a glance.
Detail pane picks up Invocation.Shadow automatically via the
JSON marshal tag added in the parent commit. New test case
covers the shadow rendering.
Not addressed (observation-only in the review):
6. "Missing test: policy references plugin that doesn't exist."
The schema can't express this case: PluginEntry.OnError is
inline with the plugin entry in YAML, and WithPolicies takes
a positional slice indexed parallel to plugins. There's no
name-keyed policy map where a missing-plugin reference could
arise. Noted in reply thread rather than inventing a test for
a non-existent case.
Tests: existing pass; 1 new case added to events_pane_test.go for
the shadow action cell.
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Summary
Two framework prerequisites for shipping guardrail plugins (IBAC, PII
scrubbers, jailbreak classifiers, etc.). Independent changes, bundled
because they land together in the same PR cycle:
feat(listeners):Record pipeline denials as SessionDenied events — close a symmetry gap where outbound rejects (extproc, forwardproxy) and inbound rejects (reverseproxy) didn't emit session events, so blocked calls never appeared in/v1/sessionsorabctl.feat(pipeline):Addon_errorpolicy (enforce | observe | off) — per-plugin knob letting operators canary a new guardrail in shadow mode before flipping to enforce. Plugin code is unchanged under all three modes; the rollout knob lives outside the plugin.Together these close the last two framework gaps identified in the pre-IBAC research pass. A third concern — cross-session intent contamination under concurrent multi-tenant traffic — is not in scope, because the current deployment model is single-tenant per pod (documented at
store.go:309). If we ever move off that model, correlation via a session header becomes the follow-up.Commit 1 — Outbound deny recording
extproclistener already hadrecordInboundRejecton inbound denials (emitted aSessionDeniedevent with Invocations / host / identity). The symmetric gaps:extprocoutbound rejects → no session eventforwardproxyoutbound rejects → no session eventreverseproxyinbound rejects → no session eventFixed with a local helper in each listener, called from the reject path before
rejectFromAction/writeRejectionreturns:Bucketing mirrors the accept-path recording in each listener:
ActiveSession()→ defaultA2A.SessionID→ActiveSession()→ defaultSkip rule: denials with no
Invocationsare not recorded — a content-free SessionDenied event would be noise without attribution, and non-plugin-originated denials (body-too-large, etc.) belong in/statscounters.Response-phase rejects (
RunResponsedenying after the upstream returned) are out of scope for this PR — no in-tree plugin rejects in that path today, and the wire-phase semantics are subtly different (the upstream already ran).Commit 2 —
on_errorpolicyThree values:
enforce(default)observeInvocation, request passesoffObserve mechanics
The plugin's
OnRequest/OnResponseruns exactly as underenforce. If it returnspipeline.Deny(...), the framework intercepts: it marks the plugin's lastInvocationwithShadow: true(or synthesizes one if the plugin didn't record), logs aWARN pipeline: plugin would have denied (shadow)line, and continues the pipeline. The request is not blocked.Body-mutation calls (
SetBody/SetResponseBody) underobserverecord aShadow: trueInvocation but do not alter the in-memory body or the wire bytes — downstream plugins and the upstream see the original.What this commit does NOT include
The parent branch
feat/plugin-on-erroralso sealed the built-in plugin registry (reservedBuiltinNames,RegisterBuiltin,IsReservedBuiltin,ChainDirection,BuildChain, built-ins switched toRegisterBuiltin, chain-placement validation). That's a different concern — preventing custom plugins from replacingjwt-validation/token-exchange— and can land separately without blocking the rollout mechanism guardrail plugins need today. We split it off so this PR is narrowly about on_error.If you want that seal later, it's a small follow-up PR on top of this one.
Dashboard queries
What
on_errordoes NOT doobservestill crashes on every request.observeruns the plugin on 100% of traffic.errorreturns / panics. Policy applies only to intentionalRejectactions; a panic inobservestill surfaces as a 500.Test plan
go build ./...andgo test -race ./...pass in both authlib and cmd/authbridge underGOWORK=offgofmt -lclean on all files touched by this PRFollow-ups (not in this PR)
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com