Skip to content

[refactor] Semantic Function Clustering Analysis: Duplicates, Misplaced Logic, and Fragmented Files in pkg/workflow #18543

Description

@github-actions

Automated semantic analysis of the pkg/ directory (563 non-test Go files across 18 packages, 280 in pkg/workflow/) identified 10 concrete refactoring opportunities. Findings are ordered by estimated impact.

Summary

  • Total Go files analyzed: 563 (280 in workflow, ~192 in cli, 37 in parser, and smaller utility packages)
  • Near-duplicate file pairs: 2 (structural duplicates with token-only differences)
  • Triplicated/duplicated function bodies: 3 groups
  • Misplaced functions: 2 (wrong files based on their domain/consumers)
  • Fragmented file sets: 2 (thin files that should be merged)
  • Missing helper abstractions: 2 (repeated expressions/patterns across 4–15+ call sites)
  • Interface bypass patterns: 1 (type-switches where polymorphism should be used)

Issue 1: Near-Duplicate Files — missing_data.go and missing_tool.go

Files: pkg/workflow/missing_data.go, pkg/workflow/missing_tool.go

Problem: These two ~163-line files are structurally identical. The only differences are token-level (variable name, log prefix, env var prefix, default title prefix string, script path). The struct definitions (MissingDataConfig / MissingToolConfig) are identical except for a comment word. The parse*Config and build*Job functions share 100% of their control-flow logic and field-access patterns.

Recommendation: Introduce a single generic config type (e.g., IssueReportingConfig) with a parameter struct covering the varying fields, and a single shared buildIssueReportingJob / parseIssueReportingConfig function. Type aliases MissingDataConfig / MissingToolConfig can remain for backward compatibility. Estimated elimination: ~120 lines of duplicated logic.


Issue 2: Triplicated GetSquidLogsSteps Across Three Engine Files

Files:

  • pkg/workflow/claude_engine.go (line 470)
  • pkg/workflow/codex_engine.go (line 354)
  • pkg/workflow/copilot_logs.go (line 450)

Problem: All three GetSquidLogsSteps implementations are byte-for-byte identical except for the logger variable. All three call isFirewallEnabled, generateSquidLogsUploadStep, and generateFirewallLogParsingStep with the same arguments and same conditional structure. Similarly, GetFirewallLogsCollectionStep is a no-op stub duplicated in both claude_engine.go and codex_engine.go.

Recommendation: Move a shared DefaultGetSquidLogsSteps(workflowData *WorkflowData, log *logger.Logger) []GitHubActionStep helper to engine_firewall_support.go (which already exists for cross-engine firewall logic). Each engine calls it with its own logger, reducing three 18-line functions to three 3-line wrappers. Alternatively, promote GetSquidLogsSteps to BaseEngine since the logic is engine-agnostic.


Issue 3: Near-Duplicate RenderMCPConfig in claude_mcp.go and gemini_mcp.go

Files:

  • pkg/workflow/claude_mcp.go (lines 12–69)
  • pkg/workflow/gemini_mcp.go (lines 12–64)

Problem: Diffing the two functions shows they differ only in receiver type, logger variable name, and one minor comment. The MCPRendererOptions struct passed in both uses identical fields (IncludeCopilotFields: false, InlineArgs: false, Format: "json"). The MCPToolRenderers struct construction is completely parallel with the same eight function closures in the same order. (Note: copilot_mcp.go legitimately differs by using IncludeCopilotFields: true, InlineArgs: true.)

Recommendation: Extract a shared renderJSONMCPConfigForEngine helper that accepts MCPRendererOptions, a config path string, and engine-specific overrides. Both Claude and Gemini engines call it with their specific logger. This can live in mcp_config_utils.go.


Issue 4: Repeated rewriteLocalhost Guard Expression (4+ Call Sites)

Files:

  • pkg/workflow/codex_mcp.go (lines 172–174 and 195–197 — appears twice)
  • pkg/workflow/copilot_mcp.go (lines 90–92)
  • pkg/workflow/mcp_config_custom.go (lines 53–55)

