Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
29 changes: 26 additions & 3 deletions actions/setup/js/safe_output_handler_manager.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldTolerateFatalFailures re-invokes partitionFailureResults redundantly, creating a subtle consistency risk.

💡 Details

main() already calls partitionFailureResults(processingResult.results) at line 1469 and stores the result in fatalFailures. shouldTolerateFatalFailures then calls partitionFailureResults a second time on the same array. This is wasteful on large result sets, and more importantly, the outer successCount (computed on line 1468 with r.success) and the inner successCount (computed inside the function with r?.success) use different null-safety, meaning they could theoretically diverge on malformed results.

Pass the pre-computed values in instead:

function shouldTolerateFatalFailures(successCount, fatalFailures) {
  return successCount > 0 && fatalFailures.length > 0;
}
// call site:
const tolerateFatalFailures = shouldTolerateFatalFailures(successCount, fatalFailures);

}

/**
* 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 @@ -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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 reason set to the resolution error message). These will be logged under the Skipped (no context or limit reached) label — misleading for target-configuration issues.

💡 Suggestion

Add 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 skippedTargetResults with a distinct label like Skipped (target not resolvable).

);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skippedHandlerResults now silently absorbs items with non-null reason, mislabeling them as "no context or limit reached".

💡 Details

The old filter was r.skipped && !r.reason && !r.deferred && !r.cancelled. The new filter drops !r.reason and instead explicitly excludes two specific reason strings. This means any skipped item that carries a reason OTHER than those two strings — for example, items returned by safe_output_processor.cjs with { success: false, skipped: true, reason: targetResult.error } — now falls into skippedHandlerResults and is logged under "Skipped (no context or limit reached)". The actual error reason is never surfaced.

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 !r.reason condition and add a separate bucket for reason-carrying skips.

core.info(`\n=== Processing Summary ===`);
core.info(`Total messages: ${processingResult.results.length}`);
Expand Down Expand Up @@ -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`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

💡 Details

These are fatalFailures — items that failed to process. Calling them "skipped" conflates them with intentionally-skipped items (e.g. standalone step handlers, limit-reached items) and makes it harder to diagnose a real failure. Suggested alternative:

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 skipped: true is already used elsewhere (intentional non-fatal target misses). Consider wording like "failed but were tolerated" to keep the vocabulary consistent and avoid misleading operators who grep for skipped in logs.

} 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"}`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] failedItemLines is computed here but only used in the !tolerateFatalFailures branch below. When failures are tolerated, the warning on line 1520 shows only a count — the operator loses visibility into which safe outputs failed and why.

💡 Suggestion

Log the detail lines as a separate core.warning even in the tolerate path:

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`);
Expand Down Expand Up @@ -1627,4 +1649,5 @@ module.exports = {
isFailedProcessingResult,
isReportOnlyFailureResult,
partitionFailureResults,
shouldTolerateFatalFailures,
};
19 changes: 19 additions & 0 deletions actions/setup/js/safe_output_handler_manager.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
isFailedProcessingResult,
isReportOnlyFailureResult,
partitionFailureResults,
shouldTolerateFatalFailures,
} from "./safe_output_handler_manager.cjs";

const require = createRequire(import.meta.url);
Expand Down Expand Up @@ -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" },

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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. shouldTolerateFatalFailures should return false when fatalFailures.length === 0, and a test would lock that in.

💡 Suggested test
it("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", () => {
Expand Down
3 changes: 2 additions & 1 deletion actions/setup/js/safe_output_processor.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ const { ERR_VALIDATION } = require("./error_codes.cjs");
* @property {number} [targetResult.number] - Target issue/PR number
* @property {string} [targetResult.contextType] - Type of context (issue or pull request)
* @property {string} [reason] - Reason why processing should not continue
* @property {boolean} [skipped] - Whether processing was intentionally skipped without failure
*/

