feat(intent): implement PolicyCompiler.Compile and formal attribution compliance test suite#48036
Conversation
…uite (P1–P10) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Implements intent policy compilation and adds formal compliance tests for attribution invariants.
Changes:
- Adds policy rule matching, merging, and safe defaults.
- Adds ten attribution and governance compliance tests.
- Policy precedence and fail-closed behavior require corrections.
Show a summary per file
| File | Description |
|---|---|
pkg/intent/policy.go |
Implements policy compilation and merging. |
pkg/intent/intent_formal_test.go |
Adds formal compliance tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/intent/policy.go:115
- Rules are ordered from highest to lowest precedence, but these unconditional assignments let every later, lower-precedence rule replace an earlier constraint—for example,
boundedcan replacepropose_only, orany_branchcan replacenone. Compare the documented restrictiveness of these enum values (or retain the first specified value) so lower-precedence rules cannot weaken the result.
if fragment.Autonomy != "" {
result.Autonomy = fragment.Autonomy
}
if fragment.WriteScope != "" {
result.WriteScope = fragment.WriteScope
pkg/intent/policy.go:129
- Appending allowed-tool lists unions permissions instead of combining constraints. If a higher-precedence rule allows only
readand a lower rule allows onlywrite, this returns both tools rather than the empty intersection;cloneStringsalso converts an explicit non-nil empty deny-all list to nil/unrestricted. Intersect non-nil allowlists while preserving the nil-versus-empty distinction.
result.AllowedTools = append(cloneStrings(base.AllowedTools), fragment.AllowedTools...)
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Medium
|
@copilot please run the Unresolved review feedback (newest first):
|
…ority/Risk in policy rules - Compile() short-circuits unlinked/ambiguous records to safestDefaultPolicy() regardless of any configured rules, fixing the fail-closed contract - Compile() uses a fresh accumulator seeded by the first matching rule so that permissive values (auto_merge: true, max_attempts > 1) are no longer silently discarded by the safest-default base - matches() now gates on Domain, Priority, and Risk via record labels; checks Org against both repo.Org and repo.Owner - Adds TestFormal_FailClosedForIndeterminate (P7b): verifies both unlinked and ambiguous records receive the safest policy even with a permissive wildcard rule Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…MergeTrue - deepCopyPolicy() helper ensures AutoMergeAllowed, AllowedTools, DeniedTools, RequiredChecks, and RuleIDs are freshly allocated so accumulated policy cannot alias the original rule.Set fields - Rename autoMergeTrue → autoMerge in TestFormal_FailClosedForIndeterminate Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All three reviewer threads are addressed and replied to. Summary of changes:
Local validation: |
|
@copilot please run the A prior sous-chef nudge was addressed, but the PR still appears active and may need final review/cleanup before merge.
|
All outstanding review feedback was addressed in the previous pass. Current state:
No further changes needed. CI must be re-triggered by a maintainer (close/reopen, |
|
@copilot please run the Unresolved review feedback (newest first):
A prior response indicates these items were addressed, so please resolve the corresponding threads if they are complete and confirm the PR is ready for maintainer review.
|
All three threads ( The PR is otherwise ready: all 27 tests pass, lint is clean, branch is up to date with main. |
🤖 PR Triage
Rationale: Implements and a formal attribution compliance test suite (2 files, 396 additions). Adds previously-missing implementation for a formal spec. Medium risk due to new API surface and policy compilation logic. CI passes. Needs human review of policy semantics.
|
|
@copilot please run the Unresolved review feedback (newest first):
A prior response indicates these items were addressed, so please resolve the corresponding threads if complete and rerun checks if anything changes.
|
Implements the
specs/intent-attribution-compliance/README.mdformal spec, which defines minimum conformance invariants for the Intent Attribution & Agent Governance system.PolicyCompilerhad noCompilemethod, making the policy compilation path untestable and unimplemented.pkg/intent/policy.goPolicyCompiler.Compile(IntentRecord, RepositoryContext) ExecutionPolicy— applies matching rules sequentially; falls back to safest default when no rules match.safestDefaultPolicy()—propose_onlyautonomy,nonewrite scope, human approval required, auto-merge explicitly denied (*bool = false),MaxAttempts: 1.PolicyRule.matches()— condition fields are wildcards when empty; currently gates onWhen.Org.mergePolicy(base, fragment)— stricter-wins semantics: OR forHumanApprovalRequired, AND forAutoMergeAllowed, min forMaxAttempts.pkg/intent/intent_formal_test.go(new)Ten deterministic tests covering all invariants from the behavioral coverage map (
!integrationbuild tag,intent_testpackage):TestFormal_ExplicitIntentWinsTestFormal_SingleClosingIssuesource: closing_issueTestFormal_MultipleClosingIssuesAmbiguousstatus: ambiguousTestFormal_AmbiguousNeverMappedmappedTestFormal_LabelFallbackTestFormal_UnlinkedWhenNoSourcestatus: unlinked,source: noneTestFormal_SafestPolicyFieldsTestFormal_PolicyDeterminismTestFormal_ExplicitOverridesSuggestedsuggestedstatusTestFormal_SingleSourcePerRecord