Skip to content

[refactor] Consolidate duplicated copilot-requests: write permission check (3 call sites)Β #44633

Description

@github-actions

πŸ”§ Semantic Function Clustering Analysis

Analysis of repository: github/gh-aw β€” non-test .go files in pkg/workflow, pkg/cli, pkg/parser (plus pkg/stringutil, pkg/console).

Executive summary

I clustered ~5,347 function declarations across 833 non-test Go files by name and purpose, then hunted for duplicate names, functional duplicates, scattered helpers, and file outliers using semantic + naming analysis.

Headline: the codebase is already well-factored. Almost every apparent duplicate turned out to be an intentional pattern (build-tag _wasm.go variants, or thin re-export wrappers over centralized stringutil/timeutil/fileutil helpers). Only one genuine, actionable duplication survived verification. Reporting it plus the false-positive patterns so future runs don't re-flag them.


βœ… Finding 1 (actionable): copilot-requests: write check is implemented 3 times

The logic "does this workflow grant copilot-requests: write?" β€” i.e. permissions.Get(PermissionCopilotRequests) == PermissionWrite β€” is duplicated across three sites with three different input shapes:

# Location Signature / input
1 pkg/workflow/permissions_operations.go:62 hasCopilotRequestsWritePermission(workflowData *WorkflowData) bool
2 pkg/cli/workflow_secrets.go:120 hasCopilotRequestsWritePermission(frontmatter map[string]any) bool
3 pkg/workflow/permissions_compiler_validator.go:221 inline workflowPermissions.Get(PermissionCopilotRequests) + PermissionWrite compare

Each independently does: obtain a *Permissions, then Get(PermissionCopilotRequests) and compare to PermissionWrite.

Side-by-side of the two named functions
// pkg/workflow/permissions_operations.go:62
func hasCopilotRequestsWritePermission(workflowData *WorkflowData) bool {
    if workflowData == nil { return false }
    perms := workflowData.CachedPermissions
    if perms == nil {
        perms = NewPermissionsParser(workflowData.Permissions).ToPermissions()
    }
    if perms == nil { return false }
    level, ok := perms.Get(PermissionCopilotRequests)
    return ok && level == PermissionWrite
}

// pkg/cli/workflow_secrets.go:120
func hasCopilotRequestsWritePermission(frontmatter map[string]any) bool {
    if frontmatter == nil { return false }
    permissionsValue, ok := frontmatter["permissions"]
    if !ok { return false }
    perms := workflow.NewPermissionsParserFromValue(permissionsValue).ToPermissions()
    level, exists := perms.Get(workflow.PermissionCopilotRequests)
    return exists && level == workflow.PermissionWrite
}

Recommendation: add one exported method on the already-shared *Permissions type and have all three sites route through it:

// pkg/workflow/permissions_operations.go (or permissions.go)
func (p *Permissions) HasCopilotRequestsWrite() bool {
    if p == nil { return false }
    level, ok := p.Get(PermissionCopilotRequests)
    return ok && level == PermissionWrite
}

Then each caller only differs in how it produces *Permissions (WorkflowData vs. NewPermissionsParserFromValue(frontmatter["permissions"])), and the comparison logic lives in one place. Low risk β€” the parsing helpers (NewPermissionsParser, NewPermissionsParserFromValue) and constants (PermissionCopilotRequests, PermissionWrite) are already exported and used at all three sites.

Impact: single source of truth for a security-relevant permission gate; eliminates the risk of the three copies drifting.


i️ Patterns that look like duplicates but are intentional (do NOT refactor)

So future automated passes don't re-flag these:

  • Build-tag _wasm.go variants β€” validateDockerImage, isDockerDaemonRunning, markDockerDaemonUnavailable, setupGHCommand, getDefaultGHHost, findGitRoot, isTTY, isUnderWorkflowsDirectory, isCustomAgentFile, isRepositoryImport. Each has a *_wasm.go stub paired with a real implementation gated by build constraints. Correct and required.
  • Thin re-export wrappers over centralized helpers β€” workflow.SanitizeName β†’ stringutil.SanitizeName; workflow.compileSchema / parser.compileSchema β†’ parser.CompileSchema. These are deliberate package-local aliases (often with type X = pkg.X) to preserve call sites while keeping one implementation. Good practice, already consolidated.

βœ… Positive organization notes

  • Feature-per-file is followed well: codemod_*.go, audit_*.go, forecast_*.go, gateway_logs_*.go, compile_*.go, add_interactive_*.go families each isolate one concern.
  • Generic string/time/file utilities are already centralized in pkg/stringutil, pkg/timeutil, pkg/fileutil rather than scattered.
  • No exact-duplicate plain functions found within a single package (beyond the intentional patterns above).

Analysis metadata

  • Files analyzed: 833 non-test .go files (pkg/workflow 437, pkg/cli 345, pkg/parser 51)
  • Function declarations cataloged: ~5,347
  • Genuine actionable duplicate found: 1 (copilot-requests permission check)
  • False-positive duplicate patterns identified: build-tag variants, thin re-export wrappers
  • Method: naming-pattern clustering + cross-package same-name detection + manual verification of every candidate
  • Note: semantic-analysis code execution was restricted in this environment, so clustering was done via pattern search over function declarations plus targeted reads of each candidate; findings below were all read-verified.

Implementation checklist

  • Add (*Permissions).HasCopilotRequestsWrite() helper in pkg/workflow
  • Route pkg/workflow/permissions_operations.go:62 through it
  • Route pkg/workflow/permissions_compiler_validator.go:221 through it
  • Route pkg/cli/workflow_secrets.go:120 through it
  • Run existing permission tests to confirm behavior unchanged

Generated by πŸ”§ Semantic Function Refactoring Β· 180 AIC Β· βŒ– 15 AIC Β· ⊞ 9.1K Β· β—·

  • expires on Jul 11, 2026, 4:12 PM UTC-08:00

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions