Repository navigation
feat: warn on empty policy startup - #3441
Sakuna Harinda (sakunaharinda) wants to merge 4 commits into
Conversation
Signed-off-by: sakunaharinda <sakunaj1996@gmail.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Welcome to the Agent Governance Toolkit! Thanks for your first pull request. |
🤖 AI Agent: test-generator — `agentmesh/server/policy_server.py`
|
🤖 AI Agent: security-scanner — View details
No security issues found. |
🤖 AI Agent: docs-sync-checker — Docs Sync
Docs Sync
|
🤖 AI Agent: code-reviewer — View details
TL;DR: 0 blockers, 1 warning. Change improves startup validation but has a minor test coverage gap.
Action items: Add a direct test for Warnings: fine as follow-up PRs. |
🤖 AI Agent: breaking-change-detector — API Compatibility
API Compatibility
|
🤖 AI Agent: contributor-guide — View details
Welcome, and thank you for contributing! 🎉 Great job adding comprehensive tests to validate the new startup warning functionality. Before merging, please ensure the documentation is updated to reflect the new behavior for empty policy directories. Refer to CONTRIBUTING.md for guidance. |
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. |
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- policy_server.py _load_policies: the state reset (_engine/_trust_policies/_trust_evaluator/_loaded_count) moved before the directory-exists early-return, so a hot-reload during a transient missing dir (unmounted volume) now wipes loaded policies and, with the fail-closed engine, turns the server deny-all. Availability-relevant behavior change unmentioned in the PR description; revert to preserving prior state on missing dir, or document and test the new semantics explicitly (the _trust_evaluator reset does fix a real stale-evaluator bug; state that too).
Minor:
- _validate_startup/startup_warnings also run on every hot-reload; load_warnings is the accurate name, especially since the reload response includes the key.
- warn-only means /ready still reports ready with zero policies; an opt-in AGT_REQUIRE_POLICIES readiness gate would let k8s catch a misconfigured policy volume before routing traffic to a deny-all sidecar. Fine as a follow-up.
|
|
||
|
|
||
| def _validate_startup() -> None: | ||
| """Record startup warnings for empty policy state.""" |
There was a problem hiding this comment.
the warning text says "governance defaults may be permissive until policies are configured", but PolicyEngine.evaluate is V26 fail-closed: no policies loaded means deny by default (and the trust endpoint 503s). An empty policy dir is deny-all, not allow-all; a warning asserting the opposite failure mode misleads operators about blast radius. Reword to the actual semantics, e.g. "all evaluations will be denied by default until policies are loaded" (same text in sidecar.py:187).
There was a problem hiding this comment.
Hi, I updated the warning message as suggested. Also, I updated _load_policies() so a transient missing policy directory no longer clears the existing in-memory policies during hot reload, which avoids the accidental deny-all state if a mounted volume disappears briefly. The trust evaluator still gets rebuilt on real reloads, so that stale-evaluator bug is fixed as well.
I also renamed startup_warnings to load_warnings, since that warning payload is produced on both startup and reload. I left /readyz warn-only for now.
| def test_list_policies_reports_startup_warning_when_empty(self, caplog, tmp_path): | ||
| from unittest.mock import patch | ||
|
|
||
| with patch.dict(os.environ, {"AGENTMESH_POLICY_DIR": str(tmp_path)}): | ||
| from agentmesh.server import policy_server | ||
|
|
||
| with caplog.at_level(logging.WARNING): | ||
| with TestClient(policy_server.app) as client: | ||
| resp = client.get("/api/v1/policies") | ||
|
|
||
| assert resp.status_code == 200 | ||
| data = resp.json() | ||
| assert data["total_loaded"] == 0 | ||
| assert data["startup_warnings"] | ||
| assert any("Startup validation: no policies loaded" in msg for msg in caplog.messages) |
There was a problem hiding this comment.
test_list_policies_reports_startup_warning_when_empty does not exercise what it claims: POLICY_DIR is read once at module import, the module is already imported by earlier tests, so patch.dict + re-import is a no-op and tmp_path is never used; the test passes only because /etc/agentmesh/policies does not exist in CI. Reload the module under the patched env or monkeypatch policy_server.POLICY_DIR directly.
There was a problem hiding this comment.
Hi, Good catch. I changed the test so that it now actually exercises the module under test instead of depending on the already-imported default path. With that fix, the tmp_path is used during startup.
|
@microsoft-github-policy-service agree |
MohammadHaroonAbuomar
left a comment
MohammadHaroonAbuomar
left a comment
There was a problem hiding this comment.
- .DS_Store: a macOS Finder artifact was committed at the repo root in this push; remove it (and consider adding it to a global gitignore).
Minor:
- all three blocking asks are resolved and verified: the warning now says evaluations are denied by default until policies load; the test monkeypatches policy_server.POLICY_DIR directly; the reload state reset is back behind the directory-exists early-return so a transient missing dir preserves loaded policies. The naming and AGT_REQUIRE_POLICIES suggestions remain optional.
| "status": "ready", | ||
| "component": "governance-sidecar", | ||
| "policies_loaded": _loaded_count, | ||
| "startup_warnings": list(_startup_warnings), |
There was a problem hiding this comment.
/ready still returns status ready when zero policies are loaded, and /readyz, which is the probe most Kubernetes manifests use, is untouched and carries neither the count nor the warning. An empty PolicyEngine does deny by default, so a server in this state denies every request while reporting healthy to its orchestrator. Failing readiness when the policy count is zero would put the signal where an operator sees it.
There was a problem hiding this comment.
Thanks — addressed.
Overrode /readyz in policy_server.py to return HTTP 503 when total_loaded == 0, and included total_loaded, policy_dir, and load_warnings in the response. Also set response_model=None to avoid FastAPI response-model generation errors.
|
|
||
|
|
||
| # Ensure override is applied at import time | ||
| _override_readyz() |
There was a problem hiding this comment.
agent-governance-python/agent-mesh/src/agentmesh/server/policy_server.py:74 — _override_readyz() runs after the override route is registered at :59, so it prunes BOTH /readyz routes; probed at head: GET /readyz returns 404 with 0 and with 1 loaded policy, so readiness can never pass. Please register the override after pruning (or prune by endpoint identity), add a test that /readyz returns 200 once policies load, and stop echoing POLICY_DIR in the response body.
|
Thank you for the original contribution and for the follow-up work on #3441. This PR has been open and inactive for more than a month, and it is now superseded by #4066, a clean replacement branch under Ricky-G that carries the policy-startup changes forward against current To keep review and agent-merge tracking focused in one place, I am closing this PR. Please use #4066 as the maintained path for this change; it is tracked by #4061 and includes the proper |
Description
This PR adds startup validation for the AgentMesh policy loading path so that an empty policy directory produces an explicit warning instead of silently proceeding with permissive governance behavior.
Type of Change
Package(s) Affected
Checklist
Attribution & Prior Art
Prior art / related projects (if any):
N/A
AI Assistance
If AI tools materially shaped this change, briefly note what was used:
N/A
IP, Patents, and Licensing
Related Issues
N/A