[refactor] Misplaced functions in compiler_safe_outputs.go, inconsistent file-name splits in workflow package

[github-actions]

Overview

Semantic clustering analysis of 858 non-test Go files (pkg/workflow: 395, pkg/cli: 306, others: 157) surfaced four concrete refactoring opportunities. The codebase is generally well-organized — most validation, codemod, audit, and log files follow a clean one-file-per-feature pattern. The issues below are localized: file names that no longer match their contents, name collisions between packages that obscure intent, and one near-empty helper file. Each finding lists the exact symbol and call sites so the moves are mechanical.

Summary

• Files analyzed: 858 non-test .go files under pkg/
• Packages with >100 files: pkg/workflow (395), pkg/cli (306)
• Findings: 4 actionable (3 outliers/misnamed-files, 1 cross-package name collision)
• Recommended scope: one PR per finding; each touches ≤ 6 files
• Status: ✅ no implementation-level duplicate code blocks found; ⚠️ four organizational issues

Finding 1: compiler_safe_outputs.go holds five functions that are not about safe outputs

File: pkg/workflow/compiler_safe_outputs.go (615 lines)

The file's name advertises safe-output compilation, but five of its eight functions are concerned with other domains. Their actual callers live in unrelated files, which is the strongest signal they belong elsewhere.

Function	Line	Real domain	Should live in
(*Compiler).parseOnSection	compiler_safe_outputs.go:90	Workflow on: trigger parsing	trigger_parser.go or new on_section_parser.go
(*Compiler).applyDefaultTools	compiler_safe_outputs.go:387	Default tools (github, bash, edit)	tools.go (next to applyDefaults which already calls it at tools.go:324)
isSandboxEnabled	compiler_safe_outputs.go:589	Sandbox config interpretation	sandbox.go (callers: firewall.go:53, strict_mode_permissions_validation.go:192)
mergeCommandOtherEvents	compiler_safe_outputs.go:19	Trigger event merging	trigger_parser.go
mergeEventConfig / parseEventTypes	compiler_safe_outputs.go:37, 69	Trigger event merging	trigger_parser.go

Only mergeSafeJobsFromIncludedConfigs (line 352) and needsGitCommands (line 575) actually relate to safe outputs.

Why this matters: the validation architecture documented in pkg/workflow/validation.go:1-66 insists on domain-focused files. The current layout makes parseOnSection hard to find for anyone navigating by file name, and a future reader editing safe-output logic will scroll past three unrelated functions.

Recommendation: split the file along the table above; rename the residual to safe_outputs_command_merge.go or absorb the remaining two functions into safe_outputs_config.go. Estimated effort: 1–2 hours; pure move, no logic changes.

Finding 2: Inconsistent compiler_safe_outputs_* vs safe_outputs_* split

pkg/workflow/ has 8 files with prefix compiler_safe_outputs_ and 27 with prefix safe_outputs_. The intended boundary appears to be "methods on *Compiler" vs "stand-alone helpers," but it does not hold:

• safe_outputs_steps.go contains (c *Compiler) methods (buildCustomActionStep, buildGitHubScriptStep — see lines 18, 145, 204) but its counterpart compiler_safe_outputs_steps.go also contains (c *Compiler) methods.
• compiler_safe_outputs_env.go has a single function (addAllSafeOutputConfigEnvVars) while safe_outputs_env.go has 11 closely-related env-builder functions.
• compiler_safe_outputs_core.go and compiler_safe_outputs_handlers.go have zero functions each (verified with grep -c "^func ").

Recommendation: pick one prefix and fold the other in. Suggested approach:

1. Delete the empty compiler_safe_outputs_core.go and compiler_safe_outputs_handlers.go files (or move their type/var declarations into the matching safe_outputs_* file).
2. Merge compiler_safe_outputs_env.go (1 function) into safe_outputs_env.go.
3. Merge compiler_safe_outputs_config.go (5 functions) into safe_outputs_config.go.
4. Either keep compiler_safe_outputs_steps.go or rename it to safe_outputs_steps_pr_checkout.go — its content is PR-checkout-step generation, which has a distinct purpose.

Estimated effort: 2–3 hours; mostly mechanical.

Finding 3: mcp_renderer_* vs mcp_rendering.go vs mcp_config_builtin.go mix render code

Three file-name patterns hold MCP rendering code:

• mcp_renderer.go, mcp_renderer_builtin.go, mcp_renderer_github.go, mcp_renderer_guard.go, mcp_renderer_types.go — the documented helper-file convention (see .github/skills/developer/SKILL.md → "Good Names: mcp_renderer.go").
• mcp_rendering.go — has renderDefaultJSONMCPConfig, renderStandardJSONMCPConfig, buildMCPRendererFactory, buildStandardJSONMCPRenderers. These are renderer-orchestration helpers.
• mcp_config_builtin.go — has renderSafeOutputsMCPConfigWithOptions (line 114) and renderAgenticWorkflowsMCPConfigWithOptions (line 170). These are also rendering, but they live in a "config" file.

