Skip to content

[refactor] Semantic Function Clustering Analysis: Misplaced Functions and Duplicate Patterns in pkg/workflow #18388

Description

@github-actions

Automated semantic analysis of the pkg/ directory (563 non-test Go files across 18 packages) identified concrete refactoring opportunities in the pkg/workflow package. Below are actionable findings organized by priority.

Summary

  • Total Go files analyzed: 563 (280 in workflow, 192 in cli, 37 in parser, 27 in console, 27 in smaller utility packages)
  • Outlier functions found: 4 (functions clearly in the wrong file)
  • Near-duplicate function pairs: 2 (same logic, duplicated in parallel files)
  • Missing helpers causing repetition: 2 (patterns repeated in 4–15+ call sites)
  • Fragmented file candidates for consolidation: 1 (two single-function files covering the same struct)

Issue 1: validateTargetRepoSlug Misplaced in validation_helpers.go

File: pkg/workflow/validation_helpers.go:186
Function: func validateTargetRepoSlug(targetRepoSlug string, log *logger.Logger) bool

Problem:

  • Lives in validation_helpers.go alongside generic validators (ValidateRequired, ValidateMaxLength, ValidateInList, etc.) but is specific to the safe-outputs GitHub entity domain
  • Has an inconsistent return convention: returns bool instead of error like every other function in the file
  • Called exclusively by 6 safe-outputs files: add_comment.go, add_reviewer.go, close_entity_helpers.go, create_discussion.go, create_issue.go, create_pull_request.go

Recommendation: Move to pkg/workflow/safe_outputs_validation.go (or safe_outputs_target_validation.go) and align the return type to error.


Issue 2: collectPackagesFromWorkflow is Extraction Logic, Not Validation

File: pkg/workflow/runtime_validation.go:196
Function: func collectPackagesFromWorkflow(workflowData *WorkflowData, extractor func(string) []string, toolCommand string) []string

Problem:

  • Produces no errors and performs no validation — it is a pure extraction/collection helper that returns a deduplicated []string
  • Lives in runtime_validation.go, surrounded by validation methods like validateExpressionSizes, validateContainerImages, validateRuntimePackages
  • Conceptually belongs in the package-extraction infrastructure (package_extraction.go, npm.go, pip.go)

Recommendation: Move to pkg/workflow/package_extraction.go.


Issue 3: isValidFullSHA Defined in Wrong Domain File

File: pkg/workflow/features_validation.go:96
Function: func isValidFullSHA(s string) bool

Problem:

  • Checks if a string is a valid 40-character hexadecimal SHA — a general-purpose utility
  • Defined in features_validation.go (feature-flag value validation) but also called from pkg/workflow/action_pins.go:165, which is a completely different domain (action pin management)
  • A general SHA-validation helper should not live in a domain-specific validation file

Recommendation: Move to pkg/workflow/validation_helpers.go or pkg/workflow/strings.go.


Issue 4: Near-Duplicate Mount Validation Functions

Files:

  • pkg/workflow/sandbox_validation.go:24func validateMountsSyntax(mounts []string) error
  • pkg/workflow/mcp_config_validation.go:299func validateMCPMountsSyntax(toolName string, mountsRaw any) error

Problem:
Both functions:

  1. Iterate over mount strings
  2. Call the shared helper validateMountStringFormat(mount) from validation_helpers.go
  3. Use the same source == "" && dest == "" && mode == "" sentinel to distinguish error types
  4. Produce identical conceptual errors ("must follow 'source:destination:mode' format", "mode must be 'ro' or 'rw'")

The only differences are: validateMCPMountsSyntax normalizes a []any/[]string input and adds toolName context; validateMountsSyntax works directly on []string. Both carry separate doc-URL references (constants.DocsSandboxURL vs constants.DocsToolsURL).

View Duplicate Logic Pattern
// sandbox_validation.go
func validateMountsSyntax(mounts []string) error {
    var errors []string
    for _, mount := range mounts {
        source, dest, mode, err := validateMountStringFormat(mount)
        if err != nil {
            if source == "" && dest == "" && mode == "" {
                errors = append(errors, fmt.Sprintf("mount '%s' must follow 'source:destination:mode' format", mount))
            } else {
                errors = append(errors, fmt.Sprintf("mount '%s' mode must be 'ro' or 'rw', got '%s'", mount, mode))
            }
        }
    }
    // ...
}

// mcp_config_validation.go
func validateMCPMountsSyntax(toolName string, mountsRaw any) error {
    // normalize to []string ...
    for _, mount := range mounts {
        source, dest, mode, err := validateMountStringFormat(mount)
        if err != nil {
            if source == "" && dest == "" && mode == "" {
                errors = append(errors, fmt.Sprintf("mount '%s' must follow 'source:destination:mode' format", mount))
            } else {
                errors = append(errors, fmt.Sprintf("mount '%s' mode must be 'ro' or 'rw', got '%s'", mount, mode))
            }
        }
    }
    // ...
}

