Enable runner-group runs-on for safe-outputs.jobs (and runner alias)#53725
runs-on for safe-outputs.jobs (and runner alias)#53725Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
runs-on for safe-outputs.jobs (and runner alias)
|
@copilot add exhaustive test suite |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in |
|
✅ PR Code Quality Reviewer completed the code quality review. Reviewed PR #53725; no actionable review comments to post because the change is consistent and no correctness/performance/security issues were found in the diff.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation is clean and correct. Runner-group object form now works for safe-outputs.jobs runs-on (and the runner alias) by storing rawRunsOn before lossy conversion and using resolveSafeJobRunsOn to render it. Validation is consistently extended across both validation paths with good test coverage. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.9 AIC · ⌖ 8.78 AIC · ⊞ 5.7K
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two issues; one non-blocking documentation suggestion.
📋 Key Themes & Highlights
Key Themes
-
Error messages lack job-name context (
frontmatter_parsing.goL52–63):validateRunsOnValueerrors don't include the job name, so a malformedgroup: 42would surface as"runs-on.group has type int"with no pointer to which job failed. ThevalidateRunsOnfunction in the same file already uses qualified field names — this block should follow that pattern. -
Fallback branch in
resolveSafeJobRunsOnis undocumented (runs_on_snippet.goL125–130): when exactly does the fallback toformatSafeJobRunsOnfire? Adding a comment (or a targeted test) clarifies the intent for future readers. -
Missing validation test for both
runs-on+runnerset simultaneously (safe_jobs_test.go): precedence is tested at render time but not at validation time — confirm that setting both fields still validates therunneralias independently.
Positive Highlights
- ✅
rawRunsOnfield is a clean, minimal fix that preserves the original shape through the rendering pipeline without structural changes. - ✅
resolveSafeJobRunsOncorrectly layers raw-form priority over the legacyformatSafeJobRunsOnpath. - ✅ Comprehensive test matrix: all
runs-onvariants (string, array, group-only object, group+labels object, runner alias) are covered at both the unit and compile/lock-file level. - ✅ Schema alignment with
$ref: "#/$defs/github_actions_runs_on"is minimal and correct. - ✅ Documentation in
safe-outputs.mdandself-hosted-runners.mdis clear and includes a working example.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 46.3 AIC · ⌖ 10.4 AIC · ⊞ 7.8K
Comment /matt to run again
| } | ||
| if err := validateRunsOnValue(jobMap["runs-on"]); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] validateRunsOnValue errors in this new block lack job-name context — a malformed runs-on (e.g. group: 42) surfaces as "runs-on.group has type int" with no pointer to which job failed, making the error harder to act on.
💡 Suggested fix
Iterate with the job name in scope and wrap errors with the qualified field path:
for jobName, jobRaw := range jobsRaw {
jobMap, ok := jobRaw.(map[string]any)
if !ok {
continue
}
if err := validateRunsOnValue(jobMap["runs-on"]); err != nil {
return nil, fmt.Errorf("safe-outputs.jobs.%s.runs-on: %w", jobName, err)
}
if err := validateRunsOnValue(jobMap["runner"]); err != nil {
return nil, fmt.Errorf("safe-outputs.jobs.%s.runner: %w", jobName, err)
}
}This matches the error message style used in validateRunsOn (safe-outputs.jobs.<id>.runs-on).
@copilot please address this.
| func resolveSafeJobRunsOn(jobConfig *SafeJobConfig, defaultRunsOn string) string { | ||
| if snippet := renderRunsOnSnippet(jobConfig.rawRunsOn); snippet != "" { | ||
| return snippet | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] resolveSafeJobRunsOn falls back to formatSafeJobRunsOn when renderRunsOnSnippet returns "", but renderRunsOnSnippet also returns "" for non-empty string values (because isEmptyRunsOnValue returns false but the YAML path encodes them normally). That means a plain string rawRunsOn does get rendered by renderRunsOnSnippet — but it would be good to add a test that explicitly covers the fallback branch to prove it isn't reachable in practice (or document when it is).
💡 Suggested clarification
Add a comment to resolveSafeJobRunsOn that explains exactly when the fallback fires (i.e. only for nil/empty rawRunsOn when nothing was configured), so future readers don't need to trace the logic:
// resolveSafeJobRunsOn renders the runs-on YAML snippet for a safe job.
// It prefers rawRunsOn (set during parsing) to preserve the original shape,
// including object-form runner groups. The fallback is reached only when
// rawRunsOn is nil/empty (no runs-on or runner key was set).
func resolveSafeJobRunsOn(jobConfig *SafeJobConfig, defaultRunsOn string) string {@copilot please address this.
| }, | ||
| expected: "runs-on:\n group: runner-group", | ||
| }, | ||
| { |
There was a problem hiding this comment.
[/tdd] The test "runs-on takes precedence over runner alias" (line ~510) includes a runner with labels: ["windows"] — yet windows isn't a macOS label and would pass validation. The precedence rule is tested at the rendering level, but there's no test confirming that when both runs-on and runner are set in frontmatter, validation is only applied to runs-on (i.e. the runner value isn't double-counted or ignored). Adding a validation-layer test for this combination would close that gap.
💡 Suggested test case for runs_on_validation_test.go
{
name: "runs-on takes precedence; runner alias is also validated independently",
frontmatter: map[string]any{
"safe-outputs": map[string]any{
"jobs": map[string]any{
"notify": map[string]any{
"runs-on": "ubuntu-latest",
"runner": "macos-latest", // runner alias still validated
},
},
},
},
wantErr: true,
errorInMsg: "safe-outputs.jobs.notify.runner",
},@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Adds runner-group object support to custom safe-output jobs.
Changes:
- Aligns safe-job schema, parsing, rendering, and validation.
- Adds variant and compilation tests.
- Documents object-form
runs-onandrunner.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_jobs.go |
Preserves raw runner configuration. |
pkg/workflow/safe_jobs_test.go |
Tests supported runner forms. |
pkg/workflow/runs_on_validation.go |
Validates safe-job runners. |
pkg/workflow/runs_on_validation_test.go |
Tests validation and macOS guards. |
pkg/workflow/runs_on_snippet.go |
Renders raw safe-job runner shapes. |
pkg/workflow/frontmatter_parsing.go |
Validates safe-job runner values. |
pkg/parser/schemas/main_workflow_schema.json |
Allows shared runs-on forms. |
docs/src/content/docs/reference/self-hosted-runners.md |
Documents supported locations. |
docs/src/content/docs/reference/safe-outputs.md |
Adds safe-job runner-group example. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
| if runsOn, exists := jobMap["runs-on"]; exists { | ||
| runsOnFields = append(runsOnFields, runnerField{name: "safe-outputs.jobs." + jobName + ".runs-on", value: runsOn}) | ||
| } | ||
| if runner, exists := jobMap["runner"]; exists { | ||
| runsOnFields = append(runsOnFields, runnerField{name: "safe-outputs.jobs." + jobName + ".runner", value: runner}) | ||
| } |
There was a problem hiding this comment.
Ponytail review: three inline findings, mostly redundant state (rawRunsOn plus the older RunsOn/runsOnArray fields) and duplicated validation loops that could collapse into a shared helper.
net: -25 lines possible.
Generated by ✂️ Ponytail Reviewer for #53725 · auto · 99.5 AIC · ⌖ 6.92 AIC · ⊞ 7.3K
Comment /ponytail to run again
Comments that could not be inline-anchored
pkg/workflow/safe_jobs.go:362
yagni: rawRunsOn duplicates RunsOn+runsOnArray, now three fields storing the same source config. resolveSafeJobRunsOn only falls back to the old pair when the raw-snippet render is empty. Drop RunsOn/runsOnArray and formatSafeJobRunsOn, always render from rawRunsOn.
pkg/workflow/runs_on_snippet.go:125
yagni: two-tier fallback (renderRunsOnSnippet(rawRunsOn) then formatSafeJobRunsOn(RunsOn, runsOnArray, ...)) for one job's runner config. renderRunsOnSnippet already handles string/array/object/empty shapes; the second branch only exists to serve the redundant RunsOn/runsOnArray fields being removed above.
pkg/workflow/frontmatter_parsing.go:54
yagni: this jobs map iteration validating runs-on/runner per job duplicates the near-identical loop added in runs_on_validation.go (validateRunsOn, building runnerFields for the macOS check). Extract one shared "for each safe-job, get runs-on/runner values" helper used by both.
Documents the architectural decision to align safe-outputs.jobs.<id>.runs-on and runner with the full github_actions_runs_on schema shape.
|
@copilot safe-outputs.jobs..runner should be deprecated in favor of safe-outputs.jobs..runs-on . Add codemod to fix. |
Could you clarify the requested change for |
|
@copilot remove support for safe-outputs.jobs..runner . only safe-outputs.jobs..runs-on should be supported. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Removed the |
PR Triage
Enables runner-group
|
|
@copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
safe-outputs.jobs.<id>.runs-onaccepted only string/array forms, unlike the rest of the workflow surface, so runner-group-only fleets could not use custom safe-jobs. This change aligns schema, parsing, rendering, and validation so custom safe-jobs support the sameruns-onforms as top-level and safe-output jobs.Schema alignment
safe-outputs.jobs.<id>.runs-onandsafe-outputs.jobs.<id>.runnernow reference#/$defs/github_actions_runs_on(string, array, object withgroup/labels).Safe-job rendering fix
runs-onshape for safe-jobs, including object form.runs-onfrom emitted safe-job YAML.Validation coverage parity
runs-onvalue validation and macOS guard checks to:safe-outputs.jobs.<id>.runs-onsafe-outputs.jobs.<id>.runnerTargeted coverage and docs
safe-outputs: jobs: notify: runs-on: group: sj-group labels: [linux] inputs: msg: description: Message text steps: - run: echo "${{ inputs.msg }}"