diff --git a/actions/setup/js/safe_outputs_handlers.cjs b/actions/setup/js/safe_outputs_handlers.cjs index da0c183bcfe..abd49949298 100644 --- a/actions/setup/js/safe_outputs_handlers.cjs +++ b/actions/setup/js/safe_outputs_handlers.cjs @@ -31,7 +31,7 @@ const { lstatGuard } = require("./symlink_guard.cjs"); const { validateValueAgainstSchema } = require("./mcp_scripts_validation.cjs"); const { resolveDataSchema } = require("./data_schema_normalizer.cjs"); -/** PR event names used for target:triggering context validation across all safe-output handlers. */ +/** PR event names used for target:triggering context detection across add_comment and update_pull_request handlers. */ const PR_EVENT_NAMES = new Set(["pull_request", "pull_request_target", "pull_request_review", "pull_request_review_comment"]); /** @@ -1869,8 +1869,15 @@ function createHandlers(server, appendSafeOutput, config = {}) { * Handler for add_comment tool * Spec cross-reference: Safe Output Outcome Evaluation §3 (`add_comment`). * Per Safe Outputs Specification MCE1: Enforces constraints during tool invocation - * to provide immediate feedback to the LLM before recording to NDJSON - * Also auto-generates a temporary_id if not provided and returns it to the agent + * to provide immediate feedback to the LLM before recording to NDJSON. + * Also auto-generates a temporary_id if not provided and returns it to the agent. + * + * Context resolution is delegated to runtime: when `target: triggering` resolves + * to a context with no issue, PR, or discussion (e.g. push, schedule), the runtime + * handler soft-skips the entry instead of failing the safe-outputs pass. + * When no triggering context is detected, a hint is included in the success response + * so the agent can recover (e.g. switch to create_issue or create_discussion). + * SEC-005 (`target_repo` allowlist) is still enforced here at MCP-phase. */ const addCommentHandler = args => { // Validate comment constraints before appending to safe outputs @@ -1902,39 +1909,40 @@ function createHandlers(server, appendSafeOutput, config = {}) { ); } - // Reject target:triggering early when no explicit item number and no issue/PR/discussion context. - // Per Safe Outputs Specification MCE1: provides actionable feedback before writing to NDJSON. - // Mirrors update_issue validation; explicit item_number bypasses this check because the - // downstream handler resolves explicit numbers before falling back to triggering context. + // Enforce SEC-005 and detect no-context events for hint generation. + // A validation error (e.g. disallowed target_repo) is surfaced immediately. + // No-context events (push, schedule, etc.) are soft-skipped at runtime; a hint is + // included in the success response so the agent can recover. + /** @type {ReturnType | null} */ + let addCommentInvocationContext = null; + try { + addCommentInvocationContext = resolveInvocationContext(context); + } catch (err) { + const errMsg = getErrorMessage(err); + if (errMsg.startsWith(ERR_VALIDATION)) { + return buildIntentErrorResponse(errMsg); + } + // Unexpected structural error: let downstream handle gracefully. + } + + // Detect whether the current event has no issue/PR/discussion context so a hint + // can be included in the success response (agent feedback, not a hard failure). const effectiveAddCommentTarget = addCommentConfig.target || "triggering"; const hasExplicitItemNumber = args?.item_number != null || args?.issue_number != null || args?.["pr-number"] != null; - if (effectiveAddCommentTarget === "triggering" && !hasExplicitItemNumber) { - /** @type {any} */ - let invocationContext = null; - try { - invocationContext = resolveInvocationContext(context); - } catch (err) { - // A validation error (e.g. disallowed target_repo / SEC-005) is a real failure — surface it. - const errMsg = getErrorMessage(err); - if (errMsg.startsWith(ERR_VALIDATION)) { - return buildIntentErrorResponse(errMsg); - } - // Unexpected structural error: skip validation and let downstream handle gracefully. - } - if (invocationContext != null) { - const { effectiveEventName, effectivePayload } = resolveEffectiveContext(invocationContext, context); - const isIssueCommentOnPR = effectiveEventName === "issue_comment" && Boolean(effectivePayload?.issue?.pull_request); - const isIssueContext = effectiveEventName === "issues" || (effectiveEventName === "issue_comment" && !isIssueCommentOnPR); - const isPRContext = PR_EVENT_NAMES.has(effectiveEventName) || isIssueCommentOnPR; - const isDiscussionContext = effectiveEventName === "discussion" || effectiveEventName === "discussion_comment"; - if (!isIssueContext && !isPRContext && !isDiscussionContext) { - return buildIntentErrorResponse( - `add_comment requires an issue, pull request, or discussion context but the workflow is running on a "${effectiveEventName}" event. ` + - `The add-comment handler uses target: triggering which only applies when an issue, pull request, or discussion triggered the workflow. ` + - `To report results from this workflow, use create_discussion or create_issue instead. ` + - `If you need to comment on a specific item, provide an explicit item_number.` - ); - } + /** @type {string | null} */ + let addCommentContextHint = null; + if (effectiveAddCommentTarget === "triggering" && !hasExplicitItemNumber && addCommentInvocationContext != null) { + const { effectiveEventName, effectivePayload } = resolveEffectiveContext(addCommentInvocationContext, context); + const isIssueCommentOnPR = effectiveEventName === "issue_comment" && Boolean(effectivePayload?.issue?.pull_request); + const isIssueContext = effectiveEventName === "issues" || (effectiveEventName === "issue_comment" && !isIssueCommentOnPR); + const isPRContext = PR_EVENT_NAMES.has(effectiveEventName) || isIssueCommentOnPR; + const isDiscussionContext = effectiveEventName === "discussion" || effectiveEventName === "discussion_comment"; + if (!isIssueContext && !isPRContext && !isDiscussionContext) { + addCommentContextHint = + `add_comment targets 'triggering' context but the workflow is running on a '${effectiveEventName}' event. ` + + `This entry will be skipped at runtime if no issue, pull request, or discussion triggered the workflow. ` + + `To ensure output is always recorded, use create_issue or create_discussion instead. ` + + `To comment on a specific item, provide an explicit item_number.`; } } @@ -1963,7 +1971,9 @@ function createHandlers(server, appendSafeOutput, config = {}) { // Append to safe outputs appendSafeOutputCounted(entry); - // Return the temporary_id to the agent so it can reference this comment + // Return the temporary_id to the agent so it can reference this comment. + // Include a hint when the entry targets 'triggering' context in a no-context event so + // the agent can recover (e.g. add create_issue as a fallback) before the run ends. return { content: [ { @@ -1972,6 +1982,7 @@ function createHandlers(server, appendSafeOutput, config = {}) { result: "success", temporary_id: entry.temporary_id, comment: `#${entry.temporary_id}`, + ...(addCommentContextHint ? { hint: addCommentContextHint } : {}), }), }, ], @@ -2255,9 +2266,12 @@ function createHandlers(server, appendSafeOutput, config = {}) { * to provide immediate feedback to the LLM before recording to NDJSON. * Uses hasUpdatePullRequestFields to validate that at least one of 'title', 'body', * or 'update_branch' is provided before recording to NDJSON. - * Rejects `target: triggering` (the default) when the workflow has no pull request context - * (e.g. on schedule or push events), so the agent receives an actionable error - * instead of a downstream Process Safe Outputs failure. + * For `target: triggering` in non-PR contexts (e.g. schedule/workflow_dispatch without + * aw_context), this MCP-phase handler still records the output. A hint is included in + * the success response so the agent can recover (e.g. switch to create_issue) before + * the run ends. The downstream runtime handler resolves context and soft-skips those + * entries instead of hard-failing the run. + * SEC-005 (`target_repo` allowlist) is still enforced here at MCP-phase. */ const updatePullRequestHandler = args => { if (!hasUpdatePullRequestFields(args)) { @@ -2267,38 +2281,48 @@ function createHandlers(server, appendSafeOutput, config = {}) { }; } - const updatePRConfig = getSafeOutputsToolConfig(config, "update_pull_request"); - const effectivePRTarget = updatePRConfig.target || "triggering"; - if (effectivePRTarget === "triggering") { - /** @type {any} */ - let invocationContext = null; - try { - invocationContext = resolveInvocationContext(context); - } catch (err) { - // A validation error (e.g. disallowed target_repo / SEC-005) is a real failure — surface it. - const errMsg = getErrorMessage(err); - if (errMsg.startsWith(ERR_VALIDATION)) { - return buildIntentErrorResponse(errMsg); - } - // Unexpected structural error: skip validation and let downstream handle gracefully. + // Enforce SEC-005 and detect no-PR-context events for hint generation. + // A validation error (e.g. disallowed target_repo) is surfaced immediately. + // No-context events (push, schedule, etc.) are soft-skipped at runtime; a hint is + // included in the success response so the agent can recover. + /** @type {ReturnType | null} */ + let updatePRInvocationContext = null; + try { + updatePRInvocationContext = resolveInvocationContext(context); + } catch (err) { + const errMsg = getErrorMessage(err); + if (errMsg.startsWith(ERR_VALIDATION)) { + return buildIntentErrorResponse(errMsg); } - if (invocationContext != null) { - const { effectiveEventName, effectivePayload } = resolveEffectiveContext(invocationContext, context); - const isIssueCommentOnPR = effectiveEventName === "issue_comment" && Boolean(effectivePayload?.issue?.pull_request); - const isPRContext = PR_EVENT_NAMES.has(effectiveEventName) || isIssueCommentOnPR; + // Unexpected structural error: let downstream handle gracefully. + } - if (!isPRContext) { - return buildIntentErrorResponse( - `update_pull_request requires a pull request context but the workflow is running on a "${effectiveEventName}" event. ` + - `The update-pull-request handler uses target: triggering which only applies when a pull request triggered the workflow. ` + - `To report results from this workflow, use create_discussion or create_issue instead. ` + - `If you need to update a specific pull request, the workflow must configure update-pull-request: target: '*' and you must supply pull_request_number.` - ); - } + // Detect whether the current event has no PR context so a hint can be included in + // the success response (agent feedback, not a hard failure). + const updatePRConfig = getSafeOutputsToolConfig(config, "update_pull_request"); + const effectivePRTarget = updatePRConfig.target || "triggering"; + /** @type {string | null} */ + let updatePRContextHint = null; + if (effectivePRTarget === "triggering" && updatePRInvocationContext != null) { + const { effectiveEventName, effectivePayload } = resolveEffectiveContext(updatePRInvocationContext, context); + const isIssueCommentOnPR = effectiveEventName === "issue_comment" && Boolean(effectivePayload?.issue?.pull_request); + const isPRContext = PR_EVENT_NAMES.has(effectiveEventName) || isIssueCommentOnPR; + if (!isPRContext) { + updatePRContextHint = + `update_pull_request targets 'triggering' context but the workflow is running on a '${effectiveEventName}' event. ` + + `This entry will be skipped at runtime if no pull request triggered the workflow. ` + + `To report results from this workflow, use create_issue or create_discussion instead. ` + + `To update a specific pull request, configure 'update-pull-request: target: \"*\"' in the workflow and provide 'pull_request_number'.`; } } - return defaultHandler("update_pull_request")(args || {}); + const updatePRResponse = defaultHandler("update_pull_request")(args || {}); + if (updatePRContextHint && !updatePRResponse.isError) { + const responseData = JSON.parse(updatePRResponse.content[0].text); + responseData.hint = updatePRContextHint; + updatePRResponse.content[0].text = JSON.stringify(responseData); + } + return updatePRResponse; }; return { diff --git a/actions/setup/js/safe_outputs_handlers.test.cjs b/actions/setup/js/safe_outputs_handlers.test.cjs index a444d4d71c4..fd4794c46e5 100644 --- a/actions/setup/js/safe_outputs_handlers.test.cjs +++ b/actions/setup/js/safe_outputs_handlers.test.cjs @@ -2281,33 +2281,35 @@ describe("safe_outputs_handlers", () => { } }); - it("should return intent error when target is triggering (default) and not in issue/PR/discussion context", () => { + it("should record entry when target is triggering (default) and not in issue/PR/discussion context", () => { const savedContext = global.context; global.context = { ...global.context, eventName: "push", payload: {} }; try { const result = handlers.addCommentHandler({ body: "A real comment body that is substantive" }); - expect(result.isError).toBe(true); + expect(result.isError).toBeUndefined(); const responseData = JSON.parse(result.content[0].text); - expect(responseData.result).toBe("error"); - expect(responseData.error).toContain("add_comment"); - expect(responseData.error).toContain('"push"'); - expect(mockAppendSafeOutput).not.toHaveBeenCalled(); + expect(responseData.result).toBe("success"); + expect(responseData.hint).toBeDefined(); + expect(responseData.hint).toContain("push"); + expect(responseData.hint).toContain("create_issue"); + expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "add_comment" })); } finally { global.context = savedContext; } }); - it("should return intent error on schedule event with default target", () => { + it("should record entry on schedule event with default target", () => { const savedContext = global.context; global.context = { ...global.context, eventName: "schedule", payload: {} }; try { const result = handlers.addCommentHandler({ body: "A real comment body that is substantive" }); - expect(result.isError).toBe(true); + expect(result.isError).toBeUndefined(); const responseData = JSON.parse(result.content[0].text); - expect(responseData.result).toBe("error"); - expect(responseData.error).toContain('"schedule"'); - expect(responseData.error).toContain("create_discussion"); - expect(mockAppendSafeOutput).not.toHaveBeenCalled(); + expect(responseData.result).toBe("success"); + expect(responseData.hint).toBeDefined(); + expect(responseData.hint).toContain("schedule"); + expect(responseData.hint).toContain("create_issue"); + expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "add_comment" })); } finally { global.context = savedContext; } @@ -2321,6 +2323,7 @@ describe("safe_outputs_handlers", () => { expect(result.isError).toBeUndefined(); const responseData = JSON.parse(result.content[0].text); expect(responseData.result).toBe("success"); + expect(responseData.hint).toBeUndefined(); expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "add_comment" })); } finally { global.context = savedContext; @@ -2339,6 +2342,7 @@ describe("safe_outputs_handlers", () => { expect(result.isError).toBeUndefined(); const responseData = JSON.parse(result.content[0].text); expect(responseData.result).toBe("success"); + expect(responseData.hint).toBeUndefined(); expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "add_comment" })); } finally { global.context = savedContext; @@ -2356,6 +2360,7 @@ describe("safe_outputs_handlers", () => { expect(result.isError).toBeUndefined(); const responseData = JSON.parse(result.content[0].text); expect(responseData.result).toBe("success"); + expect(responseData.hint).toBeUndefined(); expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "add_comment", item_number: 42 })); } finally { global.context = savedContext; @@ -2383,25 +2388,51 @@ describe("safe_outputs_handlers", () => { expect(result.isError).toBeUndefined(); const responseData = JSON.parse(result.content[0].text); expect(responseData.result).toBe("success"); + expect(responseData.hint).toBeUndefined(); expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "add_comment" })); } finally { global.context = savedContext; } }); - it("should return intent error on workflow_dispatch with no event_name override", () => { + it("should record entry on workflow_dispatch with no event_name override", () => { const savedContext = global.context; global.context = { ...global.context, eventName: "workflow_dispatch", payload: { inputs: {} }, // no event_name, no aw_context }; + try { + const result = handlers.addCommentHandler({ body: "A real comment body that is substantive" }); + expect(result.isError).toBeUndefined(); + const responseData = JSON.parse(result.content[0].text); + expect(responseData.result).toBe("success"); + expect(responseData.hint).toBeDefined(); + expect(responseData.hint).toContain("workflow_dispatch"); + expect(responseData.hint).toContain("create_issue"); + expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "add_comment" })); + } finally { + global.context = savedContext; + } + }); + + it("should return intent error when workflow_dispatch has disallowed target_repo (SEC-005)", () => { + const savedContext = global.context; + global.context = { + ...global.context, + eventName: "workflow_dispatch", + payload: { + inputs: { + target_repo: "other-owner/other-repo", + }, + }, + }; try { const result = handlers.addCommentHandler({ body: "A real comment body that is substantive" }); expect(result.isError).toBe(true); const responseData = JSON.parse(result.content[0].text); expect(responseData.result).toBe("error"); - expect(responseData.error).toContain('"workflow_dispatch"'); + expect(responseData.error).toContain("other-owner/other-repo"); expect(mockAppendSafeOutput).not.toHaveBeenCalled(); } finally { global.context = savedContext; @@ -3063,33 +3094,35 @@ describe("safe_outputs_handlers", () => { } }); - it("should return intent error when target is triggering (default) and not in PR context", () => { + it("should record entry when target is triggering (default) and not in PR context", () => { const savedContext = global.context; global.context = { ...global.context, eventName: "push", payload: {} }; try { const result = handlers.updatePullRequestHandler({ title: "Update title" }); - expect(result.isError).toBe(true); + expect(result.isError).toBeUndefined(); const responseData = JSON.parse(result.content[0].text); - expect(responseData.result).toBe("error"); - expect(responseData.error).toContain("update_pull_request"); - expect(responseData.error).toContain('"push"'); - expect(mockAppendSafeOutput).not.toHaveBeenCalled(); + expect(responseData.result).toBe("success"); + expect(responseData.hint).toBeDefined(); + expect(responseData.hint).toContain("push"); + expect(responseData.hint).toContain("create_issue"); + expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "update_pull_request", title: "Update title" })); } finally { global.context = savedContext; } }); - it("should return intent error on schedule event with default target", () => { + it("should record entry on schedule event with default target", () => { const savedContext = global.context; global.context = { ...global.context, eventName: "schedule", payload: {} }; try { const result = handlers.updatePullRequestHandler({ body: "Report" }); - expect(result.isError).toBe(true); + expect(result.isError).toBeUndefined(); const responseData = JSON.parse(result.content[0].text); - expect(responseData.result).toBe("error"); - expect(responseData.error).toContain('"schedule"'); - expect(responseData.error).toContain("create_discussion"); - expect(mockAppendSafeOutput).not.toHaveBeenCalled(); + expect(responseData.result).toBe("success"); + expect(responseData.hint).toBeDefined(); + expect(responseData.hint).toContain("schedule"); + expect(responseData.hint).toContain("create_issue"); + expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "update_pull_request", body: "Report" })); } finally { global.context = savedContext; } @@ -3128,13 +3161,14 @@ describe("safe_outputs_handlers", () => { expect(result.isError).toBeUndefined(); const responseData = JSON.parse(result.content[0].text); expect(responseData.result).toBe("success"); + expect(responseData.hint).toBeUndefined(); expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "update_pull_request" })); } finally { global.context = savedContext; } }); - it("should return intent error on workflow_dispatch with no event_name override", () => { + it("should record entry on workflow_dispatch with no event_name override", () => { const savedContext = global.context; global.context = { ...global.context, @@ -3143,10 +3177,35 @@ describe("safe_outputs_handlers", () => { }; try { const result = handlers.updatePullRequestHandler({ title: "No context title" }); + expect(result.isError).toBeUndefined(); + const responseData = JSON.parse(result.content[0].text); + expect(responseData.result).toBe("success"); + expect(responseData.hint).toBeDefined(); + expect(responseData.hint).toContain("workflow_dispatch"); + expect(responseData.hint).toContain("create_issue"); + expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "update_pull_request", title: "No context title" })); + } finally { + global.context = savedContext; + } + }); + + it("should return intent error when workflow_dispatch has disallowed target_repo (SEC-005)", () => { + const savedContext = global.context; + global.context = { + ...global.context, + eventName: "workflow_dispatch", + payload: { + inputs: { + target_repo: "other-owner/other-repo", + }, + }, + }; + try { + const result = handlers.updatePullRequestHandler({ title: "PR update with disallowed repo" }); expect(result.isError).toBe(true); const responseData = JSON.parse(result.content[0].text); expect(responseData.result).toBe("error"); - expect(responseData.error).toContain('"workflow_dispatch"'); + expect(responseData.error).toContain("other-owner/other-repo"); expect(mockAppendSafeOutput).not.toHaveBeenCalled(); } finally { global.context = savedContext; @@ -3165,6 +3224,7 @@ describe("safe_outputs_handlers", () => { expect(result.isError).toBeUndefined(); const responseData = JSON.parse(result.content[0].text); expect(responseData.result).toBe("success"); + expect(responseData.hint).toBeUndefined(); expect(mockAppendSafeOutput).toHaveBeenCalledWith(expect.objectContaining({ type: "update_pull_request" })); } finally { global.context = savedContext;