From 9a6a69502793b5abb309018737066d2b26eaae3c Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 6 Jul 2026 15:36:16 +0000 Subject: [PATCH 1/2] =?UTF-8?q?chore:=20remove=20dead=20functions=20?= =?UTF-8?q?=E2=80=94=205=20functions=20removed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove unreachable functions identified by deadcode static analysis: - NewMultiSelectForm (pkg/console/prompt_form.go) - NewTextForm (pkg/console/prompt_form.go) - SetProcessEnvLookup (pkg/workflow/process_env_lookup.go) - PolicyCompiler.Compile (pkg/intent/policy.go) - PolicyCondition.Matches (pkg/intent/policy.go) Also removed private helpers applyFailClosedDefaults, safestDefaultPolicy, mergePolicy and package-level vars that were exclusively used by the dead Compile/Matches methods, and deleted pkg/intent/policy_test.go which exclusively tested the removed functions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pkg/console/prompt_form.go | 10 - pkg/console/prompt_form_test.go | 8 - pkg/intent/policy.go | 264 --------------------- pkg/intent/policy_test.go | 368 ----------------------------- pkg/workflow/features_test.go | 33 --- pkg/workflow/github_cli_test.go | 53 ----- pkg/workflow/process_env_lookup.go | 18 -- 7 files changed, 754 deletions(-) delete mode 100644 pkg/intent/policy_test.go diff --git a/pkg/console/prompt_form.go b/pkg/console/prompt_form.go index 70b3d529910..01cc90de40d 100644 --- a/pkg/console/prompt_form.go +++ b/pkg/console/prompt_form.go @@ -22,16 +22,6 @@ func NewSelectForm[T comparable](selectField *huh.Select[T]) *huh.Form { return NewForm(huh.NewGroup(selectField)) } -// NewMultiSelectForm creates a themed, accessibility-aware single multi-select form. -func NewMultiSelectForm[T comparable](multiSelect *huh.MultiSelect[T]) *huh.Form { - return NewForm(huh.NewGroup(multiSelect)) -} - -// NewTextForm creates a themed, accessibility-aware single-text form. -func NewTextForm(text *huh.Text) *huh.Form { - return NewForm(huh.NewGroup(text)) -} - // NewConfirmForm creates a themed, accessibility-aware single-confirm form. func NewConfirmForm(confirm *huh.Confirm) *huh.Form { return NewForm(huh.NewGroup(confirm)) diff --git a/pkg/console/prompt_form_test.go b/pkg/console/prompt_form_test.go index bb0cf636e56..654b6a62823 100644 --- a/pkg/console/prompt_form_test.go +++ b/pkg/console/prompt_form_test.go @@ -18,14 +18,6 @@ func TestPromptWrappersReturnNonNilForms(t *testing.T) { Options(huh.NewOption("Option", "option")). Value(&selectValue))) - var multiValue []string - require.NotNil(t, NewMultiSelectForm(huh.NewMultiSelect[string](). - Options(huh.NewOption("Option", "option")). - Value(&multiValue))) - - var textValue string - require.NotNil(t, NewTextForm(huh.NewText().Value(&textValue))) - var confirmValue bool require.NotNil(t, NewConfirmForm(huh.NewConfirm().Value(&confirmValue))) } diff --git a/pkg/intent/policy.go b/pkg/intent/policy.go index 77bbc0dfd0c..a06ff58b883 100644 --- a/pkg/intent/policy.go +++ b/pkg/intent/policy.go @@ -1,44 +1,5 @@ package intent -import ( - "slices" - - "github.com/github/gh-aw/pkg/logger" -) - -var policyLog = logger.New("intent:policy") - -// 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 @@ -97,27 +58,6 @@ type PolicyCondition struct { 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. @@ -128,207 +68,3 @@ func (c PolicyCondition) Matches(record IntentRecord, repo RepositoryContext) bo 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 { - policyLog.Printf("Compiling execution policy: total_rules=%d, repo=%s/%s", len(c.Rules), repo.Owner, repo.Name) - - var matched []PolicyRule - for _, rule := range c.Rules { - if rule.When.Matches(record, repo) { - matched = append(matched, rule) - } - } - - if len(matched) == 0 { - policyLog.Print("No rules matched; returning fail-closed default policy") - 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} - policyLog.Printf("Matched %d rule(s); seeding from rule %s (scope=%q)", len(matched), matched[0].ID, matched[0].Scope) - - for _, rule := range matched[1:] { - policyLog.Printf("Merging rule %s (scope=%q) into accumulated policy", rule.ID, rule.Scope) - policy = mergePolicy(policy, rule.Set) - policy.RuleIDs = append(policy.RuleIDs, rule.ID) - } - - compiled := applyFailClosedDefaults(policy) - policyLog.Printf("Compiled policy: autonomy=%q, write_scope=%q, human_approval=%v, rule_ids=%v", compiled.Autonomy, compiled.WriteScope, compiled.HumanApprovalRequired, compiled.RuleIDs) - return compiled -} - -// 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 deleted file mode 100644 index 5bd1f3e2c76..00000000000 --- a/pkg/intent/policy_test.go +++ /dev/null @@ -1,368 +0,0 @@ -//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/pkg/workflow/features_test.go b/pkg/workflow/features_test.go index 11213c030f7..8b4c8a1ff6a 100644 --- a/pkg/workflow/features_test.go +++ b/pkg/workflow/features_test.go @@ -6,7 +6,6 @@ import ( "testing" "github.com/github/gh-aw/pkg/constants" - "github.com/stretchr/testify/require" ) func TestIsFeatureEnabled(t *testing.T) { @@ -80,38 +79,6 @@ func TestIsFeatureEnabled(t *testing.T) { } } -func TestIsFeatureEnabledUsesConfiguredProcessEnvLookup(t *testing.T) { - t.Run("enabled feature is detected from configured lookup", func(t *testing.T) { - SetProcessEnvLookup(func(key string) (string, bool) { - if key == "GH_AW_FEATURES" { - return "firewall", true - } - return "", false - }) - t.Cleanup(func() { - SetProcessEnvLookup(nil) - }) - - result := isFeatureEnabled(constants.FeatureFlag("firewall"), nil) - require.True(t, result, "isFeatureEnabled should use configured environment lookup") - }) - - t.Run("missing feature remains disabled", func(t *testing.T) { - SetProcessEnvLookup(func(key string) (string, bool) { - if key == "GH_AW_FEATURES" { - return "other-feature", true - } - return "", false - }) - t.Cleanup(func() { - SetProcessEnvLookup(nil) - }) - - result := isFeatureEnabled(constants.FeatureFlag("firewall"), nil) - require.False(t, result, "isFeatureEnabled should be false when configured lookup does not include firewall") - }) -} - func TestIsFeatureEnabledNoEnv(t *testing.T) { result := isFeatureEnabled(constants.FeatureFlag("firewall"), nil) if result != false { diff --git a/pkg/workflow/github_cli_test.go b/pkg/workflow/github_cli_test.go index 78bf92d33fd..b35856a8ef1 100644 --- a/pkg/workflow/github_cli_test.go +++ b/pkg/workflow/github_cli_test.go @@ -112,59 +112,6 @@ func TestExecGH(t *testing.T) { } } -func TestExecGHUsesConfiguredProcessEnvLookup(t *testing.T) { - originalDefaultHost := getDefaultGHHost() - SetDefaultGHHost("configured.ghe.com") - t.Cleanup(func() { - SetDefaultGHHost(originalDefaultHost) - }) - - t.Run("uses configured github token fallback", func(t *testing.T) { - SetProcessEnvLookup(func(key string) (string, bool) { - switch key { - case "GH_TOKEN": - return "", false - case "GITHUB_TOKEN": - return "lookup-token", true - case "GH_HOST": - return "", false - default: - return "", false - } - }) - t.Cleanup(func() { - SetProcessEnvLookup(nil) - }) - - cmd := ExecGH("auth", "status") - require.NotNil(t, cmd.Env) - assert.Contains(t, cmd.Env, "GH_TOKEN=lookup-token") - assert.Contains(t, cmd.Env, "GH_HOST=configured.ghe.com") - }) - - t.Run("does not inject gh token when both tokens are absent", func(t *testing.T) { - t.Setenv("GH_TOKEN", "ambient-gh-token") - t.Setenv("GITHUB_TOKEN", "ambient-github-token") - - SetProcessEnvLookup(func(key string) (string, bool) { - return "", false - }) - t.Cleanup(func() { - SetProcessEnvLookup(nil) - }) - - cmd := ExecGH("auth", "status") - require.NotNil(t, cmd.Env) - assert.False(t, slices.ContainsFunc(cmd.Env, func(e string) bool { - return strings.HasPrefix(e, "GH_TOKEN=") - })) - assert.False(t, slices.ContainsFunc(cmd.Env, func(e string) bool { - return strings.HasPrefix(e, "GITHUB_TOKEN=") - })) - assert.Contains(t, cmd.Env, "GH_HOST=configured.ghe.com") - }) -} - func TestExecGHWithMultipleArgs(t *testing.T) { // Save original environment originalGHToken := os.Getenv("GH_TOKEN") diff --git a/pkg/workflow/process_env_lookup.go b/pkg/workflow/process_env_lookup.go index 55e43266c64..e9370602f61 100644 --- a/pkg/workflow/process_env_lookup.go +++ b/pkg/workflow/process_env_lookup.go @@ -3,12 +3,8 @@ package workflow import ( "os" "sync" - - "github.com/github/gh-aw/pkg/logger" ) -var processEnvLookupLog = logger.New("workflow:process_env_lookup") - type envLookupFunc func(string) (string, bool) var ( @@ -16,20 +12,6 @@ var ( processEnvLookup envLookupFunc = os.LookupEnv ) -// SetProcessEnvLookup configures how workflow helpers resolve environment values. -// Passing nil restores the default process environment lookup. -func SetProcessEnvLookup(lookup func(string) (string, bool)) { - processEnvLookupMu.Lock() - defer processEnvLookupMu.Unlock() - if lookup == nil { - processEnvLookupLog.Print("Restoring default process environment lookup (os.LookupEnv)") - processEnvLookup = os.LookupEnv - return - } - processEnvLookupLog.Print("Installing custom process environment lookup override") - processEnvLookup = lookup -} - func lookupProcessEnv(key string) string { processEnvLookupMu.RLock() defer processEnvLookupMu.RUnlock() From 50a35be8912ec025f5ff8f826e25e1cdef318b4f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 6 Jul 2026 19:28:06 +0000 Subject: [PATCH 2/2] chore: address follow-up dead-code review feedback Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/intent/policy.go | 5 ++--- pkg/workflow/process_env_lookup.go | 16 ++-------------- 2 files changed, 4 insertions(+), 17 deletions(-) diff --git a/pkg/intent/policy.go b/pkg/intent/policy.go index a06ff58b883..d77b98bc530 100644 --- a/pkg/intent/policy.go +++ b/pkg/intent/policy.go @@ -58,9 +58,8 @@ type PolicyCondition struct { Org string `json:"org,omitempty"` } -// 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. +// PolicyCompiler holds policy rules for callers that still exchange policy compiler +// configuration data. // // WARNING: the compiled policy is advisory only. Runtime enforcement is not yet // wired to the orchestrator — see the intent-attribution-agent-governance spec for diff --git a/pkg/workflow/process_env_lookup.go b/pkg/workflow/process_env_lookup.go index e9370602f61..80180433ce0 100644 --- a/pkg/workflow/process_env_lookup.go +++ b/pkg/workflow/process_env_lookup.go @@ -1,22 +1,10 @@ package workflow -import ( - "os" - "sync" -) - -type envLookupFunc func(string) (string, bool) - -var ( - processEnvLookupMu sync.RWMutex - processEnvLookup envLookupFunc = os.LookupEnv -) +import "os" func lookupProcessEnv(key string) string { - processEnvLookupMu.RLock() - defer processEnvLookupMu.RUnlock() // Intentionally ignore the existence flag to preserve os.Getenv semantics: // missing variables and explicitly empty variables are both treated as "". - value, _ := processEnvLookup(key) + value, _ := os.LookupEnv(key) return value }