Refactor duplicated helper utilities and centralize pointer helpers via typeutil.Ptr (#35349)

Author: Copilot

pkg/cli/codemod_engine_env_secrets.go

@@ -4,6 +4,7 @@ import (
 	"strings"
 
 	"github.com/github/gh-aw/pkg/logger"
+	"github.com/github/gh-aw/pkg/sliceutil"
 	"github.com/github/gh-aw/pkg/workflow"
 )
 
@@ -54,7 +55,7 @@ func getEngineEnvSecretsCodemod() Codemod {
 				return cleaned, true
 			})
 			if applied {
-				engineEnvSecretsCodemodLog.Printf("Removed unsafe engine.env secret keys: %v", mapKeys(unsafeKeys))
+				engineEnvSecretsCodemodLog.Printf("Removed unsafe engine.env secret keys: %v", sliceutil.MapKeys(unsafeKeys))
 			}
 			return newContent, applied, err
 		},
@@ -238,11 +239,3 @@ func removeEmptyEngineEnvBlock(lines []string) []string {
 	}
 	return result
 }
-
-func mapKeys(m map[string]bool) []string {
-	keys := make([]string, 0, len(m))
-	for k := range m {
-		keys = append(keys, k)
-	}
-	return keys
-}

pkg/cli/gateway_logs_timeline.go

@@ -23,6 +23,8 @@ import (
 	"sort"
 	"strings"
 	"time"
+
+	"github.com/github/gh-aw/pkg/fileutil"
 )
 
 // TimelineEventSource identifies which system produced a timeline event.
