Consolidate duplicate runs-on normalization helpers across config parsers - #52975
Conversation
…pet.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
There was a problem hiding this comment.
Pull request overview
Consolidates runs-on parsing and rendering helpers into a domain-focused module, though custom-job normalization remains separate.
Changes:
- Moves shared
RunsOnValuehelpers intoruns_on_snippet.go. - Extracts safe-job rendering into
formatSafeJobRunsOn. - Simplifies safe-job construction.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_jobs.go |
Uses the extracted formatter. |
pkg/workflow/runs_on_snippet.go |
Centralizes shared types and formatting helpers. |
pkg/workflow/repo_config.go |
Removes relocated helper implementations. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| func formatSafeJobRunsOn(runsOn RunsOnValue, runsOnArray bool, defaultRunsOn string) string { | ||
| // Keep []string{""} semantically unset, matching FormatRunsOn behavior. | ||
| if runsOnArray && len(runsOn) > 0 && (len(runsOn) != 1 || runsOn[0] != "") { | ||
| if snippet := renderRunsOnSnippet([]string(runsOn)); snippet != "" { | ||
| return snippet |
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
I did not find a blocking issue in the changed lines.
Highlights
This patch is a mechanical consolidation of existing runs-on parsing and rendering helpers into runs_on_snippet.go, plus a small call-site simplification in safe_jobs.go. I checked the moved logic against the deleted implementations and the behavior is preserved, including the array-vs-scalar handling and the []string{""} defaulting edge case. I also ran go test ./pkg/workflow/... and did not see regressions in the workflow package.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 5.95 AIC · ⌖ 5.33 AIC · ⊞ 6.9K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — approving with one suggestion.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Clean elimination of duplicated
runs-onlogic — fixes are now guaranteed to propagate uniformly - ✅
formatSafeJobRunsOncorrectly preserves the[]string{""}semantic-unset edge case - ✅
indentYAMLLineswrapping the new helper is safe: it's a no-op on single-line strings, so the scalar path is unaffected - ✅
RunsOnValue,toRunsOnValue,isRunsOnArrayValue, andFormatRunsOnall belong inruns_on_snippet.go— good placement alongside their natural collaborators
Suggestion
formatSafeJobRunsOnhas no dedicated unit test. The existingTestFormatRunsOnand the threat-detection integration tests provide some indirect coverage, but a direct table-driven test (likeTestFormatRunsOninrepo_config_test.go) would lock in the array/scalar/default boundary behaviour. See inline comment for a skeleton.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 36.2 AIC · ⌖ 8.6 AIC · ⊞ 7.7K
Comment /matt to run again
| // leading "runs-on:" key) for a safe-job, given its parsed RunsOnValue and | ||
| // whether the original configured value had array shape. This centralizes the | ||
| // array-vs-scalar rendering decision so callers don't need to special-case it. | ||
| func formatSafeJobRunsOn(runsOn RunsOnValue, runsOnArray bool, defaultRunsOn string) string { |
There was a problem hiding this comment.
[/tdd] formatSafeJobRunsOn has no dedicated unit test — the previous inline logic was exercised through integration tests, but the new helper isn't tested in isolation, which reduces confidence in future changes.
💡 Suggested test skeleton
func TestFormatSafeJobRunsOn(t *testing.T) {
def := "ubuntu-latest"
tests := []struct {
name string
runsOn RunsOnValue
runsOnArray bool
want string
}{
{"scalar single label", RunsOnValue{"custom"}, false, "runs-on: custom"},
{"array empty-string falls back to default", RunsOnValue{""}, true, "runs-on: ubuntu-latest"},
{"array multi-label yields YAML array", RunsOnValue{"self-hosted", "linux"}, true, "runs-on:\n - self-hosted\n - linux"},
{"nil runsOn falls back to default", nil, false, "runs-on: ubuntu-latest"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := formatSafeJobRunsOn(tt.runsOn, tt.runsOnArray, def)
assert.Equal(t, tt.want, got)
})
}
}This mirrors the existing TestFormatRunsOn pattern in repo_config_test.go.
@copilot please address this.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (117 new lines in 📄 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. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
This is a clean consolidation refactor. The helpers RunsOnValue, toRunsOnValue, isRunsOnArrayValue, and FormatRunsOn have been correctly moved from repo_config.go into the more appropriate runs_on_snippet.go. The new formatSafeJobRunsOn helper correctly centralises the array-vs-scalar rendering logic.
The one subtle change — indentYAMLLines is now unconditionally applied in safe_jobs.go — is safe because the scalar/fallback path from formatSafeJobRunsOn always produces a single-line string, making indentYAMLLines a no-op for that path.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 23.7 AIC · ⌖ 7.94 AIC · ⊞ 5.6K
|
@copilot Quick triage for maintainer-ready follow-up: Please refresh the branch if needed, address the remaining maintainer-facing follow-up, and run the Outstanding review items (newest first):
Failed checks from the compact candidate set:
Branch update was requested automatically for this run when GitHub allows it.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Added dedicated unit tests for |
|
🎉 This pull request is included in a new release. Release: |
repo_config.go(aw.json) andsafe_jobs.go(safe-outputs jobs) each carried their own independentruns-onparsing/rendering helpers, separate from the shared normalization logic already living inruns_on_snippet.go. This duplication meant runner-shape fixes could land in one parser and miss the others.Changes
runs-ontypes/helpers intoruns_on_snippet.go: relocatedRunsOnValue(+ itsUnmarshalJSON),toRunsOnValue,isRunsOnArrayValue, andFormatRunsOnout ofrepo_config.goso all runs-on normalization/rendering code lives alongside the existingrenderRunsOnSnippet/normalizeRunsOnSnippethelpers.formatSafeJobRunsOn, replacing the inline branching previously duplicated insafe_jobs.go's job-building loop.safe_jobs.go: it now calls the single shared helper instead of re-implementing defaulting and array/scalar rendering logic.This is a pure code-organization refactor — no change to supported
runs-onshapes or rendered output.Run context: https://github.com/github/gh-aw/actions/runs/31914206398> Generated by 👨🍳 PR Sous Chef · gpt54 · 9.56 AIC · ⌖ 6.97 AIC · ⊞ 8.7K · ◷