[refactor] Reduce duplicate logic in `role_checks.go` via consolidation refactoring

[github-actions]

Executive Summary

Analysis of 659 non-test Go files across pkg/ found the codebase to be well-organized overall, with strong use of generics, centralized helpers, and clear file-per-feature structure. One file — pkg/workflow/role_checks.go (704 lines) — contains three clear cases of near-duplicate logic that are strong refactoring candidates.

---

Analysis Metadata

• Total Go files analyzed: 659 (across pkg/agentdrain, pkg/cli, pkg/console, pkg/constants, pkg/parser, pkg/stringutil, pkg/workflow, and 10 small utility packages)
• Packages with clear issues: 1 (pkg/workflow/role_checks.go)
• Detection method: Serena LSP semantic analysis + naming pattern clustering + manual body comparison
• Workflow run: §24030163052

---

Finding 1 — extractSkipRoles / extractSkipBots are identical logic

Location: pkg/workflow/role_checks.go:501–541

These two functions are structurally identical — the only difference is the literal field name string:

// Lines 501–518
func (c *Compiler) extractSkipRoles(frontmatter map[string]any) []string {
    onValue, exists := frontmatter["on"]
    if !exists || onValue == nil { return nil }
    switch on := onValue.(type) {
    case map[string]any:
        if skipRolesValue, exists := on["skip-roles"]; exists && skipRolesValue != nil {
            return extractStringSliceField(skipRolesValue, "skip-roles")
        }
    }
    return nil
}

// Lines 522–541
func (c *Compiler) extractSkipBots(frontmatter map[string]any) []string {
    onValue, exists := frontmatter["on"]
    if !exists || onValue == nil { return nil }
    switch on := onValue.(type) {
    case map[string]any:
        if skipBotsValue, exists := on["skip-bots"]; exists && skipBotsValue != nil {
            return extractStringSliceField(skipBotsValue, "skip-bots")
        }
    }
    return nil
}

Recommendation: Extract to a single private helper extractSkipField(frontmatter map[string]any, fieldName string) []string and have each public method delegate to it:

func extractSkipField(frontmatter map[string]any, fieldName string) []string {
    onValue, exists := frontmatter["on"]
    if !exists || onValue == nil { return nil }
    if on, ok := onValue.(map[string]any); ok {
        if val, exists := on[fieldName]; exists && val != nil {
            return extractStringSliceField(val, fieldName)
        }
    }
    return nil
}

func (c *Compiler) extractSkipRoles(frontmatter map[string]any) []string {
    return extractSkipField(frontmatter, "skip-roles")
}
func (c *Compiler) extractSkipBots(frontmatter map[string]any) []string {
    return extractSkipField(frontmatter, "skip-bots")
}

---

Finding 2 — mergeSkipRoles / mergeSkipBots contain identical deduplication logic

Location: pkg/workflow/role_checks.go:579–636

Both functions implement ordered deduplication of two string slices. The logic is word-for-word identical aside from variable names and the log label:

// Lines 579–606
func (c *Compiler) mergeSkipRoles(topSkipRoles, importedSkipRoles []string) []string {
    rolesSet := make(map[string]bool)
    var result []string
    for _, role := range topSkipRoles { if !rolesSet[role] { rolesSet[role]=true; result=append(result,role) } }
    for _, role := range importedSkipRoles { if !rolesSet[role] { rolesSet[role]=true; result=append(result,role) } }
    if len(result) > 0 { roleLog.Printf("Merged skip-roles: ...") }
    return result
}

// Lines 608–636  — identical pattern, "users" variable names, "skip-bots" log label
func (c *Compiler) mergeSkipBots(topSkipBots, importedSkipBots []string) []string { ... }

Note: pkg/sliceutil already exports Deduplicate[T comparable](slice []T) []T.

Recommendation: Replace both with a single helper, or delegate to sliceutil:

func mergeStringSlicesDedup(top, imported []string, logLabel string) []string {
    result := sliceutil.Deduplicate(append(top, imported...))
    if len(result) > 0 {
        roleLog.Printf("Merged %s: %v (top=%d, imported=%d, total=%d)",
            logLabel, result, len(top), len(imported), len(result))
    }
    return result
}

func (c *Compiler) mergeSkipRoles(top, imported []string) []string {
    return mergeStringSlicesDedup(top, imported, "skip-roles")
}
func (c *Compiler) mergeSkipBots(top, imported []string) []string {
    return mergeStringSlicesDedup(top, imported, "skip-bots")
}

---

Finding 3 — parseBotsValue duplicates extractStringSliceField

Location: pkg/workflow/role_checks.go:163–187 vs 544–577

parseBotsValue handles the same three type cases (string, []string, []any) as the existing extractStringSliceField, which was added later. parseBotsValue predates extractStringSliceField and was not updated when the generic helper was introduced.

// parseBotsValue (lines 163–187) — handles []any, []string, string
func parseBotsValue(botsValue any, fieldName string) []string { ... }

// extractStringSliceField (lines 544–577) — handles string, []string, []any (same cases)
func extractStringSliceField(value any, fieldName string) []string { ... }

Recommendation: Replace parseBotsValue with a call to extractStringSliceField:

func parseBotsValue(botsValue any, fieldName string) []string {
    return extractStringSliceField(botsValue, fieldName)
}

Or inline the call sites directly.

---

Positive Observations (No Action Required)

• pkg/stringutil/ — Six files with excellent semantic clustering (ansi, identifiers, sanitize, urls, pat_validation, stringutil). No overlap with pkg/workflow/strings.go, which is workflow-domain-specific with an extensive doc comment explaining the distinctions.
• pkg/workflow/validation_helpers.go — Correctly centralized; used by 32+ validation files with zero duplicate public functions across them.
• pkg/workflow/update_entity_helpers.go — Exemplary use of Go generics (parseUpdateEntityConfigTyped[T any]); update_issue_helpers.go, update_discussion_helpers.go, update_pull_request_helpers.go each delegate cleanly to the generic base.
• WASM build-tag files (e.g., github_cli_wasm.go, docker_validation_wasm.go) — Intentional platform-specific duplicates, correctly guarded with //go:build js || wasm.
• pkg/sliceutil/ — Deduplicate and Filter generics are available and underutilized; Finding 2 is an opportunity to leverage them.

---

Refactoring Checklist

• Finding 1: Extract extractSkipField(frontmatter, fieldName) in role_checks.go
• Finding 2: Extract mergeStringSlicesDedup (or use sliceutil.Deduplicate) in role_checks.go
• Finding 3: Simplify parseBotsValue to delegate to extractStringSliceField
• Update unit tests in role_checks_test.go if any test the extracted helpers directly
• Verify no behavior change with go test ./pkg/workflow/...

Estimated effort: 1–2 hours. All three changes are within a single 704-line file with no external API surface changes.

Generated by Semantic Function Refactoring · ● 584K · ◷

• expires on Apr 8, 2026, 11:40 AM UTC

[github-actions]

🍪 Issue Monster has assigned this to Copilot!

I've identified this issue as a good candidate for automated resolution and assigned it to the Copilot coding agent.

The Copilot coding agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

🍪 Om nom nom by Issue Monster · ● 191.5K · ◷