Split AWF helpers into focused modules - #51154
Conversation
Triage ResultCategory: refactor · Risk: low · Priority: low (score 22/100 — impact 10, urgency 5, quality 7) Notes: Early-stage WIP (0 diff yet), draft, agent still forming a plan. CI: copilot check in progress. Revisit once implementation lands and diff is non-trivial.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Splits the monolithic AWF helper implementation into focused modules while preserving existing APIs.
Changes:
- Separates command, environment, ARC/DinD, and feature-gate logic.
- Retains shared AWF scaffolding in
awf_helpers.go. - Adds two environment-helper edge-case tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/awf_helpers.go |
Retains shared configuration and scaffolding. |
pkg/workflow/awf_command_builder.go |
Houses AWF command and argument assembly. |
pkg/workflow/awf_env.go |
Houses environment filtering and credit helpers. |
pkg/workflow/awf_env_test.go |
Adds environment-helper edge-case tests. |
pkg/workflow/awf_arc_dind.go |
Houses ARC/DinD and digest helpers. |
pkg/workflow/awf_feature_flags.go |
Houses AWF version capability checks. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| // Command assembly, environment filtering, ARC/DinD handling, and feature gates | ||
| // live in awf_command_builder.go, awf_env.go, awf_arc_dind.go, and | ||
| // awf_feature_flags.go respectively. |
|
|
|
|
|
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Clean refactor: code moved verbatim into focused modules (awf_command_builder.go, awf_env.go, awf_arc_dind.go, awf_feature_flags.go) with no logic changes. Public API is unchanged and tests are included. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28.9 AIC · ⊞ 5.4K
Test Quality Sentinel 🧪 ReportScore: 100/100 ✅ Excellent SummaryThis PR refactors AWF helper modules with focused, high-quality test coverage. Two tests were added covering critical edge cases and defensive guards. Test Coverage Analysis
Quality Signals
Scoring DetailsConclusion✅ APPROVED: Test quality is excellent. Both tests target critical behavioral contracts (JSON transformation and nil-safety guards) with no redundancy. The implementation-to-test ratio is minimal and appropriate for refactoring that primarily moves code rather than changing behavior.
|
… modules Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (1059 new lines in pkg/workflow/) but does not have a linked Architecture Decision Record (ADR). Draft ADR committed to branch: docs/adr/51154-split-awf-helpers-into-focused-modules.md - please review and complete it before merging. This PR cannot merge until an ADR is linked in the PR body. 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: Michael Nygard ADR Format - required sections:
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on a correctness gap and structural issues introduced alongside the split.
📋 Key Themes & Highlights
Key Themes
-
Nil-safety regression —
ComputeAWFExcludeEnvVarNamesaccessesworkflowData.ParsedToolsandworkflowData.Toolswithout guardingworkflowData != nil(line 108–113 ofawf_env.go). The pre-existingHasMCPServerscall just above it already tolerates nil, making this inconsistency easy to miss. -
Four-way
fmt.Sprintfduplication —BuildAWFCommand(lines 289–398) duplicates the same format string four times, differing only in whetherPathSetup/configFileSetupappear. Any preamble change must be applied four times. -
Test split is incomplete —
awf_helpers_test.go(2,700 lines) was not split alongside the production code.awf_env_test.goadds only 2 tests for 191 lines of new code, andawf_command_builder.go,awf_feature_flags.go, andawf_arc_dind.gohave no companion test files. -
BuildAWFCommandfunction length — at 260+ lines the function is doing command assembly, probe generation, config-file creation, and max-AI-credits injection all in one place. A builder struct would make each part independently testable.
Positive Highlights
- ✅ Clean module boundaries: command builder, env filtering, ARC/DinD, and feature flags are now clearly separated
- ✅
awf_feature_flags.gois a model of deep-module design: thin interface overawfVersionAtLeast, every capability check in one place - ✅ Public API is unchanged — callers need no updates
- ✅ Good doc comments on all new public functions
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 68 AIC · ⊞ 7.1K
Comment /matt to run again
| } | ||
|
|
||
| // GitHub MCP server token is always a secret when the GitHub tool is present. | ||
| if hasGitHubTool(workflowData.ParsedTools) { |
There was a problem hiding this comment.
[/tdd] Missing nil guard on workflowData — direct field access will panic if called with nil.
workflowData.ParsedTools (line 108) and workflowData.Tools (line 113) are accessed directly without first checking workflowData != nil. Every call site in the existing tests passes a non-nil struct, so this gap is invisible in CI but reachable if a caller passes nil (as the existing HasMCPServers guard on line 103 already assumes is possible).
💡 Suggested fix
// After line 105 (HasMCPServers block), guard the next two accesses:
if workflowData != nil {
if hasGitHubTool(workflowData.ParsedTools) {
addUnique("GITHUB_MCP_SERVER_TOKEN")
}
for varName := range collectHTTPMCPHeaderSecrets(workflowData.Tools) {
addUnique(varName)
}
}And add a test:
func TestComputeAWFExcludeEnvVarNamesNilWorkflowData(t *testing.T) {
assert.NotPanics(t, func() {
names := ComputeAWFExcludeEnvVarNames(nil, []string{"CORE_TOKEN"})
assert.Equal(t, []string{"CORE_TOKEN"}, names)
})
}@copilot please address this.
| if config.PathSetup != "" && configFileSetup != "" { | ||
| command = fmt.Sprintf(`set -o pipefail | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s %s %s %s %s \ | ||
| -- %s 2>&1 | tee -a %s`, | ||
| writeAgentCLIStartMs, | ||
| config.PathSetup, | ||
| preCreateLog, | ||
| configFileSetup, | ||
| modelsJSONPathExport, | ||
| arcDindDockerHostProbe, | ||
| arcDindPrefixProbe, | ||
| toolCacheMountProbe, | ||
| awfShellcheckDirective, | ||
| awfCommand, | ||
| expandableArgs, | ||
| toolCacheMountRef, | ||
| arcDindDockerHostRef, | ||
| shellJoinArgs(awfArgs), | ||
| shellWrappedCommand, | ||
| shellEscapeArg(config.LogFile)) | ||
| } else if config.PathSetup != "" { | ||
| // Include path setup before AWF command (runs on host before AWF) | ||
| command = fmt.Sprintf(`set -o pipefail | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s %s %s %s %s \ | ||
| -- %s 2>&1 | tee -a %s`, | ||
| writeAgentCLIStartMs, | ||
| config.PathSetup, | ||
| preCreateLog, | ||
| modelsJSONPathExport, | ||
| arcDindDockerHostProbe, | ||
| arcDindPrefixProbe, | ||
| toolCacheMountProbe, | ||
| awfShellcheckDirective, | ||
| awfCommand, | ||
| expandableArgs, | ||
| toolCacheMountRef, | ||
| arcDindDockerHostRef, | ||
| shellJoinArgs(awfArgs), | ||
| shellWrappedCommand, | ||
| shellEscapeArg(config.LogFile)) | ||
| } else if configFileSetup != "" { | ||
| command = fmt.Sprintf(`set -o pipefail | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s %s %s %s %s \ | ||
| -- %s 2>&1 | tee -a %s`, | ||
| writeAgentCLIStartMs, | ||
| preCreateLog, | ||
| configFileSetup, | ||
| modelsJSONPathExport, | ||
| arcDindDockerHostProbe, | ||
| arcDindPrefixProbe, | ||
| toolCacheMountProbe, | ||
| awfShellcheckDirective, | ||
| awfCommand, | ||
| expandableArgs, | ||
| toolCacheMountRef, | ||
| arcDindDockerHostRef, | ||
| shellJoinArgs(awfArgs), | ||
| shellWrappedCommand, | ||
| shellEscapeArg(config.LogFile)) | ||
| } else { | ||
| command = fmt.Sprintf(`set -o pipefail | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| %s %s %s %s %s \ | ||
| -- %s 2>&1 | tee -a %s`, | ||
| writeAgentCLIStartMs, | ||
| preCreateLog, | ||
| modelsJSONPathExport, | ||
| arcDindDockerHostProbe, | ||
| arcDindPrefixProbe, | ||
| toolCacheMountProbe, | ||
| awfShellcheckDirective, | ||
| awfCommand, | ||
| expandableArgs, | ||
| toolCacheMountRef, | ||
| arcDindDockerHostRef, | ||
| shellJoinArgs(awfArgs), | ||
| shellWrappedCommand, | ||
| shellEscapeArg(config.LogFile)) | ||
| } |
There was a problem hiding this comment.
[/codebase-design] Four near-identical fmt.Sprintf blocks in BuildAWFCommand — a maintenance hazard.
The four branches (lines 289–398) differ only in whether config.PathSetup and configFileSetup are included as lines in the format string. Any future change to the shell preamble (e.g. a new probe variable) must be applied in all four places. This is exactly the kind of duplication that causes divergence bugs.
💡 Suggested approach
Build the preamble lines as a []string slice and join with \n, conditionally inserting config.PathSetup and configFileSetup:
lines := []string{"set -o pipefail", writeAgentCLIStartMs}
if config.PathSetup != "" {
lines = append(lines, config.PathSetup)
}
lines = append(lines, preCreateLog)
if configFileSetup != "" {
lines = append(lines, configFileSetup)
}
lines = append(lines,
modelsJSONPathExport,
arcDindDockerHostProbe,
arcDindPrefixProbe,
toolCacheMountProbe,
awfShellcheckDirective,
fmt.Sprintf("%s %s %s %s %s \\\n -- %s 2>&1 | tee -a %s",
awfCommand, expandableArgs, toolCacheMountRef,
arcDindDockerHostRef, shellJoinArgs(awfArgs),
shellWrappedCommand, shellEscapeArg(config.LogFile)),
)
command = strings.Join(lines, "\n")This reduces the four branches to zero and makes the ordering trivially auditable.
@copilot please address this.
| assert.NotPanics(t, func() { | ||
| applyDefaultMaxAICreditsEnvToMap(nil, nil) | ||
| }) | ||
| } |
There was a problem hiding this comment.
[/tdd] New awf_env_test.go covers only 2 of the 191 lines in awf_env.go — the highest-risk paths are untested.
ComputeAWFExcludeEnvVarNames (84 lines, public API) has no dedicated test in this new file; injectMaxAICreditsExpression is tested only for the missing-maxRuns branch; and addCliProxyGHTokenToEnv has zero coverage here. The test split should mirror the code split.
💡 What to add to awf_env_test.go
Move or copy the relevant cases from awf_helpers_test.go (e.g. TestComputeAWFExcludeEnvVarNames, TestInjectMaxAICreditsExpression) into awf_env_test.go, and add:
injectMaxAICreditsExpressionwith a validmaxRunsJSON to verify the happy-path insertionComputeAWFExcludeEnvVarNameswithworkflowData == nil(see nil-guard comment on line 108)addCliProxyGHTokenToEnvwith both CLI-proxy-enabled and disabled firewall configs
@copilot please address this.
| // providing a consistent and maintainable approach to AWF integration. | ||
| // Command assembly, environment filtering, ARC/DinD handling, and feature gates | ||
| // live in awf_command_builder.go, awf_env.go, awf_arc_dind.go, and | ||
| // awf_feature_flags.go respectively. |
There was a problem hiding this comment.
[/codebase-design] The 2,700-line awf_helpers_test.go was not split alongside the implementation — the module boundary exists in production code but not in test code.
Tests for BuildAWFCommand / BuildAWFArgs live in awf_helpers_test.go alongside tests for ComputeAWFExcludeEnvVarNames, awfSupports*, and ARC/DinD helpers. This makes the new awf_command_builder.go, awf_feature_flags.go, and awf_arc_dind.go modules feel incomplete and harder to navigate.
💡 Suggested next step
Create companion test files:
awf_command_builder_test.go—TestBuildAWFCommand*,TestBuildAWFArgs*,TestWrapCommandInShell*awf_feature_flags_test.go—TestAWFSupports*testsawf_arc_dind_test.go—TestBuildAWFImageTagWithDigests*,TestRewriteArcDindPath*
and move the corresponding test functions from awf_helpers_test.go. No behaviour change needed.
@copilot please address this.
| // fallback 400 to align with detection budgets. | ||
| // EngineConfig.MaxAICredits is 0 when no compile-time value was set | ||
| // (neither frontmatter nor detection-engine config provided one). | ||
| // In that case, emit a runtime expression that lets the org variable |
There was a problem hiding this comment.
[/codebase-design] BuildAWFCommand is 260+ lines and manages command-string assembly, ARC/DinD topology probes, config-file generation, max-AI-credits injection, and log-file setup all in one function — consider a builder struct.
The function has grown to the point where local variable names like arcDindPrefixProbe, arcDindDockerHostProbe, configFileSetup, expandableArgs, and modelsJSONPathExport must be read in sequence to understand their combined effect. A small awfCommandParts struct (or even a strings.Builder-based helper) would let each concern be tested in isolation and make the final assembly trivial to audit.
💡 Example shape
type awfCommandParts struct {
preamble []string // set -o pipefail, pathSetup, configFileSetup
probes []string // arcDindDockerHostProbe, toolCacheMountProbe
invocation string // awfCommand + expandableArgs + ...
shellDirectives string
}
func (p *awfCommandParts) String() string {
return strings.Join(append(p.preamble, p.probes..., p.shellDirectives, p.invocation), "\n")
}This is a follow-up refactor, not a blocker — raising it so the pattern is considered before the next addition.
@copilot please address this.
|
@copilot Please address the open review feedback, then run the Outstanding review items:
Branch refresh was requested via GitHub update-branch.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the test-split feedback. The 2,700-line
Stale inter-function doc comments that ended up in wrong files after extraction have also been fixed (removed where misplaced, restored with correct functions). All existing tests pass. |
Ran |
|
🎉 This pull request is included in a new release. Release: |
pkg/workflow/awf_helpers.gohad grown into a monolithic AWF implementation mixing command assembly, env filtering, ARC/DinD handling, digest lookup, and version gates. This refactor separates those responsibilities while keeping the public API unchanged.Module split
awf_command_builder.goawf_env.goawf_arc_dind.goawf_feature_flags.goawf_helpers.goto shared constants, config type, and small scaffoldingCoverage