Expose safe output partial-batch status counts - #50371
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (9 lines). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — two targeted observations, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- All-skipped edge case: When all results are skipped/deferred/cancelled,
statusemits"success"even though nothing completed. The intent should be documented or the status refined. items_failedsemantics: The counter includes report-only failures (non-fatal). Consumers who gate onpartial_successmay over-react to non-blocking errors.
Positive Highlights
- ✅ Clean extraction of
isFailedProcessingResultandcomputeSafeOutputsStatusinto a dedicated module — good separation of concerns. - ✅ Comprehensive test coverage for mixed success/failure, all-failure, and output-formatting scenarios.
- ✅
statusOutputsSetguard in the catch block ensures outputs are always emitted, even on unexpected errors. - ✅ Go compiler and tests updated in lock-step with the JS change — no drift.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32.5 AIC · ⌖ 8.1 AIC · ⊞ 7.1K
Comment /matt to run again
| const safeResults = Array.isArray(results) ? results : []; | ||
| const itemsSucceeded = safeResults.filter(r => r?.success).length; | ||
| const itemsFailed = safeResults.filter(isFailedProcessingResult).length; | ||
| const status = itemsFailed === 0 ? "success" : itemsSucceeded > 0 ? "partial_success" : "failure"; |
There was a problem hiding this comment.
[/codebase-design] When all results are skipped, deferred, or cancelled (0 succeeded, 0 failed), status resolves to "success" — but no item actually completed successfully. Consumers checking this output may incorrectly treat a fully-deferred batch as a clean run.
💡 Suggestion
Add a test documenting the intended behaviour for all-skipped batches. If the current "success" result is intentional, document why in a comment; if not, consider a separate "empty" status or check safeResults.length before classifying:
const status =
itemsFailed === 0 && itemsSucceeded === 0 && safeResults.length > 0
? "empty" // all items were skipped/deferred/cancelled
: itemsFailed === 0
? "success"
: itemsSucceeded > 0
? "partial_success"
: "failure";@copilot please address this.
| @@ -1592,7 +1599,8 @@ async function main() { | |||
| await writeSafeOutputSummaries(processingResult.results, allMessages); | |||
There was a problem hiding this comment.
[/tdd] items_failed in the exported output counts all active failures, including report-only ones (e.g. assign_to_agent, upload_artifact). A consumer who gates on status === "partial_success" will block on non-fatal failures, defeating the purpose of report-only categorisation.
💡 Suggestion
Decide explicitly whether items_failed should count only fatal failures or all failures, and document that contract in safe_outputs_status.cjs. If only fatal failures should count:
// in safe_outputs_status.cjs, pass a predicate or pre-filter:
function computeSafeOutputsStatus(results, { fatalOnly = false, fatalTypes } = {}) {
// ...
}Alternatively, expose two counters: items_failed (fatal) and items_reported (report-only).
@copilot please address this.
There was a problem hiding this comment.
Clean, well-structured implementation. The refactor correctly extracts isFailedProcessingResult into a shared module, the statusOutputsSet guard reliably prevents clobbering a valid status in the catch block, and all early-exit paths emit the new outputs. Tests cover success, partial-success, and failure cases. No actionable issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.6 AIC · ⌖ 7.42 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Pull request overview
Adds item-level safe-output status signals for partial-batch outcomes.
Changes:
- Computes and exports succeeded/failed counts and batch status.
- Adds status details to step summaries.
- Exposes outputs at the generated job level with tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_safe_outputs_job.go |
Exposes new job outputs. |
pkg/workflow/compiler_safe_outputs_job_test.go |
Tests job output mappings. |
actions/setup/js/safe_outputs_status.cjs |
Computes batch status and counts. |
actions/setup/js/safe_output_summary.cjs |
Adds status to summaries. |
actions/setup/js/safe_output_summary.test.cjs |
Tests summary status rendering. |
actions/setup/js/safe_output_handler_manager.cjs |
Emits and logs runtime outputs. |
actions/setup/js/safe_output_handler_manager.test.cjs |
Tests status computation and export. |
.github/skills/agentic-workflows/SKILL.md |
Removes an invalid reference. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced
| * @param {Array<{success?: boolean, deferred?: boolean, skipped?: boolean, cancelled?: boolean}>|null|undefined} results | ||
| * @returns {{itemsSucceeded: number, itemsFailed: number, status: "success" | "partial_success" | "failure"}} | ||
| */ | ||
| function computeSafeOutputsStatus(results) { | ||
| const safeResults = Array.isArray(results) ? results : []; | ||
| const itemsSucceeded = safeResults.filter(r => r?.success).length; |
| setSafeOutputsStatusOutputs(safeOutputsStatus); | ||
| statusOutputsSet = true; |
| function computeSafeOutputsStatus(results) { | ||
| const safeResults = Array.isArray(results) ? results : []; | ||
| const itemsSucceeded = safeResults.filter(r => r?.success).length; | ||
| const itemsFailed = safeResults.filter(isFailedProcessingResult).length; |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (4 tests)
Changed Files (numstat)
Inflation check: Verdict
|
|
@copilot Please address the remaining blocking review feedback on this PR, rerun the failed checks, refresh the branch if needed, then run the Failed checks: No fresh sous-chef cooldown marker was found here, so this is a follow-up nudge to investigate the failing run and finish the branch.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot Please investigate this PR and move it toward merge readiness.
|
|
🎉 This pull request is included in a new release. Release: |
safe_outputsbatches could persist many items but still appear as a flat failure when one item failed. This adds item-level status signals so consumers can distinguish partial success from total failure.Runtime outputs
items_succeeded,items_failed, andstatusfrom theprocess_safe_outputsstep.statusis one ofsuccess,partial_success, orfailure.Job-level signals
safe_outputsjob as:process_safe_outputs_items_succeededprocess_safe_outputs_items_failedprocess_safe_outputs_statusStep summary
Example output shape:
Run: https://github.com/github/gh-aw/actions/runs/30949820215