Refactor duplicated workflow parsing and timeline rendering helpers in pkg/cli and pkg/workflow
#45858
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,98 +27,68 @@ func parseMaxAICreditsValue(raw any) int64 { | |
| // parseMaxRunsValue parses max-runs from either integer or numeric-string | ||
| // frontmatter values. | ||
| func parseMaxRunsValue(raw any) int { | ||
| 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") | ||
| } | ||
|
|
||
| // 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 { | ||
|
Contributor
There was a problem hiding this comment. [/codebase-design] 💡 Examplefunc parsePositiveIntValue(raw any, fieldName string) int {
s := parseIntOrExpressionValue(raw, 1, fieldName)
if s == "" {
return 0
}
val, _ := strconv.Atoi(s)
return val
}This keeps a single source of truth for the int-validation logic. @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Refactored |
||
| 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 { | ||
| return parseIntOrExpressionValue(raw, 1, "max-turns") | ||
| } | ||
|
|
||
| // 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 parseHarnessMaxRetriesValue(raw any) string { | ||
| return parseIntOrExpressionValue(raw, 0, "harness.max-retries") | ||
|
Contributor
There was a problem hiding this comment. [/codebase-design] 💡 Suggested fixEither pass the field name through as a parameter, or rename this wrapper to // Option A: rename to be explicit
func parseHarnessMaxRetriesValue(raw any) string {
return parseIntOrExpressionValue(raw, 0, "harness.max-retries")
}
// Option B: accept a field name param and update callers
func parseNonNegativeIntOrExpressionValue(raw any, fieldName string) string {
return parseIntOrExpressionValue(raw, 0, fieldName)
}The generic name with a hard-coded label is the worst of both worlds — it looks reusable but logs the wrong field name if reused. @copilot please address this.
Contributor
Author
There was a problem hiding this comment. Renamed to
Contributor
There was a problem hiding this comment.
💡 Why this matters / suggested fixThe Consider accepting the field name as a parameter: func parseNonNegativeIntOrExpressionValue(raw any, fieldName string) string {
return parseIntOrExpressionValue(raw, 0, fieldName)
}or, since the function is only called in one place, calling the generic helper directly at that call site to keep the label accurate.
Contributor
Author
There was a problem hiding this comment. Renamed to |
||
| } | ||
|
|
||
| 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 { | ||
| trimmed := strings.TrimSpace(rawStr) | ||
| if trimmed == "" { | ||
| return "" | ||
| } | ||
| if parsed, err := strconv.Atoi(trimmed); err == nil && parsed >= minValue { | ||
| 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 isExpression(trimmed) { | ||
| return trimmed | ||
| } | ||
| engineLog.Printf("Ignoring invalid %s value: %q", fieldName, rawStr) | ||
| } | ||
| return "" | ||
| } | ||
|
|
||
| func parseMaxToolDenialsValue(raw any) string { | ||
| return parseIntOrExpressionValue(raw, 1, "max-tool-denials") | ||
| } | ||
|
|
||
| // parseAuthDefinition converts a raw auth config map (from engine.provider.auth) into | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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}, | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The test for 💡 Suggested additions{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},@copilot please address this.
Contributor
Author
There was a problem hiding this comment. Added |
||
| } | ||
|
|
||
| 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: ""}, | ||
|
Contributor
There was a problem hiding this comment. [/tdd] 💡 Suggested addition{name: "max-turns trims whitespace", parse: parseMaxTurnsValue, raw: " 3 ", expected: "3"},@copilot please address this.
Contributor
Author
There was a problem hiding this comment. Added |
||
| {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) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/codebase-design]
matchWorkflowFilenameByExperimentreceives plain filenames (no path prefix), butnormalizeWorkflowIDcallsfilepath.Basebefore stripping the extension. Callingfilepath.Baseon a plain filename likemy-flow.mdis harmless, but this is an implicit coupling — the function's behaviour depends on an internal detail ofnormalizeWorkflowID. A brief comment or the existing function-level doc should note this assumption.@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a doc-comment paragraph to
matchWorkflowFilenameByExperimentexplicitly noting thatnormalizeWorkflowIDcallsfilepath.Baseinternally, so any path prefix is stripped before matching and only the basename is returned. Commit:d1b5b8e.