A reader looking for "where do we render the safe-outputs MCP block?" must check three different file-name patterns.

Recommendation: rename mcp_rendering.go → mcp_renderer_factory.go and move the two render*MCPConfigWithOptions functions out of mcp_config_builtin.go into mcp_renderer_builtin.go (or a new mcp_renderer_safe_outputs.go). Leave mcp_config_builtin.go for type/constant declarations only.

Estimated effort: 1–2 hours.

Finding 4: containsExpression is defined twice with different semantics

Location	Signature	Semantic
pkg/workflow/expression_patterns.go:74	containsExpression(s string) bool	True only when s contains a complete ${{ ... }} with ≥ 1 char between markers.
pkg/parser/include_processor.go:356	containsExpression(v any) bool	True when any string anywhere in a nested map[string]any / []any contains the substring ${{. This is the recursive equivalent of hasExpressionMarker in the workflow package.

The two functions are unexported and in different packages, so the compiler does not complain. But identical names with different semantics is a maintenance trap: a developer porting logic between the two packages will reach for the wrong primitive. The parser version's behavior matches hasExpressionMarker(s string) bool at pkg/workflow/expression_patterns.go:67 more closely than the workflow containsExpression.

Recommendation: rename pkg/parser/include_processor.go:356 to frontmatterValueContainsExpression (matches its sole caller frontmatterContainsExpressions on line 347). This makes the type-recursive intent explicit and removes the name collision.

Estimated effort: 15–30 minutes; touches pkg/parser/include_processor.go only.

Other Observations (informational, no action recommended)

Items reviewed and found acceptable

• findGitRoot variants across pkg/gitutil/gitutil.go:85, pkg/workflow/git_helpers.go:48, pkg/cli/git.go:28: the workflow and cli versions are thin wrappers around gitutil.FindGitRoot/FindGitRootFrom, each adding logging or absolute-path resolution that's specific to the calling context. Not a duplication concern.
• *_wasm.go duplicates (docker_validation.go / docker_validation_wasm.go, git_helpers.go / git_helpers_wasm.go, npm_validation.go / npm_validation_wasm.go, etc.): these are intentional build-tag splits for WebAssembly compilation. Correct pattern.
• SanitizeName in pkg/workflow/strings.go:100: a thin re-export of stringutil.SanitizeName via a type alias on SanitizeOptions. Intentional convenience.
• 51 codemod_*.go files in pkg/cli/: each follows the one-codemod-per-file pattern documented in the repository; consistent and correct.
• 18 audit_*.go and 25 logs_*.go files in pkg/cli/: well-organized by sub-feature (render, parse, format, etc.).
• compiler_workflow_helpers.go has just 2 small functions (ContainsCheckout, GetWorkflowIDFromPath). Borderline — could be merged into compiler_yaml.go and workflow_builder.go respectively, but the cost/benefit is marginal. Listed here for awareness, not as a recommendation.

Implementation checklist

• Finding 1 — split compiler_safe_outputs.go: move parseOnSection, mergeCommandOtherEvents, mergeEventConfig, parseEventTypes → trigger_parser.go; applyDefaultTools → tools.go; isSandboxEnabled → sandbox.go.
• Finding 2 — consolidate compiler_safe_outputs_* and safe_outputs_* files; delete the two empty compiler_safe_outputs_core.go / _handlers.go.
• Finding 3 — rename mcp_rendering.go → mcp_renderer_factory.go; relocate render*MCPConfigWithOptions functions to mcp_renderer_builtin.go.
• Finding 4 — rename pkg/parser/include_processor.go:356 containsExpression → frontmatterValueContainsExpression.
• Run go build ./... and go test ./pkg/workflow/... ./pkg/parser/... after each finding to confirm no behavior change.

Analysis Metadata

• Method: name-cluster + cross-package name-collision search via grep -rn '^func ' and per-file function inventory; representative bodies confirmed via Read.
• Files analyzed: 858 non-test .go files under pkg/
• Outliers found: 5 functions in compiler_safe_outputs.go whose callers live in other-domain files
• Name collisions found: 2 across packages (containsExpression, extractToolsFromFrontmatter)
• Empty files found: 2 (compiler_safe_outputs_core.go, compiler_safe_outputs_handlers.go — both have zero func declarations)
• Closed superseded issue: [refactor] Duplicate engine MCP/log scaffolding and dead SafeOutputStepConfig type in pkg/workflow #34800
• Analysis date: 2026-05-27

Generated by 🔧 Semantic Function Refactoring · opus47 32.6M · ◷

• expires on May 29, 2026, 12:14 AM UTC

[github-actions]

🍪 Issue Monster selected this for Copilot

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

If assignment succeeds, 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 · haiku45 43.7K · ◷