feat(test): write down the safety invariants and test them as invariants - #71
Open
mkoushni wants to merge 3 commits into
Open
feat(test): write down the safety invariants and test them as invariants#71mkoushni wants to merge 3 commits into
mkoushni wants to merge 3 commits into
Conversation
The fail-closed promise lived in comments and per-seam judgment. Codify it as a catalog and contain panics in every awaited phase the way concurrent already did. Fixes praxis-proxy#24 Signed-off-by: mkoushni <mkoushni@redhat.com>
A one-plugin catalog could not fail if a contained serial panic still skipped later audit, and several cells only asserted "not allow". Name the decision per cell and cover ignore/disable outside concurrent. Signed-off-by: mkoushni <mkoushni@redhat.com>
praxis-bot
reviewed
Sep 3, 2026
praxis-bot
left a comment
There was a problem hiding this comment.
Review: feat(test): write down the safety invariants and test them as invariants
Strong work. The containment model is sound: serial, transform, and audit panics now route through on_error the same way concurrent already did. The catalog approach -- exhaustive matches that fail the build when a phase or dialect is added -- is exactly the right mechanism for these invariants. The fault-injection infrastructure is well-factored and the test matrix is thorough.
Two findings, both medium.
| Severity | File | Finding |
|---|---|---|
| Medium | executor.rs |
ContainedOutcome::Lost non-blocking paths skip OnError::Disable |
| Medium | plugin.rs |
is_dispatch_phase takes self by value; all other PluginMode methods take &self |
A non-blocking Lost outcome recorded the error but skipped the Disable circuit breaker that Error, Timeout, and Panic already trip. Match the other PluginMode predicates on the receiver too. Signed-off-by: mkoushni <mkoushni@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
docs/safety-invariants.md(linked from CONTRIBUTING) and drive them with a reusable fault-injection plugin and PDP resolver.on_errorinstead of unwindingexecute(). Halt vs continue still followscan_block; that difference is documented with a reason.{panic, error, timeout}underon_error: fail, plus ignore/disable for serial, transform, and audit. A sequential panic under ignore still runs later audit; under fail it does not.on_errorin YAML isFail. Missingsubject.idpins the differential allowlist row (Cedar dispatch error, CEL eval error, OPA default deny). Malformed policy pins Cedar dispatch / CEL compile / OPA compile-or-dispatch.{panic, error, timeout}cells drive the evaluator wrapper (FaultPdp), not the Cedar/CEL/OPA resolvers themselves. Real dialects are covered for missing attribute and malformed policy. A new phase or shipped dialect fails the build until a cell is added.Closes #24
Test plan
cargo test -p praxis-policy-core --test safety_invariantscargo test -p praxis-policy-apl-core --test safety_invariantscargo test -p praxis-policy-pdp-diff safety