Default runs-on for non-reusable custom jobs when omitted - #48397
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
runs-on for non-reusable custom jobs when omitted
There was a problem hiding this comment.
Pull request overview
Defaults non-reusable custom jobs to the workflow runner or ubuntu-latest when runs-on is omitted.
Changes:
- Adds custom-job runner fallback logic.
- Adds regression coverage for
restore-memoryjobs.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_custom_jobs.go |
Applies default runner configuration. |
pkg/workflow/compiler_custom_job_memory_test.go |
Tests omitted runs-on behavior. |
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: 1
- Review effort level: Medium
|
✅ 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 #48397 does not have the 'implementation' label and has 48 new lines of code in business logic directories (below the 100-line threshold). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting one change on test coverage.
📋 Key Themes & Highlights
Key Themes
- Partial branch coverage: the new test only exercises the
ubuntu-latestfallback; thedata.RunsOninheritance branch is untested. - Existing comment (Copilot, line 481):
data.RunsOnmay be multi-line, which the direct assignment does not indent correctly — that concern is already filed.
Positive Highlights
- ✅ Clean, targeted fix with a clear fallback chain
- ✅ Regression test added in the right file alongside similar memory-job tests
- ✅ PR description clearly explains root cause, compiler behavior change, and test coverage
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 27.7 AIC · ⌖ 4.63 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| func (c *Compiler) configureCustomJobSteps(job *Job, jobName string, configMap map[string]any, data *WorkflowData) error { | ||
| if job.RunsOn == "" { | ||
| job.RunsOn = data.RunsOn |
There was a problem hiding this comment.
[/tdd] data.RunsOn inheritance path is untested — only the ubuntu-latest fallback is exercised by the new test.
The new test omits a workflow-level runs-on:, so the job.RunsOn = data.RunsOn branch (line 481) is never taken. A regression there would go undetected.
💡 Suggested additional test case
Add a second test that sets a workflow-level runs-on: self-hosted in the frontmatter and asserts the compiled setup job inherits runs-on: self-hosted instead of the ubuntu-latest fallback. This covers both branches of the new logic.
@copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics (15 tests)
|
There was a problem hiding this comment.
Review: Default runs-on for non-reusable custom jobs
Overall: The fix is correct and targeted. One non-blocking reliability concern worth addressing:
Multi-line runs-on indentation (line 481):
data.RunsOn is a pre-rendered YAML snippet. When the workflow declares a complex runner (group/labels object), the snippet spans multiple lines. Inheriting it via job.RunsOn = data.RunsOn and emitting through Job.Write's fmt.Fprintf(b, " %s\n", job.RunsOn) mis-indents continuation lines (4 spaces instead of 6). No test exercises this path.
Consider using normalizeRunsOnSnippet or formatIndentedYAMLField (both used elsewhere) before the assignment.
Test gap: The new test only exercises the fallback path (ubuntu-latest). A second case with an explicit workflow-level runs-on label array would validate the inheritance path for non-trivial runner forms.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 41.8 AIC · ⌖ 5.3 AIC · ⊞ 5K
|
|
||
| func (c *Compiler) configureCustomJobSteps(job *Job, jobName string, configMap map[string]any, data *WorkflowData) error { | ||
| if job.RunsOn == "" { | ||
| job.RunsOn = data.RunsOn |
There was a problem hiding this comment.
The inherited data.RunsOn snippet can be a multi-line YAML block (e.g., a matrix runner or group/labels object). Assigning it directly to job.RunsOn and emitting it via fmt.Fprintf(b, " %s ", job.RunsOn) in Job.Write will mis-indent continuation lines—they will render at 4 spaces instead of the required 6.
Consider normalizing with the same helper used elsewhere (e.g., normalizeRunsOnSnippet or formatIndentedYAMLField) before assigning, or document that data.RunsOn is guaranteed to be a pre-indented snippet.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — fix is mostly correct but has a correctness bug and a gap in test coverage.
### Findings
High — broken YAML for multi-line runs-on inheritance (compiler_custom_jobs.go:481)
data.RunsOn is a raw multi-line YAML snippet. Every other site that feeds it into a Job struct calls c.indentYAMLLines(data.RunsOn, " ") first. This line assigns it bare, so array/object runner forms will produce invalid YAML with unindented continuation lines. The hardcoded "runs-on: ubuntu-latest" fallback is fine; only the data.RunsOn branch needs the indent call.
Medium — test misses the actual bug path (compiler_custom_job_memory_test.go:46)
The new test exercises the ubuntu-latest hardcoded fallback (no workflow-level runs-on present), not the data.RunsOn inheritance path. A test with a complex (array/object) workflow-level runner would catch the indentation regression.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 45.1 AIC · ⌖ 4.83 AIC · ⊞ 5.7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/compiler_custom_jobs.go:481
Missing indentation for multi-line runs-on fallback: assigning data.RunsOn directly will produce broken YAML when the workflow uses an array or object runs-on form.
<details>
<summary>💡 Details and fix</summary>
data.RunsOn stores a raw YAML snippet such as:
runs-on:
group: my-runners
labels: [ubuntu-latest]When job.RunsOn is written by jobs.go it is emitted as %s — a single 4-space prefix on the first line only. Multi-line continuation lines are not…
pkg/workflow/compiler_custom_job_memory_test.go:46
Test only covers ubuntu-latest fallback — the actual regression (multi-line runs-on) is untested: the new test verifies runs-on: ubuntu-latest is present, but the correctness bug is in the data.RunsOn branch, not the hardcoded fallback branch.
<details>
<summary>💡 Details</summary>
The test at line 46 asserts runs-on: ubuntu-latest — this hits the job.RunsOn = "runs-on: ubuntu-latest" fallback (line 483) which is fine. It does not exercise the job.RunsOn = data.RunsOn branc…
|
Great work, What looks good:
This PR is ready for review! 🚀
|
🤖 PR Triage
Rationale: Fixes compiler omitting
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Applied the fix in commit
|
🤖 PR Triage
Adds default
|
|
🎉 This pull request is included in a new release. Release: |
A workflow in
gh-aw-testfailed to compile because a custom job usingrestore-memory: trueomittedruns-on, and the generated.lock.ymlemitted that job without a runner. GitHub Actions rejects such jobs withRequired property is missing: runs-on.Root cause
jobs.<id>.runs-onwas absent.restore-memorypath made this easy to hit in deterministic setup jobs.Compiler behavior change
job.RunsOnis empty:data.RunsOnif presentruns-on: ubuntu-latestuses:) are unchanged.Regression coverage
restore-memorycustom jobs without explicitruns-onto ensure generated job YAML always includes a runner.With this change, the compiled
setupjob includes a validruns-onstanza inherited from workflow defaults.pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/30293408015
Run: https://github.com/github/gh-aw/actions/runs/30302221322
Run: https://github.com/github/gh-aw/actions/runs/30304405147
run: https://github.com/github/gh-aw/actions/runs/30308333914
Run: https://github.com/github/gh-aw/actions/runs/30311684823
Run: https://github.com/github/gh-aw/actions/runs/30316592253