diff --git a/docs/adr/44008-extract-helpers-comply-largefunc-lint-limit.md b/docs/adr/44008-extract-helpers-comply-largefunc-lint-limit.md new file mode 100644 index 00000000000..5a1fd57de2b --- /dev/null +++ b/docs/adr/44008-extract-helpers-comply-largefunc-lint-limit.md @@ -0,0 +1,48 @@ +# ADR-44008: Extract Package-Private Helpers to Comply with the `largefunc` Lint Limit + +**Date**: 2026-07-07 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +The repository enforces a `largefunc` lint rule capping function bodies at 60 lines. Four functions in `pkg/workflow` exceeded this limit after organic growth: `ResolveSHA` (67 lines), `computeAntigravityToolsCore` (65 lines), `buildInputSchema` (69 lines), and `getShellEnvironmentPolicyVars` (61 lines). All four contained self-contained logical sub-problems—semver pin lookup, bash tool expansion, JSON Schema property construction, and MCP env-var mapping—that were implemented as inline blocks. Leaving these violations unaddressed blocks the lint gate and defers the structural clarity cost indefinitely. + +### Decision + +We will extract each over-limit inline block into a dedicated, package-private helper function co-located in the same source file, with no changes to observable behavior. The helpers (`lookupEmbeddedActionPin`, `appendBashTools`, `buildChoiceInputProperty`, `buildInputProperty`, `addMCPToolEnvVars`) own exactly one sub-problem each and are named after their responsibility rather than their caller. This satisfies the `largefunc` lint limit without raising the threshold or suppressing the check. + +### Alternatives Considered + +#### Alternative 1: Raise or Suppress the `largefunc` Threshold + +The lint threshold could be increased (e.g., from 60 to 80 lines) or suppressed with a per-file `//nolint` comment for the four affected files. This avoids any code restructuring and is trivially fast to apply. + +Rejected because it signals that the lint rule is optional and sets a precedent for future exemptions. The four functions each contained a genuinely separable concern; the lint violation was a signal worth acting on rather than silencing. + +#### Alternative 2: Promote Helpers to a New Sub-Package + +The extracted logic (semver pin matching, bash tool expansion, MCP env-var mapping) could be placed in separate sub-packages (e.g., `pkg/workflow/actionpin`, `pkg/workflow/bashtools`) to give them stronger encapsulation and independent test files. + +Rejected because the helpers are not reused outside `pkg/workflow` and the sub-package boundary would add import indirection with no practical benefit. Package-private functions in the same file achieve the same readability improvement with less structural overhead. + +### Consequences + +#### Positive +- All four functions now comply with the 60-line `largefunc` lint rule, unblocking the lint gate. +- Each extracted helper is independently unit-testable without constructing the full parent receiver. +- Callers of the parent functions are shorter and read at a higher level of abstraction. + +#### Negative +- The package now has five additional unexported functions, slightly increasing its exported-symbol footprint as seen by IDEs and documentation tools. +- Readers who want to understand a full call path must navigate one extra indirection for each extracted helper. + +#### Neutral +- All helpers are package-private (`lowercase` names) and co-located with their caller; no public API surface changes. +- The refactoring carries zero behavior change risk, but reviewers still need to verify the extraction did not alter edge-case handling (e.g., the early-`continue` logic in `buildInputSchema`'s choice branch). + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/workflow/action_resolver.go b/pkg/workflow/action_resolver.go index 424b072eebc..192d3fdd814 100644 --- a/pkg/workflow/action_resolver.go +++ b/pkg/workflow/action_resolver.go @@ -113,29 +113,12 @@ 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 + // a network call. Note: we intentionally do NOT call r.cache.Set() for + // embedded pins — they are always available in memory, and writing to the + // on-disk cache would create root-owned files inside Docker containers + // (e.g. the Alpine CI test), preventing cleanup by the host. + if sha, found := lookupEmbeddedActionPin(repo, version); found { + return sha, nil } resolverLog.Printf("No embedded pin for %s@%s, querying GitHub API", repo, version) @@ -160,6 +143,33 @@ func (r *ActionResolver) ResolveSHA(ctx context.Context, repo, version string) ( return sha, nil } +// lookupEmbeddedActionPin returns the pinned SHA for repo@version from the +// embedded action pin set. It returns ("", false) if no matching pin is found. +// The embedded pins are the source-of-truth for known versions and are always +// available without network access, avoiding a ~1s gh-api subprocess for any +// action covered by the embedded pin set. +func lookupEmbeddedActionPin(repo, version string) (string, bool) { + 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) + 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..1b792ab38e9 100644 --- a/pkg/workflow/action_resolver_test.go +++ b/pkg/workflow/action_resolver_test.go @@ -115,6 +115,49 @@ func TestActionResolverFailedResolutionCache(t *testing.T) { // Note: Testing the actual GitHub API resolution requires network access // and is tested in integration tests or with network-dependent test tags +// TestLookupEmbeddedActionPin exercises the semver matching logic inside +// lookupEmbeddedActionPin directly — independently of the cache and network +// layers. It covers the three reachable code paths: precise-version match, +// range (major/minor) match, and no match. +func TestLookupEmbeddedActionPin(t *testing.T) { + t.Run("precise version returns SHA", func(t *testing.T) { + // actions/checkout@v7.0.0 is in the embedded pin set. + sha, found := lookupEmbeddedActionPin("actions/checkout", "v7.0.0") + if !found { + t.Fatal("expected embedded pin hit for actions/checkout@v7.0.0, got not-found") + } + if sha == "" { + t.Error("expected non-empty SHA for actions/checkout@v7.0.0") + } + }) + + t.Run("semver range returns SHA for compatible pin", func(t *testing.T) { + // v7 is compatible with the pinned v7.0.0. + sha, found := lookupEmbeddedActionPin("actions/checkout", "v7") + if !found { + t.Fatal("expected embedded pin hit for actions/checkout@v7 (compatible with v7.0.0), got not-found") + } + if sha == "" { + t.Error("expected non-empty SHA for actions/checkout@v7") + } + }) + + t.Run("unknown repo returns not-found", func(t *testing.T) { + _, found := lookupEmbeddedActionPin("nonexistent/action", "v1") + if found { + t.Error("expected not-found for nonexistent/action@v1") + } + }) + + t.Run("known repo but incompatible version returns not-found", func(t *testing.T) { + // actions/checkout has no v1.x.x in the embedded pin set. + _, found := lookupEmbeddedActionPin("actions/checkout", "v1") + if found { + t.Error("expected not-found for actions/checkout@v1 (no such pin)") + } + }) +} + // TestParseTagRefTSV verifies that ParseTagRefTSV correctly parses the tab-separated // output produced by the GitHub API jq expression `[.object.sha, .object.type] | @tsv`. // This is the core parsing step used when resolving action tags to SHAs; it must diff --git a/pkg/workflow/antigravity_tools.go b/pkg/workflow/antigravity_tools.go index d817d4d2198..787882bcff2 100644 --- a/pkg/workflow/antigravity_tools.go +++ b/pkg/workflow/antigravity_tools.go @@ -56,38 +56,7 @@ func computeAntigravityToolsCore(tools map[string]any) []string { // 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 = appendBashTools(toolsCore, bashConfig) } // Map edit neutral tool to write_file and replace (Antigravity's file write tools) @@ -108,6 +77,43 @@ func computeAntigravityToolsCore(tools map[string]any) []string { return toolsCore } +// appendBashTools maps the bash neutral tool configuration to run_shell_command +// entries and appends them to toolsCore. +func appendBashTools(toolsCore []string, bashConfig any) []string { + 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") + return append(toolsCore, "run_shell_command") + } + + // Single pass over bashCommands. A separate accumulator (specific) collects + // per-command entries so that if a wildcard ("*" or ":*") is found anywhere + // in the list — even after specific commands — only "run_shell_command" is + // appended and the pre-wildcard entries are discarded. This preserves the + // semantics of "any wildcard means allow all shell commands" regardless of + // command ordering. + var specific []string + for _, cmd := range bashCommands { + cmdStr, ok := cmd.(string) + if !ok { + continue + } + if cmdStr == "*" || cmdStr == ":*" { + antigravityToolsLog.Print("bash wildcard → run_shell_command") + return append(toolsCore, "run_shell_command") + } + // 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) + specific = append(specific, entry) + } + return append(toolsCore, specific...) +} + // 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..45ea14e9742 100644 --- a/pkg/workflow/build_input_schema.go +++ b/pkg/workflow/build_input_schema.go @@ -46,34 +46,20 @@ func buildInputSchema(inputs map[string]any, descriptionFn func(inputName string 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 - } + if prop, isChoice := buildChoiceInputProperty(inputDefMap, inputDescription); isChoice { properties[inputName] = prop if inputRequired { required = append(required, inputName) } continue } + inputType = "string" case "environment": inputType = "string" } } - prop := map[string]any{ - "type": inputType, - "description": inputDescription, - } - if defaultVal, ok := inputDefMap["default"]; ok { - prop["default"] = defaultVal - } + prop := buildInputProperty(inputType, inputDescription, inputDefMap) buildInputSchemaLog.Printf("Input %q: type=%s, required=%v", inputName, inputType, inputRequired) properties[inputName] = prop @@ -85,3 +71,34 @@ 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 } + +// buildChoiceInputProperty builds a JSON Schema property for a "choice" input type. +// It returns (property, true) when options are available, or (nil, false) otherwise. +func buildChoiceInputProperty(inputDefMap map[string]any, inputDescription string) (map[string]any, bool) { + options, ok := inputDefMap["options"].([]any) + if !ok || len(options) == 0 { + return nil, false + } + prop := map[string]any{ + "type": "string", + "description": inputDescription, + "enum": options, + } + if defaultVal, ok := inputDefMap["default"]; ok { + prop["default"] = defaultVal + } + return prop, true +} + +// buildInputProperty builds a JSON Schema property map for a scalar input (string, +// number, or boolean). Use buildChoiceInputProperty for "choice" inputs instead. +func buildInputProperty(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/codex_engine.go b/pkg/workflow/codex_engine.go index b8019540cb4..f72ed1208d5 100644 --- a/pkg/workflow/codex_engine.go +++ b/pkg/workflow/codex_engine.go @@ -584,43 +584,7 @@ func (e *CodexEngine) getShellEnvironmentPolicyVars(tools map[string]any, mcpToo // Check each MCP tool for required environment variables for _, toolName := range mcpTools { - switch toolName { - case "github": - // GitHub MCP server needs GITHUB_PERSONAL_ACCESS_TOKEN - envVars["GITHUB_PERSONAL_ACCESS_TOKEN"] = struct { - }{} - case "agentic-workflows": - // Agentic workflows MCP server needs GITHUB_TOKEN - envVars["GITHUB_TOKEN"] = struct { - }{} - case "safe-outputs": - // Safe outputs MCP server needs several environment variables - envVars["GH_AW_SAFE_OUTPUTS"] = struct { - }{} - envVars["GH_AW_ASSETS_BRANCH"] = struct { - }{} - envVars["GH_AW_ASSETS_MAX_SIZE_KB"] = struct { - }{} - envVars["GH_AW_ASSETS_ALLOWED_EXTS"] = struct { - }{} - envVars["GITHUB_REPOSITORY"] = struct { - }{} - envVars["GITHUB_SERVER_URL"] = struct { - }{} - default: - // For custom MCP tools, check if they have env configuration - if toolValue, ok := tools[toolName]; ok { - if toolConfig, ok := toolValue.(map[string]any); ok { - // Extract environment variable names from env configuration - if env, hasEnv := toolConfig["env"].(map[string]any); hasEnv { - for envKey := range env { - envVars[envKey] = struct { - }{} - } - } - } - } - } + addMCPToolEnvVars(toolName, tools, envVars) } sortedEnvVars := sliceutil.SortedKeys(envVars) @@ -634,6 +598,39 @@ func (e *CodexEngine) getShellEnvironmentPolicyVars(tools map[string]any, mcpToo return includeOnlyPatterns } +// addMCPToolEnvVars adds the environment variables required by the named MCP tool +// to the envVars set. For custom tools, it reads the "env" configuration map. +func addMCPToolEnvVars(toolName string, tools map[string]any, envVars map[string]struct{}) { + switch toolName { + case "github": + // GitHub MCP server needs GITHUB_PERSONAL_ACCESS_TOKEN + envVars["GITHUB_PERSONAL_ACCESS_TOKEN"] = struct{}{} + case "agentic-workflows": + // Agentic workflows MCP server needs GITHUB_TOKEN + envVars["GITHUB_TOKEN"] = struct{}{} + case "safe-outputs": + // Safe outputs MCP server needs several environment variables + envVars["GH_AW_SAFE_OUTPUTS"] = struct{}{} + envVars["GH_AW_ASSETS_BRANCH"] = struct{}{} + envVars["GH_AW_ASSETS_MAX_SIZE_KB"] = struct{}{} + envVars["GH_AW_ASSETS_ALLOWED_EXTS"] = struct{}{} + envVars["GITHUB_REPOSITORY"] = struct{}{} + envVars["GITHUB_SERVER_URL"] = struct{}{} + default: + // For custom MCP tools, check if they have env configuration + if toolValue, ok := tools[toolName]; ok { + if toolConfig, ok := toolValue.(map[string]any); ok { + // Extract environment variable names from env configuration + if env, hasEnv := toolConfig["env"].(map[string]any); hasEnv { + for envKey := range env { + envVars[envKey] = struct{}{} + } + } + } + } + } +} + // renderShellEnvironmentPolicy generates the [shell_environment_policy] section for config.toml // This controls which environment variables are passed through to MCP servers for security func (e *CodexEngine) renderShellEnvironmentPolicy(yaml *strings.Builder, tools map[string]any, mcpTools []string) {