fix(safe-outputs): add MCE1 egress context validation to 10 handlers that hard-failed on scheduled runs - #49060
Conversation
…vent hard-fail on scheduled runs Adds MCE1 egress validation to close_pull_request, merge_pull_request, mark_pull_request_as_ready_for_review, add_reviewer, reply_to_pull_request_review_comment, close_issue, add_labels, remove_labels, update_discussion, and close_discussion handlers. When a tool is called without an explicit target number (e.g., no pull_request_number) on a scheduled/push run that has no triggering PR/issue/discussion context, the handler now returns an actionable MCP error response immediately instead of writing to NDJSON and hard-failing during process_safe_outputs execution. Introduces createTriggeringContextHandler factory for DRY handler creation and registers all 10 new handlers in attachHandlers(). Adds 40 unit tests covering: schedule/push context rejection, explicit number bypass, and valid context acceptance for each handler. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds early triggering-context validation for ten safe-output handlers to prevent downstream failures.
Changes:
- Adds shared PR, issue, and discussion context validation.
- Registers the new handlers.
- Adds unit coverage for rejection, bypass, and valid-context paths.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/safe_outputs_handlers.cjs |
Implements context-aware handlers. |
actions/setup/js/safe_outputs_tools_loader.cjs |
Registers the handlers. |
actions/setup/js/safe_outputs_handlers.test.cjs |
Tests handler behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
actions/setup/js/safe_outputs_handlers.cjs:2359
- These checks establish context from the event name alone, without verifying that the payload contains the entity number downstream handlers need. For example,
workflow_dispatchcan overrideevent_nametopull_requestwhile omittingevent_payload; this branch accepts the call and writes NDJSON, then processing still fails because no PR number exists. Require the correspondingpull_request.number,issue.number, ordiscussion.numberin addition to the event type.
let hasContext;
if (contextType === "pr") {
hasContext = PR_EVENT_NAMES.has(effectiveEventName) || isIssueCommentOnPR;
} else if (contextType === "issue") {
hasContext = effectiveEventName === "issues" || (effectiveEventName === "issue_comment" && !isIssueCommentOnPR);
actions/setup/js/safe_outputs_handlers.cjs:2337
- Checking only for non-null values treats
""or whitespace as an explicit number. Several affected schemas allow arbitrary strings for number fields, so such input bypasses context validation, is appended, and fails later during number parsing—the hard-failure path this change is intended to prevent. Reuse the existing helper, which rejects blank explicit targets.
const hasExplicitNumber = explicitNumberFields.some(field => args?.[field] != null);
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Medium
| const createTriggeringContextHandler = ({ toolName, explicitNumberFields, contextType, buildErrorMessage }) => { | ||
| return args => { | ||
| // If the caller supplied an explicit target number, skip context validation. | ||
| // The downstream execution handler will resolve the number normally. | ||
| const hasExplicitNumber = explicitNumberFields.some(field => args?.[field] != null); |
Triage Summary
Fixes hard-fail on scheduled runs across 10 safe-output handlers lacking MCE1 egress validation. Large diff (639 additions), not draft — recommend expedited review.
|
|
@copilot run pr-finisher skill |
|
@copilot please run the
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in commit |
The unresolved review feedback has been addressed (commit Local validation results:
Hand-off: CI must be re-triggered by a maintainer (close/reopen PR, workflow_dispatch, or push) before merge. |
|
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the skill, address unresolved review feedback if any, and rerun checks once the branch is up to date.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All review feedback was already addressed in commit The CI failure in run This pass also committed the recompile output (242 lock files + SKILL.md) that were produced by the merge with main but not yet staged. Local validation:
Hand-off: CI must be re-triggered by a maintainer (close/reopen PR, workflow_dispatch, or push) before merge. |
update_pull_request,update_issue, andadd_commenthad MCE1 egress validation (context checked before writing to NDJSON), but 10 other tools targeting triggering entities fell through todefaultHandler— writing to NDJSON and hard-failing atprocess_safe_outputsexecution time when no PR/issue/discussion context existed (e.g. scheduled runs).Changes
safe_outputs_handlers.cjscreateTriggeringContextHandlerfactory following the existing MCE1 patternclosePullRequestHandler,mergePullRequestHandler,markPullRequestAsReadyForReviewHandler,addReviewerHandler,replyToPullRequestReviewCommentHandler,closeIssueHandler,addLabelsHandler,removeLabelsHandler,updateDiscussionHandler,closeDiscussionHandlersafe_outputs_tools_loader.cjsattachHandlers()handlerMapsafe_outputs_handlers.test.cjsRun: https://github.com/github/gh-aw/actions/runs/30523835792
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Run: https://github.com/github/gh-aw/actions/runs/30531795286