From 9e220ba21c313cff608c97ecd528774c9dd4e460 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 3 Jul 2026 13:21:28 +0000 Subject: [PATCH 1/5] Initial plan From 74f7dd2eb24f8503e10263df651d8714760d6c3c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 3 Jul 2026 13:31:10 +0000 Subject: [PATCH 2/5] Refactor workflow largefunc slice Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/action_resolver.go | 54 +++++++------ pkg/workflow/action_resolver_test.go | 27 +++++++ pkg/workflow/antigravity_engine_test.go | 10 +++ pkg/workflow/antigravity_tools.go | 72 +++++++++-------- pkg/workflow/build_input_schema.go | 102 ++++++++++++------------ pkg/workflow/build_input_schema_test.go | 20 +++++ 6 files changed, 177 insertions(+), 108 deletions(-) diff --git a/pkg/workflow/action_resolver.go b/pkg/workflow/action_resolver.go index 424b072eebc..677c72acc32 100644 --- a/pkg/workflow/action_resolver.go +++ b/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,34 @@ func (r *ActionResolver) ResolveSHA(ctx context.Context, repo, version string) ( return sha, nil } +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 { + continue + } + if !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. diff --git a/pkg/workflow/action_resolver_test.go b/pkg/workflow/action_resolver_test.go index bd1ef33a2d6..2128e73a2f4 100644 --- a/pkg/workflow/action_resolver_test.go +++ b/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) + + pins := actionpins.GetActionPinsByRepo("actions/checkout") + if len(pins) == 0 { + t.Fatal("expected embedded pins for actions/checkout") + } + + version := pins[0].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 != pins[0].SHA { + t.Fatalf("expected embedded SHA %q, got %q", pins[0].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-*") diff --git a/pkg/workflow/antigravity_engine_test.go b/pkg/workflow/antigravity_engine_test.go index c364e1d2f1a..665ad6a1494 100644 --- a/pkg/workflow/antigravity_engine_test.go +++ b/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()", "Should ignore nil bash entries") + }) } func TestGenerateAntigravitySettingsStep(t *testing.T) { diff --git a/pkg/workflow/antigravity_tools.go b/pkg/workflow/antigravity_tools.go index d817d4d2198..2810e8647cf 100644 --- a/pkg/workflow/antigravity_tools.go +++ b/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,44 @@ func computeAntigravityToolsCore(tools map[string]any) []string { return 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 +} + +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. // diff --git a/pkg/workflow/build_input_schema.go b/pkg/workflow/build_input_schema.go index be8185725f7..0c340bea8dc 100644 --- a/pkg/workflow/build_input_schema.go +++ b/pkg/workflow/build_input_schema.go @@ -20,60 +20,10 @@ func buildInputSchema(inputs map[string]any, descriptionFn func(inputName string required = []string{} for inputName, inputDef := range inputs { - inputDefMap, ok := inputDef.(map[string]any) + prop, inputType, inputRequired, ok := buildInputSchemaProperty(inputName, inputDef, descriptionFn) if !ok { - buildInputSchemaLog.Printf("Skipping input %q: expected map, got %T", inputName, inputDef) continue } - - inputType := "string" - inputDescription := descriptionFn(inputName) - inputRequired := false - - if desc, ok := inputDefMap["description"].(string); ok && desc != "" { - inputDescription = desc - } - - if req, ok := inputDefMap["required"].(bool); ok { - inputRequired = req - } - - // Map GitHub Actions input types to JSON Schema types. - if typeStr, ok := inputDefMap["type"].(string); ok { - switch typeStr { - case "number": - inputType = "number" - case "boolean": - inputType = "boolean" - case "choice": - inputType = "string" - if options, ok := inputDefMap["options"].([]any); ok && len(options) > 0 { - prop := map[string]any{ - "type": inputType, - "description": inputDescription, - "enum": options, - } - if defaultVal, ok := inputDefMap["default"]; ok { - prop["default"] = defaultVal - } - properties[inputName] = prop - if inputRequired { - required = append(required, inputName) - } - continue - } - case "environment": - inputType = "string" - } - } - - prop := map[string]any{ - "type": inputType, - "description": inputDescription, - } - if defaultVal, ok := inputDefMap["default"]; ok { - prop["default"] = defaultVal - } buildInputSchemaLog.Printf("Input %q: type=%s, required=%v", inputName, inputType, inputRequired) properties[inputName] = prop @@ -85,3 +35,53 @@ func buildInputSchema(inputs map[string]any, descriptionFn func(inputName string buildInputSchemaLog.Printf("Built input schema: %d properties, %d required", len(properties), len(required)) return properties, required } + +func buildInputSchemaProperty(inputName string, inputDef any, descriptionFn func(inputName string) string) (prop map[string]any, inputType string, inputRequired bool, ok bool) { + inputDefMap, ok := inputDef.(map[string]any) + if !ok { + buildInputSchemaLog.Printf("Skipping input %q: expected map, got %T", inputName, inputDef) + return nil, "", false, false + } + + inputType = "string" + inputDescription, inputRequired := getInputSchemaMetadata(inputName, inputDefMap, descriptionFn) + if typeStr, ok := inputDefMap["type"].(string); ok { + switch typeStr { + case "number": + inputType = "number" + case "boolean": + inputType = "boolean" + case "choice": + if options, ok := inputDefMap["options"].([]any); ok && len(options) > 0 { + prop = newInputSchemaProperty(inputType, inputDescription, inputDefMap) + prop["enum"] = options + return prop, inputType, inputRequired, true + } + } + } + + return newInputSchemaProperty(inputType, inputDescription, inputDefMap), inputType, inputRequired, true +} + +func getInputSchemaMetadata(inputName string, inputDefMap map[string]any, descriptionFn func(inputName string) string) (string, bool) { + inputDescription := descriptionFn(inputName) + if desc, ok := inputDefMap["description"].(string); ok && desc != "" { + inputDescription = desc + } + inputRequired := false + if req, ok := inputDefMap["required"].(bool); ok { + inputRequired = req + } + return inputDescription, inputRequired +} + +func newInputSchemaProperty(inputType, inputDescription string, inputDefMap map[string]any) map[string]any { + prop := map[string]any{ + "type": inputType, + "description": inputDescription, + } + if defaultVal, ok := inputDefMap["default"]; ok { + prop["default"] = defaultVal + } + return prop +} diff --git a/pkg/workflow/build_input_schema_test.go b/pkg/workflow/build_input_schema_test.go index d45cc38c11d..8eaf9aa9611 100644 --- a/pkg/workflow/build_input_schema_test.go +++ b/pkg/workflow/build_input_schema_test.go @@ -246,6 +246,26 @@ func TestBuildInputSchemaChoiceWithoutOptions(t *testing.T) { assert.False(t, hasEnum, "should not have enum when no options") } +// TestBuildInputSchemaChoiceWithInvalidOptionsType tests choice type with a non-slice +// options field falls back to a regular string property. +func TestBuildInputSchemaChoiceWithInvalidOptionsType(t *testing.T) { + inputs := map[string]any{ + "env": map[string]any{ + "type": "choice", + "description": "Environment", + "options": "staging", + }, + } + + properties, _ := buildInputSchema(inputs, defaultDescFn) + + prop, ok := properties["env"].(map[string]any) + require.True(t, ok, "env property should exist") + assert.Equal(t, "string", prop["type"], "choice with invalid options type maps to string") + _, hasEnum := prop["enum"] + assert.False(t, hasEnum, "should not have enum when options is not a slice") +} + // TestBuildInputSchemaUnknownType tests that unknown type defaults to string. func TestBuildInputSchemaUnknownType(t *testing.T) { inputs := map[string]any{ From 885ec940c131066a5c2c08c5c2012c58591ecb9b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 3 Jul 2026 13:34:54 +0000 Subject: [PATCH 3/5] Document extracted workflow helpers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/action_resolver.go | 7 +++---- pkg/workflow/antigravity_tools.go | 4 ++++ pkg/workflow/build_input_schema.go | 6 ++++++ 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/pkg/workflow/action_resolver.go b/pkg/workflow/action_resolver.go index 677c72acc32..9d2977170f2 100644 --- a/pkg/workflow/action_resolver.go +++ b/pkg/workflow/action_resolver.go @@ -138,6 +138,8 @@ 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 @@ -149,10 +151,7 @@ func (r *ActionResolver) resolveFromEmbeddedPins(repo, version string) (string, for _, pin := range actionpins.GetActionPinsByRepo(repo) { pinVersion := semverutil.EnsureVPrefix(pin.Version) - if requestedIsPrecise && pinVersion != requested { - continue - } - if !requestedIsPrecise && !semverutil.IsCompatible(pinVersion, requested) { + 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) diff --git a/pkg/workflow/antigravity_tools.go b/pkg/workflow/antigravity_tools.go index 2810e8647cf..f17183175ff 100644 --- a/pkg/workflow/antigravity_tools.go +++ b/pkg/workflow/antigravity_tools.go @@ -76,6 +76,8 @@ 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 { @@ -104,6 +106,8 @@ func appendAntigravityBashTools(toolsCore []string, bashConfig any) []string { 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) diff --git a/pkg/workflow/build_input_schema.go b/pkg/workflow/build_input_schema.go index 0c340bea8dc..bc02be0c330 100644 --- a/pkg/workflow/build_input_schema.go +++ b/pkg/workflow/build_input_schema.go @@ -36,6 +36,8 @@ func buildInputSchema(inputs map[string]any, descriptionFn func(inputName string return properties, required } +// buildInputSchemaProperty converts a single workflow input definition into a +// JSON Schema property plus its resolved type and required flag. func buildInputSchemaProperty(inputName string, inputDef any, descriptionFn func(inputName string) string) (prop map[string]any, inputType string, inputRequired bool, ok bool) { inputDefMap, ok := inputDef.(map[string]any) if !ok { @@ -63,6 +65,8 @@ func buildInputSchemaProperty(inputName string, inputDef any, descriptionFn func return newInputSchemaProperty(inputType, inputDescription, inputDefMap), inputType, inputRequired, true } +// getInputSchemaMetadata resolves the effective description and required flag +// for an input definition, applying the description fallback when needed. func getInputSchemaMetadata(inputName string, inputDefMap map[string]any, descriptionFn func(inputName string) string) (string, bool) { inputDescription := descriptionFn(inputName) if desc, ok := inputDefMap["description"].(string); ok && desc != "" { @@ -75,6 +79,8 @@ func getInputSchemaMetadata(inputName string, inputDefMap map[string]any, descri return inputDescription, inputRequired } +// newInputSchemaProperty builds a JSON Schema property map with the provided +// type, description, and any default value present on the input definition. func newInputSchemaProperty(inputType, inputDescription string, inputDefMap map[string]any) map[string]any { prop := map[string]any{ "type": inputType, From d81e73fc5af16fdebe565b9cbd994c390e2921bc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 4 Jul 2026 03:53:54 +0000 Subject: [PATCH 4/5] Initial plan: address review feedback and add ADR Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .github/workflows/design-decision-gate.lock.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/design-decision-gate.lock.yml b/.github/workflows/design-decision-gate.lock.yml index 09f5fcf2b71..07e2f560ab0 100644 --- a/.github/workflows/design-decision-gate.lock.yml +++ b/.github/workflows/design-decision-gate.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"1add653c616dba162ddd5f114e55e1cf347eaa0248a81d822f1f25fdaa99192b","body_hash":"7e583643f095382493209c5ecbcb97e582f922d97b58574d6cfdacdf33fbf79b","strict":true,"agent_id":"claude","agent_model":"claude-sonnet-4-6","engine_versions":{"claude":"2.1.198"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"1add653c616dba162ddd5f114e55e1cf347eaa0248a81d822f1f25fdaa99192b","body_hash":"3baf377646ddd659eda7ead187d6043e41c8136688d6bcd3600649cc0de38e69","strict":true,"agent_id":"claude","agent_model":"claude-sonnet-4-6","engine_versions":{"claude":"2.1.198"}} # gh-aw-manifest: {"version":1,"secrets":["ANTHROPIC_API_KEY","GH_AW_CI_TRIGGER_TOKEN","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GH_AW_OTEL_GRAFANA_AUTHORIZATION","GH_AW_OTEL_GRAFANA_ENDPOINT","GH_AW_OTEL_SENTRY_AUTHORIZATION","GH_AW_OTEL_SENTRY_ENDPOINT","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0","version":"v7.0.0"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.22","digest":"sha256:55f06588411008b7148eb64b8dfe28602a0cce3675b36c6b190b54aca138468e","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.22@sha256:55f06588411008b7148eb64b8dfe28602a0cce3675b36c6b190b54aca138468e"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.22","digest":"sha256:afb9ff9140b17d38871dfb9dbac5ff8689ea634c2f91c435da2825192d4881c1","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.22@sha256:afb9ff9140b17d38871dfb9dbac5ff8689ea634c2f91c435da2825192d4881c1"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.22","digest":"sha256:e23e1604241f579b418e6522d938285b57ada31bc27742a65c90ee2250b1755c","pinned_image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.22@sha256:e23e1604241f579b418e6522d938285b57ada31bc27742a65c90ee2250b1755c"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.22","digest":"sha256:3cdcc1e2b4b4fe602ba69fd3e21aac7ac512d5c1fce24df4ce69dc4f98164b59","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.22@sha256:3cdcc1e2b4b4fe602ba69fd3e21aac7ac512d5c1fce24df4ce69dc4f98164b59"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.3.32","digest":"sha256:63e46b56dfd70895a701b6fc6dd0189e11e2d875f327f1781e81b31848735477","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.3.32@sha256:63e46b56dfd70895a701b6fc6dd0189e11e2d875f327f1781e81b31848735477"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b","pinned_image":"ghcr.io/github/gh-aw-node@sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b"},{"image":"ghcr.io/github/github-mcp-server:v1.5.0","digest":"sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4","pinned_image":"ghcr.io/github/github-mcp-server:v1.5.0@sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4"}]} # This file was automatically generated by gh-aw. DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # From 72719f4c3b7cf0ebf50d885189ca3cc15740cc4e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 4 Jul 2026 04:05:55 +0000 Subject: [PATCH 5/5] Address review feedback: fix named return shadowing, use GetLatestActionPinByRepo, add ADR, fix pre-existing lint/golden issues Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- ...95-refactor-workflow-largefunc-hotspots.md | 62 +++++++++++++++++++ pkg/workflow/action_resolver_test.go | 10 +-- pkg/workflow/arc_dind_artifacts.go | 4 +- pkg/workflow/build_input_schema.go | 3 +- .../playwright-cli-mode.golden | 2 +- 5 files changed, 72 insertions(+), 9 deletions(-) create mode 100644 docs/adr/43195-refactor-workflow-largefunc-hotspots.md diff --git a/docs/adr/43195-refactor-workflow-largefunc-hotspots.md b/docs/adr/43195-refactor-workflow-largefunc-hotspots.md new file mode 100644 index 00000000000..131d74761b5 --- /dev/null +++ b/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.* diff --git a/pkg/workflow/action_resolver_test.go b/pkg/workflow/action_resolver_test.go index 2128e73a2f4..95b05a36211 100644 --- a/pkg/workflow/action_resolver_test.go +++ b/pkg/workflow/action_resolver_test.go @@ -76,18 +76,18 @@ func TestActionResolverEmbeddedPinDoesNotWriteCache(t *testing.T) { cache := NewActionCache(tmpDir) resolver := NewActionResolver(cache) - pins := actionpins.GetActionPinsByRepo("actions/checkout") - if len(pins) == 0 { + pin, ok := actionpins.GetLatestActionPinByRepo("actions/checkout") + if !ok { t.Fatal("expected embedded pins for actions/checkout") } - version := pins[0].Version + 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 != pins[0].SHA { - t.Fatalf("expected embedded SHA %q, got %q", pins[0].SHA, sha) + 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) diff --git a/pkg/workflow/arc_dind_artifacts.go b/pkg/workflow/arc_dind_artifacts.go index b679240dbc1..ee566fc208e 100644 --- a/pkg/workflow/arc_dind_artifacts.go +++ b/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 { diff --git a/pkg/workflow/build_input_schema.go b/pkg/workflow/build_input_schema.go index bc02be0c330..f0533cce294 100644 --- a/pkg/workflow/build_input_schema.go +++ b/pkg/workflow/build_input_schema.go @@ -46,7 +46,8 @@ func buildInputSchemaProperty(inputName string, inputDef any, descriptionFn func } inputType = "string" - inputDescription, inputRequired := getInputSchemaMetadata(inputName, inputDefMap, descriptionFn) + var inputDescription string + inputDescription, inputRequired = getInputSchemaMetadata(inputName, inputDefMap, descriptionFn) if typeStr, ok := inputDefMap["type"].(string); ok { switch typeStr { case "number": diff --git a/pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden b/pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden index ef8413ce7bc..6b7b9c38788 100644 --- a/pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden +++ b/pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden @@ -406,7 +406,7 @@ jobs: - name: Install AWF binary run: bash "${RUNNER_TEMP}/gh-aw/actions/install_awf_binary.sh" v0.27.22 --rootless - name: Install Playwright CLI - run: npm install -g @playwright/cli@0.1.14 + run: npm install -g @playwright/cli@0.1.15 env: NPM_CONFIG_MIN_RELEASE_AGE: '3' timeout-minutes: 10