Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
47 changes: 40 additions & 7 deletions pkg/cli/trial_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/github/gh-aw/pkg/sliceutil"
"github.com/github/gh-aw/pkg/stringutil"
"github.com/github/gh-aw/pkg/workflow"
"github.com/goccy/go-yaml"
)

// issuePathPattern matches the path portion of a GitHub issue URL: /owner/repo/issues/NUMBER
Expand Down Expand Up @@ -84,7 +85,8 @@ func executeTrialRun(ctx context.Context, parsedSpecs []*WorkflowSpec, hostRepoS
}

// Run the workflow and wait for completion (with trigger context if provided)
runID, err := triggerWorkflowRun(hostRepoSlug, parsedSpec.WorkflowName, opts.TriggerContext, opts.Verbose)
lockFilePath := filepath.Join(tempDir, constants.GetWorkflowDir(), parsedSpec.WorkflowName+".lock.yml")
runID, err := triggerWorkflowRun(hostRepoSlug, parsedSpec.WorkflowName, lockFilePath, opts.TriggerContext, opts.Verbose)
if err != nil {
return fmt.Errorf("failed to trigger workflow run for '%s': %w", parsedSpec.WorkflowName, err)
}
Expand Down Expand Up @@ -189,7 +191,7 @@ func executeTrialRun(ctx context.Context, parsedSpecs []*WorkflowSpec, hostRepoS
return nil
}

