diff --git a/pkg/intent/README.md b/pkg/intent/README.md index d58038e1c0d..8639691cb21 100644 --- a/pkg/intent/README.md +++ b/pkg/intent/README.md @@ -21,7 +21,7 @@ Resolution is performed by a `Resolver`, which holds a label matcher function an |------|------|-------------| | `AttributionStatus` | string | Classifies the outcome of intent attribution | | `AttributionSource` | string | Identifies the data source used for attribution | -| `IntentRecord` | struct | Holds the attribution result for a pull request or issue | +| `IntentRecord` | struct | Holds the attribution result for a pull request or issue, including optional `Domains`, `Priority`, and `Risk` classification fields | | `RootReference` | struct | Represents a referenced issue or artifact root (node ID, type, URL, labels) | | `PullRequestData` | struct | Input data for pull request resolution (node ID, URL, labels, explicit intent, closing issues) | | `Resolver` | struct | Stateless resolver that maps labels to intent records | @@ -70,7 +70,20 @@ Resolution is performed by a `Resolver`, which holds a label matcher function an `PolicyRule` configures a single policy fragment. Its `ID` identifies the matched rule in compiled policy output, `Scope` records the rule level (`"organization"`, `"repository"`, `"intent"`, or `"workflow"`), `When` holds the match criteria, and `Set` holds the `ExecutionPolicy` fields to merge when the rule applies. -`PolicyCondition` matches rule criteria against an `IntentRecord` and `RepositoryContext`. Empty condition fields act as wildcards. `Domain`, `Priority`, and `Risk` match against intent labels; `Org` matches either the repository organization or owner. +`PolicyCondition` matches rule criteria against an `IntentRecord` and `RepositoryContext`. Empty condition fields act as wildcards. `Domain`, `Priority`, and `Risk` match against `IntentRecord.Labels` (not the dedicated `Domains`/`Priority`/`Risk` fields below, which are used only by `ResolveRisk`); `Org` matches either the repository organization or owner. + +### Risk classification + +| Function | Signature | Description | +|----------|-----------|-------------| +| `ResolveRisk` | `func ResolveRisk(rec IntentRecord) string` | Returns `rec.Risk` when set; otherwise derives a risk level from `rec.Domains`/`rec.Priority`: `security`+`critical` and `production` resolve to `"high"`, `infrastructure` to `"medium"`, `documentation` to `"low"`, and anything else to `"unknown"` | + +### Tool authorization + +| Type/Method | Signature | Description | +|-------------|-----------|-------------| +| `Authorizer` | struct | Authorizes individual tool calls against a compiled `ExecutionPolicy` | +| `AuthorizeTool` | `func (a Authorizer) AuthorizeTool(policy ExecutionPolicy, tool string) error` | Returns `ErrToolDenied` if `tool` is in `DeniedTools` (denial always wins), `ErrToolNotAllowed` if `AllowedTools` is non-nil and does not contain `tool`, or `nil` otherwise. A `nil` `AllowedTools` is unrestricted; a non-nil empty `AllowedTools` denies every tool. | ## Usage Examples diff --git a/pkg/intent/governance.go b/pkg/intent/governance.go new file mode 100644 index 00000000000..a399b6fa954 --- /dev/null +++ b/pkg/intent/governance.go @@ -0,0 +1,76 @@ +package intent + +import ( + "errors" + "slices" + + "github.com/github/gh-aw/pkg/logger" +) + +var governanceLog = logger.New("intent:governance") + +// ErrToolDenied is returned by Authorizer.AuthorizeTool when the tool appears in +// the policy's DeniedTools list. A deny always wins, even if the same tool is +// also present in AllowedTools. +var ErrToolDenied = errors.New("intent: tool denied by policy") + +// ErrToolNotAllowed is returned by Authorizer.AuthorizeTool when the policy's +// AllowedTools is non-nil (restricted) and does not contain the requested tool. +var ErrToolNotAllowed = errors.New("intent: tool not allowed by policy") + +// ResolveRisk returns rec.Risk when explicitly set; otherwise it derives a risk +// classification from rec.Domains and rec.Priority using deterministic, +// precedence-ordered rules: +// +// security + critical priority -> high +// production -> high +// infrastructure -> medium +// documentation -> low +// anything else -> unknown +// +// An explicit Risk always wins over any derived value, even when the record's +// domains or priority would otherwise match a different rule. +func ResolveRisk(rec IntentRecord) string { + if rec.Risk != "" { + governanceLog.Printf("ResolveRisk: using explicit risk=%s", rec.Risk) + return rec.Risk + } + + if slices.Contains(rec.Domains, "security") && rec.Priority == "critical" { + governanceLog.Print("ResolveRisk: security+critical -> high") + return "high" + } + if slices.Contains(rec.Domains, "production") { + governanceLog.Print("ResolveRisk: production -> high") + return "high" + } + if slices.Contains(rec.Domains, "infrastructure") { + governanceLog.Print("ResolveRisk: infrastructure -> medium") + return "medium" + } + if slices.Contains(rec.Domains, "documentation") { + governanceLog.Print("ResolveRisk: documentation -> low") + return "low" + } + + governanceLog.Print("ResolveRisk: no matching rule -> unknown") + return "unknown" +} + +// Authorizer authorizes individual tool calls against a compiled ExecutionPolicy. +type Authorizer struct{} + +// AuthorizeTool reports whether tool may be called under policy. DeniedTools is +// checked first and always wins, even if tool also appears in AllowedTools. A +// nil AllowedTools means unrestricted (any tool not explicitly denied is +// allowed); a non-nil AllowedTools (including an empty, non-nil slice) restricts +// calls to the listed tools, so a non-nil empty slice denies every tool. +func (a Authorizer) AuthorizeTool(policy ExecutionPolicy, tool string) error { + if slices.Contains(policy.DeniedTools, tool) { + return ErrToolDenied + } + if policy.AllowedTools != nil && !slices.Contains(policy.AllowedTools, tool) { + return ErrToolNotAllowed + } + return nil +} diff --git a/pkg/intent/governance_formal_test.go b/pkg/intent/governance_formal_test.go new file mode 100644 index 00000000000..7ae6c8a5499 --- /dev/null +++ b/pkg/intent/governance_formal_test.go @@ -0,0 +1,238 @@ +//go:build !integration + +package intent_test + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/github/gh-aw/pkg/intent" +) + +// Formal test suite derived from specs/intent-attribution-agent-governance.md, +// focusing on the Risk classification (ResolveRisk) and Enforcement +// (Authorizer.AuthorizeTool) sections, plus fail-closed policy compilation for +// unlinked/ambiguous attribution. Each test corresponds to a named predicate or +// invariant in the behavioral coverage map. + +// TestResolveRisk_ExplicitOverride (P1/P2 — RiskExplicitOverride) +// Invariant: an explicit intent.Risk always wins over derived rules, even with +// conflicting domains/priority that would otherwise resolve differently. +func TestResolveRisk_ExplicitOverride(t *testing.T) { + rec := intent.IntentRecord{ + Risk: "low", + Domains: []string{"security", "production"}, + Priority: "critical", + } + assert.Equal(t, "low", intent.ResolveRisk(rec), + "P1/P2: explicit risk must win over derived rules") +} + +// TestResolveRisk_SecurityCriticalIsHigh (P3 — RiskSecurityCriticalHigh) +// Invariant: domains contains security AND priority == critical => high. +func TestResolveRisk_SecurityCriticalIsHigh(t *testing.T) { + rec := intent.IntentRecord{ + Domains: []string{"security"}, + Priority: "critical", + } + assert.Equal(t, "high", intent.ResolveRisk(rec), + "P3: security+critical must resolve to high") +} + +// TestResolveRisk_ProductionIsHigh (P4 — RiskProductionHigh) +// Invariant: domains contains production => high, independent of priority. +func TestResolveRisk_ProductionIsHigh(t *testing.T) { + cases := []string{"", "low", "critical", "unrecognized"} + for _, priority := range cases { + t.Run("priority="+priority, func(t *testing.T) { + rec := intent.IntentRecord{ + Domains: []string{"production"}, + Priority: priority, + } + assert.Equal(t, "high", intent.ResolveRisk(rec), + "P4: production domain must resolve to high regardless of priority") + }) + } +} + +// TestResolveRisk_InfrastructureIsMedium (P5 — RiskInfrastructureMedium) +// Invariant: domains contains infrastructure => medium. +func TestResolveRisk_InfrastructureIsMedium(t *testing.T) { + rec := intent.IntentRecord{Domains: []string{"infrastructure"}} + assert.Equal(t, "medium", intent.ResolveRisk(rec), + "P5: infrastructure domain must resolve to medium") +} + +// TestResolveRisk_DocumentationIsLow (P6 — RiskDocumentationLow) +// Invariant: domains contains documentation => low. +func TestResolveRisk_DocumentationIsLow(t *testing.T) { + rec := intent.IntentRecord{Domains: []string{"documentation"}} + assert.Equal(t, "low", intent.ResolveRisk(rec), + "P6: documentation domain must resolve to low") +} + +// TestResolveRisk_UnknownDefault (P7 — RiskUnknownDefault) +// Invariant: no matching rule (empty, unrecognized domain, security without +// critical priority) => unknown. +func TestResolveRisk_UnknownDefault(t *testing.T) { + cases := []struct { + name string + rec intent.IntentRecord + }{ + {"empty", intent.IntentRecord{}}, + {"unrecognized_domain", intent.IntentRecord{Domains: []string{"marketing"}}}, + {"security_without_critical", intent.IntentRecord{Domains: []string{"security"}, Priority: "low"}}, + {"security_no_priority", intent.IntentRecord{Domains: []string{"security"}}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, "unknown", intent.ResolveRisk(tc.rec), + "P7: non-matching input must resolve to unknown") + }) + } +} + +// TestResolveRisk_PrecedenceOrder (P8 — RiskPrecedenceOrder) +// Invariant: security+critical takes precedence when multiple domains overlap. +func TestResolveRisk_PrecedenceOrder(t *testing.T) { + rec := intent.IntentRecord{ + Domains: []string{"documentation", "infrastructure", "production", "security"}, + Priority: "critical", + } + assert.Equal(t, "high", intent.ResolveRisk(rec), + "P8: security+critical must take precedence over other overlapping domains") +} + +// TestAuthorizeTool_DeniedWins (P9 — AuthorizeToolDeniedWins) +// Invariant: a tool in DeniedTools is rejected even if it also appears in +// AllowedTools. +func TestAuthorizeTool_DeniedWins(t *testing.T) { + policy := intent.ExecutionPolicy{ + AllowedTools: []string{"read", "write"}, + DeniedTools: []string{"write"}, + } + err := intent.Authorizer{}.AuthorizeTool(policy, "write") + require.Error(t, err, "P9: denied tool must be rejected") + assert.True(t, errors.Is(err, intent.ErrToolDenied), + "P9: denied tool must return ErrToolDenied") +} + +// TestAuthorizeTool_AllowlistGate (P10 — AuthorizeToolAllowlistGate) +// Invariant: a non-nil allow list rejects tools not listed. +func TestAuthorizeTool_AllowlistGate(t *testing.T) { + policy := intent.ExecutionPolicy{AllowedTools: []string{"read"}} + err := intent.Authorizer{}.AuthorizeTool(policy, "exec") + require.Error(t, err, "P10: tool absent from a restricted allow list must be rejected") + assert.True(t, errors.Is(err, intent.ErrToolNotAllowed), + "P10: tool absent from allow list must return ErrToolNotAllowed") + + require.NoError(t, intent.Authorizer{}.AuthorizeTool(policy, "read"), + "P10: tool present in the allow list must be authorized") +} + +// TestAuthorizeTool_UnrestrictedWhenAllowedToolsNil (P11 — AuthorizeToolUnrestricted) +// Invariant: nil AllowedTools means unrestricted (except explicit denies). +func TestAuthorizeTool_UnrestrictedWhenAllowedToolsNil(t *testing.T) { + policy := intent.ExecutionPolicy{AllowedTools: nil, DeniedTools: []string{"exec"}} + + require.NoError(t, intent.Authorizer{}.AuthorizeTool(policy, "read"), + "P11: nil AllowedTools must permit any tool that isn't denied") + require.NoError(t, intent.Authorizer{}.AuthorizeTool(policy, "anything"), + "P11: nil AllowedTools must permit any tool that isn't denied") + + err := intent.Authorizer{}.AuthorizeTool(policy, "exec") + require.Error(t, err, "P11: an explicit deny must still be rejected even when unrestricted") + assert.True(t, errors.Is(err, intent.ErrToolDenied)) +} + +// TestAuthorizeTool_EmptyAllowedToolsDeniesAll (P12 — AuthorizeToolEmptyDenyAll) +// Invariant: a non-nil, empty AllowedTools denies every tool, distinct from nil. +func TestAuthorizeTool_EmptyAllowedToolsDeniesAll(t *testing.T) { + policy := intent.ExecutionPolicy{AllowedTools: []string{}} + err := intent.Authorizer{}.AuthorizeTool(policy, "read") + require.Error(t, err, "P12: non-nil empty AllowedTools must deny all tools") + assert.True(t, errors.Is(err, intent.ErrToolNotAllowed)) +} + +// TestSafestDefaultPolicy_FailClosedForIndeterminateStatus (P13 — SafestDefaultFailClosed) +// Invariant: unlinked/ambiguous status forces the safest policy regardless of +// configured rules. +func TestSafestDefaultPolicy_FailClosedForIndeterminateStatus(t *testing.T) { + autoMerge := true + permissive := intent.PolicyRule{ + ID: "wildcard-permissive", + Set: intent.ExecutionPolicy{ + Autonomy: "bounded", + WriteScope: "any_branch", + HumanApprovalRequired: false, + AutoMergeAllowed: &autoMerge, + MaxAttempts: 10, + }, + } + compiler := intent.PolicyCompiler{Rules: []intent.PolicyRule{permissive}} + repo := intent.RepositoryContext{Owner: "owner", Name: "repo"} + + for _, status := range []intent.AttributionStatus{intent.AttributionUnlinked, intent.AttributionAmbiguous} { + t.Run(string(status), func(t *testing.T) { + rec := intent.IntentRecord{Status: status} + policy := compiler.Compile(rec, repo) + + assert.Equal(t, "propose_only", policy.Autonomy, "P13: indeterminate status must force propose_only") + assert.Equal(t, "none", policy.WriteScope, "P13: indeterminate status must force no write scope") + assert.True(t, policy.HumanApprovalRequired, "P13: indeterminate status must force human approval") + require.NotNil(t, policy.AutoMergeAllowed) + assert.False(t, *policy.AutoMergeAllowed, "P13: indeterminate status must force auto-merge denial") + assert.Equal(t, 1, policy.MaxAttempts, "P13: indeterminate status must force a single attempt") + }) + } +} + +// TestEdgeCase_EmptyDomainsAndPriority validates that a fully empty intent +// record resolves to unknown, not a panic or empty string. +func TestEdgeCase_EmptyDomainsAndPriority(t *testing.T) { + risk := intent.ResolveRisk(intent.IntentRecord{}) + assert.Equal(t, "unknown", risk, "edge case: fully empty record must resolve to unknown") + assert.NotEmpty(t, risk, "edge case: ResolveRisk must never return an empty string") +} + +// TestEdgeCase_NilDeniedAndAllowedTools validates that AuthorizeTool does not +// panic on a zero-value policy. +func TestEdgeCase_NilDeniedAndAllowedTools(t *testing.T) { + require.NotPanics(t, func() { + err := intent.Authorizer{}.AuthorizeTool(intent.ExecutionPolicy{}, "read") + assert.NoError(t, err, "edge case: zero-value policy (nil AllowedTools/DeniedTools) must be unrestricted") + }) +} + +// TestEdgeCase_MultipleMatchingRulesPreserveStricterConstraint validates that a +// stricter constraint from an earlier rule isn't overridden by a later, more +// lenient rule. +func TestEdgeCase_MultipleMatchingRulesPreserveStricterConstraint(t *testing.T) { + strict := intent.PolicyRule{ + ID: "strict-first", + Set: intent.ExecutionPolicy{ + Autonomy: "propose_only", + WriteScope: "none", + }, + } + lenient := intent.PolicyRule{ + ID: "lenient-second", + Set: intent.ExecutionPolicy{ + Autonomy: "bounded", + WriteScope: "any_branch", + }, + } + compiler := intent.PolicyCompiler{Rules: []intent.PolicyRule{strict, lenient}} + rec := intent.IntentRecord{Status: intent.AttributionMapped, Labels: []string{"security"}} + repo := intent.RepositoryContext{Owner: "owner", Name: "repo"} + + policy := compiler.Compile(rec, repo) + + assert.Equal(t, "propose_only", policy.Autonomy, + "edge case: a later lenient rule must not override an earlier stricter autonomy constraint") + assert.Equal(t, "none", policy.WriteScope, + "edge case: a later lenient rule must not override an earlier stricter write-scope constraint") +} diff --git a/pkg/intent/resolver.go b/pkg/intent/resolver.go index d1fe8b67dba..340cc2c12e4 100644 --- a/pkg/intent/resolver.go +++ b/pkg/intent/resolver.go @@ -39,6 +39,13 @@ type IntentRecord struct { Labels []string `json:"labels,omitempty"` + // Domains, Priority, and Risk are optional classification dimensions used by + // ResolveRisk to derive a risk level when Risk is not explicitly set. They are + // distinct from Labels, which PolicyCondition matches against directly. + Domains []string `json:"domains,omitempty"` + Priority string `json:"priority,omitempty"` + Risk string `json:"risk,omitempty"` + Rule string `json:"rule,omitempty"` ResolverVersion string `json:"resolver_version,omitempty"` } diff --git a/specs/intent-attribution-agent-governance.md b/specs/intent-attribution-agent-governance.md index 7b61b23494d..3b027909fb9 100644 --- a/specs/intent-attribution-agent-governance.md +++ b/specs/intent-attribution-agent-governance.md @@ -937,12 +937,12 @@ The agent must not be able to modify or expand its own policy. ### `Authorizer.AuthorizeTool` Implementation Audit -The `AuthorizeTool` function as specified in this section is **not yet implemented** in the Go orchestrator. The following table documents which fields of `ExecutionPolicy` are wired to runtime enforcement and which remain unused. +`Authorizer.AuthorizeTool` and `ResolveRisk` are implemented in `pkg/intent` (see `pkg/intent/governance.go`), but neither is yet called by the Go orchestrator. The following table documents which fields of `ExecutionPolicy` are wired to runtime enforcement and which remain unused. | `ExecutionPolicy` field | Wired to enforcement? | Notes | |---|---|---| -| `AllowedTools` | **Not wired** | The `pkg/intent` package implements `PolicyCompiler.Compile()` and `mergePolicy()` for this field, but no orchestrator calls `AuthorizeTool` at tool-call time. | -| `DeniedTools` | **Not wired** | Same as `AllowedTools` — present in the spec and policy model, not enforced at runtime. | +| `AllowedTools` | **Implemented, not wired into orchestrator** | `pkg/intent` implements `PolicyCompiler.Compile()`, `mergePolicy()`, and `Authorizer.AuthorizeTool()` for this field, but no orchestrator calls `AuthorizeTool` at tool-call time yet. | +| `DeniedTools` | **Implemented, not wired into orchestrator** | Same as `AllowedTools` — `Authorizer.AuthorizeTool()` checks this field, but it is not yet invoked from the execution path. | | `Autonomy` | **Not wired** | The autonomy level is compiled into the policy but not checked against actual workflow capabilities at execution time. | | `WriteScope` | **Not wired** | Defined in the policy model; no runtime enforcement in the Go orchestrator. | | `HumanApprovalRequired` | **Not wired** | Defined in policy model; human approval gates are not currently tied to `ExecutionPolicy`. | @@ -951,9 +951,9 @@ The `AuthorizeTool` function as specified in this section is **not yet implement | `MaxAttempts` | **Not wired** | Not enforced at the orchestrator level. | | `RuleIDs` | **Provenance only** | Recorded in the policy for auditing; not used to gate execution. | -**Risk**: Policy constraints defined in `.github/intent-policy.json` (or the equivalent `rules` array) have no runtime effect until the orchestrator is wired to call `AuthorizeTool` and enforce `WriteScope`, `HumanApprovalRequired`, and `RequiredChecks`. Any policy compiled by `PolicyCompiler.Compile()` today is purely advisory. +**Risk**: Policy constraints defined in `.github/intent-policy.json` (or the equivalent `rules` array) have no runtime effect until the orchestrator calls `Authorizer.AuthorizeTool` and enforces `WriteScope`, `HumanApprovalRequired`, and `RequiredChecks`. Any policy compiled by `PolicyCompiler.Compile()` today is purely advisory. -**Required follow-up**: Implement `Authorizer.AuthorizeTool` in `pkg/intent` or a new `pkg/intent/authz` sub-package and wire it into the execution path. Gate enforcement behind a feature flag until the policy model is validated in production. +**Required follow-up**: Wire the now-implemented `Authorizer.AuthorizeTool` (in `pkg/intent`) into the execution path. Gate enforcement behind a feature flag until the policy model is validated in production. Initial observable rules: