refactor: relocate misplaced functions identified by semantic clustering analysis

docs/adr/29336-relocate-misplaced-functions-to-semantic-homes.md

@@ -0,0 +1,81 @@
+# ADR-29336: Relocate Misplaced Functions Identified by Semantic Clustering Analysis
+
+**Date**: 2026-04-30
+**Status**: Draft
+**Deciders**: pelikhan, Copilot
+
+---
+
+## Part 1 — Narrative (Human-Friendly)
+
+### Context
+
+Semantic function clustering analysis flagged 6 functions living in files whose names misrepresent their semantic domain. In `pkg/cli`, `GetBinaryPath` and `logAndValidateBinaryPath` resided in `mcp_validation.go` despite being path-resolution utilities with no validation logic. In `pkg/workflow`, `parseStringSliceAny` and `preprocessProtectedFilesField` lived in `validation_helpers.go` despite being data-coercion and preprocessing functions rather than validators. Also in `pkg/workflow`, `ValidateHeredocContent` and `ValidateHeredocDelimiter` were the only two exported validation functions outside the package's 44 `*_validation.go` files, stranded in `strings.go` alongside unrelated string utilities. This misplacement violated the file-naming convention established in ADR-27325 and extended by ADR-28282, making the codebase harder to navigate for contributors relying on file names to locate related logic.
+
+### Decision
+
+We will relocate each misplaced function to a file whose name accurately communicates its semantic domain: `GetBinaryPath` and `logAndValidateBinaryPath` move to a new `pkg/cli/mcp_helpers.go`; `parseStringSliceAny` and `preprocessProtectedFilesField` move to a new `pkg/workflow/parse_helpers.go`; `ValidateHeredocContent` and `ValidateHeredocDelimiter` move to a new `pkg/workflow/heredoc_validation.go`. All moves are intra-package; no call sites change and no behavior is altered. This applies the same outlier-relocation convention from ADR-28282 to a new set of functions surfaced by automated semantic clustering.
+
+### Alternatives Considered
+
+#### Alternative 1: Leave Functions in Place with Cross-File Documentation
+
+Inline comments could document that a function in file A primarily supports a concern owned by file B. This was rejected because documentation without structural enforcement degrades: contributors continue adding similar misplaced functions to the nearest convenient file rather than the semantically correct one, and the problem compounds over time. This alternative was also explicitly rejected in ADR-28282 for the same reason.
+
+#### Alternative 2: Consolidate Into a Single Umbrella `helpers.go`
+
+All small utilities could be gathered into a single per-package `helpers.go` to reduce file count and eliminate placement decisions. This was rejected because it recreates the catch-all pattern that ADR-27325 explicitly discourages — a function's location would again fail to communicate its purpose, defeating the file-naming convention the codebase has adopted.
+
+### Consequences
+
+#### Positive
+- `GetBinaryPath` and `logAndValidateBinaryPath` are co-located in `mcp_helpers.go`, making binary path resolution logic discoverable in a single file.
+- All data-coercion and preprocessing helpers are consolidated in `parse_helpers.go`, clearly separated from the validators in `validation_helpers.go`.
+- `ValidateHeredocContent` and `ValidateHeredocDelimiter` join their peers in `heredoc_validation.go`, completing the naming convention where all validation functions in `pkg/workflow` live in `*_validation.go` files.
+- Reinforces the semantic file-organization convention from ADR-27325 and ADR-28282 across new areas of the codebase.
+
+#### Negative
+- Increases file count in `pkg/cli/` (by 1) and `pkg/workflow/` (by 2), adding marginal navigation overhead.
+- Adds churn to `git log` for the relocated functions; `git blame` will surface the relocation commit rather than original authorship without `--follow`.
+
+#### Neutral
+- No public API surface changes; all moved functions retain their signatures.
+- The now-unused `errors` import is removed from `strings.go` as a side effect of moving `ValidateHeredocContent`.
+- The `validation_helpers.go` header comment is updated to remove stale references to the relocated parse helpers.
+
+---
+
+## Part 2 — Normative Specification (RFC 2119)
+
+> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).
+
+### Binary Path Utilities (`pkg/cli`)
+
+1. `GetBinaryPath` and `logAndValidateBinaryPath` **MUST** reside in `pkg/cli/mcp_helpers.go`.
+2. New binary path resolution helpers used by MCP server or validation logic **MUST** be added to `mcp_helpers.go` and **MUST NOT** be placed in `mcp_validation.go`.
+3. `mcp_validation.go` **MUST NOT** contain path-resolution utility functions; its scope **MUST** be limited to MCP server configuration validation logic.
+
+### Parse and Preprocessing Helpers (`pkg/workflow`)
+
+1. `parseStringSliceAny` and `preprocessProtectedFilesField` **MUST** reside in `pkg/workflow/parse_helpers.go`.
+2. New functions that coerce or preprocess raw configuration data before validation **MUST** be added to `parse_helpers.go` and **MUST NOT** be placed in `validation_helpers.go`.
+3. `validation_helpers.go` **MUST NOT** contain data-coercion or preprocessing functions; its scope **MUST** be limited to validation logic that checks constraints and returns errors.
+
+### Heredoc Validation (`pkg/workflow`)
+
+1. `ValidateHeredocContent` and `ValidateHeredocDelimiter` **MUST** reside in `pkg/workflow/heredoc_validation.go`.
+2. New exported validation functions for heredoc safety **MUST** be added to `heredoc_validation.go` and **MUST NOT** be placed in `strings.go` or other non-validation files.
+3. `strings.go` **MUST NOT** contain exported validation functions; its scope **MUST** be limited to string transformation and normalization utilities.
+
+### General File-Naming Convention
+
+1. All exported validation functions within `pkg/workflow` **MUST** reside in files whose names match the `*_validation.go` pattern.
+2. Functions whose semantic domain is misrepresented by their current file **SHOULD** be relocated to a file whose name accurately communicates that domain when identified by code review or automated analysis.
+
+### Conformance
+
+An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement — in particular, placing path-resolution utilities in `mcp_validation.go`, placing data-coercion helpers in `validation_helpers.go`, or placing exported validation functions in `strings.go` — constitutes non-conformance.
+
+---
+
+*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/25177070075) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/cli/mcp_helpers.go

