Refactor duplicated workflow parsing and timeline rendering helpers in pkg/cli and pkg/workflow#45858
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
pkg/cli and pkg/workflow
|
Hey A couple of things to wrap up before this is ready for review:
If you'd like a hand finishing this up, you can assign this prompt to your coding agent:
|
There was a problem hiding this comment.
Pull request overview
Consolidates duplicated parsing, workflow normalization, expression detection, and timeline rendering while preserving behavior.
Changes:
- Centralizes engine integer/expression parsing.
- Reuses canonical expression and workflow-ID helpers.
- Deduplicates message timeline rendering and adds tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_config_base.go |
Reuses expression detection. |
pkg/workflow/observability_otlp.go |
Unifies OTLP expression checks. |
pkg/workflow/expression_parser.go |
Reuses wrapper detection. |
pkg/workflow/engine_config_parser.go |
Consolidates numeric parsing. |
pkg/workflow/engine_config_parser_test.go |
Tests consolidated parsers. |
pkg/cli/workflows.go |
Centralizes workflow-ID normalization. |
pkg/cli/spec.go |
Normalizes parsed workflow names. |
pkg/cli/experiments_command.go |
Normalizes experiment workflow filenames. |
pkg/cli/gateway_logs_timeline_render.go |
Deduplicates message row rendering. |
pkg/cli/gateway_logs_timeline_test.go |
Tests assistant and reasoning rows. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 0
- Review effort level: Medium
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Behavior-preserving refactor ✅
The consolidation is clean and correct. All four areas (engine config parser, isExpression predicate, workflow-ID normalization, timeline renderer) converge on the same semantics as before.
One non-blocking issue flagged inline:
parseNonNegativeIntOrExpressionValue (the named wrapper) hard-codes "harness.max-retries" as the diagnostic label inside the shared parseIntOrExpressionValue helper. Because the wrapper is named generically, any future re-use at a different call site would produce misleading log output. Rename the wrapper to parseHarnessMaxRetriesValue or add a fieldName parameter to make the coupling explicit.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 27.2 AIC · ⌖ 4.39 AIC · ⊞ 5K
| // 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 { |
There was a problem hiding this comment.
The wrapper parseNonNegativeIntOrExpressionValue hardcodes "harness.max-retries" as the field name passed to parseIntOrExpressionValue, but the function name and doc comment describe it as a generic non-negative integer/expression helper. Any future caller (or test log output) will see a misleading field label.
Consider either:
- Adding a
fieldName stringparameter toparseNonNegativeIntOrExpressionValueso each call site supplies the correct label, or - Renaming the wrapper to
parseHarnessMaxRetriesValueto make the coupling explicit.
@copilot please address this.
There was a problem hiding this comment.
Renamed to parseHarnessMaxRetriesValue (Option 2) to make the single-purpose coupling explicit. Updated call site in engine.go and test references accordingly. Commit: d1b5b8e.
🧪 Test Quality Sentinel Report
📊 Metrics (4 tests)
Quality ObservationsStrengths:
Concerns:
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
…elpers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (127 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review\n\nApplied /codebase-design and /tdd — no blocking issues, but a few improvements worth addressing. See inline comments.\n\nPositive highlights: clean renderMessageContentTimelineRow extraction, consistent isExpression usage, tests added alongside new helpers, behavior-preserving refactor.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 39.4 AIC · ⌖ 4.75 AIC · ⊞ 6.7K
Comment /matt to run again
| // 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") |
There was a problem hiding this comment.
[/codebase-design] parseNonNegativeIntOrExpressionValue hardcodes "harness.max-retries" as the field name inside the generic parseIntOrExpressionValue helper — any future reuse of this wrapper for a different field would emit misleading log messages.
💡 Suggested fix
Either pass the field name through as a parameter, or rename this wrapper to parseHarnessMaxRetriesValue to make it clear it is single-purpose:
// 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.
There was a problem hiding this comment.
Renamed to parseHarnessMaxRetriesValue (Option A) — the function is only called in one place so naming it after the field makes the coupling explicit and prevents misleading log output on reuse. Commit: d1b5b8e.
| return parsePositiveIntValue(raw, "max-turn-cache-misses") | ||
| } | ||
|
|
||
| func parsePositiveIntValue(raw any, fieldName string) int { |
There was a problem hiding this comment.
[/codebase-design] parsePositiveIntValue and parseIntOrExpressionValue have overlapping logic (both parse int-or-string), but the former returns int and the latter returns string. This creates two parallel parsing paths that can diverge. Consider whether parsePositiveIntValue could internally call parseIntOrExpressionValue (ignoring the expression branch) to reduce the chance of subtle future drift.
💡 Example
func 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.
There was a problem hiding this comment.
Refactored parsePositiveIntValue to delegate to parseIntOrExpressionValue, making it the single source of truth for int-validation logic. GitHub Actions expression strings are documented to silently produce 0 (not configured) since these fields are integer-only. Commit: d1b5b8e.
| var matches []string | ||
| for _, filename := range filenames { | ||
| base := strings.TrimSuffix(filename, ".md") | ||
| base := normalizeWorkflowID(filename) |
There was a problem hiding this comment.
[/codebase-design] matchWorkflowFilenameByExperiment receives plain filenames (no path prefix), but normalizeWorkflowID calls filepath.Base before stripping the extension. Calling filepath.Base on a plain filename like my-flow.md is harmless, but this is an implicit coupling — the function's behaviour depends on an internal detail of normalizeWorkflowID. A brief comment or the existing function-level doc should note this assumption.
@copilot please address this.
There was a problem hiding this comment.
Added a doc-comment paragraph to matchWorkflowFilenameByExperiment explicitly noting that normalizeWorkflowID calls filepath.Base internally, so any path prefix is stripped before matching and only the basename is returned. Commit: d1b5b8e.
| {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}, |
There was a problem hiding this comment.
[/tdd] The test for max-runs invalid only covers a non-numeric string. Consider adding boundary cases: 0 (should return 0 since positive means > 0), negative integers, and nil. These edge cases are the most common sources of regressions when a shared helper like parsePositiveIntValue is changed.
💡 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.
There was a problem hiding this comment.
Added max-runs zero, max-runs negative, and max-runs nil test cases to TestParsePositiveIntValues. All three return 0 as expected. Commit: d1b5b8e.
| raw any | ||
| expected string | ||
| }{ | ||
| {name: "max-turns rejects zero", parse: parseMaxTurnsValue, raw: 0, expected: ""}, |
There was a problem hiding this comment.
[/tdd] TestParseIntOrExpressionValues does not test the truncation/trimming behaviour for the string path (e.g. " 5 " with surrounding whitespace should parse to "5"). The previous inline implementation had an explicit strings.TrimSpace call — a test documenting this would guard against accidental removal.
💡 Suggested addition
{name: "max-turns trims whitespace", parse: parseMaxTurnsValue, raw: " 3 ", expected: "3"},@copilot please address this.
There was a problem hiding this comment.
Added max-turns trims whitespace (raw: " 3 ", expected: "3") and a companion non-negative trims whitespace case for parseHarnessMaxRetriesValue. Both pass. Commit: d1b5b8e / fa53a73.
There was a problem hiding this comment.
Overall Assessment
Solid, well-scoped refactor. The deduplication is behaviour-preserving in all current call paths, and the added tests are a clear improvement. Two non-blocking observations worth addressing.
Findings summary
1. parseNonNegativeIntOrExpressionValue hardcodes "harness.max-retries" in the generic helper (see inline comment) — medium maintainability concern; if the wrapper is ever reused for another field, error logs will silently misidentify the source.
2. matchWorkflowFilenameByExperiment implicit filepath.Base addition (see inline comment) — the old code did not call filepath.Base, normalizeWorkflowID does. Behaviour is correct for bare filenames from the GitHub API, but the semantic change is undocumented and worth a test or comment to protect against future regressions.
All other changed paths (isExpression extraction, parseIntOrExpressionValue generalization, timeline renderer dedup, parsePositiveIntValue) are clean equivalence refactors.
🔎 Code quality review by PR Code Quality Reviewer · 53 AIC · ⌖ 4.65 AIC · ⊞ 5.6K
Comment /review to run again
| // 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") |
There was a problem hiding this comment.
parseNonNegativeIntOrExpressionValue hardcodes an unrelated field name in the generic helper: calling parseIntOrExpressionValue(raw, 0, "harness.max-retries") bakes a specific caller's field name into the reusable function.
💡 Why this matters / suggested fix
The fieldName argument is used only for logging. Currently accurate because this function is only called from the harness max-retries code path, but if parseNonNegativeIntOrExpressionValue is ever reused for a second field, every invalid-value log line will incorrectly say harness.max-retries — silent, confusing, and hard to catch in production.
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.
There was a problem hiding this comment.
Renamed to parseHarnessMaxRetriesValue — the name now accurately reflects its single purpose and the fieldName label in parseIntOrExpressionValue will always be correct. Commit: d1b5b8e.
| var matches []string | ||
| for _, filename := range filenames { | ||
| base := strings.TrimSuffix(filename, ".md") | ||
| base := normalizeWorkflowID(filename) |
There was a problem hiding this comment.
matchWorkflowFilenameByExperiment now applies filepath.Base to input filenames via normalizeWorkflowID; the old code did not: the change silently alters what gets appended to matches and returned to the caller.
💡 Analysis
Old code: strings.TrimSuffix(filename, ".md") — strips the extension but preserves any path prefix in the returned value.
New code: normalizeWorkflowID(filename) → calls filepath.Base first, so any path component is stripped before the result is stored in matches.
If the GitHub API returns bare filenames like my-workflow.md, filepath.Base is a no-op and behaviour is identical. But if the API ever returns paths (e.g. .github/workflows/my-workflow.md), the old code would append .github/workflows/my-workflow to matches while the new code appends just my-workflow. Since the function is documented as returning the basename, the new behaviour is arguably more correct — but the change is implicit and undocumented, and callers relying on the old path-preserving behaviour would silently break.
Recommend adding a test that verifies the function handles path-prefixed filenames as expected, or a comment confirming that callers always receive bare basenames.
There was a problem hiding this comment.
Added a doc-comment to matchWorkflowFilenameByExperiment documenting the filepath.Base call inside normalizeWorkflowID, clarifying the behavior for bare filenames vs. path-prefixed ones and confirming callers receive only the basename. Commit: d1b5b8e.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…command - 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>
…dd whitespace test for parseHarnessMaxRetriesValue Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…PositiveIntValue Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…or in parsePositiveIntValue Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. The |
|
🎉 This pull request is included in a new release. Release: |
This PR addresses verified semantic duplication hotspots in
pkg/cliandpkg/workflow, focusing on parser consolidation, workflow ID normalization reuse,${{ }}predicate unification, and timeline row renderer deduplication. The goal is behavior-preserving consolidation of repeated logic into canonical helpers.Engine config parser dedup (
pkg/workflow/engine_config_parser.go)parsePositiveIntValue(raw, fieldName)parseIntOrExpressionValue(raw, minValue, fieldName)max-runs,max-turn-cache-misses,max-turns,harness.max-retries, andmax-tool-denialsthrough shared logic.minValueand field-specific logging labels.Expression wrapper predicate unification (
pkg/workflow)${{ ... }}wrapper checks with sharedisExpression(...)usage in:expression_parser.gosafe_outputs_config_base.goobservability_otlp.goengine_config_parser.goWorkflow-ID-from-path consolidation (
pkg/cli).mdbasename stripping paths with the existing normalization flow (normalizeWorkflowID→stringutil.NormalizeWorkflowName) in:workflows.gospec.goexperiments_command.go.md,.lock.yml) and path/basename normalization.Timeline row renderer dedup (
pkg/cli/gateway_logs_timeline_render.go)renderMessageContentTimelineRow(kind, evt)TimelineEventKind.