Refactor dispatch/call workflow duplication with shared input, tool, and resolver helpers#33947
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors duplicated logic across the dispatch-workflow and call-workflow safe-output paths by consolidating common workflow-input extraction, tool-definition construction, and workflow file-extension resolution into shared helpers, with focused unit tests added for the new helpers.
Changes:
- Added shared trigger-aware input extraction helpers for YAML workflows and Markdown frontmatter, and rewired existing dispatch/call extractors to use them.
- Added shared safe-output helper utilities to generate MCP tool definitions and to resolve workflow file extension priority (
.lock.yml > .yml > .md→.lock.yml), then refactored dispatch/call paths to delegate. - Updated a generated workflow lockfile as part of compilation output refresh.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/workflow_inputs_extractor.go | Introduces shared trigger-aware input extraction helpers for YAML and Markdown frontmatter. |
| pkg/workflow/workflow_inputs_extractor_test.go | Adds unit tests validating trigger-aware input extraction behavior. |
| pkg/workflow/safe_outputs_workflow_helpers.go | Adds shared helpers for MCP tool definition generation and workflow extension resolution. |
| pkg/workflow/safe_outputs_workflow_helpers_test.go | Adds tests for workflow extension resolution priority behavior. |
| pkg/workflow/safe_outputs_dispatch.go | Refactors dispatch safe-output tool generation and file-extension resolution to use shared helpers. |
| pkg/workflow/safe_outputs_call_workflow.go | Refactors call-workflow safe-output tool generation and file-extension resolution to use shared helpers. |
| pkg/workflow/dispatch_workflow_validation.go | Switches workflow_dispatch input extraction to shared YAML helper. |
| pkg/workflow/dispatch_workflow_file_resolver.go | Switches Markdown workflow_dispatch input extraction to shared Markdown helper. |
| pkg/workflow/call_workflow_validation.go | Switches workflow_call input extraction (YAML/MD/parsed) to shared helpers. |
| .github/workflows/dataflow-pr-discussion-dataset.lock.yml | Updates generated workflow lockfile output (compiler/regeneration effects). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 0
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Excellent refactoring — clean consolidation of duplicated logic
This PR successfully eliminates the semantic duplication between dispatch and call workflow paths while maintaining behavior. The new helpers are well-tested and the consolidation is straightforward.
✅ Strengths
- Clear separation of concerns: Input extraction, tool generation, and file resolution are now separate, reusable helpers
- Comprehensive test coverage: Both new helper modules have focused tests covering trigger-aware extraction and extension priority
- Minimal behavior change: The refactoring preserves existing logic — just removes repetition
- Good naming: Function names like
extractInputsFromParsedWorkflow,generateWorkflowToolDefinition, andresolveWorkflowExtensionare self-documenting
📊 Review Impact
- 10 files changed: +364 -254 lines (net reduction of duplication)
- 2 new test files: Focused regression coverage for shared helpers
- Zero blocking issues found
🔎 Code quality review by PR Code Quality Reviewer · ● 760.7K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /improve-codebase-architecture, /zoom-out, and /tdd — solid refactoring with opportunities to strengthen documentation and test coverage.
📋 Key Themes & Highlights
Key Themes
- Documentation gaps: New shared helpers lack godoc — this is a great refactoring, but undocumented shared functions can create confusion
- Test coverage: Core consolidation function
generateWorkflowToolDefinitionneeds direct tests to verify dispatch vs. call differences are preserved - Naming clarity:
workflowToolDefinitionOptionscould be more domain-specific (considermcpWorkflowToolOptionsor similar)
Positive Highlights
- ✅ Excellent DRY refactoring — consolidating 200+ lines of duplication into focused helpers
- ✅ Clear trigger parameterization —
extractInputsFromParsedWorkflow(workflow, trigger)makes intent explicit - ✅ Preserved behavioral differences — dispatch vs. call retain their distinct metadata keys via
metadataKeyparameter - ✅ Good test structure — table-driven tests with clear naming
- ✅ Clean extension resolution —
resolveWorkflowExtension()eliminates repeated priority logic
🎯 Matt Pocock Skills Applied
/improve-codebase-architecture: This refactoring creates deep modules (simple interfaces, rich behavior). The shared helpers have clear, focused responsibilities. However, domain language could be strengthened — names like workflowToolDefinitionOptions are generic; consider emphasizing the MCP tool or safe-output context.
/zoom-out: The consolidation fits the broader architecture well. Dispatch and call workflows share 90% of their logic but differ in metadata routing keys — the metadataKey parameter elegantly preserves this distinction. The refactoring doesn't introduce new abstractions unnecessarily.
/tdd: Existing tests are well-structured (table-driven, trigger-aware), but the highest-impact function — generateWorkflowToolDefinition — lacks direct coverage. Testing this function would provide confidence that the consolidation preserves dispatch vs. call differences.
Summary: This is a clean, well-motivated refactoring that eliminates significant duplication. The inline comments highlight opportunities to strengthen documentation and test coverage — addressing these will make the shared helpers easier to maintain and extend. Great work! 🎉
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 1.2M
|
|
||
| triggerConfig, hasTriggerConfig := onMap[trigger] | ||
| if !hasTriggerConfig { | ||
| return make(map[string]any) |
There was a problem hiding this comment.
[/improve-codebase-architecture] Missing documentation — this is a new shared module, so godoc is essential.
💡 Suggested documentation
Add package-level and function-level documentation:
// Package workflow provides workflow compilation and validation utilities.
// extractInputsFromParsedWorkflow extracts workflow trigger inputs from a parsed
// workflow map. The trigger parameter specifies which trigger to extract inputs
// from (e.g., "workflow_dispatch", "workflow_call").
//
// Returns an empty map if the trigger or inputs are not found.
func extractInputsFromParsedWorkflow(workflow map[string]any, trigger string) map[string]any {This is a core extraction function that will be used across multiple modules — documenting the trigger parameter and return behavior prevents confusion.
| import ( | ||
| "fmt" | ||
| "sort" | ||
|
|
There was a problem hiding this comment.
[/improve-codebase-architecture] Type name lacks domain context — workflowToolDefinitionOptions is too generic.
💡 Suggested naming
Consider one of these domain-specific names:
// Option 1: Emphasize the MCP tool aspect
type mcpWorkflowToolOptions struct {
// Option 2: Emphasize the safe-output aspect
type safeOutputToolOptions struct {
// Option 3: Be explicit about both
type workflowMCPToolConfig struct {The current name "options" suggests configuration, but this is actually the definition of an MCP tool for workflow triggers. Using "options" for a struct that produces a tool schema can be confusing — consider Config or drop the suffix entirely.
| type workflowToolDefinitionOptions struct { | ||
| workflowName string | ||
| workflowInputs map[string]any | ||
| descriptionFormat string |
There was a problem hiding this comment.
[/zoom-out] Exported function lacks documentation.
💡 Suggested documentation
// generateWorkflowToolDefinition creates an MCP tool definition for workflow
// dispatch or call operations. The returned tool schema includes normalized
// naming, input parameter validation, and metadata for handler routing.
//
// The metadataKey parameter determines which internal field is used for routing
// (e.g., "_workflow_name" for dispatch, "_call_workflow_name" for call).
func generateWorkflowToolDefinition(opts workflowToolDefinitionOptions) map[string]any {This function is central to the refactoring — documenting its purpose and the significance of metadataKey helps future maintainers understand why dispatch and call use different keys.
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) |
There was a problem hiding this comment.
[/tdd] Missing test coverage for generateWorkflowToolDefinition — only resolveWorkflowExtension is tested.
💡 Suggested test cases
Add tests for the core tool generation function:
func TestGenerateWorkflowToolDefinition(t *testing.T) {
tests := []struct {
name string
opts workflowToolDefinitionOptions
assertFn func(*testing.T, map[string]any)
}{
{
name: "generates dispatch tool with required inputs",
opts: workflowToolDefinitionOptions{
workflowName: "test-workflow",
workflowInputs: map[string]any{
"environment": map[string]any{"type": "string", "required": true},
},
descriptionFormat: "Dispatch '%s'",
metadataKey: "_workflow_name",
},
assertFn: func(t *testing.T, tool map[string]any) {
assert.Equal(t, "test_workflow", tool["name"])
assert.Equal(t, "test-workflow", tool["_workflow_name"])
assert.Contains(t, tool["description"], "Dispatch 'test-workflow'")
},
},
{
name: "generates call tool with different metadata key",
opts: workflowToolDefinitionOptions{
workflowName: "reusable-workflow",
workflowInputs: map[string]any{},
descriptionFormat: "Call '%s'",
metadataKey: "_call_workflow_name",
},
assertFn: func(t *testing.T, tool map[string]any) {
assert.Equal(t, "reusable-workflow", tool["_call_workflow_name"])
assert.NotContains(t, tool, "_workflow_name")
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
tool := generateWorkflowToolDefinition(tt.opts)
tt.assertFn(t, tool)
})
}
}This is the highest-impact function in the refactoring — testing it ensures the consolidation preserves dispatch vs. call differences.
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) |
There was a problem hiding this comment.
[/tdd] Good trigger-aware test coverage, but missing edge cases.
💡 Additional test cases to consider
Add tests for error paths and malformed input:
func TestExtractInputsFromParsedWorkflow_EdgeCases(t *testing.T) {
tests := []struct {
name string
workflow map[string]any
trigger string
expected map[string]any
}{
{
name: "missing 'on' section",
workflow: map[string]any{},
trigger: "workflow_dispatch",
expected: map[string]any{},
},
{
name: "trigger exists but has no inputs",
workflow: map[string]any{
"on": map[string]any{
"workflow_dispatch": map[string]any{},
},
},
trigger: "workflow_dispatch",
expected: map[string]any{},
},
{
name: "inputs is not a map",
workflow: map[string]any{
"on": map[string]any{
"workflow_dispatch": map[string]any{
"inputs": "invalid",
},
},
},
trigger: "workflow_dispatch",
expected: map[string]any{},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := extractInputsFromParsedWorkflow(tt.workflow, tt.trigger)
assert.Equal(t, tt.expected, result)
})
}
}These edge cases protect against malformed workflow files that could cause runtime panics.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Test Inflation Analysis
💡 Suggestions for Improvement (non-blocking)Missing Assertion MessagesAll three test functions use bare assertions without descriptive messages. While this doesn't affect the test quality score, adding messages improves debuggability when tests fail. Examples from the code: // Current
assert.Equal(t, tt.expected, extension)
assert.Contains(t, dispatchInputs, "environment")
// Suggested
assert.Equal(t, tt.expected, extension, "extension should match expected resolution priority")
assert.Contains(t, dispatchInputs, "environment", "workflow_dispatch should include environment input")Impact: Low severity — tests are otherwise excellent, but assertion failures will be easier to diagnose with context messages. Project guideline: "Every testify/stdlib assertion in this codebase should have a descriptive message argument." Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. Highlights:
|
|
@copilot review all comments |
I reviewed all comments on this PR. There are no actionable review findings requiring code changes at this time. |
pelikhan
deleted the
copilot/refactor-semantic-function-clustering-please-work
branch
Semantic clustering identified high-value duplication between
dispatch-workflowandcall-workflowsafe-output paths: repeated trigger input extraction, near-identical MCP tool generation, and duplicated workflow file resolution logic. This PR consolidates those paths into shared helpers while keeping existing entry points/wrappers intact.Shared trigger-input extraction (YAML + markdown frontmatter)
workflow_inputs_extractor.gowith:extractInputsFromYAML(workflowPath, trigger)extractInputsFromMarkdown(mdPath, trigger)extractInputsFromParsedWorkflow(workflow, trigger)on -> trigger -> inputsmap walking.Shared safe-output tool definition builder
generateWorkflowToolDefinition(...)insafe_outputs_workflow_helpers.go.generateDispatchWorkflowTool(...)generateCallWorkflowTool(...)descriptionFormat, metadata key).Shared workflow file extension resolution
resolveWorkflowExtension(fileResult) (string, bool)insafe_outputs_workflow_helpers.go.populateDispatchWorkflowFiles(...)andpopulateCallWorkflowFiles(...)to use the same.lock.yml > .yml > .md(.lock.yml)priority logic.Focused regression coverage
workflow_inputs_extractor_test.gosafe_outputs_workflow_helpers_test.go