Support object-form runs-on in custom jobs schema#34007
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hey Unfortunately, this PR cannot be accepted because it violates the contribution process outlined in our CONTRIBUTING.md: 🚫 This project does not accept traditional pull requests from non-core team members. The CONTRIBUTING.md clearly states:
The current PR appears to be created by a bot agent ( What happens next?
This contribution model ensures consistency, quality, and dogfooding of the project's own agentic workflow tools. Thank you for understanding! If you'd like to contribute, please review the How Development Works section and create a detailed issue with your agentic plan. 🤖 Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
runs-on in custom jobs schema
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 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.
Code Quality Review: Schema DRY Refactoring + Type System Alignment
Verdict: Approve with one non-blocking suggestion
This PR successfully consolidates the duplicated runs-on schema definitions and aligns validation with runtime behavior. The DRY refactoring improves maintainability, and the typed parsing changes correctly preserve all three GitHub Actions runner forms.
✅ What's done well
- Clean schema consolidation: Moving the repeated
runs-ondefinition to$defs/github_actions_runs_oneliminates duplication and ensures consistent validation across top-level and job-level configuration - Type system correctness: Changing
RunsOnfromstringtoanycorrectly reflects the actual GitHub Actions schema (which accepts string, array, or object) - Comprehensive test coverage: The new tests cover all three forms (string, array, object) and verify round-trip serialization, which is exactly the right level of regression protection
- Consistent with existing patterns: The
anytype usage follows the same approach as other dynamic fields inFrontmatterConfig
💡 Non-blocking suggestion
I've left one inline comment about the nil-check pattern in frontmatter_serialization.go. The current != nil check works for the common case but has a subtle edge case with typed nil values. Consider adding explicit empty-string handling for robustness, but this is not a blocker.
🔎 Code quality review by PR Code Quality Reviewer · ● 431.9K
|
|
||
| // Execution settings | ||
| if fc.RunsOn != "" { | ||
| if fc.RunsOn != nil { |
There was a problem hiding this comment.
Inconsistent nil-check pattern: This change introduces if fc.RunsOn != nil for an any field, but the pattern is inconsistent with how other any fields are handled in this file.
💡 Suggested fix
The zero value for any is nil, but checking != nil for any types has a gotcha: a typed nil (e.g., (*string)(nil)) is not equal to nil in Go.
Options:
- Check the underlying value explicitly (safest):
if fc.RunsOn != nil {
// Add validation for empty string if RunsOn is string type
if s, ok := fc.RunsOn.(string); ok && s == "" {
// skip empty string
} else {
result["runs-on"] = fc.RunsOn
}
}- Use reflection for zero-value check (more robust but heavier):
if fc.RunsOn != nil && !reflect.ValueOf(fc.RunsOn).IsZero() {
result["runs-on"] = fc.RunsOn
}- Accept the current pattern but document the limitation in a comment:
// RunsOn is any, so nil check prevents zero-value emission
// Note: typed nil values (e.g., (*string)(nil)) will pass this check
if fc.RunsOn != nil {
result["runs-on"] = fc.RunsOn
}Why this matters: If RunsOn is ever set to a typed nil (e.g., through pointer field initialization), it will be serialized as runs-on: null in YAML, which may cause validation errors downstream.
Recommendation: Use option 1 (explicit string empty check) since the vast majority of runs-on values will be strings, and this provides the clearest intent.
|
Triage the failing build-wasm check and refresh the branch once the root cause is clear.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out and /tdd — requesting changes on test coverage and type safety.
📋 Key Themes & Recommendations
Key Themes
- Test coverage gaps — Only object form tested; array/string forms lack regression coverage
- Type safety erosion —
anyremoves compile-time guarantees without runtime validation - Round-trip stability — Serialization tests only validate one direction
- Scope drift — Lock file changes appear unrelated to schema work
Recommendations Priority
🔴 High priority (blocking):
- Add schema validation tests for array and string
runs-onforms - Add explanation for lock file changes or revert them
🟡 Medium priority (nice to have):
- Add runtime validation for
RunsOnfield to catch invalid types early - Strengthen round-trip tests with a second parse-serialize cycle
Positive Highlights
✅ Excellent schema deduplication — the $defs/github_actions_runs_on pattern is clean and maintainable
✅ Type change aligns with runtime behavior — pragmatic solution to support all GitHub Actions forms
✅ Clear PR description with concrete examples
✅ Good use of table-driven tests for the three runs-on forms
Overall: This is solid refactoring work that correctly unifies schema validation. The main concerns are test coverage completeness and the unexplained lock file changes. Once those are addressed, this will be ready to merge.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 787.4K
| } | ||
| } | ||
|
|
||
| func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_AcceptsJobRunsOnObjectForm(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] Missing test coverage for array form — only object form is tested.
💡 Suggested test expansion
The schema accepts three forms for runs-on (string, array, object), but this test only validates the object form at the job level. Add test cases for the other two forms to prevent regressions:
func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_AcceptsJobRunsOnArrayForm(t *testing.T) {
frontmatter := map[string]any{
"on": "workflow_dispatch",
"jobs": map[string]any{
"my-job": map[string]any{
"runs-on": []any{"self-hosted", "linux"},
"steps": []any{
map[string]any{"run": "echo hello"},
},
},
},
}
err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter, "/test/workflow.md")
assert.NoError(t, err, "Array form should be valid")
}Without these tests, a future schema change could break array or string forms without triggering test failures.
|
|
||
| // Workflow execution settings | ||
| RunsOn string `json:"runs-on,omitempty"` | ||
| RunsOn any `json:"runs-on,omitempty"` // Supports string, array, or object GitHub Actions runner forms |
There was a problem hiding this comment.
[/zoom-out] Type safety erosion — any loses compile-time guarantees.
💡 Consider a typed approach
Changing from string to any aligns with runtime behavior but removes all compile-time type safety. Invalid values like runs-on: 42 will now pass Go type checking and only fail at GitHub Actions runtime.
Options to preserve some type safety:
- Custom type with validation:
type RunsOnConfig struct {
value any
}
func (r *RunsOnConfig) UnmarshalJSON(data []byte) error {
var v any
if err := json.Unmarshal(data, &v); err != nil {
return err
}
// Validate v is string, []string, or map[string]any
switch v.(type) {
case string, []any, map[string]any:
r.value = v
return nil
default:
return fmt.Errorf("runs-on must be string, array, or object")
}
}- Runtime validation in
ParseFrontmatterConfig:
if runsOn, ok := frontmatter["runs-on"]; ok {
if !isValidRunsOn(runsOn) {
return nil, fmt.Errorf("invalid runs-on type")
}
config.RunsOn = runsOn
}Either approach catches invalid values before they reach GitHub Actions.
| } | ||
| }) | ||
|
|
||
| t.Run("handles top-level runs-on forms", func(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] Round-trip test only validates one direction — add a second cycle.
💡 Strengthen the round-trip assertion
The current test verifies parse → ToMap() but doesn't verify that the result can be re-parsed correctly. A bug in ToMap() could produce YAML that breaks when parsed a second time.
Add a full round-trip cycle:
t.Run("handles top-level runs-on forms", func(t *testing.T) {
// ... existing test code ...
// First round-trip (existing)
config, err := ParseFrontmatterConfig(frontmatter)
require.NoError(t, err)
tt.assertion(t, config.RunsOn)
reconstructed := config.ToMap()
tt.assertion(t, reconstructed["runs-on"])
// Second round-trip (new) — ensures ToMap() produces valid input
config2, err := ParseFrontmatterConfig(reconstructed)
require.NoError(t, err, "ToMap output should be parseable")
tt.assertion(t, config2.RunsOn)
reconstructed2 := config2.ToMap()
assert.Equal(t, reconstructed, reconstructed2, "Round-trip should be stable")
})This catches serialization bugs that only appear in the second parse cycle.
| # - docker/build-push-action@bcafcacb16a39f128d818304e6c9c0c18556b85f # v7.1.0 | ||
| # - docker/login-action@4907a6ddec9925e35a0a9e82d7399ccc52663121 # v4.1.0 | ||
| # - docker/metadata-action@030e881283bb7a6894de51c315a6bfe6a94e05cf # v6.0.0 (source v6) | ||
| # - docker/metadata-action@80c7e94dd9b9319bd5eb7a0e0fe9291e23a2a2e9 # v6 |
There was a problem hiding this comment.
[/zoom-out] Unexplained lock file change — Docker action version unpinned.
🔍 Scope drift concern
This PR is about runs-on schema validation, but the lock files show docker/metadata-action changed from a pinned SHA (v6.0.0 → commit 030e881...) to an unpinned major version reference (v6 → commit 80c7e94...).
Issues:
- Scope drift: Makes the PR harder to review — reviewers must verify this unrelated change
- Supply chain risk: Major version tags can move (though the SHA is still pinned in practice)
- Missing context: No explanation in the PR description for why this changed
Questions:
- Is this an intentional policy change (unpinned major versions for Docker actions)?
- Was this caused by running
make recompileon a different branch? - Should this be reverted and filed separately?
If unintentional, revert the lock file changes. If intentional, add a note to the PR description explaining the action version policy change.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (130 new lines across 📄 Draft ADR committed:
📋 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 Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you — especially when they wonder, "why is 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in References:
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent test quality
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
|
@copilot review all comments and reviews |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review all reviews |
Reviewed the current reviews: the actionable items are already covered in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
|
jobs.*.runs-onaccepted string and array forms in practice but still rejected the standard GitHub Actions object form ({ group: ... }) at schema-validation time. This change aligns custom job validation with the already-supported top-levelruns-onbehavior and preserves the same shape through typed parsing.Schema
runs-ondefinitions with a sharedgithub_actions_runs_onschema.runs-onandjobs.*.runs-onat the same definition.group,labels)Typed frontmatter parsing
FrontmatterConfig.RunsOnfromstringtoany.runs-onto preserve object and array forms instead of failing JSON unmarshalling.runs-onvalues correctly.Regression coverage
runs-on.runs-onin string, array, and object forms.Example now accepted in both places:
pr-sous-chef: branch updated from https://github.com/github/gh-aw/actions/runs/26296266826