Tolerate partial safe-output failures and reject unsupported targets at emit time - #36528

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

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.

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.

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

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.

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

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