Fix dynamic dispatch_workflow ref forwarding and prevent ref leakage into inputs - #50042
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
dispatch_workflow ref forwarding and prevent ref leakage into inputs
There was a problem hiding this comment.
🟡 Not ready to approve
The loader breaks workflows with a legitimate ref input, and the smoke workflow cannot exercise the new path as configured.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Fixes dynamic workflow dispatch handling so per-call refs are forwarded outside workflow inputs.
Changes:
- Extracts
reffrom dynamic tool arguments. - Adds regression tests for standard and strict schemas.
- Updates and recompiles the Copilot smoke workflow.
File summaries
| File | Description |
|---|---|
actions/setup/js/safe_outputs_tools_loader.cjs |
Separates dispatch refs from workflow inputs. |
actions/setup/js/safe_outputs_tools_loader.test.cjs |
Tests ref forwarding and exclusion. |
.github/workflows/smoke-copilot.md |
Adds ref usage to the smoke prompt. |
.github/workflows/smoke-copilot.lock.yml |
Regenerates the compiled workflow. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| 9. Artifact upload (only if build passes): stage `./gh-aw` at `$RUNNER_TEMP/gh-aw/safeoutputs/upload-artifacts/gh-aw` and call `upload_artifact` with `path: "gh-aw"`. | ||
| 10. Discussion create: call `create_discussion` in `announcements` with label `ai-generated`, title `copilot was here`, temp ID `aw_smoke_discussion`. | ||
| 11. Workflow dispatch: call `dispatch_workflow` for `haiku-printer` and include `inputs.message` with an original testing/automation haiku (non-empty string). | ||
| 11. Workflow dispatch: call `dispatch_workflow` for `haiku-printer`, set top-level `ref` to `${{ github.event.repository.default_branch }}`, and include `inputs.message` with an original testing/automation haiku (non-empty string). |
| const workflowName = tool._workflow_name.trim(); | ||
| tool.handler = args => { | ||
| // Wrap args in inputs property to match dispatch_workflow schema | ||
| const { ref, ...inputs } = args ?? {}; |
PR Triage: #50042
Fixes
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #50042 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The core fix is correct — extracting ref before wrapping remaining args as inputs properly prevents the 422 Unexpected inputs provided: ["ref"] errors. The test coverage is solid.
Two concerns already flagged in existing comments:
-
refreservation is unconditional (line 170,safe_outputs_tools_loader.cjs) — any workflow that legitimately declares arefworkflow input will have it silently promoted to the dispatch-levelreffield instead of passed as an input. This is a silent breaking change for such workflows. -
Smoke test does not exercise the dynamic handler path (line 208,
smoke-copilot.md) — the compiled per-workflow tool uses flat args so the addedrefstep may not cover the changed code path directly.
Neither blocks merge if the ref-as-reserved-field design is intentional.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 17.5 AIC · ⌖ 7.55 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues before merging.
📋 Key Themes & Highlights
Key Issues
- Falsy
refguard (ref && { ref }): silently drops an empty-stringref. Useref !== undefinedto preserve intent. inputs: {}edge case: whenargsis an explicit{},inputs: {}is now forwarded where the old code passedinputs: {}too — but theargs !== undefinedguard changes the no-args case. Downstream tolerance ofinputs: {}vs absentinputsshould be confirmed and tested.- Missing edge-case test: no test covers
ref: ''orargs: {}, leaving the falsy-guard bug undetected by the test suite.
Positive Highlights
- ✅ Clear, minimal destructuring fix for the primary bug
- ✅ Good regression test structure with both positive and negative assertions
- ✅ Strict-schema path covered by a dedicated test
- ✅ PR description accurately describes the root cause and the fix
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 30.1 AIC · ⌖ 11.1 AIC · ⊞ 7.1K
Comment /matt to run again
| // Wrap workflow inputs in inputs and pass dispatch ref as top-level field | ||
| return handlers.defaultHandler("dispatch_workflow")({ | ||
| inputs: args, | ||
| ...(ref && { ref }), |
There was a problem hiding this comment.
[/diagnosing-bugs] ref && { ref } silently drops a falsy (e.g. empty-string) ref. Use ref !== undefined to be explicit.
@copilot please address this.
| defaultHandler: defaultHandler, | ||
| }; | ||
|
|
||
| const result = attachHandlers(tools, handlers); |
There was a problem hiding this comment.
[/tdd] The test verifies ref is excluded from inputs via a negative assertion, but it doesn't assert what happens when ref is falsy (empty string, null). This is the exact edge case the truthiness guard mishandles.
💡 Suggested additional test
it('should not forward empty-string ref to top-level for dispatch_workflow handler', () => {
// With current impl, ref:'' is dropped; document whether that is intentional
result[0].handler({ ref: '', input1: 'value1' });
expect(mockHandlerFunction).toHaveBeenCalledWith({
workflow_name: 'ci',
// ref should be absent OR present — document the contract
inputs: { input1: 'value1' },
});
});@copilot please address this.
| return handlers.defaultHandler("dispatch_workflow")({ | ||
| inputs: args, | ||
| ...(ref && { ref }), | ||
| ...(args !== undefined && { inputs }), |
There was a problem hiding this comment.
[/diagnosing-bugs] When args is undefined, inputs becomes {} (from const { ref, ...inputs } = undefined ?? {}) — so args !== undefined correctly suppresses inputs: {}. But when args is {} (explicit empty object), inputs: {} is forwarded. This is a change in behaviour vs the previous inputs: args path. Confirm whether the downstream dispatch_workflow handler tolerates inputs: {} vs a missing inputs field.
@copilot please address this.
🧪 Test Quality Sentinel Report
📊 Metrics (7 tests)
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Per-workflow
dispatch_workflowtools currently pass flat args directly asinputs, so per-callrefis sent as an unexpected workflow input instead of the required top-level dispatch field. This causes 422 failures (Unexpected inputs provided: ["ref"]) and can silently dispatch on the wrong ref.Dynamic safe-output handler fix (
actions/setup/js/safe_outputs_tools_loader.cjs)_workflow_name, the handler now:reffrom tool args,refin thedispatch_workflowpayload,inputs.undefined(no syntheticinputsobject added).Regression coverage (
actions/setup/js/safe_outputs_tools_loader.test.cjs)refis forwarded top-level,refis excluded frominputs,Workflow usage enablement (
.github/workflows/smoke-copilot.md+ recompiled lock)dispatch_workflowrefusage (top-level field), ensuring this codepath is actively used.refoverride lands ininputs, causing 422 "Unexpected inputs provided" and/or dispatching the wrong ref #50027