@@ -0,0 +1,66 @@
+// This file contains MCP (Model Context Protocol) helper utilities.
+// These utilities support binary path resolution used by both the MCP server
+// and MCP validation logic.
+
+package cli
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+
+	"github.com/github/gh-aw/pkg/logger"
+)
+
+var mcpHelpersLog = logger.New("cli:mcp_helpers")
+
+// GetBinaryPath returns the path to the currently running gh-aw binary.
+// This is used by the MCP server to determine where the gh-aw binary is located
+// when launching itself with different arguments.
+//
+// Returns the absolute path to the binary, or an error if the path cannot be determined.
+func GetBinaryPath() (string, error) {
+	// Get the path to the currently running executable
+	exePath, err := os.Executable()
+	if err != nil {
+		return "", fmt.Errorf("failed to get executable path: %w", err)
+	}
+
+	// Resolve any symlinks to get the actual binary path
+	// This is important because gh extensions are typically symlinked
+	// Note: EvalSymlinks already returns an absolute path
+	resolvedPath, err := filepath.EvalSymlinks(exePath)
+	if err != nil {
+		// If we can't resolve symlinks, use the original path
+		mcpHelpersLog.Printf("Warning: failed to resolve symlinks for %s: %v", exePath, err)
+		return exePath, nil
+	}
+
+	return resolvedPath, nil
+}
+
+// logAndValidateBinaryPath determines the binary path, logs it, and validates it exists.
+// Returns the detected binary path and an error if the path cannot be determined or if the file doesn't exist.
+// This is a helper used by both runMCPServer and validateMCPServerConfiguration.
+// Diagnostics are emitted through the debug logger only.
+func logAndValidateBinaryPath() (string, error) {
+	binaryPath, err := GetBinaryPath()
+	if err != nil {
+		mcpHelpersLog.Printf("Warning: failed to get binary path: %v", err)
+		return "", err
+	}
+
+	// Check if the binary file exists
+	if _, err := os.Stat(binaryPath); err != nil {
+		if os.IsNotExist(err) {
+			mcpHelpersLog.Printf("ERROR: binary file does not exist at path: %s", binaryPath)
+			return "", fmt.Errorf("binary file does not exist at path: %s", binaryPath)
+		}
+		mcpHelpersLog.Printf("Warning: failed to stat binary file at %s: %v", binaryPath, err)
+		return "", err
+	}
+
+	// Log the binary path for debugging
+	mcpHelpersLog.Printf("gh-aw binary path: %s", binaryPath)
+	return binaryPath, nil
+}

pkg/cli/mcp_validation.go

@@ -1,5 +1,6 @@
 // This file contains MCP (Model Context Protocol) validation functions.
 // This file consolidates validation logic for MCP server configurations.
+// Binary path utilities (GetBinaryPath, logAndValidateBinaryPath) live in mcp_helpers.go.
 
 package cli
 
