fix(agt-policies): ne/not_in fail open on missing fields, bypassing deny rules

[Pranavk098]

Summary

Fixes #3297.

_rego_op_clause in agent-governance-python/agt-policies/src/agt/manifest_resolution/build.py guarded every comparison operator with _v != null before evaluating the operator. That guard is correct for positive operators (eq, gt, in, ...) — a missing field shouldn't satisfy a positive assertion. But the same guard was applied uniformly to the negative operators ne and not_in, which inverts the intended safety: when the referenced field is absent, _v != null itself is false, so the rule body never matches and evaluation falls through to default verdict := allow.

Concretely, a deny rule like:

deny if tool_call.content_hash ne "sha256:REGISTERED"

intended to block calls with an unregistered (or missing) content hash, was bypassed entirely by omitting content_hash — trivial for a caller to do.

Fix

Dropped the _v != null guard for ne and not_in only (2 lines changed). Rego's native != and not ... in already treat null correctly once the guard is removed — no restructuring of the accessor or matcher needed:

if operator == "ne":
    return f"{indent}_v := {accessor}\n{indent}_v != {literal}"
...
if operator == "not_in":
    return f"{indent}_v := {accessor}\n{indent}not _v in {literal}"

All other operators (eq, gt, lt, gte, lte, in, exists, contains, startswith, endswith, matches/regex) are untouched.

Tests

Added to tests/scenarios/test_egress_content_hash_escalation_scenarios.py, run through the real OPA-backed scenario harness (agt._harness.opa_runner.run_scenario), not string-matching on generated Rego source:

• test_missing_content_hash_denied — the exact content_hash ne example from Declarative negative operators (ne, not_in) fail open on missing fields, bypassing deny rules #3297
• A new not_in region-allowlist fixture with test_region_in_allowlist_passes, test_region_outside_allowlist_denied, and test_region_missing_denied

Watched both missing-field tests fail red (decision: allow) against the unmodified code, then pass green after the fix.

Verification

$ PYTHONPATH=src python -m pytest tests/ -q
122 passed, 8 pre-existing failures (Windows symlink privilege, cp1252 console
encoding, OPA 1.18 vs. stock-Rego-library version mismatch), 20 skipped

Confirmed via git stash that the 8 failures are identical (same tests, same errors) on unmodified upstream/main — none are introduced by this change. Also independently reproduced the issue's exact opa eval repro command against _render_rego output to confirm the fix.

Test plan

• New regression tests fail on pre-fix code, pass on post-fix code
• Full agt-policies suite has no new failures vs. baseline
• ruff check clean on changed files
• Manually reran the issue's OPA repro script against the fixed renderer

🤖 Generated with Claude Code

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[github-actions]

🤖 AI Agent: security-scanner — View details

AI-generated review output. Treat it as untrusted analysis and verify before acting.

No security issues found.

[github-actions]

🤖 AI Agent: test-generator — `agent-governance-python/agt-policies/src/agt/manifest_resolution/build.py`

AI-generated review output. Treat it as untrusted analysis and verify before acting.

agent-governance-python/agt-policies/src/agt/manifest_resolution/build.py

• test_operator_ne_missing_field -- Validate that a missing field correctly triggers a deny rule with the ne operator.
• test_operator_not_in_missing_field -- Validate that a missing field correctly triggers a deny rule with the not_in operator.

Test coverage for the changes appears to be adequate. No additional gaps identified.

[github-actions]

🤖 AI Agent: breaking-change-detector — API Compatibility

AI-generated review output. Treat it as untrusted analysis and verify before acting.

API Compatibility

Severity	Change	Impact
High	Removed _v != null guard for ne and not_in operators in _rego_op_clause function.	Behavior of ne and not_in operators changes when fields are missing. Previously, missing fields would bypass these operators; now they will satisfy the conditions. This could cause existing policies to deny requests that were previously allowed.

[github-actions]

🤖 AI Agent: code-reviewer — View details

AI-generated review output. Treat it as untrusted analysis and verify before acting.

TL;DR: 0 blockers, 1 warning. Fix addresses a critical security issue effectively, but additional test coverage is recommended.

#	Sev	Issue	Where
1	Warn	Missing tests for other negative operators (!=, not_in) in unrelated scenarios.	tests/scenarios/test_egress_content_hash_escalation_scenarios.py

Action items:

• None (no blockers identified).

Warnings:

#	Issue	Fine as follow-up PRs
1	Add tests for != and not_in operators in unrelated scenarios to ensure consistent behavior across all policy contexts.	Yes

[github-actions]

🤖 AI Agent: docs-sync-checker — Docs Sync

AI-generated review output. Treat it as untrusted analysis and verify before acting.

Docs Sync

• def _rego_op_clause(operator: str, accessor: str, value: Any) -> Optional[str]: in agent-governance-python/agt-policies/src/agt/manifest_resolution/build.py -- missing docstring
• README.md -- no updates found for the behavioral change in how ne and not_in operators handle missing fields
• CHANGELOG.md -- missing entry for the fix to ne and not_in operators to prevent bypassing deny rules on missing fields

[github-actions]

PR Review Summary

Check	Status	Details
🔍 Code Review	⚠️ Missing	No current-run comment
🛡️ Security Scan	⚠️ Missing	No current-run comment
🔄 Breaking Changes	⚠️ Missing	No current-run comment
📝 Docs Sync	⚠️ Missing	No current-run comment
🧪 Test Coverage	⚠️ Missing	No current-run comment

Verdict: ⚠️ AI review incomplete; ready for human review

AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims.

[github-actions]

🟡 Contributor Check: MEDIUM

Check	Result
Profile	MEDIUM
Credential	LOW
Overall	MEDIUM

Automated check by AGT Contributor Check.

[imran-siddique]

Thanks for this fix and especially the OPA-backed scenario tests. I am closing this in favor of #3358, which supersedes it, for two reasons. First, dropping the _v != null guard unconditionally also affects allow rules, so an allow ne/not_in would fire on a missing field, which is fail-open and can preempt a later deny in the first-match chain; the guard should stay for allow and drop only for deny. Second, this does not cover the missing-intermediate case (#3360): test_region_missing_denied uses args={} rather than omitting args, so the chained-accessor undefined path is not exercised. #3358 handles both. Your OPA scenario tests are stronger than the string-level assertions there, so it would be great to port them into #3358. Appreciate the contribution.