diff --git a/.changeset/major-safe-job-runs-on.md b/.changeset/major-safe-job-runs-on.md new file mode 100644 index 00000000000..880a8b8fad9 --- /dev/null +++ b/.changeset/major-safe-job-runs-on.md @@ -0,0 +1,7 @@ +--- +"gh-aw": major +--- + +Support runner-group objects in custom safe-job `runs-on` fields. + +The deprecated `safe-outputs.jobs..runner` alias has been removed. Run `gh aw fix` to migrate existing workflows to `safe-outputs.jobs..runs-on`. diff --git a/docs/adr/53977-support-runner-groups-custom-safe-jobs.md b/docs/adr/53977-support-runner-groups-custom-safe-jobs.md new file mode 100644 index 00000000000..5df36f2a6b6 --- /dev/null +++ b/docs/adr/53977-support-runner-groups-custom-safe-jobs.md @@ -0,0 +1,45 @@ +# ADR-53977: Extend Custom Safe-Job `runs-on` to Support Runner-Group Objects and Remove `runner` Alias + +**Date**: 2026-08-20 +**Status**: Draft +**Deciders**: pelikhan, copilot-swe-agent + +--- + +### Context + +Custom safe jobs (`safe-outputs.jobs.`) had their own limited `runs-on` parser that only accepted strings and label arrays. Runner-group object form (`{group: ..., labels: [...]}`) was not supported, leaving custom safe jobs unable to run on self-hosted runner groups. All other `runs-on` configuration surfaces in the framework (top-level `runs-on`, `safe-outputs.runs-on`, `safe-outputs.threat-detection.runs-on`) already accepted all three forms via the shared `extractCustomJobRunsOn` parser. The legacy `runner` key was a deprecated alias for `runs-on` that duplicated the configuration surface and required a separate code path. + +### Decision + +We will reuse the shared `extractCustomJobRunsOn` parser for custom safe jobs, making their `runs-on` field accept the same three forms (string, label array, runner-group object) as every other runner configuration surface. The deprecated `runner` alias will be removed as a breaking change (major version bump), and a `gh aw fix` codemod (`safe-job-runner-to-runs-on`) will be provided to automatically migrate existing workflows. + +### Alternatives Considered + +#### Alternative 1: Extend the Custom Safe-Job Parser In-Place + +Extend the existing `toRunsOnValue`/`isRunsOnArrayValue` helpers to also handle `map[string]any` (runner-group objects) without delegating to the shared parser. This avoids a code-sharing dependency, but duplicates validation logic (macOS label rejection, empty-object rejection, unknown key rejection) that is already tested and maintained in `extractCustomJobRunsOn`. Any future change to runner-group validation would need to be applied in two places. + +#### Alternative 2: Keep the `runner` Alias as a Deprecated No-Op + +Retain `runner` as a tolerated (but warned) alias rather than removing it outright, making the change non-breaking. This avoids the need for a migration codemod and a major version bump. However, it perpetuates two parallel configuration keys for the same concept and increases schema surface area indefinitely. Given that `gh aw fix` can automate the rename, the migration cost is low enough to justify a clean removal. + +### Consequences + +#### Positive +- Custom safe jobs now have full parity with all other runner configuration surfaces, enabling use of runner groups. +- Validation logic (macOS rejection, empty-object rejection, unknown-key rejection) is exercised from a single code path, reducing the risk of inconsistencies. +- The schema is simplified: one canonical key (`runs-on`) replaces two (`runs-on` and `runner`). +- The automated codemod minimizes user effort for migration. + +#### Negative +- This is a breaking change: workflows using `safe-outputs.jobs..runner` will fail validation until migrated. Users must run `gh aw fix` or manually rename the key. +- The major version bump signals a broader API break even though only one deprecated alias is removed, which may cause friction for teams managing dependency pins. + +#### Neutral +- The `SafeJobConfig.RunsOn` field type changes from `RunsOnValue` (a `[]string`) to `string` (the pre-serialized YAML snippet), aligning with how the shared compiler represents runner configuration internally. +- Two helper functions (`toRunsOnValue`, `isRunsOnArrayValue`) are deleted from `runs_on_snippet.go` as they are now unused. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/docs/src/content/docs/reference/safe-outputs.md b/docs/src/content/docs/reference/safe-outputs.md index 11c2deec077..b09466d33dc 100644 --- a/docs/src/content/docs/reference/safe-outputs.md +++ b/docs/src/content/docs/reference/safe-outputs.md @@ -1895,6 +1895,24 @@ safe-outputs: `safe-outputs.runs-on` overrides `runs-on-slim:` for safe-output jobs specifically. To override the runner for all framework jobs at once, use the top-level [`runs-on-slim:`](/gh-aw/reference/self-hosted-runners/#configuring-the-framework-job-runner) field instead. +Custom safe-jobs can select their own runner with `safe-outputs.jobs..runs-on`. This field supports runner labels, label arrays, and runner-group objects: + +```aw +--- +safe-outputs: + jobs: + notify: + runs-on: + group: safe-job-runners + labels: [linux] + inputs: + message: + description: Notification message + steps: + - run: echo "Notify" +--- +``` + ### Safe Outputs Job Concurrency (`concurrency-group:`) Control concurrency for the compiled `safe_outputs` job. When set, the job uses this group with `cancel-in-progress: false` (queuing semantics — in-progress runs are never cancelled). diff --git a/docs/src/content/docs/reference/self-hosted-runners.md b/docs/src/content/docs/reference/self-hosted-runners.md index 676183415b9..b22d5da6fd2 100644 --- a/docs/src/content/docs/reference/self-hosted-runners.md +++ b/docs/src/content/docs/reference/self-hosted-runners.md @@ -79,6 +79,8 @@ runs-on: --- ``` +The string, array, and object forms are supported by the top-level `runs-on`, `runs-on-slim`, `safe-outputs.runs-on`, `safe-outputs.threat-detection.runs-on`, and custom `safe-outputs.jobs..runs-on` fields. + ## Sharing configuration via imports `runs-on` must be set in each workflow — it is not merged from imports. Other settings like `network` and `tools` can be shared: diff --git a/pkg/cli/codemod_safe_job_runner.go b/pkg/cli/codemod_safe_job_runner.go new file mode 100644 index 00000000000..68a0613aa31 --- /dev/null +++ b/pkg/cli/codemod_safe_job_runner.go @@ -0,0 +1,147 @@ +package cli + +import ( + "strings" + + "github.com/github/gh-aw/pkg/logger" +) + +var safeJobRunnerCodemodLog = logger.New("cli:codemod_safe_job_runner") + +func getSafeJobRunnerCodemod() Codemod { + return Codemod{ + ID: "safe-job-runner-to-runs-on", + Name: "Rename safe-outputs.jobs runner to runs-on", + Description: "Renames deprecated safe-outputs.jobs..runner fields to runs-on.", + IntroducedIn: "1.5.0", + Apply: func(content string, _ map[string]any) (string, bool, error) { + newContent, applied, err := applyFrontmatterLineTransform(content, renameSafeJobRunnerKeys) + if applied { + safeJobRunnerCodemodLog.Print("Renamed safe-job runner fields to runs-on") + } + return newContent, applied, err + }, + } +} + +func renameSafeJobRunnerKeys(lines []string) ([]string, bool) { + result := append([]string(nil), lines...) + modified := false + + for i := range lines { + if !hasYAMLKey(strings.TrimSpace(lines[i]), "safe-outputs") { + continue + } + + safeOutputsIndent := len(getIndentation(lines[i])) + childIndent := -1 + for j := i + 1; j < len(lines); j++ { + trimmed := strings.TrimSpace(lines[j]) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + continue + } + + indent := len(getIndentation(lines[j])) + if indent <= safeOutputsIndent { + break + } + if childIndent == -1 { + childIndent = indent + } + if indent != childIndent || !hasYAMLKey(trimmed, "jobs") { + continue + } + + if renameSafeJobRunnerKeysInJobsBlock(result, lines, j) { + modified = true + } + break + } + } + + return result, modified +} + +func renameSafeJobRunnerKeysInJobsBlock(result, lines []string, jobsLine int) bool { + jobsIndent := len(getIndentation(lines[jobsLine])) + jobIndent := -1 + jobStarts := []int{} + blockEnd := jobsLine + 1 + + for i := jobsLine + 1; i < len(lines); i++ { + trimmed := strings.TrimSpace(lines[i]) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + blockEnd = i + 1 + continue + } + + indent := len(getIndentation(lines[i])) + if indent <= jobsIndent { + blockEnd = i + break + } + blockEnd = i + 1 + if jobIndent == -1 { + jobIndent = indent + } + if indent == jobIndent { + jobStarts = append(jobStarts, i) + } + } + + modified := false + for i, start := range jobStarts { + end := blockEnd + if i+1 < len(jobStarts) { + end = jobStarts[i+1] + } + if renameSafeJobRunnerKeyInJob(result, lines, start, end) { + modified = true + } + } + return modified +} + +func renameSafeJobRunnerKeyInJob(result, lines []string, start, end int) bool { + jobIndent := len(getIndentation(lines[start])) + fieldIndent := -1 + runnerLine := -1 + hasRunsOn := false + + for i := start + 1; i < end; i++ { + trimmed := strings.TrimSpace(lines[i]) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + continue + } + + indent := len(getIndentation(lines[i])) + if indent <= jobIndent { + break + } + if fieldIndent == -1 { + fieldIndent = indent + } + if indent != fieldIndent { + continue + } + if hasYAMLKey(trimmed, "runs-on") { + hasRunsOn = true + } + if hasYAMLKey(trimmed, "runner") { + runnerLine = i + } + } + + if runnerLine == -1 || hasRunsOn { + return false + } + replacement, replaced := findAndReplaceInLine(lines[runnerLine], "runner", "runs-on") + if replaced { + result[runnerLine] = replacement + } + return replaced +} + +func hasYAMLKey(line, key string) bool { + return strings.HasPrefix(line, key+":") +} diff --git a/pkg/cli/codemod_safe_job_runner_test.go b/pkg/cli/codemod_safe_job_runner_test.go new file mode 100644 index 00000000000..caf22502ce3 --- /dev/null +++ b/pkg/cli/codemod_safe_job_runner_test.go @@ -0,0 +1,137 @@ +//go:build !integration + +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSafeJobRunnerCodemod(t *testing.T) { + codemod := getSafeJobRunnerCodemod() + + t.Run("metadata", func(t *testing.T) { + assert.Equal(t, "safe-job-runner-to-runs-on", codemod.ID) + assert.Equal(t, "Rename safe-outputs.jobs runner to runs-on", codemod.Name) + assert.Equal(t, "Renames deprecated safe-outputs.jobs..runner fields to runs-on.", codemod.Description) + assert.Equal(t, "1.5.0", codemod.IntroducedIn) + require.NotNil(t, codemod.Apply) + }) + + tests := []struct { + name string + content string + want string + wantApplied bool + }{ + { + name: "renames scalar runner", + content: `--- +safe-outputs: + jobs: + notify: + runner: ubuntu-latest + steps: + - run: echo hi +---`, + want: `--- +safe-outputs: + jobs: + notify: + runs-on: ubuntu-latest + steps: + - run: echo hi +---`, + wantApplied: true, + }, + { + name: "preserves runner group block", + content: `--- +safe-outputs: + jobs: + notify: + runner: # runner group + group: larger-runners + labels: [linux] +---`, + want: `--- +safe-outputs: + jobs: + notify: + runs-on: # runner group + group: larger-runners + labels: [linux] +---`, + wantApplied: true, + }, + { + name: "matches keys with trailing comments", + content: `--- +safe-outputs: # security settings + jobs: # custom output jobs + notify: + runner: ubuntu-latest # legacy field +---`, + want: `--- +safe-outputs: # security settings + jobs: # custom output jobs + notify: + runs-on: ubuntu-latest # legacy field +---`, + wantApplied: true, + }, + { + name: "skips job with canonical field", + content: `--- +safe-outputs: + jobs: + notify: + runner: old-runner + runs-on: ubuntu-latest +--- +`, + want: `--- +safe-outputs: + jobs: + notify: + runner: old-runner + runs-on: ubuntu-latest +--- +`, + wantApplied: false, + }, + { + name: "ignores runner outside safe jobs", + content: `--- +runner: top-level +safe-outputs: + create-issue: {} +jobs: + build: + runner: custom +--- +`, + want: `--- +runner: top-level +safe-outputs: + create-issue: {} +jobs: + build: + runner: custom +--- +`, + wantApplied: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, applied, err := codemod.Apply(tt.content, map[string]any{}) + require.NoError(t, err) + assert.Equal(t, tt.wantApplied, applied) + assert.Equal(t, tt.want, result) + }) + } +} diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index b16c50a60b1..127ce53ed3d 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -91,6 +91,7 @@ func GetAllCodemods() []Codemod { getSafeOutputMergePRConstraintsCodemod(), // Rename deprecated merge-pull-request allowed-labels/allowed-branches getSafeOutputAddReviewerAllowlistsCodemod(), // Rename deprecated add-reviewer reviewers/team-reviewers getSafeOutputDispatchRepositoryKeyCodemod(), // Rename deprecated safe-outputs.dispatch_repository key + getSafeJobRunnerCodemod(), // Rename deprecated safe-outputs.jobs runner fields getSafeInputsToMCPScriptsCodemod(), // Rename safe-inputs to mcp-scripts getRateLimitToUserRateLimitCodemod(), // Rename rate-limit to user-rate-limit with max key migration getEffectiveTokensToAICreditsCodemod(), // Migrate obsolete effective-token budget keys to AI credits keys diff --git a/pkg/cli/fix_codemods_test.go b/pkg/cli/fix_codemods_test.go index 7c9dda1a545..43ef7735c82 100644 --- a/pkg/cli/fix_codemods_test.go +++ b/pkg/cli/fix_codemods_test.go @@ -99,6 +99,7 @@ func TestGetAllCodemods_ContainsExpectedCodemods(t *testing.T) { "safe-output-merge-pr-constraints", "safe-output-add-reviewer-allowlists", "safe-output-dispatch-repository-key", + "safe-job-runner-to-runs-on", "safe-inputs-to-mcp-scripts", "rate-limit-to-user-rate-limit", "effective-tokens-to-ai-credits", @@ -228,6 +229,7 @@ func expectedCodemodOrder() []string { "safe-output-merge-pr-constraints", "safe-output-add-reviewer-allowlists", "safe-output-dispatch-repository-key", + "safe-job-runner-to-runs-on", "safe-inputs-to-mcp-scripts", "rate-limit-to-user-rate-limit", "effective-tokens-to-ai-credits", diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 5dbce6371a6..1d25b4f4118 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -11041,16 +11041,14 @@ "description": "Description of the safe-job (used in MCP tool registration)" }, "runs-on": { - "description": "Runner specification for this job", - "oneOf": [ - { - "type": "string" - }, + "$ref": "#/$defs/github_actions_runs_on", + "description": "Runner specification for this job. Supports string, array, or runner-group object forms. Defaults to 'ubuntu-latest'.", + "examples": [ + "ubuntu-latest", + ["self-hosted", "linux", "x64"], { - "type": "array", - "items": { - "type": "string" - } + "group": "larger-runners", + "labels": ["ubuntu-latest-8-cores"] } ] }, @@ -11156,20 +11154,6 @@ "$ref": "#/$defs/githubActionsStep" } }, - "runner": { - "description": "Runner specification for this job (alias for runs-on)", - "oneOf": [ - { - "type": "string" - }, - { - "type": "array", - "items": { - "type": "string" - } - } - ] - }, "agent-output": { "type": "string", "description": "Agent output field to use as input for this safe job (alias for output)" diff --git a/pkg/workflow/compiler_custom_job_properties.go b/pkg/workflow/compiler_custom_job_properties.go index 3bcdd1ca752..aedd7a2b57d 100644 --- a/pkg/workflow/compiler_custom_job_properties.go +++ b/pkg/workflow/compiler_custom_job_properties.go @@ -80,6 +80,9 @@ func (c *Compiler) extractCustomJobRunsOn(job *Job, jobName string, configMap ma if !hasRunsOn { return nil } + if err := validateRunsOnValue(runsOn); err != nil { + return fmt.Errorf("runs-on field for job '%s' is invalid: %w", jobName, err) + } if runsOnStr, ok := runsOn.(string); ok { job.RunsOn = "runs-on: " + runsOnStr return nil diff --git a/pkg/workflow/compiler_safe_outputs.go b/pkg/workflow/compiler_safe_outputs.go index d55a5495073..1cdd475104f 100644 --- a/pkg/workflow/compiler_safe_outputs.go +++ b/pkg/workflow/compiler_safe_outputs.go @@ -28,6 +28,9 @@ func (c *Compiler) mergeSafeJobsFromIncludedConfigs(topSafeJobs map[string]*Safe compilerSafeOutputsLog.Printf("Warning: skipping included safe-outputs config with invalid JSON: %v", err) continue } + if err := validateRunsOn(map[string]any{"safe-outputs": safeOutputsConfig}, c.markdownPath); err != nil { + return nil, err + } // Extract safe-jobs from the safe-outputs.jobs field includedSafeJobs := extractSafeJobsFromFrontmatter(map[string]any{ diff --git a/pkg/workflow/frontmatter_parsing.go b/pkg/workflow/frontmatter_parsing.go index 0cc0d3d279b..b59b8d66a62 100644 --- a/pkg/workflow/frontmatter_parsing.go +++ b/pkg/workflow/frontmatter_parsing.go @@ -51,6 +51,15 @@ func ParseFrontmatterConfig(frontmatter map[string]any) (*FrontmatterConfig, err return nil, err } } + if jobsRaw, ok := safeOutputsRaw["jobs"].(map[string]any); ok { + for _, jobRaw := range jobsRaw { + if job, ok := jobRaw.(map[string]any); ok { + if err := validateRunsOnValue(job["runs-on"]); err != nil { + return nil, err + } + } + } + } } // Parse typed Runtimes field if runtimes exist diff --git a/pkg/workflow/runs_on_snippet.go b/pkg/workflow/runs_on_snippet.go index 05c0509c8d3..9d43febb90a 100644 --- a/pkg/workflow/runs_on_snippet.go +++ b/pkg/workflow/runs_on_snippet.go @@ -37,45 +37,6 @@ func (r *RunsOnValue) UnmarshalJSON(data []byte) error { return nil } -// toRunsOnValue converts a YAML-decoded runs-on value (a string or a list of -// strings) into a RunsOnValue. Values with an unsupported shape, and non-string -// list entries, are ignored. -func toRunsOnValue(value any) RunsOnValue { - switch v := value.(type) { - case string: - return RunsOnValue{v} - case []any: - labels := make(RunsOnValue, 0, len(v)) - for _, item := range v { - if itemStr, ok := item.(string); ok { - labels = append(labels, itemStr) - } - } - if len(labels) == 0 { - return nil - } - return labels - case []string: - if len(v) == 0 { - return nil - } - return RunsOnValue(v) - default: - return nil - } -} - -// isRunsOnArrayValue reports whether a YAML-decoded runs-on value has array -// shape (as opposed to a single string label). -func isRunsOnArrayValue(value any) bool { - switch value.(type) { - case []any, []string: - return true - default: - return false - } -} - // FormatRunsOn serialises a RunsOnValue to a YAML-compatible string that can // be inlined directly after "runs-on: " in a generated workflow. // diff --git a/pkg/workflow/runs_on_validation.go b/pkg/workflow/runs_on_validation.go index 75323a41f14..2404d104ac9 100644 --- a/pkg/workflow/runs_on_validation.go +++ b/pkg/workflow/runs_on_validation.go @@ -24,6 +24,7 @@ package workflow import ( + "errors" "fmt" "strings" @@ -57,6 +58,16 @@ func validateRunsOn(frontmatter map[string]any, markdownPath string) error { if threatDetection, ok := safeOutputs["threat-detection"].(map[string]any); ok { runsOnFields = append(runsOnFields, runnerField{name: "safe-outputs.threat-detection.runs-on", value: threatDetection["runs-on"]}) } + if jobs, ok := safeOutputs["jobs"].(map[string]any); ok { + for jobName, jobValue := range jobs { + if job, ok := jobValue.(map[string]any); ok { + runsOnFields = append(runsOnFields, runnerField{ + name: fmt.Sprintf("safe-outputs.jobs.%s.runs-on", jobName), + value: job["runs-on"], + }) + } + } + } } for _, field := range runsOnFields { @@ -94,6 +105,9 @@ func validateRunsOnValue(value any) error { } return nil case map[string]any: + if len(v) == 0 { + return errors.New("runs-on object is empty. Expected an object with 'group' or 'labels'. Example: runs-on:\n group: my-runner-group") + } for key, value := range v { switch key { case "group": @@ -127,7 +141,16 @@ func isEmptyRunsOnValue(value any) bool { case string: return strings.TrimSpace(v) == "" case []any: - return len(v) == 0 + if len(v) == 0 { + return true + } + for _, label := range v { + label, ok := label.(string) + if !ok || strings.TrimSpace(label) != "" { + return false + } + } + return true case map[string]any: if len(v) == 0 { return true diff --git a/pkg/workflow/runs_on_validation_test.go b/pkg/workflow/runs_on_validation_test.go index 44276d2cff2..162d07a95d1 100644 --- a/pkg/workflow/runs_on_validation_test.go +++ b/pkg/workflow/runs_on_validation_test.go @@ -191,6 +191,24 @@ func TestValidateRunsOn(t *testing.T) { wantErr: false, description: "threat-detection runs-on with a Linux runner should be accepted", }, + { + name: "macos in custom safe-job runs-on labels", + frontmatter: map[string]any{ + "safe-outputs": map[string]any{ + "jobs": map[string]any{ + "notify": map[string]any{ + "runs-on": map[string]any{ + "group": "runner-group", + "labels": []any{"linux", "macos-latest"}, + }, + }, + }, + }, + }, + wantErr: true, + errorInMsg: "safe-outputs.jobs.notify.runs-on", + description: "custom safe-job runs-on labels containing macos should be rejected", + }, } for _, tt := range tests { @@ -299,6 +317,12 @@ func TestValidateRunsOnValue(t *testing.T) { wantErr: true, errContain: "runs-on object key 'runner' is not supported", }, + { + name: "empty object is invalid", + value: map[string]any{}, + wantErr: true, + errContain: "runs-on object is empty", + }, { name: "unsupported type is invalid", value: 123, diff --git a/pkg/workflow/safe_jobs.go b/pkg/workflow/safe_jobs.go index 29bb412e20a..f98d2b5aad5 100644 --- a/pkg/workflow/safe_jobs.go +++ b/pkg/workflow/safe_jobs.go @@ -18,7 +18,7 @@ type SafeJobConfig struct { // Standard GitHub Actions job properties Name string `yaml:"name,omitempty"` Description string `yaml:"description,omitempty"` - RunsOn RunsOnValue `yaml:"runs-on,omitempty"` + RunsOn string `yaml:"runs-on,omitempty"` If string `yaml:"if,omitempty"` Needs []string `yaml:"needs,omitempty"` Steps []any `yaml:"steps,omitempty"` @@ -31,7 +31,7 @@ type SafeJobConfig struct { GitHubToken string `yaml:"github-token,omitempty"` Output string `yaml:"output,omitempty"` Max int `yaml:"max,omitempty"` // Maximum number of times this output type may be emitted per run (default: 1) - runsOnArray bool `yaml:"-"` + runsOnError error `yaml:"-"` } // parseSafeJobsConfig parses safe-jobs configuration from a jobs map. @@ -67,13 +67,14 @@ func (c *Compiler) parseSafeJobsConfig(jobsMap map[string]any) map[string]*SafeJ } } - // Parse runs-on (also accept "runner" as alias) - if runsOn, exists := jobConfig["runs-on"]; exists { - safeJob.RunsOn = toRunsOnValue(runsOn) - safeJob.runsOnArray = isRunsOnArrayValue(runsOn) - } else if runner, exists := jobConfig["runner"]; exists { - safeJob.RunsOn = toRunsOnValue(runner) - safeJob.runsOnArray = isRunsOnArrayValue(runner) + // Parse runs-on using the shared custom-job parser. + runsOnJob := &Job{} + if err := c.extractCustomJobRunsOn(runsOnJob, jobName, jobConfig); err != nil { + safeJob.runsOnError = err + } else if isEmptySafeJobRunsOn(jobConfig["runs-on"]) { + safeJob.RunsOn = "" + } else { + safeJob.RunsOn = runsOnJob.RunsOn } // Parse if condition @@ -184,6 +185,13 @@ func (c *Compiler) parseSafeJobsConfig(jobsMap map[string]any) map[string]*SafeJ return result } +func isEmptySafeJobRunsOn(value any) bool { + if _, isObject := value.(map[string]any); isObject { + return false + } + return isEmptyRunsOnValue(value) +} + // buildSafeJobs creates custom safe-output jobs defined in SafeOutputs.Jobs func (c *Compiler) buildSafeJobs(data *WorkflowData, threatDetectionEnabled bool) ([]string, error) { if data.SafeOutputs == nil || len(data.SafeOutputs.Jobs) == 0 { @@ -236,10 +244,15 @@ func (c *Compiler) buildSafeJobs(data *WorkflowData, threatDetectionEnabled bool const defaultRunsOn = "ubuntu-latest" - // Set runs-on. Preserve list-shaped input from safe-outputs.jobs as a - // YAML array; formatSafeJobRunsOn centralizes the array-vs-scalar - // rendering decision shared with other runs-on parsers. - job.RunsOn = c.indentYAMLLines(formatSafeJobRunsOn(jobConfig.RunsOn, jobConfig.runsOnArray, defaultRunsOn), " ") + // Set runs-on, defaulting to ubuntu-latest when omitted. + if jobConfig.runsOnError != nil { + return nil, fmt.Errorf("invalid runs-on for safe-job '%s': %w", normalizedJobName, jobConfig.runsOnError) + } + runsOn := jobConfig.RunsOn + if runsOn == "" { + runsOn = "runs-on: " + defaultRunsOn + } + job.RunsOn = runsOn // Set if condition - combine safe output type check with user-provided condition // Custom safe jobs should only run if the agent output contains the job name (tool call) diff --git a/pkg/workflow/safe_jobs_syntax_test.go b/pkg/workflow/safe_jobs_syntax_test.go index e2e1cbdfe11..3e1c17efc4c 100644 --- a/pkg/workflow/safe_jobs_syntax_test.go +++ b/pkg/workflow/safe_jobs_syntax_test.go @@ -102,3 +102,35 @@ Test new syntax t.Error("Expected 'deploy' job to exist in SafeOutputs.Jobs") } } + +func TestSafeOutputsJobsRunnerAliasRejected(t *testing.T) { + c := NewCompiler() + tmpDir := testutil.TempDir(t, "test-*") + workflowPath := filepath.Join(tmpDir, "test-runner-alias.md") + content := `--- +on: issues +permissions: + contents: read +safe-outputs: + jobs: + deploy: + runner: ubuntu-latest + steps: + - run: echo test +--- + +# Test workflow +` + err := os.WriteFile(workflowPath, []byte(content), 0o644) + if err != nil { + t.Fatalf("Failed to create test file: %v", err) + } + + _, err = c.ParseWorkflowFile(workflowPath) + if err == nil { + t.Fatal("Expected runner alias to be rejected") + } + if !strings.Contains(err.Error(), "runner") { + t.Errorf("Expected error to mention 'runner', got: %v", err) + } +} diff --git a/pkg/workflow/safe_jobs_test.go b/pkg/workflow/safe_jobs_test.go index 07259978c1d..f42446c6894 100644 --- a/pkg/workflow/safe_jobs_test.go +++ b/pkg/workflow/safe_jobs_test.go @@ -70,7 +70,7 @@ func TestParseSafeJobsConfig(t *testing.T) { } // Test runs-on - if len(deployJob.RunsOn) != 1 || deployJob.RunsOn[0] != "ubuntu-latest" { + if deployJob.RunsOn != "runs-on: ubuntu-latest" { t.Errorf("Expected runs-on to be 'ubuntu-latest', got %v", deployJob.RunsOn) } @@ -148,6 +148,30 @@ func TestParseSafeJobsConfig(t *testing.T) { } } +func TestBuildSafeJobsDefaultsEmptyRunsOn(t *testing.T) { + for name, runsOn := range map[string]any{ + "empty scalar": "", + "empty array": []any{}, + "empty label": []any{""}, + } { + t.Run(name, func(t *testing.T) { + c := NewCompiler() + safeJobs := c.parseSafeJobsConfig(map[string]any{ + "notify": map[string]any{"runs-on": runsOn}, + }) + _, err := c.buildSafeJobs(&WorkflowData{ + Name: "test-workflow", + SafeOutputs: &SafeOutputsConfig{Jobs: safeJobs}, + }, false) + + require.NoError(t, err) + job, exists := c.jobManager.GetJob("notify") + require.True(t, exists) + require.Equal(t, "runs-on: ubuntu-latest", job.RunsOn) + }) + } +} + func TestParseSafeJobsConfigMax(t *testing.T) { c := NewCompiler() @@ -232,7 +256,7 @@ func TestBuildSafeJobs(t *testing.T) { SafeOutputs: &SafeOutputsConfig{ Jobs: map[string]*SafeJobConfig{ "deploy": { - RunsOn: RunsOnValue{"ubuntu-latest"}, + RunsOn: "runs-on: ubuntu-latest", If: "github.event.issue.number", Env: map[string]string{ "DEPLOY_ENV": "production", @@ -378,8 +402,7 @@ func TestParseAndBuildSafeJobsRunsOnList(t *testing.T) { }, }) - require.Equal(t, RunsOnValue{"self-hosted", "linux"}, safeJobs["deploy"].RunsOn) - require.True(t, safeJobs["deploy"].runsOnArray) + require.Equal(t, "runs-on:\n - self-hosted\n - linux", safeJobs["deploy"].RunsOn) workflowData := &WorkflowData{ Name: "test-workflow", @@ -402,6 +425,102 @@ func TestParseAndBuildSafeJobsRunsOnList(t *testing.T) { require.Equal(t, "runs-on:\n - self-hosted\n - linux", job.RunsOn) } +func TestParseAndBuildSafeJobsRunsOnObject(t *testing.T) { + tests := []struct { + name string + runsOn map[string]any + expected string + }{ + { + name: "group only", + runsOn: map[string]any{"group": "safe-job-runners"}, + expected: "runs-on:\n group: safe-job-runners", + }, + { + name: "group and labels", + runsOn: map[string]any{ + "group": "safe-job-runners", + "labels": []any{"linux", "x64"}, + }, + expected: "runs-on:\n group: safe-job-runners\n labels:\n - linux\n - x64", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := NewCompiler() + safeJobs := c.parseSafeJobsConfig(map[string]any{ + "deploy": map[string]any{ + "runs-on": tt.runsOn, + "steps": []any{map[string]any{"run": "echo 'Deploying'"}}, + }, + }) + + _, err := c.buildSafeJobs(&WorkflowData{ + Name: "test-workflow", + SafeOutputs: &SafeOutputsConfig{Jobs: safeJobs}, + }, false) + require.NoError(t, err) + + jobs := c.jobManager.GetAllJobs() + require.Len(t, jobs, 1) + for _, job := range jobs { + require.Equal(t, tt.expected, job.RunsOn) + } + }) + } +} + +func TestBuildSafeJobsRejectsEmptyRunsOnObject(t *testing.T) { + c := NewCompiler() + safeJobs := c.parseSafeJobsConfig(map[string]any{ + "deploy": map[string]any{ + "runs-on": map[string]any{}, + "steps": []any{map[string]any{"run": "echo 'Deploying'"}}, + }, + }) + + _, err := c.buildSafeJobs(&WorkflowData{ + Name: "test-workflow", + SafeOutputs: &SafeOutputsConfig{Jobs: safeJobs}, + }, false) + require.ErrorContains(t, err, "invalid runs-on for safe-job 'deploy'") + require.ErrorContains(t, err, "runs-on object is empty") +} + +func TestCompileSafeJobRunsOnObject(t *testing.T) { + tmpDir := testutil.TempDir(t, "safe-job-runs-on-object") + workflowPath := filepath.Join(tmpDir, "safe-job-runs-on-object.md") + content := `--- +on: workflow_dispatch +permissions: read-all +engine: copilot +safe-outputs: + jobs: + notify: + runs-on: + group: safe-job-runners + labels: [linux] + inputs: + message: + description: Message + steps: + - run: echo hi +--- + +# Test +` + require.NoError(t, os.WriteFile(workflowPath, []byte(content), 0o644)) + + compiler := NewCompiler() + require.NoError(t, compiler.CompileWorkflow(workflowPath)) + + compiled, err := os.ReadFile(filepath.Join(tmpDir, "safe-job-runs-on-object.lock.yml")) + require.NoError(t, err) + notifyJob := extractJobSection(string(compiled), "notify") + require.Contains(t, notifyJob, " runs-on:\n group: safe-job-runners\n labels:\n - linux") +} + func TestParseAndBuildSafeJobsSingleRunsOnList(t *testing.T) { c := NewCompiler() @@ -414,8 +533,7 @@ func TestParseAndBuildSafeJobsSingleRunsOnList(t *testing.T) { }, }) - require.Equal(t, RunsOnValue{"self-hosted"}, safeJobs["deploy"].RunsOn) - require.True(t, safeJobs["deploy"].runsOnArray) + require.Equal(t, "runs-on:\n - self-hosted", safeJobs["deploy"].RunsOn) workflowData := &WorkflowData{ Name: "test-workflow", @@ -474,7 +592,7 @@ func TestBuildSafeJobsWithoutCustomIfCondition(t *testing.T) { SafeOutputs: &SafeOutputsConfig{ Jobs: map[string]*SafeJobConfig{ "notify": { - RunsOn: RunsOnValue{"ubuntu-latest"}, + RunsOn: "runs-on: ubuntu-latest", // No custom 'if' condition Inputs: map[string]*InputDefinition{ "message": { @@ -525,7 +643,7 @@ func TestBuildSafeJobsWithDashesInName(t *testing.T) { SafeOutputs: &SafeOutputsConfig{ Jobs: map[string]*SafeJobConfig{ "send-notification": { - RunsOn: RunsOnValue{"ubuntu-latest"}, + RunsOn: "runs-on: ubuntu-latest", Steps: []any{ map[string]any{ "name": "Send notification", @@ -649,7 +767,7 @@ func TestExtractSafeJobsFromFrontmatter(t *testing.T) { t.Error("Expected 'deploy' job to exist") } - if len(deployJob.RunsOn) != 1 || deployJob.RunsOn[0] != "ubuntu-latest" { + if deployJob.RunsOn != "runs-on: ubuntu-latest" { t.Errorf("Expected runs-on to be 'ubuntu-latest', got '%v'", deployJob.RunsOn) } } @@ -657,13 +775,13 @@ func TestExtractSafeJobsFromFrontmatter(t *testing.T) { func TestMergeSafeJobs(t *testing.T) { base := map[string]*SafeJobConfig{ "deploy": { - RunsOn: RunsOnValue{"ubuntu-latest"}, + RunsOn: "runs-on: ubuntu-latest", }, } additional := map[string]*SafeJobConfig{ "test": { - RunsOn: RunsOnValue{"ubuntu-latest"}, + RunsOn: "runs-on: ubuntu-latest", }, } @@ -680,7 +798,7 @@ func TestMergeSafeJobs(t *testing.T) { // Test conflict detection conflicting := map[string]*SafeJobConfig{ "deploy": { - RunsOn: RunsOnValue{"windows-latest"}, + RunsOn: "runs-on: windows-latest", }, } @@ -702,7 +820,7 @@ func TestMergeSafeJobsFromIncludedConfigs(t *testing.T) { topSafeJobs := map[string]*SafeJobConfig{ "deploy": { Name: "Deploy Application", - RunsOn: RunsOnValue{"ubuntu-latest"}, + RunsOn: "runs-on: ubuntu-latest", }, } @@ -753,7 +871,7 @@ func TestMergeSafeJobsFromIncludedConfigs(t *testing.T) { t.Error("Expected 'test' job from includes to exist") } - if len(testJob.RunsOn) != 1 || testJob.RunsOn[0] != "ubuntu-latest" { + if testJob.RunsOn != "runs-on: ubuntu-latest" { t.Errorf("Expected test job runs-on to be 'ubuntu-latest', got '%v'", testJob.RunsOn) } @@ -798,7 +916,7 @@ func TestBuildSafeJobsEnvExpressionHoisting(t *testing.T) { SafeOutputs: &SafeOutputsConfig{ Jobs: map[string]*SafeJobConfig{ "publish": { - RunsOn: RunsOnValue{"ubuntu-latest"}, + RunsOn: "runs-on: ubuntu-latest", Env: map[string]string{ "GH_TOKEN": "${{ github.token }}", "STATIC_VAR": "literal-value", @@ -981,3 +1099,17 @@ func TestSafeJobsInputTypes(t *testing.T) { t.Errorf("Expected type 'environment', got %s", envInput.Type) } } + +func TestMergeSafeJobsFromIncludedConfigsRejectsMacOSRunner(t *testing.T) { + c := NewCompiler() + + _, err := c.mergeSafeJobsFromIncludedConfigs(nil, []string{`{ +"jobs": { +"notify": { +"runs-on": "macos-latest" +} +} +}`}) + + require.ErrorContains(t, err, "safe-outputs.jobs.notify.runs-on") +} diff --git a/pkg/workflow/safe_outputs_import_test.go b/pkg/workflow/safe_outputs_import_test.go index eba75476d9b..6f14d845953 100644 --- a/pkg/workflow/safe_outputs_import_test.go +++ b/pkg/workflow/safe_outputs_import_test.go @@ -1010,7 +1010,7 @@ This workflow imports safe-jobs from a shared workflow. // Verify job configuration job := workflowData.SafeOutputs.Jobs["my-custom-job"] assert.Equal(t, "My Custom Job", job.Name, "Job name should match") - assert.Equal(t, RunsOnValue{"ubuntu-latest"}, job.RunsOn, "Job runs-on should match") + assert.Equal(t, "runs-on: ubuntu-latest", job.RunsOn, "Job runs-on should match") assert.Len(t, job.Steps, 1, "Job should have 1 step") assert.Contains(t, job.Permissions, "contents", "Job should have contents permission") assert.Contains(t, job.Permissions, "issues", "Job should have issues permission") @@ -1397,7 +1397,7 @@ func TestMergeSafeOutputsJobsNotMerged(t *testing.T) { Jobs: map[string]*SafeJobConfig{ "existing-job": { Name: "Existing Job", - RunsOn: RunsOnValue{"ubuntu-latest"}, + RunsOn: "runs-on: ubuntu-latest", }, }, }