Problem: The following expression is repeated verbatim at 4 locations across 3 files, with an additional 14+ variant occurrences of the nil-guard chain across the package:

rewriteLocalhost := workflowData != nil && (workflowData.SandboxConfig == nil ||
    workflowData.SandboxConfig.Agent == nil ||
    !workflowData.SandboxConfig.Agent.Disabled)

Recommendation: Add shouldRewriteLocalhostToDocker(workflowData *WorkflowData) bool in mcp_config_utils.go or sandbox.go. All call sites replace the 3-line guard with a single readable predicate. This clarifies the semantics: firewall enabled = rewrite localhost.


Issue 5: Misplaced Firewall Step Generators in copilot_installer.go

File: pkg/workflow/copilot_installer.go (lines 35–79)

Functions: generateSquidLogsUploadStep, generateFirewallLogParsingStep

Problem: These two functions generate steps consumed by all three engine types (Claude, Codex, Copilot — verified by the triplicated GetSquidLogsSteps above). They are defined in copilot_installer.go, a file whose name implies Copilot-specific installation logic. A developer looking for firewall step generation utilities for the Claude engine will not look in a Copilot-specific file.

Recommendation: Move both functions to engine_firewall_support.go, which already exists for cross-engine firewall integration. The copilot_installer.go file should retain only GenerateCopilotInstallerSteps.


Issue 6: Internal Near-Duplicate Within claude_logs.goparseToolCallsWithSequence vs parseToolCalls

File: pkg/workflow/claude_logs.go

Functions: (e *ClaudeEngine) parseToolCallsWithSequence (line 338, 93 lines), (e *ClaudeEngine) parseToolCalls (line 433, 78 lines)

Problem: These two methods share almost identical logic for processing a []any content array: handling tool_use and tool_result cases, looking up/inserting into toolCallMap, and computing inputSize. The only difference is that parseToolCallsWithSequence additionally builds and returns a []string sequence. The tool_use branch (including Bash handling, prettify call, and map upsert) is copied verbatim.

Recommendation: Eliminate parseToolCalls as a separate function. Call parseToolCallsWithSequence from all existing call sites and discard the returned sequence when not needed. The caller in the entry["type"] == "user" branch already ignores the sequence — it can trivially call the sequence variant.


Issue 7: Fragmented safe_output* Namespace With a Thin Config File

Files:

  • pkg/workflow/safe_outputs.go — 8 lines, pure documentation/comments, no code
  • pkg/workflow/safe_output_config.go — 38 lines, a single function parseBaseSafeOutputConfig
  • Plus pkg/workflow/safe_outputs_config.go — 508 lines with the bulk of config parsing

Problem: The safe_output_config.go (singular) file contains only one function that belongs logically in safe_outputs_config.go (plural). The naming inconsistency between safe_output_* and safe_outputs_* files creates confusion. The safe_outputs.go index file is a pure comment shell with no code.

Recommendation: Merge parseBaseSafeOutputConfig from safe_output_config.go into safe_outputs_config.go. Establish a uniform safe_outputs_ prefix for all files in this domain. Remove or repurpose safe_outputs.go (if kept, it should contain exported package-level types). Similarly, consider whether safe_output_builder.go content (env var builders) belongs with safe_outputs_env.go.


Issue 8: LogParser Interface Methods Scattered Inconsistently Across *_logs.go vs *_engine.go

Files: claude_logs.go, claude_engine.go, codex_logs.go, codex_engine.go, gemini_logs.go, copilot_logs.go

Problem: The LogParser interface (defined in agentic_engine.go) has three methods. Implementations are inconsistently split:

  • Claude: ParseLogMetrics in claude_logs.go, but GetLogParserScriptId/GetLogFileForParsing/GetDefaultDetectionModel elsewhere
  • Codex: GetLogParserScriptId in codex_logs.go, other methods elsewhere
  • Copilot: GetFirewallLogsCollectionStep and GetSquidLogsSteps (workflow-step generators) are in copilot_logs.go instead of copilot_engine.go

