Refactor workflow engine execution builders and log parsers to clear largefunc lint slice - #49793
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors large workflow execution builders and log parsers into focused helpers while preserving their existing responsibilities.
Changes:
- Decomposes four engine execution builders.
- Introduces stateful Copilot and Claude parsing helpers.
- Separates command, environment, firewall, and rendering phases.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/antigravity_engine.go |
Splits Antigravity step construction into phases. |
pkg/workflow/behavior_defined_engine.go |
Extracts behavior-defined setup, command, and environment helpers. |
pkg/workflow/claude_logs.go |
Decomposes mixed-log parsing and metric extraction. |
pkg/workflow/codex_engine.go |
Splits Codex execution assembly into focused helpers. |
pkg/workflow/copilot_logs.go |
Adds parser state and block-processing helpers. |
pkg/workflow/pi_engine.go |
Separates Pi command, routing, environment, and step construction. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
|
|
||
| claudeLogsLog.Print("JSON array parse failed, trying JSONL format") | ||
| if verbose { | ||
| fmt.Fprintf(os.Stderr, "Failed to parse Claude log as JSON array, trying JSONL format\n") |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: N/A — No Test Changes
|
…ition Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (827 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
|
![github-actions[bot]](https://cdn.github.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 72.8 AIC · ⌖ 11.4 AIC · ⊞ 5.4K
| } | ||
| } | ||
| } | ||
| if e.claudeEntryType(entry) == "result" { |
There was a problem hiding this comment.
Redundant claudeEntryType call — double dispatch per result entry.
The switch e.claudeEntryType(entry) on line 253 already matched "result" and dispatched applyClaudeResultEntry. This line then calls claudeEntryType(entry) a second time on every iteration just to drive the break. The bare break here is inside the for body (not the switch), so it does correctly exit the loop — but the redundant call is misleading.
A cleaner approach uses a boolean flag set inside the switch:
var done bool
for _, entry := range logEntries {
switch e.claudeEntryType(entry) {
case "result":
e.applyClaudeResultEntry(&metrics, entry, verbose)
done = true
case "assistant":
currentSequence = append(currentSequence, e.claudeAssistantSequence(entry, toolCallMap)...)
case "user":
e.claudeUserToolResults(entry, toolCallMap)
}
if done {
break
}
}@copilot please address this.
There was a problem hiding this comment.
Fixed in 91d41c7. Replaced the redundant second claudeEntryType call with a done boolean flag set inside the "result" case of the switch, driving the break without a duplicate dispatch.
There was a problem hiding this comment.
Verdict: Request changes — one real behavior regression found in the "no behavior change" refactor
💡 Details
The mechanical function-splitting is generally faithful to the original control flow across antigravity_engine.go, behavior_defined_engine.go, codex_engine.go, pi_engine.go, and copilot_logs.go. However, claude_logs.go's new extractClaudeJSONArrayEntries helper drops the original's guarantee that the outer loop index (i) always advances past a consumed multi-line array block, even when the array fails to parse as JSON. In the original code, i = j happened before the continue, regardless of parse success. In the refactor, i = next only happens inside the if entries, next := ...; len(entries) > 0 branch — when parsing fails, the lines that were already consumed into the buffer get reprocessed one line at a time in the next iterations, which can produce spurious/duplicate JSONL entries in mixed-format Claude logs. Flagged inline.
There's also a pre-existing review comment (from Copilot) about a dropped json.Unmarshal error message in the fallback diagnostic at claude_logs.go:162 — that one stands and is a legitimate (if lower-severity) diagnostics regression, not duplicated here.
The grumpy-coder sub-agent's first pass found no additional issues beyond the known one; its output was reviewed and is consistent with my own second pass.
🔎 Code quality review by PR Code Quality Reviewer · auto · 155.1 AIC · ⌖ 8.26 AIC · ⊞ 7.9K
Comment /review to run again
| func (e *ClaudeEngine) parseClaudeMixedLogEntries(logContent string, verbose bool) []map[string]any { | ||
| var logEntries []map[string]any | ||
| lines := strings.Split(logContent, "\n") | ||
| for i := 0; i < len(lines); i++ { | ||
| trimmedLine := strings.TrimSpace(lines[i]) | ||
| if trimmedLine == "" { | ||
| continue | ||
| } | ||
| if strings.HasPrefix(trimmedLine, "[") { | ||
| if entries, next := extractClaudeJSONArrayEntries(lines, i); len(entries) > 0 { | ||
| logEntries = append(logEntries, entries...) | ||
| i = next | ||
| continue | ||
| } |
There was a problem hiding this comment.
When a multi-line JSON array block fails to parse (both direct and substring-extraction attempts), the consumed lines are no longer skipped in this refactor — inner lines get reprocessed individually as if they were standalone JSONL entries.
There was a problem hiding this comment.
Fixed in 91d41c7. extractClaudeJSONArrayEntries now always returns the last consumed line index (end) regardless of parse outcome. parseClaudeMixedLogEntries unconditionally assigns i = next before the len(entries) > 0 check, so multi-line array blocks are always skipped even when both JSON parse attempts fail.
|
@copilot Unresolved review threads to address (newest first):
I also attempted a branch refresh. Please address the items above, rerun checks as needed, and then continue with the
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /diagnosing-bugs — requesting changes on two correctness issues and one readability trap.
📋 Key Themes & Highlights
Key Themes
- Correctness — double sequence append (
copilot_logs.go:283):finalize()manually appendsp.currentSequencetoToolSequences, then immediately passes it toFinalizeToolMetricswhich appends it again — the final sequence appears twice in every Copilot JSONL parse result. - Readability trap — codex firewall branch (
codex_engine.go:294–296): initialise-then-immediately-overwrite pattern hides the real harness-vs-no-harness branch logic; an explicitif/elsemakes the intent clear. - Diagnostics loss (
claude_logs.go:162): thejson.Unmarshalerror is dropped from the verbose diagnostic path (pre-existing but worsened by removing the inline%vformat from the old code). - Pre-existing heuristic surfaced by refactor (
copilot_logs.go:251): the new state type is the right home for atoolUseIDMapthat would lethandleUserEntrycorrelate output sizes to the correct tool by ID rather than by iteration order.
Positive Highlights
- ✅ Excellent decomposition: each engine now has clear, named build phases —
buildCommand,buildEnv,buildStep— that are easy to test in isolation. - ✅ The
copilotSessionJSONLParserandcopilotDebugLogParseStatestate types are a clean application of the "narrow state" pattern; they make the multi-phase parsing logic far more navigable. - ✅
strings.SplitSeq(Go 1.26 range-over-func) is used well here — avoids allocating the full slice. - ✅ Consistent use of
CachedAllowedDomainsComputedguard in all*AllowedDomainshelpers — no re-computation regression.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 154.2 AIC · ⌖ 15 AIC · ⊞ 7.1K
Comment /matt to run again
| } else { | ||
| codexCommandWithSetup = fmt.Sprintf(`%s && INSTRUCTION="$(cat /tmp/gh-aw/aw-prompts/prompt.txt)" && %s`, npmPathSetup, codexCommand) | ||
| codexCommandWithSetup := fmt.Sprintf(`%s && %s`, GetNpmBinPathSetup(), codexCommand) | ||
| if harnessScriptName == "" { |
There was a problem hiding this comment.
[/codebase-design] Logic inversion in the firewall branch: line 294 initialises codexCommandWithSetup with the harness-safe form, then the harnessScriptName == "" guard immediately overwrites it with the INSTRUCTION-injection form. The initialisation on line 294 is only kept when there is a harness script — but that is the intended harness-safe form. This is actually correct, but reading it as "initialise then conditionally overwrite" is misleading; the original intent was "harness → no INSTRUCTION, no-harness → inject INSTRUCTION".
💡 Suggested fix: use an explicit if/else
var codexCommandWithSetup string
if harnessScriptName != "" {
codexCommandWithSetup = fmt.Sprintf(`%s && %s`, GetNpmBinPathSetup(), codexCommand)
} else {
codexCommandWithSetup = fmt.Sprintf(`%s && INSTRUCTION="$(cat /tmp/gh-aw/aw-prompts/prompt.txt)" && %s`, GetNpmBinPathSetup(), codexCommand)
}This matches the structure of the non-firewall branch (lines 319-334) and makes the intent immediately clear to future maintainers.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 91d41c7 with the suggested explicit if/else on harnessScriptName != "".
| if !p.foundSessionEntry { | ||
| return p.metrics, false | ||
| } | ||
| if len(p.currentSequence) > 0 { |
There was a problem hiding this comment.
[/codebase-design] Double-append of currentSequence into ToolSequences. Lines 283–284 explicitly append p.currentSequence to p.metrics.ToolSequences, and then FinalizeToolMetrics (called 5 lines later) also appends opts.CurrentSequence when it is non-empty (metrics.go:218–220). The final tool sequence will appear twice in the parsed metrics, which would corrupt sequence-based reporting.
💡 Suggested fix
Remove the manual append on line 283–284 and let FinalizeToolMetrics handle it exclusively:
func (p *copilotSessionJSONLParser) finalize() (LogMetrics, bool) {
if p.turns == 0 && p.assistantMessageCount > 0 {
p.turns = p.assistantMessageCount
...
}
if !p.foundSessionEntry {
return p.metrics, false
}
// Remove the manual append — FinalizeToolMetrics handles currentSequence
copilotLogsLog.Printf(...)
FinalizeToolMetrics(FinalizeToolMetricsOptions{
Metrics: &p.metrics,
ToolCallMap: p.toolCallMap,
CurrentSequence: p.currentSequence,
Turns: p.turns,
TokenUsage: p.totalTokenUsage,
})
return p.metrics, true
}Alternatively, pass an empty CurrentSequence to FinalizeToolMetrics since the append was already done.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 91d41c7. Removed the manual p.metrics.ToolSequences = append(...) in finalize(); FinalizeToolMetrics is now the sole owner of that append via CurrentSequence.
| continue | ||
| } | ||
| outputSize := len(content.Content) | ||
| for toolName, toolInfo := range p.toolCallMap { |
There was a problem hiding this comment.
[/codebase-design] handleUserEntry updates MaxOutputSize by iterating all tools and updating the first one whose current MaxOutputSize is smaller than the current output size. This heuristic (carried over from the original code) does not correlate ToolUseID with the correct tool — it applies the output size to a random tool. In a multi-tool session this silently misattributes output sizes.
💡 Suggested improvement
The SessionEntry carries ToolUseID on the tool-result content. If the assistant entries stored a toolUseID → toolName map when processing tool_use content items, the correlation would be exact:
// In handleAssistantEntry, also record:
p.toolUseIDMap[content.ID] = content.Name // content.ID is the tool_use id
// In handleUserEntry:
for _, content := range entry.Message.Content {
if content.Type != "tool_result" || content.ToolUseID == "" {
continue
}
toolName, ok := p.toolUseIDMap[content.ToolUseID]
if !ok {
continue
}
if outputSize := len(content.Content); outputSize > p.toolCallMap[toolName].MaxOutputSize {
p.toolCallMap[toolName].MaxOutputSize = outputSize
}
}This is a pre-existing approximation, but the refactor is the right time to address it since the state type now has a clear home for toolUseIDMap.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 91d41c7. Added toolUseIDMap map[string]string to copilotSessionJSONLParser. handleAssistantEntry records content.ID → content.Name for every tool_use content item. handleUserEntry now looks up the exact tool name by content.ToolUseID instead of iterating all tools for a first-smaller-than match.
|
|
||
| claudeLogsLog.Print("JSON array parse failed, trying JSONL format") | ||
| if verbose { | ||
| fmt.Fprintf(os.Stderr, "Failed to parse Claude log as JSON array, trying JSONL format\n") |
There was a problem hiding this comment.
[/diagnosing-bugs] The json.Unmarshal error is dropped when the fallback diagnostic fires. A previous review comment already flagged this (id 3699300105). The verbose path at line 162 prints a generic message without the original parse error, making malformed/mixed Claude logs much harder to diagnose in production.
💡 Suggested fix
if err := json.Unmarshal([]byte(logContent), &logEntries); err == nil {
return logEntries
}
// Keep the error for diagnostics
jsonArrayErr := err
claudeLogsLog.Print("JSON array parse failed, trying JSONL format")
if verbose {
fmt.Fprintf(os.Stderr, "Failed to parse Claude log as JSON array (%v), trying JSONL format
", jsonArrayErr)
}@copilot please address this.
…nostic Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in the latest commit: restored the |
|
@copilot Please continue triage on this PR. Unresolved review follow-up appears to remain on the current head:
I also requested a branch refresh for this PR. Run details: https://github.com/github/gh-aw/actions/runs/30753985170
|
…arsers and codex engine Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All five code issues identified by |
…sub-agent Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. The test |
|
@copilot resolve the merge conflicts on this branch. |
…fix-function-lengths # Conflicts: # pkg/cli/pr_code_quality_reviewer_workflow_contract_test.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…e main conflicts Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Merged |
|
@copilot Please continue triage on this PR. Remaining reviewer-visible blockers:
Run details: https://github.com/github/gh-aw/actions/runs/30758413653
|
Branch is already up to date with |
|
🎉 This pull request is included in a new release. Release: |
This slice targets the workflow engine
GetExecutionStepsbuilders and workflow log parsers that were still trippinglargefunc. The change keeps scope to those paths by splitting multi-phase functions into focused private helpers without changing external behavior.Engine execution builders
GetExecutionStepsimplementations into explicit phases for:AntigravityEngineBehaviorDefinedEngineCodexEnginePiEngineLog parser decomposition
Stateful parsing cleanup
Representative shape of the refactor
Scope note
pkg/workflowlargefunchits after this change are outside this issue’s scoped paths (for example Codex installation/tool expansion helpers).run: https://github.com/github/gh-aw/actions/runs/30751969165
"$BODY_49793"