Refactor duplicated key sorting, engine env assembly, and engine max-* codemods#41674
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ 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. Changed files: pkg/cli/codemod_engine_max_runs.go, pkg/cli/codemod_engine_max_turns.go, pkg/cli/codemod_engine_to_top_level_helpers.go, pkg/workflow/claude_engine.go, pkg/workflow/codex_engine.go, pkg/workflow/copilot_engine_execution.go, pkg/workflow/engine_helpers.go — none are test files. |
There was a problem hiding this comment.
Pull request overview
This PR refactors recurring workflow compilation patterns by consolidating deterministic key sorting, engine environment assembly, and duplicated codemod logic for migrating deprecated engine.max-* frontmatter fields into shared helpers.
Changes:
- Replaced ad-hoc env key sorting in workflow step rendering with
sliceutil.SortedKeysfor deterministic output. - Centralized common engine env assembly (timeouts, max-turns resolution, engine+agent env merges, mcp-scripts secret passthrough) into
pkg/workflow/engine_helpers.goand reused it across Claude/Codex/Copilot. - Unified the
engine.max-runsandengine.max-turnscodemods behind a sharedmigrateEngineFieldToTopLevelhelper.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/engine_helpers.go | Adds shared env assembly helpers and switches env rendering to sliceutil.SortedKeys. |
| pkg/workflow/copilot_engine_execution.go | Replaces duplicated env setup with shared engine helper calls. |
| pkg/workflow/codex_engine.go | Replaces duplicated env setup with shared engine helper calls. |
| pkg/workflow/claude_engine.go | Replaces duplicated env setup with shared engine helper calls. |
| pkg/cli/codemod_engine_to_top_level_helpers.go | Introduces shared codemod helper for migrating engine.* fields to top-level. |
| pkg/cli/codemod_engine_max_turns.go | Refactors max-turns codemod to use the shared helper. |
| pkg/cli/codemod_engine_max_runs.go | Refactors max-runs codemod to use the shared helper. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 7
- Review effort level: Low
| // applyEngineMaxTurnsEnv sets GH_AW_MAX_TURNS from engine.max-turns or the default expression. | ||
| func applyEngineMaxTurnsEnv(env map[string]string, workflowData *WorkflowData) { | ||
| if workflowData.EngineConfig != nil && workflowData.EngineConfig.MaxTurns != "" { | ||
| env["GH_AW_MAX_TURNS"] = workflowData.EngineConfig.MaxTurns | ||
| return | ||
| } | ||
| env["GH_AW_MAX_TURNS"] = compilerenv.BuildDefaultMaxTurnsExpression() | ||
| } |
…duplication Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (190 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
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /improve-codebase-architecture, /tdd, and /zoom-out — no blocking issues, but a few actionable improvements worth considering.
📋 Key Themes & Highlights
Key Themes
- 9-parameter helper signature:
migrateEngineFieldToTopLevelaccepts three trailingstringmessage parameters that are positional and silently swappable. AmigrationMessagesstruct would make call sites self-documenting and resistant to silent bugs. - Nil log guard without nil callers: The
if log != nilguard inapplyEngineAndAgentEnvsignals a contract that no current caller honours — either remove it or document when nil is valid. - Test coverage gap on shared engine helpers: The four new
apply*functions inengine_helpers.goare untested directly; they're only covered through integration paths. Since they now own the env assembly contract for all three engines, direct unit tests would shrink the regression blast radius. - Copilot MCP ordering constraint is invisible: Claude/Codex call
applyEngineAndAgentEnv+applyMCPScriptsSecretEnvback-to-back; Copilot inserts the integration-ID injection in between. The ordering is correct but the why is not surfaced at theapplyMCPScriptsSecretEnvcall site.
Positive Highlights
- ✅ Clean, consistent extraction — the three engine implementations are now visually identical in their env-assembly section
- ✅
sliceutil.SortedKeysreplaces a local sort-and-iterate pattern, reducing noise and aligning with the existing utility layer - ✅
migrateEngineFieldToTopLevelcorrectly preserves all branching semantics from both original implementations (inline-map skip, top-level collision removal, positioned insertion) - ✅ Net −102 lines with no behaviour change — exactly the right shape for a refactor PR
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 78.2 AIC · ⌖ 7.27 AIC · ⊞ 6.5K
| "github.com/github/gh-aw/pkg/logger" | ||
| ) | ||
|
|
||
| func migrateEngineFieldToTopLevel( |
There was a problem hiding this comment.
[/improve-codebase-architecture] Nine positional parameters — three of which are string message literals at the end — makes the call site fragile: it's easy to swap skipInlineMessage, removedMessage, and migratedMessage without the compiler noticing.
💡 Suggestion: use a named-struct options type
Extract the three message strings into a migrationMessages struct so each call site reads as self-documenting named fields:
type migrationMessages struct {
SkipInline string
Removed string
Migrated string
}
func migrateEngineFieldToTopLevel(
content string,
frontmatter map[string]any,
engineField string,
targetTopLevelField string,
preserveTopLevelFields []string,
log *logger.Logger,
msgs migrationMessages,
) (string, bool, error)This is especially valuable because both codemods share this helper — a silently swapped message string would produce incorrect log output while the migration logic itself still passes tests.
| agentConfig := getAgentConfig(workflowData) | ||
| if agentConfig != nil && len(agentConfig.Env) > 0 { | ||
| maps.Copy(env, agentConfig.Env) | ||
| if log != nil { |
There was a problem hiding this comment.
[/improve-codebase-architecture] The if log != nil guard is added here, but no call site in this PR (or existing code) passes nil for log. This makes the function signature misleading — it implies nil is a valid and expected input when it isn't.
Consider removing the guard and requiring a non-nil logger from all callers. If there's a future use case for a no-op logger, logger.Discard() (or equivalent) is the idiomatic Go approach, not a nil check inside the function.
| } | ||
|
|
||
| // applyEngineAndAgentEnv merges custom environment variables from engine and agent configs. | ||
| func applyEngineAndAgentEnv(env map[string]string, workflowData *WorkflowData, log *logger.Logger) { |
There was a problem hiding this comment.
[/tdd] The four new shared helpers (applyOptionalEngineToolTimeouts, applyEngineMaxTurnsEnv, applyEngineAndAgentEnv, applyMCPScriptsSecretEnv) have no dedicated unit tests — they're only exercised indirectly through the engine integration paths.
💡 Why this matters
These functions are now the single point of definition for env assembly across Claude, Codex, and Copilot. A regression in any one of them breaks all three engines simultaneously, making the blast radius significantly larger than the original per-engine code.
Direct unit tests would:
- Document the expected
envstate after each call (e.g.,GH_AW_MAX_TURNSdefault vs. configured) - Catch regressions before they reach the integration layer
- Make the
nilworkflowData / empty config edge cases explicit
Existing engine_helpers_test.go already tests FormatStepWithCommandAndEnv at this level — the new helpers fit the same pattern.
| } | ||
| } | ||
| } | ||
| applyMCPScriptsSecretEnv(env, workflowData) |
There was a problem hiding this comment.
[/zoom-out] In Claude and Codex, applyEngineAndAgentEnv and applyMCPScriptsSecretEnv are called back-to-back. In Copilot, they're separated by the Copilot integration-ID injection (the comment on that block explains it must come last). This asymmetry is correct and intentional, but it's invisible to a future developer adding a fourth engine — they'd likely call the two helpers consecutively and accidentally allow user env to override the integration ID.
A brief comment near the applyMCPScriptsSecretEnv call here (e.g., // Must come after integration ID injection) would make the ordering requirement explicit and prevent the pattern from being silently violated.
|
@copilot run pr-reviewer skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added nil guards in commit
|
This issue called out three recurring patterns: ad-hoc key sorting instead of
sliceutil.SortedKeys, repeated engine env assembly logic across runtimes, and near-identical codemods for migratingengine.max-runs/engine.max-turns. This PR consolidates those paths into shared helpers while preserving current behavior and output shape.Workflow env rendering now uses shared sorted-key utility
sort.StringsinFormatStepWithCommandAndEnvwithsliceutil.SortedKeys.Engine env assembly deduplicated across Claude/Codex/Copilot
pkg/workflow/engine_helpers.gofor:pkg/workflow/claude_engine.gopkg/workflow/codex_engine.gopkg/workflow/copilot_engine_execution.goEngine max- codemods unified behind one migration helper*
migrateEngineFieldToTopLevelinpkg/cli/codemod_engine_to_top_level_helpers.go.codemod_engine_max_runs.gocodemod_engine_max_turns.go