Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pkg/cli/run_workflow_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/parser"
"github.com/github/gh-aw/pkg/sliceutil"
"github.com/github/gh-aw/pkg/workflow"
"github.com/goccy/go-yaml"
)
Expand Down Expand Up @@ -237,7 +238,7 @@ func validateWorkflowInputs(markdownPath string, providedInputs []string) error
// Add helpful information about valid inputs
if len(workflowInputs) > 0 {
var inputDescriptions []string
sortedNames := slices.Sorted(maps.Keys(workflowInputs))
sortedNames := sliceutil.SortedKeys(workflowInputs)
for _, name := range sortedNames {
def := workflowInputs[name]
required := ""
Expand Down
9 changes: 8 additions & 1 deletion pkg/sliceutil/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ All functions in this package are pure: they never modify their input. They are
| `Map` | `func[T, U any](slice []T, transform func(T) U) []U` | Applies `transform` to every element and returns the results as a new slice |
| `MapKeys` | `func[K comparable, V any](m map[K]V) []K` | Converts the keys of a map into a slice; order is not guaranteed |
| `FilterMapKeys` | `func[K comparable, V any](m map[K]V, predicate func(K, V) bool) []K` | Returns map keys for which `predicate(key, value)` is `true`; order is not guaranteed |
| `SortedKeys` | `func[K cmp.Ordered, V any](m map[K]V) []K` | Returns the keys of a map in sorted order; K must satisfy `cmp.Ordered` (e.g. `string`, `int`) |
| `Any` | `func[T any](slice []T, predicate func(T) bool) bool` | Returns `true` if at least one element satisfies `predicate`; returns `false` for nil or empty slices |
| `Deduplicate` | `func[T comparable](slice []T) []T` | Returns a new slice with duplicate elements removed, preserving order of first occurrence |
| `MergeUnique` | `func[T comparable](base []T, extra ...T) []T` | Returns a deduplicated slice starting with `base` and appending unseen values from `extra` |
Expand Down Expand Up @@ -48,6 +49,11 @@ merged := sliceutil.MergeUnique([]string{"a", "b"}, "b", "c")
// Exclude values
filtered := sliceutil.Exclude([]string{"a", "b", "c"}, "b")
// filtered = ["a", "c"]

// Sorted map keys
m := map[string]int{"banana": 2, "apple": 1}
keys := sliceutil.SortedKeys(m)
// keys = ["apple", "banana"]
```

## Dependencies
Expand All @@ -62,7 +68,8 @@ filtered := sliceutil.Exclude([]string{"a", "b", "c"}, "b")

- `Any` is implemented via `slices.ContainsFunc` from the standard library.
- `Deduplicate`, `MergeUnique`, and `Exclude` use hash sets (`map[T]struct{}`) for O(n) behavior.
- None of these functions sort their output; callers that require sorted results should call `slices.Sort` on the returned slice.
- `SortedKeys` delegates to `slices.Sorted(maps.Keys(m))` from the standard library and returns a new sorted slice each call.
- None of the other functions sort their output; callers that require sorted results should call `slices.Sort` on the returned slice.

---

Expand Down
9 changes: 9 additions & 0 deletions pkg/sliceutil/sliceutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
package sliceutil

import (
"cmp"
"maps"
"slices"

"github.com/github/gh-aw/pkg/logger"
Expand Down Expand Up @@ -55,6 +57,13 @@ func FilterMapKeys[K comparable, V any](m map[K]V, predicate func(K, V) bool) []
return result
}

// SortedKeys returns the keys of a map in sorted order.
// K must satisfy cmp.Ordered (e.g. string, int).
// This is a pure function that does not modify the input map.
func SortedKeys[K cmp.Ordered, V any](m map[K]V) []K {
return slices.Sorted(maps.Keys(m))
}

// Any returns true if at least one element in the slice satisfies the predicate.
// Returns false for nil or empty slices.
// This is a pure function that does not modify the input slice.
Expand Down
28 changes: 28 additions & 0 deletions pkg/sliceutil/spec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,34 @@ func TestSpec_PublicAPI_FilterMapKeys(t *testing.T) {
})
}

// TestSpec_PublicAPI_SortedKeys validates the documented behavior of SortedKeys
// as described in the sliceutil README.md specification.
func TestSpec_PublicAPI_SortedKeys(t *testing.T) {
t.Run("returns map keys in sorted order", func(t *testing.T) {
m := map[string]int{"banana": 2, "apple": 1, "cherry": 3}
keys := SortedKeys(m)
assert.Equal(t, []string{"apple", "banana", "cherry"}, keys, "SortedKeys should return keys in sorted order")
})

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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})

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")
})

t.Run("works with non-string ordered key types", func(t *testing.T) {
m := map[int]string{3: "c", 1: "a", 2: "b"}
keys := SortedKeys(m)
assert.Equal(t, []int{1, 2, 3}, keys, "SortedKeys should sort integer keys numerically")
})
}

// TestSpec_PublicAPI_Any validates the documented behavior of Any as described
// in the sliceutil README.md specification.
func TestSpec_PublicAPI_Any(t *testing.T) {
Expand Down
4 changes: 1 addition & 3 deletions pkg/workflow/bots_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,6 @@ Test workflow content with bot and default roles.`

// TestMergeBots tests the mergeBots helper function
func TestMergeBots(t *testing.T) {
compiler := NewCompiler()

tests := []struct {
name string
top []string
Expand Down Expand Up @@ -243,7 +241,7 @@ func TestMergeBots(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := compiler.mergeBots(tt.top, tt.imported)
result := mergeBots(tt.top, tt.imported)
assert.Equal(t, tt.expected, result, "mergeBots result mismatch")
})
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/workflow/compiler_main_job.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package workflow

import (
"fmt"
"maps"
"os"
"slices"
"sort"
Expand All @@ -13,6 +12,7 @@ import (
"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/setutil"
"github.com/github/gh-aw/pkg/sliceutil"
)

var compilerMainJobLog = logger.New("workflow:compiler_main_job")
Expand Down Expand Up @@ -109,7 +109,7 @@ func (c *Compiler) buildMainJob(data *WorkflowData, activationJobCreated bool) (
// so the agent job gets them transitively through activation
// Custom jobs that depend on agent should run AFTER the agent job, not before it
if data.Jobs != nil {
for _, jobName := range slices.Sorted(maps.Keys(data.Jobs)) {
for _, jobName := range sliceutil.SortedKeys(data.Jobs) {
// Skip built-in jobs as they are handled separately and should not become custom dependencies.
if isBuiltinJobName(jobName) {
continue
Expand Down
6 changes: 3 additions & 3 deletions pkg/workflow/compiler_orchestrator_workflow.go
Original file line number Diff line number Diff line change
Expand Up @@ -428,11 +428,11 @@ func (c *Compiler) extractAdditionalConfigurations(
}

workflowData.Roles = c.extractRoles(frontmatter)
workflowData.Bots = expandBotNames(c.mergeBots(c.extractBots(frontmatter), importsResult.MergedBots))
workflowData.Bots = expandBotNames(mergeBots(c.extractBots(frontmatter), importsResult.MergedBots))
workflowData.LabelNames = c.extractLabelNames(frontmatter)
workflowData.RateLimit = c.extractRateLimitConfig(frontmatter)
workflowData.SkipRoles = c.mergeSkipRoles(c.extractSkipRoles(frontmatter), importsResult.MergedSkipRoles)
workflowData.SkipBots = expandBotNames(c.mergeSkipBots(c.extractSkipBots(frontmatter), importsResult.MergedSkipBots))
workflowData.SkipRoles = mergeSkipRoles(c.extractSkipRoles(frontmatter), importsResult.MergedSkipRoles)
workflowData.SkipBots = expandBotNames(mergeSkipBots(c.extractSkipBots(frontmatter), importsResult.MergedSkipBots))
workflowData.SkipAuthorAssociations = c.extractSkipAuthorAssociations(frontmatter)
workflowData.AllowBotAuthoredTriggerComment = c.extractAllowBotAuthoredTriggerComment(frontmatter)
workflowData.ActivationGitHubToken = c.resolveActivationGitHubToken(frontmatter, importsResult)
Expand Down
6 changes: 3 additions & 3 deletions pkg/workflow/compiler_yaml_step_conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ package workflow

import (
"fmt"
"maps"
"os"
"slices"
"strings"

"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/sliceutil"
"github.com/goccy/go-yaml"
)

Expand Down Expand Up @@ -153,7 +153,7 @@ func (c *Compiler) renderStepFromMap(out *strings.Builder, step map[string]any,
case map[string]any:
// For complex fields like "with" or "env" — sort keys for stable output.
fmt.Fprintf(out, "%s:\n", field)
for _, key := range slices.Sorted(maps.Keys(v)) {
for _, key := range sliceutil.SortedKeys(v) {
if field == "env" {
fmt.Fprintf(out, "%s %s: %s\n", indent, key, formatStepEnvValueForYAML(v[key]))
} else {
Expand Down Expand Up @@ -194,7 +194,7 @@ func (c *Compiler) renderStepFromMap(out *strings.Builder, step map[string]any,
case map[string]any:
// Sort keys for stable output.
fmt.Fprintf(out, "%s:\n", field)
for _, key := range slices.Sorted(maps.Keys(v)) {
for _, key := range sliceutil.SortedKeys(v) {
if field == "env" {
fmt.Fprintf(out, "%s %s: %s\n", indent, key, formatStepEnvValueForYAML(v[key]))
} else {
Expand Down
19 changes: 9 additions & 10 deletions pkg/workflow/domains.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@ import (
_ "embed"
"encoding/json"
"fmt"
"maps"
"slices"
"sort"
"strings"

"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/sliceutil"
"github.com/github/gh-aw/pkg/stringutil"
)

Expand Down Expand Up @@ -359,7 +358,7 @@ func getEcosystemDomains(category string) []string {
domainMap[d] = struct{}{}
}
}
result := slices.Sorted(maps.Keys(domainMap))
result := sliceutil.SortedKeys(domainMap)
return result
}

Expand Down Expand Up @@ -425,7 +424,7 @@ func getDomainsFromRuntimes(runtimes map[string]any) []string {
}
}

return slices.Sorted(maps.Keys(domainMap))
return sliceutil.SortedKeys(domainMap)
}

// GetAllowedDomains returns the allowed domains from network permissions.
Expand Down Expand Up @@ -520,7 +519,7 @@ func GetAllowedDomains(network *NetworkPermissions) []string {
}
}

return slices.Sorted(maps.Keys(domainMap))
return sliceutil.SortedKeys(domainMap)
}

// ecosystemPriority defines the order in which ecosystems are checked by GetDomainEcosystem.
Expand Down Expand Up @@ -729,7 +728,7 @@ func mergeDomainsWithNetworkToolsAndRuntimes(defaultDomains []string, network *N
}
}

domains := slices.Sorted(maps.Keys(domainMap))
domains := sliceutil.SortedKeys(domainMap)

// Join with commas for AWF --allow-domains flag
return strings.Join(domains, ",")
Expand Down Expand Up @@ -856,7 +855,7 @@ func GetBlockedDomains(network *NetworkPermissions) []string {
}
}

return slices.Sorted(maps.Keys(domainMap))
return sliceutil.SortedKeys(domainMap)
}

// formatBlockedDomains formats blocked domains as a comma-separated string suitable for AWF's --block-domains flag
Expand Down Expand Up @@ -922,7 +921,7 @@ func mergeAPITargetDomains(domainsStr string, apiTarget string) string {
domainMap[d] = struct{}{}
}

return strings.Join(slices.Sorted(maps.Keys(domainMap)), ",")
return strings.Join(sliceutil.SortedKeys(domainMap), ",")
}

// computeAllowedDomainsForSanitization computes the allowed domains for sanitization
Expand Down Expand Up @@ -1009,7 +1008,7 @@ func expandAllowedDomains(entries []string) []string {
domainMap[entry] = struct{}{}
}
}
return slices.Sorted(maps.Keys(domainMap))
return sliceutil.SortedKeys(domainMap)
}

// computeExpandedAllowedDomainsForSanitization computes the allowed domains for URL sanitization,
Expand Down Expand Up @@ -1050,5 +1049,5 @@ func (c *Compiler) computeExpandedAllowedDomainsForSanitization(data *WorkflowDa
domainMap["github.com"] = struct{}{}

// Produce a sorted, comma-separated result
return strings.Join(slices.Sorted(maps.Keys(domainMap)), ","), nil
return strings.Join(sliceutil.SortedKeys(domainMap), ","), nil
}
10 changes: 0 additions & 10 deletions pkg/workflow/map_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
//
// Map Operations:
// - excludeMapKeys() - Create new map excluding specified keys
// - sortedMapKeys() - Return sorted keys of a map[string]string
//
// For type conversion utilities, use pkg/typeutil directly:
// - typeutil.ParseIntValue() - Strictly parse numeric types to int; returns (value, ok).
Expand All @@ -34,9 +33,6 @@
package workflow

import (
"maps"
"slices"

"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/setutil"
)
Expand Down Expand Up @@ -66,9 +62,3 @@ func excludeMapKeys(original map[string]any, excludeKeys ...string) map[string]a
}
return result
}

// sortedMapKeys returns the keys of a map[string]string in sorted order.
// Used to produce deterministic output when writing environment variables.
func sortedMapKeys(m map[string]string) []string {
return slices.Sorted(maps.Keys(m))
}
3 changes: 2 additions & 1 deletion pkg/workflow/mcp_renderer_github.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/sliceutil"
)

// RenderGitHubMCP generates the GitHub MCP server configuration
Expand Down Expand Up @@ -189,7 +190,7 @@ func (r *MCPConfigRendererUnified) renderGitHubTOML(yaml *strings.Builder, githu
)

// Write environment variables in sorted order for deterministic output
envKeys := sortedMapKeys(envVars)
envKeys := sliceutil.SortedKeys(envVars)

writeTOMLInlineStringMapSection(yaml, " ", "env", envVars)

Expand Down
6 changes: 4 additions & 2 deletions pkg/workflow/mcp_renderer_section_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import (
"encoding/json"
"fmt"
"strings"

"github.com/github/gh-aw/pkg/sliceutil"
)

func writeJSONStringMapEntries(yaml *strings.Builder, values map[string]string, indent string) {
for i, key := range sortedMapKeys(values) {
for i, key := range sliceutil.SortedKeys(values) {
comma := ","
if i == len(values)-1 {
comma = ""
Expand Down Expand Up @@ -36,7 +38,7 @@ func writeJSONStringMapSection(yaml *strings.Builder, indent, name string, value

func writeTOMLInlineStringMapSection(yaml *strings.Builder, indent, name string, values map[string]string) {
fmt.Fprintf(yaml, "%s%s = { ", indent, name)
for i, key := range sortedMapKeys(values) {
for i, key := range sliceutil.SortedKeys(values) {
if i > 0 {
yaml.WriteString(", ")
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/workflow/mcp_setup_generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ func buildSafeOutputsConfigRuntimeEnvVars(safeOutputConfig string) ([]string, ma
// Prefix secret env vars to avoid colliding with reserved/known step env var names.
addEnvValue(safeOutputsSecretEnvPrefix+k, v)
}
return sortedMapKeys(envValues), envValues
return sliceutil.SortedKeys(envValues), envValues
}

func buildSafeOutputsConfigRuntimeData(safeOutputConfig string) (string, []string, map[string]string) {
Expand Down
Loading
Loading