Conversation
Proto changes: - Rename reset_seconds → reset_at_unix_seconds on ResultTokenBucket, ResultFixedWindow, and ResultSlidingWindow to clarify the field is a Unix timestamp, not a duration. Client changes: - Validate empty rules client-side in both ArcjetGuard.guard() and ArcjetGuardSync.guard(), returning a VALIDATION_ERROR decision instead of sending a pointless request to the server. - Fix decide_connect.py import path (proto.decide.v2 → arcjet.guard.proto.decide.v2). Tests: - Add empty rules validation tests for both sync and async clients.
There was a problem hiding this comment.
Pull request overview
Introduces a new arcjet.guard Python SDK surface (rules, typed results, proto conversion, and client) backed by Decide v2 protobufs, plus a comprehensive new guard-focused test suite.
Changes:
- Added
arcjet.guardcore modules: public types, rule factories, proto conversion, local WASM sensitive-info evaluation, and async/sync clients. - Added generated Decide v2 protobuf bindings under
src/arcjet/guard/proto/decide/v2/. - Added extensive unit + integration test coverage for factories, conversions, client behavior, and end-to-end “round trip” flows.
Reviewed changes
Copilot reviewed 17 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/arcjet/guard/types.py |
Defines public Decision/RuleResult* dataclasses for guard. |
src/arcjet/guard/rules.py |
Implements configured rule factories and *WithInput binding + Layer 3 inspection helpers. |
src/arcjet/guard/convert.py |
Converts between SDK rule inputs/results and Decide v2 protobufs. |
src/arcjet/guard/_local.py |
Local sensitive-info evaluation + hashing utilities used during submission building. |
src/arcjet/guard/client.py |
Async/sync clients + launch factories that call the Decide v2 Guard RPC. |
src/arcjet/guard/__init__.py |
Public re-exports for the new guard API. |
src/arcjet/guard/proto/decide/v2/decide_pb2.py |
Generated Decide v2 protobuf runtime code. |
src/arcjet/guard/proto/decide/v2/decide_pb2.pyi |
Generated type stubs for Decide v2 protobufs. |
src/arcjet/guard/proto/decide/v2/decide_connect.py |
Generated connect-python client/server bindings for Decide v2. |
src/arcjet/guard/proto/**/__init__.py |
Package markers for the new guard proto namespace. |
tests/unit/guard/conftest.py |
Guard test helpers + overrides unit-level protobuf stubbing fixture. |
tests/unit/guard/test_rules.py |
Unit tests for rule factories and Layer 3 inspection behavior. |
tests/unit/guard/test_convert.py |
Unit tests for rule/proto conversion and decision mapping. |
tests/unit/guard/test_local.py |
Unit tests for hashing + local sensitive-info WASM evaluation behavior. |
tests/integration/guard/test_e2e.py |
End-to-end simulated server “round trip” tests across multiple rule types. |
tests/integration/guard/test_client.py |
Integration tests for async/sync client pipelines and failure modes. |
tests/test_guard.py |
Additional top-level comprehensive guard tests (currently duplicative with unit tests). |
pyproject.toml |
Excludes new generated guard protobuf code from coverage/linters/type-checkers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def decision_from_proto( | ||
| response: pb.GuardResponse, | ||
| rules: Sequence[RuleWithInput], | ||
| ) -> Decision: | ||
| """Convert a proto ``GuardResponse`` to the SDK ``Decision``. | ||
|
|
||
| Correlates proto results back to SDK rule instances using | ||
| ``config_id`` and ``input_id``. | ||
| """ |
There was a problem hiding this comment.
decision_from_proto() takes a rules sequence but doesn’t use it (the implementation builds results purely from response.decision.rule_results). This makes the signature/documentation misleading and misses an opportunity to enforce the “submission order” guarantee by reordering/validating results against the submitted rules. Consider either using rules to validate/order the results (and to synthesize NOT_RUN/UNKNOWN for missing entries), or remove the parameter and update the docstring accordingly.
| def _make_error_decision(message: str) -> Decision: | ||
| return Decision( | ||
| conclusion="ALLOW", | ||
| id="", | ||
| results=(RuleResultError(message=message, code="TRANSPORT_ERROR"),), | ||
| reason="ERROR", | ||
| ) |
There was a problem hiding this comment.
Decision.results is documented as “one per submission”, but _make_error_decision() always returns a single RuleResultError regardless of how many rules were submitted. This breaks the Layer 3 lookup contract (and makes decision.results length inconsistent with successful calls). Consider synthesizing one error result per submitted rule (and populating _internal_results with each rule’s config_id/input_id), or update the Decision.results contract to explicitly allow a single non-correlated error result for transport failures.
| """Per-rule results, one per submission, in submission order.""" | ||
|
|
There was a problem hiding this comment.
Decision.results is described as “one per submission, in submission order”, but several call paths create decisions that don’t satisfy that (e.g., transport fail-open returns 1 result even if multiple rules were submitted, and the empty-rules validation error returns 1 result despite 0 submissions). Either adjust the SDK behavior to always produce one result per submitted rule (recommended for Layer 3 consistency), or relax/clarify this docstring to match the actual guarantees.
| """Per-rule results, one per submission, in submission order.""" | |
| """Rule results associated with this decision. | |
| In a successful evaluation with submitted rules, this will typically contain | |
| one ``RuleResult`` per submitted rule, in submission order. | |
| In certain error or fail-open scenarios, this 1:1 correspondence is not | |
| guaranteed. For example, transport fail-open or validation errors may | |
| produce a single synthetic error result for the entire decision, or a | |
| result even when no rules were submitted. | |
| """ |
| """Exhaustive tests for arcjet.guard — types, rules, convert.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| from arcjet.guard import ( | ||
| CustomWithInput, | ||
| FixedWindowWithInput, | ||
| PromptInjectionWithInput, | ||
| SensitiveInfoWithInput, | ||
| SlidingWindowWithInput, | ||
| TokenBucketWithInput, | ||
| detect_prompt_injection, | ||
| fixed_window, | ||
| local_custom, | ||
| local_detect_sensitive_info, | ||
| sliding_window, | ||
| token_bucket, | ||
| ) |
There was a problem hiding this comment.
This file appears to substantially duplicate the new guard test suites under tests/unit/guard/ (and some integration assertions). Keeping two near-identical copies will increase CI time and maintenance cost when the API evolves. Consider removing this top-level tests/test_guard.py in favor of the unit/integration suites (or vice versa), or refactor shared helpers/parametrized tests so the behavior is tested once.
No description provided.