refactor(pkg/workflow): break up 5 large functions exceeding 60-line lint limit#47287
Conversation
…ow slice A) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ce A Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors five oversized workflow compiler functions into focused helpers while preserving behavior and satisfying the 60-line lint limit.
Changes:
- Splits trigger/default handling and Claude command construction into helpers.
- Decomposes prompt generation and safe-output job assembly.
- Preserves command/label trigger event handling.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/trigger_parser.go |
Extracts trigger parsing and finalization helpers. |
pkg/workflow/tools.go |
Decomposes defaults and event-map generation. |
pkg/workflow/compiler_yaml_prompt.go |
Separates imports, mappings, and prompt chunks. |
pkg/workflow/compiler_safe_outputs_job.go |
Extracts safe-output steps, conditions, and dependencies. |
pkg/workflow/claude_engine.go |
Separates Claude arguments, commands, and environment setup. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 0
- Review effort level: Medium
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. PR #47287 contains only refactoring of 5 production Go files (claude_engine.go, compiler_safe_outputs_job.go, compiler_yaml_prompt.go, tools.go, trigger_parser.go) to break up large functions. Test Quality Sentinel analysis skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Refactor Review
The extraction is clean and behavior-preserving overall. One minor correctness gap to note:
seenNeeds[preActName] removed (compiler_safe_outputs_job.go, buildSafeOutputsJobNeeds): After appending preActName to needs, the corresponding seenNeeds[preActName] = struct{}{} update was dropped. The map is not read again after this point, so it has no current impact — but the map is left inconsistent. If the function is extended with further deduplication logic, this silent omission will produce a duplicate-needs bug. Recommend restoring the update for defensive correctness.
All other extracted helpers are behaviorally equivalent to the originals. The permissionModeValueIndex index-capture-before-append pattern is preserved correctly, and the calculatePreambleInsertIndex reuse of generateSetupStep for counting was already present in the original code.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 52.9 AIC · ⌖ 5.61 AIC · ⊞ 5K
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (963 new lines across pkg/workflow) but does not have a linked Architecture Decision Record (ADR). Draft ADR committed: docs/adr/47287-decompose-pkg-workflow-large-functions-claude-engine-and-compiler.md -- review and complete it before merging. The decision pattern here is already established by ADR-47061 (docs/adr/47061-function-length-lint-compliance-via-helper-extraction.md). The new draft ADR-47287 scopes that decision to this PR specific 5 functions and records the side-effect bug fix in buildCommandTriggerEventsMap. This PR cannot merge until an ADR is linked in the PR body. 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.
|
🤖 PR Triage
Score breakdown: Impact 30 + Urgency 15 + Quality 15 Large function refactor in
|
There was a problem hiding this comment.
LGTM — no correctness issues found.
Thoroughly reviewed all five files. The extractions are behaviorally identical to the originals across all control paths. The declared bug fix (len(data.LabelCommand) > 0 guard in buildCommandTriggerEventsMap) is correct and closes the spurious labeled-event emission in slash_command-only workflows.
📋 Review notes
What was verified
claude_engine.go—permissionModeValueIndexstill points to the correct slice position after subsequent appends;buildClaudeCliArgsreturn order matches call-site destructuring. ✓compiler_safe_outputs_job.go—buildPreambleTokenStepsmutatesoutputsas a side effect; the godoc notes it. The droppedseenNeeds[preActName] = struct{}{}at the end ofbuildSafeOutputsJobNeedsis benign —seenNeedsis unused after that point. ✓compiler_yaml_prompt.go—enrichExpressionMappingscalled beforebuildMainWorkflowPromptChunkspreserves original ordering (imports → main extraction → filter → experiments → inline embed).mergeKnownNeedsExpressionsprecedence (alloverridesknownNeeds) matches original map-merge semantics. ✓tools.go—detectTriggerTypeshort-circuits viaif data.On != "" { return false, false }; no redundant file reads. The defer-extractedpopulateWorkflowDataCachefires on every return path. ✓trigger_parser.go—parseLockForAgentFromOnMapcalled unconditionally fromparseOnMapPreamble, matching the original position. UnifiedparseCommandTriggerFromOnMapcorrectly includestriggerKeyin error messages. ✓
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 168.1 AIC · ⌖ 4.63 AIC · ⊞ 5.7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting with non-blocking improvements.
📋 Key Themes & Highlights
Key Themes
- Missing regression test: The
len(data.LabelCommand) > 0bug fix has no accompanying test — that's the highest-risk finding. - Magic constant fragility:
insertIndex += 6incalculatePreambleInsertIndexis implicitly coupled to the staging download step's YAML line count. - Builder with hidden mutation:
buildLabelCommandEventsMapsecretly writes todata.HasDispatchItemNumber— surprises callers. seenNeedsmap consistency: ThepreActNameupdate was accidentally dropped during extraction.- Free-function vs method inconsistency:
detectTriggerTypestands apart from all its method-based siblings.
Positive Highlights
- ✅ Excellent extraction discipline: every new helper has a clear, single responsibility and a doc comment.
- ✅ The
resolveClaudePermissionModeandappendClaudeCustomEngineArgssplit is a clean seam — both are independently testable. - ✅
buildSafeOutputsJobConditionreturning aConditionNode(no more mutation at the call site) is a real improvement. - ✅
mergeLabelCommandOtherEventsas a standalone free function makes the merge logic easy to unit-test. - ✅ Lock-file drift catch during recompile is a great safety net for catching behaviour regressions.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 123.9 AIC · ⌖ 6.02 AIC · ⊞ 6.7K
Comment /matt to run again
| maps.Copy(commandEventsMap, data.CommandOtherEvents) | ||
| } | ||
| // Merge label events if label_command is configured alongside slash_command. | ||
| if len(data.LabelCommand) > 0 { |
There was a problem hiding this comment.
[/tdd] The bug fix (len(data.LabelCommand) > 0 guard in buildCommandTriggerEventsMap) is not covered by a regression test — without one, this exact bug could re-emerge silently.
💡 Suggested test skeleton
func TestBuildCommandTriggerEventsMap_SlashCommandOnly_NoLabeledEvent(t *testing.T) {
// Arrange: slash_command-only workflow (no LabelCommand)
data := &WorkflowData{
Command: []Command{{Name: "deploy"}},
}
c := &Compiler{}
eventsMap, err := c.buildCommandTriggerEventsMap(data)
require.NoError(t, err)
for _, v := range eventsMap {
if m, ok := v.(map[string]any); ok {
types, _ := m["types"].([]any)
require.NotContains(t, types, "labeled",
"slash_command-only must not emit spurious labeled event")
}
}
}This is exactly the regression path described in the PR body.
@copilot please address this.
There was a problem hiding this comment.
Added two regression tests in pkg/workflow/tools_events_test.go (commit fix(pkg/workflow): address 5 review threads):
TestBuildCommandTriggerEventsMap_SlashCommandOnly_NoLabeledEvent— verifies nolabeledtype appears whenLabelCommandis emptyTestBuildCommandTriggerEventsMap_WithLabelCommand_IncludesLabeledEvent— verifieslabeledIS included whenLabelCommandis configured
| insertIndex += len(buildAgentOutputDownloadSteps(agentArtifactPrefix, c.getActionPin)) | ||
| if data.SafeOutputs.UploadArtifact != nil { | ||
| // The staging download step has 6 YAML string entries. | ||
| insertIndex += 6 |
There was a problem hiding this comment.
[/codebase-design] The magic constant 6 is a fragile coupling between calculatePreambleInsertIndex and the staging download step's internal line count. If the step ever gains or loses a YAML line, the insertion index silently drifts, causing out-of-order token minting.
💡 Suggestion
Replace the inline hardcoded count with a named constant or a helper that returns the actual step slice:
const uploadArtifactStagingDownloadStepLineCount = 6 // name:, continue-on-error:, uses:, with:, name:, path:Or, better, build the staging download step as a []string via a dedicated function and use len(...) — the same pattern already used for setup steps and artifact download steps — so the count stays self-consistent automatically.
@copilot please address this.
There was a problem hiding this comment.
Replaced the inline 6 with a named constant uploadArtifactStagingDownloadStepCount = 6 defined at the top of compiler_safe_outputs_job.go, with a doc comment explaining which YAML entries it counts and that it must stay in sync with the literal slice in buildPreambleTokenSteps.
| return nil | ||
| // buildLabelCommandEventsMap builds the events map for a label_command trigger workflow. | ||
| // In decentralized mode it uses workflow_dispatch; in standard mode it generates label events. | ||
| func (c *Compiler) buildLabelCommandEventsMap(data *WorkflowData) map[string]any { |
There was a problem hiding this comment.
[/codebase-design] buildLabelCommandEventsMap is named as a builder but mutates data.HasDispatchItemNumber as a side effect. This breaks the principle of narrow interfaces: callers won't expect a function that returns a map to also change workflow state.
💡 Suggestion
Return the mutation as a second return value instead of writing directly to data:
func (c *Compiler) buildLabelCommandEventsMap(data *WorkflowData) (map[string]any, bool) {
...
hasDispatchItemNumber := ensureWorkflowDispatchItemNumberInput(labelEventsMap)
return labelEventsMap, hasDispatchItemNumber
}Then the caller (applyLabelCommandTriggerOnSection) applies it explicitly:
labelEventsMap, hasDispatch := c.buildLabelCommandEventsMap(data)
if hasDispatch {
data.HasDispatchItemNumber = true
}This keeps builder functions pure and makes the state mutation visible at the call site.
@copilot please address this.
There was a problem hiding this comment.
Changed buildLabelCommandEventsMap to return (map[string]any, bool) — the bool indicates whether a workflow_dispatch item-number input was added. The caller (applyLabelCommandTriggerOnSection) now applies data.HasDispatchItemNumber = true explicitly when the returned bool is true, making the state mutation visible at the call site.
| } | ||
| // detectTriggerType checks whether the workflow is a slash_command/command or label_command trigger | ||
| // by inspecting already-parsed WorkflowData fields and, if needed, reading the frontmatter from disk. | ||
| func detectTriggerType(data *WorkflowData, markdownPath string) (isCommand, isLabelCommand bool) { |
There was a problem hiding this comment.
[/codebase-design] detectTriggerType is a package-level function but all its sibling helpers (applyDefaultOnSection, applyCommandTriggerOnSection, etc.) are *Compiler methods. This inconsistency makes the module harder to navigate: readers expect either all helpers to be methods or all to be free functions, not a mix.
💡 Suggestion
detectTriggerType doesn't use any Compiler state, so it's intentionally a free function — that's fine. Adding a comment makes the intent explicit:
// detectTriggerType is a free function (not a Compiler method) because it reads
// only WorkflowData and the on-disk frontmatter; it has no dependency on Compiler state.
func detectTriggerType(data *WorkflowData, markdownPath string) (isCommand, isLabelCommand bool) {Alternatively, convert it to a method for consistency (func (c *Compiler) detectTriggerType(...)) even if c is unused — matching every other trigger helper in the file.
@copilot please address this.
There was a problem hiding this comment.
Added a doc comment to detectTriggerType explicitly stating it is intentionally a free function (not a *Compiler method) because it reads only WorkflowData and on-disk frontmatter and has no dependency on Compiler state.
| if data.SafeOutputs != nil && messagesContainPreActivationRef(data.SafeOutputs.Messages) { | ||
| if _, exists := c.jobManager.GetJob(string(constants.PreActivationJobName)); exists { | ||
| preActName := string(constants.PreActivationJobName) | ||
| if !setutil.Contains(seenNeeds, preActName) { |
There was a problem hiding this comment.
[/codebase-design] The seenNeeds[preActName] = struct{}{} update was removed when buildSafeOutputsJobNeeds was extracted. Since preActName is the last item ever appended and the map is local to the function, this is harmless in the current code — but leaving the map update out is inconsistent with how every other needs entry is tracked and could silently allow duplicates if the block is ever extended.
💡 Suggestion
Add the tracking line back for consistency:
needs = append(needs, preActName)
seenNeeds[preActName] = struct{}{} // keep map consistent with all other appends@copilot please address this.
There was a problem hiding this comment.
Restored seenNeeds[preActName] = struct{}{} immediately after the needs = append(needs, preActName) line, keeping the deduplication map consistent with every other append in buildSafeOutputsJobNeeds.
|
@copilot run pr-finishser skill |
- Add regression test for LabelCommand guard in buildCommandTriggerEventsMap
- Replace magic constant 6 with uploadArtifactStagingDownloadStepCount
- Return hasDispatchItemNumber from buildLabelCommandEventsMap instead of mutating data
- Add comment explaining detectTriggerType is intentionally a free function
- Restore seenNeeds[preActName] = struct{}{} tracking in buildSafeOutputsJobNeeds
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🚀 Looks great, Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
|
🎉 This pull request is included in a new release. Release: |
Daily
largefunclint scan flagged 5 functions inpkg/workflowranging from 258–373 lines (limit: 60). This refactor extracts cohesive sub-responsibilities into focused helpers, preserving all behavior.Changes by file
claude_engine.go—GetExecutionSteps(373→~55 lines)Extracted:
buildClaudeCliArgs,resolveClaudePermissionMode,appendClaudeCustomEngineArgs,buildClaudeCommandString,buildClaudeFullCommand,buildClaudeCommandEnv,buildClaudeBaseEnvMap,applyClaudeTimeoutEnvVars,applyClaudeModelEnvVarstools.go—applyDefaults(340→~45 lines)Extracted:
populateWorkflowDataCache(defer body),detectTriggerType,applyDefaultOnSection,applyDefaultPermissionsAlso reduced two previously-extracted helpers that were themselves over limit:
applyCommandTriggerOnSection(93→~50 lines) viabuildCommandTriggerEventsMapapplyLabelCommandTriggerOnSection(81→~35 lines) viabuildLabelCommandEventsMap+mergeLabelCommandOtherEventstrigger_parser.go—parseOnSection(258→~42 lines)Extracted:
parseOnMapPreamble(stop-after/reaction/status-comment/lock-for-agent),parseCommandTriggerFromOnMap(unifiesslash_command+ deprecatedcommandpaths),parseLabelCommandFromOnMap,finalizeCommandTriggerStatecompiler_safe_outputs_job.go—buildSafeOutputsJobFromParts(271→~55 lines)Extracted:
buildPreambleTokenSteps,calculatePreambleInsertIndex,insertPreambleTokenStepsIntoSteps,appendFinalSafeOutputStepscompiler_yaml_prompt.go—generatePrompt(284→~37 lines)Extracted:
enrichExpressionMappings(main-markdown extraction + activation filter + experiments),buildMainWorkflowPromptChunks(inline embed vs runtime-import),mergeKnownNeedsExpressionsBug fixed
buildCommandTriggerEventsMapwas missing thelen(data.LabelCommand) > 0guard before merging label events, causing spuriouslabeledevent types to appear inslash_command-only workflows. Caught via lock-file drift check during recompile.