Skip to content

Fix safe-output summary outcome classification #51484

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.

Sign up for GitHub

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

Merged
dsyme merged 6 commits into from
Aug 9, 2026
Merged

Fix safe-output summary outcome classification #51484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dab683d
Initial plan
Copilot Aug 9, 2026
4fa5ace
Fix safe output outcome summaries
Copilot Aug 9, 2026
afd59a6
Address safe-output summary review feedback
dsyme Aug 9, 2026
ccf5c82
Deduplicate skipped safe-output results
dsyme Aug 9, 2026
38acfa2
Clarify delegated safe-output outcomes
dsyme Aug 9, 2026
328e81d
Merge branch 'main' into copilot/fix-safe-output-run-summary
dsyme Aug 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 38 additions & 3 deletions actions/setup/js/add_comment.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,8 @@ async function main(config = {}) {
return {
success: false,
skipped: true,
reasonCode: "MAX_COUNT_REACHED",
reason: "Max count reached",
error: `Max count of ${maxCount} reached`,
};
}
Expand Down Expand Up @@ -682,6 +684,8 @@ async function main(config = {}) {
return {
success: false,
skipped: true,
reasonCode: "NO_CONTEXT",
reason: "No target context available",
error: targetResult.error,
};
}
Expand All @@ -696,6 +700,8 @@ async function main(config = {}) {
return {
success: false,
skipped: true,
reasonCode: "NO_CONTEXT",
reason: "No target context available",
error: targetResult.error,
};
}
Expand All @@ -716,14 +722,31 @@ async function main(config = {}) {
});
if (requiredLabels.length > 0) {
const itemLabels = (filterItem.labels || []).map(/** @param {any} l */ l => (typeof l === "string" ? l : l.name || ""));
if (!requiredLabels.every(r => itemLabels.includes(r))) {
const missingLabels = requiredLabels.filter(r => !itemLabels.includes(r));
if (missingLabels.length > 0) {
core.info(`Skipping add_comment for #${itemNumber}: does not match required-labels filter (${requiredLabels.join(", ")})`);
return { success: false, skipped: true, error: `Item does not match required-labels filter` };
return {
success: false,
skipped: true,
reasonCode: "REQUIRED_LABELS_MISMATCH",
reason: "Required labels missing",
error: "Item does not match required-labels filter",
target: { repo: itemRepo, number: itemNumber },
safeDetails: { requiredLabels, missingLabels },
};
}
}
if (requiredTitlePrefix && !filterItem.title?.startsWith(requiredTitlePrefix)) {
core.info(`Skipping add_comment for #${itemNumber}: title does not start with required prefix "${requiredTitlePrefix}"`);
return { success: false, skipped: true, error: `Item title does not start with required prefix` };
return {
success: false,
skipped: true,
reasonCode: "REQUIRED_TITLE_PREFIX_MISMATCH",
reason: "Required title prefix missing",
error: "Item title does not start with required prefix",
target: { repo: itemRepo, number: itemNumber },
safeDetails: { requiredTitlePrefix },
};
}
} catch (err) {
core.warning(`Could not fetch item #${itemNumber} to check filters: ${getErrorMessage(err)}`);
Expand Down Expand Up @@ -1049,6 +1072,9 @@ async function main(config = {}) {
success: true,
warning: `Target not found: ${discussionErrorMessage}`,
skipped: true,
reasonCode: "TARGET_NOT_FOUND",
reason: "Target not found",
target: { repo: itemRepo, number: itemNumber },
};
}

Expand All @@ -1063,6 +1089,9 @@ async function main(config = {}) {
return {
success: false,
skipped: true,
reasonCode: "DISCUSSIONS_TOKEN_SCOPE_MISMATCH",
reason: "GitHub token cannot add comments to discussions",
target: { repo: itemRepo, number: itemNumber },
error: warningMessage,
};
}
Expand All @@ -1083,6 +1112,9 @@ async function main(config = {}) {
success: true,
warning: `Target not found: ${errorMessage}`,
skipped: true,
reasonCode: "TARGET_NOT_FOUND",
reason: "Target not found",
target: { repo: itemRepo, number: itemNumber },
};
}

Expand All @@ -1093,6 +1125,9 @@ async function main(config = {}) {
success: true,
warning: `Target is locked: ${errorMessage}`,
skipped: true,
reasonCode: "TARGET_LOCKED",
reason: "Target is locked",
target: { repo: itemRepo, number: itemNumber },
};
}

