fix(policies): add NOT_IN operator to enum, evaluator, bridge, schema, and CLI#2373
Open
DhineshPonnarasan wants to merge 1 commit into
Open
Conversation
…, and CLI - Add PolicyOperator.NOT_IN enum member to schema.py - Implement list-exclusion semantics in _match_condition() (evaluator.py) - Correct shared policy bridge: not_in now maps to NOT_IN (was NE) - Add NOT_IN to _OPERATOR_MAP_FROM_POLICY reverse mapping (shared.py) - Add not_in branch to CLI _evaluate_condition() (cli.py) - Add 'not_in' to PolicyOperator enum in policy_schema.json Previously PolicyDocument.from_yaml() raised ValidationError on policies using operator: not_in, and the bridge silently mapped not_in to scalar NE semantics, corrupting allowlist-exclusion rules. Regression tests added for: - YAML loading and deserialization - evaluator semantics (in-list vs out-of-list, missing field, malformed target) - NOT_IN vs NE semantic difference - bridge roundtrip (shared <-> PolicyDocument) - CLI test-runner scenarios - end-to-end from YAML policy file 77 targeted tests passing, 223 broader policy tests passing, lint clean.
🤖 AI Agent: breaking-change-detector — API CompatibilityAPI CompatibilityNo breaking changes detected. |
🤖 AI Agent: security-scanner — View detailsNo security issues found. |
🤖 AI Agent: docs-sync-checker — Docs SyncDocs Sync
Please address these documentation issues. |
🤖 AI Agent: code-reviewer — View detailsTL;DR: 0 blockers, 1 warning. Solid fix with minor follow-up needed.
Action items: None.
|
🤖 AI Agent: test-generator — View detailsTest coverage looks good. No gaps identified. |
|
🟡 Contributor Check: MEDIUM
Automated check by AGT Contributor Check. |
PR Review Summary
Verdict: |
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
not_inwas accepted by the policy linter and shared validator but broke silently at runtime due to two bugs:PolicyOperatorenum missingNOT_IN—PolicyDocument.from_yaml()raisedValidationErrorfor any policy usingoperator: not_in._OPERATOR_MAP_TO_POLICYmapped"not_in"→PolicyOperator.NE(scalar!=), silently rewriting allowlist-exclusion rules into scalar inequality checks.This PR is an additive-only fix at all five operator coupling points. No existing operator semantics are changed.
Root Cause
Changes
schema.pyNOT_IN = "not_in"toPolicyOperatorenumevaluator.pyNOT_INbranch in_match_condition()with list-exclusion semanticsshared.py"not_in"→PolicyOperator.NOT_IN(wasNE); add reverse mappingcli.pynot_inbranch in_evaluate_condition(); remove staleListimportpolicy_schema.json"not_in"toPolicyOperator.enumFiles unchanged (pre-existing behavior preserved)
bridge.py—document_to_governance()data loss for non-auto-generated rule names is pre-existing architectural debt, not introduced herelint_policy.py—KNOWN_OPERATORSalready contained"not_in"before this PRTests Added
12 new regression tests across 3 files:
test_not_in_operator_loads_from_yamltest_policy_language.pytest_not_in_inside_allowlist_does_not_matchtest_policy_language.pytest_not_in_outside_allowlist_matchestest_policy_language.pytest_not_in_differs_from_netest_policy_language.pytest_not_in_missing_field_no_matchtest_policy_language.pytest_not_in_malformed_target_fails_closedtest_policy_language.pytest_not_in_empty_allowlist_matches_everythingtest_policy_language.pyvalue=[]→ deny fires for every present value (intentional Python membership semantics)test_not_in_end_to_end_from_yamltest_policy_language.pytest_not_in_mapping_to_policy_operatortest_shared_policy.py"not_in"→PolicyOperator.NOT_INtest_not_in_mapping_from_policy_operatortest_shared_policy.pyPolicyOperator.NOT_IN→"not_in"test_not_in_roundtrip_bridgetest_shared_policy.pySharedPolicySchema→PolicyDocument→ backtest_not_in_scenario_semanticstest_policy_cli.pytestsubcommand: 2/2 scenarios passPre-existing Issues (not introduced, not fixed in this PR)
re.match(anchored) incli.pyvsre.search(anywhere) inevaluator.pyforMATCHES— semantic drift in the CLI dev tool onlybridge.pyreverse map:GTE → "gt"andLTE → "lt"data lossTest Results
Prior Art
Standard Python membership semantics (
in/not in). No external prior art. This operator was already documented in the policy schema JSON and accepted by the linter — the implementation simply completes the missing runtime and deserialization wiring.Fixes #2372.
@imran-siddique — would appreciate a review when you get a chance. The change is additive-only (no existing operator semantics touched), and all 78 targeted tests plus the broader 223-test policy suite are green. Happy to address any feedback.