Refactor three pkg/workflow largefunc hotspots into local helpers

docs/adr/43195-refactor-workflow-largefunc-hotspots.md

@@ -0,0 +1,62 @@
+# ADR-43195: Refactor Three `pkg/workflow` Largefunc Hotspots into Local Helpers
+
+**Date**: 2026-07-04
+**Status**: Draft
+**Deciders**: Unknown
+
+---
+
+### Context
+
+Three functions in `pkg/workflow` exceeded the `largefunc` linter threshold, each mixing unrelated concerns in a single body:
+
+1. **`ActionResolver.ResolveSHA`** contained an inline loop over the embedded pin set to check for a semver-compatible pin before falling back to the GitHub API. The resolution sequence (failed-run short-circuit → cache hit → embedded pin → GitHub API) was correct but obscured by the in-body loop.
+
+2. **`computeAntigravityToolsCore`** contained both wildcard detection and per-command string mapping for bash tool expansion in one flat body, making the two distinct concerns hard to read and test independently.
+
+3. **`buildInputSchema`** contained per-input schema construction inline, including metadata extraction (`description`, `required`) and property assembly, making it harder to follow the logic for each input type (`number`, `boolean`, `choice`, string fallback).
+
+### Decision
+
+We will extract focused local helpers from each hotspot without changing observable behavior:
+
+- **`resolveFromEmbeddedPins(repo, version string) (string, bool)`** — moves the semver-compatible pin scan out of `ResolveSHA`. The embedded-pin path intentionally does not write to the on-disk cache (to avoid creating root-owned files in Docker/Alpine CI environments), and the helper documents this invariant explicitly with a comment. The extracted helper is not registered in the on-disk cache; that remains the exclusive responsibility of `resolveFromGitHub`.
+
+- **`appendAntigravityBashTools`** and a wildcard detector — separate wildcard detection from per-command `run_shell_command(...)` generation. The canonical output format (`run_shell_command(...)`) is unchanged.
+
+- **`buildInputSchemaProperty`**, **`getInputSchemaMetadata`**, and **`newInputSchemaProperty`** — break per-input schema assembly into three focused helpers. `buildInputSchemaProperty` owns type dispatch; `getInputSchemaMetadata` owns description/required extraction with fallback; `newInputSchemaProperty` owns property map construction with optional default.
+
+Targeted regression tests are added to lock in the behavioral invariants that motivated the extraction: embedded pins do not write to the cache, invalid `choice.options` shapes fall back to plain string properties, and non-string bash command entries are silently ignored during Antigravity tool expansion.
+
+### Alternatives Considered
+
+#### Alternative 1: Add Comments Only
+
+Add explanatory comments inside each large function to demarcate sections without extracting helpers. This avoids function proliferation.
+
+Rejected because comments do not enforce separation or reduce function length, the `largefunc` linter finding would remain, and the logic would continue to be tested only through the public surface (no targeted tests for the embedded behavior).
+
+#### Alternative 2: Move Helpers to Separate Files
+
+Move each extracted helper into a new file (e.g., `embedded_pins.go`) to make the extracted concern explicitly discoverable.
+
+Not pursued for this change because the helpers are tightly coupled to their parent functions and have no callers outside the immediate context. Moving them to separate files would add file overhead without improving navigation. Future callers can motivate a move at that time.
+
+### Consequences
+
+#### Positive
+- `largefunc` linter findings for all three hotspots are resolved without disabling the linter.
+- Each extracted helper has a documented single responsibility, making future edits easier to scope.
+- New regression tests make it harder to accidentally reintroduce the subtle embedded-pin caching bug (Docker/Alpine file ownership).
+
+#### Negative
+- Developers following a code path must now trace into helper calls even though each hop is simple.
+- Three new unexported helpers in `build_input_schema.go` increase the surface of the file without reducing its line count.
+
+#### Neutral
+- No logic changes are introduced; all behavioral invariants are preserved.
+- The existing test coverage for `ResolveSHA`, `computeAntigravityToolsCore`, and `buildInputSchema` continues to exercise the helpers indirectly through their callers.
+
+---
+
+*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.*

pkg/workflow/action_resolver.go

