Deduplicate runtime override cloning in applyRuntimeOverrides#43134
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. No ADR enforcement needed: PR #43134 does not have the 'implementation' label and has only 67 new lines of code in business logic directories (threshold is 100). |
There was a problem hiding this comment.
Pull request overview
Refactors applyRuntimeOverrides to deduplicate the “clone runtime + apply action-repo/action-version overrides” logic, and adds a focused regression test for the known-runtime override path to ensure overrides don’t mutate the global knownRuntimes.
Changes:
- Introduces
cloneRuntimeWithActionOverridesto centralize runtime cloning plus action overrides. - Reuses the helper for both “existing requirement” and “known runtime” override paths.
- Adds a test covering the known-runtime action override cloning behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/runtime_overrides.go | Adds a helper for cloning runtimes with action overrides and uses it in both override branches. |
| pkg/workflow/runtime_override_test.go | Adds a test asserting the known-runtime override path clones and applies action overrides without mutating the global runtime definition. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
| func cloneRuntimeWithActionOverrides(base *Runtime, actionRepo, actionVersion string) *Runtime { | ||
| customRuntime := &Runtime{ | ||
| ID: base.ID, | ||
| Name: base.Name, | ||
| ActionRepo: base.ActionRepo, | ||
| ActionVersion: base.ActionVersion, | ||
| VersionField: base.VersionField, | ||
| DefaultVersion: base.DefaultVersion, | ||
| Commands: base.Commands, | ||
| ExtraWithFields: base.ExtraWithFields, | ||
| } | ||
|
|
||
| if actionRepo != "" { | ||
| customRuntime.ActionRepo = actionRepo | ||
| } | ||
| if actionVersion != "" { | ||
| customRuntime.ActionVersion = actionVersion | ||
| } | ||
|
|
||
| return customRuntime | ||
| } |
| if knownNode.ActionRepo == "custom/setup-node" { | ||
| t.Fatal("expected known runtime to remain unchanged") | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (1 test)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. 0% implementation tests (threshold: 30%). The new test TestApplyRuntimeOverrides_KnownRuntimeActionOverrides is a high-value design test that enforces the clone-isolation invariant introduced by the refactoring. No violations found.
There was a problem hiding this comment.
Review: Deduplicate runtime override cloning in applyRuntimeOverrides
The refactor is clean and well-motivated. One issue requires attention before merge.
Blocking issue — incomplete field copy in cloneRuntimeWithActionOverrides
The Runtime struct has 9 fields; the new clone helper copies only 8. The ManifestFiles []string field is absent from the struct literal (line 17). This means any cloned runtime (created when action-repo or action-version is overridden) will have a nil ManifestFiles.
Today this is latent: ManifestFiles is only read via knownRuntimes directly (in buildBaseManifestFiles), not via req.Runtime.ManifestFiles. But it is a regression hidden by the current access pattern — adding one line files = append(files, req.Runtime.ManifestFiles...) anywhere would silently break runtime manifest detection for customised runtimes.
The simplest fix uses a value-copy:
func cloneRuntimeWithActionOverrides(base *Runtime, actionRepo, actionVersion string) *Runtime {
r := *base // copies all 9 fields
if actionRepo != "" {
r.ActionRepo = actionRepo
}
if actionVersion != "" {
r.ActionVersion = actionVersion
}
return &r
}This also keeps the helper correct automatically when new fields are added to Runtime.
Non-blocking observations
CommandsandExtraWithFieldsare slice/map fields that will be shared (shallow-copy) between the original and the clone. This is safe now because neither is mutated after construction, but worth a comment or a future deep-copy if mutation is ever added.- The new test
TestApplyRuntimeOverrides_KnownRuntimeActionOverridescorrectly validates the clone-vs-pointer check and that the global known runtime is unmodified. It does not cover the missing-field case; aManifestFilesassertion onnodeReq.Runtimewould catch the regression.
Verdict: REQUEST_CHANGES on the missing field; everything else is an improvement.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 46 AIC · ⌖ 6.26 AIC · ⊞ 4.8K
| VersionField: base.VersionField, | ||
| DefaultVersion: base.DefaultVersion, | ||
| Commands: base.Commands, | ||
| ExtraWithFields: base.ExtraWithFields, |
There was a problem hiding this comment.
Missing ManifestFiles field in clone — the Runtime struct has 9 fields but cloneRuntimeWithActionOverrides only copies 8. ManifestFiles is not included in the struct literal.
While today ManifestFiles is only read via the global knownRuntimes slice (not through req.Runtime.ManifestFiles), any future code that accesses req.Runtime.ManifestFiles on an action-overridden runtime will silently get nil. A shallow copy guards against this:
customRuntime := *base // shallow copy all fields
if actionRepo != "" {
customRuntime.ActionRepo = actionRepo
}
if actionVersion != "" {
customRuntime.ActionVersion = actionVersion
}
return &customRuntimeNote: Commands and ExtraWithFields are slice/map references shared between the clone and the original — currently safe since neither is mutated, but worth documenting or deep-copying if that changes.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two correctness issues and one test-coverage gap.
📋 Key Themes & Highlights
Issues Found
- Missing field in clone —
ManifestFilesis not copied incloneRuntimeWithActionOverrides, making cloned runtimes silently lossy (correctness bug). - Shallow copy of reference types —
Commands(slice) andExtraWithFields(map) are copied by reference; mutations to the clone would corrupt the original runtime. - Incomplete test coverage — The new test only exercises the both-fields-overridden path; a single-field-only case (e.g. only
action-repo) would fully spec the||branch condition.
Positive Highlights
- ✅ Clean extraction of
cloneRuntimeWithActionOverrides— the deduplication is correct and well-motivated - ✅ New test verifies pointer inequality (clone isolation) and global-runtime immutability — exactly the right assertions
- ✅ PR description is clear and the code change is minimal and focused
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 51.1 AIC · ⌖ 6.5 AIC · ⊞ 6.6K
Comment /matt to run again
| VersionField: base.VersionField, | ||
| DefaultVersion: base.DefaultVersion, | ||
| Commands: base.Commands, | ||
| ExtraWithFields: base.ExtraWithFields, |
There was a problem hiding this comment.
[/codebase-design] ManifestFiles is missing from the clone — this field exists on Runtime but is not copied, so any cloned runtime silently loses its manifest-file detection hints.
💡 Fix
Add the missing field to the struct literal in cloneRuntimeWithActionOverrides:
ManifestFiles: base.ManifestFiles,Without this, a runtime cloned for action overrides will behave differently from the original when manifest-file detection is evaluated (e.g. triggering the Go runtime from go.mod).
@copilot please address this.
| ActionVersion: base.ActionVersion, | ||
| VersionField: base.VersionField, | ||
| DefaultVersion: base.DefaultVersion, | ||
| Commands: base.Commands, |
There was a problem hiding this comment.
[/codebase-design] Commands and ExtraWithFields are shallow-copied — both are reference types (slice and map). Mutations to the clone's slice or map would affect the original Runtime, undermining the mutation-safety goal of this helper.
💡 Safer deep copy
For the Commands slice:
Commands: slices.Clone(base.Commands),For the ExtraWithFields map:
ExtraWithFields: maps.Clone(base.ExtraWithFields),The current call sites only mutate ActionRepo and ActionVersion (scalars), so there is no active bug today — but the helper's contract implies full isolation. Callers that later mutate the map or slice would silently corrupt shared runtime state.
@copilot please address this.
| if nodeReq.Version != "22" { | ||
| t.Fatalf("expected version 22, got %s", nodeReq.Version) | ||
| } | ||
| if nodeReq.Runtime == knownNode { |
There was a problem hiding this comment.
[/tdd] The identity check nodeReq.Runtime == knownNode tests the pointer, which is good — but the test only covers the case where both action-repo and action-version are specified. The known-runtime path branches on actionRepo != "" || actionVersion != ""; a test for the single-field case (e.g., only action-repo, no action-version) would fully specify the behaviour of the OR condition.
💡 Suggested additional test
func TestApplyRuntimeOverrides_KnownRuntimeActionRepoOnly(t *testing.T) {
// ... look up knownNode as before ...
requirements := map[string]*RuntimeRequirement{}
applyRuntimeOverrides(map[string]any{
"node": map[string]any{
"action-repo": "custom/setup-node",
// no action-version
},
}, requirements)
nodeReq := requirements["node"]
if nodeReq.Runtime == knownNode {
t.Fatal("expected clone even when only action-repo is overridden")
}
if nodeReq.Runtime.ActionVersion != knownNode.ActionVersion {
t.Fatalf("expected ActionVersion to be inherited, got %s", nodeReq.Runtime.ActionVersion)
}
}@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — shallow-copy bugs undermine the clone's safety guarantee
The refactor's correctness hinges on cloneRuntimeWithActionOverrides producing a fully independent copy, but the new helper performs a shallow copy for two reference-type fields:
ExtraWithFields(map) — the map pointer is shared with the originalknownRuntimesentry. Any future write to the clone's map will mutate the global definition. Elixir's entry has a non-emptyExtraWithFields, so this is immediately exploitable.Commands(slice) — the slice header is copied but the backing array is shared. Direct index writes or in-capacity appends on the clone corrupt the original.
Both issues exist in the same function that was introduced specifically to centralize safe cloning — if anything, that makes them worse than the duplicate code they replaced.
Additionally, the new test covers only the known-runtime branch; the existing-requirement branch at line 95 goes untested.
Findings summary
| Severity | Issue |
|---|---|
| High | ExtraWithFields shallow copy shares map with global knownRuntimes |
| Medium | Commands shallow copy shares slice backing array |
| Medium | Missing test for existing-requirement branch (line 95 call site) |
| (Pre-existing, flagged elsewhere) | ManifestFiles field not copied at all |
🔎 Code quality review by PR Code Quality Reviewer · 82.9 AIC · ⌖ 5.42 AIC · ⊞ 5.4K
Comment /review to run again
| VersionField: base.VersionField, | ||
| DefaultVersion: base.DefaultVersion, | ||
| Commands: base.Commands, | ||
| ExtraWithFields: base.ExtraWithFields, |
There was a problem hiding this comment.
Shallow copy of ExtraWithFields defeats the purpose of cloning — any write to customRuntime.ExtraWithFields will mutate the shared global knownRuntimes entry.
💡 Suggested fix
The assignment ExtraWithFields: base.ExtraWithFields copies the map header (a pointer), not the map contents. Because knownRuntimes entries are package-level globals, a caller that later adds an entry to the cloned runtime's ExtraWithFields will silently corrupt the original global definition — exactly the side-effect the clone was introduced to prevent.
Elixir's global entry carries "otp-version": "27" in ExtraWithFields, so the risk is not theoretical.
Deep-copy the map:
var extraWithFields map[string]string
if len(base.ExtraWithFields) > 0 {
extraWithFields = make(map[string]string, len(base.ExtraWithFields))
for k, v := range base.ExtraWithFields {
extraWithFields[k] = v
}
}
customRuntime := &Runtime{
// ... other fields ...
ExtraWithFields: extraWithFields,
}| ActionVersion: base.ActionVersion, | ||
| VersionField: base.VersionField, | ||
| DefaultVersion: base.DefaultVersion, | ||
| Commands: base.Commands, |
There was a problem hiding this comment.
Shallow copy of Commands slice shares the backing array with the global runtime definition.
💡 Suggested fix
Commands: base.Commands copies the slice header (pointer, length, capacity) but both slices share the same underlying array. Any in-capacity append or direct index write (customRuntime.Commands[i] = "x") on the clone will silently corrupt the original knownRuntimes entry.
Use a defensive copy:
commands := append([]string(nil), base.Commands...)then assign Commands: commands in the struct literal. The same pattern applies to ManifestFiles once it is added to the clone.
|
|
||
| func TestDetectRuntimeRequirementsWithOverrides(t *testing.T) { | ||
| tests := []struct { | ||
| name string |
There was a problem hiding this comment.
New test only covers one of the two call sites of cloneRuntimeWithActionOverrides — the existing-requirement branch (line 95 in runtime_overrides.go) has zero coverage in this diff.
💡 Suggested fix
The test exercises the path where runtimeID is not yet in requirements (known-runtime branch, runtime_overrides.go:106). The other call site at line 95, where action overrides are applied to an already-existing RuntimeRequirement, is untested. A shallow-copy bug or nil-Runtime bug in that branch would be invisible.
Add a test that pre-populates requirements with a node entry (including a non-nil Runtime and a non-empty ExtraWithFields), then calls applyRuntimeOverrides with an action-repo override, and asserts:
- the returned
Runtimepointer is different from the pre-populated one (clone happened) - the overridden fields are applied
- the original
Runtimeis not mutated
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot please run the
Run: https://github.com/github/gh-aw/actions/runs/28645166299
|
🤖 PR Triage
Score Breakdown: Impact 28 + Urgency 14 + Quality 13 Rationale: Deduplicates runtime override cloning logic (67+/39−, 2 files). Has 6 comments indicating active review discussion. Clean scope targeting Recommended Action: Review together with #43135 as a runtime-refactor batch. Address open review comments first.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented a PR-finisher pass and pushed |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed remaining in-scope review feedback in |
|
🎉 This pull request is included in a new release. Release: |
applyRuntimeOverridesduplicated the sameRuntimeclone-and-override logic in both the existing-requirement and known-runtime paths. That made action override behavior easy to drift asRuntimeevolves.Refactor runtime cloning
cloneRuntimeWithActionOverrides(base, actionRepo, actionVersion)to centralizeRuntimecopying plusaction-repo/action-versionoverrides.applyRuntimeOverrides.Preserve override behavior
ActionRepoonlyActionVersiononlyAdd focused coverage