Expand Down
23 changes: 20 additions & 3 deletions actions/setup/js/add_labels.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,14 +232,31 @@
});
if (requiredLabels.length > 0) {
const itemLabels = (item.labels || []).map(/** @param {any} l */ l => (typeof l === "string" ? l : l.name || ""));
if (!requiredLabels.every(r => itemLabels.includes(r))) {
const missingLabels = requiredLabels.filter(r => !itemLabels.includes(r));
if (missingLabels.length > 0) {
core.info(`Skipping add_labels for ${contextType} #${itemNumber}: does not match required-labels filter (${requiredLabels.join(", ")})`);
return { success: false, skipped: true, error: `Item does not match required-labels filter` };
return {
success: false,
skipped: true,
reasonCode: "REQUIRED_LABELS_MISMATCH",
reason: "Required labels missing",
error: "Item does not match required-labels filter",
target: { repo: itemRepo, number: itemNumber },
safeDetails: { requiredLabels, missingLabels },
};
}
}
if (requiredTitlePrefix && !item.title?.startsWith(requiredTitlePrefix)) {
core.info(`Skipping add_labels for ${contextType} #${itemNumber}: title does not start with required prefix "${requiredTitlePrefix}"`);
return { success: false, skipped: true, error: `Item title does not start with required prefix` };
return {
success: false,
skipped: true,
reasonCode: "REQUIRED_TITLE_PREFIX_MISMATCH",
reason: "Required title prefix missing",
error: "Item title does not start with required prefix",
target: { repo: itemRepo, number: itemNumber },
safeDetails: { requiredTitlePrefix },
};
}
}

Expand Down Expand Up @@ -345,7 +362,7 @@

const issueNodeId = issueData?.node_id;
if (!issueNodeId) {
throw new Error(`${SAFE_OUTPUT_E099}: Failed to resolve GraphQL node ID for ${contextType} #${itemNumber}`);

Check warning on line 365 in actions/setup/js/add_labels.cjs

View workflow job for this annotation

GitHub Actions / lint-js 

This file imports error_codes.cjs but this thrown Error message does not reference a standardized error code (e.g. ERR_API, ERR_NOT_FOUND). Prefix the message with an imported ERR_* constant for consistency with other errors in this file
}

