-
Notifications
You must be signed in to change notification settings - Fork 458
Refactor duplicated key sorting, engine env assembly, and engine max-* codemods #41674
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
e72041d
Initial plan
Copilot 0f9d0e9
refactor: deduplicate engine env and codemod migration logic
Copilot 108b01d
docs(adr): add draft ADR-41674 for engine env assembly and codemod de…
github-actions[bot] 7f511fb
fix: add nil guards to engine helper functions and codemod logger calls
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
45 changes: 45 additions & 0 deletions
45
docs/adr/41674-deduplicate-engine-env-assembly-and-codemod-migration-helpers.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # ADR-41674: Deduplicate Engine Env Assembly and Codemod Migration Helpers | ||
|
|
||
| **Date**: 2026-06-26 | ||
| **Status**: Draft | ||
| **Deciders**: Unknown (automated refactor by copilot-swe-agent) | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| The three agentic engine implementations (Claude, Codex, and Copilot) each contained near-identical ~20-line inline blocks responsible for setting tool-timeout env vars (`GH_AW_STARTUP_TIMEOUT`, `GH_AW_TOOL_TIMEOUT`), max-turns env resolution (`GH_AW_MAX_TURNS`), engine/agent env merges, and MCP-scripts secret passthrough. Similarly, two codemods for migrating deprecated `engine.max-runs` and `engine.max-turns` to top-level YAML keys shared ~80 lines of near-identical frontmatter transformation logic. Any change to env-assembly semantics — such as adding a new runtime env var — required the same edit in three separate engine files. The `FormatStepWithCommandAndEnv` function also used a bespoke ad-hoc sort instead of the existing `sliceutil.SortedKeys` utility. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will extract the repeated env-assembly logic into four shared helper functions in `pkg/workflow/engine_helpers.go` (`applyOptionalEngineToolTimeouts`, `applyEngineMaxTurnsEnv`, `applyEngineAndAgentEnv`, `applyMCPScriptsSecretEnv`) and introduce a single `migrateEngineFieldToTopLevel` helper in `pkg/cli/codemod_engine_to_top_level_helpers.go` that both codemods delegate to. We will also replace the ad-hoc key-sort in `FormatStepWithCommandAndEnv` with `sliceutil.SortedKeys`. This centralises shared behavior without changing runtime semantics or user-visible output. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Keep Inline Per-Engine Logic (Status Quo) | ||
|
|
||
| Each engine file retains its own copy of the env-assembly blocks. The approach is maximally self-contained: reading any single engine file gives the full picture of what env vars it sets. This was rejected because the duplication made it error-prone to keep the three engines in sync when semantics change, as demonstrated by the three identical blocks already being the dominant maintenance burden called out in the PR description. | ||
|
|
||
| #### Alternative 2: Interface-Based Polymorphism | ||
|
|
||
| Define an `EngineEnvApplier` interface with an `ApplyCommonEnv(env map[string]string, wd *WorkflowData)` method, then have each engine embed a common implementation. This is a valid Go pattern for shared behavior across types, but it adds interface indirection for what is essentially stateless, side-effecting helper logic with no need for substitutability. Package-level functions are simpler and sufficient here. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - Single source of truth for each env-assembly concern; future additions (e.g., a new `GH_AW_*` env var) require exactly one code change. | ||
| - Net reduction of ~160 lines of duplicated code across the three engine files and two codemod files. | ||
| - `FormatStepWithCommandAndEnv` uses the shared `sliceutil.SortedKeys` utility consistently with other callers in the codebase. | ||
| - `migrateEngineFieldToTopLevel` is parameterised and reusable for any future `engine.*` → top-level migration codemods. | ||
|
|
||
| #### Negative | ||
| - New engine implementations must know to call the shared helpers; this implicit contract is not enforced by the type system. | ||
| - The generic `migrateEngineFieldToTopLevel` signature (9 parameters) is harder to read in isolation than each of the original self-contained codemods, increasing cognitive overhead for understanding a single migration path. | ||
|
|
||
| #### Neutral | ||
| - The refactor is behavior-preserving: no change to env-var names, values, or precedence order; existing tests continue to exercise the same code paths via the new helper call sites. | ||
| - The `docs/adr/` directory convention is being adopted concurrently with this PR, so this ADR is the first to use the PR-number-as-ADR-number naming scheme. | ||
|
|
||
| --- | ||
|
|
||
| *ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| package cli | ||
|
|
||
| import ( | ||
| "strings" | ||
|
|
||
| "github.com/github/gh-aw/pkg/logger" | ||
| ) | ||
|
|
||
| func migrateEngineFieldToTopLevel( | ||
| content string, | ||
| frontmatter map[string]any, | ||
| engineField string, | ||
| targetTopLevelField string, | ||
| preserveTopLevelFields []string, | ||
| log *logger.Logger, | ||
| skipInlineMessage string, | ||
| removedMessage string, | ||
| migratedMessage string, | ||
| ) (string, bool, error) { | ||
| engineValue, hasEngine := frontmatter["engine"] | ||
| if !hasEngine { | ||
| return content, false, nil | ||
| } | ||
| engineMap, ok := engineValue.(map[string]any) | ||
| if !ok { | ||
| return content, false, nil | ||
| } | ||
| if _, hasEngineField := engineMap[engineField]; !hasEngineField { | ||
| return content, false, nil | ||
| } | ||
|
|
||
| hasPreservedTopLevelField := false | ||
| for _, field := range preserveTopLevelFields { | ||
| if _, exists := frontmatter[field]; exists { | ||
| hasPreservedTopLevelField = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| return applyFrontmatterLineTransform(content, func(lines []string) ([]string, bool) { | ||
| for _, line := range lines { | ||
| trimmed := strings.TrimSpace(line) | ||
| if !isTopLevelKey(line) || !strings.HasPrefix(trimmed, "engine:") { | ||
| continue | ||
| } | ||
| inlineValue := strings.TrimSpace(strings.TrimPrefix(trimmed, "engine:")) | ||
| if strings.HasPrefix(inlineValue, "{") && strings.Contains(inlineValue, engineField+":") { | ||
| if log != nil { | ||
| log.Print(skipInlineMessage) | ||
| } | ||
| return lines, false | ||
| } | ||
|
Copilot marked this conversation as resolved.
|
||
| } | ||
|
|
||
| fieldSuffix := "" | ||
| inEngineBlock := false | ||
| engineIndent := "" | ||
| engineFieldPrefix := engineField + ":" | ||
| for _, line := range lines { | ||
| trimmed := strings.TrimSpace(line) | ||
| if isTopLevelKey(line) && strings.HasPrefix(trimmed, "engine:") { | ||
| inEngineBlock = true | ||
| engineIndent = getIndentation(line) | ||
| continue | ||
| } | ||
| if inEngineBlock && len(trimmed) > 0 && !strings.HasPrefix(trimmed, "#") && len(getIndentation(line)) <= len(engineIndent) { | ||
| inEngineBlock = false | ||
| } | ||
| if inEngineBlock && strings.HasPrefix(trimmed, engineFieldPrefix) { | ||
| parts := strings.SplitN(line, ":", 2) | ||
| if len(parts) == 2 { | ||
| fieldSuffix = parts[1] | ||
| } | ||
| break | ||
| } | ||
| } | ||
|
|
||
| result, removed := removeFieldFromBlock(lines, engineField, "engine") | ||
| if !removed { | ||
| return lines, false | ||
| } | ||
|
|
||
| if hasPreservedTopLevelField { | ||
| if log != nil { | ||
| log.Print(removedMessage) | ||
| } | ||
| return result, true | ||
| } | ||
|
Copilot marked this conversation as resolved.
|
||
|
|
||
| insertAt := 0 | ||
| for i, line := range result { | ||
| if isTopLevelKey(line) && strings.HasPrefix(strings.TrimSpace(line), "engine:") { | ||
| insertAt = i | ||
| break | ||
| } | ||
| } | ||
|
|
||
| topLevelLine := targetTopLevelField + ":" + fieldSuffix | ||
| withTopLevel := make([]string, 0, len(result)+1) | ||
| withTopLevel = append(withTopLevel, result[:insertAt]...) | ||
| withTopLevel = append(withTopLevel, topLevelLine) | ||
| withTopLevel = append(withTopLevel, result[insertAt:]...) | ||
|
|
||
| if log != nil { | ||
| log.Print(migratedMessage) | ||
| } | ||
| return withTopLevel, true | ||
| }) | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/improve-codebase-architecture] Nine positional parameters — three of which are
stringmessage literals at the end — makes the call site fragile: it's easy to swapskipInlineMessage,removedMessage, andmigratedMessagewithout the compiler noticing.💡 Suggestion: use a named-struct options type
Extract the three message strings into a
migrationMessagesstruct so each call site reads as self-documenting named fields: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.