@@ -285,23 +287,17 @@ func auditEntryToTimelineEvent(entry AuditLogEntry) (UnifiedTimelineEvent, bool)
 // Returns an empty string when no file is found.
 func findGatewayJSONLPath(logDir string) string {
 	// Root-level gateway.jsonl (pre-mcp-logs-subdirectory artifact layout)
-	if p := filepath.Join(logDir, "gateway.jsonl"); fileExists(p) {
+	if p := filepath.Join(logDir, "gateway.jsonl"); fileutil.FileExists(p) {
 		return p
 	}
 	// mcp-logs/ subdirectory (standard layout after artifact download)
-	if p := filepath.Join(logDir, "mcp-logs", "gateway.jsonl"); fileExists(p) {
+	if p := filepath.Join(logDir, "mcp-logs", "gateway.jsonl"); fileutil.FileExists(p) {
 		return p
 	}
 	// Fallback: rpc-messages.jsonl
 	return findRPCMessagesPath(logDir)
 }
 
-// fileExists is a small helper that reports whether path exists and is a regular file.
-func fileExists(path string) bool {
-	info, err := os.Stat(path)
-	return err == nil && !info.IsDir()
-}
-
 // collectGatewayTimelineEvents reads the gateway JSONL file in logDir and returns a slice
 // of timeline events, one per parseable, recognised log entry. The file may be
 // gateway.jsonl or rpc-messages.jsonl (the latter is used as a fallback).

pkg/cli/mcp_helpers.go

@@ -10,6 +10,7 @@ import (
 	"path/filepath"
 
 	"github.com/github/gh-aw/pkg/logger"
+	"github.com/github/gh-aw/pkg/typeutil"
 )
 
 var mcpHelpersLog = logger.New("cli:mcp_helpers")
@@ -40,7 +41,7 @@ func GetBinaryPath() (string, error) {
 }
 
 // boolPtr returns a pointer to the given bool value, used for optional *bool fields.
-func boolPtr(b bool) *bool { return new(b) }
+func boolPtr(b bool) *bool { return typeutil.Ptr(b) }
 
 // logAndValidateBinaryPath determines the binary path, logs it, and validates it exists.
 // Returns the detected binary path and an error if the path cannot be determined or if the file doesn't exist.

pkg/typeutil/convert_test.go

@@ -8,6 +8,18 @@ import (
 	"testing"
 )
 
+func TestPtr(t *testing.T) {
+	b := Ptr(true)
+	if b == nil || !*b {
+		t.Fatalf("Ptr(true) = %v, want pointer to true", b)
+	}
+
+	s := Ptr("hello")
+	if s == nil || *s != "hello" {
+		t.Fatalf("Ptr(\"hello\") = %v, want pointer to hello", s)
+	}
+}
+
 func TestParseIntValue(t *testing.T) {
 	tests := []struct {
 		name     string

pkg/typeutil/ptr.go

@@ -0,0 +1,4 @@
+package typeutil
+
+// Ptr returns a pointer to the provided value.
+func Ptr[T any](v T) *T { return &v }

pkg/workflow/compiler_safe_outputs_config_test.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/github/gh-aw/pkg/typeutil"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -1666,10 +1667,10 @@ func TestHandlerConfigPushToPullRequestBranchPatchSizeOverridesGlobal(t *testing
 }
 
 // testBoolPtr is a helper function for bool pointers in config tests
-func testBoolPtr(b bool) *bool { return new(b) }
+func testBoolPtr(b bool) *bool { return typeutil.Ptr(b) }
 
 // testStringPtr is a helper function for string pointers in config tests
-func testStringPtr(s string) *string { return new(s) }
+func testStringPtr(s string) *string { return typeutil.Ptr(s) }
 
 // TestAutoEnabledHandlers tests that missing_tool and missing_data
 // are automatically enabled even when not explicitly configured.

pkg/workflow/role_checks.go

@@ -633,29 +633,31 @@ func (c *Compiler) extractAllowBotAuthoredTriggerComment(frontmatter map[string]
 	return false
 }
 
-// mergeStringSlicesDedup merges two string slices with deduplication (preserving order of first occurrence).
-// Logs the merged result under the given logLabel when the result is non-empty.
-func mergeStringSlicesDedup(top, imported []string, logLabel string) []string {
-	result := sliceutil.Deduplicate(append(top, imported...))
+// mergeSkipRoles merges top-level skip-roles with imported skip-roles (union)
+func (c *Compiler) mergeSkipRoles(topSkipRoles []string, importedSkipRoles []string) []string {
+	result := sliceutil.MergeUnique(topSkipRoles, importedSkipRoles...)
 	if len(result) > 0 {
-		roleLog.Printf("Merged %s: %v (top=%d, imported=%d, total=%d)", logLabel, result, len(top), len(imported), len(result))
+		roleLog.Printf("Merged %s: %v (top=%d, imported=%d, total=%d)", "skip-roles", result, len(topSkipRoles), len(importedSkipRoles), len(result))
 	}
 	return result
 }
 
-// mergeSkipRoles merges top-level skip-roles with imported skip-roles (union)
-func (c *Compiler) mergeSkipRoles(topSkipRoles []string, importedSkipRoles []string) []string {
-	return mergeStringSlicesDedup(topSkipRoles, importedSkipRoles, "skip-roles")
-}
-
 // mergeSkipBots merges top-level skip-bots with imported skip-bots (union)
 func (c *Compiler) mergeSkipBots(topSkipBots []string, importedSkipBots []string) []string {
-	return mergeStringSlicesDedup(topSkipBots, importedSkipBots, "skip-bots")
+	result := sliceutil.MergeUnique(topSkipBots, importedSkipBots...)
+	if len(result) > 0 {
+		roleLog.Printf("Merged %s: %v (top=%d, imported=%d, total=%d)", "skip-bots", result, len(topSkipBots), len(importedSkipBots), len(result))
+	}
+	return result
 }
 
 // mergeBots merges top-level bots with imported bots (union)
 func (c *Compiler) mergeBots(topBots []string, importedBots []string) []string {
-	return mergeStringSlicesDedup(topBots, importedBots, "bots")
+	result := sliceutil.MergeUnique(topBots, importedBots...)
+	if len(result) > 0 {
+		roleLog.Printf("Merged %s: %v (top=%d, imported=%d, total=%d)", "bots", result, len(topBots), len(importedBots), len(result))
+	}
+	return result
 }
 
 // extractActivationGitHubToken extracts the 'github-token' field from the 'on:' section of frontmatter.

pkg/workflow/test_helpers_shared_test.go

@@ -1,20 +1,22 @@
 package workflow
 
+import "github.com/github/gh-aw/pkg/typeutil"
+
 // boolPtr returns a pointer to a bool value.
 // This is a shared helper used by both unit and integration tests.
 func boolPtr(b bool) *bool {
-	return new(b)
+	return typeutil.Ptr(b)
 }
 
 // ptrBool returns a pointer to a bool value.
 func ptrBool(b bool) *bool {
-	return new(b)
+	return typeutil.Ptr(b)
 }
 
 // strPtr returns a pointer to a string value.
 // This is a shared helper used by tests to create *string values for templatable fields.
 func strPtr(s string) *string {
-	return new(s)
+	return typeutil.Ptr(s)
 }
 
 // mockValidationError helps create validation errors for testing.