diff --git a/docs/adr/45858-consolidate-duplicated-parsing-helpers.md b/docs/adr/45858-consolidate-duplicated-parsing-helpers.md new file mode 100644 index 00000000000..1f05e0dbb90 --- /dev/null +++ b/docs/adr/45858-consolidate-duplicated-parsing-helpers.md @@ -0,0 +1,50 @@ +# ADR-45858: Consolidate Duplicated Parsing Helpers into Canonical Shared Functions + +**Date**: 2026-07-16 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +The `pkg/workflow` and `pkg/cli` packages contained multiple near-identical inline implementations of the same low-level parsing operations: positive-integer parsing from frontmatter values, integer-or-expression parsing for GitHub Actions template strings (`${{ ... }}`), workflow-ID normalization from file paths, and timeline row construction. Because each call site copied the logic independently, a bug fix or behavioral change (e.g., accepting an additional expression format) required updates in N places. The duplication also introduced subtle inconsistencies: some sites logged field-specific error messages, others logged generic ones, and some omitted logging entirely. + +### Decision + +We will extract each repeated parsing operation into a single, parameterized, unexported helper within the package that owns the concept: +- `parsePositiveIntValue(raw any, fieldName string) int` and `parseIntOrExpressionValue(raw any, minValue int, fieldName string) string` in `pkg/workflow` +- `isExpression(s string) bool` as a shared predicate replacing all `strings.HasPrefix(…, "${{") && strings.HasSuffix(…, "}}")` checks in `pkg/workflow` +- `normalizeWorkflowID(filename string) string` as the single call site for `.md` basename stripping in `pkg/cli` +- `renderMessageContentTimelineRow(kind TimelineEventKind, evt UnifiedTimelineEvent) []string` replacing duplicated timeline row construction in `pkg/cli` + +All existing public-facing functions become thin wrappers that delegate to the shared helper. + +### Alternatives Considered + +#### Alternative 1: Maintain Inline Copies at Each Call Site (Status Quo) + +Keep the parsing logic repeated at each of the five or more call sites. This requires no refactoring and avoids any indirection. We rejected it because a logic change (e.g., updating expression detection) requires touching every site individually, and omissions create behavioral inconsistencies that are hard to catch in code review. + +#### Alternative 2: Introduce a Dedicated Utility Package (e.g., `pkg/parseutil`) + +Move the helpers to a new package so they are importable across `pkg/workflow`, `pkg/cli`, and any future packages. This would allow cross-package reuse without creating import cycles. We rejected it for this change because all current call sites are within their own package, the helpers are implementation details that should not be part of the public API surface, and the extra package indirection adds maintenance overhead that is not yet justified. + +### Consequences + +#### Positive +- Parsing behavior is now consistent at all call sites; a fix to `isExpression` or `parseIntOrExpressionValue` propagates automatically to every consumer. +- Error log messages are uniformly parameterized with the field name, making diagnostics more actionable. +- New tests for the shared helpers provide regression coverage that did not previously exist for many of the duplicated call sites. + +#### Negative +- Stack traces for parsing failures are now one frame deeper; developers reading a crash log must look through the wrapper to find the shared helper. +- The helpers are unexported, so they cannot be reused across package boundaries without further refactoring (e.g., if `pkg/cli` ever needs `isExpression`, it must either re-implement it or create an import dependency). + +#### Neutral +- Existing public-facing parser functions (`parseMaxRunsValue`, `parseMaxTurnsValue`, etc.) retain their signatures — callers are unaffected. +- The `normalizeWorkflowID` and `renderMessageContentTimelineRow` helpers follow the same package-local pattern; their behavior is behavior-preserving and the change is transparent to external packages. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/experiments_command.go b/pkg/cli/experiments_command.go index 55ae47a1d49..c3474f02243 100644 --- a/pkg/cli/experiments_command.go +++ b/pkg/cli/experiments_command.go @@ -460,10 +460,15 @@ func findRemoteWorkflowFilenameForExperiment(repoOverride, experimentName string // matchWorkflowFilenameByExperiment returns the basename (without .md) of the first file in // filenames whose sanitized name matches experimentName. Returns "" when no match is found. // Logs a warning when more than one file maps to the same sanitized name. +// +// Note: normalizeWorkflowID calls filepath.Base internally, so any path prefix in filenames +// is stripped before matching. Callers that supply bare filenames (e.g. "my-flow.md") are +// unaffected; callers supplying full paths (e.g. ".github/workflows/my-flow.md") will have +// the directory component removed — only the basename is returned and compared. func matchWorkflowFilenameByExperiment(filenames []string, experimentName string) string { var matches []string for _, filename := range filenames { - base := strings.TrimSuffix(filename, ".md") + base := normalizeWorkflowID(filename) if workflow.SanitizeWorkflowIDForCacheKey(base) == experimentName { matches = append(matches, base) } @@ -486,7 +491,7 @@ func findWorkflowFileForExperiment(experimentName string) string { return "" } for _, f := range mdFiles { - base := strings.TrimSuffix(filepath.Base(f), ".md") + base := normalizeWorkflowID(f) if workflow.SanitizeWorkflowIDForCacheKey(base) == experimentName { return f } diff --git a/pkg/cli/gateway_logs_timeline_render.go b/pkg/cli/gateway_logs_timeline_render.go index 074904d4d70..0dc749745fe 100644 --- a/pkg/cli/gateway_logs_timeline_render.go +++ b/pkg/cli/gateway_logs_timeline_render.go @@ -323,11 +323,15 @@ func renderAgentToolDoneRow(evt UnifiedTimelineEvent) []string { // // Detail shows a truncated preview of the message content. Status is left empty. func renderAgentAssistantMessageRow(evt UnifiedTimelineEvent) []string { + return renderMessageContentTimelineRow(TimelineKindAssistantMessage, evt) +} + +func renderMessageContentTimelineRow(kind TimelineEventKind, evt UnifiedTimelineEvent) []string { ts := formatTimelineTime(evt) src := timelineSourceLabel(evt.Source) - kind := timelineEventIcon(TimelineKindAssistantMessage) + " " + timelineEventKindLabel(TimelineKindAssistantMessage) + kindLabel := timelineEventIcon(kind) + " " + timelineEventKindLabel(kind) detail := stringutil.Truncate(evt.MessageContent, 48) - return []string{ts, src, kind, detail, ""} + return []string{ts, src, kindLabel, detail, ""} } // renderAgentReasoningRow renders a TimelineKindReasoning event as a table row. @@ -336,11 +340,7 @@ func renderAgentAssistantMessageRow(evt UnifiedTimelineEvent) []string { // // Detail shows a truncated preview of the reasoning content. Status is left empty. func renderAgentReasoningRow(evt UnifiedTimelineEvent) []string { - ts := formatTimelineTime(evt) - src := timelineSourceLabel(evt.Source) - kind := timelineEventIcon(TimelineKindReasoning) + " " + timelineEventKindLabel(TimelineKindReasoning) - detail := stringutil.Truncate(evt.MessageContent, 48) - return []string{ts, src, kind, detail, ""} + return renderMessageContentTimelineRow(TimelineKindReasoning, evt) } // renderSteeringRow renders a TimelineKindSteering event as a table row. diff --git a/pkg/cli/gateway_logs_timeline_test.go b/pkg/cli/gateway_logs_timeline_test.go index c4f64ed7aec..d3fc7370295 100644 --- a/pkg/cli/gateway_logs_timeline_test.go +++ b/pkg/cli/gateway_logs_timeline_test.go @@ -567,6 +567,38 @@ func TestRenderAgentToolDoneRow_StatusFromSuccessFlag(t *testing.T) { } } +func TestRenderAgentAssistantMessageRow(t *testing.T) { + evt := UnifiedTimelineEvent{ + Time: time.Date(2024, 1, 15, 10, 0, 3, 0, time.UTC), + Source: TimelineSourceAgent, + Kind: TimelineKindAssistantMessage, + MessageContent: "assistant response", + } + row := renderAgentAssistantMessageRow(evt) + if !strings.Contains(row[2], string(TimelineKindAssistantMessage)) { + t.Errorf("Kind = %q; want assistant_message label", row[2]) + } + if row[3] != "assistant response" { + t.Errorf("Detail = %q; want assistant response", row[3]) + } +} + +func TestRenderAgentReasoningRow(t *testing.T) { + evt := UnifiedTimelineEvent{ + Time: time.Date(2024, 1, 15, 10, 0, 4, 0, time.UTC), + Source: TimelineSourceAgent, + Kind: TimelineKindReasoning, + MessageContent: "reasoning summary", + } + row := renderAgentReasoningRow(evt) + if !strings.Contains(row[2], string(TimelineKindReasoning)) { + t.Errorf("Kind = %q; want reasoning label", row[2]) + } + if row[3] != "reasoning summary" { + t.Errorf("Detail = %q; want reasoning summary", row[3]) + } +} + // ─── timelineEventIcon / timelineEventKindLabel / timelineSourceLabel ───────── func TestTimelineEventIcon_AllKinds(t *testing.T) { diff --git a/pkg/cli/spec.go b/pkg/cli/spec.go index d136127f524..db77d22c5c6 100644 --- a/pkg/cli/spec.go +++ b/pkg/cli/spec.go @@ -417,7 +417,7 @@ func parseWorkflowSpec(spec string) (*WorkflowSpec, error) { Version: version, }, WorkflowPath: workflowPath, - WorkflowName: strings.TrimSuffix(filepath.Base(workflowPath), ".md"), + WorkflowName: normalizeWorkflowID(workflowPath), Host: explicitHost, }, nil } @@ -439,7 +439,7 @@ func parseLocalWorkflowSpec(spec string) (*WorkflowSpec, error) { Version: "", // Local workflows have no version }, WorkflowPath: spec, // Keep the "./" prefix in WorkflowPath - WorkflowName: strings.TrimSuffix(filepath.Base(spec), ".md"), + WorkflowName: normalizeWorkflowID(spec), }, nil } diff --git a/pkg/cli/workflows.go b/pkg/cli/workflows.go index c25c1716f50..52c79f4cd2d 100644 --- a/pkg/cli/workflows.go +++ b/pkg/cli/workflows.go @@ -180,7 +180,7 @@ func getWorkflowStatus(workflowIdOrName string, repoOverride string, verbose boo workflowsLog.Printf("Getting workflow status: workflow=%s", workflowIdOrName) // Extract workflow name for lookup - filename := strings.TrimSuffix(filepath.Base(workflowIdOrName), ".md") + filename := normalizeWorkflowID(workflowIdOrName) // Get all GitHub workflows githubWorkflows, err := fetchGitHubWorkflows(repoOverride, verbose) @@ -243,7 +243,7 @@ func getAvailableWorkflowNames() []string { } return sliceutil.Map(mdFiles, func(file string) string { - return strings.TrimSuffix(filepath.Base(file), ".md") + return normalizeWorkflowID(file) }) } @@ -256,7 +256,7 @@ func suggestWorkflowNames(target string) []string { } // Normalize target: strip .md extension and get basename if it's a path - normalizedTarget := strings.TrimSuffix(filepath.Base(target), ".md") + normalizedTarget := normalizeWorkflowID(target) workflowsLog.Printf("Suggesting workflow names for %q (available: %d)", normalizedTarget, len(availableNames)) // Use the existing FindClosestMatches function from parser package diff --git a/pkg/workflow/engine.go b/pkg/workflow/engine.go index cf2e1b41428..e337de5f3b0 100644 --- a/pkg/workflow/engine.go +++ b/pkg/workflow/engine.go @@ -428,7 +428,7 @@ func (c *Compiler) ExtractEngineConfig(frontmatter map[string]any) (string, *Eng config.HarnessScript = use } if v, ok := h["max-retries"]; ok { - config.HarnessMaxRetries = parseNonNegativeIntOrExpressionValue(v) + config.HarnessMaxRetries = parseHarnessMaxRetriesValue(v) } if v, ok := h["initial-delay-ms"]; ok { config.HarnessInitialDelayMs = parseMaxTurnsValue(v) diff --git a/pkg/workflow/engine_config_parser.go b/pkg/workflow/engine_config_parser.go index bd56b53d244..027aa9ab82d 100644 --- a/pkg/workflow/engine_config_parser.go +++ b/pkg/workflow/engine_config_parser.go @@ -27,62 +27,45 @@ func parseMaxAICreditsValue(raw any) int64 { // parseMaxRunsValue parses max-runs from either integer or numeric-string // frontmatter values. func parseMaxRunsValue(raw any) int { - if val, ok := typeutil.ParseIntValue(raw); ok && val > 0 { - return val - } - if rawStr, ok := raw.(string); ok { - if parsed, err := strconv.Atoi(rawStr); err == nil && parsed > 0 { - return parsed - } - engineLog.Printf("Ignoring invalid max-runs value: %q", rawStr) - } - return 0 + return parsePositiveIntValue(raw, "max-runs") } // parseMaxTurnCacheMissesValue parses max-turn-cache-misses from either integer or // numeric-string frontmatter values. func parseMaxTurnCacheMissesValue(raw any) int { - if val, ok := typeutil.ParseIntValue(raw); ok && val > 0 { - return val - } - if rawStr, ok := raw.(string); ok { - if parsed, err := strconv.Atoi(rawStr); err == nil && parsed > 0 { - return parsed - } - engineLog.Printf("Ignoring invalid max-turn-cache-misses value: %q", rawStr) - } - return 0 + return parsePositiveIntValue(raw, "max-turn-cache-misses") +} + +// parsePositiveIntValue parses a strictly-positive integer from raw. +// Delegates to parseIntOrExpressionValue for a single int-validation path. +// GitHub Actions expression strings (e.g. "${{ inputs.value }}") are silently +// treated as 0 (not configured) because these fields are integer-only. +func parsePositiveIntValue(raw any, fieldName string) int { + s := parseIntOrExpressionValue(raw, 1, fieldName) + if s == "" || isExpression(s) { + return 0 + } + val, err := strconv.Atoi(s) + if err != nil { + return 0 + } + return val } func parseMaxTurnsValue(raw any) string { - if val, ok := typeutil.ParseIntValue(raw); ok && val > 0 { - return strconv.Itoa(val) - } - if rawStr, ok := raw.(string); ok { - trimmed := strings.TrimSpace(rawStr) - if trimmed == "" { - return "" - } - if parsed, err := strconv.Atoi(trimmed); err == nil && parsed > 0 { - return strconv.Itoa(parsed) - } - // Match the same GitHub Actions expression wrapper accepted by the schema. - // The schema and GitHub Actions runtime are responsible for validating the - // expression body itself; this helper only needs to preserve templated values. - if strings.HasPrefix(trimmed, "${{") && strings.HasSuffix(trimmed, "}}") { - return trimmed - } - engineLog.Printf("Ignoring invalid max-turns value: %q", rawStr) - } - return "" + return parseIntOrExpressionValue(raw, 1, "max-turns") } -// parseNonNegativeIntOrExpressionValue parses a raw frontmatter value that must be a -// non-negative integer (≥ 0) or a GitHub Actions expression template (${{ ... }}). +// parseHarnessMaxRetriesValue parses harness.max-retries from a raw frontmatter value +// that must be a non-negative integer (≥ 0) or a GitHub Actions expression template (${{ ... }}). // It is intentionally distinct from parseMaxTurnsValue which rejects zero. // Returns the canonical string representation, or "" when the value is absent/invalid. -func parseNonNegativeIntOrExpressionValue(raw any) string { - if val, ok := typeutil.ParseIntValue(raw); ok && val >= 0 { +func parseHarnessMaxRetriesValue(raw any) string { + return parseIntOrExpressionValue(raw, 0, "harness.max-retries") +} + +func parseIntOrExpressionValue(raw any, minValue int, fieldName string) string { + if val, ok := typeutil.ParseIntValue(raw); ok && val >= minValue { return strconv.Itoa(val) } if rawStr, ok := raw.(string); ok { @@ -90,35 +73,22 @@ func parseNonNegativeIntOrExpressionValue(raw any) string { if trimmed == "" { return "" } - if parsed, err := strconv.Atoi(trimmed); err == nil && parsed >= 0 { + if parsed, err := strconv.Atoi(trimmed); err == nil && parsed >= minValue { return strconv.Itoa(parsed) } - if strings.HasPrefix(trimmed, "${{") && strings.HasSuffix(trimmed, "}}") { + // Match the same GitHub Actions expression wrapper accepted by the schema. + // The schema and GitHub Actions runtime are responsible for validating the + // expression body itself; this helper only needs to preserve templated values. + if isExpression(trimmed) { return trimmed } - engineLog.Printf("Ignoring invalid harness.max-retries value: %q", rawStr) + engineLog.Printf("Ignoring invalid %s value: %q", fieldName, rawStr) } return "" } func parseMaxToolDenialsValue(raw any) string { - if val, ok := typeutil.ParseIntValue(raw); ok && val > 0 { - return strconv.Itoa(val) - } - if rawStr, ok := raw.(string); ok { - trimmed := strings.TrimSpace(rawStr) - if trimmed == "" { - return "" - } - if parsed, err := strconv.Atoi(trimmed); err == nil && parsed > 0 { - return strconv.Itoa(parsed) - } - if strings.HasPrefix(trimmed, "${{") && strings.HasSuffix(trimmed, "}}") { - return trimmed - } - engineLog.Printf("Ignoring invalid max-tool-denials value: %q", rawStr) - } - return "" + return parseIntOrExpressionValue(raw, 1, "max-tool-denials") } // parseAuthDefinition converts a raw auth config map (from engine.provider.auth) into diff --git a/pkg/workflow/engine_config_parser_test.go b/pkg/workflow/engine_config_parser_test.go new file mode 100644 index 00000000000..a1223ae6f11 --- /dev/null +++ b/pkg/workflow/engine_config_parser_test.go @@ -0,0 +1,64 @@ +//go:build !integration + +package workflow + +import "testing" + +func TestParsePositiveIntValues(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + parse func(any) int + raw any + expected int + }{ + {name: "max-runs int", parse: parseMaxRunsValue, raw: 3, expected: 3}, + {name: "max-runs string", parse: parseMaxRunsValue, raw: "5", expected: 5}, + {name: "max-runs invalid", parse: parseMaxRunsValue, raw: "oops", expected: 0}, + {name: "max-runs zero", parse: parseMaxRunsValue, raw: 0, expected: 0}, + {name: "max-runs negative", parse: parseMaxRunsValue, raw: -1, expected: 0}, + {name: "max-runs nil", parse: parseMaxRunsValue, raw: nil, expected: 0}, + {name: "max-turn-cache-misses int", parse: parseMaxTurnCacheMissesValue, raw: 2, expected: 2}, + {name: "max-turn-cache-misses string", parse: parseMaxTurnCacheMissesValue, raw: "7", expected: 7}, + {name: "max-turn-cache-misses invalid", parse: parseMaxTurnCacheMissesValue, raw: "bad", expected: 0}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := tt.parse(tt.raw); got != tt.expected { + t.Fatalf("got %d, want %d", got, tt.expected) + } + }) + } +} + +func TestParseIntOrExpressionValues(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + parse func(any) string + raw any + expected string + }{ + {name: "max-turns rejects zero", parse: parseMaxTurnsValue, raw: 0, expected: ""}, + {name: "max-turns accepts expression", parse: parseMaxTurnsValue, raw: " ${{ inputs.max_turns }} ", expected: "${{ inputs.max_turns }}"}, + {name: "max-turns trims whitespace", parse: parseMaxTurnsValue, raw: " 3 ", expected: "3"}, + {name: "non-negative accepts zero", parse: parseHarnessMaxRetriesValue, raw: 0, expected: "0"}, + {name: "non-negative accepts expression", parse: parseHarnessMaxRetriesValue, raw: "${{ inputs.max_retries }}", expected: "${{ inputs.max_retries }}"}, + {name: "non-negative trims whitespace", parse: parseHarnessMaxRetriesValue, raw: " 2 ", expected: "2"}, + {name: "max-tool-denials rejects zero", parse: parseMaxToolDenialsValue, raw: "0", expected: ""}, + {name: "max-tool-denials accepts expression", parse: parseMaxToolDenialsValue, raw: "${{ inputs.max_tool_denials }}", expected: "${{ inputs.max_tool_denials }}"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := tt.parse(tt.raw); got != tt.expected { + t.Fatalf("got %q, want %q", got, tt.expected) + } + }) + } +} diff --git a/pkg/workflow/expression_parser.go b/pkg/workflow/expression_parser.go index eeebc88e47f..f84125d9ef5 100644 --- a/pkg/workflow/expression_parser.go +++ b/pkg/workflow/expression_parser.go @@ -17,7 +17,7 @@ func stripExpressionWrapper(expression string) string { // Trim whitespace expr := strings.TrimSpace(expression) // Check if it starts with ${{ and ends with }} - if strings.HasPrefix(expr, "${{") && strings.HasSuffix(expr, "}}") { + if isExpression(expr) { // Remove the wrapper and trim inner whitespace return strings.TrimSpace(expr[3 : len(expr)-2]) } diff --git a/pkg/workflow/observability_otlp.go b/pkg/workflow/observability_otlp.go index ee414e50920..e2ff2fd52f1 100644 --- a/pkg/workflow/observability_otlp.go +++ b/pkg/workflow/observability_otlp.go @@ -103,7 +103,7 @@ func shouldRewriteAuthorizationForSentry(endpoint string) bool { // whitespace. func isGitHubActionsExpression(value string) bool { trimmed := strings.TrimSpace(value) - return strings.HasPrefix(trimmed, "${{") && strings.HasSuffix(trimmed, "}}") + return isExpression(trimmed) } // extractOTLPEndpointDomain parses an OTLP endpoint URL and returns its hostname. diff --git a/pkg/workflow/safe_outputs_config_base.go b/pkg/workflow/safe_outputs_config_base.go index 32aa26419d5..8ce74319b5b 100644 --- a/pkg/workflow/safe_outputs_config_base.go +++ b/pkg/workflow/safe_outputs_config_base.go @@ -1,8 +1,6 @@ package workflow import ( - "strings" - "github.com/github/gh-aw/pkg/typeutil" ) @@ -22,7 +20,7 @@ func (c *Compiler) parseBaseSafeOutputConfig(configMap map[string]any, config *B switch v := max.(type) { case string: // Accept GitHub Actions expression strings - if strings.HasPrefix(v, "${{") && strings.HasSuffix(v, "}}") { + if isExpression(v) { safeOutputsConfigLog.Printf("Parsed max as GitHub Actions expression: %s", v) config.Max = &v }