diff --git a/.github/aw/actions-lock.json b/.github/aw/actions-lock.json index b1b9be1fbba..1aeb18aac35 100644 --- a/.github/aw/actions-lock.json +++ b/.github/aw/actions-lock.json @@ -165,6 +165,11 @@ } }, "containers": { + "ghcr.io/fabio-rovai/open-ontologies:latest": { + "image": "ghcr.io/fabio-rovai/open-ontologies:latest", + "digest": "sha256:2932c10682eac29ccf840a6bd6c4c7c82c5ce770ad9e94697d44057187452530", + "pinned_image": "ghcr.io/fabio-rovai/open-ontologies:latest@sha256:2932c10682eac29ccf840a6bd6c4c7c82c5ce770ad9e94697d44057187452530" + }, "ghcr.io/github/gh-aw-firewall/agent:0.27.43": { "image": "ghcr.io/github/gh-aw-firewall/agent:0.27.43", "digest": "sha256:04e2d1987a565000a8f114b89d806ae7a3864dd4f944be65275b28c93d8690e6", @@ -264,11 +269,6 @@ "image": "python:alpine", "digest": "sha256:26730869004e2b9c4b9ad09cab8625e81d256d1ce97e72df5520e806b1709f92", "pinned_image": "python:alpine@sha256:26730869004e2b9c4b9ad09cab8625e81d256d1ce97e72df5520e806b1709f92" - }, - "ghcr.io/fabio-rovai/open-ontologies:latest": { - "image": "ghcr.io/fabio-rovai/open-ontologies:latest", - "digest": "sha256:2932c10682eac29ccf840a6bd6c4c7c82c5ce770ad9e94697d44057187452530", - "pinned_image": "ghcr.io/fabio-rovai/open-ontologies:latest@sha256:2932c10682eac29ccf840a6bd6c4c7c82c5ce770ad9e94697d44057187452530" } } } diff --git a/docs/adr/50638-guard-dispatch-input-forwarding-with-lock-file-schema-check.md b/docs/adr/50638-guard-dispatch-input-forwarding-with-lock-file-schema-check.md new file mode 100644 index 00000000000..6ed2ce38cbd --- /dev/null +++ b/docs/adr/50638-guard-dispatch-input-forwarding-with-lock-file-schema-check.md @@ -0,0 +1,48 @@ +# ADR-50638: Guard workflow_dispatch Input Forwarding with Lock File Schema Check + +**Date**: 2026-08-05 +**Status**: Draft +**Deciders**: pelikhan, copilot-swe-agent + +--- + +### Context + +`gh aw trial` supports mixed-trigger trials where an entity-triggered workflow (which declares an `issue_number` `workflow_dispatch` input) runs alongside a scheduled or global companion workflow (which does not). Prior to this change, the `triggerWorkflowRun` function appended `--field issue_number=…` unconditionally for every workflow in the trial whenever a `--trigger-context` was provided. + +GitHub's `workflow_dispatch` API rejects requests that include undeclared inputs with `HTTP 422: Unexpected inputs provided`. The failure was silent: the companion workflow was installed but never dispatched, with no user-visible indication that the trial had partially failed. The compiled `.lock.yml` file — already written to disk before dispatch — contains the full `on.workflow_dispatch.inputs` schema and is the authoritative source of truth for what each workflow accepts. + +### Decision + +We will parse each workflow's compiled lock file at dispatch time to check whether it declares the requested `workflow_dispatch` input before forwarding it. A new `workflowDeclaresDispatchInput(lockFilePath, inputName)` function reads and parses the lock file's `on.workflow_dispatch.inputs` map; if the input is absent (or the file is missing or unreadable), the function returns `false` and the input is silently omitted rather than forwarded. This fail-safe direction ensures that no workflow ever receives an undeclared input. + +### Alternatives Considered + +#### Alternative 1: Reject mixed trigger types before installation + +Detect at the start of a trial that the selected workflows have incompatible trigger types and abort with an error message before any installation occurs. This would provide the clearest user feedback but would break the documented use case of pairing an issue-triggered workflow with a scheduled companion, which is an intentional and supported pattern. + +#### Alternative 2: Strip trigger-derived inputs at a higher level + +Remove all trigger-derived input forwarding from `executeTrialRun` for workflows whose names are not matched by the entity-triggered pattern, without consulting the lock file. This avoids per-dispatch I/O but introduces a naming-convention dependency and would silently drop inputs for any future workflow type that legitimately accepts `issue_number` without matching the entity pattern. + +### Consequences + +#### Positive +- Mixed-trigger trials (entity-triggered workflow + scheduled companion) now dispatch correctly without HTTP 422 errors. +- The lock file is already present locally at dispatch time, so no additional network calls are required. +- Fail-safe semantics: parse or read errors return `false`, ensuring inputs are never forwarded to workflows that would reject them. +- Behavior is fully tested: declared, undeclared, no-inputs, schedule-only, and missing-file cases are all covered. + +#### Negative +- Each workflow dispatch now incurs an additional file read and YAML parse of the lock file, adding minor I/O overhead at dispatch time. +- When the lock file is missing or malformed, the input is silently dropped with only a log entry; the operator may not notice that the trigger context was not forwarded. +- The approach is specific to `issue_number`; forwarding any future trigger-derived inputs will require the same guard to be extended or generalized. + +#### Neutral +- The `triggerWorkflowRun` signature gains a `lockFilePath` parameter, which changes the internal API surface and requires callers to construct the path. +- Verbose mode now emits an informational message when an input is omitted rather than silently skipping it, improving debuggability. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/trial_helpers.go b/pkg/cli/trial_helpers.go index 3e3cfa3730b..caa0fef8082 100644 --- a/pkg/cli/trial_helpers.go +++ b/pkg/cli/trial_helpers.go @@ -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 @@ -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) } @@ -189,25 +191,33 @@ 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)) } - // Trigger workflow using gh CLI - lockFileName := workflowName + ".lock.yml" + // Trigger workflow using gh CLI. + // Derive lockFileName from lockFilePath so both the declaration check and + // the dispatch invocation always reference the same compiled file. + lockFileName := filepath.Base(lockFilePath) // 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. + // 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")) @@ -269,6 +279,39 @@ func parseIssueSpec(input string) string { return "" } +// 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). +// A missing file is treated as a safe failure; a parse error on an existing file is +// surfaced as a warning since it indicates a compiler or format problem. +func workflowDeclaresDispatchInput(lockFilePath, inputName string) bool { + content, err := os.ReadFile(lockFilePath) + if err != nil { + if !os.IsNotExist(err) { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Could not read lock file %s: %v", lockFilePath, err))) + } + 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 { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Could not parse lock file %s: %v", lockFilePath, err))) + trialLog.Printf("Failed to parse lock file %s: %v", lockFilePath, err) + return false + } + + _, ok := parsed.On.WorkflowDispatch.Inputs[inputName] + return ok +} + // saveTrialResult saves a trial result to a JSON file func saveTrialResult(filename string, result any, verbose bool) error { jsonBytes, err := json.MarshalIndent(result, "", " ") diff --git a/pkg/cli/trial_issue_mode_test.go b/pkg/cli/trial_issue_mode_test.go index f8a2948f172..31d126ec031 100644 --- a/pkg/cli/trial_issue_mode_test.go +++ b/pkg/cli/trial_issue_mode_test.go @@ -3,6 +3,7 @@ package cli import ( + "os" "testing" ) @@ -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") + } + }) +}