// The GraphQL updateIssue mutation only accepts Issue node IDs, and
Expand Down
96 changes: 71 additions & 25 deletions actions/setup/js/safe_output_handler_manager.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
const { loadAgentOutput } = require("./load_agent_output.cjs");
const { getErrorMessage } = require("./error_helpers.cjs");
const { ERR_CONFIG, ERR_PARSE, ERR_VALIDATION } = require("./error_codes.cjs");
const { computeSafeOutputsStatus, isFailedProcessingResult } = require("./safe_outputs_status.cjs");
const { classifySafeOutputResult, computeSafeOutputsStatus, isFailedProcessingResult } = require("./safe_outputs_status.cjs");
const { hasUnresolvedTemporaryIds, replaceTemporaryIdReferences, replaceArtifactUrlReferences, normalizeTemporaryId } = require("./temporary_id.cjs");
const { generateMissingInfoSections } = require("./missing_info_formatter.cjs");
const { setCollectedMissings } = require("./missing_messages_helper.cjs");
Expand Down Expand Up @@ -623,9 +623,6 @@ function rollbackReviewResultsForPR(results, repo, prNumber, errorMessage) {
* the skip must be back-propagated here so the Processing Summary reflects the actual
* outcome (skipped) rather than a misleading success count.
*
* Note: uses `skipReason` (not `reason`) so that the step-summary generator does not
* treat these entries as delegated-step skips and omit them from the output.
*
* @param {Array<{type: string, success: boolean, skipped?: boolean, skipReason?: string}>} results - Processing results to mutate
* @param {string} skipReason - Human-readable reason for the skip
*/
Expand Down Expand Up @@ -691,14 +688,41 @@ function partitionFailureResults(results) {
/**
* Export item-level safe-output status as GitHub Actions outputs.
*
* @param {{itemsSucceeded: number, itemsFailed: number, status: string}} status
* @param {{itemsSucceeded: number, itemsApplied?: number, itemsSkipped?: number, itemsWarnings?: number, itemsCancelled?: number, itemsDeferred?: number, itemsFailed: number, status: string}} status
*/
function setSafeOutputsStatusOutputs(status) {
core.setOutput("items_succeeded", String(status.itemsSucceeded));
core.setOutput("items_applied", String(status.itemsApplied ?? status.itemsSucceeded));
core.setOutput("items_skipped", String(status.itemsSkipped ?? 0));
core.setOutput("items_warnings", String(status.itemsWarnings ?? 0));
core.setOutput("items_cancelled", String(status.itemsCancelled ?? 0));
core.setOutput("items_deferred", String(status.itemsDeferred ?? 0));
Comment thread
dsyme marked this conversation as resolved.
core.setOutput("items_failed", String(status.itemsFailed));
core.setOutput("status", status.status);
}

/**
* @param {string} type
* @param {number} messageIndex
* @param {Record<string, any>} result
* @returns {Record<string, any>}
*/
function buildSkippedResult(type, messageIndex, result) {
const message = result.reason || result.warning || result.error || "Handler returned skipped: true";
return {
type,
messageIndex,
success: result.success === true,
skipped: true,
...(result.warning ? { warning: result.warning } : {}),
...(result.reason ? { reason: result.reason } : {}),
...(result.reasonCode ? { reasonCode: result.reasonCode } : {}),
...(result.errorCode ? { errorCode: result.errorCode } : {}),
error: message,
result,
};
}

/**
* 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)
Expand Down Expand Up @@ -808,6 +832,7 @@ async function processMessages(messageHandlers, messages, onItemCreated = null)
messageIndex: i,
success: false,
skipped: true,
delegated: true,
reason: "Handled by standalone step",
});
continue;
Expand Down Expand Up @@ -842,6 +867,7 @@ async function processMessages(messageHandlers, messages, onItemCreated = null)
messageIndex: i,
success: false,
skipped: true,
delegated: true,
reason: "Handled by custom safe output job",
});
continue;
Expand Down Expand Up @@ -908,18 +934,14 @@ async function processMessages(messageHandlers, messages, onItemCreated = null)
// Call the message handler with the individual message and resolved temp IDs
const result = await messageHandler(effectiveMessage, resolvedTemporaryIds, temporaryIdMap);

// Check if the handler explicitly returned a skipped result (e.g. if_no_changes: warn/ignore).
// Skipped results should NOT trigger fail-fast cancellation of subsequent messages.
if (result && result.success === false && result.skipped === true && !result.deferred) {
const msg = result.error || "Handler returned success: false with skipped: true";
// Check if the handler explicitly returned a skipped result (e.g. policy filters,
// no-op warnings, or if_no_changes: warn/ignore). Skipped results should NOT
// trigger fail-fast cancellation of subsequent messages, and any summary-safe
// diagnostics supplied by the handler must be preserved.
if (result && result.skipped === true && !result.deferred) {
const msg = result.reason || result.warning || result.error || "Handler returned skipped: true";
core.info(`⏭ Message ${i + 1} (${messageType}) skipped — ${msg}`);
results.push({
type: messageType,
messageIndex: i,
success: false,
skipped: true,
error: msg,
});
results.push(buildSkippedResult(messageType, i, result));
continue;
}

Expand Down Expand Up @@ -1093,15 +1115,31 @@ async function processMessages(messageHandlers, messages, onItemCreated = null)
// Call the handler again with updated temp ID map
const result = await deferred.handler(deferred.message, resolvedTemporaryIds, temporaryIdMap);

if (result && result.skipped === true && !result.deferred) {
Comment thread
dsyme marked this conversation as resolved.
const msg = result.reason || result.warning || result.error || "Handler returned skipped: true";
core.info(`⏭ Retry of message ${deferred.messageIndex + 1} (${deferred.type}) skipped — ${msg}`);
const resultIndex = results.findIndex(r => r.messageIndex === deferred.messageIndex);
if (resultIndex >= 0) {
results[resultIndex] = buildSkippedResult(deferred.type, deferred.messageIndex, result);
}
continue;
}

// Check if the handler explicitly returned a failure
if (result && result.success === false && !result.deferred) {
const errorMsg = result.error || "Handler returned success: false";
core.error(`✗ Retry of message ${deferred.messageIndex + 1} (${deferred.type}) failed: ${errorMsg}`);
// Update the result to error
// Replace the deferred record so terminal retry failures classify as failed.
const resultIndex = results.findIndex(r => r.messageIndex === deferred.messageIndex);
if (resultIndex >= 0) {
results[resultIndex].success = false;
results[resultIndex].error = errorMsg;
results[resultIndex] = {
type: deferred.type,
messageIndex: deferred.messageIndex,
success: false,
deferred: false,
error: errorMsg,
result: { ...result, success: false, deferred: false, error: errorMsg },
};
}
continue;
}
Expand Down Expand Up @@ -1157,11 +1195,19 @@ async function processMessages(messageHandlers, messages, onItemCreated = null)
logCreatedItemFromResult(onItemCreated, deferred.type, result);
}
} catch (error) {
core.error(`✗ Retry of message ${deferred.messageIndex + 1} (${deferred.type}) failed: ${getErrorMessage(error)}`);
// Update the result to error
const errorMsg = getErrorMessage(error);
core.error(`✗ Retry of message ${deferred.messageIndex + 1} (${deferred.type}) failed: ${errorMsg}`);
// Replace the deferred record so terminal retry exceptions classify as failed.
const resultIndex = results.findIndex(r => r.messageIndex === deferred.messageIndex);
if (resultIndex >= 0) {
results[resultIndex].error = getErrorMessage(error);
results[resultIndex] = {
type: deferred.type,
messageIndex: deferred.messageIndex,
success: false,
deferred: false,
error: errorMsg,
result: { success: false, deferred: false, error: errorMsg },
};
}
}
}
Expand Down Expand Up @@ -1606,10 +1652,10 @@ async function main() {
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 skippedStandaloneResults = processingResult.results.filter(r => r.delegated && r.reason === "Handled by standalone step");
const skippedCustomJobResults = processingResult.results.filter(r => r.delegated && 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 => classifySafeOutputResult(r) === "skipped");

core.info(`\n=== Processing Summary ===`);
core.info(`Total messages: ${processingResult.results.length}`);
Expand Down
Loading
Loading