Skip to content

fix(dispatch-workflow): expose ref parameter in per-workflow tool schema when allowed-refs is configured #49754

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 5 commits into from
Aug 2, 2026
+104 −11
Merged

fix(dispatch-workflow): expose ref parameter in per-workflow tool schema when allowed-refs is configured #49754

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fe1f3f7
Initial plan
Copilot Aug 2, 2026
f33f4a1
fix: expose ref parameter in dispatch-workflow tool schema when allow…
Copilot Aug 2, 2026
b0a8d37
Merge branch 'main' into copilot/fix-dispatch-workflow-ref-parameter
github-actions[bot] Aug 2, 2026
15d949b
fix(dispatch-workflow): clean allowed refs text and tighten required …
Copilot Aug 2, 2026
30362bd
Merge remote-tracking branch 'origin/main' into copilot/fix-dispatch-…
Copilot Aug 2, 2026
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
32 changes: 29 additions & 3 deletions pkg/workflow/safe_outputs_dispatch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package workflow

import (
"fmt"
"strings"

"github.com/github/gh-aw/pkg/logger"
)

Expand Down Expand Up @@ -94,15 +97,38 @@ func workflowHasAwContextInput(fileResult *findWorkflowFileResult, workflowName
// generateDispatchWorkflowTool generates an MCP tool definition for a specific workflow.
// The tool will be named after the workflow (normalized to underscores) and accept
// the workflow's defined workflow_dispatch inputs as parameters.
func generateDispatchWorkflowTool(workflowName string, workflowInputs map[string]any) map[string]any {
safeOutputsDispatchWorkflowLog.Printf("Generating dispatch-workflow tool: workflow=%s, inputs=%d", workflowName, len(workflowInputs))
// When allowedRefs is non-empty, a 'ref' parameter is added to let the agent
// specify which branch/tag/SHA to dispatch to, validated against the configured globs.
func generateDispatchWorkflowTool(workflowName string, workflowInputs map[string]any, allowedRefs []string) map[string]any {
safeOutputsDispatchWorkflowLog.Printf("Generating dispatch-workflow tool: workflow=%s, inputs=%d, allowedRefs=%d", workflowName, len(workflowInputs), len(allowedRefs))

descriptionFormat := "Dispatch the '%s' workflow with workflow_dispatch trigger. This workflow must support workflow_dispatch and be in .github/workflows/ directory in the same repository."

tool := generateWorkflowToolDefinition(workflowToolDefinitionOptions{
workflowName: workflowName,
workflowInputs: workflowInputs,
descriptionFormat: "Dispatch the '%s' workflow with workflow_dispatch trigger. This workflow must support workflow_dispatch and be in .github/workflows/ directory in the same repository.",
descriptionFormat: descriptionFormat,
metadataKey: "_workflow_name",
})

@github-actions github-actions Bot Aug 2, 2026

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 type assertions on tool["inputSchema"] and inputSchema["properties"] ignore the ok bool, so a future shape change in generateWorkflowToolDefinition would turn into a nil-map write panic here instead of a clear error.

💡 Details

properties, _ := inputSchema["properties"].(map[string]any) discards the failure case. Today generateWorkflowToolDefinition always returns a non-nil properties map (via buildInputSchema), so this is currently safe, but the coupling is implicit and undocumented — nothing enforces that invariant at this call site. If generateWorkflowToolDefinition is refactored later (e.g. to omit properties when there are no inputs, which would be a reasonable change), properties["ref"] = ... panics on a nil map write with an unhelpful stack trace.

Suggested hardening:

inputSchema, ok := tool["inputSchema"].(map[string]any)
if !ok {
    safeOutputsDispatchWorkflowLog.Printf("Warning: unexpected tool shape for %s, skipping ref injection", workflowName)
} else if properties, ok := inputSchema["properties"].(map[string]any); ok {
    properties["ref"] = map[string]any{...}
}


// When allowed-refs is configured, inject a 'ref' property so the agent can
// specify the target branch/tag/SHA. The runtime handler validates the value
// against the configured glob patterns before dispatching.
if len(allowedRefs) > 0 {
inputSchema, _ := tool["inputSchema"].(map[string]any)
properties, _ := inputSchema["properties"].(map[string]any)
allowedRefsDesc := strings.Join(allowedRefs, ", ")

refDesc := fmt.Sprintf("The git ref (branch, tag, or SHA) to dispatch the workflow on. Must match one of the configured allowed ref patterns: %s. If omitted, the dispatching workflow's ref is used.", allowedRefsDesc)
properties["ref"] = map[string]any{

@github-actions github-actions Bot Aug 2, 2026

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.

[/codebase-design] Using %v on []string in user-facing descriptions produces Go-style [silencer/* refs/heads/main] rather than a clean comma-separated list. Agents reading the MCP tool schema will see Go syntax noise.

💡 Suggested fix

Replace both %v calls with strings.Join:

refDesc := fmt.Sprintf(
    "The git ref ... allowed ref patterns: %s. If omitted ...",
    strings.Join(allowedRefs, ", "),
)
tool["description"] = desc + fmt.Sprintf(
    " Use the 'ref' parameter to target a specific branch or tag (allowed patterns: %s).",
    strings.Join(allowedRefs, ", "),
)

This also means the test assertions (assert.Contains(t, ..., "silencer/*")) will still pass, so no test changes are needed.

@copilot please address this.

"type": "string",
"description": refDesc,
}
Comment on lines +123 to +126

@github-actions github-actions Bot Aug 2, 2026

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.

%v formatting of []string (e.g. [silencer/* refs/heads/main]) is duplicated verbatim in both the ref property description and the tool description, bloating the schema shown to the agent.

💡 Details

fmt.Sprintf("...%v", allowedRefs) renders as a Go slice literal with square brackets and space-separated entries, not a natural-language list (e.g. silencer/*, refs/heads/main). This is harder for an LLM (and humans) to parse cleanly, especially if a pattern itself contains a space. The same list is also repeated twice — once in refDesc and again in the appended tool["description"] — adding redundant tokens to every dispatch tool's description with no added information.

Suggested fix: format with strings.Join(allowedRefs, ", ") and drop the duplicate mention in the outer description (or make the outer sentence generic, e.g. "Use the 'ref' parameter to target a specific branch or tag; see parameter description for allowed patterns.").


desc, _ := tool["description"].(string)
tool["description"] = desc + fmt.Sprintf(" Use the 'ref' parameter to target a specific branch or tag (allowed patterns: %s).", allowedRefsDesc)
}

inputSchema, _ := tool["inputSchema"].(map[string]any)
properties, _ := inputSchema["properties"].(map[string]any)
requiredCount := 0
Expand Down
6 changes: 3 additions & 3 deletions pkg/workflow/safe_outputs_tools_generation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func generateDynamicTools(data *WorkflowData, markdownPath string) ([]map[string
fileResult, err := findWorkflowFile(workflowName, markdownPath)
if err != nil {
safeOutputsConfigLog.Printf("Warning: error finding workflow %s: %v", workflowName, err)
dynamicTools = append(dynamicTools, generateDispatchWorkflowTool(workflowName, make(map[string]any)))
dynamicTools = append(dynamicTools, generateDispatchWorkflowTool(workflowName, make(map[string]any), data.SafeOutputs.DispatchWorkflow.AllowedRefs))
continue
}

Expand All @@ -108,7 +108,7 @@ func generateDynamicTools(data *WorkflowData, markdownPath string) ([]map[string
useMD = true
} else {
safeOutputsConfigLog.Printf("Warning: no workflow file found for %s (checked .lock.yml, .yml, .md)", workflowName)
dynamicTools = append(dynamicTools, generateDispatchWorkflowTool(workflowName, make(map[string]any)))
dynamicTools = append(dynamicTools, generateDispatchWorkflowTool(workflowName, make(map[string]any), data.SafeOutputs.DispatchWorkflow.AllowedRefs))
continue
}

Expand All @@ -126,7 +126,7 @@ func generateDynamicTools(data *WorkflowData, markdownPath string) ([]map[string
workflowInputs = make(map[string]any)
}

dynamicTools = append(dynamicTools, generateDispatchWorkflowTool(workflowName, workflowInputs))
dynamicTools = append(dynamicTools, generateDispatchWorkflowTool(workflowName, workflowInputs, data.SafeOutputs.DispatchWorkflow.AllowedRefs))
}
}

Expand Down
77 changes: 72 additions & 5 deletions pkg/workflow/safe_outputs_tools_generation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ func TestGenerateDispatchWorkflowToolBasic(t *testing.T) {
},
}

tool := generateDispatchWorkflowTool("deploy-app", workflowInputs)
tool := generateDispatchWorkflowTool("deploy-app", workflowInputs, nil)

assert.Equal(t, "deploy_app", tool["name"], "Tool name should be normalized")
assert.Equal(t, "deploy-app", tool["_workflow_name"], "Internal workflow name should be preserved")
Expand All @@ -278,7 +278,7 @@ func TestGenerateDispatchWorkflowToolBasic(t *testing.T) {

// TestGenerateDispatchWorkflowToolEmptyInputs tests dispatch workflow tool with no inputs.
func TestGenerateDispatchWorkflowToolEmptyInputs(t *testing.T) {
tool := generateDispatchWorkflowTool("simple-workflow", make(map[string]any))
tool := generateDispatchWorkflowTool("simple-workflow", make(map[string]any), nil)

assert.Equal(t, "simple_workflow", tool["name"], "Name should be normalized")

Expand Down Expand Up @@ -313,7 +313,7 @@ func TestGenerateDispatchWorkflowToolRequiredSorted(t *testing.T) {

// Run multiple times to catch non-determinism from map iteration
for i := range 10 {
tool := generateDispatchWorkflowTool("cleanup-worker", workflowInputs)
tool := generateDispatchWorkflowTool("cleanup-worker", workflowInputs, nil)

inputSchema, ok := tool["inputSchema"].(map[string]any)
require.True(t, ok, "inputSchema should be present (iteration %d)", i)
Expand All @@ -326,8 +326,75 @@ func TestGenerateDispatchWorkflowToolRequiredSorted(t *testing.T) {
}
}

// TestGenerateFilteredToolsJSONWithStandardOutputs tests that standard safe outputs produce
// the expected tools in the filtered output (regression test for the completeness check).
// TestGenerateDispatchWorkflowToolWithAllowedRefs tests that a 'ref' parameter is injected
// into the tool schema when allowed-refs is configured. This ensures the agent can supply
// a target ref that is validated against the configured glob patterns by the runtime handler.
func TestGenerateDispatchWorkflowToolWithAllowedRefs(t *testing.T) {
workflowInputs := map[string]any{
"model": map[string]any{
"description": "Model to run",
"type": "string",
"required": true,
},
}
allowedRefs := []string{"silencer/*", "refs/heads/main"}

tool := generateDispatchWorkflowTool("t3000-unit-tests", workflowInputs, allowedRefs)

assert.Equal(t, "t3000_unit_tests", tool["name"], "Tool name should be normalized")

inputSchema, ok := tool["inputSchema"].(map[string]any)
require.True(t, ok, "inputSchema should be present")

properties, ok := inputSchema["properties"].(map[string]any)
require.True(t, ok, "properties should be present")

// ref property should be injected
refProp, ok := properties["ref"].(map[string]any)
require.True(t, ok, "ref property should exist when allowed-refs is configured")
assert.Equal(t, "string", refProp["type"], "ref property should be a string")
assert.Contains(t, refProp["description"].(string), "silencer/*", "ref description should mention allowed patterns")
assert.Contains(t, refProp["description"].(string), "refs/heads/main", "ref description should mention all allowed patterns")

// ref should not be in required (it is optional)
required, hasRequired := inputSchema["required"].([]string)
if hasRequired {
assert.NotContains(t, required, "ref", "ref should not be required")

@github-actions github-actions Bot Aug 2, 2026

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.

[/tdd] The .([]string) type assertion on required will silently skip the assertion if the slice is []any — meaning the "ref should not be required" check never actually runs.

💡 Suggested fix

Handle both possible slice types:

requiredRaw := inputSchema["required"]
if requiredRaw != nil {
    switch r := requiredRaw.(type) {
    case []string:
        assert.NotContains(t, r, "ref")
    case []any:
        for _, v := range r {
            assert.NotEqual(t, "ref", v)
        }
    }
}

Alternatively, introduce a small containsInRequired(schema, field) test helper since the same pattern appears in other tests.

@copilot please address this.

}

// description should mention the allowed patterns
desc := tool["description"].(string)
assert.Contains(t, desc, "silencer/*", "tool description should mention allowed ref patterns")
}

// TestGenerateDispatchWorkflowToolNoRefWithoutAllowedRefs tests that no 'ref' property
// is added when allowed-refs is not configured (nil or empty).
func TestGenerateDispatchWorkflowToolNoRefWithoutAllowedRefs(t *testing.T) {
workflowInputs := map[string]any{
"platform": map[string]any{
"description": "Target platform",
"type": "string",
"required": true,
},
}

for _, allowedRefs := range [][]string{nil, {}} {
tool := generateDispatchWorkflowTool("build-workflow", workflowInputs, allowedRefs)

inputSchema, ok := tool["inputSchema"].(map[string]any)
require.True(t, ok, "inputSchema should be present")

properties, ok := inputSchema["properties"].(map[string]any)
require.True(t, ok, "properties should be present")

_, hasRef := properties["ref"]
assert.False(t, hasRef, "ref property should not be present when allowed-refs is not configured")

required, ok := inputSchema["required"].([]string)
require.True(t, ok, "required should be present for required workflow input")
assert.Equal(t, []string{"platform"}, required, "required should only include workflow inputs, not ref")
}
}

// TestComputeRequiredFieldRemovalsCloseDiscussion verifies that allow-body: false for
// close-discussion produces a required field removal for the body field.
Expand Down
Loading