Fix safe-output summary outcome classification - #51484
Conversation
PR Triage
|
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ 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 (default_business_additions: 0).
|
There was a problem hiding this comment.
Pull request overview
Fixes safe-output outcome classification and diagnostics for skipped and non-successful operations.
Changes:
- Adds shared outcome classification and aggregate counts.
- Adds grouped summaries and structured diagnostics.
- Enriches comment and label policy-skip results.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/types/handler-factory.d.ts |
Expands handler result diagnostics. |
actions/setup/js/safe_outputs_status.cjs |
Adds normalized outcome classification. |
actions/setup/js/safe_output_summary.test.cjs |
Tests outcome-aware summaries. |
actions/setup/js/safe_output_summary.cjs |
Renders counts, overview, and diagnostics. |
actions/setup/js/safe_output_handler_manager.test.cjs |
Tests status outputs and preserved diagnostics. |
actions/setup/js/safe_output_handler_manager.cjs |
Preserves skipped-result metadata and exports counts. |
actions/setup/js/add_labels.cjs |
Adds structured policy-skip details. |
actions/setup/js/add_comment.cjs |
Adds structured skip reasons and targets. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
actions/setup/js/safe_output_handler_manager.cjs:933
- Diagnostic preservation currently applies only to skipped results. The ordinary
success: falsebranch immediately below still drops the handler result, so the newly declaredreasonCode,reason,target, andsafeDetailsfields never reach summaries for actual failures. Preserve the nested result there as well.
...(result.reasonCode ? { reasonCode: result.reasonCode } : {}),
...(result.errorCode ? { errorCode: result.errorCode } : {}),
error: msg,
result,
actions/setup/js/safe_output_summary.cjs:476
- Manager-generated cancellations keep diagnostics on the processing record rather than in
result.result(for example, threat aborts atsafe_output_handler_manager.cjs:782-790). Passing only the nested value here makes the detail showUNCLASSIFIEDinstead of the availableerrorCode. Merge the wrapper and object-valued handler result before rendering, while preserving array results.
result: result.result,
- Files reviewed: 7/8 changed files
- Comments generated: 4
- Review effort level: Balanced
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ponytail review: 1 finding.
net: -20 lines possible.
Generated by ✂️ Ponytail Reviewer for #51484 · auto · 32.3 AIC · ⌖ 4.25 AIC · ⊞ 6.8K
Comment /ponytail to run again
There was a problem hiding this comment.
The classification logic and outcome precedence in classifySafeOutputResult is correct. The fix properly handles success:true, skipped:true results (TARGET_NOT_FOUND, TARGET_LOCKED) as skipped rather than success. Well-tested with appropriate coverage.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 37.7 AIC · ⌖ 6.77 AIC · ⊞ 5.5K
🧪 Test Quality Sentinel ReportPR #51484 — "Fix safe-output summary outcome classification" Score: 100/100 ✅ Excellent
Coverage Highlights✅ New Behavioral Tests (5 Total)Click to expand test classifications
📊 Line Count Analysis
Test Quality AnalysisStrengths ✅
Test MaintenanceExisting tests were updated to verify new output fields:
This represents proper test maintenance for expanded APIs. Verdict✅ APPROVE — This PR demonstrates excellent test quality:
No issues flagged.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on three correctness issues and one test-assertion gap.
📋 Key Themes & Highlights
Key Themes
- Delegated-skip heuristic is fragile (
safe_outputs_status.cjsline 32): the three-condition implicit test will misclassify any new handler that returns{ skipped: true, reason: '...' }without a nestedresult. An explicitdelegated: trueflag at the source would be safer. skippedHandlerResultslog bucket is broken for new code paths (safe_output_handler_manager.cjsline 1645): the existing!r.reasonfilter excludes all newly-enriched policy-skipped results, so theSkipped (no context or limit reached)log count will be wrong.safeDetailsrendering is coupled to handler-specific field names (safe_output_summary.cjsline 241): hardcodedrequiredLabels/missingLabelskeys mean new handlers with different shapes silently produce no diagnostics.- warning/skipped precedence (
safe_outputs_status.cjsline 35):success: true, skipped: true, warningoutcomes count as skips, not warnings — intent should be documented or the precedence adjusted.
Positive Highlights
- ✅
classifySafeOutputResultis a clean centralization of outcome logic — shared by status computation, summary rendering, and aggregation. - ✅ Structured
reasonCode/reason/target/safeDetailsfields make diagnostics auditable without leaking raw exception text. - ✅ New tests cover the key regression cases (policy-skip rendered as Skipped, grouped overview counts, diagnostic preservation).
- ✅ The
generateOutcomeOverviewgrouped table is a nice UX improvement for bulk runs.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 86.2 AIC · ⌖ 6.95 AIC · ⊞ 7.1K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/safe_output_handler_manager.cjs:1645
[/diagnosing-bugs] skippedHandlerResults filter uses !r.reason, but handler-skipped results now propagate reason into the processing record — so policy-skipped results (e.g. REQUIRED_LABELS_MISMATCH) will be excluded from this log bucket and the Skipped (no context or limit reached) count will be underreported.
<details>
<summary>💡 Suggested fix</summary>
Replace the !r.reason exclusion with explicit delegation-reason checks:
const skippedHandlerResults = processingRes…
</details>
<details><summary>actions/setup/js/safe_outputs_status.cjs:32</summary>
**[/diagnosing-bugs]** The `"delegated"` heuristic (`result?.skipped && result?.reason && result?.result === undefined`) is fragile: any new handler that returns `skipped: true` plus a `reason` string but omits a nested `result` object will be silently classified as delegated and excluded from all counts.
<details>
<summary>💡 Suggested hardening</summary>
Delegated skips are synthetic records created by a specific code path in the handler manager. Consider tagging them explicitly at the sour…
</details>
<details><summary>actions/setup/js/safe_output_summary.cjs:241</summary>
**[/codebase-design]** `safeDetails` is decoded in `formatOutcomeDiagnostics` with hardcoded field access (`safeDetails?.requiredLabels`, `safeDetails?.missingLabels`). This works for the two current handlers but couples the rendering logic to handler-specific field names; a new handler with different `safeDetails` shape (e.g. `safeDetails.allowedBranches`) will silently produce no diagnostics.
<details>
<summary>💡 Suggested approach</summary>
Either:
1. Document the `safeDetails` contract …
</details>
<details><summary>actions/setup/js/safe_output_summary.test.cjs:626</summary>
**[/tdd]** The `success: true, skipped: true` test verifies the rendered summary contains `⚠️ Add Comment - Skipped` but asserts `not.toContain('Success')` — this would also pass if the label were 'Failed' or blank. A more precise assertion would check that the exact status text is `Skipped`:
```js
expect(summary).toContain('⚠️ Add Comment - Skipped (Message 2)');
expect(summary).not.toContain('- Success');
expect(summary).not.toContain('- Failed');@copilot please address this.
actions/setup/js/safe_outputs_status.cjs:35
[/diagnosing-bugs] "skipped" is checked before "warning" in classifySafeOutputResult. A result that has both skipped: true and warning: "..." (e.g. TARGET_LOCKED in add_comment.cjs) will classify as "skipped", but the OUTCOME_DISPLAY entry for "skipped" uses the same emoji (⚠️) as "warning". This is consistent visually but means the aggregate itemsWarnings count will never include lock-skips, which may mislead callers expecting warnings to appear in the warnings bu…
|
Thanks for tackling the safe-output summary outcome classification fix. This is a well-scoped change that normalizes outcome semantics, adds outcome-aware summaries and structured diagnostics, and comes with solid new test coverage for mixed result scenarios. Ready for review.
|
Safe-output run summaries could render expected skips as failures, count skipped no-ops as successes, and omit non-success outcome counts. This made policy skips hard to diagnose and caused aggregate counts to disagree with per-item details.
Shared outcome semantics
Outcome-aware summaries
Skipped, notFailed.Safe diagnostics
reasonCode,reason,target, andsafeDetails.add_commentandadd_labels.