Support array/object runner specs for runs-on-slim and safe-outputs runner fields#39654
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…s-on fields Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
runs-on-slim and safe-outputs runner fields
There was a problem hiding this comment.
Pull request overview
Aligns runs-on-slim, safe-outputs.runs-on, and safe-outputs.threat-detection.runs-on with top-level runs-on semantics by accepting string / label-array / {group, labels} forms across schema, parsing, validation, and rendering.
Changes:
- Expands schema + editor metadata so safe-outputs and threat-detection runner fields accept full GitHub Actions
runs-onshapes. - Adds shared normalization/serialization helpers and updates parsing/rendering paths to propagate non-string runner specs end-to-end.
- Extends macOS runner rejection and adds tests/docs examples for the new runner spec forms.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/threat_detection.go | Parses threat-detection runs-on as a snippet and normalizes job runner rendering. |
| pkg/workflow/threat_detection_test.go | Adds coverage for threat-detection runner array/object forms. |
| pkg/workflow/safe_outputs_runtime.go | Updates framework job runs-on selection to use normalized snippet values. |
| pkg/workflow/safe_outputs_runs_on_test.go | Adds compilation/runtime expectations for safe-outputs runner array/object forms and indentation behavior. |
| pkg/workflow/safe_outputs_import_test.go | Updates import assertions to reflect normalized runs-on snippet storage. |
| pkg/workflow/safe_outputs_config.go | Extracts safe-outputs runs-on using the shared snippet renderer. |
| pkg/workflow/runs_on_validation.go | Extends macOS runner rejection to runs-on-slim and safe-outputs runner fields; adds runs-on shape validation helpers. |
| pkg/workflow/runs_on_validation_test.go | Adds tests asserting macOS rejection across the newly-covered runner fields/forms. |
| pkg/workflow/runs_on_unmarshal.go | Adds JSON unmarshaling support for safe-outputs and threat-detection runner shapes into normalized snippets. |
| pkg/workflow/runs_on_snippet.go | Introduces shared helpers to render/normalize runs-on YAML snippets. |
| pkg/workflow/imports.go | Adjusts safe-outputs import merge logic to treat “empty” runner values consistently across shapes. |
| pkg/workflow/frontmatter_types_test.go | Adds tests ensuring ParseFrontmatterConfig handles safe-outputs runner forms. |
| pkg/workflow/frontmatter_parsing.go | Adds validation for safe-outputs and threat-detection runs-on shapes during typed frontmatter parsing. |
| pkg/workflow/central_slash_command_workflow.go | Ensures central slash-command runner resolution formats safe-outputs snippets consistently for inline YAML usage. |
| pkg/parser/schemas/main_workflow_schema.json | Updates schema definitions for safe-outputs and threat-detection runs-on to reference github_actions_runs_on. |
| pkg/parser/schema_location_test.go | Adds schema-location tests for safe-outputs/threat-detection runner array/object acceptance. |
| docs/src/content/docs/reference/self-hosted-runners.md | Updates examples to show array/object runner specs for safe-outputs/threat-detection and runs-on-slim. |
| docs/src/content/docs/reference/safe-outputs.md | Updates docs examples to include safe-outputs runner array support. |
| docs/src/content/docs/reference/frontmatter.md | Updates runner precedence/type language to include safe-outputs and threat-detection runner forms. |
| docs/src/content/docs/reference/frontmatter-full.md | Refreshes the full reference doc (includes runner form documentation and other regenerated sections). |
| docs/public/editor/autocomplete-data.json | Updates editor typing/autocomplete metadata for safe-outputs runs-on unions. |
| .github/workflows/pr-code-quality-reviewer.lock.yml | Updates generated workflow lock metadata after recompilation. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 22/22 changed files
- Comments generated: 5
| if data != nil && data.SafeOutputs != nil && data.SafeOutputs.RunsOn != "" { | ||
| safeOutputsRuntimeLog.Printf("Framework job runs-on from safe-outputs: %s", data.SafeOutputs.RunsOn) | ||
| return "runs-on: " + data.SafeOutputs.RunsOn | ||
| snippet := normalizeRunsOnSnippet(data.SafeOutputs.RunsOn) | ||
| safeOutputsRuntimeLog.Printf("Framework job runs-on from safe-outputs: %s", snippet) | ||
| return c.indentYAMLLines(snippet, " ") |
| runsOn := "runs-on: ubuntu-latest" | ||
| if data.SafeOutputs.ThreatDetection.RunsOn != "" { | ||
| runsOn = "runs-on: " + data.SafeOutputs.ThreatDetection.RunsOn | ||
| runsOn = normalizeRunsOnSnippet(data.SafeOutputs.ThreatDetection.RunsOn) | ||
| } |
| # Maximum allowed size for git patches in kilobytes (KB) for | ||
| # push-to-pull-request-branch only. Overrides safe-outputs max-patch-size for this | ||
| # output type. Defaults to 4096 KB (4 MB) when unset. | ||
| # output type. Defaults to 1024 KB (1 MB) when unset. | ||
| # (optional) | ||
| max-patch-size: 1 |
| # Maximum allowed size for git patches in kilobytes (KB) for create-pull-request | ||
| # only. Overrides safe-outputs max-patch-size for this output type. Defaults to | ||
| # 4096 KB (4 MB) when unset. | ||
| # 1024 KB (1 MB) when unset. | ||
| # (optional) | ||
| max-patch-size: 1 |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
|
❌ Test Quality Sentinel failed during test quality analysis. |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines across schema, parsing, and rendering) but does not have a linked Architecture Decision Record (ADR).
📄 Draft ADR — copy to docs/adr/39654-extend-runs-on-schema-to-safe-outputs-runner-fields.md# ADR-39654: Extend the shared runs-on schema and snippet normalization to safe-outputs runner fields
**Date**: 2026-06-16
**Status**: Draft
## Context
ADR-38965 brought `runs-on-slim` to parity with top-level `runs-on` by reusing the shared `#/$defs/github_actions_runs_on` schema and rendering a canonical `runs-on:` YAML snippet. However, three sibling runner-selection fields were left behind and, in practice, accepted only a plain string label: `runs-on-slim`, `safe-outputs.runs-on`, and `safe-outputs.threat-detection.runs-on`. These fields configure the same concept — which runner a generated job executes on — yet they could not express the array-of-labels or `{ group, labels }` runner-group forms that `runs-on` already supports. Self-hosted users could not target safe-outputs or threat-detection jobs consistently, and such values failed to compile. Downstream rendering also assumed string concatenation, which breaks on non-string runner specs.
## Decision
We will treat `runs-on-slim`, `safe-outputs.runs-on`, and `safe-outputs.threat-detection.runs-on` as the same kind of value as top-level `runs-on`: their schema references the shared `github_actions_runs_on` shapes (string, label array, or `{ group, labels }` object); values are parsed into a single canonical `runs-on:` YAML snippet via shared helpers (`renderRunsOnSnippet`, `normalizeRunsOnSnippet`); and framework job rendering, threat-detection job rendering, and central slash-command resolution all consume the normalized snippet uniformly. Existing precedence (`safe-outputs.runs-on` over `runs-on-slim`) is preserved, and macOS runner rejection is extended to all three fields with consistent error wording.
## Alternatives Considered
### Alternative 1: Keep the safe-outputs runner fields string-only and document the limitation
Leave them typed as strings and tell users they cannot mirror an array or runner-group value. Rejected because it permanently blocks legitimate self-hosted configs and entrenches an inconsistent model where fields configuring the identical concept accept different syntax.
### Alternative 2: Add per-field validation and rendering for the array/object forms
Duplicate validation and YAML-rendering logic separately for each safe-outputs runner field. Rejected because it duplicates non-trivial logic already maintained for `runs-on`/`runs-on-slim`, inviting drift — the same reasoning behind ADR-38965.
## Consequences
### Positive
- All four runner fields reach parity, accepting string, label-array, and `{ group, labels }` forms.
- A single normalization path feeds every consumer, so future runner-spec changes apply uniformly.
- Self-hosted setups can route safe-outputs and threat-detection jobs to their existing runners.
- macOS rejection is enforced uniformly with actionable errors.
### Negative
- The internal contract widens from `string` to a normalized snippet, requiring every consumer to be updated and re-tested.
- Snippet normalization adds indentation-sensitivity; correctness depends on the exact upstream rendering format.
### Neutral
- Tests, editor autocomplete metadata, and reference docs were expanded for the new shapes.
- This is an incremental extension of ADR-38965, not a new architectural direction; precedence is unchanged.📋 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. This decision builds directly on ADR-38965; recording the extension keeps the runner-spec design history coherent for future contributors. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be complete:
ADRs live in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — commenting (no blocking issues found, but several test gaps and documentation concerns worth addressing).
📋 Key Themes & Highlights
Key Themes
- Untested utility functions:
renderRunsOnSnippet,normalizeRunsOnSnippet, andisEmptyRunsOnValueare the shared foundation for all three runner fields; their edge-case behaviour (partial objects, idempotency, silent error paths) lacks dedicated unit tests. - Dual parse paths:
SafeOutputsConfig.RunsOnis set via bothextractSafeOutputsConfig(YAML-map path, compilation time) andUnmarshalJSON(JSON round-trip viaParseFrontmatterConfig). The relationship is undocumented and could diverge. - Doc regeneration side-effect:
frontmatter-full.mdregeneration pulled in several unrelated wording changes, including the removal of a security caveat onrolesexact-match semantics. - Incomplete test sub-table: The new
handles safe-outputs runs-on formstest block covers only string/array forsafe-outputs.runs-on; object form andthreat-detection.runs-on(any form) are missing.
Positive Highlights
- ✅ Clean extraction of helpers into dedicated
runs_on_snippet.goandruns_on_unmarshal.gofiles. - ✅ macOS validation now covers all four runner fields symmetrically; new test cases are clear and named well.
- ✅
isEmptyRunsOnValuecorrectly handles nil, empty array, and empty-string cases — good defensive coding. - ✅
imports.gocorrectly migrated fromresult.RunsOn == ""toisEmptyRunsOnValue(result.RunsOn), keeping import merge semantics consistent with the new snippet format. - ✅
normalizeRunsOnSnippetidempotency guard prevents double-prefixing in the rendering pipeline — smart safety net.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
Comments that could not be inline-anchored
pkg/workflow/runs_on_snippet.go:24
[/tdd] Silent YAML marshal failure could produce malformed workflow output with no diagnostic — add a log call here.
If yaml.Marshal ever fails for a valid Go value (e.g., a deeply nested map[string]any that triggers the marshaler), renderRunsOnSnippet returns "". The caller then sets SafeOutputsConfig.RunsOn = "", which causes formatFrameworkJobRunsOn to fall back to the default runner silently — not a syntax error, but the user-specified runner is silently ignored.
<details>…
pkg/workflow/runs_on_unmarshal.go:20
[/tdd] Two parallel paths write SafeOutputsConfig.RunsOn; the relationship should be documented to prevent divergence.
UnmarshalJSON here handles the JSON round-trip in ParseFrontmatterConfig (map → JSON → struct). A second path in safe_outputs_config.go:530 (extractSafeOutputsConfig) handles direct YAML-map parsing at compile time. Both call renderRunsOnSnippet, so they are currently equivalent — but there is nothing preventing a future change to one without the other.
<detai…
pkg/workflow/frontmatter_types_test.go:421
[/tdd] The new test sub-table covers only string and array forms for safe-outputs.runs-on; two gaps remain.
- Object form (
{group, labels}) forsafe-outputs.runs-onis not tested throughParseFrontmatterConfig, so theUnmarshalJSONobject branch is exercised only indirectly byTestSafeOutputsRunsOnConfiguration. safe-outputs.threat-detection.runs-onhas no tests here at all — the newfrontmatter_parsing.govalidation block for it (lines 41-44) is untested.
<det…
pkg/workflow/runs_on_validation.go:136
[/tdd] isEmptyRunsOnValue partial-object logic is not covered by the new tests — consider adding table entries for the tricky branches.
The branch at line 136 (!hasGroup && !hasLabels → return false) handles unknown keys like {"unknown": "value"}, and the line-139 check handles {"group": ""} (treated as empty). Neither case has a dedicated test, and the edge of {"group": "some-group"} without a "labels" key is also untested (it should be non-empty — only group is enough).
<det…
pkg/workflow/runs_on_snippet.go:36
[/tdd] normalizeRunsOnSnippet has an idempotency guard (HasPrefix(value, "runs-on:")) that is untested — easy to lose in a future refactor.
The function is called from safe_outputs_runtime.go and threat_detection.go with values that renderRunsOnSnippet has already normalized (so they start with "runs-on:"). If that guard is ever removed or the prefix changes, double-prefixed output like runs-on: runs-on: ubuntu-latest would silently land in the generated YAML.
<details>
<sum…
pkg/workflow/safe_outputs_runs_on_test.go:438
[/tdd] The 6-space indent on the object form here is not self-documenting — the existing runs-on-slim object test has the clarifying comment but this new case does not.
Line 449 of the same file explains: "Object continuation lines start at 2-space (DefaultMarshalOptions) so indentYAMLLines(" ") produces 2+4=6 spaces". The same reasoning applies here but is missing.
<details>
<summary>💡 Suggested change</summary>
{
// Object continuation lines start at 2-space YAML in…
</details>
<details><summary>pkg/workflow/frontmatter_parsing.go:37</summary>
**[/grill-with-docs]** The post-unmarshal validation reads the raw frontmatter map again rather than the already-deserialized `config.SafeOutputs.RunsOn` — worth a comment explaining why.
`config.SafeOutputs.RunsOn` is already a normalized `string` at this point (set by `UnmarshalJSON`), so `validateRunsOnValue(config.SafeOutputs.RunsOn)` would always pass (it only rejects non-string/array/object types). Re-reading the raw map is necessary to validate the *original* value before coercion.
Wit…
</details>
<details><summary>docs/src/content/docs/reference/frontmatter-full.md:960</summary>
**[/grill-with-docs]** The `roles` documentation drops the exact-match security caveat without explanation — this seems unrelated to the runner-spec changes.
The previous doc explicitly warned: _"⚠️ `roles: [write]` rejects actors with `admin` or `maintainer` roles because matching is exact, not a minimum threshold."_ The new text changes the framing to _"permission level"_ and removes that warning entirely. If the matching behavior is still exact-match, this is a meaningful security regressio…
</details>|
@copilot Run or-finisher skill |
Ran the PR finisher pass on this branch. Current HEAD is |
|
@copilot review and update changes using DRY/YAGNI/SOLID |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in I tightened the implementation with a single normalization path for runs-on snippets (DRY), kept only the needed behavior changes (YAGNI), and ensured responsibilities stay focused (SOLID):
Validation run: |
|
@copilot review all comments and summarize the remaining blockers, then rerun the stale smoke workflows to refresh CI status.
|
runs-on-slim,safe-outputs.runs-on, andsafe-outputs.threat-detection.runs-onwere inconsistent with the rest of the workflow surface by accepting only string labels in practice. This change aligns them with top-levelruns-onsemantics so string, label-array, and{group, labels}forms are handled consistently end-to-end.Schema + editor metadata parity
main_workflow_schema.jsonto allowgithub_actions_runs_onshapes for:runs-on-slimsafe-outputs.runs-onsafe-outputs.threat-detection.runs-ondocs/public/editor/autocomplete-data.jsontyping for safe-outputsruns-on.Parsing + normalization
runs-onvalues now parse as any supported shape and normalize to a canonicalruns-on:YAML snippet.safe-outputs.runs-onoverruns-on-slim).Rendering + downstream consumers
runs-on-slim.Validation hardening
runs-on-slimsafe-outputs.runs-onsafe-outputs.threat-detection.runs-onTests + docs updates
self-hosted-runners,frontmatter,frontmatter-full,safe-outputs) with array/object examples and type language.