func triggerWorkflowRun(repoSlug, workflowName string, triggerContext string, verbose bool) (string, error) {
func triggerWorkflowRun(repoSlug, workflowName, lockFilePath string, triggerContext string, verbose bool) (string, error) {
trialLog.Printf("Triggering workflow run: workflow=%s, repo=%s, hasTriggerContext=%v", workflowName, repoSlug, triggerContext != "")
if verbose {
fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Triggering workflow run for: "+workflowName))
Expand All @@ -201,13 +203,19 @@ func triggerWorkflowRun(repoSlug, workflowName string, triggerContext string, ve
// Build the command args
args := []string{"workflow", "run", lockFileName, "--repo", repoSlug}

// If trigger context is provided, extract issue number and add it as input
// If trigger context is provided, extract issue number and add it as input.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lockFileName (used to invoke gh workflow run) and the caller's lockFilePath (used to inspect declared inputs) are built via two separate, unlinked expressions — a future edit to one path convention without the other will silently break input-forwarding decisions.

// Only forward the input when the compiled workflow declares an "issue_number"
// workflow_dispatch input; otherwise gh returns HTTP 422 and the run is skipped.
if triggerContext != "" {
issueNumber := parseIssueSpec(triggerContext)
if issueNumber != "" {
args = append(args, "--field", "issue_number="+issueNumber)
if verbose {
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Using issue number %s from trigger context", issueNumber)))
if workflowDeclaresDispatchInput(lockFilePath, "issue_number") {
args = append(args, "--field", "issue_number="+issueNumber)
if verbose {
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Using issue number %s from trigger context", issueNumber)))
}
} else if verbose {
fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Workflow '%s' does not declare an issue_number input, running without trigger context", workflowName)))
}
} else if verbose {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Could not extract issue number from trigger context, running without inputs"))
Expand Down Expand Up @@ -269,7 +277,32 @@ func parseIssueSpec(input string) string {
return ""
}

// saveTrialResult saves a trial result to a JSON file
// workflowDeclaresDispatchInput reports whether the compiled lock file at lockFilePath
// declares the given workflow_dispatch input. It returns false if the file cannot be
// read or parsed, so that trigger-derived inputs are not forwarded to workflows whose
// workflow_dispatch schema does not declare them (which would cause an HTTP 422).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Swallowing read/parse errors as false conflates "input genuinely not declared" with "lock file missing/corrupt/wrong path" — this silently hides real bugs (e.g. a stale or mistyped lockFilePath) rather than surfacing them.

💡 Details

workflowDeclaresDispatchInput returns false uniformly whether the file genuinely lacks the input, the file doesn't exist yet (e.g. compile step failed or path mismatch), or the YAML fails to parse for an unrelated reason. All three cases currently just log via trialLog.Printf and silently disable issue_number forwarding — which is exactly the class of bug this PR set out to fix (workflow silently never receiving the intended input), just moved one level deeper.

Consider distinguishing "file not found" (arguably fine to fail-safe) from parse errors on an existing file (should probably surface a warning to the user even outside verbose mode, since it indicates a compiler/format problem).

if verbose || err != nil {
    fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Could not parse lock file %s: %v", lockFilePath, err)))
}

func workflowDeclaresDispatchInput(lockFilePath, inputName string) bool {
content, err := os.ReadFile(lockFilePath)
if err != nil {
trialLog.Printf("Failed to read lock file %s: %v", lockFilePath, err)
return false
}

var parsed struct {
On struct {
WorkflowDispatch struct {
Inputs map[string]any `yaml:"inputs"`
} `yaml:"workflow_dispatch"`
} `yaml:"on"`
}
if err := yaml.Unmarshal(content, &parsed); err != nil {
trialLog.Printf("Failed to parse lock file %s: %v", lockFilePath, err)
return false
}

_, ok := parsed.On.WorkflowDispatch.Inputs[inputName]
return ok

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The saveTrialResult doc comment was dropped/merged into the new function's closing brace, losing the godoc entry for that function.

}
func saveTrialResult(filename string, result any, verbose bool) error {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnosing-bugs] The doc comment // saveTrialResult saves a trial result to a JSON file was dropped in this diff, and there is no blank line between workflowDeclaresDispatchInput's closing brace and saveTrialResult.

💡 Suggested fix

Restore the blank line and the comment:

	return ok
}

// saveTrialResult saves a trial result to a JSON file
func saveTrialResult(filename string, result any, verbose bool) error {

@copilot please address this.

jsonBytes, err := json.MarshalIndent(result, "", " ")
if err != nil {
Expand Down
74 changes: 74 additions & 0 deletions pkg/cli/trial_issue_mode_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package cli

import (
"os"
"testing"
)

Expand Down Expand Up @@ -195,3 +196,76 @@ func TestTrialWorkflowSpecParsing(t *testing.T) {
})
}
}

func TestWorkflowDeclaresDispatchInput(t *testing.T) {
testCases := []struct {
name string
content string
input string
expected bool
}{
{
name: "declares issue_number input",
content: `on:
workflow_dispatch:
inputs:
issue_number:
description: "Issue number"
required: false
type: string
`,
input: "issue_number",
expected: true,
},
{
name: "does not declare issue_number input",
content: `on:
workflow_dispatch:
inputs:
aw_context:
description: "Agent caller context"
required: false
type: string
`,
input: "issue_number",
expected: false,
},
{
name: "workflow_dispatch without inputs",
content: `on:
workflow_dispatch:
`,
input: "issue_number",
expected: false,
},
{
name: "no workflow_dispatch trigger",
content: `on:
schedule:
- cron: "0 0 * * *"
`,
input: "issue_number",
expected: false,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
dir := t.TempDir()
lockFile := dir + "/workflow.lock.yml"
if err := os.WriteFile(lockFile, []byte(tc.content), 0644); err != nil {
t.Fatalf("Failed to write lock file: %v", err)
}

if got := workflowDeclaresDispatchInput(lockFile, tc.input); got != tc.expected {
t.Errorf("workflowDeclaresDispatchInput() = %v, want %v", got, tc.expected)
}
})
}

t.Run("missing file returns false", func(t *testing.T) {
if workflowDeclaresDispatchInput("/nonexistent/path.lock.yml", "issue_number") {
t.Errorf("expected false for missing file")
}
})
}
Loading