Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 101 additions & 27 deletions pkg/workflow/dispatch_workflow_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"strings"

"github.com/github/gh-aw/pkg/fileutil"
"github.com/github/gh-aw/pkg/parser"
"github.com/goccy/go-yaml"
)

Expand Down Expand Up @@ -99,12 +100,26 @@ func (c *Compiler) validateDispatchWorkflow(data *WorkflowData, workflowPath str
continue // Skip further validation for this workflow
}
} else {
// Only .md exists - needs to be compiled first
compileErr := fmt.Errorf("dispatch-workflow: workflow '%s' must be compiled first\n\nThe workflow source file exists at: %s\nBut the compiled .lock.yml file is missing.\n\nTo fix:\n1. Compile the workflow: gh aw compile %s\n2. Commit the generated .lock.yml file\n3. Ensure .lock.yml files are not in .gitignore", workflowName, fileResult.mdPath, workflowName)
if returnErr := collector.Add(compileErr); returnErr != nil {
return returnErr // Fail-fast mode
// Only .md exists — it may be a same-batch compilation target.
// Validate via the .md frontmatter so a second compile pass is not required.
mdHasDispatch, checkErr := mdHasWorkflowDispatch(fileResult.mdPath)
if checkErr != nil {
readErr := fmt.Errorf("dispatch-workflow: failed to read workflow source %s: %w", fileResult.mdPath, checkErr)
if returnErr := collector.Add(readErr); returnErr != nil {
return returnErr // Fail-fast mode
}
continue // Skip further validation for this workflow
Comment on lines +105 to +111
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When mdHasWorkflowDispatch() returns an error, it can be due to frontmatter parsing (not just file I/O). Wrapping it as "failed to read workflow source" is misleading and makes debugging harder; consider wording like "failed to read or parse workflow source frontmatter" (or splitting read vs parse errors) so the reported failure matches the underlying cause.

This issue also appears on line 321 of the same file.

Copilot uses AI. Check for mistakes.
}
continue // Skip further validation for this workflow
if !mdHasDispatch {
dispatchErr := fmt.Errorf("dispatch-workflow: workflow '%s' does not support workflow_dispatch trigger (must include 'workflow_dispatch' in the 'on' section)", workflowName)
if returnErr := collector.Add(dispatchErr); returnErr != nil {
return returnErr // Fail-fast mode
}
continue // Skip further validation for this workflow
}
// .md exists with workflow_dispatch — valid same-batch compilation target.
dispatchWorkflowValidationLog.Printf("Workflow '%s' is valid for dispatch (found .md source at %s with workflow_dispatch trigger)", workflowName, fileResult.mdPath)
continue // Trigger validated; skip YAML-specific checks below
}

// Parse the workflow YAML to check for workflow_dispatch trigger
Expand All @@ -127,28 +142,7 @@ func (c *Compiler) validateDispatchWorkflow(data *WorkflowData, workflowPath str
continue // Skip further validation for this workflow
}

// Check if workflow_dispatch is in the "on" section
hasWorkflowDispatch := false
switch on := onSection.(type) {
case string:
// Simple trigger like "on: push"
if on == "workflow_dispatch" {
hasWorkflowDispatch = true
}
case []any:
// Array of triggers like "on: [push, workflow_dispatch]"
for _, trigger := range on {
if triggerStr, ok := trigger.(string); ok && triggerStr == "workflow_dispatch" {
hasWorkflowDispatch = true
break
}
}
case map[string]any:
// Map of triggers like "on: { push: {}, workflow_dispatch: {} }"
_, hasWorkflowDispatch = on["workflow_dispatch"]
}

if !hasWorkflowDispatch {
if !containsWorkflowDispatch(onSection) {
dispatchErr := fmt.Errorf("dispatch-workflow: workflow '%s' does not support workflow_dispatch trigger (must include 'workflow_dispatch' in the 'on' section)", workflowName)
if returnErr := collector.Add(dispatchErr); returnErr != nil {
return returnErr // Fail-fast mode
Expand Down Expand Up @@ -296,3 +290,83 @@ func findWorkflowFile(workflowName string, currentWorkflowPath string) (*findWor

return result, nil
}

// mdHasWorkflowDispatch reads a .md workflow file's frontmatter and reports whether
// the workflow includes a workflow_dispatch trigger in its 'on:' section.
// This is used to validate same-batch dispatch-workflow targets whose .lock.yml has
// not yet been generated.
func mdHasWorkflowDispatch(mdPath string) (bool, error) {
content, err := os.ReadFile(mdPath) // #nosec G304 -- mdPath is validated via isPathWithinDir in findWorkflowFile
if err != nil {
return false, err
}
result, err := parser.ExtractFrontmatterFromContent(string(content))
if err != nil || result == nil {
return false, err
}
onSection, hasOn := result.Frontmatter["on"]
if !hasOn {
return false, nil
}
return containsWorkflowDispatch(onSection), nil
}

// extractMDWorkflowDispatchInputs reads a .md workflow file's frontmatter and extracts
// the workflow_dispatch inputs schema, mirroring extractWorkflowDispatchInputs for .md sources.
func extractMDWorkflowDispatchInputs(mdPath string) (map[string]any, error) {
content, err := os.ReadFile(mdPath) // #nosec G304 -- mdPath is validated via isPathWithinDir in findWorkflowFile
if err != nil {
return nil, err
}
result, err := parser.ExtractFrontmatterFromContent(string(content))
if err != nil || result == nil {
return make(map[string]any), nil
}
onSection, hasOn := result.Frontmatter["on"]
if !hasOn {
return make(map[string]any), nil
}
onMap, ok := onSection.(map[string]any)
if !ok {
return make(map[string]any), nil
}
workflowDispatch, hasWorkflowDispatch := onMap["workflow_dispatch"]
if !hasWorkflowDispatch {
return make(map[string]any), nil
}
workflowDispatchMap, ok := workflowDispatch.(map[string]any)
if !ok {
return make(map[string]any), nil
}
inputs, hasInputs := workflowDispatchMap["inputs"]
if !hasInputs {
return make(map[string]any), nil
}
inputsMap, ok := inputs.(map[string]any)
if !ok {
return make(map[string]any), nil
}
return inputsMap, nil
}

// containsWorkflowDispatch reports whether the given 'on:' section value includes
// a workflow_dispatch trigger. It handles the three GitHub Actions forms:
// - string: "on: workflow_dispatch"
// - []any: "on: [push, workflow_dispatch]"
// - map[string]any: "on:\n workflow_dispatch: ..."
func containsWorkflowDispatch(onSection any) bool {
switch on := onSection.(type) {
case string:
return on == "workflow_dispatch"
case []any:
for _, trigger := range on {
if triggerStr, ok := trigger.(string); ok && triggerStr == "workflow_dispatch" {
return true
}
}
case map[string]any:
_, ok := on["workflow_dispatch"]
return ok
}
return false
}
94 changes: 75 additions & 19 deletions pkg/workflow/dispatch_workflow_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,10 @@ This workflow tries to dispatch to itself.
assert.Contains(t, errMsg, "workflow_dispatch", "Should suggest workflow_dispatch alternative")
}

// TestDispatchWorkflowErrorMessage_MustCompile tests that uncompiled workflow
// error message includes compilation instructions
func TestDispatchWorkflowErrorMessage_MustCompile(t *testing.T) {
// TestDispatchWorkflowBatchAware_MDWithDispatch tests that a workflow that only has a .md file
// (no .lock.yml) is accepted as a valid same-batch dispatch target when the .md has
// workflow_dispatch in its 'on:' section.
func TestDispatchWorkflowBatchAware_MDWithDispatch(t *testing.T) {
compiler := NewCompilerWithVersion("1.0.0")

tmpDir := t.TempDir()
Expand All @@ -190,7 +191,7 @@ func TestDispatchWorkflowErrorMessage_MustCompile(t *testing.T) {
err = os.MkdirAll(workflowsDir, 0755)
require.NoError(t, err, "Failed to create workflows directory")

// Create an uncompiled workflow (only .md file, no .lock.yml)
// Create a target workflow that only has .md (no .lock.yml) with workflow_dispatch trigger
targetWorkflow := `---
on: workflow_dispatch
engine: copilot
Expand All @@ -200,13 +201,13 @@ permissions:

# Target Workflow

This workflow needs to be compiled.
This workflow is a same-batch compilation target.
`
targetFile := filepath.Join(workflowsDir, "target.md")
err = os.WriteFile(targetFile, []byte(targetWorkflow), 0644)
require.NoError(t, err, "Failed to write target workflow")

// Create a dispatcher workflow that references the uncompiled workflow
// Create a dispatcher workflow that references the .md-only target
dispatcherWorkflow := `---
on: issues
engine: copilot
Expand All @@ -221,7 +222,7 @@ safe-outputs:

# Dispatcher Workflow

This workflow references an uncompiled workflow.
This workflow dispatches to a same-batch target.
`
dispatcherFile := filepath.Join(awDir, "dispatcher.md")
err = os.WriteFile(dispatcherFile, []byte(dispatcherWorkflow), 0644)
Expand All @@ -238,20 +239,75 @@ This workflow references an uncompiled workflow.
workflowData, err := compiler.ParseWorkflowFile("dispatcher.md")
require.NoError(t, err, "Failed to parse workflow")

// Validate the workflow - should fail with enhanced error message
// Validation should succeed: .md exists with workflow_dispatch (same-batch target)
err = compiler.validateDispatchWorkflow(workflowData, dispatcherFile)
require.Error(t, err, "Validation should fail for uncompiled workflow")
assert.NoError(t, err, "Validation should pass for .md-only same-batch target with workflow_dispatch")
}

// Verify enhanced error message content
errMsg := err.Error()
assert.Contains(t, errMsg, "must be compiled first", "Should state the requirement")
assert.Contains(t, errMsg, "target", "Should mention workflow name")
assert.Contains(t, errMsg, "source file exists", "Should acknowledge file exists")
assert.Contains(t, errMsg, "compiled .lock.yml file is missing", "Should explain what's missing")
assert.Contains(t, errMsg, "To fix:", "Should include fix instructions header")
assert.Contains(t, errMsg, "gh aw compile target", "Should show exact compilation command")
assert.Contains(t, errMsg, "Commit the generated .lock.yml", "Should mention committing")
assert.Contains(t, errMsg, ".gitignore", "Should warn about gitignore")
// TestDispatchWorkflowBatchAware_MDWithoutDispatch tests that a workflow that only has a .md file
// (no .lock.yml) and does NOT have workflow_dispatch in its 'on:' section fails validation.
func TestDispatchWorkflowBatchAware_MDWithoutDispatch(t *testing.T) {
compiler := NewCompilerWithVersion("1.0.0")

tmpDir := t.TempDir()
awDir := filepath.Join(tmpDir, ".github", "aw")
workflowsDir := filepath.Join(tmpDir, ".github", "workflows")

err := os.MkdirAll(awDir, 0755)
require.NoError(t, err, "Failed to create aw directory")
err = os.MkdirAll(workflowsDir, 0755)
require.NoError(t, err, "Failed to create workflows directory")

// Create a target workflow that only has .md (no .lock.yml) WITHOUT workflow_dispatch
targetWorkflow := `---
on: issues
engine: copilot
permissions:
contents: read
---

# Target Workflow

This workflow does not support workflow_dispatch.
`
targetFile := filepath.Join(workflowsDir, "target.md")
err = os.WriteFile(targetFile, []byte(targetWorkflow), 0644)
require.NoError(t, err, "Failed to write target workflow")

// Create a dispatcher workflow that references the .md-only target
dispatcherWorkflow := `---
on: issues
engine: copilot
permissions:
contents: read
safe-outputs:
dispatch-workflow:
workflows:
- target
max: 1
---

# Dispatcher Workflow
`
dispatcherFile := filepath.Join(awDir, "dispatcher.md")
err = os.WriteFile(dispatcherFile, []byte(dispatcherWorkflow), 0644)
require.NoError(t, err, "Failed to write dispatcher workflow")

// Change to the aw directory
oldDir, err := os.Getwd()
require.NoError(t, err, "Failed to get current directory")
err = os.Chdir(awDir)
require.NoError(t, err, "Failed to change directory")
defer func() { _ = os.Chdir(oldDir) }()

// Parse the dispatcher workflow
workflowData, err := compiler.ParseWorkflowFile("dispatcher.md")
require.NoError(t, err, "Failed to parse workflow")

// Validation should fail: .md exists but lacks workflow_dispatch
err = compiler.validateDispatchWorkflow(workflowData, dispatcherFile)
require.Error(t, err, "Validation should fail when .md target lacks workflow_dispatch")
assert.Contains(t, err.Error(), "does not support workflow_dispatch trigger", "Should explain missing trigger")
}

// TestDispatchWorkflowErrorMessage_MultipleErrors tests that multiple errors
Expand Down
29 changes: 22 additions & 7 deletions pkg/workflow/safe_outputs_generation.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,17 @@ func populateDispatchWorkflowFiles(data *WorkflowData, markdownPath string) {
continue
}

// Determine which file to use - priority: .lock.yml > .yml
// Determine which file to use - priority: .lock.yml > .yml > .md (batch target)
var extension string
if fileResult.lockExists {
extension = ".lock.yml"
} else if fileResult.ymlExists {
extension = ".yml"
} else if fileResult.mdExists {
// .md-only: the workflow is a same-batch compilation target that will produce a .lock.yml
extension = ".lock.yml"
} else {
safeOutputsConfigLog.Printf("Warning: workflow file not found for %s (only .md exists, needs compilation)", workflowName)
safeOutputsConfigLog.Printf("Warning: no workflow file found for %s (checked .lock.yml, .yml, .md)", workflowName)
continue
}

Expand Down Expand Up @@ -1314,17 +1317,23 @@ func generateFilteredToolsJSON(data *WorkflowData, markdownPath string) (string,
continue
}

// Determine which file to use - priority: .lock.yml > .yml
// Determine which file to use - priority: .lock.yml > .yml > .md (batch target)
var workflowPath string
var extension string
var useMD bool
if fileResult.lockExists {
workflowPath = fileResult.lockPath
extension = ".lock.yml"
} else if fileResult.ymlExists {
workflowPath = fileResult.ymlPath
extension = ".yml"
} else if fileResult.mdExists {
// .md-only: the workflow is a same-batch compilation target that will produce a .lock.yml
workflowPath = fileResult.mdPath
extension = ".lock.yml"
useMD = true
Comment on lines +1330 to +1334
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New .md-only handling was added here (treating a target with only target.md present as a same-batch workflow and extracting workflow_dispatch inputs from frontmatter). There doesn’t appear to be a test covering this branch for tools generation / WorkflowFiles mapping (e.g., ensuring .md-only targets map to .lock.yml and that inputs declared under on.workflow_dispatch.inputs in frontmatter are reflected in the generated tool schema). Adding a focused unit test would help prevent regressions.

Copilot uses AI. Check for mistakes.
} else {
safeOutputsConfigLog.Printf("Warning: workflow file not found for %s (only .md exists, needs compilation)", workflowName)
safeOutputsConfigLog.Printf("Warning: no workflow file found for %s (checked .lock.yml, .yml, .md)", workflowName)
// Continue with empty inputs
tool := generateDispatchWorkflowTool(workflowName, make(map[string]any))
filteredTools = append(filteredTools, tool)
Expand All @@ -1335,9 +1344,15 @@ func generateFilteredToolsJSON(data *WorkflowData, markdownPath string) (string,
data.SafeOutputs.DispatchWorkflow.WorkflowFiles[workflowName] = extension

// Extract workflow_dispatch inputs
workflowInputs, err := extractWorkflowDispatchInputs(workflowPath)
if err != nil {
safeOutputsConfigLog.Printf("Warning: failed to extract inputs for workflow %s from %s: %v", workflowName, workflowPath, err)
var workflowInputs map[string]any
var inputsErr error
if useMD {
workflowInputs, inputsErr = extractMDWorkflowDispatchInputs(workflowPath)
} else {
workflowInputs, inputsErr = extractWorkflowDispatchInputs(workflowPath)
}
if inputsErr != nil {
safeOutputsConfigLog.Printf("Warning: failed to extract inputs for workflow %s from %s: %v", workflowName, workflowPath, inputsErr)
// Continue with empty inputs
workflowInputs = make(map[string]any)
}
Expand Down
Loading