refactor(workflow): extract helpers to fix function-length lint violations#44008
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…r helpers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage · Run §28868091227
Score breakdown: Impact 10 · Urgency 5 · Quality 13 Rationale: Draft. Extracts helpers from 4 over-limit functions in Labels:
|
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
Refactors several pkg/workflow functions that slightly exceeded the largefunc lint limit by extracting focused, package-private helpers while preserving existing behavior and keeping the logic co-located.
Changes:
- Extracted embedded action-pin semver matching into
lookupEmbeddedActionPinfromActionResolver.ResolveSHA. - Extracted bash neutral-tool expansion into
appendBashToolsfromcomputeAntigravityToolsCore. - Extracted JSON Schema property construction into
buildChoiceInputPropertyandbuildInputPropertyfrombuildInputSchema. - Extracted MCP-tool environment-variable collection into
addMCPToolEnvVarsfromCodexEngine.getShellEnvironmentPolicyVars.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/action_resolver.go | Extracts embedded action-pin lookup into a helper to reduce ResolveSHA length without changing resolution behavior. |
| pkg/workflow/antigravity_tools.go | Extracts bash tool mapping/expansion logic into appendBashTools to shorten computeAntigravityToolsCore. |
| pkg/workflow/build_input_schema.go | Extracts schema property construction helpers to reduce duplication and shorten buildInputSchema. |
| pkg/workflow/codex_engine.go | Extracts MCP tool → required env var mapping into addMCPToolEnvVars to shorten getShellEnvironmentPolicyVars. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Low
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The four extractions are clean behavioral-preserving refactors. Each helper is well-named, co-located, and package-private.
Specifically verified:
lookupEmbeddedActionPin: semver loop moved intact;resolverLogis package-level and accessible.appendBashTools: early-return on wildcard is equivalent to the originalhasWildcardflag — per-command iteration is correctly skipped in both cases.buildChoiceInputProperty:(nil, false)fall-through preserves the original "no options → scalar string" path.addMCPToolEnvVars: noCodexEnginefields used; correctly promoted to a package-level function.
No behavior changes detected. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 26.4 AIC · ⌖ 5.79 AIC · ⊞ 4.8K
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (135 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
🛠️ Agentic Maintenance updated this pull request branch. |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting on test coverage gaps for the two highest-risk extractions.
📋 Key Themes & Highlights
Summary
This is a clean lint-driven refactor. All four extractions are faithful: no logic changes, good docstrings, appropriate package-private visibility. The diff is easy to verify mechanically.
Issues Found
-
lookupEmbeddedActionPin— Contains semver matching logic (precise vs. range) with no direct unit test. The existingResolveSHAtests never exercise the embedded-pin path, so the new helper is tested only through integration with the broaderResolveSHAcall, which never reaches the embedded-pin branch in the test suite. -
addMCPToolEnvVars— A switch over three named MCP tools plus a custom code path, each with specific env-var lists. No unit test covers these branches directly. A missing env-var addition would silently produce a broken workflow config.
Positive Highlights
- ✅ All extracted helpers have clear docstrings explaining why (not just what)
- ✅
appendBashToolscorrectly uses early-return style vs. the nested if/else in the original - ✅
buildChoiceInputPropertyandbuildInputPropertyare already covered via the existingTestBuildInputSchema*suite (no gap there) - ✅
computeAntigravityToolsCorehas excellent test coverage (10+ test cases) — the extraction doesn't reduce coverage - ✅ Lint violation is fully addressed in all four files
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 95.7 AIC · ⌖ 5.55 AIC · ⊞ 6.6K
Comment /matt to run again
| return pin.SHA, true | ||
| } | ||
|
|
||
| return "", false |
There was a problem hiding this comment.
[/tdd] lookupEmbeddedActionPin contains non-trivial semver matching logic (precise vs. range version comparison) but has no direct test — existing ResolveSHA tests only cover cache-hit and network-error paths; the embedded-pin branch is untested.
💡 Suggested test
lookupEmbeddedActionPin is package-private and can be unit-tested directly:
func TestLookupEmbeddedActionPin(t *testing.T) {
t.Run("SHA returned for semver range match", func(t *testing.T) {
sha, found := lookupEmbeddedActionPin("actions/checkout", "v4")
require.True(t, found)
require.NotEmpty(t, sha)
})
t.Run("false for unknown repo", func(t *testing.T) {
_, found := lookupEmbeddedActionPin("nonexistent/action", "v1")
require.False(t, found)
})
}This locks down the semver-matching logic independently of the cache layer.
@copilot please address this.
There was a problem hiding this comment.
Done in commit b299c85. Added TestLookupEmbeddedActionPin to action_resolver_test.go covering four paths: precise-version match, semver-range match, unknown-repo not-found, and known-repo incompatible-version not-found.
| return toolsCore | ||
| } | ||
|
|
||
| // generateAntigravitySettingsStep creates a GitHub Actions step that writes the |
There was a problem hiding this comment.
[/codebase-design] appendBashTools now returns early for the wildcard path, which is an improvement — but the two-pass loop over bashCommands (scan for wildcard, then iterate again for specific commands) is worth noting as a minor navigability issue.
💡 Consider single-pass alternative
The current implementation iterates bashCommands twice: once to detect any wildcard, and once to produce per-command entries. This means if any wildcard is mid-slice, commands before it are silently ignored during the second pass (which is fine, since the function returns early on wildcard). But a single-pass form makes the intent explicit and eliminates the implicit dependency between the two loops:
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")
}
normalized, _ := normalizeBashCommand(cmdStr)
entry := fmt.Sprintf("run_shell_command(%s)", normalized)
antigravityToolsLog.Printf("bash %q → %s", cmdStr, entry)
toolsCore = append(toolsCore, entry)
}
return toolsCoreThis is a nit — the current version is correct and the tests cover both paths.
@copilot please address this.
There was a problem hiding this comment.
Done in commit b299c85. Refactored to a true single-pass loop using a separate specific accumulator — wildcard (* or :*) anywhere in the list discards accumulated entries and returns only run_shell_command. The two-pass structure is gone.
There was a problem hiding this comment.
Clean refactoring — no behavioral issues found
All four function extractions faithfully preserve the original logic. Spot-checked:
lookupEmbeddedActionPin:resolverLogis package-level; semver-match loop and early-return on hit are identical to the inline version.appendBashTools: early-returnon wildcard is semantically equivalent to the original flag+break+if/elsepattern.buildChoiceInputProperty/buildInputProperty:inputType = "string"moved after thebuildChoiceInputPropertycall, butinputTypeis already"string"by initialization — no observable difference.addMCPToolEnvVars: map is passed by reference in Go; mutations are visible to the caller — extraction is safe.
Lint goal achieved with zero behavior change.
🔎 Code quality review by PR Code Quality Reviewer · 124.6 AIC · ⌖ 6.19 AIC · ⊞ 5.4K
Comment /review to run again
|
@copilot please run the
|
…edActionPin tests - Refactor appendBashTools to single-pass loop using a separate `specific` accumulator. Wildcard (* or :*) anywhere in the list discards accumulated entries and returns only run_shell_command, preserving the original two-pass semantics regardless of command ordering. This is important because DefaultBashTools injection in tools.go places specific commands before the wildcard sentinel. - Add TestLookupEmbeddedActionPin to action_resolver_test.go: covers precise-version match, semver-range match, unknown-repo not-found, and known-repo incompatible-version not-found paths. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Addressed both unresolved review threads in commit
Local validation: |
|
@copilot please run the
|
Addresses a slice of the
largefunclint backlog (limit: 60 lines) inpkg/workflow. Four functions just over the limit were split into focused helpers with no behavior changes.Extractions
action_resolver.go—ResolveSHA(67→~52 lines): extractedlookupEmbeddedActionPin, which encapsulates the embedded-pin semver matching loopantigravity_tools.go—computeAntigravityToolsCore(65→~35 lines): extractedappendBashTools, which handles the bash neutral-tool →run_shell_commandexpansion including wildcard and per-command casesbuild_input_schema.go—buildInputSchema(69→~56 lines): extractedbuildChoiceInputProperty(enum/choice schema) andbuildInputProperty(scalar schema), removing an inline map-construction block repeated for choice vs. non-choice pathscodex_engine.go—getShellEnvironmentPolicyVars(61→~27 lines): extractedaddMCPToolEnvVars, which owns the switch mapping known MCP tool names (github,agentic-workflows,safe-outputs, custom) to their required environment variablesAll new helpers are package-private and co-located with the refactored function.