@@ -112,30 +112,8 @@ func (r *ActionResolver) ResolveSHA(ctx context.Context, repo, version string) (
 
 	resolverLog.Printf("Cache miss for %s@%s, checking embedded action pins", repo, version)
 
-	// Check embedded action pins for a semver-compatible version before making
-	// a network call. The embedded pins are the source-of-truth for known versions
-	// and are always available without network access. This avoids a ~1s gh-api
-	// subprocess for any action that is already covered by the embedded pin set.
-	requested := semverutil.EnsureVPrefix(version)
-	requestedVer := semverutil.ParseVersion(requested)
-	requestedIsPrecise := requestedVer != nil && requestedVer.IsPreciseVersion()
-
-	for _, pin := range actionpins.GetActionPinsByRepo(repo) {
-		pinVersion := semverutil.EnsureVPrefix(pin.Version)
-		if requestedIsPrecise {
-			if pinVersion != requested {
-				continue
-			}
-		} else if !semverutil.IsCompatible(pinVersion, requested) {
-			continue
-		}
-
-		resolverLog.Printf("Embedded pin hit for %s@%s → %s (%s)", repo, version, pin.SHA, pin.Version)
-		// Note: we intentionally do NOT call r.cache.Set() here. The embedded pins
-		// are always available in memory so there is nothing to persist, and writing
-		// to the on-disk cache would create root-owned files when compiling inside
-		// Docker containers (e.g. the Alpine CI test), preventing cleanup by the host.
-		return pin.SHA, nil
+	if sha, found := r.resolveFromEmbeddedPins(repo, version); found {
+		return sha, nil
 	}
 
 	resolverLog.Printf("No embedded pin for %s@%s, querying GitHub API", repo, version)
@@ -160,6 +138,33 @@ func (r *ActionResolver) ResolveSHA(ctx context.Context, repo, version string) (
 	return sha, nil
 }
 
+// resolveFromEmbeddedPins returns the SHA from the embedded action pin set for
+// the requested repo/version when a semver-compatible pin is available.
+func (r *ActionResolver) resolveFromEmbeddedPins(repo, version string) (string, bool) {
+	// Check embedded action pins for a semver-compatible version before making
+	// a network call. The embedded pins are the source-of-truth for known versions
+	// and are always available without network access. This avoids a ~1s gh-api
+	// subprocess for any action that is already covered by the embedded pin set.
+	requested := semverutil.EnsureVPrefix(version)
+	requestedVer := semverutil.ParseVersion(requested)
+	requestedIsPrecise := requestedVer != nil && requestedVer.IsPreciseVersion()
+
+	for _, pin := range actionpins.GetActionPinsByRepo(repo) {
+		pinVersion := semverutil.EnsureVPrefix(pin.Version)
+		if (requestedIsPrecise && pinVersion != requested) || (!requestedIsPrecise && !semverutil.IsCompatible(pinVersion, requested)) {
+			continue
+		}
+		resolverLog.Printf("Embedded pin hit for %s@%s → %s (%s)", repo, version, pin.SHA, pin.Version)
+		// Note: we intentionally do NOT call r.cache.Set() here. The embedded pins
+		// are always available in memory so there is nothing to persist, and writing
+		// to the on-disk cache would create root-owned files when compiling inside
+		// Docker containers (e.g. the Alpine CI test), preventing cleanup by the host.
+		return pin.SHA, true
+	}
+
+	return "", false
+}
+
 // ParseTagRefTSV parses the tab-separated output from the GitHub API
 // `[.object.sha, .object.type] | @tsv` jq expression.
 // It returns the object SHA and type, or an error if the output is malformed.

pkg/workflow/action_resolver_test.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/github/gh-aw/pkg/actionpins"
 	"github.com/github/gh-aw/pkg/gitutil"
 	"github.com/github/gh-aw/pkg/testutil"
 )
@@ -70,6 +71,32 @@ func TestActionResolverCache(t *testing.T) {
 	}
 }
 
+func TestActionResolverEmbeddedPinDoesNotWriteCache(t *testing.T) {
+	tmpDir := testutil.TempDir(t, "test-*")
+	cache := NewActionCache(tmpDir)
+	resolver := NewActionResolver(cache)
+
+	pin, ok := actionpins.GetLatestActionPinByRepo("actions/checkout")
+	if !ok {
+		t.Fatal("expected embedded pins for actions/checkout")
+	}
+
+	version := pin.Version
+	sha, err := resolver.ResolveSHA(context.Background(), "actions/checkout", version)
+	if err != nil {
+		t.Fatalf("expected embedded pin resolution to succeed, got: %v", err)
+	}
+	if sha != pin.SHA {
+		t.Fatalf("expected embedded SHA %q, got %q", pin.SHA, sha)
+	}
+	if _, found := cache.Get("actions/checkout", version); found {
+		t.Fatalf("expected embedded pin resolution not to write %q to cache", version)
+	}
+	if !resolver.GetUsedCacheKeys()[formatActionCacheKey("actions/checkout", version)] {
+		t.Fatalf("expected used cache keys to include actions/checkout@%s", version)
+	}
+}
+
 func TestActionResolverFailedResolutionCache(t *testing.T) {
 	// Create a cache and resolver
 	tmpDir := testutil.TempDir(t, "test-*")

pkg/workflow/antigravity_engine_test.go

@@ -487,6 +487,16 @@ func TestComputeAntigravityToolsCore(t *testing.T) {
 		assert.Contains(t, result, "run_shell_command(cat)", "Should normalize 'cat *'")
 		assert.NotContains(t, result, "run_shell_command(jq *)", "Should not emit run_shell_command with wildcard suffix")
 	})
