Refactor runtime setup extra-field merging into shared helpers#43135
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors runtime setup step generation by extracting shared helpers to consistently merge runtime default with: fields with user-provided overrides and to emit deterministic, sorted with: entries across the gh-aw, generic runtime, and Go go-version-file paths.
Changes:
- Added shared helpers to merge runtime/user
with:fields and append them in stable sorted order. - Updated gh-aw setup step generation to reuse the shared sorted
with:rendering helper. - Added tests covering merged
with:field output and ordering for generic runtimes, Gogo-version-file, and gh-aw.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/runtime_step_generator.go | Introduces shared merge/sorted-append helpers and wires them into all runtime setup generation paths. |
| pkg/workflow/runtime_setup_test.go | Adds tests for merged and deterministic ordering of runtime with: fields for generic runtimes and Go go-version-file. |
| pkg/workflow/runtime_gh_aw_test.go | Adds gh-aw-specific test coverage for merge + deterministic ordering. |
| pkg/workflow/gh_aw_setup_steps.go | Switches gh-aw setup step with: rendering to use the shared sorted-append helper. |
| .github/skills/agentic-workflow-designer/SKILL.md | Removes documented fields/notes in the workflow designer skill (appears unrelated to the runtime refactor). |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 6
- Review effort level: Low
| func mergeRuntimeWithFields(req *RuntimeRequirement) map[string]string { | ||
| allExtraFields := make(map[string]string) | ||
|
|
||
| maps.Copy(allExtraFields, req.Runtime.ExtraWithFields) | ||
|
|
||
| for k, v := range req.ExtraFields { | ||
| allExtraFields[k] = formatYAMLValue(v) | ||
| } | ||
|
|
||
| return allExtraFields | ||
| } |
| func appendSortedWithFieldEntries(step GitHubActionStep, withFields map[string]string) GitHubActionStep { | ||
| for _, key := range sliceutil.SortedKeys(withFields) { | ||
| step = append(step, fmt.Sprintf(" %s: %s", key, withFields[key])) | ||
| workflowLog.Printf(" Added extra field to runtime setup: %s = %s", key, withFields[key]) |
| After confirmation, generate one workflow file using the same skeleton style as `.github/aw/create-agentic-workflow.md`. | ||
|
|
||
| ```markdown | ||
| --- | ||
| emoji: <emoji> | ||
| description: <brief description> | ||
| strict: true | ||
| on: | ||
| <trigger config> | ||
| max-turns: <integer or omit> | ||
| max-ai-credits: <integer or omit for default 1000> | ||
| max-daily-ai-credits: <integer or omit to leave disabled> | ||
| user-rate-limit: <object or omit> | ||
| models: <object or omit> | ||
| permissions: |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (108 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
|
🤖 PR Triage
Score Breakdown: Impact 28 + Urgency 14 + Quality 13 Rationale: Extracts duplicated extra-field merging into shared helpers in runtime setup (108+/59−, 5 files). Good structure improvement but touches core runtime. Paired with #43134 for batch review. Recommended Action: Review together with #43134 as a runtime-refactor batch.
|
🧪 Test Quality Sentinel Report
📊 Metrics (3 tests)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Refactor runtime setup extra-field merging into shared helpers
The refactoring is clean and well-executed. The three duplication sites are correctly unified, precedence semantics (runtime defaults < user overrides) are preserved, and the new tests provide solid coverage across all three paths (gh-aw, generic runtime, Go go-version-file).
Highlights:
mergeRuntimeWithFieldscleanly centralizes the copy-then-override patternappendSortedWithFieldEntrieseliminates the repeated sorted-keys loop- Test coverage now locks in override behavior and deterministic ordering across all paths
Pre-existing note (not introduced by this PR): the uv early-return guard checks len(req.ExtraFields) == 0 but ignores runtime.ExtraWithFields. Since uv has no ExtraWithFields today this is safe, but a follow-up hardening would be len(req.ExtraFields) == 0 && len(runtime.ExtraWithFields) == 0.
No blocking issues. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 51.3 AIC · ⌖ 5.87 AIC · ⊞ 4.8K
There was a problem hiding this comment.
REQUEST_CHANGES — 2 new blocking issues + confirms existing findings
The refactor direction is sound — shared helpers eliminate real duplication. But there are correctness issues in both existing and new comments that need to be addressed before merging.
Blocking (new findings):
-
** versionless path ignores ** (line 156): The early-return guard only checks , completely ignoring . The runtime currently has no defaults, so this is a latent bug — but adding defaults to later will silently drop them with no test failure. Fix: use as the guard.
-
Dead unreachable branch (line 112): The block in the version-defaulting section is unreachable — returns at line 107. Future maintainers who add gh-aw version defaulting logic here will be surprised when it never runs.
Confirming existing critical finding
The existing review's top finding (duplicate key at line 146) is correct and should be treated as blocking. If contains , the generated YAML will have two conflicting entries — with the hardcoded value losing under last-value-wins semantics, inverting the advertised override precedence.
Test gaps (confirmed)
All three precedence tests (, , ) are missing assertions for the displaced runtime default values. A regression that emits duplicate keys would pass all three tests as written.
🔎 Code quality review by PR Code Quality Reviewer · 149.5 AIC · ⌖ 5.51 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/runtime_step_generator.go:156
uv early-return ignores Runtime.ExtraWithFields, silently dropping any runtime-defined defaults: The guard checks len(req.ExtraFields) == 0 but does not account for req.Runtime.ExtraWithFields; any default extra fields added to the uv runtime definition would be silently discarded on the versionless path.
<details>
<summary>💡 Suggested fix</summary>
Call mergeRuntimeWithFields first so the guard covers both field sources:
} else if runtime.ID == "uv" {
withFields :…
</details>
<details><summary>pkg/workflow/runtime_step_generator.go:112</summary>
**Dead unreachable code: the `gh-aw` branch at line 112 can never execute**: The `gh-aw` path is fully handled and returned at lines 90–107. The `if runtime.ID == "gh-aw"` check at line 112 is inside the fallthrough branch that is only reached for non-gh-aw runtimes, making it permanently dead. Any version-defaulting logic placed here silently has no effect, which will confuse future maintainers.
<details>
<summary>💡 Suggested fix</summary>
Remove the dead inner branch:
```go
// Use default…
</details>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /improve-codebase-architecture, and /tdd — requesting changes on a few structural issues and test gaps worth addressing before merge.
📋 Key Themes & Highlights
Key Themes
- Dead code not removed: The unreachable
runtime.ID == "gh-aw"branch (line 112) survived the refactor and should be deleted. - Logging side-effect in pure helper:
appendSortedWithFieldEntrieslogs internally; this was scoped to the generic path before but now fires across all three paths. - Helper signature over-couples to struct:
mergeRuntimeWithFieldsaccepts*RuntimeRequirementbut only reads two fields — a narrower signature improves testability. - Override tests incomplete: Tests assert the user value is present but don't assert the runtime default is absent; that negative assertion is what actually proves override semantics.
Positive Highlights
- ✅ Excellent motivation: three near-identical merge/sort blocks is exactly the right duplication to remove.
- ✅ Good test strategy: all three paths (gh-aw, generic runtime, go version-file) are covered.
- ✅ Clean helper naming:
mergeRuntimeWithFieldsandappendSortedWithFieldEntriesare immediately readable. - ✅ Net code reduction even including new tests — disciplined cleanup.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 85.7 AIC · ⌖ 5.87 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/runtime_step_generator.go:63
[/codebase-design] mergeRuntimeWithFields dereferences req.Runtime without a nil guard — if a caller passes a RuntimeRequirement with a nil Runtime, this silently panics.
<details>
<summary>💡 Suggested nil guard</summary>
func mergeRuntimeWithFields(req *RuntimeRequirement) map[string]string {
if req == nil || req.Runtime == nil {
return map[string]string{}
}
allExtraFields := make(map[string]string)
// ...
}Callers today always populate `Runti…
pkg/workflow/runtime_step_generator.go:76
[/codebase-design] The log call inside appendSortedWithFieldEntries couples a rendering helper to a side-effectful logger, making it harder to test in isolation and noisier when called from the gh-aw path (which uses a different log prefix).
<details>
<summary>💡 Suggested approach</summary>
Move logging into the callers that have context about which runtime is being configured, or accept a logger/context as parameter. The helper's job is purely to append formatted lines — logging is …
pkg/workflow/runtime_step_generator.go:112
[/codebase-design] Dead branch: the runtime.ID == "gh-aw" check at line 112 is unreachable — execution always returns early from the outer if runtime.ID == "gh-aw" block at lines 90–108. This vestigial guard survived from before the refactor.
<details>
<summary>💡 Safe to remove</summary>
// Lines 110-117 — the inner gh-aw check can be deleted:
if version == "" {
// runtime.ID == "gh-aw" branch is dead; only the else path is reachable here
version = runtime.DefaultVersio…
</details>
<details><summary>pkg/workflow/runtime_setup_test.go:518</summary>
**[/tdd]** The ordering assertions check against a hardcoded assumption that `cache-dependency-path` sorts before `package-manager-cache` alphabetically — but this is implicitly testing `sliceutil.SortedKeys` rather than the helper's contract. A test that directly calls `appendSortedWithFieldEntries` with a known map and asserts the resulting slice order would be a more precise unit test.
<details>
<summary>💡 Suggested unit test shape</summary>
```go
func TestAppendSortedWithFieldEntries_Ord…
</details>
<details><summary>pkg/workflow/runtime_setup_test.go:543</summary>
**[/tdd]** The Go version-file test doesn't assert that the merged fields do *not* include `go-version` (the version field used by the non-mod-file path). A user who also sets `ExtraFields: {"go-version": "1.22"}` might get unexpected double output. A negative assertion or a test for that boundary would tighten the spec.
<details>
<summary>💡 Additional assertion to consider</summary>
```go
// Confirm the version-file path does not accidentally add go-version: from req.Version
assert.NotConta…
</details>
<details><summary>pkg/workflow/runtime_gh_aw_test.go:173</summary>
**[/tdd]** The test asserts `alpha: 'user'` (with single quotes) because `formatYAMLValue` wraps string values in quotes, but the assertion for `beta: true` has no quotes (correct for booleans). It would strengthen the test to also assert that the *runtime* default `'runtime'` value for `alpha` is *not* present — confirming override semantics, not just presence.
<details>
<summary>💡 Suggested additional assertion</summary>
```go
assert.NotContains(t, content, "alpha: 'runtime'") // runtime d…
</details>
<details><summary>pkg/workflow/runtime_step_generator.go:60</summary>
**[/improve-codebase-architecture]** `mergeRuntimeWithFields` is tied to a struct (`*RuntimeRequirement`) but only uses two fields: `req.Runtime.ExtraWithFields` and `req.ExtraFields`. This tight coupling makes the function harder to reuse if a future caller has the fields but not the full struct.
<details>
<summary>💡 Consider a narrower signature</summary>
```go
func mergeRuntimeWithFields(runtimeDefaults map[string]string, userFields map[string]any) map[string]stringCallers become:
`…
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in commits 14b15a4 and 8e9f437. I fixed the blocking runtime setup issues (duplicate |
|
@copilot please run the
|
Checked the latest state on the updated branch ( |
generateSetupStepwas duplicating the same runtime/user extra-field merge and sortedwith:rendering logic across thegh-aw, Gogo-version-file, and generic runtime paths. That duplication made precedence, formatting, and ordering behavior easier to drift.What changed
pkg/workflow/runtime_step_generator.goto:Runtime.ExtraWithFieldswithRuntimeRequirement.ExtraFieldsreq.ExtraFieldsoverride runtime defaults)with:entries in stable sorted orderWhere the shared logic is now used
gh-awsetup step generationgo-version-fileis usedBehavior locked in with tests
with:fieldsgh-aw, generic runtime, and Go special-case pathsExample