feat: add --validate flag to check policy YAML without evaluating#18
feat: add --validate flag to check policy YAML without evaluating#18Deepak8858 wants to merge 2 commits intotexasbe2trill:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to support validating a policy YAML without performing evaluation, but the submitted changes currently focus on strengthening LoadPolicy validation logic and adding unit tests for the new validations.
Changes:
- Added additional validation in
LoadPolicyfor missing/duplicatesafety_tiers,roles, andresourcesnames, plus requiredrole.max_tier. - Added
internal/config/load_test.gowith table-driven tests covering the new validation errors and a basic “valid policy” test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/config/load.go | Adds more structural validation for parsed policy YAML (names/duplicates/required fields). |
| internal/config/load_test.go | Adds unit tests for the new LoadPolicy validation behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(policy.SafetyTiers) == 0 { | ||
| return nil, fmt.Errorf("invalid policy: at least one safety_tier is required") | ||
| } |
There was a problem hiding this comment.
PR description/issue acceptance criteria describe a new --validate CLI mode (e.g. go run ./cmd/policyforge ... --validate), but this PR only changes LoadPolicy validation + tests. There is no cmd/policyforge directory in this repo and there are no Go code changes adding/handling a --validate flag, so the core feature appears unimplemented and the acceptance criteria can’t be met as-is.
| tierNames := make(map[string]bool) | ||
| for i, tier := range policy.SafetyTiers { | ||
| if tier.Name == "" { | ||
| return nil, fmt.Errorf("invalid policy: safety_tier at index %d is missing a name", i) | ||
| } | ||
| if tierNames[tier.Name] { | ||
| return nil, fmt.Errorf("invalid policy: duplicate safety_tier name found: '%s'", tier.Name) | ||
| } | ||
| tierNames[tier.Name] = true | ||
| } |
There was a problem hiding this comment.
SafetyTiers are treated as a fixed ordered set elsewhere (tierRank only recognizes read_only/supervised_write/autonomous_write). As written, a policy can define an unknown tier name (or a role/agent max_tier that’s unknown) and exceedsMaxTier will silently return false, which can bypass the intended approval gating. Consider validating that tier names, role.max_tier, and allowed tier references are restricted to the supported constants (and/or that all referenced tiers exist in safety_tiers).
| if len(policy.Resources) == 0 { | ||
| return nil, fmt.Errorf("invalid policy: at least one resource is required") | ||
| } | ||
| for _, role := range policy.Roles { | ||
| if role.MaxTier == "" { | ||
| return nil, fmt.Errorf("invalid policy: role '%s' must define max_tier", role.Name) | ||
| resourceNames := make(map[string]bool) | ||
| for i, resource := range policy.Resources { | ||
| if resource.Name == "" { | ||
| return nil, fmt.Errorf("invalid policy: resource at index %d is missing a name", i) | ||
| } | ||
| if resourceNames[resource.Name] { | ||
| return nil, fmt.Errorf("invalid policy: duplicate resource name found: '%s'", resource.Name) | ||
| } | ||
| resourceNames[resource.Name] = true | ||
| } |
There was a problem hiding this comment.
While adding policy validation, agent_envelopes are currently not validated at all (e.g., missing/duplicate name, missing max_tier). Because agent envelopes are used as an additional authorization layer, allowing empty/unknown max_tier can effectively remove that constraint. Consider adding similar required-field + uniqueness checks for policy.AgentEnvelopes (and validating max_tier against supported tiers).
| _, err := LoadPolicy(path) | ||
| if err == nil { | ||
| t.Fatal("expected error, got nil") | ||
| } | ||
| if err.Error() != "invalid policy: "+tt.expectedError { | ||
| t.Fatalf("expected error 'invalid policy: %s', got '%v'", tt.expectedError, err) | ||
| } |
There was a problem hiding this comment.
These tests assert exact err.Error() strings. That makes the suite brittle (any small wording/punctuation change breaks tests even when behavior is correct). Prefer asserting on a stable substring/category (e.g., strings.Contains) or comparing against a typed/sentinel error (if introduced) so refactors don’t force mass test updates.
This PR adds a new CLI command which supports a flag. When set, it loads and validates the policy YAML file and exits with code 0 if valid, or code 1 if invalid.
Closes #1