Skip to content

[refactor] Semantic function clustering: GitHub App resolution near-duplicates and outlier functions in role_checks.go #29651

Description

@github-actions

Overview

Semantic analysis of 746 non-test Go source files across 22 packages identified actionable refactoring opportunities. The codebase is generally well-organized with strong DRY principles. Three specific patterns warrant attention.


Finding 1: Near-Duplicate GitHub App Resolution Functions (High Priority)

Two structurally identical patterns exist across different files in pkg/workflow:

Pattern Apkg/workflow/workflow_github_app.go:36

func resolveTopLevelGitHubApp(frontmatter map[string]any, importsResult *parser.ImportsResult) *GitHubAppConfig {
    if app := extractTopLevelGitHubApp(frontmatter); app != nil {
        return app
    }
    if importsResult != nil && importsResult.MergedTopLevelGitHubApp != "" {
        var appMap map[string]any
        if err := json.Unmarshal([]byte(importsResult.MergedTopLevelGitHubApp), &appMap); err == nil {
            app := parseAppConfig(appMap)
            if app.AppID != "" && app.PrivateKey != "" {
                workflowGitHubAppLog.Print("Using top-level github-app from imported shared workflow")
                return app
            }
        }
    }
    return nil
}

Pattern Bpkg/workflow/role_checks.go:624

func (c *Compiler) resolveActivationGitHubApp(frontmatter map[string]any, importsResult *parser.ImportsResult) *GitHubAppConfig {
    if app := c.extractActivationGitHubApp(frontmatter); app != nil {
        return app
    }
    if importsResult != nil && importsResult.MergedActivationGitHubApp != "" {
        var appMap map[string]any
        if err := json.Unmarshal([]byte(importsResult.MergedActivationGitHubApp), &appMap); err == nil {
            app := parseAppConfig(appMap)
            if app.AppID != "" && app.PrivateKey != "" {
                roleLog.Print("Using on.github-app from imported shared workflow")
                return app
            }
        }
    }
    return nil
}

Similarity: ~90% — identical 3-step structure (extract → check imports JSON → parse + validate). Differences: which YAML key path, which importsResult field, and the log message.

Recommendation: Extract a shared helper to remove the duplicated JSON-parse-and-validate block:

// resolveGitHubAppFromImports parses a GitHubAppConfig from a JSON-encoded
// merged import string. Returns nil if the string is empty, unparseable, or incomplete.
func resolveGitHubAppFromImports(mergedJSON string) *GitHubAppConfig {
    if mergedJSON == "" {
        return nil
    }
    var appMap map[string]any
    if err := json.Unmarshal([]byte(mergedJSON), &appMap); err != nil {
        return nil
    }
    app := parseAppConfig(appMap)
    if app.AppID != "" && app.PrivateKey != "" {
        return app
    }
    return nil
}

Eliminates ~12 lines of duplicated logic from two call sites.


Finding 2: Outlier Functions in role_checks.go (Medium Priority)

pkg/workflow/role_checks.go (639 lines) is named for role/membership checks, but its final ~70 lines contain GitHub App and token configuration resolution that belongs with workflow_github_app.go:

Function Current File Better Home
extractActivationGitHubToken() role_checks.go:573 workflow_github_app.go
extractActivationGitHubApp() role_checks.go:589 workflow_github_app.go
resolveActivationGitHubToken() role_checks.go:608 workflow_github_app.go
resolveActivationGitHubApp() role_checks.go:624 workflow_github_app.go

workflow_github_app.go already hosts extractTopLevelGitHubApp and resolveTopLevelGitHubApp, making it the natural home for all GitHub App configuration extraction and resolution.

Why this matters: A developer looking for GitHub App token logic must search two files. Moving these 4 functions co-locates all app resolution logic and reduces role_checks.go by ~80 lines.

Estimated effort: 1–2 hours (move functions, verify no circular import, update callers in compiler_orchestrator_workflow.go)


Finding 3: Generic Slice Utilities in runtime_definitions.go (Low Priority)

pkg/workflow/runtime_definitions.go contains two generic string-slice utilities used broadly across the workflow package, but buried inside a runtime-config file:

  • mergeUnique(base []string, extra ...string) []string — used in imports.go (5 call sites)
  • excludeFromSlice(base []string, exclude ...string) []string — used in safe_outputs_config_generation.go and compiler_safe_outputs_config.go

These are general-purpose utilities that do not depend on runtime configuration data. Their current home makes them hard to discover for contributors working on import merging or safe-output generation.

Recommendation: Move to pkg/workflow/map_helpers.go (which already hosts similar excludeMapKeys and sortedMapKeys utilities) or a new pkg/workflow/slice_helpers.go.

Note: mergeUnique differs subtly from sliceutil.Deduplicate—it merges two slices while preserving order, rather than deduplicating a single slice in place—so it cannot be replaced by the existing sliceutil function.

Estimated effort: 30 minutes (move + verify build)


Patterns Reviewed but Not Flagged

View patterns that were analyzed but are NOT refactoring targets
  • resolveImportPath across 3 files: run_push.go:resolveImportPathLocal explicitly documents "This is needed to avoid circular dependencies with imports.go" — intentional design, not a duplicate.
  • workflow/strings.go vs stringutil/sanitize.go: Different domains (workflow artifact names vs code identifiers). stringutil/sanitize.go:94 cross-references workflow.SanitizeArtifactIdentifier — intentional separation.
  • safe_outputs_* files (24) and strict_mode_* files (6): Well-organized feature-based file decomposition. No action needed.
  • Token precedence resolution (computeEffectivePRCheckoutToken etc. in safe_outputs_config_helpers.go): 4 similar functions, but each has distinct precedence rules; defer generification until a 5th variant is needed.
  • Update entity helpers (update_entity_helpers.go, update_issue_helpers.go, etc.): Excellent progressive abstraction — each entity-specific file delegates to the generic parseUpdateEntityConfigTyped. No duplication.

Implementation Checklist

  • Priority 1: Add resolveGitHubAppFromImports helper to workflow_github_app.go; update resolveTopLevelGitHubApp and resolveActivationGitHubApp to use it
  • Priority 2: Move 4 activation GitHub app/token functions from role_checks.go to workflow_github_app.go
  • Priority 3: Move mergeUnique and excludeFromSlice from runtime_definitions.go to map_helpers.go or a new slice_helpers.go
  • Verify go build ./... and tests pass after each change

Analysis Metadata

  • Total Go files analyzed: 746 (non-test, pkg/ directory)
  • Packages covered: 22 (actionpins, agentdrain, cli, console, constants, envutil, fileutil, gitutil, logger, parser, repoutil, semverutil, sliceutil, stats, stringutil, styles, testutil, timeutil, tty, types, typeutil, workflow)
  • Near-duplicates detected: 1 high-confidence (GitHub App resolution, ~90% similarity)
  • Outlier functions found: 4 (in wrong file based on naming convention)
  • Detection method: Pattern grep + Serena LSP semantic analysis + manual code review
  • Workflow run: §25238367543

Generated by Semantic Function Refactoring · ● 784.2K ·

  • expires on May 4, 2026, 12:12 AM UTC

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