Uh oh!
There was an error while loading. Please reload this page.
Preserve safe-output partial success and preflight dispatch inputs#53264
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53264 does not have the 'implementation' label and has only 58 new lines of code in business logic directories (threshold: 100).
|
✅ PR Code Quality Reviewer completed the code quality review.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Request changes
This patch changes runtime semantics in a risky way: mixed-result safe-output batches now report success even when a fatal output failed, which suppresses the only hard failure signal downstream jobs can reliably key off.
Blocking theme
- fatal safe-output failures are downgraded to warnings whenever any other item succeeded, so workflow conclusions, alerts, and retry logic lose the failure signal
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 14.7 AIC · ⌖ 7.31 AIC · ⊞ 6.9K
Comment /review to run again
| const failedItemLines = fatalFailures.map(r => ` - ${r.type}: ${r.error || "Unknown error"}`); | ||
| const failedItems = failedItemLines.join("\n"); | ||
| failedOutputsMessage = `${failureCount} safe output(s) failed:\n${failedItems}`; | ||
| if (shouldFailSafeOutputs(fatalFailures, safeOutputsStatus)) { |
There was a problem hiding this comment.
This partial-success change masks real handler failures from the job result. Once any safe output succeeds, failedOutputsMessage stays null, so the workflow exits green even when another output hard-failed; that breaks downstream failure handling and makes retries or alerts impossible for mixed-result batches.
💡 Why this is blocking
The previous behavior failed the job whenever a fatal handler failure existed. After this patch, a batch like create_check_run succeeds and push_to_pull_request_branch fails will only emit a warning and then return success at line 1825. Any workflow logic watching the step conclusion now loses the failure signal completely.
If partial success is the intended product behavior, the step still needs a machine-readable failure indicator beyond log text — for example, preserving core.setFailed(...) for fatal failures while introducing a separate non-blocking mode behind explicit config, or at minimum exporting a dedicated output that downstream jobs already consume before changing the step conclusion semantics.
There was a problem hiding this comment.
Review
The two features are well-scoped and correctly implemented.
Partial success preservation (shouldFailSafeOutputs): The condition fatalFailures.length > 0 && status.itemsSucceeded === 0 correctly gates job failure on a fully-failed batch, so a batch where at least one item succeeds degrades gracefully. Both branches are covered by tests.
Preflight dispatch input validation:required_inputs is populated at compile time via requiredWorkflowDispatchInputs (sorted for determinism) and threaded through to the CJS handler. The Object.hasOwn guard is the correct idiom for input presence. Error messages are clear and actionable.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 22 AIC · ⌖ 7.91 AIC · ⊞ 5.6K
There was a problem hiding this comment.
Pull request overview
Addresses #53263 by preserving successful safe outputs and validating workflow-dispatch inputs before API calls.
Changes:
- Treat mixed-result safe-output batches as partial successes.
- Compile and validate required dispatch inputs.
- Add regression tests for both behaviors.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_handler_registry.go | Serializes required-input metadata. |
pkg/workflow/safe_outputs_dispatch.go | Extracts required dispatch inputs. |
pkg/workflow/safe_outputs_config_generation_test.go | Tests generated metadata. |
pkg/workflow/dispatch_workflow.go | Stores required-input configuration. |
actions/setup/js/safe_output_handler_manager.test.cjs | Tests partial-success handling. |
actions/setup/js/safe_output_handler_manager.cjs | Avoids failing mixed-result batches. |
actions/setup/js/dispatch_workflow.test.cjs | Tests preflight rejection. |
actions/setup/js/dispatch_workflow.cjs | Rejects missing inputs before dispatch. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Balanced
| if isRequired, _ := definitionMap["required"].(bool); isRequired { | ||
| required = append(required, name) | ||
| } |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a few correctness and test-coverage gaps.
📋 Key Themes & Highlights
Issues
- Silent degradation on parse error (
safe_outputs_dispatch.go): whenworkflowDispatchInputserrors,continueskips required-input population but the workflow is already registered inWorkflowFiles, so it can be dispatched without preflight. requiredas string"true"(requiredWorkflowDispatchInputs): GitHub Actions YAML permitsrequired: "true"(string). The current type-assertion.(bool)silently misses these inputs.- Double-reporting warning (
dispatch_workflow.cjs):core.warning(error)is called before returning{ success: false, error }. Callers that surface the returned error field will produce duplicate log entries. - Test coverage gaps:
shouldFailSafeOutputshas a singleitwith twoexpectcalls (harder to read failure output); the plural missing-inputs code path indispatch_workflowhas no test.
Positive Highlights
- ✅ Clean extraction of
workflowHasAwContextInputinto the more generalworkflowDispatchInputs— reduces duplication and makes the aw_context check a special case of a more general pattern. - ✅
sort.Strings(required)inrequiredWorkflowDispatchInputsensures deterministic YAML output across compilations. - ✅
shouldFailSafeOutputsis correctly exported and unit-tested; the partial-success logic is easy to reason about. - ✅ Preflight check correctly short-circuits before the API call and returns an actionable error message.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 49.1 AIC · ⌖ 9.19 AIC · ⊞ 7.7K
Comment /matt to run again
| // We check the lock file first (compiled YAML), falling back to the markdown frontmatter. | ||
| if workflowHasAwContextInput(fileResult, workflowName) { | ||
| inputs, err := workflowDispatchInputs(fileResult) | ||
| if err != nil { |
There was a problem hiding this comment.
[/diagnosing-bugs] Silent degradation on input-extraction error: continue on line ~68 skips RequiredInputs population but WorkflowFiles[workflowName] is already written (~line 64), so the workflow stays dispatchable at runtime without required-input preflight.
💡 Suggested fix
Unlike aw_context, required inputs are not optional — silently skipping them can allow dispatches that miss mandatory fields. Consider logging explicitly that preflight will be skipped, or removing the workflow entry from WorkflowFiles:
inputs, err:=workflowDispatchInputs(fileResult)
iferr!=nil {
safeOutputsConfigLog.Printf(
"Warning: error extracting inputs for %s — required-input preflight will be skipped: %v",
workflowName, err,
)
// fallthrough: dispatch is still allowed, but required-input check is disabled
}@copilot please address this.
| expect(fatalFailures).toEqual([{ type: "create_issue", success: false, error: "Validation failed" }]); | ||
| }); | ||
| it("does not fail the job when a batch has successful outputs and failed items", () => { |
There was a problem hiding this comment.
[/tdd] The shouldFailSafeOutputs test covers only two boundary cases and uses a single it block with two expect calls. This makes it hard to see which case fails and leaves the "all items failed" edge case (itemsSucceeded: 0, zero fatal failures) untested.
💡 Suggested improvement
Split into named cases and add the zero-failure edge:
it("does not fail the job when at least one item succeeded",()=>{expect(shouldFailSafeOutputs([{type: "update_pull_request",success: false}],{itemsSucceeded: 1})).toBe(false);});it("fails the job when every item failed",()=>{expect(shouldFailSafeOutputs([{type: "update_pull_request",success: false}],{itemsSucceeded: 0})).toBe(true);});it("does not fail the job when there are no fatal failures",()=>{expect(shouldFailSafeOutputs([],{itemsSucceeded: 0})).toBe(false);});The third case confirms an empty fatalFailures array always returns false, matching the && short-circuit in the implementation.
@copilot please address this.
| if (missingInputs.length > 0) { | ||
| const error = `Workflow "${workflowName}" requires input${missingInputs.length === 1 ? "" : "s"}: ${missingInputs.map(input => `'${input}'`).join(", ")}. Provide ${missingInputs.length === 1 ? "it" : "them"} and retry.`; | ||
| core.warning(error); | ||
| return { success: false, error }; |
There was a problem hiding this comment.
[/diagnosing-bugs] The error string in the PR description body ('${input}') references the loop variable input (singular), but the implementation correctly iterates missingInputs and formats them. However, the bug remains in the error message template in the PR description — not in the code itself — so no code change is needed. Flagging for accuracy of the PR description only.
Actually, looking more carefully at the implementation: the error message is correct. But there's a subtle issue: core.warning(error) is called and then{ success: false, error } is returned. The caller will also likely log or surface the error. This double-reporting (warning + returned error object) may cause duplicate messages in the Actions log.
💡 Suggestion
If the caller already surfaces the returned error field, remove the core.warning(error) call here to avoid duplicate messages:
consterror=`Workflow "${workflowName}" requires input${...}: ${...}.Provide${...} and retry.`;// Let the caller handle surfacing — don't double-warn herereturn{success: false, error };Check how other early-return failure paths in this function handle logging vs. returning errors.
@copilot please address this.
| expect(result).toEqual({ | ||
| success: false, | ||
| error: `Workflow "test-workflow" requires input: 'message'. Provide it and retry.`, | ||
| }); |
There was a problem hiding this comment.
[/tdd] The dispatch test verifies the single-missing-input case but not multiple missing inputs. The implementation builds a plural-aware error string (requires inputs: 'a', 'b'), but there is no test that asserts the plural form or the comma-separated list.
💡 Suggested test case
it("should fail with all missing input names when multiple required inputs are absent",async()=>{consthandler=awaitmain({workflows: ["multi-workflow"],workflow_files: {"multi-workflow": ".lock.yml"},required_inputs: {"multi-workflow": ["env","message"]},});constresult=awaithandler({type: "dispatch_workflow",workflow_name: "multi-workflow",inputs: {}},{});expect(result.success).toBe(false);expect(result.error).toMatch(/requiresinputs:/);expect(result.error).toContain("'env'");expect(result.error).toContain("'message'");expect(github.rest.actions.createWorkflowDispatch).not.toHaveBeenCalled();});@copilot please address this.
| for name, definition := range inputs { | ||
| definitionMap, ok := definition.(map[string]any) | ||
| if !ok { | ||
| continue |
There was a problem hiding this comment.
[/diagnosing-bugs]requiredWorkflowDispatchInputs silently skips any input whose definition is not a map[string]any. YAML inputs whose required key is set to the string "true" (instead of boolean true) will also be silently skipped — GitHub Actions YAML allows this form.
💡 Suggestion
Add a fallback to handle string "true" for robustness:
funcrequiredWorkflowDispatchInputs(inputsmap[string]any) []string {
varrequired []stringforname, definition:=rangeinputs {
definitionMap, ok:=definition.(map[string]any)
if!ok {
continue
}
switchv:=definitionMap["required"].(type) {
casebool:
ifv {
required=append(required, name)
}
casestring:
ifv=="true" {
required=append(required, name)
}
}
}
sort.Strings(required)
returnrequired
}Add a test case with required: "true" to confirm this edge case.
@copilot please address this.
gh-aw-bot
commented
Aug 17, 2026
@copilot this PR is ready for the next finishing pass. Please address these items, newest first:
_Run:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
…ix-safe-outputs-job Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in
|
gh-aw-bot
commented
Aug 17, 2026
@copilot this PR is ready for the next finishing pass. Please address these items, newest first:
Run: https://github.com/github/gh-aw/actions/runs/31996014734
|
A single non-retryable safe-output failure currently marks the entire batch failed even after other outputs succeed. Workflow dispatches also reach GitHub’s API before detecting missing required inputs.
Partial-success batches
safe_outputsonly when no item in the batch succeeded.Dispatch workflow preflight
workflow_dispatchinputs during compilation.createWorkflowDispatch.