From d0e9506606e3ec814137bbdedb766be2232ee3a2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Apr 2026 11:56:35 +0000 Subject: [PATCH 1/2] Initial plan From 9ccfdfa3c1cc88e83b9d8248df0bbf1e9dd00791 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 8 Apr 2026 12:18:37 +0000 Subject: [PATCH 2/2] refactor: implement semantic function clustering improvements (#issue) - Issue 1: Replace DependencyGraph.findGitRoot() with findGitRootForPath() in pkg/cli/dependency_graph.go, removing the manual filesystem-walking implementation in favor of the existing git-command-based utility - Issue 2: Remove 5 alias wrappers from pkg/workflow/map_helpers.go; migrate all internal callers (claude_logs.go, mcp_scripts_parser.go, frontmatter_extraction_metadata.go, publish_artifacts.go, safe_outputs_config.go, publish_assets.go, metrics.go) to use typeutil.* directly; update test files accordingly - Issue 3: Add typeutil.ParseBool() to pkg/typeutil/convert.go; update parseBoolFromFrontmatter and ParseBoolFromConfig to delegate to it" Agent-Logs-Url: https://github.com/github/gh-aw/sessions/283402d8-43b1-4753-8c0b-cfcca2e2a476 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/dependency_graph.go | 30 ++---- pkg/cli/logs_parsing_test.go | 5 +- pkg/parser/frontmatter_hash.go | 10 +- pkg/typeutil/convert.go | 16 +++ pkg/workflow/claude_logs.go | 25 ++--- pkg/workflow/config_helpers.go | 19 ++-- .../frontmatter_extraction_metadata.go | 9 +- pkg/workflow/map_helpers.go | 83 +++------------ pkg/workflow/map_helpers_test.go | 18 ++-- pkg/workflow/mcp_scripts_parser.go | 5 +- pkg/workflow/metrics.go | 31 +++--- pkg/workflow/metrics_test.go | 16 +-- pkg/workflow/publish_artifacts.go | 9 +- pkg/workflow/publish_assets.go | 3 +- pkg/workflow/safe_outputs_config.go | 5 +- pkg/workflow/type_conversion_test.go | 100 +++++++++--------- 16 files changed, 167 insertions(+), 217 deletions(-) diff --git a/pkg/cli/dependency_graph.go b/pkg/cli/dependency_graph.go index b43883c7e04..246b5eec957 100644 --- a/pkg/cli/dependency_graph.go +++ b/pkg/cli/dependency_graph.go @@ -247,7 +247,12 @@ func (g *DependencyGraph) resolveImportPath(importPath string, baseDir string) s // If that fails, try resolving with parser's cache-aware resolution // Note: We create a minimal cache here just for resolution - importCache := parser.NewImportCache(g.findGitRoot()) + gitRoot, err := findGitRootForPath(g.workflowsDir) + if err != nil { + depGraphLog.Printf("Failed to find git root, using fallback: %v", err) + gitRoot = g.workflowsDir + } + importCache := parser.NewImportCache(gitRoot) fullPath, err := parser.ResolveIncludePath(importPath, baseDir, importCache) if err != nil { depGraphLog.Printf("Failed to resolve import path %s: %v", importPath, err) @@ -258,29 +263,6 @@ func (g *DependencyGraph) resolveImportPath(importPath string, baseDir string) s return fullPath } -// findGitRoot finds the git repository root -func (g *DependencyGraph) findGitRoot() string { - depGraphLog.Printf("Finding git root starting from: %s", g.workflowsDir) - // Start from workflows directory and walk up - dir := g.workflowsDir - for { - gitDir := filepath.Join(dir, ".git") - if _, err := os.Stat(gitDir); err == nil { - depGraphLog.Printf("Found git root at: %s", dir) - return dir - } - parent := filepath.Dir(dir) - if parent == dir { - // Reached filesystem root - depGraphLog.Printf("Reached filesystem root, no .git directory found") - break - } - dir = parent - } - depGraphLog.Printf("Using fallback git root: %s", g.workflowsDir) - return g.workflowsDir // Fallback to workflows dir -} - // GetAffectedWorkflows returns the list of workflows that need to be recompiled // when the given file is modified func (g *DependencyGraph) GetAffectedWorkflows(modifiedPath string) []string { diff --git a/pkg/cli/logs_parsing_test.go b/pkg/cli/logs_parsing_test.go index d56f70b0b9f..6efcf116bc9 100644 --- a/pkg/cli/logs_parsing_test.go +++ b/pkg/cli/logs_parsing_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/github/gh-aw/pkg/testutil" + "github.com/github/gh-aw/pkg/typeutil" "github.com/github/gh-aw/pkg/workflow" ) @@ -165,7 +166,7 @@ func TestConvertToInt(t *testing.T) { } for _, tt := range tests { - result := workflow.ConvertToInt(tt.value) + result := typeutil.ConvertToInt(tt.value) if result != tt.expected { t.Errorf("ConvertToInt(%v) = %d, expected %d", tt.value, result, tt.expected) } @@ -186,7 +187,7 @@ func TestConvertToFloat(t *testing.T) { } for _, tt := range tests { - result := workflow.ConvertToFloat(tt.value) + result := typeutil.ConvertToFloat(tt.value) if result != tt.expected { t.Errorf("ConvertToFloat(%v) = %f, expected %f", tt.value, result, tt.expected) } diff --git a/pkg/parser/frontmatter_hash.go b/pkg/parser/frontmatter_hash.go index 28519f38517..2b78ea0abde 100644 --- a/pkg/parser/frontmatter_hash.go +++ b/pkg/parser/frontmatter_hash.go @@ -14,6 +14,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" ) var frontmatterHashLog = logger.New("parser:frontmatter_hash") @@ -21,14 +22,7 @@ var frontmatterHashLog = logger.New("parser:frontmatter_hash") // parseBoolFromFrontmatter extracts a boolean value from a frontmatter map. // Returns false if the key is absent, the map is nil, or the value is not a bool. func parseBoolFromFrontmatter(m map[string]any, key string) bool { - if m == nil { - return false - } - if v, ok := m[key]; ok { - b, _ := v.(bool) - return b - } - return false + return typeutil.ParseBool(m, key) } // FileReader is a function type that reads file content diff --git a/pkg/typeutil/convert.go b/pkg/typeutil/convert.go index a2621f39635..3eff737fd45 100644 --- a/pkg/typeutil/convert.go +++ b/pkg/typeutil/convert.go @@ -11,6 +11,9 @@ // the caller needs to distinguish "missing/invalid" from a zero value, or when string // inputs are not expected (e.g. YAML config field parsing). // +// Bool Extraction: +// - ParseBool() - Extract a bool from map[string]any by key; returns false on missing, nil map, or non-bool. +// // Safe Conversions (return 0 on overflow or invalid input): // - SafeUint64ToInt() - Convert uint64 to int, returning 0 on overflow // - SafeUintToInt() - Convert uint to int, returning 0 on overflow @@ -110,6 +113,19 @@ func ConvertToInt(val any) int { return 0 } +// ParseBool extracts a boolean value from a map[string]any by key. +// Returns false if the map is nil, the key is absent, or the value is not a bool. +func ParseBool(m map[string]any, key string) bool { + if m == nil { + return false + } + if v, ok := m[key]; ok { + b, _ := v.(bool) + return b + } + return false +} + // ConvertToFloat safely converts any value to float64, returning 0 on failure. // // Supported input types: float64, int, int64, and string (parsed via strconv.ParseFloat). diff --git a/pkg/workflow/claude_logs.go b/pkg/workflow/claude_logs.go index 468b94b16cc..2526ba9edd9 100644 --- a/pkg/workflow/claude_logs.go +++ b/pkg/workflow/claude_logs.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" ) var claudeLogsLog = logger.New("workflow:claude_logs") @@ -106,7 +107,7 @@ func (e *ClaudeEngine) extractClaudeResultMetrics(line string) LogMetrics { // Extract total_cost_usd directly if totalCost, exists := jsonData["total_cost_usd"]; exists { - if cost := ConvertToFloat(totalCost); cost > 0 { + if cost := typeutil.ConvertToFloat(totalCost); cost > 0 { metrics.EstimatedCost = cost } } @@ -114,10 +115,10 @@ func (e *ClaudeEngine) extractClaudeResultMetrics(line string) LogMetrics { // Extract usage information with all token types if usage, exists := jsonData["usage"]; exists { if usageMap, ok := usage.(map[string]any); ok { - inputTokens := ConvertToInt(usageMap["input_tokens"]) - outputTokens := ConvertToInt(usageMap["output_tokens"]) - cacheCreationTokens := ConvertToInt(usageMap["cache_creation_input_tokens"]) - cacheReadTokens := ConvertToInt(usageMap["cache_read_input_tokens"]) + inputTokens := typeutil.ConvertToInt(usageMap["input_tokens"]) + outputTokens := typeutil.ConvertToInt(usageMap["output_tokens"]) + cacheCreationTokens := typeutil.ConvertToInt(usageMap["cache_creation_input_tokens"]) + cacheReadTokens := typeutil.ConvertToInt(usageMap["cache_read_input_tokens"]) totalTokens := inputTokens + outputTokens + cacheCreationTokens + cacheReadTokens if totalTokens > 0 { @@ -128,7 +129,7 @@ func (e *ClaudeEngine) extractClaudeResultMetrics(line string) LogMetrics { // Extract number of turns if numTurns, exists := jsonData["num_turns"]; exists { - if turns := ConvertToInt(numTurns); turns > 0 { + if turns := typeutil.ConvertToInt(numTurns); turns > 0 { metrics.Turns = turns } } @@ -241,7 +242,7 @@ func (e *ClaudeEngine) parseClaudeJSONLog(logContent string, verbose bool) LogMe if typeStr, ok := entryType.(string); ok && typeStr == "result" { // Found the result payload, extract cost and token data if totalCost, exists := entry["total_cost_usd"]; exists { - if cost := ConvertToFloat(totalCost); cost > 0 { + if cost := typeutil.ConvertToFloat(totalCost); cost > 0 { metrics.EstimatedCost = cost } } @@ -249,10 +250,10 @@ func (e *ClaudeEngine) parseClaudeJSONLog(logContent string, verbose bool) LogMe // Extract usage information with all token types if usage, exists := entry["usage"]; exists { if usageMap, ok := usage.(map[string]any); ok { - inputTokens := ConvertToInt(usageMap["input_tokens"]) - outputTokens := ConvertToInt(usageMap["output_tokens"]) - cacheCreationTokens := ConvertToInt(usageMap["cache_creation_input_tokens"]) - cacheReadTokens := ConvertToInt(usageMap["cache_read_input_tokens"]) + inputTokens := typeutil.ConvertToInt(usageMap["input_tokens"]) + outputTokens := typeutil.ConvertToInt(usageMap["output_tokens"]) + cacheCreationTokens := typeutil.ConvertToInt(usageMap["cache_creation_input_tokens"]) + cacheReadTokens := typeutil.ConvertToInt(usageMap["cache_read_input_tokens"]) totalTokens := inputTokens + outputTokens + cacheCreationTokens + cacheReadTokens if totalTokens > 0 { @@ -263,7 +264,7 @@ func (e *ClaudeEngine) parseClaudeJSONLog(logContent string, verbose bool) LogMe // Extract number of turns if numTurns, exists := entry["num_turns"]; exists { - if turns := ConvertToInt(numTurns); turns > 0 { + if turns := typeutil.ConvertToInt(numTurns); turns > 0 { metrics.Turns = turns } } diff --git a/pkg/workflow/config_helpers.go b/pkg/workflow/config_helpers.go index 42e4f9827ac..e2b7154b12f 100644 --- a/pkg/workflow/config_helpers.go +++ b/pkg/workflow/config_helpers.go @@ -41,6 +41,7 @@ import ( "fmt" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" "github.com/goccy/go-yaml" ) @@ -182,18 +183,14 @@ func preprocessExpiresField(configData map[string]any, log *logger.Logger) bool // Returns the boolean value, or false if not present or invalid. // If log is provided, it will log the extracted value for debugging. func ParseBoolFromConfig(m map[string]any, key string, log *logger.Logger) bool { - if value, exists := m[key]; exists { - if log != nil { - log.Printf("Parsing %s from config", key) - } - if boolValue, ok := value.(bool); ok { - if log != nil { - log.Printf("Parsed %s from config: %t", key, boolValue) - } - return boolValue - } + if log != nil { + log.Printf("Parsing %s from config", key) + } + result := typeutil.ParseBool(m, key) + if log != nil { + log.Printf("Parsed %s from config: %t", key, result) } - return false + return result } // unmarshalConfig unmarshals a config value from a map into a typed struct using YAML. diff --git a/pkg/workflow/frontmatter_extraction_metadata.go b/pkg/workflow/frontmatter_extraction_metadata.go index 8a610367314..29ec9ec6f16 100644 --- a/pkg/workflow/frontmatter_extraction_metadata.go +++ b/pkg/workflow/frontmatter_extraction_metadata.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" ) var frontmatterMetadataLog = logger.New("workflow:frontmatter_extraction_metadata") @@ -172,9 +173,9 @@ func (c *Compiler) extractToolsTimeout(tools map[string]any) (string, error) { case int64: timeout = int(v) case uint: - timeout = safeUintToInt(v) // Safe conversion to prevent overflow (alert #418) + timeout = typeutil.SafeUintToInt(v) // Safe conversion to prevent overflow (alert #418) case uint64: - timeout = safeUint64ToInt(v) // Safe conversion to prevent overflow (alert #416) + timeout = typeutil.SafeUint64ToInt(v) // Safe conversion to prevent overflow (alert #416) case float64: timeout = int(v) default: @@ -221,9 +222,9 @@ func (c *Compiler) extractToolsStartupTimeout(tools map[string]any) (string, err case int64: timeout = int(v) case uint: - timeout = safeUintToInt(v) // Safe conversion to prevent overflow (alert #417) + timeout = typeutil.SafeUintToInt(v) // Safe conversion to prevent overflow (alert #417) case uint64: - timeout = safeUint64ToInt(v) // Safe conversion to prevent overflow (alert #415) + timeout = typeutil.SafeUint64ToInt(v) // Safe conversion to prevent overflow (alert #415) case float64: timeout = int(v) default: diff --git a/pkg/workflow/map_helpers.go b/pkg/workflow/map_helpers.go index 2bea48da0bf..c0133fbe114 100644 --- a/pkg/workflow/map_helpers.go +++ b/pkg/workflow/map_helpers.go @@ -1,14 +1,14 @@ -// This file provides generic map and type conversion utilities. +// This file provides generic map utilities. // // This file contains low-level helper functions for working with map[string]any -// structures and type conversions. These utilities are used throughout the workflow -// compilation process to safely parse and manipulate configuration data. +// structures. These utilities are used throughout the workflow compilation process +// to safely parse and manipulate configuration data. // // # Organization Rationale // // These functions are grouped in a helper file because they: // - Provide generic, reusable utilities (used by 10+ files) -// - Have no specific domain focus (work with any map/type data) +// - Have no specific domain focus (work with any map data) // - Are small, stable functions (< 50 lines each) // - Follow clear, single-purpose patterns // @@ -16,54 +16,24 @@ // // # Key Functions // -// Type Conversion (delegated to pkg/typeutil for general-purpose reuse): -// - parseIntValue() - Strictly parse numeric types to int; returns (value, ok). Use when -// the caller needs to distinguish "missing/invalid" from a zero value, or when string -// inputs are not expected (e.g. YAML config field parsing). Delegates to typeutil.ParseIntValue. -// - safeUint64ToInt() - Convert uint64 to int, returning 0 on overflow. Delegates to typeutil.SafeUint64ToInt. -// - safeUintToInt() - Convert uint to int, returning 0 on overflow. Delegates to typeutil.SafeUintToInt. -// - ConvertToInt() - Leniently convert any value (int/int64/float64/string) to int, returning 0 -// on failure. Use when the input may come from heterogeneous sources such as JSON metrics, -// log parsing, or user-provided strings where a zero default on failure is acceptable. -// Delegates to typeutil.ConvertToInt. -// - ConvertToFloat() - Safely convert any value (float64/int/int64/string) to float64. -// Delegates to typeutil.ConvertToFloat. -// // Map Operations: // - excludeMapKeys() - Create new map excluding specified keys // - sortedMapKeys() - Return sorted keys of a map[string]string // -// These utilities handle common type conversion and map manipulation patterns that -// occur frequently during YAML-to-struct parsing and configuration processing. - -package workflow - -import ( - "sort" - - "github.com/github/gh-aw/pkg/typeutil" -) - -// parseIntValue strictly parses numeric types to int, returning (value, true) on success -// and (0, false) for any unrecognized or non-numeric type. -// -// Use this when the caller needs to distinguish a missing/invalid value from a legitimate -// zero, or when string inputs are not expected (e.g. YAML config field parsing where the -// YAML library has already produced a typed numeric value). -// -// For lenient conversion that also handles string inputs and returns 0 on failure, use -// ConvertToInt instead. +// For type conversion utilities, use pkg/typeutil directly: +// - typeutil.ParseIntValue() - Strictly parse numeric types to int; returns (value, ok). +// - typeutil.SafeUint64ToInt() - Convert uint64 to int, returning 0 on overflow. +// - typeutil.SafeUintToInt() - Convert uint to int, returning 0 on overflow. +// - typeutil.ConvertToInt() - Leniently convert any value to int, returning 0 on failure. +// - typeutil.ConvertToFloat() - Safely convert any value to float64. +// - typeutil.ParseBool() - Extract a bool from map[string]any by key. // -// This is a package-private alias for typeutil.ParseIntValue. -func parseIntValue(value any) (int, bool) { return typeutil.ParseIntValue(value) } +// These utilities handle common map manipulation patterns that occur frequently +// during YAML-to-struct parsing and configuration processing. -// safeUint64ToInt safely converts uint64 to int, returning 0 if overflow would occur. -// This is a package-private alias for typeutil.SafeUint64ToInt. -func safeUint64ToInt(u uint64) int { return typeutil.SafeUint64ToInt(u) } +package workflow -// safeUintToInt safely converts uint to int, returning 0 if overflow would occur. -// This is a package-private alias for typeutil.SafeUintToInt. -func safeUintToInt(u uint) int { return typeutil.SafeUintToInt(u) } +import "sort" // excludeMapKeys creates a new map excluding the specified keys func excludeMapKeys(original map[string]any, excludeKeys ...string) map[string]any { @@ -91,26 +61,3 @@ func sortedMapKeys(m map[string]string) []string { sort.Strings(keys) return keys } - -// ConvertToInt leniently converts any value to int, returning 0 on failure. -// -// Unlike parseIntValue, this function also handles string inputs via strconv.Atoi, -// making it suitable for heterogeneous sources such as JSON metrics, log-parsed data, -// or user-provided configuration where a zero default on failure is acceptable and -// the caller does not need to distinguish "invalid" from a genuine zero. -// -// For strict numeric-only parsing where the caller must distinguish missing/invalid -// values from zero, use parseIntValue instead. -// -// This is a workflow-package alias for typeutil.ConvertToInt. For new code outside -// this package, prefer using typeutil.ConvertToInt directly. -func ConvertToInt(val any) int { return typeutil.ConvertToInt(val) } - -// ConvertToFloat safely converts any value to float64, returning 0 on failure. -// -// Supported input types: float64, int, int64, and string (parsed via strconv.ParseFloat). -// Returns 0 for any other type or for strings that cannot be parsed as a float. -// -// This is a workflow-package alias for typeutil.ConvertToFloat. For new code outside -// this package, prefer using typeutil.ConvertToFloat directly. -func ConvertToFloat(val any) float64 { return typeutil.ConvertToFloat(val) } diff --git a/pkg/workflow/map_helpers_test.go b/pkg/workflow/map_helpers_test.go index eac11182e9d..cf69938bd45 100644 --- a/pkg/workflow/map_helpers_test.go +++ b/pkg/workflow/map_helpers_test.go @@ -4,6 +4,8 @@ package workflow import ( "testing" + + "github.com/github/gh-aw/pkg/typeutil" ) func TestParseIntValue(t *testing.T) { @@ -59,12 +61,12 @@ func TestParseIntValue(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, ok := parseIntValue(tt.value) + result, ok := typeutil.ParseIntValue(tt.value) if ok != tt.ok { - t.Errorf("parseIntValue() ok = %v, want %v", ok, tt.ok) + t.Errorf("typeutil.ParseIntValue() ok = %v, want %v", ok, tt.ok) } if result != tt.expected { - t.Errorf("parseIntValue() result = %v, want %v", result, tt.expected) + t.Errorf("typeutil.ParseIntValue() result = %v, want %v", result, tt.expected) } }) } @@ -130,12 +132,12 @@ func TestParseIntValueTruncation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, ok := parseIntValue(tt.value) + result, ok := typeutil.ParseIntValue(tt.value) if !ok { - t.Errorf("parseIntValue() should return ok=true for float64") + t.Errorf("typeutil.ParseIntValue() should return ok=true for float64") } if result != tt.expected { - t.Errorf("parseIntValue(%v) = %v, want %v", tt.value, result, tt.expected) + t.Errorf("typeutil.ParseIntValue(%v) = %v, want %v", tt.value, result, tt.expected) } // Note: We can't directly test if warning was logged, but we verify the conversion is correct }) @@ -287,9 +289,9 @@ func TestSafeUint64ToInt(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := safeUint64ToInt(tt.value) + result := typeutil.SafeUint64ToInt(tt.value) if result != tt.expected { - t.Errorf("safeUint64ToInt(%d) = %d, want %d", tt.value, result, tt.expected) + t.Errorf("typeutil.SafeUint64ToInt(%d) = %d, want %d", tt.value, result, tt.expected) } }) } diff --git a/pkg/workflow/mcp_scripts_parser.go b/pkg/workflow/mcp_scripts_parser.go index 12513eb93cc..f26e0fc29e4 100644 --- a/pkg/workflow/mcp_scripts_parser.go +++ b/pkg/workflow/mcp_scripts_parser.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" ) var mcpScriptsLog = logger.New("workflow:mcp_scripts") @@ -175,7 +176,7 @@ func parseMCPScriptsMap(mcpScriptsMap map[string]any) (*MCPScriptsConfig, bool) case int: toolConfig.Timeout = t case uint64: - toolConfig.Timeout = safeUint64ToInt(t) // Safe conversion to prevent overflow (alert #414) + toolConfig.Timeout = typeutil.SafeUint64ToInt(t) // Safe conversion to prevent overflow (alert #414) case float64: toolConfig.Timeout = int(t) case string: @@ -346,7 +347,7 @@ func (c *Compiler) mergeMCPScripts(main *MCPScriptsConfig, importedConfigs []str case int: toolConfig.Timeout = t case uint64: - toolConfig.Timeout = safeUint64ToInt(t) // Safe conversion to prevent overflow (alert #413) + toolConfig.Timeout = typeutil.SafeUint64ToInt(t) // Safe conversion to prevent overflow (alert #413) case float64: toolConfig.Timeout = int(t) case string: diff --git a/pkg/workflow/metrics.go b/pkg/workflow/metrics.go index 78074e8b17a..6ab9baae06d 100644 --- a/pkg/workflow/metrics.go +++ b/pkg/workflow/metrics.go @@ -7,6 +7,7 @@ import ( "time" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" ) var metricsLog = logger.New("workflow:metrics") @@ -81,8 +82,8 @@ func ExtractJSONMetrics(line string, verbose bool) LogMetrics { // ExtractJSONTokenUsage extracts token usage from JSON data func ExtractJSONTokenUsage(data map[string]any) int { // Prefer explicit input+output sums at the top-level - inputTop := ConvertToInt(data["input_tokens"]) - outputTop := ConvertToInt(data["output_tokens"]) + inputTop := typeutil.ConvertToInt(data["input_tokens"]) + outputTop := typeutil.ConvertToInt(data["output_tokens"]) if inputTop > 0 || outputTop > 0 { totalTokens := inputTop + outputTop if metricsLog.Enabled() { @@ -95,7 +96,7 @@ func ExtractJSONTokenUsage(data map[string]any) int { tokenFields := []string{"tokens", "token_count", "total_tokens"} for _, field := range tokenFields { if val, exists := data[field]; exists { - if tokens := ConvertToInt(val); tokens > 0 { + if tokens := typeutil.ConvertToInt(val); tokens > 0 { return tokens } } @@ -105,18 +106,18 @@ func ExtractJSONTokenUsage(data map[string]any) int { if usage, exists := data["usage"]; exists { if usageMap, ok := usage.(map[string]any); ok { // Claude format: {"usage": {"input_tokens": 10, "output_tokens": 5, "cache_creation_input_tokens": 100, "cache_read_input_tokens": 200}} - inputTokens := ConvertToInt(usageMap["input_tokens"]) - outputTokens := ConvertToInt(usageMap["output_tokens"]) - cacheCreationTokens := ConvertToInt(usageMap["cache_creation_input_tokens"]) - cacheReadTokens := ConvertToInt(usageMap["cache_read_input_tokens"]) + inputTokens := typeutil.ConvertToInt(usageMap["input_tokens"]) + outputTokens := typeutil.ConvertToInt(usageMap["output_tokens"]) + cacheCreationTokens := typeutil.ConvertToInt(usageMap["cache_creation_input_tokens"]) + cacheReadTokens := typeutil.ConvertToInt(usageMap["cache_read_input_tokens"]) // OpenAI format: {"usage": {"prompt_tokens": 100, "completion_tokens": 50}} // If Claude fields are not present, try OpenAI fields if inputTokens == 0 { - inputTokens = ConvertToInt(usageMap["prompt_tokens"]) + inputTokens = typeutil.ConvertToInt(usageMap["prompt_tokens"]) } if outputTokens == 0 { - outputTokens = ConvertToInt(usageMap["completion_tokens"]) + outputTokens = typeutil.ConvertToInt(usageMap["completion_tokens"]) } totalTokens := inputTokens + outputTokens + cacheCreationTokens + cacheReadTokens @@ -127,7 +128,7 @@ func ExtractJSONTokenUsage(data map[string]any) int { // Generic token count fields inside usage for _, field := range tokenFields { if val, exists := usageMap[field]; exists { - if tokens := ConvertToInt(val); tokens > 0 { + if tokens := typeutil.ConvertToInt(val); tokens > 0 { return tokens } } @@ -140,8 +141,8 @@ func ExtractJSONTokenUsage(data map[string]any) int { if deltaMap, ok := delta.(map[string]any); ok { if usage, exists := deltaMap["usage"]; exists { if usageMap, ok := usage.(map[string]any); ok { - inputTokens := ConvertToInt(usageMap["input_tokens"]) - outputTokens := ConvertToInt(usageMap["output_tokens"]) + inputTokens := typeutil.ConvertToInt(usageMap["input_tokens"]) + outputTokens := typeutil.ConvertToInt(usageMap["output_tokens"]) if inputTokens > 0 || outputTokens > 0 { return inputTokens + outputTokens } @@ -160,7 +161,7 @@ func ExtractJSONCost(data map[string]any) float64 { // Prefer explicit total_cost_usd at top-level if val, exists := data["total_cost_usd"]; exists { - if cost := ConvertToFloat(val); cost > 0 { + if cost := typeutil.ConvertToFloat(val); cost > 0 { if metricsLog.Enabled() { metricsLog.Printf("Cost extracted: value=%.6f", cost) } @@ -170,7 +171,7 @@ func ExtractJSONCost(data map[string]any) float64 { for _, field := range costFields { if val, exists := data[field]; exists { - if cost := ConvertToFloat(val); cost > 0 { + if cost := typeutil.ConvertToFloat(val); cost > 0 { return cost } } @@ -181,7 +182,7 @@ func ExtractJSONCost(data map[string]any) float64 { if billingMap, ok := billing.(map[string]any); ok { for _, field := range costFields { if val, exists := billingMap[field]; exists { - if cost := ConvertToFloat(val); cost > 0 { + if cost := typeutil.ConvertToFloat(val); cost > 0 { return cost } } diff --git a/pkg/workflow/metrics_test.go b/pkg/workflow/metrics_test.go index 87b06fd138a..da16bb6619f 100644 --- a/pkg/workflow/metrics_test.go +++ b/pkg/workflow/metrics_test.go @@ -5,6 +5,8 @@ package workflow import ( "encoding/json" "testing" + + "github.com/github/gh-aw/pkg/typeutil" ) func TestExtractJSONMetrics(t *testing.T) { @@ -428,9 +430,9 @@ func TestConvertToInt(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := ConvertToInt(tt.val) + result := typeutil.ConvertToInt(tt.val) if result != tt.expected { - t.Errorf("ConvertToInt(%v) = %d, want %d", tt.val, result, tt.expected) + t.Errorf("typeutil.ConvertToInt(%v) = %d, want %d", tt.val, result, tt.expected) } }) } @@ -506,9 +508,9 @@ func TestConvertToFloat(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := ConvertToFloat(tt.val) + result := typeutil.ConvertToFloat(tt.val) if result != tt.expected { - t.Errorf("ConvertToFloat(%v) = %f, want %f", tt.val, result, tt.expected) + t.Errorf("typeutil.ConvertToFloat(%v) = %f, want %f", tt.val, result, tt.expected) } }) } @@ -806,7 +808,7 @@ func TestFinalizeToolCallsAndSequence(t *testing.T) { } } -// TestConvertToIntTruncation tests float truncation scenarios in ConvertToInt +// TestConvertToIntTruncation tests float truncation scenarios in typeutil.ConvertToInt func TestConvertToIntTruncation(t *testing.T) { tests := []struct { name string @@ -878,9 +880,9 @@ func TestConvertToIntTruncation(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := ConvertToInt(tt.val) + result := typeutil.ConvertToInt(tt.val) if result != tt.expected { - t.Errorf("ConvertToInt(%v) = %v, want %v", tt.val, result, tt.expected) + t.Errorf("typeutil.ConvertToInt(%v) = %v, want %v", tt.val, result, tt.expected) } // Note: We can't directly test if warning was logged, but we verify the conversion is correct }) diff --git a/pkg/workflow/publish_artifacts.go b/pkg/workflow/publish_artifacts.go index 215bed2785d..165348c98df 100644 --- a/pkg/workflow/publish_artifacts.go +++ b/pkg/workflow/publish_artifacts.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" ) var publishArtifactsLog = logger.New("workflow:publish_artifacts") @@ -93,28 +94,28 @@ func (c *Compiler) parseUploadArtifactConfig(outputMap map[string]any) *UploadAr // Parse max-uploads. if maxUploads, exists := configMap["max-uploads"]; exists { - if v, ok := parseIntValue(maxUploads); ok && v > 0 { + if v, ok := typeutil.ParseIntValue(maxUploads); ok && v > 0 { config.MaxUploads = v } } // Parse default-retention-days. if retDays, exists := configMap["default-retention-days"]; exists { - if v, ok := parseIntValue(retDays); ok && v > 0 { + if v, ok := typeutil.ParseIntValue(retDays); ok && v > 0 { config.DefaultRetentionDays = v } } // Parse max-retention-days. if maxRetDays, exists := configMap["max-retention-days"]; exists { - if v, ok := parseIntValue(maxRetDays); ok && v > 0 { + if v, ok := typeutil.ParseIntValue(maxRetDays); ok && v > 0 { config.MaxRetentionDays = v } } // Parse max-size-bytes. if maxBytes, exists := configMap["max-size-bytes"]; exists { - if v, ok := parseIntValue(maxBytes); ok && v > 0 { + if v, ok := typeutil.ParseIntValue(maxBytes); ok && v > 0 { config.MaxSizeBytes = int64(v) } } diff --git a/pkg/workflow/publish_assets.go b/pkg/workflow/publish_assets.go index 3d429ca46e2..ad4dd0e6b39 100644 --- a/pkg/workflow/publish_assets.go +++ b/pkg/workflow/publish_assets.go @@ -7,6 +7,7 @@ import ( "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" ) var publishAssetsLog = logger.New("workflow:publish_assets") @@ -49,7 +50,7 @@ func (c *Compiler) parseUploadAssetConfig(outputMap map[string]any) *UploadAsset // Parse max-size if maxSize, exists := configMap["max-size"]; exists { - if maxSizeInt, ok := parseIntValue(maxSize); ok && maxSizeInt > 0 { + if maxSizeInt, ok := typeutil.ParseIntValue(maxSize); ok && maxSizeInt > 0 { config.MaxSizeKB = maxSizeInt } } diff --git a/pkg/workflow/safe_outputs_config.go b/pkg/workflow/safe_outputs_config.go index 047c3d3eed9..f96ee96e87f 100644 --- a/pkg/workflow/safe_outputs_config.go +++ b/pkg/workflow/safe_outputs_config.go @@ -4,6 +4,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/typeutil" ) var safeOutputsConfigLog = logger.New("workflow:safe_outputs_config") @@ -626,8 +627,8 @@ func (c *Compiler) parseBaseSafeOutputConfig(configMap map[string]any, config *B config.Max = &v } default: - // Convert integer/float64/etc to string via parseIntValue - if maxInt, ok := parseIntValue(max); ok { + // Convert integer/float64/etc to string via typeutil.ParseIntValue + if maxInt, ok := typeutil.ParseIntValue(max); ok { safeOutputsConfigLog.Printf("Parsed max as integer: %d", maxInt) s := defaultIntStr(maxInt) config.Max = s diff --git a/pkg/workflow/type_conversion_test.go b/pkg/workflow/type_conversion_test.go index c5e05a8af6c..22ed3cfba22 100644 --- a/pkg/workflow/type_conversion_test.go +++ b/pkg/workflow/type_conversion_test.go @@ -5,9 +5,11 @@ package workflow import ( "math" "testing" + + "github.com/github/gh-aw/pkg/typeutil" ) -// TestConvertToIntEdgeCases provides comprehensive coverage for ConvertToInt edge cases +// TestConvertToIntEdgeCases provides comprehensive coverage for typeutil.ConvertToInt edge cases func TestConvertToIntEdgeCases(t *testing.T) { tests := []struct { name string @@ -83,9 +85,9 @@ func TestConvertToIntEdgeCases(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := ConvertToInt(tt.input) + result := typeutil.ConvertToInt(tt.input) if result != tt.expected { - t.Errorf("ConvertToInt(%v) = %d; want %d", tt.input, result, tt.expected) + t.Errorf("typeutil.ConvertToInt(%v) = %d; want %d", tt.input, result, tt.expected) } }) } @@ -159,17 +161,17 @@ func TestConvertToIntOverflow(t *testing.T) { // Test should not panic defer func() { if r := recover(); r != nil { - t.Errorf("ConvertToInt panicked with %v: %v", tt.input, r) + t.Errorf("typeutil.ConvertToInt panicked with %v: %v", tt.input, r) } }() - result := ConvertToInt(tt.input) + result := typeutil.ConvertToInt(tt.input) tt.validate(t, result) }) } } -// TestParseIntValueEdgeCases provides comprehensive coverage for parseIntValue edge cases +// TestParseIntValueEdgeCases provides comprehensive coverage for typeutil.ParseIntValue edge cases func TestParseIntValueEdgeCases(t *testing.T) { tests := []struct { name string @@ -198,7 +200,7 @@ func TestParseIntValueEdgeCases(t *testing.T) { {"float64 near zero", 0.9, 0, true}, {"float64 zero", 0.0, 0, true}, - // Unsupported types - parseIntValue does NOT support strings + // Unsupported types - typeutil.ParseIntValue does NOT support strings {"string value", "42", 0, false}, {"nil value", nil, 0, false}, {"bool value", true, 0, false}, @@ -208,12 +210,12 @@ func TestParseIntValueEdgeCases(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, ok := parseIntValue(tt.input) + result, ok := typeutil.ParseIntValue(tt.input) if ok != tt.ok { - t.Errorf("parseIntValue(%v) ok = %v, want %v", tt.input, ok, tt.ok) + t.Errorf("typeutil.ParseIntValue(%v) ok = %v, want %v", tt.input, ok, tt.ok) } if result != tt.expected { - t.Errorf("parseIntValue(%v) = %d, want %d", tt.input, result, tt.expected) + t.Errorf("typeutil.ParseIntValue(%v) = %d, want %d", tt.input, result, tt.expected) } }) } @@ -274,17 +276,17 @@ func TestParseIntValueOverflow(t *testing.T) { // Test should not panic defer func() { if r := recover(); r != nil { - t.Errorf("parseIntValue panicked with %v: %v", tt.input, r) + t.Errorf("typeutil.ParseIntValue panicked with %v: %v", tt.input, r) } }() - result, ok := parseIntValue(tt.input) + result, ok := typeutil.ParseIntValue(tt.input) tt.validate(t, result, ok) }) } } -// TestConvertToFloatEdgeCases provides comprehensive coverage for ConvertToFloat edge cases +// TestConvertToFloatEdgeCases provides comprehensive coverage for typeutil.ConvertToFloat edge cases func TestConvertToFloatEdgeCases(t *testing.T) { tests := []struct { name string @@ -336,9 +338,9 @@ func TestConvertToFloatEdgeCases(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := ConvertToFloat(tt.input) + result := typeutil.ConvertToFloat(tt.input) if result != tt.expected { - t.Errorf("ConvertToFloat(%v) = %f; want %f", tt.input, result, tt.expected) + t.Errorf("typeutil.ConvertToFloat(%v) = %f; want %f", tt.input, result, tt.expected) } }) } @@ -430,11 +432,11 @@ func TestConvertToFloatSpecialValues(t *testing.T) { // Test should not panic defer func() { if r := recover(); r != nil { - t.Errorf("ConvertToFloat panicked with %v: %v", tt.input, r) + t.Errorf("typeutil.ConvertToFloat panicked with %v: %v", tt.input, r) } }() - result := ConvertToFloat(tt.input) + result := typeutil.ConvertToFloat(tt.input) tt.validate(t, result) }) } @@ -444,7 +446,7 @@ func TestConvertToFloatSpecialValues(t *testing.T) { func TestPolymorphicTypeHandling(t *testing.T) { // This tests the common case where YAML/JSON can provide values as different types - t.Run("ConvertToInt with polymorphic inputs", func(t *testing.T) { + t.Run("typeutil.ConvertToInt with polymorphic inputs", func(t *testing.T) { tests := []struct { name string input any @@ -468,15 +470,15 @@ func TestPolymorphicTypeHandling(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := ConvertToInt(tt.input) + result := typeutil.ConvertToInt(tt.input) if result != tt.expected { - t.Errorf("ConvertToInt(%v) = %d; want %d", tt.input, result, tt.expected) + t.Errorf("typeutil.ConvertToInt(%v) = %d; want %d", tt.input, result, tt.expected) } }) } }) - t.Run("ConvertToFloat with polymorphic inputs", func(t *testing.T) { + t.Run("typeutil.ConvertToFloat with polymorphic inputs", func(t *testing.T) { tests := []struct { name string input any @@ -497,9 +499,9 @@ func TestPolymorphicTypeHandling(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := ConvertToFloat(tt.input) + result := typeutil.ConvertToFloat(tt.input) if result != tt.expected { - t.Errorf("ConvertToFloat(%v) = %f; want %f", tt.input, result, tt.expected) + t.Errorf("typeutil.ConvertToFloat(%v) = %f; want %f", tt.input, result, tt.expected) } }) } @@ -538,41 +540,41 @@ func TestTypeSafetyNoPanics(t *testing.T) { make(chan int), } - t.Run("ConvertToInt no panics", func(t *testing.T) { + t.Run("typeutil.ConvertToInt no panics", func(t *testing.T) { for i, input := range inputs { func() { defer func() { if r := recover(); r != nil { - t.Errorf("ConvertToInt panicked on input %d (%T): %v", i, input, r) + t.Errorf("typeutil.ConvertToInt panicked on input %d (%T): %v", i, input, r) } }() - _ = ConvertToInt(input) + _ = typeutil.ConvertToInt(input) }() } }) - t.Run("ConvertToFloat no panics", func(t *testing.T) { + t.Run("typeutil.ConvertToFloat no panics", func(t *testing.T) { for i, input := range inputs { func() { defer func() { if r := recover(); r != nil { - t.Errorf("ConvertToFloat panicked on input %d (%T): %v", i, input, r) + t.Errorf("typeutil.ConvertToFloat panicked on input %d (%T): %v", i, input, r) } }() - _ = ConvertToFloat(input) + _ = typeutil.ConvertToFloat(input) }() } }) - t.Run("parseIntValue no panics", func(t *testing.T) { + t.Run("typeutil.ParseIntValue no panics", func(t *testing.T) { for i, input := range inputs { func() { defer func() { if r := recover(); r != nil { - t.Errorf("parseIntValue panicked on input %d (%T): %v", i, input, r) + t.Errorf("typeutil.ParseIntValue panicked on input %d (%T): %v", i, input, r) } }() - _, _ = parseIntValue(input) + _, _ = typeutil.ParseIntValue(input) }() } }) @@ -594,32 +596,32 @@ func TestZeroValueReturns(t *testing.T) { "", } - t.Run("ConvertToInt returns zero for invalid inputs", func(t *testing.T) { + t.Run("typeutil.ConvertToInt returns zero for invalid inputs", func(t *testing.T) { for _, input := range invalidInputs { - result := ConvertToInt(input) + result := typeutil.ConvertToInt(input) if result != 0 { - t.Errorf("ConvertToInt(%v) = %d; want 0", input, result) + t.Errorf("typeutil.ConvertToInt(%v) = %d; want 0", input, result) } } }) - t.Run("ConvertToFloat returns zero for invalid inputs", func(t *testing.T) { + t.Run("typeutil.ConvertToFloat returns zero for invalid inputs", func(t *testing.T) { for _, input := range invalidInputs { - result := ConvertToFloat(input) + result := typeutil.ConvertToFloat(input) if result != 0.0 { - t.Errorf("ConvertToFloat(%v) = %f; want 0.0", input, result) + t.Errorf("typeutil.ConvertToFloat(%v) = %f; want 0.0", input, result) } } }) - t.Run("parseIntValue returns zero and false for invalid inputs", func(t *testing.T) { + t.Run("typeutil.ParseIntValue returns zero and false for invalid inputs", func(t *testing.T) { for _, input := range invalidInputs { - result, ok := parseIntValue(input) + result, ok := typeutil.ParseIntValue(input) if ok { - t.Errorf("parseIntValue(%v) ok = true; want false", input) + t.Errorf("typeutil.ParseIntValue(%v) ok = true; want false", input) } if result != 0 { - t.Errorf("parseIntValue(%v) = %d; want 0", input, result) + t.Errorf("typeutil.ParseIntValue(%v) = %d; want 0", input, result) } } }) @@ -656,19 +658,19 @@ func TestFloatToIntTruncationBehavior(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Test ConvertToInt - result := ConvertToInt(tt.input) + // Test typeutil.ConvertToInt + result := typeutil.ConvertToInt(tt.input) if result != tt.expected { - t.Errorf("ConvertToInt(%v) = %d; want %d", tt.input, result, tt.expected) + t.Errorf("typeutil.ConvertToInt(%v) = %d; want %d", tt.input, result, tt.expected) } - // Test parseIntValue for consistency - parsed, ok := parseIntValue(tt.input) + // Test typeutil.ParseIntValue for consistency + parsed, ok := typeutil.ParseIntValue(tt.input) if !ok { - t.Errorf("parseIntValue(%v) ok = false; want true", tt.input) + t.Errorf("typeutil.ParseIntValue(%v) ok = false; want true", tt.input) } if parsed != tt.expected { - t.Errorf("parseIntValue(%v) = %d; want %d", tt.input, parsed, tt.expected) + t.Errorf("typeutil.ParseIntValue(%v) = %d; want %d", tt.input, parsed, tt.expected) } }) }