/**
Expand Down Expand Up @@ -187,7 +188,7 @@ async function processSafeOutput(config, stagedPreviewOptions, handlerConfig = n
} else {
core.info(targetResult.error);
}
return { success: false, reason: targetResult.error };
return { success: false, skipped: !targetResult.shouldFail, reason: targetResult.error };
}

return {
Expand Down
24 changes: 24 additions & 0 deletions actions/setup/js/safe_output_processor.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,30 @@ describe("safe_output_processor", () => {
expect(mockCore.setFailed).toHaveBeenCalled();
});

it("should mark unsupported triggering target as skipped instead of failed", async () => {
const agentOutput = {
items: [{ type: "test_type", value: "test_value" }],
};
fs.writeFileSync(outputFile, JSON.stringify(agentOutput));
process.env.GH_AW_AGENT_OUTPUT = outputFile;
process.env.GH_AW_TEST_TARGET = "triggering";
global.context = {
eventName: "schedule",
repo: {
owner: "testowner",
repo: "testrepo",
},
payload: {},
};

const result = await processor.processSafeOutput(defaultConfig, defaultStagedOptions);

expect(result.success).toBe(false);
expect(result.skipped).toBe(true);
expect(result.reason).toContain('Target is "triggering"');
expect(mockCore.setFailed).not.toHaveBeenCalled();
});

it("should find multiple items when findMultiple is true", async () => {
const agentOutput = {
items: [
Expand Down
99 changes: 99 additions & 0 deletions actions/setup/js/safe_outputs_handlers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment_id, thread_id, and review_id are entity IDs, not target/item numbers — including them in hasExplicitTargetNumber can produce false positives.

💡 Details

hasExplicitTargetNumber is used to decide whether a wildcard (*) target is safe to emit because "the agent provided an explicit target". But comment_id, thread_id, and review_id identify specific sub-resources within an issue or PR, not the issue/PR itself. An agent that passes only comment_id: 456 with no issue_number or pull_request_number will bypass boundary validation, but the downstream routing will still have no issue/PR context to resolve against.

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
];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] comment_id, thread_id, and review_id are child-resource identifiers, not top-level issue/PR targets. An agent emitting comment_id: 123 on a target: "*" create_issue call would bypass this validation even though no issue/PR target is present. The error message also only mentions item_number, issue_number, pull_request_number — the three extra fields are invisible to the agent trying to self-correct.

💡 Suggestion

Split the field set: keep item_number, issue_number, pull_request_number, pr_number, pr, pull_number, and discussion_number as valid bypass-fields, and remove comment_id, thread_id, and review_id. Update the error message to list every accepted field so agents know exactly what to include.

];

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
Expand Down Expand Up @@ -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);
Comment thread
pelikhan marked this conversation as resolved.
Resolved
if (targetValidationResponse) return targetValidationResponse;
appendSafeOutput(entry);

return {
Expand All @@ -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: [
Expand Down Expand Up @@ -310,6 +391,8 @@ function createHandlers(server, appendSafeOutput, config = {}) {
targetFileName: targetFileName,
};

const targetValidationResponse = maybeRejectEmitBoundaryTarget(entry);
if (targetValidationResponse) return targetValidationResponse;
appendSafeOutput(entry);

return {
Expand Down Expand Up @@ -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: [
Expand Down Expand Up @@ -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: [
Expand All @@ -660,6 +747,8 @@ function createHandlers(server, appendSafeOutput, config = {}) {
};
}

const targetValidationResponse = maybeRejectEmitBoundaryTarget(entry);
if (targetValidationResponse) return targetValidationResponse;
appendSafeOutput(entry);
return {
content: [
Expand Down Expand Up @@ -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: [
Expand All @@ -990,6 +1081,8 @@ function createHandlers(server, appendSafeOutput, config = {}) {
};
}

const targetValidationResponse = maybeRejectEmitBoundaryTarget(entry);
if (targetValidationResponse) return targetValidationResponse;
appendSafeOutput(entry);
return {
content: [
Expand Down Expand Up @@ -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: [
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading
Loading