Repository navigation
Tolerate partial safe-output failures and reject unsupported targets at emit time #36528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
004218c
657d026
dc10941
d7d8ff4
6e59854
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -567,6 +567,19 @@ function partitionFailureResults(results) { | |
| return { fatalFailures, reportOnlyFailures }; | ||
| } | ||
|
|
||
| /** | ||
| * Determine whether fatal safe-output item failures should be tolerated as warnings. | ||
| * This keeps the job green-with-warning when at least one item succeeded in the batch. | ||
| * | ||
| * @param {Array<{success?: boolean}>} results | ||
| * @returns {boolean} | ||
| */ | ||
| function shouldTolerateFatalFailures(results) { | ||
| const successCount = results.filter(r => r?.success).length; | ||
| const { fatalFailures } = partitionFailureResults(results); | ||
| return successCount > 0 && fatalFailures.length > 0; | ||
| } | ||
|
|
||
| /** | ||
| * Process all messages from agent output in the order they appear | ||
| * Dispatches each message to the appropriate handler while maintaining shared state (temporary ID map) | ||
|
|
@@ -1455,13 +1468,16 @@ async function main() { | |
| const successCount = processingResult.results.filter(r => r.success).length; | ||
| const { fatalFailures, reportOnlyFailures } = partitionFailureResults(processingResult.results); | ||
| const failureCount = fatalFailures.length; | ||
| const tolerateFatalFailures = shouldTolerateFatalFailures(processingResult.results); | ||
| const reportOnlyFailureCount = reportOnlyFailures.length; | ||
| const cancelledCount = processingResult.results.filter(r => r.cancelled).length; | ||
| const deferredCount = processingResult.results.filter(r => r.deferred).length; | ||
| const skippedStandaloneResults = processingResult.results.filter(r => r.skipped && r.reason === "Handled by standalone step"); | ||
| const skippedCustomJobResults = processingResult.results.filter(r => r.skipped && r.reason === "Handled by custom safe output job"); | ||
| const skippedNoHandlerResults = processingResult.results.filter(r => !r.success && !r.skipped && r.error?.includes("No handler loaded")); | ||
| const skippedHandlerResults = processingResult.results.filter(r => r.skipped && !r.reason && !r.deferred && !r.cancelled); | ||
| const skippedHandlerResults = processingResult.results.filter( | ||
| r => r.skipped && !r.deferred && !r.cancelled && r.reason !== "Handled by standalone step" && r.reason !== "Handled by custom safe output job" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnose] The updated filter now includes processor-level target-miss skips (which have 💡 SuggestionAdd a dedicated bucket for processor-level skips so the summary label is accurate: const skippedTargetResults = processingResult.results.filter(
r => r.skipped && !r.deferred && !r.cancelled && r.reason && ![
"Handled by standalone step",
"Handled by custom safe output job"
].includes(r.reason)
);
const skippedHandlerResults = processingResult.results.filter(
r => r.skipped && !r.deferred && !r.cancelled && !r.reason
);Then log |
||
| ); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💡 DetailsThe old filter was At minimum, the summary log should print the reasons for items in this bucket: if (skippedHandlerResults.length > 0) {
core.info(`Skipped (no context or limit reached): ${skippedHandlerResults.length}`);
const reasonGroups = skippedHandlerResults.map(r => ` - ${r.type}: ${r.reason || 'no reason'}`);
core.info(reasonGroups.join('\n'));
}Alternatively, keep the |
||
| core.info(`\n=== Processing Summary ===`); | ||
| core.info(`Total messages: ${processingResult.results.length}`); | ||
|
|
@@ -1500,10 +1516,16 @@ async function main() { | |
| core.info(`Synthetic updates: ${syntheticUpdateCount}`); | ||
|
|
||
| if (failureCount > 0) { | ||
| core.warning(`${failureCount} message(s) failed to process`); | ||
| if (tolerateFatalFailures) { | ||
| core.warning(`${failureCount} message(s) were skipped because ${successCount} other message(s) succeeded`); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "were skipped" is inaccurate language for fatal failures — it obscures the real problem from users reading logs. 💡 DetailsThese are core.warning(`${failureCount} message(s) failed but were tolerated because ${successCount} other message(s) succeeded`);This makes it clear that failure occurred, rather than suggesting the messages were benignly bypassed.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The warning text says "were skipped" but these items genuinely failed — they're only tolerated, not skipped. Calling them "skipped" is inconsistent with how |
||
| } else { | ||
|
Comment on lines
+1524
to
+1528
|
||
| core.warning(`${failureCount} message(s) failed to process`); | ||
| } | ||
| const failedItemLines = fatalFailures.map(r => ` - ${r.type}: ${r.error || "Unknown error"}`); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnose] 💡 SuggestionLog the detail lines as a separate if (tolerateFatalFailures) {
core.warning(`${failureCount} message(s) were tolerated (${successCount} succeeded): \n${failedItems}`);
} else {
core.warning(`${failureCount} message(s) failed to process`);
core.setFailed(`${failureCount} safe output(s) failed:\n${failedItems}`);
} |
||
| const failedItems = failedItemLines.join("\n"); | ||
| core.setFailed(`${failureCount} safe output(s) failed:\n${failedItems}`); | ||
| if (!tolerateFatalFailures) { | ||
| core.setFailed(`${failureCount} safe output(s) failed:\n${failedItems}`); | ||
| } | ||
| } | ||
| if (reportOnlyFailureCount > 0) { | ||
| core.warning(`${reportOnlyFailureCount} agent assignment(s) failed but were reported without failing safe_outputs`); | ||
|
|
@@ -1627,4 +1649,5 @@ module.exports = { | |
| isFailedProcessingResult, | ||
| isReportOnlyFailureResult, | ||
| partitionFailureResults, | ||
| shouldTolerateFatalFailures, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ import { | |
| isFailedProcessingResult, | ||
| isReportOnlyFailureResult, | ||
| partitionFailureResults, | ||
| shouldTolerateFatalFailures, | ||
| } from "./safe_output_handler_manager.cjs"; | ||
|
|
||
| const require = createRequire(import.meta.url); | ||
|
|
@@ -146,6 +147,24 @@ describe("Safe Output Handler Manager", () => { | |
| expect(reportOnlyFailures).toEqual([{ type: "assign_to_agent", success: false, error: "Insufficient permissions" }]); | ||
| expect(fatalFailures).toEqual([{ type: "create_issue", success: false, error: "Validation failed" }]); | ||
| }); | ||
|
|
||
| it("tolerates fatal failures when another safe output succeeded", () => { | ||
| expect( | ||
| shouldTolerateFatalFailures([ | ||
| { type: "create_issue", success: true }, | ||
| { type: "update_issue", success: false, error: "Target is invalid" }, | ||
| ]) | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| it("does not tolerate fatal failures when nothing succeeded", () => { | ||
| expect( | ||
| shouldTolerateFatalFailures([ | ||
| { type: "update_issue", success: false, error: "Target is invalid" }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] Two good cases, but the all-success batch (no failures at all) is not tested. 💡 Suggested testit("does not tolerate when there are no failures", () => {
expect(
shouldTolerateFatalFailures([
{ type: "create_issue", success: true },
{ type: "update_issue", success: true },
])
).toBe(false);
}); |
||
| { type: "assign_to_agent", success: false, error: "No permission" }, | ||
| ]) | ||
| ).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe("loadHandlers", () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -131,6 +131,83 @@ function resolvePatchWorkspacePath(workspacePath) { | |
| function createHandlers(server, appendSafeOutput, config = {}) { | ||
| const TOKEN_THRESHOLD = 16000; | ||
|
|
||
| /** | ||
| * @param {string} eventName | ||
| * @param {any} payload | ||
| * @returns {boolean} | ||
| */ | ||
| const isIssueOrPullRequestContext = (eventName, payload) => { | ||
| const prEventNames = new Set(["pull_request", "pull_request_target", "pull_request_review", "pull_request_review_comment"]); | ||
| const isIssueCommentOnPR = eventName === "issue_comment" && Boolean(payload?.issue?.pull_request); | ||
| const isIssueContext = eventName === "issues" || (eventName === "issue_comment" && !isIssueCommentOnPR); | ||
| const isPRContext = prEventNames.has(eventName) || isIssueCommentOnPR; | ||
| return isIssueContext || isPRContext; | ||
| }; | ||
|
|
||
| /** | ||
| * @param {Record<string, any>} entry | ||
| * @returns {boolean} | ||
| */ | ||
| const hasExplicitTargetNumber = entry => { | ||
| const candidateFields = [ | ||
| "item_number", | ||
| "issue_number", | ||
| "pull_request_number", | ||
| "pr_number", | ||
| "pr", | ||
| "pull_number", | ||
| "discussion_number", | ||
| "comment_id", | ||
| "thread_id", | ||
| "review_id", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
💡 Details
Suggested fix: remove these three fields from the candidate list. If routing for comment-level tools genuinely needs comment IDs to resolve the parent, that should be an explicit lookup in the handler, not a bypass gate. const candidateFields = [
"item_number",
"issue_number",
"pull_request_number",
"pr_number",
"pr",
"pull_number",
"discussion_number",
// comment_id, thread_id, review_id removed — not item/target numbers
];
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/diagnose] 💡 SuggestionSplit the field set: keep |
||
| ]; | ||
|
|
||
| return candidateFields.some(field => { | ||
| const value = entry?.[field]; | ||
| if (value === undefined || value === null || value === "") return false; | ||
| const parsed = typeof value === "number" ? value : parseInt(String(value), 10); | ||
| return Number.isInteger(parsed) && parsed > 0; | ||
| }); | ||
| }; | ||
|
|
||
| /** | ||
| * Reject or at emit time when the current event | ||
| * cannot provide an implicit issue/PR target and the item does not supply one. | ||
| * | ||
| * @param {Record<string, any>} entry | ||
| * @returns {string|null} | ||
| */ | ||
| const validateEmitBoundaryTarget = entry => { | ||
| const normalizedType = String(entry?.type || "").replace(/-/g, "_"); | ||
| const configuredTarget = typeof config?.[normalizedType]?.target === "string" ? config[normalizedType].target.trim() : ""; | ||
| if (configuredTarget !== "triggering" && configuredTarget !== "*") { | ||
| return null; | ||
| } | ||
|
|
||
| const eventName = context?.eventName || "unknown"; | ||
| if (isIssueOrPullRequestContext(eventName, context?.payload)) { | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| if (configuredTarget === "*") { | ||
| if (hasExplicitTargetNumber(entry)) { | ||
| return null; | ||
| } | ||
| return `Configured target for '${normalizedType}' is "*" but the current event '${eventName}' is not an issue or pull request event, and this MCP call did not include an explicit target number. Include item_number, issue_number, or pull_request_number in the tool call, use an explicit numeric target, or run the workflow from an issue or pull request event.`; | ||
| } | ||
|
|
||
| return `Configured target for '${normalizedType}' is "triggering" but the current event '${eventName}' is not an issue or pull request event. Use an explicit numeric target, switch to target "*" with an item_number/issue_number/pull_request_number in the tool call, or run the workflow from an issue or pull request event.`; | ||
| }; | ||
|
|
||
|
|
||
| /** | ||
| * @param {Record<string, any>} entry | ||
| * @returns {{content: Array<{type: "text", text: string}>, isError: true}|null} | ||
| */ | ||
| const maybeRejectEmitBoundaryTarget = entry => { | ||
| const targetError = validateEmitBoundaryTarget(entry); | ||
| return targetError ? buildIntentErrorResponse(targetError) : null; | ||
| }; | ||
|
|
||
| /** | ||
| * Detect and offload large string fields to files. | ||
| * @param {Record<string, any>} entry | ||
|
|
@@ -158,6 +235,8 @@ function createHandlers(server, appendSafeOutput, config = {}) { | |
|
|
||
| const fileInfo = writeLargeContentToFile(largeContent); | ||
| entry[largeFieldName] = `[Content too large, saved to file: ${fileInfo.filename}]`; | ||
| const targetValidationResponse = maybeRejectEmitBoundaryTarget(entry); | ||
|
pelikhan marked this conversation as resolved.
Resolved
|
||
| if (targetValidationResponse) return targetValidationResponse; | ||
| appendSafeOutput(entry); | ||
|
|
||
|
|
||
| return { | ||
|
|
@@ -182,6 +261,8 @@ function createHandlers(server, appendSafeOutput, config = {}) { | |
| if (largeContentResponse) return largeContentResponse; | ||
|
|
||
| // Normal case - no large content | ||
| const targetValidationResponse = maybeRejectEmitBoundaryTarget(entry); | ||
| if (targetValidationResponse) return targetValidationResponse; | ||
| appendSafeOutput(entry); | ||
| return { | ||
| content: [ | ||
|
|
@@ -310,6 +391,8 @@ function createHandlers(server, appendSafeOutput, config = {}) { | |
| targetFileName: targetFileName, | ||
| }; | ||
|
|
||
| const targetValidationResponse = maybeRejectEmitBoundaryTarget(entry); | ||
| if (targetValidationResponse) return targetValidationResponse; | ||
| appendSafeOutput(entry); | ||
|
|
||
| return { | ||
|
|
@@ -501,6 +584,8 @@ function createHandlers(server, appendSafeOutput, config = {}) { | |
| if (allowEmpty) { | ||
| server.debug(`allow-empty is enabled for create_pull_request - skipping patch generation`); | ||
| // Append the safe output entry without generating a patch | ||
| const targetValidationResponse = maybeRejectEmitBoundaryTarget(entry); | ||
| if (targetValidationResponse) return targetValidationResponse; | ||
| appendSafeOutput(entry); | ||
| return { | ||
| content: [ | ||
|
|
@@ -638,6 +723,8 @@ function createHandlers(server, appendSafeOutput, config = {}) { | |
| entry.base_commit = bundleResult.baseCommit; | ||
| } | ||
|
|
||
| const targetValidationResponse = maybeRejectEmitBoundaryTarget(entry); | ||
| if (targetValidationResponse) return targetValidationResponse; | ||
| appendSafeOutput(entry); | ||
| return { | ||
| content: [ | ||
|
|
@@ -660,6 +747,8 @@ function createHandlers(server, appendSafeOutput, config = {}) { | |
| }; | ||
| } | ||
|
|
||
| const targetValidationResponse = maybeRejectEmitBoundaryTarget(entry); | ||
| if (targetValidationResponse) return targetValidationResponse; | ||
| appendSafeOutput(entry); | ||
| return { | ||
| content: [ | ||
|
|
@@ -968,6 +1057,8 @@ function createHandlers(server, appendSafeOutput, config = {}) { | |
| entry.base_commit = bundleResult.baseCommit; | ||
| } | ||
|
|
||
| const targetValidationResponse = maybeRejectEmitBoundaryTarget(entry); | ||
| if (targetValidationResponse) return targetValidationResponse; | ||
| appendSafeOutput(entry); | ||
| return { | ||
| content: [ | ||
|
|
@@ -990,6 +1081,8 @@ function createHandlers(server, appendSafeOutput, config = {}) { | |
| }; | ||
| } | ||
|
|
||
| const targetValidationResponse = maybeRejectEmitBoundaryTarget(entry); | ||
| if (targetValidationResponse) return targetValidationResponse; | ||
| appendSafeOutput(entry); | ||
| return { | ||
| content: [ | ||
|
|
@@ -1260,6 +1353,8 @@ function createHandlers(server, appendSafeOutput, config = {}) { | |
| const largeContentResponse = maybeHandleLargeContent(entry); | ||
| if (largeContentResponse) return largeContentResponse; | ||
|
|
||
| const targetValidationResponse = maybeRejectEmitBoundaryTarget(entry); | ||
| if (targetValidationResponse) return targetValidationResponse; | ||
| appendSafeOutput(entry); | ||
| return { | ||
| content: [ | ||
|
|
@@ -1291,6 +1386,8 @@ function createHandlers(server, appendSafeOutput, config = {}) { | |
| server.debug(`temporary_id for create_project: ${entry.temporary_id}`); | ||
|
|
||
| // Append to safe outputs | ||
| const targetValidationResponse = maybeRejectEmitBoundaryTarget(entry); | ||
| if (targetValidationResponse) return targetValidationResponse; | ||
| appendSafeOutput(entry); | ||
|
|
||
| // Return the temporary_id to the agent so it can reference this project | ||
|
|
@@ -1350,6 +1447,8 @@ function createHandlers(server, appendSafeOutput, config = {}) { | |
| server.debug(`temporary_id for add_comment: ${entry.temporary_id}`); | ||
|
|
||
| // Append to safe outputs | ||
| const targetValidationResponse = maybeRejectEmitBoundaryTarget(entry); | ||
| if (targetValidationResponse) return targetValidationResponse; | ||
| appendSafeOutput(entry); | ||
|
|
||
| // Return the temporary_id to the agent so it can reference this comment | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldTolerateFatalFailuresre-invokespartitionFailureResultsredundantly, creating a subtle consistency risk.💡 Details
main()already callspartitionFailureResults(processingResult.results)at line 1469 and stores the result infatalFailures.shouldTolerateFatalFailuresthen callspartitionFailureResultsa second time on the same array. This is wasteful on large result sets, and more importantly, the outersuccessCount(computed on line 1468 withr.success) and the innersuccessCount(computed inside the function withr?.success) use different null-safety, meaning they could theoretically diverge on malformed results.Pass the pre-computed values in instead: