Refactor AWF helpers into focused workflow modules#48633
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 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
Splits AWF integration and tests into focused modules while preserving existing behavior and APIs.
Changes:
- Separates command, capability, environment, digest, and ARC/DinD logic.
- Reorganizes the monolithic test suite by domain.
- Retains shared configuration and workflow-call helpers.
Show a summary per file
| File | Description |
|---|---|
awf_helpers.go |
Retains shared configuration helpers. |
awf_command.go |
Houses command assembly. |
awf_command_builder.go |
Builds arguments and shell wrappers. |
awf_capabilities.go |
Contains version capability gates. |
awf_env.go |
Handles environment exclusions and credits. |
awf_digest.go |
Resolves image digests. |
awf_arc_dind.go |
Handles ARC/DinD rewriting. |
awf_helpers_test.go |
Removes the monolithic test suite. |
awf_command_test.go |
Tests command assembly. |
awf_args_test.go |
Tests argument construction. |
awf_capabilities_test.go |
Tests capability gates. |
awf_env_test.go |
Tests environment handling. |
awf_digest_test.go |
Tests digest resolution. |
awf_arc_dind_test.go |
Tests ARC/DinD behavior. |
awf_api_helpers_test.go |
Tests API helper behavior. |
awf_custom_api_targets_test.go |
Tests custom API targets. |
awf_provider_targets_test.go |
Tests provider targets. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Medium
| // BuildAWFCommand builds a complete AWF command with all arguments. | ||
| // This consolidates the AWF command building logic that was duplicated across | ||
| // Copilot, Claude, and Codex engines. |
| // When AWF also supports chroot config (v0.27.1+), the Python patch body is embedded inside | ||
| // the same if-block so the script only contains one DOCKER_HOST condition check. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (37 tests)
Quality Signals ✅
Test Inflation AnalysisTwo files exceed the 2:1 ratio:
These ratios are acceptable for configuration helper functions with multiple version-dependent code paths and shell-script generation. Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Refactor AWF helpers into focused workflow modules
The decomposition is well-structured — splitting a 1000-line mixed-responsibility file into seven focused modules (command, builder, capabilities, env, digest, arc-dind, helpers) with matching test splits is a clean improvement. Behaviour is preserved as claimed.
Two issues found:
-
Blocking — nil dereference in
ComputeAWFExcludeEnvVarNames(awf_env.goline 106):workflowDatafields are accessed without a nil guard; the function lacks the top-levelif workflowData == nil { return coreSecretVarNames }that comparable helpers use. This is a latent panic. -
Non-blocking — duplicated run-type switch in
BuildAWFCommand(awf_command.golines ~152-168): theIsEvalsRun/IsDetectionRunswitch appears twice in the same block. These must stay in sync if a new run type is added.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 90 AIC · ⌖ 5.96 AIC · ⊞ 5K
| } | ||
|
|
||
| // 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.
Nil dereference in ComputeAWFExcludeEnvVarNames
workflowData is dereferenced without a nil guard. Line 106 calls workflowData.ParsedTools, line 111 workflowData.Tools, line 118 workflowData.MCPScripts, and line 155 workflowData.ExcludedEnv — all of which panic if workflowData is nil. The function accepts a pointer but has no top-level nil guard, unlike similar helpers (HasMCPServers, isGitHubCLIModeEnabled) which safely handle nil.
Suggested fix:
func ComputeAWFExcludeEnvVarNames(workflowData *WorkflowData, coreSecretVarNames []string) []string {
if workflowData == nil {
return coreSecretVarNames
}
// ... rest unchanged
}@copilot please address this.
| defaultMaxAICredits := strconv.FormatInt(constants.DefaultMaxAICredits, 10) | ||
| if config.WorkflowData != nil { | ||
| switch { | ||
| case config.WorkflowData.IsEvalsRun: |
There was a problem hiding this comment.
Duplicated run-type switch in BuildAWFCommand
The IsEvalsRun/IsDetectionRun switch appears twice in the same if block — once to pick defaultMaxAICredits (lines 150-156) and again to pick the right compilerenv.Build* expression (lines 164-168). Both blocks must be updated together if a new run type is ever added, creating a silent divergence risk.
Suggested refactor — compute the run type once:
runType := runTypeStandard
if config.WorkflowData != nil {
switch {
case config.WorkflowData.IsEvalsRun:
runType = runTypeEvals
case config.WorkflowData.IsDetectionRun:
runType = runTypeDetection
}
}
// then use runType for both defaultMaxAICredits and expr selection@copilot please address this.
There was a problem hiding this comment.
Verdict: Request changes — mostly a clean mechanical split, but one real test-coverage regression must be fixed before merge.
Verified function-level parity between the original awf_helpers.go/awf_helpers_test.go and all seven new split files: no functions were dropped, added, or altered, go build ./... and go vet ./... are clean, and no duplicated consts/vars across the new files. The organizational split is faithful to the stated goal.
💡 Themes and verification notes
- Compared
funcsignatures across the old monolith and the new combined files: zero diffs. - Manually reviewed
awf_env.go,awf_digest.go,awf_capabilities.go,awf_arc_dind.go,awf_command_builder.go, and the trimmedawf_helpers.goline-by-line — logic, constants, and doc comments carried over verbatim with no behavior drift. - One blocking finding:
TestBuildAWFArgsCliProxy(testing realBuildAWFArgs--difc-proxy-host/--difc-proxy-ca-certinjection behavior, not just the capability-gate predicate) was dropped during the test-file split. Confirmed by diffing test function names between the deletedawf_helpers_test.goand the new split test files — this was the only missing test function out of ~38. Flagged as a review comment onawf_args_test.go. - No other coverage gaps found; all other original test functions have a home in the new split files.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 75.5 AIC · ⌖ 4.51 AIC · ⊞ 7.4K
Comment /review to run again
| // But should still emit --enable-host-access for backward compat | ||
| assert.Contains(t, args, "--enable-host-access", "Should still emit --enable-host-access for legacy mode") | ||
| }) | ||
| } |
There was a problem hiding this comment.
Test coverage regression: TestBuildAWFArgsCliProxy (4 subtests exercising BuildAWFArgs's actual --difc-proxy-host/--difc-proxy-ca-cert arg injection and version gating) from the deleted awf_helpers_test.go was dropped during the split and not migrated into this file.
💡 Details
The original pkg/workflow/awf_helpers_test.go (lines 1349-1526, verified via git show <base>:pkg/workflow/awf_helpers_test.go) contained TestBuildAWFArgsCliProxy, which asserted that BuildAWFArgs correctly:
- omits
--difc-proxy-host/--difc-proxy-ca-cert/deprecated--enable-cli-proxy/--cli-proxy-policywhen thecli-proxyfeature flag is absent - includes
--difc-proxy-host host.docker.internal:18443and--difc-proxy-ca-certwhen cli-proxy is enabled and the firewall version supports it - version-gates the flags correctly against
awfSupportsCliProxy
Only TestAWFSupportsCliProxy (in awf_capabilities_test.go) survived the split, and it only tests the boolean capability-gate predicate — not the actual BuildAWFArgs arg-construction/injection behavior. This is a real functional-behavior test, not boilerplate, and its absence means a regression in the CLI-proxy arg-building logic in awf_command_builder.go/awf_command.go would go undetected by CI.
Fix: port TestBuildAWFArgsCliProxy from the original file (same subtests) into awf_args_test.go, since that's where the rest of BuildAWFArgs arg-construction tests now live.
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (3,413 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 MatterADRs 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting with a few targeted improvements; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Stale doc comment (
awf_digest.go):agent-actis appended in code but omitted from the function's example format string — easy one-liner fix. - Module boundary leak (
awf_env.go):injectMaxAICreditsExpressionis a JSON config manipulation helper with its only call site inawf_command.go; it should live there (or in aawf_config.go). - Duplicate switch logic (
awf_command.go): Two nearly identicalIsEvalsRun/IsDetectionRunswitches within the same block can be collapsed into one, removing the silent redundancy indefaultMaxAICredits. - Thin test file for a large function (
awf_command_test.go):BuildAWFCommandis 399 lines; the dedicated test file has 2 cases. Key paths (tool-cache probe, ARC/DinD pre-create, config-file printf,ResolveMaxAICreditsFromEnv) are tested elsewhere or not at all.
Positive Highlights
- ✅ Excellent decomposition: seven focused modules with clear, non-overlapping responsibilities.
- ✅ Test suite restructured in parallel — each domain now has its own
_test.go. - ✅ Public API surface preserved; this is a pure organisational refactor.
- ✅ Thorough nil-safety across all new modules.
- ✅ Sorting of
--exclude-envargs for deterministic lock-file output is a nice correctness touch.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 68.2 AIC · ⌖ 5.24 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| // buildAWFImageTagWithDigests returns an image tag value for AWF's --image-tag flag. | ||
| // When known firewall container digests are available, it appends AWF's digest | ||
| // metadata format: |
There was a problem hiding this comment.
[/codebase-design] The doc comment lists four digest keys (squid, agent, api-proxy, cli-proxy) but the code also appends agent-act (line 32). The comment is stale and will mislead anyone reasoning about which images are digest-pinned.
💡 Suggested fix
Update both doc examples to include agent-act:
// <tag>,squid=sha256:...,agent=sha256:...,agent-act=sha256:...,api-proxy=sha256:...,cli-proxy=sha256:...
// For arc-dind: same + ,build-tools=sha256:...
@copilot please address this.
| // shellEscapeArgWithVarPreserved is then used to double-quote the JSON arg while | ||
| // preserving the ${varName} reference for bash expansion and escaping bare $ signs | ||
| // (e.g. "$schema" → "\$schema"). | ||
| func injectMaxAICreditsExpression(awfConfigJSON string, expr string) string { |
There was a problem hiding this comment.
[/codebase-design] injectMaxAICreditsExpression manipulates raw config JSON and is only called from awf_command.go. Its current home in awf_env.go (env exclusion / credits env vars) breaks the module boundary described in the PR description.
💡 Why this matters
The split declares awf_env.go owns env exclusion and credits-env injection, while awf_command.go owns command assembly. injectMaxAICreditsExpression is a JSON string manipulation step used only at command-assembly time — it belongs in awf_command.go alongside its sole call site, or in a new awf_config.go.
Moving it will make awf_env.go a clean leaf module with no dependency on JSON config shape.
@copilot please address this.
| cmd := BuildAWFCommand(config) | ||
| assert.NotContains(t, cmd, "--allow-host-service-ports", "Should NOT emit --allow-host-service-ports in strict mode") | ||
| }) | ||
| } |
There was a problem hiding this comment.
[/tdd] awf_command_test.go covers only 2 scenarios for BuildAWFCommand: service-port legacy mode and the credits-expression routing check. The 399-line BuildAWFCommand function has significant untested paths — ARC/DinD mount pre-creation, tool-cache probe, config-file printf setup, and the ResolveMaxAICreditsFromEnv branch.
💡 Suggested additions
At minimum, add:
- A test verifying
mkdir -pis emitted for ARC/DinD mounts (topology = ArcDind). - A test for
ResolveMaxAICreditsFromEnv=trueto confirm the shell-variable form (${GH_AW_MAX_AI_CREDITS:-1000}) is used instead of the Actions expression. - A test that
awf-config.jsonprintf line appears in the output.
awf_arc_dind_test.go covers TestBuildAWFCommand_ArcDindPreCreatesMountDirs but it is isolated in a different file — a note or cross-reference here would help discoverability.
@copilot please address this.
| defaultMaxAICredits := strconv.FormatInt(constants.DefaultMaxAICredits, 10) | ||
| if config.WorkflowData != nil { | ||
| switch { | ||
| case config.WorkflowData.IsEvalsRun: |
There was a problem hiding this comment.
[/codebase-design] There are two consecutive switch blocks on IsEvalsRun/IsDetectionRun (lines 152–155 for defaultMaxAICredits, lines 164–168 for expr) that are almost identical. The first is redundant — defaultMaxAICredits is set but then overridden when ResolveMaxAICreditsFromEnv is false (the common path), which always picks expr from the second switch.
💡 Suggested refactor
Collapse into a single switch that sets both defaultMaxAICredits and the right builder function, then derive expr once:
creditsDefault := strconv.FormatInt(constants.DefaultMaxAICredits, 10)
buildExpr := compilerenv.BuildDefaultMaxAICreditsExpression
if config.WorkflowData != nil && (config.WorkflowData.IsEvalsRun || config.WorkflowData.IsDetectionRun) {
creditsDefault = strconv.FormatInt(constants.DefaultDetectionMaxAICredits, 10)
if config.WorkflowData.IsEvalsRun {
buildExpr = compilerenv.BuildDefaultEvalsMaxAICreditsExpression
} else {
buildExpr = compilerenv.BuildDefaultDetectionMaxAICreditsExpression
}
}This also makes it obvious that evals and detection share the same fallback integer but differ in the expression builder.
@copilot please address this.
pkg/workflow/awf_helpers.gohad grown into a large mixed-responsibility file, with its coverage concentrated in an equally large companion test file. This change decomposes both into smaller domain-focused units while preserving the existing AWF command-building surface and behavior.Code split by responsibility
awf_command.go: top-level AWF command assemblyawf_command_builder.go: AWF args, command prefix, shell wrappingawf_capabilities.go: AWF version/capability gatesawf_env.go: env exclusion and max-AI-credits injectionawf_digest.go: image digest lookup and tag augmentationawf_arc_dind.go: ARC/DinD path rewriting and chroot patch helpersawf_helpers.go: shared constants/config and workflow-callnetwork_allowedhandlingTests split along the same boundaries
awf_helpers_test.gowith focused test files for:Behavior kept stable
Example of the new boundary: