refactor: SortedKeys sweep, deduplicate dedup logic, redistribute misplaced helpers#41829
Conversation
…dKeys sweep - Replace ~101 manual 'for k := range m + sort.Strings' patterns with sliceutil.SortedKeys across 59 files in pkg/workflow, pkg/cli, pkg/parser - Fix cache_integrity.go: use sliceutil.Deduplicate in canonicalReposScope for both []any and []string cases (was hand-rolling sorted adjacent-dedup) - Move isDescendant from codemod_github_app.go to yaml_frontmatter_utils.go (generic YAML-indent helper used by 12+ files) - Move isExpressionValue from codemod_effective_tokens_to_ai_credits.go to yaml_frontmatter_utils.go (generic expression detection) - Move getEngineAPIHosts from notify_comment.go to engine_api_targets.go (engine-to-API-host mapping belongs with other engine API target code) - Move buildSafeOutputJobsEnvVars and systemSafeOutputJobNames from notify_comment.go to safe_outputs_env.go (safe-output concerns) - Remove toEnvVarCase from notify_comment.go; use existing normalizeJobNameForEnvVar (a functional superset) throughout Net: 302 insertions, 725 deletions (-423 lines) Closes #41818 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. PR #41829 ('refactor: SortedKeys sweep, deduplicate dedup logic, redistribute misplaced helpers') contains only production Go source file changes across pkg/cli, pkg/parser, and pkg/workflow — no *_test.go, *.test.cjs, or *.test.js files were touched. |
There was a problem hiding this comment.
Pull request overview
This PR performs a broad refactor across gh-aw to standardize deterministic map-key ordering via sliceutil.SortedKeys, remove duplicated “collect-keys-then-sort” patterns, consolidate dedup logic in cache_integrity.go, and relocate several helpers out of notify_comment.go into more appropriate files.
Changes:
- Replaced repeated “collect keys → sort.Strings” blocks with
sliceutil.SortedKeys(...)across workflow/compiler/parser/CLI code paths to keep output deterministic and reduce duplicated idioms. - Simplified
canonicalReposScopeinpkg/workflow/cache_integrity.goby reusingsliceutil.Deduplicate+sort.Stringsinstead of hand-rolled adjacent-dedup loops. - Moved safe-output env-var construction and engine API host targeting helpers out of
notify_comment.gointo dedicated workflow files (safe_outputs_env.go,engine_api_targets.go), and centralized YAML-frontmatter helpers underpkg/cli/yaml_frontmatter_utils.go.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/yaml.go | Uses sliceutil.SortedKeys to deterministically order YAML map keys. |
| pkg/workflow/unified_prompt_step.go | Stabilizes multiple map-key iteration sites via sliceutil.SortedKeys. |
| pkg/workflow/tools_validation_github_toolsets.go | Uses sliceutil.SortedKeys for deterministic tool suggestion/error output. |
| pkg/workflow/service_ports.go | Uses sliceutil.SortedKeys for deterministic service ordering. |
| pkg/workflow/samples_validation.go | Uses sliceutil.SortedKeys for deterministic sample field processing. |
| pkg/workflow/samples_replay.go | Uses sliceutil.SortedKeys for deterministic sample entry collection. |
| pkg/workflow/safe_outputs_tools_generation.go | Uses sliceutil.SortedKeys for deterministic safe-output tool generation. |
| pkg/workflow/safe_outputs_max_validation.go | Uses sliceutil.SortedKeys for deterministic validation iteration and messages. |
| pkg/workflow/safe_outputs_env.go | Adds safe-output job mapping/env-var builder and related JSON marshaling. |
| pkg/workflow/safe_outputs_app_config.go | Uses sliceutil.SortedKeys for deterministic permission field rendering. |
| pkg/workflow/safe_outputs_actions.go | Uses sliceutil.SortedKeys for deterministic action input/env ordering. |
| pkg/workflow/runtime_step_generator.go | Uses sliceutil.SortedKeys for deterministic runtime step YAML fields. |
| pkg/workflow/runtime_detection.go | Uses sliceutil.SortedKeys for deterministic runtime detection output. |
| pkg/workflow/repo_memory_prompt.go | Uses sliceutil.SortedKeys for deterministic extension list output. |
| pkg/workflow/redact_secrets.go | Uses sliceutil.SortedKeys for deterministic secret/action reference ordering. |
| pkg/workflow/permissions_validation.go | Uses sliceutil.SortedKeys for deterministic toolset list rendering in messages. |
| pkg/workflow/observability_otlp.go | Uses sliceutil.SortedKeys for deterministic header/resource attribute output. |
| pkg/workflow/notify_comment.go | Removes unrelated helpers moved into dedicated workflow utility files. |
| pkg/workflow/model_alias_validation.go | Uses sliceutil.SortedKeys for deterministic alias graph traversal. |
| pkg/workflow/mcp_scripts_renderer.go | Uses sliceutil.SortedKeys for deterministic MCP tool/env iteration. |
| pkg/workflow/mcp_scripts_generator.go | Uses sliceutil.SortedKeys for deterministic MCP config/script generation. |
| pkg/workflow/mcp_config_validation.go | Uses sliceutil.SortedKeys for deterministic MCP validation error output. |
| pkg/workflow/mcp_config_custom.go | Uses sliceutil.SortedKeys for deterministic MCP config validation messaging. |
| pkg/workflow/jobs.go | Uses sliceutil.SortedKeys for deterministic env/outputs/with/secrets rendering. |
| pkg/workflow/github_toolset_validation_error.go | Uses sliceutil.SortedKeys for deterministic toolset error formatting. |
| pkg/workflow/gh_aw_setup_steps.go | Uses sliceutil.SortedKeys for deterministic setup-step with: field output. |
| pkg/workflow/engine_definition.go | Uses sliceutil.SortedKeys for deterministic engine catalog IDs output. |
| pkg/workflow/engine_api_targets.go | Hosts engine→API-host helper (getEngineAPIHosts) moved here for cohesion. |
| pkg/workflow/dependabot.go | Uses sliceutil.SortedKeys for deterministic dependency output generation. |
| pkg/workflow/compiler_yaml.go | Uses sliceutil.SortedKeys for deterministic header/prompt env var ordering. |
| pkg/workflow/compiler_safe_outputs_job.go | Uses sliceutil.SortedKeys for deterministic safe-output script ordering. |
| pkg/workflow/compiler_orchestrator_tools.go | Uses sliceutil.SortedKeys for deterministic included-file merging. |
| pkg/workflow/compiler_main_job.go | Uses sliceutil.SortedKeys for deterministic built-in job-name iteration. |
| pkg/workflow/compiler_jobs.go | Uses sliceutil.SortedKeys for deterministic job iteration ordering. |
| pkg/workflow/compiler_experiments.go | Uses sliceutil.SortedKeys for deterministic experiment ordering. |
| pkg/workflow/codex_engine.go | Uses sliceutil.SortedKeys for deterministic policy var ordering. |
| pkg/workflow/central_slash_command_workflow.go | Uses sliceutil.SortedKeys for deterministic command/label/trigger/type ordering. |
| pkg/workflow/call_workflow_secrets.go | Uses sliceutil.SortedKeys for deterministic workflow_call secret ordering. |
| pkg/workflow/cache_integrity.go | Dedup logic consolidated with sliceutil.Deduplicate and sorting. |
| pkg/workflow/agentic_engine.go | Uses sliceutil.SortedKeys for deterministic supported engine list output. |
| pkg/workflow/action_cache.go | Uses sliceutil.SortedKeys for deterministic JSON marshaling of cache keys. |
| pkg/parser/schema_suggestions.go | Uses sliceutil.SortedKeys for deterministic accepted-field suggestions. |
| pkg/parser/mcp.go | Uses sliceutil.SortedKeys for deterministic container env arg ordering. |
| pkg/parser/import_topological.go | Uses sliceutil.SortedKeys for deterministic dependency key iteration. |
| pkg/parser/import_cycle.go | Uses sliceutil.SortedKeys for deterministic cycle-start selection. |
| pkg/parser/frontmatter_hash.go | Uses sliceutil.SortedKeys for deterministic map hashing input. |
| pkg/cli/yaml_frontmatter_utils.go | Centralizes YAML-frontmatter helpers (isDescendant, isExpressionValue). |
| pkg/cli/update_container_pins.go | Uses sliceutil.SortedKeys for deterministic image ordering with empty-slice normalization. |
| pkg/cli/tool_graph.go | Uses sliceutil.SortedKeys for deterministic Mermaid graph generation. |
| pkg/cli/redacted_domains.go | Uses sliceutil.SortedKeys for deterministic domain list output. |
| pkg/cli/logs_usage_activity.go | Uses sliceutil.SortedKeys for deterministic allowed/blocked domain lists. |
| pkg/cli/log_aggregation.go | Uses sliceutil.SortedKeys for deterministic domain aggregation output. |
| pkg/cli/jsonworkflow_to_markdown.go | Uses sliceutil.SortedKeys for deterministic warning/toolset rendering. |
| pkg/cli/interactive.go | Uses sliceutil.SortedKeys for deterministic network bucket output. |
| pkg/cli/forecast.go | Uses sliceutil.SortedKeys for deterministic trigger-name extraction. |
| pkg/cli/firewall_log.go | Uses sliceutil.SortedKeys for deterministic merged domain output. |
| pkg/cli/experiments_command.go | Uses sliceutil.SortedKeys for deterministic assignment formatting. |
| pkg/cli/experiments_analyze_statistics.go | Uses sliceutil.SortedKeys for deterministic variant ordering. |
| pkg/cli/codemod_github_app.go | Removes local helper in favor of shared YAML-frontmatter helper. |
| pkg/cli/codemod_effective_tokens_to_ai_credits.go | Removes local expression helper in favor of shared YAML-frontmatter helper. |
| pkg/cli/codemod_dependabot_permissions.go | Uses sliceutil.SortedKeys for deterministic remaining-permissions ordering. |
| pkg/cli/audit_report_experiments.go | Uses sliceutil.SortedKeys for deterministic audit label/assignment output. |
| pkg/cli/audit_expanded.go | Uses sliceutil.SortedKeys for deterministic MCP server listing. |
| pkg/cli/audit_diff.go | Uses sliceutil.SortedKeys for deterministic diff output ordering. |
| pkg/cli/audit_cross_run.go | Uses sliceutil.SortedKeys for deterministic cross-run inventories. |
| pkg/cli/audit_comparison.go | Uses sliceutil.SortedKeys for deterministic failure-server aggregation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 66/66 changed files
- Comments generated: 0
- Review effort level: Low
… helper redistribution Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (302 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. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.mcszy.xyz/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out, /tdd, and /improve-codebase-architecture — leaving a COMMENT (no blocking issues found).
📋 Key Themes & Highlights
Positive highlights
- ✅ Massive cleanup: 101
SortedKeyssubstitutions eliminate a recurring 4-line idiom across 59 files — single, obvious commit of intent - ✅ Correct dedup consolidation:
canonicalReposScopenow uses the shared utility; the sort-then-dedup → dedup-then-sort reorder is semantically equivalent - ✅ Well-motivated redistribution:
getEngineAPIHosts,buildSafeOutputJobsEnvVars,isDescendant, andisExpressionValueall land in files that match their purpose - ✅ Logger renamed correctly:
notifyCommentLog→safeOutputsEnvLogafter the move - ✅
toEnvVarCaseremoval is clean —normalizeJobNameForEnvVaris a strict superset for all current inputs
Improvement opportunities (inline comments above)
SortedKeysnil-for-empty contract (update_container_pins.go:243) — only one of 101 call sites guards against nil; eitherSortedKeysshould guarantee non-nil or the contract should be documented- Orphaned test (
safe_outputs_env.go:319) —TestBuildSafeOutputJobsEnvVarslives innotify_comment_test.go, not the new home file isDescendantneeds tests (yaml_frontmatter_utils.go:250) —isNestedUnderhas table-driven tests;isDescendant(different calling convention) does notgetEngineAPIHostsneeds tests (engine_api_targets.go:277) — engine-type dispatch + override path with no regression coverage- Dedup comment (
cache_integrity.go:143) — order reversal (dedup-then-sort vs old sort-then-dedup) is correct but warrants a one-liner to aid future readers
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 135.5 AIC · ⌖ 10 AIC · ⊞ 6.6K
| for img := range imageSet { | ||
| images = append(images, img) | ||
| images := sliceutil.SortedKeys(imageSet) | ||
| if images == nil { |
There was a problem hiding this comment.
[/improve-codebase-architecture] This nil guard is the only one across ~100 SortedKeys call sites in this PR — exposing a contract gap: sliceutil.SortedKeys can return nil for empty maps because slices.Sorted over an empty iterator starts from a nil slice.
💡 Suggestion
For most callers, nil and []string{} are identical (range-for, len). But callers that JSON-marshal, compare against nil, or pass to APIs that distinguish null from [] are silently affected at the other call sites.
Option A — harden SortedKeys to always return non-nil:
func SortedKeys[K cmp.Ordered, V any](m map[K]V) []K {
if len(m) == 0 {
return []K{}
}
return slices.Sorted(maps.Keys(m))
}Option B — add a doc comment to SortedKeys explicitly stating nil-for-empty behaviour so future callers can guard when it matters.
Either way, the inconsistency between this lone guard and the 100+ unguarded sites should be resolved.
@copilot please address this.
| // - Custom safe-output jobs (from safe-outputs.jobs) with an empty URL key, so the handler | ||
| // manager knows those message types are handled by a dedicated job step and should be | ||
| // skipped gracefully rather than reported as "No handler loaded". | ||
| func buildSafeOutputJobsEnvVars(jobNames []string) (string, []string) { |
There was a problem hiding this comment.
[/zoom-out] The test for this function, TestBuildSafeOutputJobsEnvVars, still lives in pkg/workflow/notify_comment_test.go — the file it was moved from. While Go allows any file in the package to test any unexported symbol, the co-location is now misleading: readers of safe_outputs_env.go won't find coverage in the obvious place.
💡 Suggestion
Move (or duplicate) TestBuildSafeOutputJobsEnvVars to safe_outputs_env_test.go to match the source file. This also makes it easier to spot which functions lack coverage when skimming test files.
@copilot please address this.
| // isDescendant returns true if childIndent is deeper (more indented) than parentIndent. | ||
| // It is used as a "belongs to this block" check — any line more indented than the parent | ||
| // is treated as being within the parent's scope. | ||
| func isDescendant(childIndent, parentIndent string) bool { |
There was a problem hiding this comment.
[/tdd] isDescendant is newly consolidated here next to isNestedUnder, but has no unit tests — unlike isNestedUnder which has table-driven tests in yaml_frontmatter_utils_test.go.
💡 Suggestion
Add a table-driven test to match the existing isNestedUnder coverage:
func TestIsDescendant(t *testing.T) {
tests := []struct{
name string
childIndent string
parentIndent string
want bool
}{
{"deeper", " ", " ", true},
{"same", " ", " ", false},
{"shallower"," ", " ", false},
{"both empty","", "", false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := isDescendant(tt.childIndent, tt.parentIndent); got != tt.want {
t.Errorf("isDescendant(%q, %q) = %v, want %v", tt.childIndent, tt.parentIndent, got, tt.want)
}
})
}
}Also worth noting: isDescendant(childIndent, parentIndent) takes pre-extracted indent strings, while isNestedUnder(currentLine, parentIndent) takes a full line and extracts the indent itself. A doc comment distinguishing the two calling conventions would prevent misuse.
@copilot please address this.
| // Resolution order (per engine): | ||
| // - engine.api-target (explicit GHES / enterprise override) takes precedence | ||
| // - Default public API hostname(s) for the engine | ||
| func getEngineAPIHosts(data *WorkflowData, engine CodingAgentEngine) []string { |
There was a problem hiding this comment.
[/tdd] getEngineAPIHosts has engine-type dispatch with 4 hardcoded Copilot endpoints and an api-target override path, but no unit tests. A future change to the hostname list (e.g. adding an endpoint or changing the override logic) has no regression protection.
💡 Suggested test skeleton
func TestGetEngineAPIHosts(t *testing.T) {
tests := []struct {
name string
engine CodingAgentEngine
apiTarget string
want []string
}{
{"nil engine", nil, "", nil},
{"copilot defaults", &CopilotEngine{}, "",
[]string{"api.enterprise.githubcopilot.com", "api.githubcopilot.com", ...}},
{"explicit api-target overrides engine", &CopilotEngine{}, "ghes.example.com",
[]string{"ghes.example.com"}},
{"claude", &ClaudeEngine{}, "", []string{"api.anthropic.com"}},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
data := &WorkflowData{EngineConfig: &EngineConfig{APITarget: tt.apiTarget}}
got := getEngineAPIHosts(data, tt.engine)
if !slices.Equal(got, tt.want) {
t.Errorf("got %v, want %v", got, tt.want)
}
})
}
}@copilot please address this.
| strs = append(strs, strings.ToLower(s)) | ||
| } | ||
| } | ||
| strs = sliceutil.Deduplicate(strs) |
There was a problem hiding this comment.
[/improve-codebase-architecture] The old code sorted first, then used adjacent-dedup. The new code deduplicates first (preserving insertion order), then sorts — semantically equivalent, and correctly so. Consider a brief inline comment to make that explicit, since the order reversal can surprise readers expecting the classic sort-then-dedup pattern.
💡 Suggested comment
// Deduplicate (order-independent), then sort for deterministic hashing.
strs = sliceutil.Deduplicate(strs)
sort.Strings(strs)@copilot please address this.
There was a problem hiding this comment.
Clean mechanical refactor — all 101+ SortedKeys replacements are semantically equivalent to the manual collect-then-sort idiom they replace. The function redistributions (isDescendant, isExpressionValue, getEngineAPIHosts, buildSafeOutputJobsEnvVars) are same-package moves with no caller changes needed. The cache_integrity.go dedup-before-sort vs sort-then-adjacent-dedup change is provably equivalent. The toEnvVarCase→normalizeJobNameForEnvVar substitution produces identical output for all current hardcoded job names.
One non-blocking observation: TestCanonicalReposScope covers dedup for []any but not for []string; see inline comment.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 152.4 AIC · ⌖ 12 AIC · ⊞ 5.2K
| for i, s := range v { | ||
| strs[i] = strings.ToLower(s) | ||
| } | ||
| strs = sliceutil.Deduplicate(strs) |
There was a problem hiding this comment.
Missing dedup test for the []string branch: the []any case has a "dedup array" test in TestCanonicalReposScope but the []string case only has a sort test — no duplicate-entry coverage.
💡 Suggested addition to TestCanonicalReposScope
{name: "dedup string slice", repos: []string{"github/gh-aw", "github/gh-aw"}, expected: "github/gh-aw"},Both branches were changed from sort→adjacent-dedup to sliceutil.Deduplicate→sort. The []any branch already has regression coverage for dedup; adding the analogous case for []string keeps the test matrix symmetric and catches any future regression in this branch.
|
@copilot run pr-finisher skill |
|
🎉 This pull request is included in a new release. Release: |
Addresses the semantic function clustering analysis in the linked issue: reinvented utilities at ~40+ sites, inconsistent dedup within a single file, and several helpers living in the wrong file (
notify_comment.goaccumulating unrelated logic).Changes
sliceutil.SortedKeyssweep (101 replacements, 59 files)Replaced the manual collect-then-sort idiom with the existing utility:
Covers
pkg/workflow,pkg/cli, andpkg/parser.cache_integrity.godedup fixcanonicalReposScopewas hand-rolling sorted adjacent-dedup for both[]anyand[]stringcases — while the same file already calledsliceutil.Deduplicateat line 110. Replaced both withsliceutil.Deduplicate+sort.Strings.Outlier function redistribution
isDescendantcodemod_github_app.goyaml_frontmatter_utils.goisNestedUnderisExpressionValuecodemod_effective_tokens_to_ai_credits.goyaml_frontmatter_utils.go${{ }}detectiongetEngineAPIHostsnotify_comment.goengine_api_targets.gobuildSafeOutputJobsEnvVars+systemSafeOutputJobNamesnotify_comment.gosafe_outputs_env.gotoEnvVarCaseremovedDuplicate of
normalizeJobNameForEnvVar(which also handles hyphens and digits). RemovedtoEnvVarCasefromnotify_comment.go;buildSafeOutputJobsEnvVarsnow usesnormalizeJobNameForEnvVardirectly.Net: 66 files, −423 lines (302 added / 725 deleted).