Keep assign_to_agent failures from failing safe_outputs - #36112
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates safe output handling so assign_to_agent failures are reported without causing the overall safe_outputs job to fail, while preserving fatal behavior for other safe output failures.
Changes:
- Adds helper functions to classify failed processing results and partition fatal vs report-only failures.
- Updates
main()failure handling to excludeassign_to_agentfailures fromcore.setFailed. - Adds focused unit tests for failure classification and partitioning behavior.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/safe_output_handler_manager.cjs |
Adds failure classification helpers and applies report-only handling for assignment failures. |
actions/setup/js/safe_output_handler_manager.test.cjs |
Adds tests covering active/skipped/deferred/cancelled failure classification and partitioning. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #36112 does not have the 'implementation' label and has 0 new lines of code in business logic directories (≤100 threshold). Pre-fetched summary: has_implementation_label=false, requires_adr_by_default_volume=false, default_business_additions=0. |
There was a problem hiding this comment.
One non-blocking issue flagged (PR already merged).
### Findings
Medium: abstraction inconsistency in partitionFailureResults — the function re-implements the r?.type === "assign_to_agent" predicate inline instead of delegating to isReportOnlyFailureResult. If a second report-only handler type is ever added to isReportOnlyFailureResult, partitionFailureResults will silently disagree. The fix is one line: replace the inline filter predicates with calls to the exported function.
Everything else (classification logic, job-outcome semantics, test coverage for the new paths) looks correct.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 770K
| * @returns {{fatalFailures: Array<any>, reportOnlyFailures: Array<any>}} | ||
| */ | ||
| function partitionFailureResults(results) { | ||
| const failedResults = results.filter(isFailedProcessingResult); |
There was a problem hiding this comment.
partitionFailureResults duplicates the type check instead of calling isReportOnlyFailureResult, creating a maintenance trap where the two can silently diverge.
💡 Suggested fix
Replace the inline re-implementation:
// current — two separate places define what "report-only" means
const reportOnlyFailures = failedResults.filter(r => r?.type === "assign_to_agent");
const fatalFailures = failedResults.filter(r => r?.type !== "assign_to_agent");With a call to the already-exported predicate:
const reportOnlyFailures = failedResults.filter(isReportOnlyFailureResult);
const fatalFailures = failedResults.filter(r => !isReportOnlyFailureResult(r));Now there is a single source of truth: adding a second report-only type to isReportOnlyFailureResult automatically updates the partitioning, and the relationship between the two functions is explicit and verifiable.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- DRY opportunity:
partitionFailureResultsinlines theassign_to_agenttype check rather than delegating toisReportOnlyFailureResult, creating a latent drift risk if new report-only types are added later. - Test coverage gaps: the helpers' null/undefined guard cases and the negative path of
isReportOnlyFailureResult(non-assign type, successful assign) are untested.
Positive Highlights
- ✅ Clean extraction of three pure helpers with clear JSDoc — easy to read and reason about
- ✅
core.setFailedis now gated only onfatalFailures, which is the right fix - ✅ Assignment failures are still surfaced via
core.warningso downstream signal is preserved - ✅ Focused unit tests cover the happy path and skipped/deferred/cancelled exclusions well
The core fix is correct and well-structured. The inline comments flag small improvements that would tighten the test contract and remove the one duplication.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.6M
| return { fatalFailures, reportOnlyFailures }; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
[/tdd] partitionFailureResults duplicates the assign_to_agent type check instead of reusing isReportOnlyFailureResult — a DRY violation that could cause drift if a second report-only type is ever added.
💡 Suggested refactor
function partitionFailureResults(results) {
const failedResults = results.filter(isFailedProcessingResult);
const reportOnlyFailures = failedResults.filter(isReportOnlyFailureResult);
return { fatalFailures, reportOnlyFailures };
}This way the single definition of what counts as report-only lives in isReportOnlyFailureResult and partitionFailureResults is just composition.
| describe("report-only assignment failures", () => { | ||
| it("recognizes only active failures as failed processing results", () => { | ||
| expect(isFailedProcessingResult({ success: false })).toBe(true); | ||
| expect(isFailedProcessingResult({ success: false, deferred: true })).toBe(false); |
There was a problem hiding this comment.
[/tdd] The isFailedProcessingResult tests don't verify null/undefined guard cases, even though the implementation uses optional chaining (result?.) for that purpose.
💡 Suggested additions
expect(isFailedProcessingResult(null)).toBe(false);
expect(isFailedProcessingResult(undefined)).toBe(false);
expect(isFailedProcessingResult({ success: true })).toBe(false);These pin the null-safety contract so a future refactor that drops optional chaining is caught immediately.
| }) | ||
| ).toBe(true); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] Missing a complementary test: isReportOnlyFailureResult should return false for a non-assign_to_agent failure (e.g. create_issue). Without it, the positive-only coverage could mask a bug where all failures are treated as report-only.
💡 Suggested addition
it('does not treat non-assign_to_agent failures as report-only', () => {
expect(isReportOnlyFailureResult({ type: 'create_issue', success: false })).toBe(false);
expect(isReportOnlyFailureResult({ type: 'assign_to_agent', success: true })).toBe(false);
});
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (5 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
|
safe_outputscould fail after already applying successful items when a laterassign_to_agentstep hit a token/permission error. In practice this left runs in a partial-success state: issues were created, assignment failed, and the whole job concludedfailure.Failure classification
assign_to_agenthandler failures as report-only in the safe output handler manager.Job outcome semantics
assign_to_agentfailures from the fatalsafe_outputsfailure count.Focused coverage