Repository navigation
fix(agt-policies): refuse malformed governance input in the migrator instead of crashing or silently dropping it - #4377
Open
MohammadHaroonAbuomar wants to merge 6 commits into
Open
MohammadHaroonAbuomar wants to merge 6 commits into
MohammadHaroonAbuomar wants to merge 6 commits into
Conversation
The one-way v4-to-v5 resolver let several malformed inputs escape as bare Python exceptions, which the CLI does not catch (it catches only ResolutionError), so the user saw a traceback, exit 1 and no migration report. Others were accepted and produced a bundle that silently denied everything below the bad rule, with only a log warning. merge: a rule with a null, string or boolean priority raised TypeError from the rule sort. It is now refused with the rule name. build: a condition value with no JSON spelling (a YAML date, !!binary, .nan, .inf) raised TypeError from json.dumps in the renderer. Rules are now validated before anything is written: a missing condition, an and/ or/not compound condition (the generated Rego emits one scalar predicate per rule), an invalid or non-string field path, an unsupported operator, a value of the wrong shape for its operator, a regex that is not valid Go RE2, or a value JSON cannot write exactly all raise ResolutionError naming the rule. The always-matching fail-closed deny stays in the renderer as an unreachable last line of defence, and _rego_op_clause now returns None instead of raising when a literal cannot be written. discover: OSError from Path.resolve() or from the is_file() calls in the walk (an unreadable parent directory) propagated raw. Both are now ResolutionError. A non-existent action_path was walked as a directory and returned the root file; it is refused, as is a path that is neither a file nor a directory. scope: an empty or blank scope matched nothing and dropped the document and its deny rules without a word; a list-valued scope raised AttributeError; a path outside the root raised ValueError from relative_to. All three are now ResolutionError. Two existing tests changed: the invalid-field test now expects the refusal rather than the deny-all rendering, and the mixed-type test exercises merge_documents directly, since its assertion is about the never-matches analysis and resolve_manifest now refuses every compound condition. Refs #3137 Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
A regular file named "policies" next to a GovernancePolicy() source made policies_dir.mkdir() raise FileExistsError during --write, after the governance chains had already been moved aside (chains apply before policies). The preflight pass now records it as manual review so the write never starts, and a failure to create the staging directory is recorded the same way instead of escaping. --write-report to a path whose parent did not exist raised FileNotFoundError after the report had already been printed. The parent directory is now created, and any OSError from the write is reported on stderr without changing the migration's own exit code. Refs #3137 Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
List the inputs the v4-to-v5 migration now refuses in the README's refusal paragraph and add the CHANGELOG entry for the fixed defects. Refs #3137 Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
The merge layer still let three bare exceptions escape the migrator,
which catches only ResolutionError and so exited with a traceback and
no report.
The priority check called math.isfinite() on every numeric priority.
For an int that does not fit a float (10**400) that raises
OverflowError instead of returning. Only a float can be non-finite,
so the finiteness test now applies to floats alone; an int of any
size is a valid priority and sorts above smaller ones on both the
single-document and multi-document paths.
A document with `rules: null` (a bare `rules:` line) raised TypeError
from the for loop. A string or mapping under `rules` was walked
element by element and misreported as a nameless rule. A new
_document_rules helper refuses any non-list value with a
ResolutionError naming the type. A missing key still means no rules,
matching the other loaders in the repo, none of which treat null as
an empty list.
A NUL byte in action_path made Path.resolve() raise ValueError
("embedded null character") past the OSError/RuntimeError handler in
discover_policies. ValueError is now caught beside OSError at both
the resolve and the chain-walk step and mapped to the same
ResolutionError.
Refs #3137
Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
The --write preflight checked `policies_dir.exists() and not is_dir()`. exists() follows symlinks, so a dangling symlink named `policies` passed the check and mkdir() raised FileExistsError after the governance chains had already been moved aside. The check now looks at the link itself: `(is_symlink() or exists()) and not is_dir()`. A policies directory the user cannot write to failed the same way: the staging TemporaryDirectory raised PermissionError after the chains were moved. The preflight now records a finding when os.access(policies_dir, os.W_OK) is false, so nothing is applied. The test skips when running as root, where directory permissions are not enforced. A NUL byte in the --write-report path made Path.write_text() raise ValueError rather than OSError, so the handler that keeps a failed report copy from changing the exit code let it escape. ValueError is now caught beside OSError and reported on stderr like any other write failure. Refs #3137 Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
MohammadHaroonAbuomar
requested review from
liamcrumm and
Prayag (prayagupa)
as code owners
October 20, 2026 08:38
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
github-actions
Bot
added
documentation
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
Signed-off-by: MohammadHaroonAbuomar <40180927+MohammadHaroonAbuomar@users.noreply.github.com>
This branch has not been deployed
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.
Related Issue
Part of #3137. The findings this PR does not fix are tracked there.
Problem & Solution
Issue #3137 lists 36 findings from an audit of the agt-policies package. I re-audited every one against current
main(51d7ded):run_sync; it is out of scope here and noted for a follow-up.agt migrate v4-to-v5path. This PR fixes them.run_sync)What changes for a user of the migrator:
ResolutionErrorthat names the rule. This covers compoundand/or/notconditions, a rule without a condition, an unsupported operator, an invalid field path, a value of the wrong shape for its operator, a value with no JSON spelling (a YAML date,!!binary,.nan), a priority that is not a finite number, ascopethat is present but blank or not a string, and a regex rule when no RE2 validator is available. Before, each of these either rendered an always-matching fail-closed deny (so every lower-priority rule became dead code and the bundle denied everything) or crashed the CLI with a bareTypeErrororAttributeErrortraceback.ruleskey that is not a list is refused instead of raising inside the sort.OSErrorfromPath.resolve()and from stat calls during the walk intoResolutionErrornaming the path. A non-existentaction_pathis refused instead of being walked as a directory.filter_by_scopewith a path outside the root raisesResolutionError, notValueError.--writeruns a preflight pass: apoliciespath that exists but is not a writable directory (a regular file, a broken symlink, a directory without write permission) is recorded as a refusal before any governance file moves, instead of crashing withFileExistsErrororPermissionErrorafter the chains have moved. A staging-directory failure is recorded the same way.--write-reportcreates the parent directory and reports any write error (including a NUL byte in the path) on stderr. It no longer changes the exit code once the migration itself has succeeded.The migration refuses rather than guesses. Nothing is coerced.
Impact on Your Work
Running
agt migrate v4-to-v5on a project whose governance files hold any of the inputs above either produced a traceback with no report or wrote a bundle that denied every action. Both outcomes hid the real problem. With this change the CLI prints which rule or path it could not translate and exits cleanly, so the user can fix the source file and rerun.Timeline
None
Alternatives Considered
Coerce the input instead of refusing it (for example, treat a null priority as 0, treat a blank scope as the whole tree, or drop a compound condition). Rejected: the migrator must not guess at policy intent, and a silently widened or narrowed rule is worse than a clear refusal.
Type of Change
Package(s) Affected
Core & runtime:
Governance & security:
Platform & tooling:
CLI plugins:
Shared / other:
Testing
Unit Testing
New and changed tests in
agent-governance-python/agt-policies/tests/test_migrate.py(7) andtests/test_migrate_resolution.py(42). They cover each refusal above (compound conditions, unsupported operators, invalid fields, non-JSON values, bad and non-finite priorities including10**400, blank and non-string scope, non-listrules, missing RE2 validator), the discoveryOSErrorpaths and NUL bytes, the--writepreflight (regular file, broken symlink, unwritable directory), and--write-reportfailures leaving the exit code alone.Manual Testing
Environment: venv
/tmp/agt-3137-venv2withagent-control-specification 0.4.0b0built frompolicy-engine/sdk/python,PYTHONPATH=srcfromagent-governance-python/agt-policies.Full agt-policies suite on this branch at 3c2769e (
pytest -o addopts=""to get the summary line the repo's-qaddopts suppress):tests/test_migrate.pyandtests/test_migrate_resolution.pyalone:The 17 skips are the pre-existing
agent_osimport and cedar-dispatcher skips. The new root-only skip did not fire;test_write_refuses_when_policies_dir_is_not_writableran and passed.To confirm the tests detect the defects, I copied the branch's two test files into a throwaway worktree of
origin/main(51d7ded):All 49 failures are tests this branch added or changed. The same copy into a worktree of 134334c (the state before the last two commits) gave:
The 12 are exactly the tests added in 4e3ef19 and 3c2769e (broken symlink, not writable, write-report NUL, two int-too-large-for-float, six non-list rules, discover NUL).
test_merge_missing_rules_key_means_no_rulespasses on all three states, as intended. The two10**400tests also pass onorigin/mainbecause main has no finiteness check at all, so the "12 of 13 fail on the old source" claim holds relative to 134334c, not relative toorigin/main.CI checks run locally:
ruff check src/ --select E,F,W --ignore E501,scripts/check_license_headers.pyon new files, and the cspell added-lines check. All clean.Checklist
Attribution & Prior Art
Prior art / related projects (if any):
AI Assistance
If AI tools materially shaped this change, briefly note what was used:
Claude Code re-audited the findings and drafted the fixes and tests under my direction; an independent verification pass re-ran the tests on main and on the branch and probed the original defects with adversarial inputs, and I reviewed the result.
IP, Patents, and Licensing