Skip to content

[dead-code] chore: remove dead functions — 5 functions removed #43807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
Jul 6, 2026
Merged

[dead-code] chore: remove dead functions — 5 functions removed #43807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9a6a695
chore: remove dead functions — 5 functions removed
github-actions[bot] Jul 6, 2026
5df5416
Merge remote-tracking branch 'origin/main' into dead-code-removal-202…
Copilot Jul 6, 2026
50a35be
chore: address follow-up dead-code review feedback
Copilot Jul 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions pkg/console/prompt_form.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
8 changes: 0 additions & 8 deletions pkg/console/prompt_form_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
}
269 changes: 2 additions & 267 deletions pkg/intent/policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -97,238 +58,12 @@ 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.
// 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
// the required follow-up before treating compiled policies as a security gate.
type PolicyCompiler struct {

@github-actions github-actions Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] Stale docstring: PolicyCompiler still claims it "compiles a set of rules" and describes scope-precedence sorting behaviour — but Compile() was just deleted. A future reader will search in vain for the method this comment describes.

💡 Suggested update

Replace the docstring to reflect the struct's current state as a data holder:

// PolicyCompiler holds the rules that will be used to compile an ExecutionPolicy.
// Rules are evaluated with scope precedence (organization > repository > intent > workflow).
//
// WARNING: the compiled policy is advisory only. Runtime enforcement is not yet
// wired to the orchestrator.
type PolicyCompiler struct {
    Rules []PolicyRule
}

Or, if Compile is not coming back soon, simply remove the scope-precedence description entirely since there is no runtime behaviour to document.

@copilot please address this.

Copilot AI Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 50a35be by updating the PolicyCompiler doc comment in pkg/intent/policy.go so it no longer describes the removed compile behavior.

@github-actions github-actions Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty exported struct with misleading doc comment: PolicyCompiler is left exported as a struct with a Rules []PolicyRule field but no methods — its Compile implementation was the entire value proposition, now gone.

💡 Context

The remaining struct and its doc comment ("compiles a set of rules into an ExecutionPolicy") now describe behaviour that no longer exists in the package. PolicyCompiler{Rules: [...]}. has no callable methods — it is a dead configuration struct.

PolicyCondition has the same issue: it describes match-condition semantics but its Matches method was removed. The field documentation (e.g. "Labels are matched as flat strings") on the struct fields is also gone with the method, but the struct itself remains exported.

If PolicyCompiler is genuinely dead (no external callers confirmed), it should be removed in a follow-up, or at minimum the doc comment should be corrected so it doesn't claim to "compile" anything. Leaving it exported invites third-party consumers to depend on a no-op type.

Copilot AI Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 50a35be by correcting the PolicyCompiler doc comment to describe the remaining data-holder role instead of the removed compile behavior.

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