+
+	t.Run("bash ignores non-string command entries", func(t *testing.T) {
+		tools := map[string]any{
+			"bash": []any{"grep", 123, nil},
+		}
+		result := computeAntigravityToolsCore(tools)
+		assert.Contains(t, result, "run_shell_command(grep)", "Should keep valid string commands")
+		assert.NotContains(t, result, "run_shell_command(123)", "Should ignore non-string bash entries")
+		assert.NotContains(t, result, "run_shell_command(<nil>)", "Should ignore nil bash entries")
+	})
 }
 
 func TestGenerateAntigravitySettingsStep(t *testing.T) {

pkg/workflow/antigravity_tools.go

@@ -54,40 +54,8 @@ func computeAntigravityToolsCore(tools map[string]any) []string {
 		return toolsCore
 	}
 
-	// Map bash neutral tool to run_shell_command
 	if bashConfig, hasBash := tools["bash"]; hasBash {
-		bashCommands, ok := bashConfig.([]any)
-		if !ok || len(bashCommands) == 0 {
-			// bash with no specific commands - allow all shell commands
-			antigravityToolsLog.Print("bash (no specific commands) → run_shell_command")
-			toolsCore = append(toolsCore, "run_shell_command")
-		} else {
-			// Check for wildcard (* or :*)
-			hasWildcard := false
-			for _, cmd := range bashCommands {
-				if cmdStr, ok := cmd.(string); ok && (cmdStr == "*" || cmdStr == ":*") {
-					hasWildcard = true
-					break
-				}
-			}
-			if hasWildcard {
-				antigravityToolsLog.Print("bash wildcard → run_shell_command")
-				toolsCore = append(toolsCore, "run_shell_command")
-			} else {
-				// Add an entry for each specific command: run_shell_command(cmd)
-				for _, cmd := range bashCommands {
-					if cmdStr, ok := cmd.(string); ok {
-						// Normalize trailing " *" wildcard (e.g. "jq *" → "jq") so that
-						// all engines emit the canonical prefix form (run_shell_command(jq))
-						// regardless of whether the command was written with or without the wildcard.
-						normalized, _ := normalizeBashCommand(cmdStr)
-						entry := fmt.Sprintf("run_shell_command(%s)", normalized)
-						antigravityToolsLog.Printf("bash %q → %s", cmdStr, entry)
-						toolsCore = append(toolsCore, entry)
-					}
-				}
-			}
-		}
+		toolsCore = appendAntigravityBashTools(toolsCore, bashConfig)
 	}
 
 	// Map edit neutral tool to write_file and replace (Antigravity's file write tools)
@@ -108,6 +76,48 @@ func computeAntigravityToolsCore(tools map[string]any) []string {
 	return toolsCore
 }
 
+// appendAntigravityBashTools maps the neutral bash tool configuration to
+// Antigravity shell tool entries and appends them to toolsCore.
+func appendAntigravityBashTools(toolsCore []string, bashConfig any) []string {
+	bashCommands, ok := bashConfig.([]any)
+	if !ok || len(bashCommands) == 0 {
+		antigravityToolsLog.Print("bash (no specific commands) → run_shell_command")
+		return append(toolsCore, "run_shell_command")
+	}
+	if hasAntigravityBashWildcard(bashCommands) {
+		antigravityToolsLog.Print("bash wildcard → run_shell_command")
+		return append(toolsCore, "run_shell_command")
+	}
+
+	for _, cmd := range bashCommands {
+		cmdStr, ok := cmd.(string)
+		if !ok {
+			continue
+		}
+		// Normalize trailing " *" wildcard (e.g. "jq *" → "jq") so that
+		// all engines emit the canonical prefix form (run_shell_command(jq))
+		// regardless of whether the command was written with or without the wildcard.
+		normalized, _ := normalizeBashCommand(cmdStr)
+		entry := fmt.Sprintf("run_shell_command(%s)", normalized)
+		antigravityToolsLog.Printf("bash %q → %s", cmdStr, entry)
+		toolsCore = append(toolsCore, entry)
+	}
+
+	return toolsCore
+}
+
+// hasAntigravityBashWildcard reports whether the bash command allowlist
+// contains the unrestricted '*' or ':*' wildcard forms.
+func hasAntigravityBashWildcard(bashCommands []any) bool {
+	for _, cmd := range bashCommands {
+		cmdStr, ok := cmd.(string)
+		if ok && (cmdStr == "*" || cmdStr == ":*") {
+			return true
+		}
+	}
+	return false
+}
+
 // generateAntigravitySettingsStep creates a GitHub Actions step that writes the
 // Antigravity CLI project settings file (.antigravity/settings.json) before execution.
 //

pkg/workflow/arc_dind_artifacts.go

@@ -15,9 +15,9 @@ import (
 func rewriteTmpGhAwPathsForArcDind(paths []string) []string {
 	result := make([]string, len(paths))
 	for i, p := range paths {
-		if strings.HasPrefix(p, constants.TmpGhAwDirSlash) {
+		if suffix, ok := strings.CutPrefix(p, constants.TmpGhAwDirSlash); ok {
 			// /tmp/gh-aw/foo → ${{ runner.temp }}/gh-aw/foo
-			result[i] = constants.GhAwRootDir + "/" + strings.TrimPrefix(p, constants.TmpGhAwDirSlash)
+			result[i] = constants.GhAwRootDir + "/" + suffix
 		} else if p == constants.TmpGhAwDir {
 			result[i] = constants.GhAwRootDir
 		} else {