Split pkg/workflow/awf_config.go into types / schema / build / policy files - #55496
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
No blocking issues found in the changed lines.
### Review notes
This appears to be a mechanical split of pkg/workflow/awf_config.go into build/schema/policy files, and the moved symbols still resolve from the same package without changed call sites. I also checked for duplicate definitions, changed exported symbol locations, and obvious missed test/doc references in the touched files; nothing actionable stood out.
The requested grumpy-coder sub-agent was unavailable in this environment, so that advisory pass was skipped.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 6.36 AIC · ⌖ 7.77 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Clean structural refactor — no logic changes, and the file boundaries are well-chosen. Types stay in awf_config.go, construction in awf_config_build.go, schema validation in awf_config_schema.go, and policy/domain resolution in awf_config_policy.go. The shared awfConfigLog package-level variable remains accessible across all files. README and skill documentation are updated to match. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 27.2 AIC · ⌖ 8.96 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Pull request overview
Splits the monolithic AWF configuration implementation into focused type, build, schema, and policy files.
Changes:
- Moves existing logic into responsibility-specific files.
- Updates generated API documentation and release-integration guidance.
- Preserves package-level APIs and behavior.
Show a summary per file
| File | Description |
|---|---|
.github/skills/awf-release-integrator/SKILL.md |
Adds the new AWF files to integration guidance. |
pkg/workflow/README.md |
Updates the BuildAWFConfigJSON source path. |
pkg/workflow/awf_config.go |
Retains AWF configuration types and logger. |
pkg/workflow/awf_config_build.go |
Contains configuration construction and extraction logic. |
pkg/workflow/awf_config_schema.go |
Contains schema embedding and validation. |
pkg/workflow/awf_config_policy.go |
Contains model and domain policy helpers. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| | `artifact_manager.go` | `NewArtifactManager` | `func NewArtifactManager() *ArtifactManager` | NewArtifactManager creates a new artifact manager | | ||
| | `auto_update_workflow.go` | `GenerateAutoUpdateWorkflow` | `func GenerateAutoUpdateWorkflow(opts GenerateAutoUpdateWorkflowOptions) error` | GenerateAutoUpdateWorkflow generates or removes the agentic-auto-upgrade. | | ||
| | `awf_config.go` | `BuildAWFConfigJSON` | `func BuildAWFConfigJSON(config AWFCommandConfig) (string, error)` | BuildAWFConfigJSON generates a compact JSON config file for AWF from the provided command configuration. | | ||
| | `awf_config_build.go` | `BuildAWFConfigJSON` | `func BuildAWFConfigJSON(config AWFCommandConfig) (string, error)` | BuildAWFConfigJSON generates a compact JSON config file for AWF from the provided command configuration. | |
There was a problem hiding this comment.
Two small abstractions look like they can be collapsed back into the AWF builder without changing behavior. net: -28 lines possible.
Generated by ✂️ Ponytail Reviewer for #55496 · codex · mai10 · 13 AIC · ⌖ 2.06 AIC · ⊞ 18.3K
Comment /ponytail to run again
| // resolveModelPolicyForAWFConfig applies policy precedence independently per list: | ||
| // allowed rules are narrowed using intersection with env policy, while blocked | ||
| // rules are widened using union with env policy. | ||
| func resolveModelPolicyForAWFConfig(workflowData *WorkflowData) ([]string, []string) { |
There was a problem hiding this comment.
pkg/workflow/awf_config_policy.go:34-96: yagni: a new policy helper file plus three one-call helpers for a single builder path. Inline the policy merge logic in BuildAWFConfigJSON and drop the extra layer.
| return workflowData.SandboxConfig.Agent.Platform | ||
| } | ||
|
|
||
| // extractModelFallback returns an AWFModelFallbackConfig if the workflow has configured |
There was a problem hiding this comment.
pkg/workflow/awf_config_build.go:425-531: yagni: eight one-call extractor helpers just to map one field into an AWF struct. Inline the few lines in BuildAWFConfigJSON and keep the builder local; the indirection adds churn without new behavior.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — requesting changes on one cross-file coupling issue introduced by the split.
📋 Key Themes & Highlights
Key Issue
- Implicit cross-file logger dependency:
awfConfigLogis declared inawf_config.go(the types file) but consumed by bothawf_config_build.goandawf_config_schema.go. This is the one seam that didn't get cut cleanly — both new files silently rely on a package-level variable from a sibling file, which is exactly what file splits should eliminate. The fix is to move the logger declaration intoawf_config_build.go(dominant user) and giveawf_config_schema.goits ownawfConfigSchemaLogwith a scoped namespace.
Positive Highlights
- ✅ Excellent split rationale: types / schema / build / policy maps cleanly onto the four distinct responsibilities
- ✅ Every new file has a cross-reference header — easy to navigate
- ✅ PR description is thorough: pre/post line counts, file table, doc reference updates all present
- ✅ Skill and README updates are included in the same commit — nothing fell through
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 55.5 AIC · ⌖ 10 AIC · ⊞ 7.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/awf_config_build.go:791
[/codebase-design] awfConfigLog is declared in awf_config.go (the types file) but used here — a reader of this file cannot see where the logger comes from. The split aimed to make each file self-contained, but the logger still silently leaks across the seam.
<details>
<summary>💡 Suggested fix</summary>
Move awfConfigLog out of awf_config.go (types-only) and into awf_config_build.go, which owns the majority of log calls:
// awf_config_build.go
var awfConfigLog = logger.Ne…
</details>
<details><summary>pkg/workflow/awf_config_schema.go:27</summary>
**[/codebase-design]** `getCompiledAWFConfigSchema` uses `awfConfigLog` which is declared in a different file (`awf_config.go`). This creates a hidden cross-file dependency that breaks the "each file is self-contained" goal of the split.
<details>
<summary>💡 Suggested fix</summary>
Either move the logger declaration to this file (see the comment on `awf_config_build.go`), or introduce a local `awfConfigSchemaLog`:
```go
var awfConfigSchemaLog = logger.New("workflow:awf_config_schema")…
|
Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32774702076
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Follow-up pass done:
Build passes and the AWF config tests are green; |
|
🎉 This pull request is included in a new release. Release: |
pkg/workflow/awf_config.gohad grown to 1,090 lines mixing schema validation, config construction, and model-policy resolution — the repo's 2nd-largest non-test Go file and under active churn. This splits it along those seams, matching the "one file per functionality" convention inpkg/workflow.Pure code move: no logic changes. Verified by diffing sorted file contents before/after — the only deltas are the new per-file
package/import/header blocks.File layout
awf_config.goAWFConfigFileand theAWF*Configtypesawf_config_schema.go//go:embedschema, cached compiled schema,validateAWFConfigJSON,normalizeTemplatableModelFallbackEnabled,buildAWFConfigSchemaURLawf_config_build.goBuildAWFConfigJSON+ build/extract helpers (extractPlatformType,extractModelFallback,extractBoundedQueriesConfig,resolveAWFContainerAgentTimeoutMinutes, …)awf_config_policy.goresolveModelPolicyForAWFConfig,intersectModelPolicyRules,unionModelPolicyRules,splitDomainListDoc references
pkg/workflow/README.md:BuildAWFConfigJSONrow now points atawf_config_build.go. Type rows are unchanged since the types stayed put..github/skills/awf-release-integrator/SKILL.md: AWF integration file list extended with the three new files so release integration still covers all the mapping code.Each new file carries a short header cross-referencing its siblings, e.g.:
Note:
golint-customstill flagsBuildAWFConfigJSONfor length (339 lines), as it did before the move — decomposing that function is out of scope here and would stop this from being a mechanical, reviewable move.