refactor: move outlier functions to their semantic homes

docs/adr/28282-relocate-outlier-functions-to-semantic-homes.md

@@ -0,0 +1,81 @@
+# ADR-28282: Relocate Outlier Functions to Their Semantic Homes
+
+**Date**: 2026-04-24
+**Status**: Draft
+**Deciders**: pelikhan, Copilot
+
+---
+
+## Part 1 — Narrative (Human-Friendly)
+
+### Context
+
+Several functions in `pkg/workflow` and `pkg/cli` had accumulated in files that did not own their semantic domain. `computeIntegrityCacheKey` lived in `cache.go` despite calling three helpers (`cacheIntegrityLevel`, `computePolicyHash`, `generateIntegrityAwareCacheKey`) that all resided in `cache_integrity.go`. Three npm/Node.js step-generation functions (`BuildStandardNpmEngineInstallSteps`, `BuildNpmEngineInstallStepsWithAWF`, `GetNpmBinPathSetup`) lived in `engine_helpers.go` alongside non-npm helpers rather than with the rest of npm logic in `nodejs.go`. In `pkg/cli`, `safePercent` and `formatPercent` were split across `audit_cross_run_render.go` and `audit_diff.go` despite being a compute–format pair consumed by both audit rendering files. This placement made the call graph opaque and complicated discoverability for contributors.
+
+### Decision
+
+We will relocate each outlier function to the file that already owns its semantic domain: `computeIntegrityCacheKey` moves to `cache_integrity.go`, the three npm functions move to `nodejs.go` (updating the logger reference from `engineHelpersLog` to `nodejsLog`), and `safePercent`/`formatPercent` are consolidated in a new `pkg/cli/audit_math_helpers.go`. No behavior is changed; this is a pure relocation. This extends the file-organization convention established in ADR-27325 beyond the specific files it addressed.
+
+### Alternatives Considered
+
+#### Alternative 1: Leave Functions in Place, Add Cross-File Documentation
+
+Inline comments could document that a function in file A primarily supports file B. This was rejected because comments do not enforce boundaries — over time contributors would continue adding similar "outlier" functions to the nearest convenient file rather than the semantically correct one. Documentation without structural enforcement degrades.
+
+#### Alternative 2: Extract a Shared `helpers.go` Umbrella File
+
+All small utilities could be consolidated into a single `pkg/workflow/helpers.go`. This reduces the number of files and requires no judgment about semantic ownership. It was rejected because it recreates the catch-all umbrella pattern that ADR-27325 explicitly discourages; a function's location would again fail to communicate its purpose.
+
+#### Alternative 3: Move Functions to a Sub-Package
+
+Creating sub-packages (e.g., `pkg/workflow/cacheintegrity`) would enforce hard import boundaries. This was rejected because the functions are used exclusively within their parent packages, making the added indirection and forced symbol export an unnecessary cost for what is logically internal rearrangement.
+
+### Consequences
+
+#### Positive
+- `computeIntegrityCacheKey` and all its transitive callees now reside in `cache_integrity.go`, making the entire integrity cache key computation readable in a single file.
+- All npm/Node.js step-generation logic is consolidated in `nodejs.go`, simplifying discovery and reducing cognitive load when working on engine installation steps.
+- `safePercent` and `formatPercent` share a single definition in `audit_math_helpers.go`, enabling consistent `formatPercent(safePercent(a, b))` composition across all audit rendering code.
+- Extends and reinforces the semantic file-organization convention from ADR-27325 to new areas of the codebase.
+
+#### Negative
+- Increases file count in `pkg/workflow/` and `pkg/cli/`, adding marginal navigation overhead.
+- The move adds churn to `git log` for the relocated functions; `git blame` will show the relocation commit rather than the original authorship without `--follow`.
+
+#### Neutral
+- No public API surface changes; all moved functions retain their signatures.
+- The `engine_helpers.go` header comment is updated to remove stale npm function references, keeping the file's documented scope accurate.
+- The logger reference change (`engineHelpersLog` → `nodejsLog`) is a cosmetic alignment with the new file's existing logger; no log output changes.
+
+---
+
+## 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).
+
+### Cache Integrity
+
+1. `computeIntegrityCacheKey` and all helpers it calls (`cacheIntegrityLevel`, `computePolicyHash`, `generateIntegrityAwareCacheKey`) **MUST** reside in `pkg/workflow/cache_integrity.go`.
+2. New cache key computation helpers that depend on integrity level or policy hash **MUST** be added to `cache_integrity.go` and **MUST NOT** be placed in `cache.go` or other cache-adjacent files.
+3. `cache.go` **MUST NOT** contain integrity-aware cache key computation logic; it **MUST** be limited to cache entry parsing and default key generation.
+
+### npm / Node.js Step Generation
+
+1. `BuildStandardNpmEngineInstallSteps`, `BuildNpmEngineInstallStepsWithAWF`, and `GetNpmBinPathSetup` **MUST** reside in `pkg/workflow/nodejs.go`.
+2. New npm package installation or Node.js path helpers **MUST** be added to `nodejs.go` and **MUST NOT** be placed in `engine_helpers.go`.
+3. `engine_helpers.go` **MUST NOT** contain npm package installation or Node.js binary path logic; its scope **MUST** be limited to base installation steps, secret validation, environment variable filtering, and other engine-agnostic helpers.
+4. npm-related log statements within `nodejs.go` **MUST** use `nodejsLog` and **MUST NOT** use `engineHelpersLog`.
+
+### Audit Math Helpers
+
+1. `safePercent` and `formatPercent` **MUST** be defined in `pkg/cli/audit_math_helpers.go` and **MUST NOT** be duplicated in other files within `pkg/cli`.
+2. Audit rendering code that requires percentage computation or formatting **SHOULD** compose `formatPercent(safePercent(a, b))` using the canonical helpers from `audit_math_helpers.go`.
+3. New percentage or ratio computation helpers used across multiple audit files **MUST** be placed in `audit_math_helpers.go`.
+
+### 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 integrity cache key logic in `cache.go`, placing npm helpers in `engine_helpers.go`, or duplicating `safePercent`/`formatPercent` outside `audit_math_helpers.go` — constitutes non-conformance.
+
+---
+
+*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24893632435) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/cli/audit_cross_run_render.go