Recommendation: Extract a shared validateMountStrings(mounts []string, docsURL string) []string helper into validation_helpers.go that both functions call for the core loop.


Issue 5: Write-Permission Check Logic Duplicated Across Two Files

Files:

  • pkg/workflow/dangerous_permissions_validation.gofindWritePermissions(permissions *Permissions) []PermissionScope
  • pkg/workflow/strict_mode_validation.govalidateStrictPermissions(frontmatter map[string]any) error

Problem:
Both functions check for the presence of write-level permissions but use completely different implementations:

  • dangerous_permissions_validation.go uses NewPermissionsParserPermissions struct → iterates GetAllPermissionScopes() checking for PermissionWrite
  • strict_mode_validation.go uses NewPermissionsParserFromValuePermissionsParser.IsAllowed() on a hardcoded list of three scopes

The semantic intent is identical: "find write permissions that violate policy."

Recommendation: Extract a shared internal helper such as findWriteScopesForPolicy(permissions *Permissions, scopes []PermissionScope) []PermissionScope into permissions_operations.go (which already exists as the operations file) and have both validators call it.


Issue 6: Missing emitWarning Helper — Pattern Repeated in 15+ Places

Affected files: compiler.go, compiler_orchestrator_engine.go, compiler_orchestrator_tools.go, agent_validation.go, action_pins.go, action_sha_checker.go, and more

Problem:
The two-line warning emission pattern appears at 15+ call sites across 8+ files:

fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg))
c.IncrementWarningCount()

There is no shared emitWarning(c *Compiler, msg string) helper method on *Compiler, causing code duplication and risking the two lines becoming out of sync (e.g., emitting a warning without incrementing the count).

Recommendation: Add func (c *Compiler) emitWarning(msg string) to pkg/workflow/compiler.go or a compiler_helpers.go file and replace all call sites.


Issue 7: Firewall Validation Fragmented Across Two Single-Function Files

Files:

  • pkg/workflow/firewall_validation.go — 2 functions: validateFirewallConfig (validates log-level field) and ValidateLogLevel
  • pkg/workflow/network_firewall_validation.go — 1 function: validateNetworkFirewallConfig (validates allow-urls requires ssl-bump: true)

Problem:
Both files validate sub-aspects of the same NetworkPermissions/Firewall struct family. Two thin files covering the same struct's validation is fragmented organization; per Go conventions, related validation for the same domain object should be co-located.

Recommendation: Consolidate network_firewall_validation.go into firewall_validation.go (or create a network_validation.go containing both).


Issue 8: Manual Error Aggregation Bypasses Existing ErrorCollector

Files:

  • pkg/workflow/npm_validation.govalidateNpxPackages
  • pkg/workflow/runtime_validation.govalidateContainerImages, validateRuntimePackages

Problem:
Three functions use a manual var errors []string / append / strings.Join / NewValidationError pattern that is a reimplementation of the ErrorCollector already extracted in pkg/workflow/error_aggregation.go. Other functions in the same package (e.g., dispatch_workflow_validation.go, strict_mode_validation.go) correctly use NewErrorCollector() / collector.Add() / collector.FormattedError().

Recommendation: Refactor the three functions to use NewErrorCollector() from error_aggregation.go for consistency.


Refactoring Checklist

  • Issue 1: Move validateTargetRepoSlug from validation_helpers.go to safe_outputs_target_validation.go, align return type to error
  • Issue 2: Move collectPackagesFromWorkflow from runtime_validation.go to package_extraction.go
  • Issue 3: Move isValidFullSHA from features_validation.go to validation_helpers.go or strings.go
  • Issue 4: Extract shared mount-string validation loop into validateMountStrings helper; call from both validateMountsSyntax and validateMCPMountsSyntax
  • Issue 5: Extract shared write-permission query helper into permissions_operations.go
  • Issue 6: Add func (c *Compiler) emitWarning(msg string) and replace 15+ duplicate call sites
  • Issue 7: Consolidate network_firewall_validation.go into firewall_validation.go
  • Issue 8: Migrate validateNpxPackages, validateContainerImages, validateRuntimePackages to use ErrorCollector

Analysis Metadata

  • Total Go files analyzed: 563 (excluding *_test.go)
  • Primary focus: pkg/workflow/ (280 files), pkg/parser/ (37 files)
  • Outliers found: 4 functions in wrong files
  • Near-duplicate pairs: 2
  • Missing helper patterns: 2 (warning emit, error aggregation)
  • Fragmented file candidates: 1
  • Detection method: Serena semantic LSP analysis + naming pattern analysis + static grep
  • Analysis date: 2026-02-25
  • Workflow run: §22407937539

Generated by Semantic Function Refactoring

  • expires on Feb 27, 2026, 5:30 PM 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