Recommendation: Establish a clear file-placement convention: *_logs.go files contain only log-parsing logic (ParseLogMetrics + private helpers); interface-compliance stubs (GetLogParserScriptId, GetLogFileForParsing, GetDefaultDetectionModel) live in *_engine.go; workflow-step generators live in *_engine.go or a *_engine_steps.go file.


Issue 9: Type-Switch in compiler_yaml_main_job.go Bypasses Interface (With Duplicate CodexEngine Block)

File: pkg/workflow/compiler_yaml_main_job.go (lines 279–310)

Problem: Lines 279–310 contain four separate if engine, ok := engine.(*ConcreteType); ok { ... } type-assertion blocks for GetFirewallLogsCollectionStep, including a duplicate *CodexEngine block at lines 287–294 and 303–310. GetFirewallLogsCollectionStep is not part of the CodingAgentEngine interface, forcing this fragile type-switch that also accidentally duplicates the Codex case.

Recommendation: Add GetFirewallLogsCollectionStep(workflowData *WorkflowData) []GitHubActionStep to the WorkflowExecutor interface (or a new FirewallLogProvider interface) with a no-op default in BaseEngine. Replace the four type-assertion blocks with a single polymorphic call. This also eliminates the duplicate CodexEngine block as a side effect.


Issue 10: Parallel No-Op renderCacheMemoryMCPConfig Methods on Claude and Gemini

Files:

  • pkg/workflow/claude_mcp.go (lines 77–84)
  • pkg/workflow/gemini_mcp.go (lines 66–71)

Problem: Both methods are empty no-ops with an identical comment explaining that cache-memory uses a simple file share. They exist only because the MCPToolRenderers struct's RenderCacheMemory field requires a function. These zero-body engine methods add noise and must be updated together if the signature ever changes.

Recommendation: Define a single noOpCacheMemoryRenderer package-level variable or function in mcp_config_utils.go and assign it to RenderCacheMemory in all non-Copilot/non-Codex engine MCP config calls. Eliminates two empty engine methods and makes the "intentionally no-op" contract explicit.


Refactoring Checklist

  • Issue 1: Unify missing_data.go / missing_tool.go into a shared IssueReportingConfig abstraction
  • Issue 2: Extract DefaultGetSquidLogsSteps to engine_firewall_support.go; make stubs in each engine delegate to it
  • Issue 3: Extract renderJSONMCPConfigForEngine shared helper to mcp_config_utils.go
  • Issue 4: Add shouldRewriteLocalhostToDocker helper; replace 4+ inline guard expressions
  • Issue 5: Move generateSquidLogsUploadStep / generateFirewallLogParsingStep from copilot_installer.go to engine_firewall_support.go
  • Issue 6: Eliminate parseToolCalls; call parseToolCallsWithSequence from all sites
  • Issue 7: Merge safe_output_config.go's single function into safe_outputs_config.go; normalize naming convention
  • Issue 8: Establish *_logs.go vs *_engine.go file-placement convention for LogParser interface methods
  • Issue 9: Add GetFirewallLogsCollectionStep to interface; remove type-switch blocks + duplicate CodexEngine case
  • Issue 10: Define noOpCacheMemoryRenderer in mcp_config_utils.go; assign to both Claude and Gemini renderers

Analysis Metadata

  • Total Go files analyzed: 563 (excluding *_test.go)
  • Primary focus: pkg/workflow/ (280 files)
  • Near-duplicate file pairs: 2
  • Triplicated function groups: 1
  • Near-duplicate function pairs: 3
  • Misplaced functions: 2
  • Missing helper abstractions: 2
  • Interface bypass patterns: 1
  • Fragmented file candidates: 2
  • Detection method: Serena semantic LSP analysis + naming pattern analysis + static code inspection
  • Analysis date: 2026-02-26
  • Workflow run: §22453185060

References:

Generated by Semantic Function Refactoring

  • expires on Feb 28, 2026, 5:29 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