@@ -386,11 +386,3 @@ func formatRunIDs(ids []int64) string {
 	}
 	return strings.Join(parts, ", ")
 }
-
-// safePercent returns percentage of part/total, returning 0 when total is 0.
-func safePercent(part, total int) float64 {
-	if total == 0 {
-		return 0
-	}
-	return float64(part) / float64(total) * 100
-}

pkg/cli/audit_diff.go

@@ -226,11 +226,6 @@ func formatVolumeChange(total1, total2 int) string {
 	return formatPercent(pctChange)
 }
 
-// formatPercent formats a float percentage with no decimal places
-func formatPercent(pct float64) string {
-	return fmt.Sprintf("%.0f%%", pct)
-}
-
 // MCPToolDiffEntry represents the diff for a single MCP tool between two runs
 type MCPToolDiffEntry struct {
 	ServerName      string `json:"server_name"`

pkg/cli/audit_math_helpers.go

@@ -0,0 +1,16 @@
+package cli
+
+import "fmt"
+
+// safePercent returns percentage of part/total, returning 0 when total is 0.
+func safePercent(part, total int) float64 {
+	if total == 0 {
+		return 0
+	}
+	return float64(part) / float64(total) * 100
+}
+
+// formatPercent formats a float percentage with no decimal places
+func formatPercent(pct float64) string {
+	return fmt.Sprintf("%.0f%%", pct)
+}

pkg/workflow/cache.go

@@ -63,40 +63,6 @@ func generateDefaultCacheKey(cacheID string) string {
 	return fmt.Sprintf("memory-%s-${{ env.GH_AW_WORKFLOW_ID_SANITIZED }}-${{ github.run_id }}", cacheID)
 }
 
-// computeIntegrityCacheKey returns the effective cache key for a cache entry, incorporating
-// the integrity level and policy hash prefix. The key always starts with
-// "memory-{integrityLevel}-{policyHash}-" to ensure cache isolation across integrity levels
-// and guard policies, even when the user has specified a custom key suffix.
-//
-// When no custom key is set the full key is:
-//
-//	memory-{integrityLevel}-{policyHash}-[{cacheID}-]{workflowID}-{runID}
-//
-// When a custom key is set, it is used as the suffix:
-//
-//	memory-{integrityLevel}-{policyHash}-{customKey}-{runID}
-//
-// githubConfig may be nil for workflows without a GitHub guard policy, in which case the
-// sentinel value "nopolicy" and the default integrity level "none" are used.
-func computeIntegrityCacheKey(cache CacheMemoryEntry, githubConfig *GitHubToolConfig) string {
-	integrityLevel := cacheIntegrityLevel(githubConfig)
-	policyHash := computePolicyHash(githubConfig)
-	integrityPrefix := fmt.Sprintf("memory-%s-%s-", integrityLevel, policyHash)
-
-	// If a custom key was explicitly set, prefix it with the integrity/policy namespace
-	// to prevent cross-integrity or cross-policy cache sharing.
-	if cache.Key != "" && cache.Key != generateDefaultCacheKey(cache.ID) {
-		customKey := cache.Key
-		runIdSuffix := "-${{ github.run_id }}"
-		if !strings.HasSuffix(customKey, runIdSuffix) {
-			customKey = customKey + runIdSuffix
-		}
-		return integrityPrefix + customKey
-	}
-
-	return generateIntegrityAwareCacheKey(cache.ID, integrityLevel, policyHash)
-}
-
 // parseCacheMemoryEntry parses a single cache-memory entry from a map
 func parseCacheMemoryEntry(cacheMap map[string]any, defaultID string) (CacheMemoryEntry, error) {
 	cacheLog.Printf("Parsing cache-memory entry: defaultID=%s", defaultID)

pkg/workflow/cache_integrity.go

@@ -188,6 +188,40 @@ func cacheIntegrityLevel(github *GitHubToolConfig) string {
 	return string(github.MinIntegrity)
 }
 
+// computeIntegrityCacheKey returns the effective cache key for a cache entry, incorporating
+// the integrity level and policy hash prefix. The key always starts with
+// "memory-{integrityLevel}-{policyHash}-" to ensure cache isolation across integrity levels
+// and guard policies, even when the user has specified a custom key suffix.
+//
+// When no custom key is set the full key is:
+//
+//	memory-{integrityLevel}-{policyHash}-[{cacheID}-]{workflowID}-{runID}
+//
+// When a custom key is set, it is used as the suffix:
+//
+//	memory-{integrityLevel}-{policyHash}-{customKey}-{runID}
+//
+// githubConfig may be nil for workflows without a GitHub guard policy, in which case the
+// sentinel value "nopolicy" and the default integrity level "none" are used.
+func computeIntegrityCacheKey(cache CacheMemoryEntry, githubConfig *GitHubToolConfig) string {
+	integrityLevel := cacheIntegrityLevel(githubConfig)
+	policyHash := computePolicyHash(githubConfig)
+	integrityPrefix := fmt.Sprintf("memory-%s-%s-", integrityLevel, policyHash)
+
+	// If a custom key was explicitly set, prefix it with the integrity/policy namespace
+	// to prevent cross-integrity or cross-policy cache sharing.
+	if cache.Key != "" && cache.Key != generateDefaultCacheKey(cache.ID) {
+		customKey := cache.Key
+		runIdSuffix := "-${{ github.run_id }}"
+		if !strings.HasSuffix(customKey, runIdSuffix) {
+			customKey = customKey + runIdSuffix
+		}
+		return integrityPrefix + customKey
+	}
+
+	return generateIntegrityAwareCacheKey(cache.ID, integrityLevel, policyHash)
+}
+
 // generateIntegrityAwareCacheKey generates the new-format cache key that includes
 // the integrity level and policy hash as prefixes.
 //

pkg/workflow/engine_helpers.go

@@ -8,23 +8,24 @@
 //
 // These helper functions are grouped here because they:
 //   - Are used by 3+ engine implementations (shared utilities)
-//   - Provide common patterns for agent installation and npm setup
+//   - Provide common patterns for agent installation and secret validation
 //   - Have a clear domain focus (engine workflow generation)
 //   - Are stable and change infrequently
 //
 // This follows the helper file conventions documented in skills/developer/SKILL.md.
 //
 // # Key Functions
 //
-// Agent Installation:
-//   - GenerateAgentInstallSteps() - Generate agent installation workflow steps
+// Base Installation:
+//   - GetBaseInstallationSteps() - Generate base installation steps for an engine
 //
-// NPM Installation:
-//   - GenerateNpmInstallStep() - Generate npm package installation step
-//   - GenerateEngineDependenciesInstallStep() - Generate engine dependencies install step
+// Secret Validation:
+//   - GenerateMultiSecretValidationStep() - Validate at least one of multiple secrets
+//   - BuildDefaultSecretValidationStep() - Build secret validation step for an engine
 //
 // Configuration:
-//   - GetClaudeSystemPrompt() - Get system prompt for Claude engine
+//   - FormatStepWithCommandAndEnv() - Format a step with command and environment variables
+//   - FilterEnvForSecrets() - Filter environment variables to only include allowed secrets
 //
 // These functions encapsulate shared logic that would otherwise be duplicated across
 // engine files, maintaining DRY principles while keeping engine-specific code separate.
@@ -114,91 +115,6 @@ func GetBaseInstallationSteps(config EngineInstallConfig, workflowData *Workflow
 	return steps
 }
 
-// BuildStandardNpmEngineInstallSteps creates standard npm installation steps for engines
-// This helper extracts the common pattern shared by Copilot, Codex, and Claude engines.
-//
-// Parameters:
-//   - packageName: The npm package name (e.g., "@github/copilot")
-//   - defaultVersion: The default version constant (e.g., constants.DefaultCopilotVersion)
-//   - stepName: The display name for the install step (e.g., "Install GitHub Copilot CLI")
-//   - cacheKeyPrefix: The cache key prefix (e.g., "copilot")
-//   - workflowData: The workflow data containing engine configuration
-//
-// Returns:
-//   - []GitHubActionStep: The installation steps including Node.js setup
-func BuildStandardNpmEngineInstallSteps(
-	packageName string,
-	defaultVersion string,
-	stepName string,
-	cacheKeyPrefix string,
-	workflowData *WorkflowData,
-) []GitHubActionStep {
-	engineHelpersLog.Printf("Building npm engine install steps: package=%s, version=%s", packageName, defaultVersion)
-
-	// Use version from engine config if provided, otherwise default to pinned version
-	version := defaultVersion
-	if workflowData.EngineConfig != nil && workflowData.EngineConfig.Version != "" {
-		version = workflowData.EngineConfig.Version
-		engineHelpersLog.Printf("Using engine config version: %s", version)
-	}
-
-	// Add npm package installation steps (includes Node.js setup)
-	// Always pass false for runInstallScripts: engine CLI installs must never run
-	// pre/post install scripts regardless of the workflow's run-install-scripts setting.
-	// This is a supply chain security requirement for the engine binary itself.
-	return GenerateNpmInstallSteps(
-		packageName,
-		version,
-		stepName,
-		cacheKeyPrefix,
-		true,  // Include Node.js setup
-		false, // Always disable scripts for engine CLI installs
-	)
-}
-
-// BuildNpmEngineInstallStepsWithAWF injects an AWF installation step between the Node.js
-// setup step and the CLI install steps when the firewall is enabled. This eliminates the
-// duplicated AWF-injection pattern shared by Claude, Gemini, and Copilot engines.
-//
-// The expected layout of npmSteps is:
-//   - npmSteps[0]  – Node.js setup step
-//   - npmSteps[1:] – CLI installation step(s)
-//
-// Parameters:
-//   - npmSteps: Pre-computed npm installation steps (from BuildStandardNpmEngineInstallSteps
-//     or GenerateCopilotInstallerSteps)
-//   - workflowData: The workflow data (used to determine firewall configuration)
-//
-// Returns:
-//   - []GitHubActionStep: Steps in order: Node.js setup, AWF (if enabled), CLI install
-func BuildNpmEngineInstallStepsWithAWF(npmSteps []GitHubActionStep, workflowData *WorkflowData) []GitHubActionStep {
-	var steps []GitHubActionStep
-
-	if len(npmSteps) > 0 {
-		steps = append(steps, npmSteps[0]) // Node.js setup step
-	}
-
-	// Inject AWF installation after Node.js setup but before the CLI install steps
-	if isFirewallEnabled(workflowData) {
-		firewallConfig := getFirewallConfig(workflowData)
-		agentConfig := getAgentConfig(workflowData)
-		var awfVersion string
-		if firewallConfig != nil {
-			awfVersion = firewallConfig.Version
-		}
-		awfInstall := generateAWFInstallationStep(awfVersion, agentConfig)
-		if len(awfInstall) > 0 {
-			steps = append(steps, awfInstall)
-		}
-	}
-
-	if len(npmSteps) > 1 {
-		steps = append(steps, npmSteps[1:]...) // CLI installation and subsequent steps
-	}
-
-	return steps
-}
-
 // GenerateMultiSecretValidationStep creates a GitHub Actions step that validates at least one
 // of multiple secrets is available.
 // secretNames: slice of secret names to validate (e.g., []string{"CODEX_API_KEY", "OPENAI_API_KEY"})
@@ -410,28 +326,6 @@ func FilterEnvForSecrets(env map[string]string, allowedNamesAndKeys []string) ma
 	return filtered
 }
 
-// GetNpmBinPathSetup returns a simple shell command that adds hostedtoolcache bin directories
-// to PATH. This is specifically for npm-installed CLIs (like Claude and Codex) that need
-// to find their binaries installed via `npm install -g`.
-//
-// Unlike GetHostedToolcachePathSetup(), this does NOT use GH_AW_TOOL_BINS because AWF's
-// native chroot mode already handles tool-specific paths (GOROOT, JAVA_HOME, etc.) via
-// AWF_HOST_PATH and the entrypoint.sh script. This function only adds the generic
-// hostedtoolcache bin directories for npm packages.
-//
-// Returns:
-//   - string: A shell command that exports PATH with hostedtoolcache bin directories prepended
-func GetNpmBinPathSetup() string {
-	// Find all bin directories in hostedtoolcache (Node.js, Python, etc.)
-	// This finds paths like /opt/hostedtoolcache/node/22.13.0/x64/bin
-	//
-	// After the find, re-prepend GOROOT/bin if set. The find returns directories
-	// alphabetically, so go/1.23.12 shadows go/1.25.0. Re-prepending GOROOT/bin
-	// ensures the Go version set by actions/setup-go takes precedence.
-	// AWF's entrypoint.sh exports GOROOT before the user command runs.
-	return `export PATH="$(find /opt/hostedtoolcache -maxdepth 4 -type d -name bin 2>/dev/null | tr '\n' ':')$PATH"; [ -n "$GOROOT" ] && export PATH="$GOROOT/bin:$PATH" || true`
-}
-
 // EngineHasValidateSecretStep checks if the engine provides a validate-secret step.
 // This is used to determine whether the secret_verification_result job output should be added.
 //