Skip to content

[refactor] Semantic clustering: extract duplicated linter AST helpers + copilot-perms predicateΒ #39167

Description

@github-actions

πŸ”§ Semantic Function Clustering Analysis

Analysis of repository: github/gh-aw β€” pkg/ source tree

Overview

Scanned 878 non-test Go source files in pkg/ (excluding *_test.go and testdata/), cataloging 5,251 top-level functions and methods. Functions were clustered by name and verified across files with ripgrep plus manual reading to separate genuine duplication from intentional patterns.

Headline: code organization is strong. The one-feature-per-file convention is well followed (each codemod_*.go, create_*, and similar lives in its own file), and shared utilities are already centralized in pkg/stringutil. Most same-name collisions turned out to be intentional and should NOT be changed:

  • WASM build-tag pairs (22 *_wasm.go files): console.go/console_wasm.go, github_cli.go/github_cli_wasm.go, remote_fetch.go/remote_fetch_wasm.go, and others provide parallel implementations behind build constraints. Correct, not duplication.
  • Delegating wrappers: workflow.SanitizeName, parser.FindClosestMatches, parser.LevenshteinDistance, and cli.GetVersion are thin one-line forwarders to the canonical stringutil / workflow implementation. Correct re-exports, not duplication.

Two genuine cleanup opportunities remain.

Key Findings

No. Finding Severity Scope
1 Identical AST helper functions copied across linter analyzer packages Medium 3 packages
2 Duplicated copilot-requests write-permission predicate Low 2 files
3 Same-name functions with different semantics (naming collision) Informational 4 files

Finding 1 β€” Duplicated linter AST helpers (Priority 1)

Three go/analysis analyzer packages each carry their own copy of the same AST helpers:

Function timesleepnocontext execcommandwithoutcontext ctxbackground
contextParamName yes (*ast.FuncType) yes (*ast.FuncType) yes (*ast.FuncDecl variant)
enclosingFuncType yes yes β€”
contextContextType / contextType yes yes yes (variant)

The contextParamName, enclosingFuncType, and contextContextType bodies are byte-identical between pkg/linters/timesleepnocontext/timesleepnocontext.go and pkg/linters/execcommandwithoutcontext/execcommandwithoutcontext.go. pkg/linters/ctxbackground/ctxbackground.go carries a close variant (contextParamName over *ast.FuncDecl, plus an equivalent contextType).

Byte-identical helper (both packages)
func enclosingFuncType(node ast.Node) *ast.FuncType {
    switch fn := node.(type) {
    case *ast.FuncDecl:
        return fn.Type
    case *ast.FuncLit:
        return fn.Type
    default:
        return nil
    }
}

func contextParamName(pass *analysis.Pass, fn *ast.FuncType) (string, bool) {
    if fn == nil || fn.Params == nil {
        return "", false
    }
    ctxType := contextContextType(pass)
    if ctxType == nil {
        return "", false
    }
    for _, field := range fn.Params.List {
        t := pass.TypesInfo.TypeOf(field.Type)
        if t == nil || !types.Identical(t, ctxType) {
            continue
        }
        for _, name := range field.Names {
            if name.Name != "_" {
                return name.Name, true
            }
        }
    }
    return "", false
}

Recommendation: extract a small shared helper package, e.g. pkg/linters/internal/astx, exposing EnclosingFuncType(node), ContextType(pass), and ContextParamName(pass, fn). Import it from all three analyzers. Reconcile the *ast.FuncType vs *ast.FuncDecl variant by standardizing on *ast.FuncType and routing *ast.FuncDecl through EnclosingFuncType or fn.Type.

Impact: removes roughly 50 duplicated lines across 3 packages, gives a single source of truth for context-parameter detection, and means less to copy when the next context-aware linter is added. Estimated effort: 1-2 hours.

Finding 2 β€” Duplicated copilot-requests permission check (Priority 2)

The predicate perms.Get(PermissionCopilotRequests) == PermissionWrite is implemented twice:

  • pkg/cli/workflow_secrets.go:116 β€” hasCopilotRequestsWritePermission(frontmatter map[string]any) bool
  • pkg/workflow/permissions_operations.go:62 β€” hasCopilotRequestsWritePermission(workflowData *WorkflowData) bool

Both build a *Permissions from different inputs (a raw frontmatter map vs a *WorkflowData), then run the identical final comparison.

Recommendation: add a method func (p *Permissions) HasCopilotRequestsWrite() bool in pkg/workflow, and have both callers construct their *Permissions and delegate to it. Keeps the permission-level constant and the comparison in one place. Estimated effort: 30 minutes.

Finding 3 β€” Naming collisions, different semantics (Informational)

These share a name but are genuinely different functions; flagged only for potential reader confusion, with no action required if the packages stay separate:

  • extractToolsFromFrontmatter β€” pkg/parser/content_extractor.go returns (string, error); pkg/workflow/frontmatter_extraction_metadata.go returns map[string]any. Different contracts under the same name.
  • resolveImportInputPath β€” pkg/parser/import_field_extractor.go returns (string, bool); pkg/workflow/expression_extraction.go returns (any, bool).

Recommendation (optional): rename one of each pair to reflect its specific return contract (for example extractToolsString vs extractToolsMap) if they are ever surfaced together.

What Was Verified Clean (no action)

Patterns checked and confirmed correct
  • SanitizeName β€” pkg/workflow/strings.go is a one-line wrapper over stringutil.SanitizeName; SanitizeOptions is a type alias.
  • FindClosestMatches, LevenshteinDistance β€” pkg/parser/schema_suggestions.go forwards to pkg/stringutil/fuzzy_match.go.
  • GetVersion β€” pkg/cli/version.go forwards to pkg/workflow/version.go (single source of truth compilerVersion).
  • All 22 *_wasm.go files β€” intentional build-tag-gated parallel implementations.
  • One-feature-per-file convention β€” well followed across pkg/workflow (400 files) and pkg/cli (319 files).

Recommended Actions

  • P1: Extract shared pkg/linters/internal/astx for ContextParamName / EnclosingFuncType / ContextType; update the 3 analyzers.
  • P2: Add (*Permissions).HasCopilotRequestsWrite() and delegate both callers.
  • P3 (optional): Rename colliding extractToolsFromFrontmatter / resolveImportInputPath for clarity.

Analysis Metadata

  • Source files analyzed: 878 (pkg/, excluding *_test.go and testdata/)
  • Functions and methods cataloged: 5,251
  • WASM build-tag pairs (excluded from duplicate detection): 22
  • Genuine duplication clusters found: 2
  • Naming collisions (non-duplicate): 2
  • Detection method: name-based clustering plus cross-file verification with ripgrep and manual implementation comparison
  • Analysis date: 2026-06-14

Generated by πŸ”§ Semantic Function Refactoring Β· 266.8 AIC Β· βŒ– 14.5 AIC Β· ⊞ 8.6K Β· β—·

  • expires on Jun 15, 2026, 4:15 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