refactor: consolidate triplicate merge helpers and add sliceutil.SortedKeys#41388
Conversation
…edKeys Cluster 1 (High): collapse mergeSkipRoles/mergeSkipBots/mergeBots into a single mergeUniqueLogged(label, top, imported) helper in role_checks.go. The three public methods delegate to it, removing ~12 lines of duplicated logic. Cluster 2 (Medium): add SortedKeys[K cmp.Ordered, V any] to pkg/sliceutil, replace 13 inline slices.Sorted(maps.Keys(...)) sites across 5 files with sliceutil.SortedKeys(...). sortedMapKeys in map_helpers.go now delegates. Update sliceutil README and spec tests. Closes #41337 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR refactors workflow compilation utilities by consolidating duplicate merge-and-log helpers in pkg/workflow and centralizing repeated “sorted map keys” patterns into a new generic helper in pkg/sliceutil, improving consistency and reducing scattered one-liners.
Changes:
- Consolidated three byte-identical merge helpers in
role_checks.gointo a singlemergeUniqueLoggedhelper used by the existing methods. - Added
sliceutil.SortedKeysand updated multiple call sites to use it instead of inlineslices.Sorted(maps.Keys(...)). - Updated
sliceutildocumentation and spec tests to cover the new public helper.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/role_checks.go | Replaces three near-identical merge helpers with a shared logged merge helper while keeping existing method entrypoints. |
| pkg/workflow/map_helpers.go | Delegates sortedMapKeys to sliceutil.SortedKeys and cleans up imports. |
| pkg/workflow/domains.go | Uses sliceutil.SortedKeys to produce deterministic domain lists without local maps/slices imports. |
| pkg/workflow/compiler_yaml_step_conversion.go | Uses sliceutil.SortedKeys when rendering nested maps for stable YAML output. |
| pkg/workflow/compiler_main_job.go | Uses sliceutil.SortedKeys for deterministic iteration over job names. |
| pkg/cli/run_workflow_validation.go | Uses sliceutil.SortedKeys to present deterministic “valid inputs” in validation errors. |
| pkg/sliceutil/sliceutil.go | Introduces the generic SortedKeys helper for ordered map keys. |
| pkg/sliceutil/spec_test.go | Adds spec coverage validating SortedKeys behavior across key types and empty maps. |
| pkg/sliceutil/README.md | Documents SortedKeys in the public API list and examples, and clarifies sorting behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 0
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel analysis complete for PR #41388. Score: 80/100 APPROVE. Both add_comment and submit_pull_request_review were successfully applied in the prior run (limits 1/1 reached). No duplicate writes needed. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #41388 does not have the 'implementation' label and has only 70 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out, /improve-codebase-architecture, and /tdd — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Redundant wrapper:
sortedMapKeysinmap_helpers.gois now a passthrough tosliceutil.SortedKeys; either remove it or document why it's kept (see inline comment) - Unused receivers: The three wrapper methods in
role_checks.gostill carry(c *Compiler)receivers they don't use; the refactor makes this pre-existing smell more visible - Nil-map spec gap:
spec_test.gocovers empty maps but not nil maps; a one-liner test would complete the contract
Positive Highlights
- ✅ Excellent PR description: clearly splits the two clusters of duplication and explains the before/after
- ✅
SortedKeysis the right generic shape (K cmp.Ordered, V any) and placed in the correct package - ✅ Spec tests are descriptive and follow the existing naming convention
- ✅ README and doc comments updated in sync — nothing left stale
- ✅ Unused
maps/slicesimports cleaned up consistently across all 5 affected files - ✅ Log output is semantically unchanged — the label parameterisation is a safe mechanical refactor
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 54.1 AIC · ⌖ 9.84 AIC · ⊞ 6.5K
| t.Run("returns empty slice for empty map", func(t *testing.T) { | ||
| m := map[string]int{} | ||
| keys := SortedKeys(m) | ||
| assert.Empty(t, keys, "SortedKeys of empty map should return empty slice") |
There was a problem hiding this comment.
[/tdd] Missing nil-map test case — the spec covers empty maps but not nil maps.
Go's maps.Keys handles nil maps safely (produces an empty iterator), but an explicit test makes the contract clear and guards against implementation changes.
💡 Suggested test case
t.Run("returns empty slice for nil map", func(t *testing.T) {
var m map[string]int
keys := SortedKeys(m)
assert.Empty(t, keys, "SortedKeys of nil map should return empty slice")
})This rounds out the spec alongside the existing empty-map and typed-key cases.
| // Used to produce deterministic output when writing environment variables. | ||
| func sortedMapKeys(m map[string]string) []string { | ||
| return slices.Sorted(maps.Keys(m)) | ||
| return sliceutil.SortedKeys(m) |
There was a problem hiding this comment.
[/improve-codebase-architecture] sortedMapKeys is now a single-line passthrough to sliceutil.SortedKeys — consider removing it.
The wrapper adds an indirection layer within the workflow package without providing any domain semantics. Callers can call sliceutil.SortedKeys directly, which is already what every other changed file in this PR does.
💡 Suggested follow-up
Replace every sortedMapKeys(m) call site in the workflow package with sliceutil.SortedKeys(m), then delete sortedMapKeys and its header doc entry. This removes a now-unnecessary indirection layer and makes the pattern uniform across the codebase.
If the wrapper is intentionally kept for discoverability, add a comment explaining that, so future readers know it's not an accident.
| } | ||
|
|
||
| // mergeSkipRoles merges top-level skip-roles with imported skip-roles (union) | ||
| func (c *Compiler) mergeSkipRoles(topSkipRoles []string, importedSkipRoles []string) []string { |
There was a problem hiding this comment.
[/zoom-out] The three wrapper methods still carry (c *Compiler) receivers they don't use — now that mergeUniqueLogged is a package-level function, the wrappers could be too.
This is a pre-existing pattern that the refactor makes more visible: the receiver was already unused before this PR, but now the indirection is explicit. Consider completing the cleanup by making them package-level functions (or removing the wrappers entirely and calling mergeUniqueLogged at call sites with the label inline).
💡 Suggested change
// Before (unused receiver):
func (c *Compiler) mergeSkipRoles(top, imported []string) []string {
return mergeUniqueLogged("skip-roles", top, imported)
}
// After (package-level, consistent with mergeUniqueLogged):
func mergeSkipRoles(top, imported []string) []string {
return mergeUniqueLogged("skip-roles", top, imported)
}This is a non-breaking change for callers within the same package.
There was a problem hiding this comment.
The refactoring is correct and well-executed. Behavioral equivalence is maintained across all 8 call sites in domains.go, 2 in compiler_yaml_step_conversion.go, and 1 each in compiler_main_job.go and run_workflow_validation.go. Import cleanup is accurate — maps/slices removed exactly where no longer used, retained where still needed (slices.Contains in two files, slices.Collect+maps.Keys in run_workflow_validation.go).
One non-blocking observation on test coverage (see inline comment). No correctness, concurrency, or security issues found.
🔎 Code quality review by PR Code Quality Reviewer · 88.9 AIC · ⌖ 6.81 AIC · ⊞ 5.2K
| t.Run("returns empty slice for empty map", func(t *testing.T) { | ||
| m := map[string]int{} | ||
| keys := SortedKeys(m) | ||
| assert.Empty(t, keys, "SortedKeys of empty map should return empty slice") |
There was a problem hiding this comment.
Test does not pin nil-map return contract: assert.Empty passes for both nil and []string{}, so this sub-test cannot detect if a future refactor silently changes SortedKeys(nil) from returning nil to returning an allocated empty slice.
💡 Suggested fix
Add a nil-map sub-test that explicitly asserts the return value is nil (the current behavior of slices.Sorted on a zero-element iterator):
t.Run("returns nil for nil map", func(t *testing.T) {
var m map[string]int
keys := SortedKeys(m)
assert.Nil(t, keys, "SortedKeys of nil map should return nil, not an empty slice")
})This matters because callers that JSON-serialize the result get null vs [] — a meaningful difference if this helper is ever used in structured output or HTTP responses.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Go: 1 file ( Score breakdown: Behavioral coverage 40/40 · Edge-case coverage 20/30 · Duplication 20/20 · Proportional growth 0/10 (inflation flag mechanically triggered — see note).
Verdict
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 80/100. Test quality is Excellent — 0% of new tests are implementation tests (threshold: 30%). All 3 sub-tests verify observable behavioral contracts of SortedKeys. Build tag present, no mock libraries, all assertions carry descriptive messages.
|
@copilot run pr-finisher skill |
…s, drop unused receivers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 591c202. Three threads from
|
This comment has been minimized.
This comment has been minimized.
|
Great work on this refactor! 🎉 Collapsing three byte-identical merge helpers in The PR is well-scoped, the description clearly explains both clusters of changes with before/after code snippets, and the spec tests in
|
|
@copilot review all comments and address unresolved review feedback.
|
|
@copilot please resolve the open inline comments and rerun review.
|
Three byte-identical merge methods in
role_checks.goand 13 scatteredslices.Sorted(maps.Keys(...))one-liners across 5 files were reinventing logic that either already existed or should be centralized.Cluster 1 — Triplicate merge helpers (
role_checks.go)mergeSkipRoles,mergeSkipBots, andmergeBotswere identical except for their log label. Collapsed into a single package-level helper:The three methods now delegate to it. Log output is unchanged.
Cluster 2 —
sliceutil.SortedKeysgeneric helperAdded to
pkg/sliceutil:slices.Sorted(maps.Keys(...))sites acrossdomains.go,compiler_main_job.go,compiler_yaml_step_conversion.go,run_workflow_validation.gosortedMapKeysinmap_helpers.gonow delegates tosliceutil.SortedKeysmaps/slicesstdlib imports removed from affected filessliceutilREADME and spec tests updated