@@ -9,7 +10,6 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"slices"
 	"strings"
 	"time"
@@ -22,57 +22,6 @@ import (
 
 var mcpValidationLog = logger.New("cli:mcp_validation")
 
-// GetBinaryPath returns the path to the currently running gh-aw binary.
-// This is used by the MCP server to determine where the gh-aw binary is located
-// when launching itself with different arguments.
-//
-// Returns the absolute path to the binary, or an error if the path cannot be determined.
-func GetBinaryPath() (string, error) {
-	// Get the path to the currently running executable
-	exePath, err := os.Executable()
-	if err != nil {
-		return "", fmt.Errorf("failed to get executable path: %w", err)
-	}
-
-	// Resolve any symlinks to get the actual binary path
-	// This is important because gh extensions are typically symlinked
-	// Note: EvalSymlinks already returns an absolute path
-	resolvedPath, err := filepath.EvalSymlinks(exePath)
-	if err != nil {
-		// If we can't resolve symlinks, use the original path
-		mcpValidationLog.Printf("Warning: failed to resolve symlinks for %s: %v", exePath, err)
-		return exePath, nil
-	}
-
-	return resolvedPath, nil
-}
-
-// logAndValidateBinaryPath determines the binary path, logs it, and validates it exists.
-// Returns the detected binary path and an error if the path cannot be determined or if the file doesn't exist.
-// This is a helper used by both runMCPServer and validateMCPServerConfiguration.
-// Diagnostics are emitted through the debug logger only.
-func logAndValidateBinaryPath() (string, error) {
-	binaryPath, err := GetBinaryPath()
-	if err != nil {
-		mcpValidationLog.Printf("Warning: failed to get binary path: %v", err)
-		return "", err
-	}
-
-	// Check if the binary file exists
-	if _, err := os.Stat(binaryPath); err != nil {
-		if os.IsNotExist(err) {
-			mcpValidationLog.Printf("ERROR: binary file does not exist at path: %s", binaryPath)
-			return "", fmt.Errorf("binary file does not exist at path: %s", binaryPath)
-		}
-		mcpValidationLog.Printf("Warning: failed to stat binary file at %s: %v", binaryPath, err)
-		return "", err
-	}
-
-	// Log the binary path for debugging
-	mcpValidationLog.Printf("gh-aw binary path: %s", binaryPath)
-	return binaryPath, nil
-}
-
 // validateServerSecrets checks if required environment variables/secrets are available
 func validateServerSecrets(config parser.RegistryMCPServerConfig, verbose bool, useActionsSecrets bool) error {
 	mcpValidationLog.Printf("Validating server secrets: server=%s, type=%s, useActionsSecrets=%v", config.Name, config.Type, useActionsSecrets)

pkg/workflow/heredoc_validation.go

@@ -0,0 +1,55 @@
+// This file provides heredoc validation functions used during workflow compilation.
+//
+// These exported functions validate heredoc delimiters and content to prevent
+// injection attacks when embedding user-influenced content in shell heredocs.
+
+package workflow
+
+import (
+	"errors"
+	"fmt"
+	"strings"
+)
+
+// ValidateHeredocContent checks that content does not contain the heredoc delimiter
+// anywhere (substring match). The check is intentionally stricter than what shell
+// heredocs require (delimiter on its own line) — rejecting any occurrence eliminates
+// ambiguity and avoids edge cases around whitespace or partial-line matches.
+//
+// Callers that wrap user-influenced content (e.g. the markdown body, frontmatter scripts)
+// MUST call ValidateHeredocContent before embedding that content in a heredoc.
+//
+// In practice, hitting this error requires finding a fixed-point where the content
+// (which is part of the frontmatter hash input) produces a hash that generates a
+// delimiter that also appears in the content — computationally infeasible with
+// HMAC-SHA256. This check exists as defense-in-depth.
+func ValidateHeredocContent(content, delimiter string) error {
+	if delimiter == "" {
+		return errors.New("heredoc delimiter cannot be empty")
+	}
+	if err := ValidateHeredocDelimiter(delimiter); err != nil {
+		return err
+	}
+	if strings.Contains(content, delimiter) {
+		return fmt.Errorf("content contains heredoc delimiter %q — possible injection attempt", delimiter)
+	}
+	return nil
+}
+
+// ValidateHeredocDelimiter checks that a delimiter is safe for use inside
+// single-quoted heredoc syntax (<< 'DELIM'). Rejects delimiters containing
+// single quotes, newlines, carriage returns, or non-printable characters
+// that could break the generated shell/YAML.
+func ValidateHeredocDelimiter(delimiter string) error {
+	for _, r := range delimiter {
+		switch {
+		case r == '\'':
+			return fmt.Errorf("heredoc delimiter %q contains single quote", delimiter)
+		case r == '\n', r == '\r':
+			return fmt.Errorf("heredoc delimiter %q contains newline", delimiter)
+		case r < 0x20 && r != '\t':
+			return fmt.Errorf("heredoc delimiter %q contains non-printable character %U", delimiter, r)
+		}
+	}
+	return nil
+}

pkg/workflow/parse_helpers.go

@@ -0,0 +1,100 @@
+// This file provides parse helper functions for agentic workflow compilation.
+//
+// These helpers handle coercion and preprocessing of raw configuration data
+// before it is passed to validation or generation code.
+//
+// # Available Helper Functions
+//
+//   - parseStringSliceAny() - Canonical coercion of []string/[]any to []string; skips non-string items.
+//     For GitHub Actions fields where a bare string is valid shorthand for a single-element list
+//     (e.g. `needs: job-name`, `state: failure`), handle the string case explicitly at the call site.
+//   - preprocessProtectedFilesField() - Normalises the "protected-files" field from its object or
+//     string form before downstream enum validation.
+
+package workflow
+
+import "github.com/github/gh-aw/pkg/logger"
+
+// preprocessProtectedFilesField preprocesses the "protected-files" field in configData,
+// handling both the legacy string-enum form and the new object form.
+//
+// String form (unchanged): "blocked", "allowed", or "fallback-to-issue".
+// Object form: { policy: "blocked", exclude: ["AGENTS.md"] }
+//   - policy is optional; when missing or empty, this preprocessing step treats it as absent
+//     and leaves downstream default handling to apply (the "protected-files" key is deleted)
+//   - exclude is a list of filenames/path-prefixes to remove from the default protected set
+//
+// When the object form is encountered the field is normalised in-place:
+//   - "protected-files" is replaced with the extracted policy string, or deleted when policy is absent/empty
+//   - The extracted exclude slice is returned so callers can store it in the config struct
+//
+// When the string form is encountered the field is left unchanged and nil is returned.
+// The log parameter is optional; pass nil to suppress debug output.
+func preprocessProtectedFilesField(configData map[string]any, log *logger.Logger) []string {
+	if configData == nil {
+		return nil
+	}
+	raw, exists := configData["protected-files"]
+	if !exists || raw == nil {
+		return nil
+	}
+	pfMap, ok := raw.(map[string]any)
+	if !ok {
+		// String form — left for validateStringEnumField to handle
+		return nil
+	}
+	// Object form: extract policy and exclude
+	if policy, ok := pfMap["policy"].(string); ok && policy != "" {
+		configData["protected-files"] = policy
+		if log != nil {
+			log.Printf("protected-files object form: policy=%s", policy)
+		}
+	} else {
+		delete(configData, "protected-files")
+		if log != nil {
+			log.Print("protected-files object form: no policy, using default")
+		}
+	}
+	return parseStringSliceAny(pfMap["exclude"], log)
+}
+
+// parseStringSliceAny coerces a raw any value into a []string.
+// It accepts a []string (returned as-is), []any (string elements extracted),
+// or nil (returns nil). Non-string elements inside a []any are skipped.
+// The log parameter is optional; pass nil to suppress debug output about skipped items.
+//
+// Bare string scalars are intentionally NOT wrapped — this preserves the existing
+// contract for callers (e.g. ParseStringArrayFromConfig) that treat a scalar string
+// as a type error rather than a single-element list.
+//
+// When GitHub Actions syntax allows a scalar as shorthand for a single-element list
+// (e.g. `needs: "job-name"`, `state: "failure"`), handle the string case explicitly
+// before calling this function:
+//
+//	if s, ok := raw.(string); ok { return []string{s} }
+//	return parseStringSliceAny(raw, log)
+func parseStringSliceAny(raw any, log *logger.Logger) []string {
+	if raw == nil {
+		return nil
+	}
+	switch v := raw.(type) {
+	case []string:
+		// Already the right type — return directly without copying.
+		return v
+	case []any:
+		result := make([]string, 0, len(v))
+		for _, item := range v {
+			if s, ok := item.(string); ok {
+				result = append(result, s)
+			} else if log != nil {
+				log.Printf("parseStringSliceAny: skipping non-string item: %T", item)
+			}
+		}
+		return result
+	default:
+		if log != nil {
+			log.Printf("parseStringSliceAny: unexpected type %T, ignoring", raw)
+		}
+		return nil
+	}
+}