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
10 changes: 5 additions & 5 deletions .github/aw/actions-lock.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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"
}
}
}
Original file line number Diff line number Diff line change
@@ -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.*
59 changes: 51 additions & 8 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,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.

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,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).

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)))
}

// 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

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.

}

// saveTrialResult saves a trial result to a JSON file
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, "", " ")
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