fix(dispatch-workflow): expose ref parameter in per-workflow tool schema when allowed-refs is configured - #49754
Conversation
…ed-refs is configured - Update generateDispatchWorkflowTool to accept allowedRefs []string - Inject optional 'ref' property into the tool input schema when allowed-refs is non-empty - Include allowed ref patterns in property description and tool description so the agent understands the contract - Pass DispatchWorkflowConfig.AllowedRefs to all three generateDispatchWorkflowTool call sites in generateDynamicTools - Add tests for the new ref parameter injection and the no-ref case Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
ref parameter in per-workflow tool schema when allowed-refs is configured
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has ≤100 new lines of code in business logic directories (97 additions, threshold is 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
Exposes an optional dispatch target ref when allowed-refs is configured.
Changes:
- Adds
refto generated workflow tool schemas. - Passes allowed refs through tool-generation call sites.
- Adds schema-generation tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_dispatch.go |
Injects the ref tool parameter. |
pkg/workflow/safe_outputs_tools_generation.go |
Passes allowed-ref configuration. |
pkg/workflow/safe_outputs_tools_generation_test.go |
Tests conditional ref generation. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Suppressed comments (1)
pkg/workflow/safe_outputs_dispatch.go:124
workflowInputsare flattened into this samepropertiesmap, so a valid target workflow input namedrefis silently overwritten here. If that input is required, its oldrequiredentry also remains, making the supposedly optional dispatch ref required; once the bridge lifts this value tomessage.ref, the target workflow input can no longer be supplied at all. Use a distinct tool argument (for exampletarget_ref) that the bridge maps tomessage.ref, or explicitly reject/reserve colliding workflow input names.
properties["ref"] = map[string]any{
"type": "string",
"description": refDesc,
}
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
| properties["ref"] = map[string]any{ | ||
| "type": "string", | ||
| "description": refDesc, | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(dispatch-workflow): expose ref parameter in per-workflow tool schema
The fix is correct and well-tested. The logic that injects the ref property only when allowedRefs is non-empty correctly preserves backward compatibility.
One non-blocking suggestion: fmt.Sprintf("%v", allowedRefs) formats a []string as [a b c] (brackets, space-separated, no quotes). For LLM-facing tool descriptions, strings.Join(allowedRefs, ", ") would produce a cleaner, more readable list. See inline comment.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28.5 AIC · ⌖ 7.61 AIC · ⊞ 5.4K
| inputSchema, _ := tool["inputSchema"].(map[string]any) | ||
| properties, _ := inputSchema["properties"].(map[string]any) | ||
|
|
||
| refDesc := fmt.Sprintf("The git ref (branch, tag, or SHA) to dispatch the workflow on. Must match one of the configured allowed ref patterns: %v. If omitted, the dispatching workflow's ref is used.", allowedRefs) |
There was a problem hiding this comment.
Minor: %v formatting of []string yields [silencer/* refs/heads/main] (bracketed, space-separated, no quotes). For an LLM-facing description, a cleaner comma-separated format is preferable.
Consider using strings.Join:
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.", strings.Join(allowedRefs, ", "))Apply the same change on line 127 for the tool-level description append.
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on two small but concrete issues.
📋 Key Themes & Highlights
Issues
- Brittle test assertion (
safe_outputs_tools_generation_test.goL362):.([]string)type assertion onrequiredsilently no-ops if the slice is[]any, so the "ref should not be required" guard is never actually exercised. - Go-syntax leaking into user-facing strings (
safe_outputs_dispatch.goL121):fmt.Sprintf("%v", allowedRefs)renders as[silencer/* refs/heads/main]— usestrings.Join(allowedRefs, ", ")for clean output.
Positive Highlights
- ✅ Root cause is correctly identified and fixed: the missing
allowedRefsparameter propagation is the exact gap that madeallowed-refsunreachable. - ✅ All three call sites in
generateDynamicToolsare updated consistently. - ✅ Good backward-compatibility discipline: no
refproperty injected whenallowedRefsis nil/empty. - ✅ Test names read as specifications and the two new tests cover both positive and negative cases cleanly.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.2 AIC · ⌖ 20.3 AIC · ⊞ 7.1K
Comment /matt to run again
| // 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") |
There was a problem hiding this comment.
[/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.
| properties, _ := inputSchema["properties"].(map[string]any) | ||
|
|
||
| refDesc := fmt.Sprintf("The git ref (branch, tag, or SHA) to dispatch the workflow on. Must match one of the configured allowed ref patterns: %v. If omitted, the dispatching workflow's ref is used.", allowedRefs) | ||
| properties["ref"] = map[string]any{ |
There was a problem hiding this comment.
[/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.
🧪 Test Quality Sentinel Report
📊 Metrics (2 new tests)
✅ Test Design QualityBoth new tests validate critical design invariants:
Both tests follow Go conventions: descriptive failure messages on assertions, proper use of Verdict
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 75/100. 0% implementation tests (threshold: 30%), 0 violations. Both new tests are pure design tests validating schema injection behavior and backward compatibility. No mocking, proper build tags, high-value coverage of the ref parameter feature.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · auto · 61.7 AIC · ⌖ 14.1 AIC · ⊞ 7.8K
Comment /review to run again
| inputSchema, _ := tool["inputSchema"].(map[string]any) | ||
| properties, _ := inputSchema["properties"].(map[string]any) | ||
|
|
||
| refDesc := fmt.Sprintf("The git ref (branch, tag, or SHA) to dispatch the workflow on. Must match one of the configured allowed ref patterns: %v. If omitted, the dispatching workflow's ref is used.", allowedRefs) |
There was a problem hiding this comment.
Unconditional properties["ref"] = ... silently overwrites any workflow_dispatch input literally named ref, and no test covers this collision.
💡 Details
If a target workflow defines its own workflow_dispatch input named ref (a plausible, unreserved name), this code overwrites its schema entry — including type, description, and required-ness — with the injected ref-glob description, with no warning, log, or merge logic. A caller-defined required ref input silently loses its required semantics from the caller's perspective (the property is replaced but the pre-existing required array entry, if any, stays, producing an inconsistent schema where ref is both required and described as optional/glob-validated).
Suggested fix: detect a name collision before injecting, and either skip injection with a log warning, or rename the injected property (e.g. dispatch_ref) to avoid clobbering user-defined inputs.
if len(allowedRefs) > 0 {
inputSchema, _ := tool["inputSchema"].(map[string]any)
properties, _ := inputSchema["properties"].(map[string]any)
if _, collision := properties["ref"]; collision {
safeOutputsDispatchWorkflowLog.Printf("Warning: workflow %s already defines a 'ref' input; skipping allowed-refs injection to avoid clobbering it", workflowName)
} else {
properties["ref"] = map[string]any{...}
}
}Also add a regression test with a workflow_dispatch input literally named ref plus allowed-refs configured, asserting the original input is preserved.
| 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", | ||
| }) |
There was a problem hiding this comment.
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{...}
}| properties["ref"] = map[string]any{ | ||
| "type": "string", | ||
| "description": refDesc, | ||
| } |
There was a problem hiding this comment.
%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.").
|
@copilot Please address the open review feedback on this PR, then run the pr-finisher skill. Outstanding review items observed:
Branch refresh was requested for this PR. After fixing the review feedback, run the pr-finisher skill.
|
…assertion Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Fixed in commit
|
|
@copilot Please give this PR a finishing pass and run the pr-finisher skill. Current triage signals:
After that, run the pr-finisher skill.
|
PR Triage
Automated triage — see full report issue for details. Structured data: {
"action": "defer",
"category": "bug",
"pr_number": 49754,
"risk": "low"
}
|
…workflow-ref-parameter Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Finishing pass completed on head Current blocker: unresolved review threads are still open, and Validation status from this pass:
This head is ready for maintainer follow-up once remaining review-thread state/mergeability is cleared and CI is re-triggered by a maintainer. |
|
🎉 This pull request is included in a new release. Release: |
generateDispatchWorkflowToolnever added arefproperty to the generated MCP tool schema, leavingmessage.refalways absent at runtime. Every dispatch silently fell through the priority chain toGITHUB_REF(the dispatching workflow's ref), makingallowed-refsconfiguration unreachable dead config despite the backend supporting it since #49408.Changes
safe_outputs_dispatch.go—generateDispatchWorkflowToolnow acceptsallowedRefs []string. When non-empty, injects an optionalrefstring property into the tool'sinputSchema.propertieswith a description listing the allowed glob patterns, and appends those patterns to the tool description:{ "ref": { "type": "string", "description": "The git ref (branch, tag, or SHA) to dispatch the workflow on. Must match one of the configured allowed ref patterns: [silencer/* refs/heads/main]. If omitted, the dispatching workflow's ref is used." } }No
refproperty is injected whenallowed-refsis absent, preserving existing behavior.safe_outputs_tools_generation.go— All threegenerateDispatchWorkflowToolcall sites updated to passdata.SafeOutputs.DispatchWorkflow.AllowedRefs.safe_outputs_tools_generation_test.go— Existing tests updated for new signature; two new tests added: one verifyingrefinjection whenallowed-refsis set, one verifying no injection when it is absent.