[refactor] Semantic Function Clustering Analysis: Refactoring Opportunities in pkg/

[github-actions]

Overview

Semantic function clustering analysis was performed on all 607 non-test Go files across 18 packages in pkg/. This run replaces issue #22473 (closed). Since the prior analysis (2026-03-23), there have been no Go source changes — the only intervening commit was a JavaScript dev-dependency bump.

Progress since last analysis: Two of the five previously-reported issues have been resolved:

• ✅ staged field gap in safe_outputs_config_generation.go — now covered via addStagedIfTrue across all handlers
• ✅ update_entity_helpers.go entity-specific parsers — moved to separate update_issue_helpers.go, update_discussion_helpers.go, update_pull_request_helpers.go

Three findings remain open. One new finding is introduced.

---

Critical Findings

1 high-priority dual-system architecture risk, 3 low-priority issues (2 carry-over, 1 new).

---

Issue 1: Two Parallel Safe-Output Config Generation Systems (High Priority — Carry-over)

Severity: High
Files:

• pkg/workflow/safe_outputs_config_generation.go (767 lines) — produces GH_AW_SAFE_OUTPUTS_CONFIG_PATH (config.json)
• pkg/workflow/compiler_safe_outputs_config.go (976 lines) — produces GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG env var

Both files now contain cross-reference comments explaining the dual-path architecture, which is a positive step. However, the structural risk remains: adding a new handler field requires updating both files independently. The two systems serve distinct runtime consumers (MCP server startup vs. handler manager runtime), but there is no compile-time enforcement that they stay in sync.

Current state: 767 lines in the old path, 976 lines in the new path — both growing in parallel.

Recommendation: Evaluate whether a shared data structure or a code-generation step could enforce parity between the two paths. At minimum, add an integration test that validates both paths produce consistent field sets for each handler type.

---

Issue 2: ForInspector Public Wrapper Redundancy (Low Priority — New)

Severity: Low
File: pkg/workflow/mcp_scripts_generator.go

The file contains 5 private generator functions paired with 5 public wrappers suffixed ForInspector that are pure pass-through delegations:

// Private function (line 35)
func generateMCPScriptsToolsConfig(mcpScripts *MCPScriptsConfig) string { ... }

// Public wrapper (line 367) — identical signature, zero logic
func GenerateMCPScriptsToolsConfigForInspector(mcpScripts *MCPScriptsConfig) string {
    return generateMCPScriptsToolsConfig(mcpScripts)
}

The same pattern repeats for MCPServerScript, JavaScriptToolScript, ShellToolScript, and PythonToolScript. Meanwhile, a sixth related function — generateMCPScriptGoToolScript (line 311) — is private and called directly from mcp_setup_generator.go with no ForInspector wrapper, making the pattern inconsistent.

The ForInspector suffix implies these are inspector-specific, but they are general-purpose generators. The CLI (pkg/cli/mcp_inspect_mcp_scripts_files.go) calls all 5 wrappers.

Recommendation: Remove the 5 private functions and make their implementations directly public (capitalize the first letter). Rename to drop the ForInspector suffix to reflect their general purpose. Update the single call site in mcp_inspect_mcp_scripts_files.go. Estimated effort: < 1 hour.

All 5 wrapper pairs

Private (internal)	Public wrapper (ForInspector)
generateMCPScriptsToolsConfig	GenerateMCPScriptsToolsConfigForInspector
generateMCPScriptsMCPServerScript	GenerateMCPScriptsMCPServerScriptForInspector
generateMCPScriptJavaScriptToolScript	GenerateMCPScriptJavaScriptToolScriptForInspector
generateMCPScriptShellToolScript	GenerateMCPScriptShellToolScriptForInspector
generateMCPScriptPythonToolScript	GenerateMCPScriptPythonToolScriptForInspector

---

Issue 3: Near-Duplicate Integer Parsing — parseIntValue vs ConvertToInt (Low Priority — Carry-over)

Severity: Low
File: pkg/workflow/map_helpers.go

// parseIntValue: handles int, int64, float64, uint64 — returns (value, ok)
func parseIntValue(value any) (int, bool)

// ConvertToInt: handles int, int64, float64, AND string — returns int (0 on failure)
func ConvertToInt(val any) int

ConvertToInt is a functional superset of parseIntValue for numeric types, adding string parsing with different error semantics (silent zero vs (int, bool) with explicit failure signal). Both are used across the package with no documented guidance on which to prefer.

Recommendation: Add a doc comment to each function clarifying the intended use case, or refactor ConvertToInt to delegate to parseIntValue for the numeric cases, adding only the string-parse branch on top. This makes the relationship explicit and eliminates subtle divergence (e.g., parseIntValue handles uint64 overflow safely; ConvertToInt does not).

---

Issue 4: safeUintToInt Now Correctly in map_helpers.go (Previous Report Correction)

The previous analysis (#22473) reported safeUintToInt as defined in frontmatter_extraction_metadata.go. This was incorrect — the function is and was defined in map_helpers.go at line 94. frontmatter_extraction_metadata.go calls safeUintToInt (lines 164, 206) but does not define it. No action needed.

---

Progress Summary

#	Finding	Status
1	Dual config generation systems	⚠️ Open (cross-ref comments added, sync risk remains)
2	staged field gap in old config path	✅ Resolved
3	safeUintToInt in wrong file	❌ Previous report error — was already correct
4	parseIntValue vs ConvertToInt near-duplicate	⚠️ Open
5	update_entity_helpers.go multi-entity parsers	✅ Resolved
NEW	ForInspector public wrapper redundancy	🆕 New finding

---

Recommendations Summary

Priority	Finding	File(s)	Effort
High	Document/enforce sync between dual config paths	safe_outputs_config_generation.go, compiler_safe_outputs_config.go	2–4 hours
Low	Remove ForInspector wrapper indirection	mcp_scripts_generator.go, mcp_inspect_mcp_scripts_files.go	< 1 hour
Low	Document or unify parseIntValue / ConvertToInt	map_helpers.go	< 1 hour

Implementation Checklist

• Add integration test validating field parity between the two config generation paths
• Evaluate whether a shared struct or code generation can enforce dual-path sync
• Remove ForInspector wrappers; export private functions directly; rename to drop ForInspector suffix
• Document or unify parseIntValue / ConvertToInt distinction in map_helpers.go

---

Analysis Metadata

• Total Go files analyzed: 607 (excluding *_test.go)
• Total functions cataloged: 2,819
• Packages covered: 18 (cli, workflow, parser, console, stringutil, fileutil, and 12 others)
• Detection method: Naming pattern clustering, Serena LSP semantic analysis, diff analysis vs prior run
• Replaces: [refactor] Semantic Function Clustering Analysis: Refactoring Opportunities in pkg/ #22473
• Workflow run: §23487183103
• Analysis date: 2026-03-24

References:

• §23487183103

Generated by Semantic Function Refactoring · ◷

• expires on Mar 26, 2026, 11:36 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 · ◷