feat(guard): error/warning decision model with has_failed_open() gate#146
feat(guard): error/warning decision model with has_failed_open() gate#146qw-in wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Arcjet Review — 🔴 High Risk
Decision: Reviewers Assigned
Rationale: This PR changes the guard decision/error model, including fail-open detection via has_failed_open(), interpretation of response-level server diagnostics as warnings, and deprecates has_error(). These are security-sensitive semantics because downstream applications may use these APIs to decide whether to allow or deny requests. The implementation is well tested and I did not identify a concrete vulnerability, but the behavioral/API impact warrants human review. No specific escalation reviewers are configured.
Summary of Changes
Adds a public Warning type, surfaces GuardResponse.errors as decision.warnings, introduces Decision.error_results() and Decision.has_failed_open(), updates missing-decision handling to synthesize a fail-open ALLOW with an error result, and adds unit/integration coverage for warnings, failed-open behavior, and has_error() deprecation.
Escalation Triggers
- Authentication & Authorization: The diff contains the configured content pattern "token" in TokenBucket-related guard code and tests. Although these are rate-limit tokens rather than auth credentials, the trigger fired and the guard decision/fail-open behavior is security-sensitive.
Review Focus Areas
- Confirm with the service/API contract that every top-level GuardResponse.errors entry is truly non-fatal and should be exposed only as decision.warnings without affecting conclusion or has_failed_open().
If any server error in this field can indicate degraded rule processing, treating it as informational could cause callers to allow traffic that should fail closed. - Verify that has_failed_open() should be defined strictly as conclusion == "ALLOW" plus at least one RuleResultError, and that there are no other degraded-decision states represented outside decision.results.
Applications may gate fail-closed behavior on this method, so missing an error source would create a security bypass. - Check release compatibility for emitting a DeprecationWarning from has_error(), especially for users running with warnings treated as errors.
Even though DeprecationWarning is usually ignored by default, some production/test environments promote warnings to failures, making this a potentially breaking behavioral change. - Confirm that exporting a class named Warning is intentional despite the name colliding with Python's built-in Warning hierarchy.
The public API name may confuse users or tooling; if intentional, documentation should make the distinction clear.
Notes
Security checklist applied: no hardcoded secrets, dependency changes, injection sinks, crypto changes, or credential logging were introduced in the diff. PR size is below the 1000-line automated review threshold.
Review: b9f8aad0 | Model: openai/gpt-5.5 | Powered by Arcjet Review
|
Admittedly fairly rushed as I have other things going on but wanted to get the ball rolling! |
There was a problem hiding this comment.
Pull request overview
This PR updates the Arcjet Python Guard decision surface to model warnings vs errors as separate severity axes, exposing request-validation diagnostics publicly while keeping errored rule results queryable and adding a has_failed_open() gate for fail-closed policies.
Changes:
- Adds
Decision.warnings,Decision.error_results(), andDecision.has_failed_open(); deprecatesDecision.has_error()(now emitsDeprecationWarning). - Converts
GuardResponse.errorsinto structuredWarningobjects (with safe coercion/fallbacks) and attaches them toDecision. - Extends unit + integration test coverage for warnings surfacing and failed-open semantics (transport error, missing decision, empty rules).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/guard/test_rules.py | Adds a regression test that Decision.has_error() emits a DeprecationWarning. |
| tests/unit/guard/test_convert.py | Adds coverage for warnings surfacing, error_results(), and has_failed_open() behaviors (including malformed fields). |
| tests/integration/guard/test_client.py | Extends sync/async integration assertions for warnings, error_results(), and has_failed_open() on fail-open paths. |
| src/arcjet/guard/_types.py | Introduces public Warning type; adds new Decision helpers; adds per-rule warnings fields and deprecates has_error(). |
| src/arcjet/guard/_convert.py | Implements _warnings_from_proto() and wires decision-level warnings into decision_from_proto(). |
| src/arcjet/guard/init.py | Exports Warning from arcjet.guard. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| code = 42 # type: ignore[assignment] | ||
| message = None # type: ignore[assignment] | ||
|
|
||
| warnings = _warnings_from_proto(cast("list[pb.ResultError]", [_BadError()])) |
| warnings: tuple[Warning, ...] = () | ||
| """Per-rule warnings — this rule was processed correctly (the result is | ||
| trustworthy) but something about it should be fixed. Informational; never | ||
| changes the rule's conclusion. Empty until the Decide service emits | ||
| per-rule diagnostics.""" |
| warnings: tuple[Warning, ...] = () | ||
| """Per-rule warnings — this rule was processed correctly (the result is | ||
| trustworthy) but something about it should be fixed. Informational; never | ||
| changes the rule's conclusion. Empty until the Decide service emits | ||
| per-rule diagnostics.""" |
| warnings: tuple[Warning, ...] = () | ||
| """Per-rule warnings — this rule was processed correctly (the result is | ||
| trustworthy) but something about it should be fixed. Informational; never | ||
| changes the rule's conclusion. Empty until the Decide service emits | ||
| per-rule diagnostics.""" |
Surface a two-axis error/warning model on Decision: severity (error vs warning) carried by channel, not inferred from placement. Matches the JS/Go SDKs. * Decision.warnings: decision-level diagnostics from request validation (proto GuardResponse.errors), surfaced publicly instead of only logged. Coerced to str at the SDK boundary (non-string code/message fall back to "UNKNOWN"/"Unknown warning") since the wire is untrusted. * Decision.error_results(): the RuleResultError entries (rules or the decision that could not be processed). * Decision.has_failed_open(): conclusion == ALLOW AND error_results() is non-empty — the fail-closed gate. * Decision.has_error(): deprecated (DeprecationWarning); now the conflated union of warnings + error_results. Kept working for one major cycle. * Per-rule result variants carry an empty `warnings` tuple for forward-compat (the Decide service does not yet emit per-rule diagnostics; no proto change is required to ship). * Warning exported from arcjet.guard. Behavior unchanged for runtime degradation: transport failures already return a fail-open ALLOW decision carrying a synthetic TRANSPORT_ERROR result, so has_failed_open()/error_results() see them. Encode/misconfig errors still raise ArcjetError (programmer errors surface, not fail open). The dropped _has_response_errors internal field is replaced by the public warnings tuple; the server-diagnostic logger.warning is removed since diagnostics are now a structured public surface. Tests: new TestWarningsAndFailedOpen covers warnings surfacing, allow+error is failed-open, deny+error is not, the warning/error severity independence, the missing-decision synthetic, non-string field coercion, and the empty case. Extended the transport-error and empty-rules client tests (sync + async) with has_failed_open()/ error_results()/warnings assertions. Pinned the has_error() DeprecationWarning. Co-Authored-By: Claude <noreply@anthropic.com>
485f87a to
340d9ac
Compare
Surface a two-axis error/warning model on Decision: severity (error vs warning) carried by channel, not inferred from placement. Matches the JS/Go SDKs.
warningstuple for forward-compat (the Decide service does not yet emit per-rule diagnostics; no proto change is required to ship).Behavior unchanged for runtime degradation: transport failures already return a fail-open ALLOW decision carrying a synthetic TRANSPORT_ERROR result, so has_failed_open()/error_results() see them. Encode/misconfig errors still raise ArcjetError (programmer errors surface, not fail open). The dropped _has_response_errors internal field is replaced by the public warnings tuple; the server-diagnostic logger.warning is removed since diagnostics are now a structured public surface.
Tests: new TestWarningsAndFailedOpen covers warnings surfacing, allow+error is failed-open, deny+error is not, the warning/error severity independence, the missing-decision synthetic, non-string field coercion, and the empty case. Extended the transport-error and empty-rules client tests (sync + async) with has_failed_open()/ error_results()/warnings assertions. Pinned the has_error() DeprecationWarning.