Implement ResolveRisk and Authorizer.AuthorizeTool in pkg/intent (#53742)

Author: Copilot

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
 

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
+}

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")
+}

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"`
 }

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: