From 86f15bfcaded3897f6986327016ebafada1df824 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 00:54:12 +0000 Subject: [PATCH 1/8] Initial plan From d6736ca2cead744ac19adc9211200335cf73023c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 01:18:37 +0000 Subject: [PATCH 2/8] refactor: deduplicate workflow id and expression parsing helpers Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/cli/experiments_command.go | 4 +- pkg/cli/gateway_logs_timeline_render.go | 14 ++--- pkg/cli/gateway_logs_timeline_test.go | 32 ++++++++++ pkg/cli/spec.go | 4 +- pkg/cli/workflows.go | 6 +- pkg/workflow/engine_config_parser.go | 71 ++++++----------------- pkg/workflow/engine_config_parser_test.go | 59 +++++++++++++++++++ pkg/workflow/expression_parser.go | 2 +- pkg/workflow/observability_otlp.go | 2 +- pkg/workflow/safe_outputs_config_base.go | 4 +- 10 files changed, 127 insertions(+), 71 deletions(-) create mode 100644 pkg/workflow/engine_config_parser_test.go diff --git a/pkg/cli/experiments_command.go b/pkg/cli/experiments_command.go index cc14d633f68..26c832dd384 100644 --- a/pkg/cli/experiments_command.go +++ b/pkg/cli/experiments_command.go @@ -436,7 +436,7 @@ func findRemoteWorkflowFilenameForExperiment(repoOverride, experimentName string 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) } @@ -459,7 +459,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_config_parser.go b/pkg/workflow/engine_config_parser.go index bd56b53d244..2810c68ad2f 100644 --- a/pkg/workflow/engine_config_parser.go +++ b/pkg/workflow/engine_config_parser.go @@ -27,21 +27,16 @@ 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 { + return parsePositiveIntValue(raw, "max-turn-cache-misses") +} + +func parsePositiveIntValue(raw any, fieldName string) int { if val, ok := typeutil.ParseIntValue(raw); ok && val > 0 { return val } @@ -49,32 +44,13 @@ func parseMaxTurnCacheMissesValue(raw any) int { if parsed, err := strconv.Atoi(rawStr); err == nil && parsed > 0 { return parsed } - engineLog.Printf("Ignoring invalid max-turn-cache-misses value: %q", rawStr) + engineLog.Printf("Ignoring invalid %s value: %q", fieldName, rawStr) } return 0 } 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 @@ -82,7 +58,11 @@ func parseMaxTurnsValue(raw any) string { // 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 { + 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 +70,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..584c2d22108 --- /dev/null +++ b/pkg/workflow/engine_config_parser_test.go @@ -0,0 +1,59 @@ +//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-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: "non-negative accepts zero", parse: parseNonNegativeIntOrExpressionValue, raw: 0, expected: "0"}, + {name: "non-negative accepts expression", parse: parseNonNegativeIntOrExpressionValue, raw: "${{ inputs.max_retries }}", expected: "${{ inputs.max_retries }}"}, + {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 } From 6462689f20bdb36d0ce99230e199b457dea6ef0b Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 16 Jul 2026 04:38:00 +0000 Subject: [PATCH 3/8] docs(adr): add draft ADR-45858 for consolidating duplicated parsing helpers Co-Authored-By: Claude Sonnet 4.6 --- ...-consolidate-duplicated-parsing-helpers.md | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 docs/adr/45858-consolidate-duplicated-parsing-helpers.md 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.* From 210588b8f3bd430f66d6a9b1d78c5a3d508c2d3d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 04:46:41 +0000 Subject: [PATCH 4/8] chore: triaging review threads for pr-finisher Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/skills/agentic-workflows/SKILL.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index 499e6113b76..4c2b5396a97 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -31,6 +31,7 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/dependabot.md` - `.github/aw/deployment-status.md` - `.github/aw/designer.md` +- `.github/aw/evals.md` - `.github/aw/experiments.md` - `.github/aw/github-agentic-workflows.md` - `.github/aw/github-mcp-server.md` From 93b3a3f2b65ea9df424c0864db1888910e087aa9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 04:58:09 +0000 Subject: [PATCH 5/8] fix: address review feedback on engine_config_parser and experiments_command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename parseNonNegativeIntOrExpressionValue → parseHarnessMaxRetriesValue to make the single-purpose coupling explicit and avoid misleading log output on reuse - Refactor parsePositiveIntValue to delegate to parseIntOrExpressionValue, eliminating a parallel int-validation path that could silently diverge - Add boundary test cases (zero, negative, nil) for parsePositiveIntValue - Add whitespace-trimming test case for parseIntOrExpressionValue - Add doc comment to matchWorkflowFilenameByExperiment clarifying that normalizeWorkflowID calls filepath.Base, stripping any path prefix from filenames Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/skills/agentic-workflows/SKILL.md | 1 - pkg/cli/experiments_command.go | 5 +++++ pkg/workflow/engine.go | 2 +- pkg/workflow/engine_config_parser.go | 20 ++++++++------------ pkg/workflow/engine_config_parser_test.go | 8 ++++++-- 5 files changed, 20 insertions(+), 16 deletions(-) diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index 4c2b5396a97..499e6113b76 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -31,7 +31,6 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/dependabot.md` - `.github/aw/deployment-status.md` - `.github/aw/designer.md` -- `.github/aw/evals.md` - `.github/aw/experiments.md` - `.github/aw/github-agentic-workflows.md` - `.github/aw/github-mcp-server.md` diff --git a/pkg/cli/experiments_command.go b/pkg/cli/experiments_command.go index 26c832dd384..a71aea15f99 100644 --- a/pkg/cli/experiments_command.go +++ b/pkg/cli/experiments_command.go @@ -433,6 +433,11 @@ 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 { 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 2810c68ad2f..6b1fd9f77be 100644 --- a/pkg/workflow/engine_config_parser.go +++ b/pkg/workflow/engine_config_parser.go @@ -37,27 +37,23 @@ func parseMaxTurnCacheMissesValue(raw any) int { } func parsePositiveIntValue(raw any, fieldName string) int { - if val, ok := typeutil.ParseIntValue(raw); ok && val > 0 { - return val + s := parseIntOrExpressionValue(raw, 1, fieldName) + if s == "" { + return 0 } - if rawStr, ok := raw.(string); ok { - if parsed, err := strconv.Atoi(rawStr); err == nil && parsed > 0 { - return parsed - } - engineLog.Printf("Ignoring invalid %s value: %q", fieldName, rawStr) - } - return 0 + val, _ := strconv.Atoi(s) + return val } func parseMaxTurnsValue(raw any) string { 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 { +func parseHarnessMaxRetriesValue(raw any) string { return parseIntOrExpressionValue(raw, 0, "harness.max-retries") } diff --git a/pkg/workflow/engine_config_parser_test.go b/pkg/workflow/engine_config_parser_test.go index 584c2d22108..2bb50b213f1 100644 --- a/pkg/workflow/engine_config_parser_test.go +++ b/pkg/workflow/engine_config_parser_test.go @@ -16,6 +16,9 @@ func TestParsePositiveIntValues(t *testing.T) { {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}, @@ -42,8 +45,9 @@ func TestParseIntOrExpressionValues(t *testing.T) { }{ {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: "non-negative accepts zero", parse: parseNonNegativeIntOrExpressionValue, raw: 0, expected: "0"}, - {name: "non-negative accepts expression", parse: parseNonNegativeIntOrExpressionValue, raw: "${{ inputs.max_retries }}", expected: "${{ inputs.max_retries }}"}, + {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: "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 }}"}, } From f13e96c7cbfaa7646d51e6efee61849070558bb7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 04:59:14 +0000 Subject: [PATCH 6/8] fix: document expression-to-zero behavior in parsePositiveIntValue; add whitespace test for parseHarnessMaxRetriesValue Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/engine_config_parser.go | 6 +++++- pkg/workflow/engine_config_parser_test.go | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/pkg/workflow/engine_config_parser.go b/pkg/workflow/engine_config_parser.go index 6b1fd9f77be..9ea2f472e26 100644 --- a/pkg/workflow/engine_config_parser.go +++ b/pkg/workflow/engine_config_parser.go @@ -36,12 +36,16 @@ func parseMaxTurnCacheMissesValue(raw any) int { 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 == "" { return 0 } - val, _ := strconv.Atoi(s) + val, _ := strconv.Atoi(s) // expression strings produce 0 (not configured) return val } diff --git a/pkg/workflow/engine_config_parser_test.go b/pkg/workflow/engine_config_parser_test.go index 2bb50b213f1..a1223ae6f11 100644 --- a/pkg/workflow/engine_config_parser_test.go +++ b/pkg/workflow/engine_config_parser_test.go @@ -48,6 +48,7 @@ func TestParseIntOrExpressionValues(t *testing.T) { {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 }}"}, } From 86d8c1f3f684370bfaaedf121d78f45455f993ec Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 05:16:35 +0000 Subject: [PATCH 7/8] Fix strconvparseignorederror lint: handle strconv.Atoi error in parsePositiveIntValue Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/skills/agentic-workflows/SKILL.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index 499e6113b76..4c2b5396a97 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -31,6 +31,7 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/dependabot.md` - `.github/aw/deployment-status.md` - `.github/aw/designer.md` +- `.github/aw/evals.md` - `.github/aw/experiments.md` - `.github/aw/github-agentic-workflows.md` - `.github/aw/github-mcp-server.md` From 47ff3773a634e6c2d688ea1f8c49fa2996cd52e2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 16 Jul 2026 05:17:44 +0000 Subject: [PATCH 8/8] Fix strconvparseignorederror lint: explicitly handle strconv.Atoi error in parsePositiveIntValue Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/engine_config_parser.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/workflow/engine_config_parser.go b/pkg/workflow/engine_config_parser.go index 9ea2f472e26..027aa9ab82d 100644 --- a/pkg/workflow/engine_config_parser.go +++ b/pkg/workflow/engine_config_parser.go @@ -42,10 +42,13 @@ func parseMaxTurnCacheMissesValue(raw any) int { // 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 == "" { + if s == "" || isExpression(s) { + return 0 + } + val, err := strconv.Atoi(s) + if err != nil { return 0 } - val, _ := strconv.Atoi(s) // expression strings produce 0 (not configured) return val }