Skip to content

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

@github-actions

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

Metadata

Metadata

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions