diff --git a/docs/adr/43594-policy-compiler-more-restrictive-wins-merge.md b/docs/adr/43594-policy-compiler-more-restrictive-wins-merge.md new file mode 100644 index 00000000000..511e37898e5 --- /dev/null +++ b/docs/adr/43594-policy-compiler-more-restrictive-wins-merge.md @@ -0,0 +1,53 @@ +# ADR-43594: Policy Compiler with More-Restrictive-Wins Merge Semantics + +**Date**: 2026-07-05 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +The gh-aw agent orchestrator needs a runtime authorization layer that controls what actions an agent may take for a given intent (e.g., which tools it may call, whether it may auto-merge, how many attempts it may make). Before this PR, the `Authorizer.AuthorizeTool` function was unimplemented — any policy defined in `.github/intent-policy.json` was purely advisory with no runtime effect. + +The governance spec (`specs/intent-attribution-agent-governance.md`) requires that policy precedence follows a strict hierarchy: organization constraints > repository constraints > intent-specific rules > workflow defaults > agent request. The key safety constraint is fail-closed behavior: when no rules match (or intent is ambiguous), the system MUST apply the most restrictive policy possible rather than granting elevated authority. + +A multi-rule policy system needs a merge strategy that determines which value wins when rules at different precedence levels specify conflicting constraints. The wrong strategy could allow a lower-precedence rule to silently weaken a security constraint established by a higher-precedence rule. + +### Decision + +We will implement a `PolicyCompiler` in `pkg/intent/policy.go` that compiles an ordered list of `PolicyRule` entries into a single `ExecutionPolicy` using **more-restrictive-wins** merge semantics. Rules are processed in declaration order (highest to lowest precedence). For each field, the merge function always keeps the value that represents the stricter constraint: `DeniedTools` and `RequiredChecks` use union, `AllowedTools` uses intersection when both sides constrain, `HumanApprovalRequired` uses OR, `AutoMergeAllowed` uses AND, and `MaxAttempts` uses min. The baseline is always `safestDefaultPolicy()` — a fail-closed policy that denies all write operations and requires human approval. + +### Alternatives Considered + +#### Alternative 1: Last-Write-Wins (Declaration Order Override) + +Each rule applies sequentially, with later rules fully overriding earlier rules' values. This is the simplest implementation and matches how many configuration systems work (e.g., environment variable layering). + +This was rejected because it allows lower-precedence rules (e.g., intent-specific) to silently weaken constraints established by higher-precedence rules (e.g., organizational). An org rule setting `human_approval_required: true` could be cleared by a downstream repo rule setting it `false`. This violates the spec's requirement that higher-precedence rules must not be weakened. + +#### Alternative 2: Single Flat Policy (No Rule Precedence) + +Eliminate rule scopes and precedence entirely. Maintain a single global `ExecutionPolicy` per repository, loaded from a config file. Intent-specific behavior is encoded as label-to-policy mappings in a flat lookup table, not as composable rules. + +This was rejected because it cannot express the layered governance model required by the spec (org-wide defaults + repo overrides + intent-specific adjustments). A flat model would require duplicating the most restrictive constraints in every policy entry, making the config error-prone and difficult to audit. + +### Consequences + +#### Positive +- Fail-closed by default: when no rules match, `safestDefaultPolicy()` produces `autonomy=propose_only`, `write_scope=none`, `human_approval_required=true`, `auto_merge_allowed=false`, `max_attempts=1`. +- Higher-precedence org/repo constraints cannot be weakened by lower-precedence intent rules; the merge invariant is enforced at the `mergePolicy()` level, making it impossible to bypass by rule ordering. +- `RuleIDs` field records which rules contributed to the compiled policy, enabling auditability of each policy decision. +- 3 unit tests covering org → repo → intent precedence are included and passing. + +#### Negative +- The compiled `ExecutionPolicy` is currently **not wired to runtime enforcement**: no orchestrator path calls `AuthorizeTool` or checks `WriteScope`/`HumanApprovalRequired`/`RequiredChecks` at execution time. Policy today is advisory only. All fields in `ExecutionPolicy` are documented as "Not wired" in the spec's implementation audit table. +- `PolicyCondition.Matches` checks label values as flat strings across all dimensions (domain, priority, risk). Callers must ensure label values are unique across dimensions to avoid false positives (e.g., a domain value that collides with a priority value would cause incorrect rule matches). + +#### Neutral +- The `PolicyCompiler` lives in `pkg/intent/policy.go` alongside the existing attribution types; it does not yet have its own sub-package. A follow-up will move authorization logic to `pkg/intent/authz` when `AuthorizeTool` is implemented. +- The merge semantics for `AllowedTools` are asymmetric: if neither side specifies tools, the result is unrestricted; if only one side specifies tools, that restriction is adopted; if both sides restrict, the intersection is used. This means adding `AllowedTools` to a rule makes it harder to relax at lower precedence levels. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/outcome_eval.go b/pkg/cli/outcome_eval.go index 06d57328714..fcd9908aacd 100644 --- a/pkg/cli/outcome_eval.go +++ b/pkg/cli/outcome_eval.go @@ -105,6 +105,8 @@ var outcomeEvaluators = map[string]outcomeEvaluator{ "push_to_pull_request_branch": evalPushToPRBranch, "add_reviewer": evalAddReviewer, "submit_pull_request_review": evalSubmitPullRequestReview, + "dispatch_workflow": evalDispatchWorkflow, + "update_discussion": evalUpdateDiscussion, } // EvaluateOutcomes checks the current state of all safe output items from a run. diff --git a/pkg/cli/outcome_eval_workflow.go b/pkg/cli/outcome_eval_workflow.go new file mode 100644 index 00000000000..447f3db26e9 --- /dev/null +++ b/pkg/cli/outcome_eval_workflow.go @@ -0,0 +1,104 @@ +package cli + +import ( + "fmt" + "math" + + "github.com/github/gh-aw/pkg/logger" +) + +// maxSafeFloat64Int is the largest integer that can be represented exactly as +// float64 (2^53). GitHub run IDs decoded from JSON arrive as float64; any value +// beyond this threshold cannot be round-tripped to int64 without precision loss. +const maxSafeFloat64Int = 1 << 53 + +var outcomeEvalWorkflowLog = logger.New("cli:outcome_eval_workflow") +var workflowOutcomeGHAPIGet = ghAPIGet + +// evalDispatchWorkflow checks whether a dispatched workflow run completed successfully. +// It looks for a run_id in the item metadata and queries the workflow run status. +// Spec: specs/safe-output-outcome-evaluation.md §20 +func evalDispatchWorkflow(item CreatedItemReport, repoOverride string) OutcomeReport { + repo := resolveItemRepo(item, repoOverride) + outcomeEvalWorkflowLog.Printf("Evaluating dispatch_workflow: repo=%s, url=%s", repo, item.URL) + + report := OutcomeReport{ + Type: item.Type, + ObjectURL: item.URL, + Repo: repo, + } + + // Extract run_id from metadata if available. + // JSON numbers unmarshal as float64; convert carefully to int64 to avoid + // precision loss for large GitHub run IDs (which can exceed 2^32). + var runID int64 + if item.Metadata != nil { + if v, ok := item.Metadata["run_id"]; ok { + switch id := v.(type) { + case float64: + // Guard against float64 values that cannot round-trip to int64. + // math.MaxInt64 is not representable exactly as float64; use 2^53 + // (maxSafeFloat64Int) as the safe upper bound for lossless conversion. + if id > 0 && id <= maxSafeFloat64Int && id == math.Trunc(id) { + runID = int64(id) + } + case int: + runID = int64(id) + case int64: + runID = id + } + } + } + + if runID <= 0 { + // No run ID available — workflow may not have been dispatched or ID not captured + report.Result = OutcomePending + report.Detail = "no run ID available; dispatch may still be queued" + return report + } + + data, err := workflowOutcomeGHAPIGet(fmt.Sprintf("actions/runs/%d", runID), repo) + if err != nil { + report.Result = OutcomeError + report.EvalError = err.Error() + return report + } + + status, _ := data["status"].(string) + conclusion, _ := data["conclusion"].(string) + outcomeEvalWorkflowLog.Printf("dispatch_workflow run %d: status=%s, conclusion=%s", runID, status, conclusion) + + switch { + case status == "completed" && conclusion == "success": + report.Result = OutcomeAccepted + report.Detail = "workflow run completed with success" + case status == "completed" && (conclusion == "failure" || conclusion == "timed_out" || conclusion == "cancelled" || conclusion == "action_required"): + // action_required means the run is blocked and requires manual intervention; + // treat it as rejected rather than ignored since it does not self-resolve. + report.Result = OutcomeRejected + report.Detail = "workflow run completed with " + conclusion + case status == "completed": + // neutral and skipped indicate the run did not contribute meaningful output. + report.Result = OutcomeIgnored + report.Detail = "workflow run completed with " + conclusion + default: + report.Result = OutcomePending + report.Detail = "workflow run status: " + status + } + return report +} + +// evalUpdateDiscussion checks whether a discussion edit stuck. +// Full evaluation requires GraphQL (same pattern as evalCloseDiscussion). +// Until the GraphQL evaluator is implemented this returns OutcomeIgnored so that +// callers do not retry indefinitely. +// Spec: specs/safe-output-outcome-evaluation.md §12 +func evalUpdateDiscussion(item CreatedItemReport, repoOverride string) OutcomeReport { + return OutcomeReport{ + Type: item.Type, + ObjectURL: item.URL, + Repo: resolveItemRepo(item, repoOverride), + Result: OutcomeIgnored, + Detail: "discussion update check requires GraphQL (not yet implemented); outcome is advisory only", + } +} diff --git a/pkg/cli/outcome_eval_workflow_test.go b/pkg/cli/outcome_eval_workflow_test.go new file mode 100644 index 00000000000..65aa5c45ac9 --- /dev/null +++ b/pkg/cli/outcome_eval_workflow_test.go @@ -0,0 +1,206 @@ +//go:build !integration + +package cli + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestEvalDispatchWorkflowNoRunID(t *testing.T) { + // No metadata at all → pending with an informative detail message. + report := evalDispatchWorkflow(CreatedItemReport{ + Type: "dispatch_workflow", + Repo: "owner/repo", + }, "owner/repo") + + assert.Equal(t, OutcomePending, report.Result) + assert.Contains(t, report.Detail, "no run ID available") +} + +func TestEvalDispatchWorkflowRunIDZero(t *testing.T) { + // run_id present but zero → treated as missing. + report := evalDispatchWorkflow(CreatedItemReport{ + Type: "dispatch_workflow", + Repo: "owner/repo", + Metadata: map[string]any{"run_id": float64(0)}, + }, "owner/repo") + + assert.Equal(t, OutcomePending, report.Result) +} + +func TestEvalDispatchWorkflowAPIError(t *testing.T) { + old := workflowOutcomeGHAPIGet + t.Cleanup(func() { workflowOutcomeGHAPIGet = old }) + workflowOutcomeGHAPIGet = func(_ string, _ string) (map[string]any, error) { + return nil, errors.New("connection refused") + } + + report := evalDispatchWorkflow(CreatedItemReport{ + Type: "dispatch_workflow", + Repo: "owner/repo", + Metadata: map[string]any{"run_id": float64(12345678)}, + }, "owner/repo") + + assert.Equal(t, OutcomeError, report.Result) + assert.Contains(t, report.EvalError, "connection refused") +} + +func TestEvalDispatchWorkflowCompletedSuccess(t *testing.T) { + old := workflowOutcomeGHAPIGet + t.Cleanup(func() { workflowOutcomeGHAPIGet = old }) + workflowOutcomeGHAPIGet = func(_ string, _ string) (map[string]any, error) { + return map[string]any{ + "status": "completed", + "conclusion": "success", + }, nil + } + + report := evalDispatchWorkflow(CreatedItemReport{ + Type: "dispatch_workflow", + Repo: "owner/repo", + Metadata: map[string]any{"run_id": float64(12345678)}, + }, "owner/repo") + + assert.Equal(t, OutcomeAccepted, report.Result) + assert.Contains(t, report.Detail, "success") +} + +func TestEvalDispatchWorkflowCompletedFailure(t *testing.T) { + for _, conclusion := range []string{"failure", "timed_out", "cancelled"} { + t.Run(conclusion, func(t *testing.T) { + old := workflowOutcomeGHAPIGet + t.Cleanup(func() { workflowOutcomeGHAPIGet = old }) + workflowOutcomeGHAPIGet = func(_ string, _ string) (map[string]any, error) { + return map[string]any{ + "status": "completed", + "conclusion": conclusion, + }, nil + } + + report := evalDispatchWorkflow(CreatedItemReport{ + Type: "dispatch_workflow", + Repo: "owner/repo", + Metadata: map[string]any{"run_id": float64(99999)}, + }, "owner/repo") + + assert.Equal(t, OutcomeRejected, report.Result) + assert.Contains(t, report.Detail, conclusion) + }) + } +} + +func TestEvalDispatchWorkflowCompletedOtherConclusion(t *testing.T) { + old := workflowOutcomeGHAPIGet + t.Cleanup(func() { workflowOutcomeGHAPIGet = old }) + workflowOutcomeGHAPIGet = func(_ string, _ string) (map[string]any, error) { + return map[string]any{ + "status": "completed", + "conclusion": "skipped", + }, nil + } + + report := evalDispatchWorkflow(CreatedItemReport{ + Type: "dispatch_workflow", + Repo: "owner/repo", + Metadata: map[string]any{"run_id": float64(42)}, + }, "owner/repo") + + assert.Equal(t, OutcomeIgnored, report.Result) + assert.Contains(t, report.Detail, "skipped") +} + +func TestEvalDispatchWorkflowInProgress(t *testing.T) { + old := workflowOutcomeGHAPIGet + t.Cleanup(func() { workflowOutcomeGHAPIGet = old }) + workflowOutcomeGHAPIGet = func(_ string, _ string) (map[string]any, error) { + return map[string]any{ + "status": "in_progress", + "conclusion": "", + }, nil + } + + report := evalDispatchWorkflow(CreatedItemReport{ + Type: "dispatch_workflow", + Repo: "owner/repo", + Metadata: map[string]any{"run_id": float64(77)}, + }, "owner/repo") + + assert.Equal(t, OutcomePending, report.Result) + assert.Contains(t, report.Detail, "in_progress") +} + +func TestEvalDispatchWorkflowRunIDInt64(t *testing.T) { + // run_id supplied as int64 (not float64) must be handled without panic. + old := workflowOutcomeGHAPIGet + t.Cleanup(func() { workflowOutcomeGHAPIGet = old }) + workflowOutcomeGHAPIGet = func(_ string, _ string) (map[string]any, error) { + return map[string]any{ + "status": "completed", + "conclusion": "success", + }, nil + } + + report := evalDispatchWorkflow(CreatedItemReport{ + Type: "dispatch_workflow", + Repo: "owner/repo", + Metadata: map[string]any{"run_id": int64(9876543210)}, + }, "owner/repo") + + assert.Equal(t, OutcomeAccepted, report.Result) +} + +func TestEvalDispatchWorkflowFloat64OverflowGuard(t *testing.T) { + // A float64 value above 2^53 cannot represent integers exactly and must be + // treated as an invalid run_id (OutcomePending) rather than silently truncated. + // Use 2^53 + 2 (= 9007199254740994): consecutive integers around 2^53 collapse + // to the same float64, so this value would be mangled if cast to int64 directly. + aboveMaxSafeInt := float64(maxSafeFloat64Int) + 2 + + report := evalDispatchWorkflow(CreatedItemReport{ + Type: "dispatch_workflow", + Repo: "owner/repo", + Metadata: map[string]any{"run_id": aboveMaxSafeInt}, + }, "owner/repo") + + assert.Equal(t, OutcomePending, report.Result, + "a float64 run_id above 2^53 must be treated as invalid (OutcomePending)") +} + +func TestEvalDispatchWorkflowActionRequired(t *testing.T) { + // action_required is a blocking conclusion that requires manual intervention; + // it must map to OutcomeRejected (not OutcomeIgnored). + old := workflowOutcomeGHAPIGet + t.Cleanup(func() { workflowOutcomeGHAPIGet = old }) + workflowOutcomeGHAPIGet = func(_ string, _ string) (map[string]any, error) { + return map[string]any{ + "status": "completed", + "conclusion": "action_required", + }, nil + } + + report := evalDispatchWorkflow(CreatedItemReport{ + Type: "dispatch_workflow", + Repo: "owner/repo", + Metadata: map[string]any{"run_id": float64(12345678)}, + }, "owner/repo") + + assert.Equal(t, OutcomeRejected, report.Result, + "action_required conclusion must map to OutcomeRejected") + assert.Contains(t, report.Detail, "action_required") +} + +func TestEvalUpdateDiscussionReturnsIgnored(t *testing.T) { + // evalUpdateDiscussion must return OutcomeIgnored (not OutcomePending) so that + // callers do not enter an infinite retry loop waiting for a terminal status. + report := evalUpdateDiscussion(CreatedItemReport{ + Type: "update_discussion", + URL: "https://github.com/owner/repo/discussions/1", + }, "owner/repo") + + assert.Equal(t, OutcomeIgnored, report.Result, + "evalUpdateDiscussion must return OutcomeIgnored to prevent infinite retry") + assert.NotEmpty(t, report.Detail) +} diff --git a/pkg/intent/policy.go b/pkg/intent/policy.go new file mode 100644 index 00000000000..f0aa4e18132 --- /dev/null +++ b/pkg/intent/policy.go @@ -0,0 +1,321 @@ +package intent + +import "slices" + +// autonomyOrder maps autonomy level names to their restrictiveness rank. +// Higher rank means more restrictive. +var autonomyOrder = map[string]int{ + "propose_only": 3, + "supervised": 2, + "bounded": 1, + "": 0, +} + +// writeScopeOrder maps write_scope values to their restrictiveness rank. +// Higher rank means more restrictive. +var writeScopeOrder = map[string]int{ + "none": 3, + "feature_branch": 2, + "any_branch": 1, + "": 0, +} + +// scopePriorityOrder maps scope names to their precedence rank. +// Higher rank means the scope takes precedence over lower ranks. +// Rules must be applied from highest to lowest precedence so that a higher- +// precedence scope (e.g. organization) seeds the policy before lower-precedence +// rules (e.g. intent) can only narrow it. +var scopePriorityOrder = map[string]int{ + "organization": 4, // highest: org-wide rules always dominate + "repository": 3, // repo-specific constraints narrow org rules + "intent": 2, // intent-level rules narrow within a repo + "workflow": 1, // workflow defaults are lowest named scope + "": 0, // unscoped rules are lowest priority of all +} + +// ExecutionPolicy governs what an agent may do for a given intent. +// +// WARNING: PolicyCompiler is advisory only. All fields except Autonomy are +// compiled and recorded for audit but are NOT yet wired into runtime enforcement. +// Do not rely on this policy to gate actual tool calls or merge operations until +// Authorizer.AuthorizeTool is implemented and integrated into the execution path. +type ExecutionPolicy struct { + Autonomy string `json:"autonomy"` + + // AllowedTools controls which tools the agent may call. + // nil means unrestricted; []string{} (non-nil empty) means deny-all; non-empty + // means restricted to the listed tools. JSON omitempty cannot preserve the + // nil-vs-empty distinction; callers must check AllowedTools != nil at runtime. + AllowedTools []string `json:"allowed_tools,omitempty"` + DeniedTools []string `json:"denied_tools,omitempty"` + + WriteScope string `json:"write_scope"` + + RequiredChecks []string `json:"required_checks,omitempty"` + + HumanApprovalRequired bool `json:"human_approval_required"` + + // AutoMergeAllowed uses a pointer so that an unset rule fragment (nil) is + // distinguishable from an explicit denial (false). The merge logic only applies + // the AND (more-restrictive) step when at least one side has an explicit value. + // nil means the rule did not express a preference; false is an explicit denial; + // true is an explicit grant. + AutoMergeAllowed *bool `json:"auto_merge_allowed,omitempty"` + + MaxAttempts int `json:"max_attempts"` + + RuleIDs []string `json:"rule_ids,omitempty"` +} + +// RepositoryContext carries repository-level context used when matching policy rules. +type RepositoryContext struct { + Owner string `json:"owner,omitempty"` + Name string `json:"name,omitempty"` + Visibility string `json:"visibility,omitempty"` // "public" or "private" + Org string `json:"org,omitempty"` +} + +// PolicyRule pairs a match condition with a policy fragment to apply. +type PolicyRule struct { + ID string `json:"id"` + Scope string `json:"scope,omitempty"` // "organization", "repository", "intent", or "workflow" + When PolicyCondition `json:"when"` + Set ExecutionPolicy `json:"set"` +} + +// PolicyCondition describes when a rule applies. +type PolicyCondition struct { + Domain string `json:"domain,omitempty"` + Priority string `json:"priority,omitempty"` + Risk string `json:"risk,omitempty"` + Org string `json:"org,omitempty"` +} + +// Matches returns true when the condition is satisfied by the given intent and repository. +// Labels are matched as flat strings. If a record carries labels like ["security","p1","critical"], +// the Domain/Priority/Risk fields each check for the presence of their value anywhere in that +// slice. Callers must ensure label values are unique across dimensions (e.g. no priority value +// that could collide with a domain value) to avoid false positives. +func (c PolicyCondition) Matches(record IntentRecord, repo RepositoryContext) bool { + if c.Domain != "" && !slices.Contains(record.Labels, c.Domain) { + return false + } + if c.Priority != "" && !slices.Contains(record.Labels, c.Priority) { + return false + } + if c.Risk != "" && !slices.Contains(record.Labels, c.Risk) { + return false + } + if c.Org != "" && c.Org != repo.Org && c.Org != repo.Owner { + return false + } + return true +} + +// PolicyCompiler compiles a set of rules into an ExecutionPolicy for a given intent. +// Rules are sorted by scope precedence (organization > repository > intent > workflow) +// before merging; within the same scope, declaration order is preserved. +// +// WARNING: the compiled policy is advisory only. Runtime enforcement is not yet +// wired to the orchestrator — see the intent-attribution-agent-governance spec for +// the required follow-up before treating compiled policies as a security gate. +type PolicyCompiler struct { + Rules []PolicyRule +} + +// Compile returns the most restrictive policy produced by merging all matching rules. +// If no rules match, the safe fail-closed default is returned. When rules do match, +// they are first sorted by scope precedence so that organization constraints seed the +// policy before repository or intent rules can only narrow them. The first rule's +// policy acts as the baseline (so rules can express less-restrictive-than-safe values +// such as supervised autonomy or auto_merge_allowed=true); subsequent rules are merged +// using more-restrictive-wins logic. Fields left unset by all rules receive fail-closed +// defaults so that an incomplete rule set never grants open access. +func (c *PolicyCompiler) Compile(record IntentRecord, repo RepositoryContext) ExecutionPolicy { + var matched []PolicyRule + for _, rule := range c.Rules { + if rule.When.Matches(record, repo) { + matched = append(matched, rule) + } + } + + if len(matched) == 0 { + return safestDefaultPolicy() + } + + // Sort by scope precedence (highest first) so that organization rules seed + // the policy before repository or intent rules. slices.SortStableFunc preserves + // declaration order within the same scope. + slices.SortStableFunc(matched, func(a, b PolicyRule) int { + return scopePriorityOrder[b.Scope] - scopePriorityOrder[a.Scope] + }) + + // Seed from the first matching rule's policy fragment, not from the safest default. + // This allows higher-precedence rules to establish a less-restrictive-than-safe + // baseline (e.g. supervised autonomy) that lower-precedence rules can only tighten. + policy := matched[0].Set + policy.RuleIDs = []string{matched[0].ID} + + for _, rule := range matched[1:] { + policy = mergePolicy(policy, rule.Set) + policy.RuleIDs = append(policy.RuleIDs, rule.ID) + } + + return applyFailClosedDefaults(policy) +} + +// applyFailClosedDefaults fills in safe fail-closed values for any ExecutionPolicy field +// that rules left unset (at its zero value). This ensures an incomplete rule set never +// inadvertently grants open access. +// +// String fields (Autonomy, WriteScope) and MaxAttempts use non-zero safe defaults, so +// an unset zero value is detected and replaced. HumanApprovalRequired is also set here: +// its safe default (true) differs from its zero value (false), so any compiled policy +// that has not explicitly set it via the OR merge path is upgraded to true (fail-closed). +// AutoMergeAllowed uses *bool; nil (unset) is replaced with false (deny auto-merge). +func applyFailClosedDefaults(p ExecutionPolicy) ExecutionPolicy { + safe := safestDefaultPolicy() + if p.Autonomy == "" { + p.Autonomy = safe.Autonomy + } + if p.WriteScope == "" { + p.WriteScope = safe.WriteScope + } + // HumanApprovalRequired: override false (zero) to the safe default (true). + // Rules that want to allow unapproved execution must set AutoMergeAllowed instead; + // this field cannot currently be relaxed below the fail-closed default. + if !p.HumanApprovalRequired { + p.HumanApprovalRequired = safe.HumanApprovalRequired + } + // AutoMergeAllowed: nil means no rule expressed a preference; default to false. + if p.AutoMergeAllowed == nil { + p.AutoMergeAllowed = safe.AutoMergeAllowed + } + if p.MaxAttempts == 0 { + p.MaxAttempts = safe.MaxAttempts + } + return p +} + +// safestDefaultPolicy returns the most restrictive ExecutionPolicy baseline. +// Unknown or ambiguous intent must not grant elevated authority. +func safestDefaultPolicy() ExecutionPolicy { + return ExecutionPolicy{ + Autonomy: "propose_only", + WriteScope: "none", + HumanApprovalRequired: true, + AutoMergeAllowed: new(false), + MaxAttempts: 1, + } +} + +// mergePolicy merges a new policy fragment into an existing accumulated policy, +// always preserving the more restrictive value for each field. +// +// The existing policy represents already-accumulated (higher-precedence) constraints. +// The incoming fragment represents a lower-precedence rule's desired settings. +// The result must never be less restrictive than the existing policy. +func mergePolicy(existing, incoming ExecutionPolicy) ExecutionPolicy { + result := existing + + // Autonomy: keep the more restrictive level. + if autonomyOrder[incoming.Autonomy] > autonomyOrder[existing.Autonomy] { + result.Autonomy = incoming.Autonomy + } + + // WriteScope: keep the more restrictive scope. + if writeScopeOrder[incoming.WriteScope] > writeScopeOrder[existing.WriteScope] { + result.WriteScope = incoming.WriteScope + } + + // HumanApprovalRequired: true is more restrictive; use OR. + if incoming.HumanApprovalRequired { + result.HumanApprovalRequired = true + } + + // AutoMergeAllowed uses *bool so that "not set" (nil) is distinct from "explicitly + // denied" (false). More-restrictive-wins semantics: + // nil + nil → nil (neither expressed a preference) + // nil + *v → *v (adopt the incoming explicit value) + // *v + nil → *v (keep the existing explicit value) + // *true + *true → *true + // any + *false → *false (false is more restrictive) + // *false + any → *false + switch { + case existing.AutoMergeAllowed == nil && incoming.AutoMergeAllowed == nil: + // leave result.AutoMergeAllowed as nil + case existing.AutoMergeAllowed == nil: + result.AutoMergeAllowed = incoming.AutoMergeAllowed + case incoming.AutoMergeAllowed == nil: + // result already holds existing.AutoMergeAllowed from `result := existing` above + default: + // both sides have explicit values; AND (false wins) + v := *existing.AutoMergeAllowed && *incoming.AutoMergeAllowed + result.AutoMergeAllowed = new(v) + } + + // MaxAttempts: lower is more restrictive; keep the minimum of both if both are set. + if incoming.MaxAttempts > 0 { + if result.MaxAttempts == 0 || incoming.MaxAttempts < result.MaxAttempts { + result.MaxAttempts = incoming.MaxAttempts + } + } + + // RequiredChecks: union of both lists (adding checks is always more restrictive). + for _, check := range incoming.RequiredChecks { + if !slices.Contains(result.RequiredChecks, check) { + result.RequiredChecks = append(result.RequiredChecks, check) + } + } + + // DeniedTools: union of both lists (denying more tools is always more restrictive). + for _, tool := range incoming.DeniedTools { + if !slices.Contains(result.DeniedTools, tool) { + result.DeniedTools = append(result.DeniedTools, tool) + } + } + + // AllowedTools uses nil vs non-nil to distinguish unrestricted from restricted: + // nil → unrestricted (no tool filter) + // []string{} → deny-all (empty set; more restrictive than any non-empty set) + // non-empty → restricted to the listed tools + // + // More-restrictive-wins semantics: + // unrestricted + unrestricted → unrestricted (nil) + // unrestricted + restricted → adopt the restriction + // restricted + unrestricted → keep the restriction + // restricted_A + restricted_B → intersection(A, B); empty intersection → deny-all ([]string{}) + // any combination with deny-all → deny-all + existingRestricts := existing.AllowedTools != nil + incomingRestricts := incoming.AllowedTools != nil + + switch { + case !existingRestricts && !incomingRestricts: + result.AllowedTools = nil + case !existingRestricts: + result.AllowedTools = slices.Clone(incoming.AllowedTools) + case !incomingRestricts: + // result was initialized from existing at the top of this function (`result := existing`), + // so result.AllowedTools already holds the existing restriction. No change needed. + case len(existing.AllowedTools) == 0 || len(incoming.AllowedTools) == 0: + // At least one side is deny-all; deny-all is always more restrictive. + result.AllowedTools = []string{} + default: + // Both sides restrict to non-empty sets; use the intersection. + var intersection []string + for _, tool := range existing.AllowedTools { + if slices.Contains(incoming.AllowedTools, tool) { + intersection = append(intersection, tool) + } + } + // An empty intersection means no tool satisfies both restrictions: deny all. + // Use []string{} (non-nil) to signal deny-all rather than nil (unrestricted). + if intersection == nil { + result.AllowedTools = []string{} + } else { + result.AllowedTools = intersection + } + } + + return result +} diff --git a/pkg/intent/policy_test.go b/pkg/intent/policy_test.go new file mode 100644 index 00000000000..5bd1f3e2c76 --- /dev/null +++ b/pkg/intent/policy_test.go @@ -0,0 +1,368 @@ +//go:build !integration + +package intent_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/github/gh-aw/pkg/intent" +) + +// boolPtr returns a pointer to b, used to set *bool fields in ExecutionPolicy literals. +func boolPtr(b bool) *bool { return new(b) } + +// Tests for PolicyCompiler.Compile() covering organization > repository > intent +// precedence. A lower-precedence rule MUST NOT weaken a constraint imposed by a +// higher-precedence rule. +// +// Spec (intent-attribution-agent-governance.md §Policy precedence): +// +// organization constraints +// > repository constraints +// > intent-specific rules +// > workflow defaults +// > agent request + +// TestPolicyCompilerOrgConstraintsPreservedOverRepo verifies that organization-scoped +// constraints cannot be weakened by a later repository-scoped rule. +// +// Scenario: an org rule denies a dangerous tool and requires human approval; +// a repository rule then tries to remove the denied tool and clear the approval +// requirement. The compiled policy MUST preserve both constraints from the org rule. +func TestPolicyCompilerOrgConstraintsPreservedOverRepo(t *testing.T) { + compiler := &intent.PolicyCompiler{ + Rules: []intent.PolicyRule{ + { + ID: "org-security-policy", + Scope: "organization", + When: intent.PolicyCondition{}, + Set: intent.ExecutionPolicy{ + HumanApprovalRequired: true, + AutoMergeAllowed: boolPtr(false), + DeniedTools: []string{"delete_repository", "push_direct_to_main"}, + RequiredChecks: []string{"security-scan"}, + MaxAttempts: 2, + }, + }, + { + ID: "repo-permissive-policy", + Scope: "repository", + When: intent.PolicyCondition{}, + Set: intent.ExecutionPolicy{ + // Lower-precedence attempt to weaken org constraints: + HumanApprovalRequired: false, // tries to remove approval requirement + AutoMergeAllowed: boolPtr(true), // tries to enable auto-merge + DeniedTools: []string{}, // tries to clear denied tools + RequiredChecks: []string{}, // tries to clear required checks + MaxAttempts: 10, // tries to increase max attempts + }, + }, + }, + } + + record := intent.IntentRecord{Status: intent.AttributionMapped} + repo := intent.RepositoryContext{Owner: "github", Name: "gh-aw"} + + policy := compiler.Compile(record, repo) + + // Org rule's HumanApprovalRequired=true must NOT be cleared by the repo rule. + assert.True(t, policy.HumanApprovalRequired, + "organization HumanApprovalRequired=true must not be weakened by repository rule") + + // Org rule's AutoMergeAllowed=false must NOT be enabled by the repo rule. + require.NotNil(t, policy.AutoMergeAllowed, + "AutoMergeAllowed must be explicitly set by the org rule") + assert.False(t, *policy.AutoMergeAllowed, + "organization AutoMergeAllowed=false must not be enabled by repository rule") + + // Org's denied tools must be preserved even when the repo rule passes an empty list. + assert.Contains(t, policy.DeniedTools, "delete_repository", + "organization denied tool 'delete_repository' must not be removed by repository rule") + assert.Contains(t, policy.DeniedTools, "push_direct_to_main", + "organization denied tool 'push_direct_to_main' must not be removed by repository rule") + + // Org's required check must be preserved. + assert.Contains(t, policy.RequiredChecks, "security-scan", + "organization required check 'security-scan' must not be removed by repository rule") + + // MaxAttempts must not be increased beyond the org limit. + // Use Equal (not LessOrEqual) to verify the org rule's value (2) was actually applied, + // not silently replaced by the safe default (1). + assert.Equal(t, 2, policy.MaxAttempts, + "organization MaxAttempts=2 must be applied and not raised by repository rule") + + // Both rules should appear in the applied rule IDs. + require.Contains(t, policy.RuleIDs, "org-security-policy") + require.Contains(t, policy.RuleIDs, "repo-permissive-policy") +} + +// TestPolicyCompilerRepoConstraintsPreservedOverIntent verifies that repository-scoped +// constraints cannot be weakened by a lower-precedence intent-specific rule. +// +// Scenario: a repository rule enforces AllowedTools to a restricted set and adds +// required checks; an intent-specific rule for "low" priority tries to widen +// AllowedTools and does not add checks. The repo's AllowedTools restriction and +// required checks must be preserved. +func TestPolicyCompilerRepoConstraintsPreservedOverIntent(t *testing.T) { + compiler := &intent.PolicyCompiler{ + Rules: []intent.PolicyRule{ + { + ID: "repo-tool-restriction", + Scope: "repository", + When: intent.PolicyCondition{}, + Set: intent.ExecutionPolicy{ + AllowedTools: []string{"issue_read", "list_issues", "list_prs"}, + RequiredChecks: []string{"unit-tests", "lint"}, + MaxAttempts: 3, + }, + }, + { + ID: "intent-low-priority", + Scope: "intent", + When: intent.PolicyCondition{ + Priority: "low", + }, + Set: intent.ExecutionPolicy{ + // Lower-precedence attempt to widen tool access and add a check. + AllowedTools: []string{"issue_read", "list_issues", "list_prs", "create_pr"}, + RequiredChecks: []string{"docs-build"}, + MaxAttempts: 5, + }, + }, + }, + } + + record := intent.IntentRecord{ + Status: intent.AttributionMapped, + Labels: []string{"low"}, + } + repo := intent.RepositoryContext{Owner: "github", Name: "gh-aw"} + + policy := compiler.Compile(record, repo) + + // AllowedTools: the repo restricts to 3 tools; intent tries to add "create_pr". + // The intersection (repo's tighter set) must be used — "create_pr" must NOT appear. + assert.NotContains(t, policy.AllowedTools, "create_pr", + "intent rule must not expand AllowedTools beyond what the repository rule allows") + assert.Contains(t, policy.AllowedTools, "issue_read", + "tools allowed by both repo and intent rules must be preserved") + assert.Contains(t, policy.AllowedTools, "list_issues", + "tools allowed by both repo and intent rules must be preserved") + + // RequiredChecks: union of both rules; all checks must be present. + assert.Contains(t, policy.RequiredChecks, "unit-tests", + "repository required check must be preserved") + assert.Contains(t, policy.RequiredChecks, "lint", + "repository required check must be preserved") + assert.Contains(t, policy.RequiredChecks, "docs-build", + "intent required check must be added to the union") + + // MaxAttempts: intent tries to raise it; repo's tighter limit must win. + assert.LessOrEqual(t, policy.MaxAttempts, 3, + "repository MaxAttempts=3 must not be increased by lower-precedence intent rule") + + // Both rules matched. + require.Contains(t, policy.RuleIDs, "repo-tool-restriction") + require.Contains(t, policy.RuleIDs, "intent-low-priority") +} + +// TestPolicyCompilerNoRulesMatchReturnsSafeDefault verifies that when no rules +// match the intent, the compiled policy equals the safe fail-closed default. +// +// Spec: "Unknown or ambiguous intent must not grant elevated authority." +func TestPolicyCompilerNoRulesMatchReturnsSafeDefault(t *testing.T) { + compiler := &intent.PolicyCompiler{ + Rules: []intent.PolicyRule{ + { + ID: "security-critical-rule", + Scope: "intent", + When: intent.PolicyCondition{ + Domain: "security", + Priority: "critical", + }, + Set: intent.ExecutionPolicy{ + RequiredChecks: []string{"security-tests", "dependency-review"}, + DeniedTools: []string{"delete_repository"}, + MaxAttempts: 2, + }, + }, + { + ID: "documentation-low-risk", + Scope: "intent", + When: intent.PolicyCondition{ + Domain: "documentation", + }, + Set: intent.ExecutionPolicy{ + RequiredChecks: []string{"docs-build"}, + MaxAttempts: 3, + }, + }, + }, + } + + // Ambiguous intent has no matching labels for either rule. + record := intent.IntentRecord{ + Status: intent.AttributionAmbiguous, + Source: intent.SourceNone, + Labels: nil, + } + repo := intent.RepositoryContext{Owner: "github", Name: "gh-aw"} + + policy := compiler.Compile(record, repo) + + // No rules matched → safe default must govern the policy. + assert.Equal(t, "propose_only", policy.Autonomy, + "ambiguous intent with no matching rules must produce propose_only autonomy") + assert.Equal(t, "none", policy.WriteScope, + "ambiguous intent with no matching rules must produce write_scope=none") + assert.True(t, policy.HumanApprovalRequired, + "ambiguous intent with no matching rules must require human approval") + assert.False(t, policy.AutoMergeAllowed == nil || *policy.AutoMergeAllowed, + "ambiguous intent with no matching rules must not allow auto-merge") + assert.Equal(t, 1, policy.MaxAttempts, + "ambiguous intent with no matching rules must produce max_attempts=1") + assert.Empty(t, policy.RuleIDs, + "no applied rule IDs should be recorded when no rules match") +} + +// TestPolicyCompilerRulesCanGrantLessRestrictiveThanSafeDefault verifies that +// matching rules can produce a policy less restrictive than the safe default +// (e.g. supervised autonomy, auto_merge_allowed=true, max_attempts>1). +// Previously the compiler always seeded from safestDefaultPolicy(), making this +// impossible. +func TestPolicyCompilerRulesCanGrantLessRestrictiveThanSafeDefault(t *testing.T) { + compiler := &intent.PolicyCompiler{ + Rules: []intent.PolicyRule{ + { + ID: "supervised-docs-rule", + Scope: "intent", + When: intent.PolicyCondition{ + Domain: "documentation", + }, + Set: intent.ExecutionPolicy{ + Autonomy: "supervised", + WriteScope: "feature_branch", + AutoMergeAllowed: boolPtr(true), + HumanApprovalRequired: false, + MaxAttempts: 5, + }, + }, + }, + } + + record := intent.IntentRecord{ + Status: intent.AttributionMapped, + // Labels carries the domain value used by PolicyCondition.Matches to satisfy + // the When.Domain="documentation" condition on the rule above. + Labels: []string{"documentation"}, + } + repo := intent.RepositoryContext{Owner: "github", Name: "gh-aw"} + + policy := compiler.Compile(record, repo) + + assert.Equal(t, "supervised", policy.Autonomy, + "a matching rule must be able to grant supervised autonomy (not just propose_only)") + assert.Equal(t, "feature_branch", policy.WriteScope, + "a matching rule must be able to grant feature_branch write scope") + require.NotNil(t, policy.AutoMergeAllowed, + "AutoMergeAllowed must be explicitly set by the matching rule") + assert.True(t, *policy.AutoMergeAllowed, + "a matching rule must be able to enable auto_merge_allowed") + assert.Equal(t, 5, policy.MaxAttempts, + "a matching rule must be able to set max_attempts > 1") + assert.Contains(t, policy.RuleIDs, "supervised-docs-rule") +} + +// TestPolicyCompilerScopeOrderingEnforced verifies that rules are applied in +// scope-precedence order (organization > repository > intent) regardless of +// the declaration order in the Rules slice. A lower-precedence scope declared +// first must not override a higher-precedence scope declared last. +func TestPolicyCompilerScopeOrderingEnforced(t *testing.T) { + // Rules are intentionally listed in reverse precedence order: intent first, + // then repository, then organization. The compiled policy must still apply + // them highest-precedence-first (org seeds the policy, intent only narrows). + compiler := &intent.PolicyCompiler{ + Rules: []intent.PolicyRule{ + { + // Declared first but lowest precedence: should NOT seed the policy. + ID: "intent-rule", + Scope: "intent", + When: intent.PolicyCondition{}, + Set: intent.ExecutionPolicy{ + Autonomy: "bounded", + MaxAttempts: 10, + }, + }, + { + ID: "repo-rule", + Scope: "repository", + When: intent.PolicyCondition{}, + Set: intent.ExecutionPolicy{ + Autonomy: "supervised", + MaxAttempts: 5, + }, + }, + { + // Declared last but highest precedence: MUST seed the policy. + ID: "org-rule", + Scope: "organization", + When: intent.PolicyCondition{}, + Set: intent.ExecutionPolicy{ + Autonomy: "propose_only", + MaxAttempts: 2, + }, + }, + }, + } + + record := intent.IntentRecord{Status: intent.AttributionMapped} + repo := intent.RepositoryContext{Owner: "github", Name: "gh-aw"} + + policy := compiler.Compile(record, repo) + + // The org rule (propose_only, MaxAttempts=2) must dominate. + // If scope ordering were ignored, the intent rule (bounded, MaxAttempts=10) would + // seed the policy and produce a different autonomy level. + assert.Equal(t, "propose_only", policy.Autonomy, + "organization scope must take precedence over lower scopes") + assert.Equal(t, 2, policy.MaxAttempts, + "organization MaxAttempts=2 must win; declaration order must not override scope precedence") +} + +// rules restrict AllowedTools to non-overlapping sets, the compiled policy denies +// all tools ([]string{} sentinel) rather than silently reverting to unrestricted (nil). +func TestPolicyCompilerAllowedToolsDenyAllOnEmptyIntersection(t *testing.T) { + compiler := &intent.PolicyCompiler{ + Rules: []intent.PolicyRule{ + { + ID: "repo-allows-read-tools", + Scope: "repository", + When: intent.PolicyCondition{}, + Set: intent.ExecutionPolicy{ + AllowedTools: []string{"issue_read", "list_issues"}, + }, + }, + { + ID: "intent-allows-write-tools", + Scope: "intent", + When: intent.PolicyCondition{}, + Set: intent.ExecutionPolicy{ + AllowedTools: []string{"create_pr", "push_branch"}, + }, + }, + }, + } + + record := intent.IntentRecord{Status: intent.AttributionMapped} + repo := intent.RepositoryContext{Owner: "github", Name: "gh-aw"} + + policy := compiler.Compile(record, repo) + + // The intersection is empty (no tool appears in both sets). + // Result MUST be deny-all (non-nil empty slice), not unrestricted (nil). + assert.Equal(t, []string{}, policy.AllowedTools, + "non-overlapping AllowedTools sets must produce deny-all ([]string{}), not unrestricted (nil)") +} diff --git a/specs/github-mcp-access-control-compliance/README.md b/specs/github-mcp-access-control-compliance/README.md index 7cfb8b308a7..2eac652a074 100644 --- a/specs/github-mcp-access-control-compliance/README.md +++ b/specs/github-mcp-access-control-compliance/README.md @@ -13,9 +13,13 @@ that implementations satisfy the normative requirements in §§4–10 of the spe |---|---|---| | `exact-match-allow.yaml` | Exact repository pattern allows matching repo | T-GH-011, T-GH-012 | | `wildcard-deny.yaml` | Owner-wildcard pattern denies non-matching owner | T-GH-013, T-GH-014 | +| `empty-repos-block.yaml` | Absent or empty `repos` list denies all repository access | T-GH-015, T-GH-016 | | `role-deny.yaml` | Role filter denies access when user role is insufficient | T-GH-019, T-GH-020 | +| `tool-name-filter.yaml` | `allowed-tools` filter allows or denies by tool name | T-GH-031, T-GH-032, T-GH-033 | +| `blocked-user-deny.yaml` | `blocked-users` denies listed actors unconditionally | T-GH-071, T-GH-072 | | `private-repo-block.yaml` | `private-repos: false` blocks access to private repository | T-GH-024, T-GH-025 | | `integrity-level-block.yaml` | `min-integrity: approved` blocks content below the threshold | T-GH-051, T-GH-052 | +| `combined-filter-allow.yaml` | All access-control conditions must be jointly satisfied | T-GH-081, T-GH-082, T-GH-083 | ## Fixture Schema @@ -35,6 +39,23 @@ expected: reason: string # Expected denial reason substring (informative) ``` +## Error Code Reference + +When `expected.decision` is `deny`, the fixture records the MCP JSON-RPC error code that the +implementation MUST return. The codes used in these fixtures are: + +| Code | Denial Reason | +|---|---| +| `-32001` | Repository not on the allowlist (`repos` filter) | +| `-32002` | User role is insufficient (`role` filter) | +| `-32003` | Repository is private and `private-repos: false` | +| `-32004` | User is blocked (`blocked-users` filter) | +| `-32005` | Tool name not permitted (`allowed-tools` filter) | +| `-32006` | Content integrity level below threshold (`min-integrity` filter) | + +A `null` error code in `expected.error_code` means the scenario produces an `allow` decision +and no error is returned. + ## Adding New Fixtures 1. Copy the most relevant existing fixture file. diff --git a/specs/github-mcp-access-control-compliance/blocked-user-deny.yaml b/specs/github-mcp-access-control-compliance/blocked-user-deny.yaml new file mode 100644 index 00000000000..b934a97e0b6 --- /dev/null +++ b/specs/github-mcp-access-control-compliance/blocked-user-deny.yaml @@ -0,0 +1,64 @@ +# Blocked User Deny — Compliance Fixture +# Tests: T-GH-071, T-GH-072 +# Spec: §7 User and Actor Restrictions, §7.2 Blocked Users + +fixture_id: "blocked-user-deny" +description: > + When a user is listed in the `blocked-users` field, the tool MUST deny all requests + from that user regardless of repository, role, or integrity status (T-GH-071). + A user not in the blocked-users list MUST be processed through the normal access + decision flow (T-GH-072). + +spec_refs: + - "§7.2 — blocked-users MUST deny access unconditionally for listed actors" + - "§7.2 — T-GH-071: blocked user is denied regardless of other conditions" + - "§7.2 — T-GH-072: non-blocked user proceeds through normal access decision" + +scenarios: + # --- Scenario A: request from a blocked user is denied --- + - scenario_id: "blocked-user-deny-A" + description: "A user in blocked-users is denied even when repo matches and role is sufficient" + input: + tool_config: + repos: + - "github/gh-aw" + roles: + - "write" + private-repos: true + min-integrity: "unapproved" + blocked-users: + - "bad-actor" + request: + repository: "github/gh-aw" + user_login: "bad-actor" + user_role: "write" + is_private: true + content_integrity: "approved" + expected: + decision: deny + error_code: -32004 + reason: "user is blocked" + + # --- Scenario B: request from a non-blocked user proceeds normally --- + - scenario_id: "blocked-user-deny-B" + description: "A user NOT in blocked-users is allowed when all other conditions are satisfied" + input: + tool_config: + repos: + - "github/gh-aw" + roles: + - "write" + private-repos: true + min-integrity: "unapproved" + blocked-users: + - "bad-actor" + request: + repository: "github/gh-aw" + user_login: "good-contributor" + user_role: "write" + is_private: true + content_integrity: "unapproved" + expected: + decision: allow + error_code: null + reason: "" diff --git a/specs/github-mcp-access-control-compliance/combined-filter-allow.yaml b/specs/github-mcp-access-control-compliance/combined-filter-allow.yaml new file mode 100644 index 00000000000..7b22d48f016 --- /dev/null +++ b/specs/github-mcp-access-control-compliance/combined-filter-allow.yaml @@ -0,0 +1,121 @@ +# Combined Multi-Filter Allow — Compliance Fixture +# Tests: T-GH-081, T-GH-082, T-GH-083 +# Spec: §10 Combined Condition Evaluation + +fixture_id: "combined-filter-allow" +description: > + When multiple access control conditions are present (repository scope, role filter, + private-repos, min-integrity), ALL conditions MUST be satisfied for a request to be + allowed (T-GH-081). Failure of any single condition MUST produce a deny decision + even when all other conditions are satisfied (T-GH-082). The deny error code MUST + correspond to the first failing condition in evaluation order (T-GH-083). + +spec_refs: + - "§10 — Combined conditions require all to be satisfied for allow" + - "§10.1 — T-GH-081: request satisfying all conditions is allowed" + - "§10.2 — T-GH-082: request failing any single condition is denied" + - "§10.3 — T-GH-083: error code reflects the failing condition type" + +scenarios: + # --- Scenario A: all conditions satisfied → allowed --- + - scenario_id: "combined-filter-allow-A" + description: "Request satisfying all four conditions (repo, role, visibility, integrity) is allowed" + input: + tool_config: + repos: + - "github/gh-aw" + roles: + - "write" + private-repos: true + min-integrity: "approved" + request: + repository: "github/gh-aw" + user_role: "write" + is_private: true + content_integrity: "approved" + expected: + decision: allow + error_code: null + reason: "" + + # --- Scenario B: repo condition fails → denied --- + - scenario_id: "combined-filter-allow-B" + description: "Request with wrong repository is denied even when role, visibility, and integrity match" + input: + tool_config: + repos: + - "github/gh-aw" + roles: + - "write" + private-repos: true + min-integrity: "approved" + request: + repository: "github/other-repo" + user_role: "write" + is_private: true + content_integrity: "approved" + expected: + decision: deny + error_code: -32001 + reason: "repository not in allowed list" + + # --- Scenario D: role condition fails → denied --- + - scenario_id: "combined-filter-allow-D" + description: "Request with insufficient role is denied even when repo, visibility, and integrity match" + input: + tool_config: + repos: + - "github/gh-aw" + roles: + - "write" + private-repos: true + min-integrity: "approved" + request: + repository: "github/gh-aw" + user_role: "read" + is_private: true + content_integrity: "approved" + expected: + decision: deny + error_code: -32002 + reason: "user role does not meet minimum required role" + + # --- Scenario E: private-repos condition fails → denied --- + - scenario_id: "combined-filter-allow-E" + description: "Request targeting a public repo is denied when private-repos=true requires a private repository" + input: + tool_config: + repos: + - "github/gh-aw" + roles: + - "write" + private-repos: true + min-integrity: "approved" + request: + repository: "github/gh-aw" + user_role: "write" + is_private: false + content_integrity: "approved" + expected: + decision: deny + error_code: -32003 + reason: "repository visibility does not satisfy private-repos requirement" + - scenario_id: "combined-filter-allow-C" + description: "Request with insufficient integrity is denied even when repo, role, and visibility match" + input: + tool_config: + repos: + - "github/gh-aw" + roles: + - "write" + private-repos: true + min-integrity: "approved" + request: + repository: "github/gh-aw" + user_role: "write" + is_private: true + content_integrity: "none" + expected: + decision: deny + error_code: -32006 + reason: "content integrity below minimum required level" diff --git a/specs/github-mcp-access-control-compliance/empty-repos-block.yaml b/specs/github-mcp-access-control-compliance/empty-repos-block.yaml new file mode 100644 index 00000000000..ac051158a5e --- /dev/null +++ b/specs/github-mcp-access-control-compliance/empty-repos-block.yaml @@ -0,0 +1,45 @@ +# Empty Repos Block — Compliance Fixture +# Tests: T-GH-015, T-GH-016 +# Spec: §5 Repository Scoping, §5.3 Empty or Absent Repository List + +fixture_id: "empty-repos-block" +description: > + When the `repos` list is absent or empty, the tool configuration MUST deny access + to all repositories (T-GH-015, T-GH-016). An absent repos field is equivalent to + an empty allow-list: no repository matches and every request is denied. + +spec_refs: + - "§5.3 — An absent or empty repos list MUST deny all repository access" + - "§5.3 — T-GH-015: absent repos list denies access" + - "§5.3 — T-GH-016: empty repos list denies access" + +scenarios: + # --- Scenario A: absent repos field denies all repositories --- + - scenario_id: "empty-repos-block-A" + description: "Tool config with no repos field denies request for any repository" + input: + tool_config: + # repos field is intentionally absent + min-integrity: "unapproved" + request: + repository: "github/gh-aw" + content_integrity: "unapproved" + expected: + decision: deny + error_code: -32001 + reason: "repository not in allowed list" + + # --- Scenario B: empty repos list denies all repositories --- + - scenario_id: "empty-repos-block-B" + description: "Tool config with an empty repos: [] denies request for any repository" + input: + tool_config: + repos: [] + min-integrity: "unapproved" + request: + repository: "github/gh-aw" + content_integrity: "unapproved" + expected: + decision: deny + error_code: -32001 + reason: "repository not in allowed list" diff --git a/specs/github-mcp-access-control-compliance/tool-name-filter.yaml b/specs/github-mcp-access-control-compliance/tool-name-filter.yaml new file mode 100644 index 00000000000..15b8e1fc3a5 --- /dev/null +++ b/specs/github-mcp-access-control-compliance/tool-name-filter.yaml @@ -0,0 +1,75 @@ +# Tool Name Filter — Compliance Fixture +# Tests: T-GH-031, T-GH-032, T-GH-033 +# Spec: §6 Tool-Level Filtering, §6.1 Allowed Tool Names + +fixture_id: "tool-name-filter" +description: > + When `allowed-tools` is configured, the MCP gateway MUST allow only the explicitly + listed tool names (T-GH-031). A request invoking a tool not in the allowed list + MUST be denied with error code -32005 (T-GH-032). When `allowed-tools` is absent, + all tool names are permitted subject to other access controls (T-GH-033). + +spec_refs: + - "§6.1 — allowed-tools restricts which tool names are callable" + - "§6.1 — T-GH-031: listed tool name is allowed" + - "§6.1 — T-GH-032: unlisted tool name is denied" + - "§6.1 — T-GH-033: absent allowed-tools permits all tool names" + +scenarios: + # --- Scenario A: allowed tool name passes the filter --- + - scenario_id: "tool-name-filter-A" + description: "Tool 'issue_read' is allowed when allowed-tools contains 'issue_read'" + input: + tool_config: + repos: + - "*/*" + allowed-tools: + - "issue_read" + - "list_issues" + min-integrity: "unapproved" + request: + repository: "example/repo" + tool_name: "issue_read" + content_integrity: "unapproved" + expected: + decision: allow + error_code: null + reason: "" + + # --- Scenario B: unlisted tool name is denied --- + - scenario_id: "tool-name-filter-B" + description: "Tool 'delete_repository' is denied when not in allowed-tools" + input: + tool_config: + repos: + - "*/*" + allowed-tools: + - "issue_read" + - "list_issues" + min-integrity: "unapproved" + request: + repository: "example/repo" + tool_name: "delete_repository" + content_integrity: "unapproved" + expected: + decision: deny + error_code: -32005 + reason: "tool not in allowed-tools list" + + # --- Scenario C: absent allowed-tools permits all tool names --- + - scenario_id: "tool-name-filter-C" + description: "Any tool name is permitted when allowed-tools is not configured" + input: + tool_config: + repos: + - "*/*" + # no allowed-tools field + min-integrity: "unapproved" + request: + repository: "example/repo" + tool_name: "delete_repository" + content_integrity: "unapproved" + expected: + decision: allow + error_code: null + reason: "" diff --git a/specs/intent-attribution-agent-governance.md b/specs/intent-attribution-agent-governance.md index 0095a785d94..5184edce5df 100644 --- a/specs/intent-attribution-agent-governance.md +++ b/specs/intent-attribution-agent-governance.md @@ -33,6 +33,45 @@ The central principle is: The system does not claim that GitHub labels or merged pull requests prove business impact, ROI, or realized customer value. +## Conformance + +The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this document are to be interpreted as described in [RFC 2119](https://www.ietf.org/rfc/rfc2119.txt). + +### RFC 2119 Norms + +#### Attribution-Resolution Order + +An implementation MUST resolve attribution in the following precedence order: + +1. Explicit intent metadata attached to the artifact (`ExplicitIntent` on the pull request). +2. A single closing issue linked to the pull request. +3. Pull request labels used as an artifact-label fallback when no closing issue is present. + +An implementation MUST NOT skip earlier sources in favor of later sources unless the earlier source is unavailable or explicitly absent. + +An implementation MUST NOT mix sources across precedence levels for a single attribution record. Each record MUST be attributed to exactly one source. + +#### Ambiguous-Root Handling + +An implementation MUST produce an `ambiguous` attribution when two or more distinct closing issues are linked to the same pull request and no explicit intent override is present. + +An implementation MUST NOT resolve ambiguity by arbitrary selection (e.g., first, last, or random issue). + +An ambiguous attribution MUST be recorded with `status: "ambiguous"` and `source: "closing_issue"`. + +An ambiguous attribution MUST NOT be treated as equivalent to a mapped attribution for reporting or authorization purposes. + +#### Fail-Closed Behavior + +An implementation MUST apply the safest available execution policy when the intent is `unlinked`, `ambiguous`, or otherwise indeterminate. + +The safest available policy MUST be: autonomy `propose_only`, write scope `none`, `human_approval_required: true`, `auto_merge_allowed: false`, `max_attempts: 1`. + +An implementation MUST NOT grant elevated authority based on absent or unresolved attribution. + +A policy decision MUST be deterministic: given identical attribution inputs, the same policy MUST always be produced. + + ## Current implementation The existing implementation provides the initial attribution and reporting foundation: @@ -291,7 +330,116 @@ Separating dimensions prevents initiatives, priorities, domains, and risk labels Only the configured scoring dimension contributes to weight. -## Intent record +### `.github/intent-policy.json` Schema + +The future `.github/intent-policy.json` configuration file supersedes `.github/objective-mapping.json` for repositories that require policy governance in addition to attribution. + +**Top-level fields:** + +| Field | Type | Required | Description | +|-------|------|----------|-------------| +| `version` | integer | Yes | Schema version. Current stable value: `1`. | +| `labels` | object | Yes | Map of GitHub label name → label descriptor (see below). | +| `scoring` | object | No | Scoring strategy for weighted attribution reporting. | +| `attribution` | object | No | Attribution-resolution behaviour overrides. | +| `rules` | array | No | Ordered policy rules compiled into an `ExecutionPolicy`. | + +**`labels` descriptor fields:** + +| Field | Type | Required | Description | +|-------|------|----------|-------------| +| `dimension` | string | Yes | One of `"priority"`, `"domain"`, `"risk"`, `"initiative"`. Controls which axis the label belongs to. | +| `value` | string | Yes | Canonical value for this label within its dimension (e.g., `"critical"`, `"security"`). | +| `weight` | integer | No | Numeric weight for scoring. Only meaningful on the active scoring dimension. | + +**`scoring` fields:** + +| Field | Type | Required | Description | +|-------|------|----------|-------------| +| `dimension` | string | Yes | The dimension used for weight computation (e.g., `"priority"`). | +| `strategy` | string | Yes | One of `"max"` (use the highest weight found) or `"sum"` (add all weights). | + +**`attribution` fields:** + +| Field | Type | Required | Description | +|-------|------|----------|-------------| +| `multiple_roots` | string | No | Behaviour when multiple closing issues are found. `"ambiguous"` (default) or `"first"`. MUST be `"ambiguous"` for governance use. | +| `allow_artifact_label_fallback` | boolean | No | Whether PR labels may be used when no closing issue is present. Default: `true`. | + +**`rules` array element fields:** + +| Field | Type | Required | Description | +|-------|------|----------|-------------| +| `id` | string | Yes | Stable unique identifier for the rule (e.g., `"security-critical"`). Used in policy decision provenance. | +| `scope` | string | No | Hint for evaluation ordering: `"organization"`, `"repository"`, `"intent"`, or `"workflow"`. Rules MUST be listed from highest to lowest precedence. | +| `when` | object | No | Match conditions. All specified fields must match. An empty `when` matches all intents. | +| `set` | object | Yes | `ExecutionPolicy` fragment to merge when the rule matches. | + +**`when` match condition fields:** + +| Field | Type | Description | +|-------|------|-------------| +| `domain` | string | Match if any label with `dimension: "domain"` has this value. | +| `priority` | string | Match if any label with `dimension: "priority"` has this value. | +| `risk` | string | Match if any label with `dimension: "risk"` has this value. | +| `org` | string | Match if the repository's owner or org equals this value. | + +**`set` ExecutionPolicy fragment fields:** + +| Field | Type | Description | +|-------|------|-------------| +| `autonomy` | string | `"propose_only"`, `"supervised"`, or `"bounded"`. | +| `write_scope` | string | `"none"`, `"feature_branch"`, or `"any_branch"`. | +| `allowed_tools` | array of string | Tool names the agent may call. Empty means unrestricted. | +| `denied_tools` | array of string | Tool names the agent must never call. Union with higher-precedence denials. | +| `required_checks` | array of string | Check names that must pass before completion. Union with higher-precedence checks. | +| `human_approval_required` | boolean | Whether a human must approve before the agent proceeds with write operations. | +| `auto_merge_allowed` | boolean | Whether the agent may auto-merge pull requests after checks pass. | +| `max_attempts` | integer | Maximum number of times the agent may retry the workflow. | + +**Example `.github/intent-policy.json`:** + +```json +{ + "version": 1, + "labels": { + "critical": { "dimension": "priority", "value": "critical", "weight": 100 }, + "p1": { "dimension": "priority", "value": "high", "weight": 50 }, + "security": { "dimension": "domain", "value": "security" }, + "documentation": { "dimension": "domain", "value": "documentation" } + }, + "scoring": { "dimension": "priority", "strategy": "max" }, + "attribution": { "multiple_roots": "ambiguous", "allow_artifact_label_fallback": true }, + "rules": [ + { + "id": "security-critical", + "scope": "intent", + "when": { "domain": "security", "priority": "critical" }, + "set": { + "autonomy": "supervised", + "human_approval_required": true, + "auto_merge_allowed": false, + "required_checks": ["unit-tests", "security-tests"], + "max_attempts": 2 + } + }, + { + "id": "documentation-safe", + "scope": "intent", + "when": { "domain": "documentation" }, + "set": { + "autonomy": "bounded", + "write_scope": "feature_branch", + "human_approval_required": false, + "auto_merge_allowed": true, + "required_checks": ["docs-build"], + "max_attempts": 3 + } + } + ] +} +``` + ```go type IntentRecord struct { @@ -743,7 +891,26 @@ func (a *Authorizer) AuthorizeTool( The agent must not be able to modify or expand its own policy. -## Outcome evaluation +### `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. + +| `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. | +| `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`. | +| `AutoMergeAllowed` | **Not wired** | Not enforced by the orchestrator. | +| `RequiredChecks` | **Not wired** | Not checked before workflow execution. | +| `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. + +**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. + Initial observable rules: diff --git a/specs/otel-observability-spec.md b/specs/otel-observability-spec.md index ccfd0a92433..990caf4fc2f 100644 --- a/specs/otel-observability-spec.md +++ b/specs/otel-observability-spec.md @@ -849,6 +849,21 @@ Level 1 and Level 2 compatibility validation MUST cover the following behaviors: The repository enforcement entry point for these checks is `make validate-otel-contract`. This target MUST remain focused on the customer-facing compatibility contract rather than all possible OTEL-related tests. +### 17.1.1 Test ID Stubs: Level 1 Compliance + +The following test IDs are stubs for Level 1 (Stable Configuration and Export) compliance tests. Implementations MUST provide tests that correspond to each stub before claiming Level 1 conformance. + +| Test ID | Area | Description | +|---------|------|-------------| +| **T-OT-001** | Compiler config | Compiler accepts `observability.otlp.endpoint` with a valid HTTPS URL and emits `OTEL_EXPORTER_OTLP_ENDPOINT` in the generated workflow environment. | +| **T-OT-002** | Compiler config | Compiler rejects `observability.otlp.endpoint` set to a non-HTTPS URL when `if-missing: block` is configured, producing a descriptive validation error. | +| **T-OT-003** | Endpoint normalization | When multiple endpoints are declared, the compiler preserves them in declaration order and retains the first endpoint in the `OTEL_EXPORTER_OTLP_ENDPOINT` variable for backward compatibility. | +| **T-OT-004** | OTLP export | The runtime JavaScript helper encodes span payloads as valid OTLP/HTTP protobuf-JSON and POSTs them to the configured endpoint; a successful 200 response is recognized as accepted. | +| **T-OT-005** | Trace context | The compiler injects `GITHUB_AW_OTEL_TRACE_ID`, `GITHUB_AW_OTEL_PARENT_SPAN_ID`, and `TRACEPARENT` into the generated workflow environment when OTLP observability is enabled. | +| **T-OT-006** | Local mirrors | The runtime helper writes each exported span as a raw OTLP/HTTP JSON line with a `resourceSpans` key to `/tmp/gh-aw/otel.jsonl`; the file format MUST NOT be an envelope-only summary. | +| **T-OT-007** | Compiler config | `observability.otlp.headers` entries are emitted as `OTEL_EXPORTER_OTLP_HEADERS` in `key=value,key=value` format and are masked in diagnostics, job summaries, and artifacts. | + + ### 17.2 Optional Extension Tests An implementation claiming Level 3 MUST add automated tests for every Level 3 feature it enables, including any OpenTelemetry-native root spans, full-duration job spans, metrics, structured logs, outcome span links, Collector mode, or versioned mirror companion files. diff --git a/specs/replace-label-compliance/README.md b/specs/replace-label-compliance/README.md new file mode 100644 index 00000000000..232bc956ed3 --- /dev/null +++ b/specs/replace-label-compliance/README.md @@ -0,0 +1,62 @@ +# Replace-Label Compliance Fixtures + +This directory contains compliance fixtures for the normative requirements of the +[Replace-Label Specification](../replace-label-spec.md). + +Each fixture describes test scenarios with input configurations and the expected +access-control decisions. Fixtures cover the glob-matching and blocklist-ordering +requirements defined in §4 of the specification. + +## Fixture Files + +| Filename | Scenario | Spec Coverage | +|---|---|---| +| `rl-001-glob-semantics.yaml` | Glob pattern matching for `allowed-add`, `allowed-remove`, and `blocked` follows gobwas/glob semantics | RL-001, T-RL-020–T-RL-023 | +| `rl-003-blocklist-ordering.yaml` | Blocklist evaluation occurs before allowlist evaluation (security boundary) | RL-003, T-RL-023–T-RL-024 | + +## Fixture Schema + +Each fixture file is a YAML document with the following top-level keys: + +```yaml +fixture_id: string # Unique identifier referencing the RL requirement code +description: string # Human-readable scenario description +spec_refs: # Normative requirements under test (RL codes and § references) + - string +scenarios: + - scenario_id: string # Unique sub-scenario identifier + description: string # Sub-scenario description + input: + safe_output_config: # replace-label safe-output configuration under test + allowed-add: [...] + allowed-remove: [...] + blocked: [...] + message: # Simulated agent message + label_to_add: string + label_to_remove: string + expected: + decision: allow | deny # Required outcome + error_code: integer | null # Expected error code on deny + reason: string # Expected denial reason substring (informative) +``` + +## Adding New Fixtures + +1. Copy the most relevant existing fixture file. +2. Assign a new `fixture_id` matching the RL requirement code being tested. +3. Update `input.safe_output_config` and `input.message` to reflect the new scenario. +4. Set `expected` fields to match the required outcome. +5. Register the new fixture in the table above and reference it from §9 of + `specs/replace-label-spec.md`. + +## Related Test IDs + +The following test IDs defined in the replace-label specification map to these fixtures: + +| Test ID | Fixture | Description | +|---------|---------|-------------| +| T-RL-020 | `rl-001-glob-semantics.yaml` | Star glob matches label name substring | +| T-RL-021 | `rl-001-glob-semantics.yaml` | Exact pattern matches only exact name | +| T-RL-022 | `rl-001-glob-semantics.yaml` | Character class pattern matches correctly | +| T-RL-023 | `rl-001-glob-semantics.yaml`, `rl-003-blocklist-ordering.yaml` | Glob pattern rejects non-matching label; blocked label rejected even when allowed | +| T-RL-024 | `rl-003-blocklist-ordering.yaml` | Blocked label rejected even with wildcard allowed-add | diff --git a/specs/replace-label-compliance/rl-001-glob-semantics.yaml b/specs/replace-label-compliance/rl-001-glob-semantics.yaml new file mode 100644 index 00000000000..8551258d710 --- /dev/null +++ b/specs/replace-label-compliance/rl-001-glob-semantics.yaml @@ -0,0 +1,87 @@ +# RL-001 Glob Semantics — Compliance Fixture +# Tests: T-RL-020, T-RL-021, T-RL-022, T-RL-023 +# Spec: §4.1 Glob Pattern Matching (RL-001) + +fixture_id: "rl-001-glob-semantics" +description: > + Glob pattern matching for `allowed-add`, `allowed-remove`, and `blocked` MUST follow + gobwas/glob semantics: `*` matches any sequence of characters within a label name, + and `[...]` denotes a character class (RL-001). + +spec_refs: + - "RL-001 — Glob pattern matching MUST use gobwas/glob semantics" + - "§4.1 — allowed-add and allowed-remove glob evaluation" + - "§4.2 — blocked glob evaluation" + - "T-RL-020 — Star glob matches any sequence within a label name" + - "T-RL-021 — Exact pattern matches only the exact label name" + - "T-RL-022 — Character class pattern matches a label in the class" + - "T-RL-023 — Glob pattern does NOT match when no characters satisfy the pattern" + +scenarios: + # --- Scenario A: star glob matches a label name containing extra characters --- + - scenario_id: "rl-001-glob-star-match" + description: "allowed-add pattern 'priority-*' allows adding 'priority-high'" + input: + safe_output_config: + allowed-add: + - "priority-*" + allowed-remove: [] + blocked: [] + message: + label_to_add: "priority-high" + label_to_remove: "" + expected: + decision: allow + error_code: null + reason: "" + + # --- Scenario B: star glob does NOT match a completely different prefix --- + - scenario_id: "rl-001-glob-star-no-match" + description: "allowed-add pattern 'priority-*' rejects adding 'bug'" + input: + safe_output_config: + allowed-add: + - "priority-*" + allowed-remove: [] + blocked: [] + message: + label_to_add: "bug" + label_to_remove: "" + expected: + decision: deny + error_code: -32002 + reason: "label not in allowed-add list" + + # --- Scenario C: character class pattern matches --- + - scenario_id: "rl-001-glob-char-class-match" + description: "allowed-add pattern 'p[0-9]' allows adding 'p1'" + input: + safe_output_config: + allowed-add: + - "p[0-9]" + allowed-remove: [] + blocked: [] + message: + label_to_add: "p1" + label_to_remove: "" + expected: + decision: allow + error_code: null + reason: "" + + # --- Scenario D: exact pattern matches only the exact label --- + - scenario_id: "rl-001-exact-no-glob" + description: "allowed-remove pattern 'bug' rejects removing 'bug-fix' (exact match, no wildcard)" + input: + safe_output_config: + allowed-add: [] + allowed-remove: + - "bug" + blocked: [] + message: + label_to_add: "" + label_to_remove: "bug-fix" + expected: + decision: deny + error_code: -32002 + reason: "label not in allowed-remove list" diff --git a/specs/replace-label-compliance/rl-003-blocklist-ordering.yaml b/specs/replace-label-compliance/rl-003-blocklist-ordering.yaml new file mode 100644 index 00000000000..9450f3f3912 --- /dev/null +++ b/specs/replace-label-compliance/rl-003-blocklist-ordering.yaml @@ -0,0 +1,97 @@ +# RL-003 Blocklist Ordering — Compliance Fixture +# Tests: T-RL-023, T-RL-024 +# Spec: §4.2 Blocklist Enforcement (RL-003) + +fixture_id: "rl-003-blocklist-ordering" +description: > + A label MUST NOT match any pattern in the `blocked` list. Blocklist evaluation MUST + occur before allowlist evaluation (it is a security boundary). A label matching a + blocked pattern MUST be rejected immediately, regardless of any allowlist entry (RL-003). + +spec_refs: + - "RL-003 — Blocklist evaluation MUST occur before allowlist evaluation" + - "RL-003 — A blocked label MUST be rejected regardless of allowlist entries" + - "§4.2 — Security boundary: blocklist checked first" + - "T-RL-023 — Blocked label is rejected even when also in allowed-add" + - "T-RL-024 — Blocked label is rejected even when allowed-add contains a wildcard" + +scenarios: + # --- Scenario A: blocked label rejected even when it appears in allowed-add --- + - scenario_id: "rl-003-block-overrides-allow" + description: > + A label that matches both allowed-add and blocked MUST be rejected. + Blocklist evaluation occurs first and is a security boundary. + input: + safe_output_config: + allowed-add: + - "security" + allowed-remove: [] + blocked: + - "security" + message: + label_to_add: "security" + label_to_remove: "" + expected: + decision: deny + error_code: -32003 + reason: "label matches blocked pattern" + + # --- Scenario B: wildcard blocked pattern overrides wildcard allowed-add pattern --- + - scenario_id: "rl-003-wildcard-block-overrides-wildcard-allow" + description: > + A label matching a blocked wildcard 'security-*' MUST be rejected even when + the same label matches an allowed-add wildcard '*'. + input: + safe_output_config: + allowed-add: + - "*" + allowed-remove: [] + blocked: + - "security-*" + message: + label_to_add: "security-critical" + label_to_remove: "" + expected: + decision: deny + error_code: -32003 + reason: "label matches blocked pattern" + + # --- Scenario C: non-blocked label is allowed by wildcard --- + - scenario_id: "rl-003-unblocked-label-allowed" + description: > + A label that does NOT match any blocked pattern passes through to allowlist + evaluation and is allowed when it matches allowed-add. + input: + safe_output_config: + allowed-add: + - "*" + allowed-remove: [] + blocked: + - "security-*" + message: + label_to_add: "bug" + label_to_remove: "" + expected: + decision: allow + error_code: null + reason: "" + + # --- Scenario D: blocked remove pattern prevents label removal --- + - scenario_id: "rl-003-block-prevents-removal" + description: > + A blocked pattern applies equally to removal operations. Attempting to remove + a label matching a blocked pattern MUST be rejected. + input: + safe_output_config: + allowed-add: [] + allowed-remove: + - "*" + blocked: + - "~*" + message: + label_to_add: "" + label_to_remove: "~internal" + expected: + decision: deny + error_code: -32003 + reason: "label matches blocked pattern" diff --git a/specs/safe-output-outcome-evaluation.md b/specs/safe-output-outcome-evaluation.md index 4ca6904803e..17a825bacad 100644 --- a/specs/safe-output-outcome-evaluation.md +++ b/specs/safe-output-outcome-evaluation.md @@ -97,7 +97,7 @@ Rows marked `evalGenericSticky` fallback are generic existence checks, not type- | `close_pull_request` | `evalCloseSticky` | still closed | | `close_discussion` | `evalCloseDiscussion` | none yet | | `create_discussion` | `evalCreateDiscussion` | none yet | -| `update_discussion` | `evalGenericSticky` fallback | discussion target exists | +| `update_discussion` | `evalUpdateDiscussion` | none yet (pending GraphQL) | | `create_pull_request_review_comment` | `evalReviewComment` | none yet | | `submit_pull_request_review` | dedicated review evaluator | review affected PR lifecycle | | `reply_to_pull_request_review_comment` | `evalGenericSticky` fallback | review target exists | @@ -105,12 +105,13 @@ Rows marked `evalGenericSticky` fallback are generic existence checks, not type- | `push_to_pull_request_branch` | `evalPushToPRBranch` | merged | | `mark_pull_request_as_ready_for_review` | `evalMarkReady` | reviewed | | `assign_to_agent` | `evalAssignToAgent` | merged or completed | -| `dispatch_workflow` | `evalGenericSticky` fallback | dispatch target exists | +| `dispatch_workflow` | `evalDispatchWorkflow` | workflow run completed with success | | `autofix_code_scanning_alert` | `evalGenericSticky` fallback | alert target exists | | `create_code_scanning_alert` | `evalGenericSticky` fallback | alert target exists | | `link_sub_issue` | `evalGenericSticky` fallback | sub-issue link target exists | | `hide_comment` | `evalHideComment` | none yet | | `assign_milestone` | `evalAssignMilestone` | milestone still set | +| `replace_label` | `evalGenericSticky` fallback | label target still exists | | `update_project` | `evalGenericSticky` fallback | object still exists | | `update_release` | `evalGenericSticky` fallback | object still exists | | `noop` | explicit skip | skipped | @@ -129,7 +130,7 @@ Rows marked `evalGenericSticky` fallback are generic existence checks, not type- | `close_pull_request` | partial | `pkg/workflow/safe_outputs_dispatch.go`, `pkg/cli/outcome_eval.go` (`evalCloseSticky`) | `actions/setup/js/close_pull_request.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `close_discussion` | partial | `pkg/workflow/safe_outputs_dispatch.go`, `pkg/cli/outcome_eval.go` (`evalCloseDiscussion`) | `actions/setup/js/close_discussion.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `create_discussion` | partial | `pkg/workflow/safe_outputs_dispatch.go`, `pkg/cli/outcome_eval.go` (`evalCreateDiscussion`) | `actions/setup/js/create_discussion.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | -| `update_discussion` | not-started | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval.go` (`evalGenericSticky` fallback) | `actions/setup/js/update_discussion.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | +| `update_discussion` | partial | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval_workflow.go` (`evalUpdateDiscussion`) | `actions/setup/js/update_discussion.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `create_pull_request_review_comment` | partial | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval.go` (`evalReviewComment`) | `actions/setup/js/create_pr_review_comment.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `submit_pull_request_review` | implemented | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval_review.go` | `actions/setup/js/submit_pr_review.cjs`, `actions/setup/js/evaluate_outcomes.cjs` | | `reply_to_pull_request_review_comment` | not-started | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval.go` (`evalGenericSticky` fallback) | `actions/setup/js/reply_to_pr_review_comment.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | @@ -137,12 +138,13 @@ Rows marked `evalGenericSticky` fallback are generic existence checks, not type- | `push_to_pull_request_branch` | partial | `pkg/workflow/push_to_pull_request_branch_validation.go`, `pkg/cli/outcome_eval.go` (`evalPushToPRBranch`) | `actions/setup/js/push_to_pull_request_branch.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `mark_pull_request_as_ready_for_review` | partial | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval.go` (`evalMarkReady`) | `actions/setup/js/mark_pull_request_as_ready_for_review.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `assign_to_agent` | partial | `pkg/workflow/safe_outputs_dispatch.go`, `pkg/cli/outcome_eval.go` (`evalAssignToAgent`) | `actions/setup/js/assign_to_agent.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | -| `dispatch_workflow` | not-started | `pkg/workflow/dispatch_workflow.go`, `pkg/cli/outcome_eval.go` (`evalGenericSticky` fallback) | `actions/setup/js/dispatch_workflow.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | +| `dispatch_workflow` | partial | `pkg/workflow/dispatch_workflow.go`, `pkg/cli/outcome_eval_workflow.go` (`evalDispatchWorkflow`) | `actions/setup/js/dispatch_workflow.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `autofix_code_scanning_alert` | not-started | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval.go` (`evalGenericSticky` fallback) | `actions/setup/js/autofix_code_scanning_alert.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `create_code_scanning_alert` | not-started | `pkg/workflow/create_code_scanning_alert.go`, `pkg/cli/outcome_eval.go` (`evalGenericSticky` fallback) | `actions/setup/js/create_code_scanning_alert.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `link_sub_issue` | not-started | `pkg/workflow/link_sub_issue.go`, `pkg/cli/outcome_eval.go` (`evalGenericSticky` fallback) | `actions/setup/js/link_sub_issue.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `hide_comment` | partial | `pkg/workflow/hide_comment.go`, `pkg/cli/outcome_eval.go` (`evalHideComment`) | `actions/setup/js/hide_comment.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `assign_milestone` | partial | `pkg/workflow/assign_milestone.go`, `pkg/cli/outcome_eval.go` (`evalAssignMilestone`) | `actions/setup/js/assign_milestone.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | +| `replace_label` | not-started | `pkg/workflow/replace_label.go`, `pkg/cli/outcome_eval.go` (`evalGenericSticky` fallback) | `actions/setup/js/replace_label.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `update_project` | not-started | `pkg/workflow/update_project.go`, `pkg/cli/outcome_eval.go` (`evalGenericSticky` fallback) | `actions/setup/js/update_project.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `update_release` | not-started | `pkg/workflow/safe_outputs_config.go`, `pkg/cli/outcome_eval.go` (`evalGenericSticky` fallback) | `actions/setup/js/update_release.cjs`, `actions/setup/js/evaluate_outcomes.cjs` (generic fallback) | | `noop` | implemented | `pkg/cli/outcome_eval.go` (explicit skip in `EvaluateOutcomes`) | `actions/setup/js/evaluate_outcomes.cjs` (`NOOP_TYPES`) |