feat: add governance policy linter CLI command (#404)#432
feat: add governance policy linter CLI command (#404)#432imran-siddique merged 6 commits intomicrosoft:mainfrom
Conversation
ADO requires service connection names at compile time for task authorization. Runtime variables cannot be used for connectedservicename. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add lint-policy subcommand to agent-compliance CLI that validates YAML policy files for common mistakes: Checks (errors): missing required fields (version, name, rules), unknown operators, unknown actions, invalid priority values, invalid YAML format Checks (warnings): empty rules lists, deprecated field names (type→action, op→operator), conflicting rules (same condition with both allow and deny) CLI: agent-compliance lint-policy <path> [--json] [--strict] Exit code 0 if no errors, 1 if errors (or warnings with --strict) 34 new tests covering all lint checks and CLI integration. Closes microsoft#404 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🤖 AI Agent: breaking-change-detector — Summary🔍 API Compatibility ReportSummaryThe pull request introduces a new CLI subcommand Findings
Migration GuideNo migration steps are required as this change is additive and does not affect existing functionality. Notes
✅ No breaking changes detected. |
🤖 AI Agent: test-generator — `packages/agent-compliance/src/agent_compliance/cli/main.py`🧪 Test Coverage Analysis
|
There was a problem hiding this comment.
🤖 AI Agent: code-reviewer
Pull Request Review: feat: add governance policy linter CLI command (#404)
Summary
This PR introduces a new lint-policy subcommand to the agent-compliance CLI, which validates YAML policy files for common mistakes. The linter checks for missing fields, invalid YAML, unknown operators/actions, deprecated fields, conflicting rules, and other issues. It supports JSON output and a strict mode to treat warnings as errors.
🔴 CRITICAL Issues
-
Unvalidated YAML Parsing:
- The linter uses
yaml.safe_loadto parse YAML files. Whilesafe_loadmitigates some risks (e.g., arbitrary code execution), it does not validate the schema of the YAML file. This could lead to false negatives if the file structure deviates from expectations but still parses successfully. - Action: Integrate schema validation using
pydanticmodels orcerberusto ensure the YAML structure adheres strictly to the expected format.
- The linter uses
-
Thread Safety:
- The
lint_pathfunction processes multiple files in a directory sequentially. If this function is used in a concurrent context (e.g., multi-threaded CLI execution), the sharedLintResultobject could lead to race conditions. - Action: Refactor
lint_pathto use thread-local storage or ensure results aggregation is thread-safe.
- The
-
Sandbox Escape via File Paths:
- The
lint_pathfunction accepts arbitrary file paths without sanitization. This could allow malicious users to supply paths that escape the intended directory scope or access sensitive files. - Action: Validate and sanitize file paths to ensure they are within the expected directory scope. Reject paths with traversal patterns like
../.
- The
🟡 WARNING: Potential Breaking Changes
-
CLI Behavior Change:
- Adding the
--strictflag changes the behavior of the CLI by treating warnings as errors. This could break existing workflows that rely on warnings being non-blocking. - Action: Clearly document this change in the release notes and provide guidance for users who may be affected.
- Adding the
-
Deprecation of Fields:
- The linter flags deprecated fields (
type,op, etc.) with warnings. If these fields are removed in future versions, it will break backward compatibility. - Action: Provide a migration guide for users and ensure deprecation warnings are prominently documented.
- The linter flags deprecated fields (
💡 Suggestions for Improvement
-
Enhanced Conflict Detection:
- The current conflict detection logic only checks for
allowanddenyactions with identical conditions. Consider extending this to detect conflicts across other actions (e.g.,auditvs.block). - Action: Expand the
seendictionary logic to track all actions and their interactions.
- The current conflict detection logic only checks for
-
Improved Error Reporting:
- The error messages for invalid YAML files (
Invalid YAML: {exc}) could be more user-friendly by including specific suggestions for fixing the issue. - Action: Parse the
yaml.YAMLErrorobject to extract detailed information about the error (e.g., expected token, line number).
- The error messages for invalid YAML files (
-
Unit Test Coverage:
- While the tests cover a wide range of scenarios, edge cases like deeply nested rules, large YAML files, and invalid file encodings are not explicitly tested.
- Action: Add tests for these edge cases to ensure robustness.
-
Performance Optimization:
- The
_find_linefunction performs linear scans of the file's lines, which could be slow for large YAML files. - Action: Optimize
_find_lineusing precomputed line indices or a more efficient search algorithm.
- The
-
CLI Output Formatting:
- The CLI output for
lint-policycould be improved by adding color-coded messages (e.g., red for errors, yellow for warnings) for better readability. - Action: Use a library like
coloramaorrichto enhance the CLI output.
- The CLI output for
Additional Observations
-
Use of Frozen Sets:
- The use of
frozensetforKNOWN_OPERATORSandKNOWN_ACTIONSis appropriate for immutability and performance. This is a good design choice.
- The use of
-
Strict Mode Implementation:
- The
--strictflag is implemented correctly, ensuring warnings are treated as errors when enabled. This is a useful feature for enforcing stricter policy compliance.
- The
-
Documentation:
- The PR includes clear documentation for the new CLI command, its options, and expected behavior. This is commendable.
Final Recommendation
- Address the CRITICAL issues related to YAML validation, thread safety, and sandbox escape.
- Document the WARNING changes to ensure users are aware of potential impacts.
- Consider implementing the SUGGESTIONS to improve functionality, performance, and user experience.
Once the critical issues are resolved, this PR will be ready for merging.
🤖 AI Agent: docs-sync-checker — Issues Found📝 Documentation Sync ReportIssues Found
Suggestions
Action Items
Please address these issues to ensure documentation is in sync with the new feature. |
🤖 AI Agent: security-scanner — Security Findings for Pull Request #404Security Findings for Pull Request #404
SummaryThe changes introduced in this pull request enhance the functionality of the governance toolkit by adding a linter for policy files. However, several security concerns need to be addressed, particularly around deserialization attacks and supply chain vulnerabilities. Implementing the suggested fixes will help mitigate these risks and strengthen the overall security posture of the toolkit. |
Description
Adds a \lint-policy\ subcommand to the \�gent-compliance\ CLI that validates YAML policy files for common mistakes.
Lint checks
Usage
\\�ash
agent-compliance lint-policy policies/
agent-compliance lint-policy policy.yaml --json
agent-compliance lint-policy policies/ --strict # treat warnings as errors
\\
Tests
34 new tests — all passing.
Closes #404