Fix failure_kind misclassification for safe_outputs post-agent failures - #50037
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
failure_kind misclassification for safe_outputs post-agent failures
|
🧠 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. |
|
✅ 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 (40 additions detected). |
There was a problem hiding this comment.
Pull request overview
Fixes safe-output failures being misclassified as driver_exit.
Changes:
- Prioritizes successful-agent/failed-safe-output metadata as
agent_logic. - Normalizes safe-output job names.
- Adds regression coverage and updates counters.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_report.go |
Updates failure classification and job-name matching. |
pkg/cli/logs_report_test.go |
Covers the corrected classification. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
| func normalizeJobName(name string) string { | ||
| normalized := strings.ToLower(strings.TrimSpace(name)) | ||
| normalized = strings.ReplaceAll(normalized, " ", "_") | ||
| return strings.ReplaceAll(normalized, "-", "_") |
| // non-zero turn count (e.g. backfilled from the usage-activity summary). | ||
| failureKind := "" | ||
| if isDriverExitFailure(run) { | ||
| if isSafeOutputsFailureAfterSuccessfulAgent(pr.JobDetails) { |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review of PR #50037 — Fix failure_kind misclassification for safe_outputs post-agent failures
The core logic and approach are correct: classifying safe_outputs failures after a successful agent job as agent_logic is the right fix, and normalizeJobName is a clean addition.
One blocking issue found:
The new check at line 252 is not guarded by isFailureConclusion(run.Conclusion). Every other branch in the classification block is gated on the overall run being a failure (either internally in isDriverExitFailure or explicitly in the else if). Without this guard, a run that concluded success at the workflow level but happens to match the job-detail pattern would incorrectly increment totalAgentLogicFailures and receive a non-empty FailureKind.
See inline comment for the one-line fix.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 31.5 AIC · ⌖ 12.2 AIC · ⊞ 5.4K
| // non-zero turn count (e.g. backfilled from the usage-activity summary). | ||
| failureKind := "" | ||
| if isDriverExitFailure(run) { | ||
| if isSafeOutputsFailureAfterSuccessfulAgent(pr.JobDetails) { |
There was a problem hiding this comment.
Missing run-level failure guard
isSafeOutputsFailureAfterSuccessfulAgent only inspects job-level conclusions and does not check that the overall workflow run itself failed. A run with Conclusion = "success" that incidentally matches the job-detail patterns will still increment totalAgentLogicFailures and get a non-empty FailureKind.
All other branches here are guarded:
isDriverExitFailurecallsisFailureConclusion(run.Conclusion)internally- The
else ifbranch usesisFailureConclusion(run.Conclusion)explicitly
Suggested fix:
if isFailureConclusion(run.Conclusion) && isSafeOutputsFailureAfterSuccessfulAgent(pr.JobDetails) {@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — requesting changes on two issues before merging.
📋 Key Themes & Highlights
Issues
normalizeJobNameis untested — the regression test only uses the already-normalised"safe_outputs"literal; the space and hyphen variants that motivated the helper are never exercised. Deleting the function would not break any test.- Cancellation edge case —
isFailureConclusionlikely includes"cancelled". A user-cancelled run whereagentfinishedsuccessbefore the cancellation propagated would be misclassified asagent_logic. This needs an explicit policy decision and, at minimum, a comment.
Positive Highlights
- ✅ Root cause properly addressed — precedence fix, not just a symptom patch
- ✅ Regression test added alongside the fix
- ✅
normalizeJobNameis a clean, reusable helper; just needs coverage
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 38.8 AIC · ⌖ 9.94 AIC · ⊞ 7.1K
Comment /matt to run again
| } | ||
| } | ||
|
|
||
| // TestBuildLogsDataNoArtifactsFailureUnclassified verifies that failed runs whose |
There was a problem hiding this comment.
[/tdd] The new test only exercises the "safe_outputs" (underscore) job name, even though normalizeJobName was added specifically to handle "safe outputs" and "safe-outputs" variants. Those variants are never tested, so normalizeJobName is effectively unverified.
💡 Suggested extra cases
Add two additional ProcessedRun entries (runs 6 and 7) and assert FailureKind=agent_logic for them:
// space-separated variant
{
Run: WorkflowRun{DatabaseID: 6, Conclusion: "failure", Turns: 0, TurnsAvailable: true},
JobDetails: []JobInfoWithDuration{
{JobInfo: JobInfo{Name: "agent", Conclusion: "success"}},
{JobInfo: JobInfo{Name: "safe outputs", Conclusion: "failure"}},
},
},
// hyphenated variant
{
Run: WorkflowRun{DatabaseID: 7, Conclusion: "failure", Turns: 0, TurnsAvailable: true},
JobDetails: []JobInfoWithDuration{
{JobInfo: JobInfo{Name: "agent", Conclusion: "success"}},
{JobInfo: JobInfo{Name: "safe-outputs", Conclusion: "failure"}},
},
},Without these, removing normalizeJobName entirely would not break any test.
@copilot please address this.
| } | ||
|
|
||
| return agentSucceeded && safeOutputsFailed | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] isSafeOutputsFailureAfterSuccessfulAgent does a linear scan but short-circuits only after visiting all jobs. More importantly, a cancelled safe_outputs job ("cancelled") would also be caught by isFailureConclusion — is that intended? If a run was cancelled by the user mid-flight the agent job can finish success before cancellation propagates, producing a false agent_logic classification.
💡 Consider
Clarify whether cancellation should map to agent_logic or remain unclassified. If not, add an explicit guard:
if normalizedName == "safe_outputs" && isFailureConclusion(job.Conclusion) &&
!strings.EqualFold(job.Conclusion, "cancelled") {
safeOutputsFailed = true
}If cancellation should be agent_logic, document that intent with a comment so future readers know it is deliberate.
@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 95/100 — Excellent
📊 Metrics (4 tests)
Key Observations
Verdict
|
There was a problem hiding this comment.
Found one blocking correctness bug in the new classification branch.
💡 Summary of findings
isSafeOutputsFailureAfterSuccessfulAgent(pr.JobDetails)is checked without also requiringisFailureConclusion(run.Conclusion), unlike the other two branches in the same if/else chain. This can misclassify a run-levelsuccessconclusion asFailureKind="agent_logic"and inflateTotalAgentLogicFailures, contradicting the run's own conclusion.- Test coverage only exercises the case where the overall run conclusion is already
failure; there's no regression test for the success-conclusion edge case that the missing guard would let through.
🔎 Code quality review by PR Code Quality Reviewer · auto · 125.4 AIC · ⌖ 5.61 AIC · ⊞ 7.9K
Comment /review to run again
| // non-zero turn count (e.g. backfilled from the usage-activity summary). | ||
| failureKind := "" | ||
| if isDriverExitFailure(run) { | ||
| if isSafeOutputsFailureAfterSuccessfulAgent(pr.JobDetails) { |
There was a problem hiding this comment.
This branch classifies a run as agent_logic based solely on job metadata, without checking isFailureConclusion(run.Conclusion) — unlike every other branch here — so a run whose overall conclusion is success (e.g. via continue-on-error on the safe_outputs job) can still get FailureKind="agent_logic" and inflate totalAgentLogicFailures.
💡 Details
Compare with the other two branches, both gated on isFailureConclusion(run.Conclusion):
} else if isDriverExitFailure(run) { // itself requires isFailureConclusion
...
} else if isFailureConclusion(run.Conclusion) && (run.TurnsAvailable || run.Turns > 0) {isSafeOutputsFailureAfterSuccessfulAgent has no equivalent guard — it only inspects pr.JobDetails. If a workflow run's overall GitHub Actions conclusion is success but a safe_outputs job step failed (possible with continue-on-error: true, or if job-level conclusion diverges from the run-level conclusion for any reason), this run will be counted and labeled as an agent-logic failure even though it's not a failed run by the run-level classification used everywhere else in this loop (totalErrors, other counters, etc. still won't be touched, causing inconsistent/contradictory summary counts).
Suggested fix:
if isFailureConclusion(run.Conclusion) && isSafeOutputsFailureAfterSuccessfulAgent(pr.JobDetails) {
failureKind = "agent_logic"
totalAgentLogicFailures++
} else if isDriverExitFailure(run) {Also add a test case where run.Conclusion == "success" but job details show agent=success/safe_outputs=failure, asserting FailureKind stays empty.
|
@copilot sous-chef triage: This PR still has unresolved blocking review feedback and needs another fix pass before maintainers can investigate quickly.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 9ceccd3.
I also ran required local validation ( |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot sous-chef triage: This PR still needs another pass before maintainers can investigate quickly.
|
Current head
Merge-readiness for this head is pending human-side review-thread resolution and CI re-trigger on a maintainer push/dispatch. |
PR Triage: #50037
Fixes
|
|
@copilot This PR still needs another fix pass before maintainers can investigate quickly. Please address the unresolved review feedback, refresh the branch if needed, then run the Unresolved reviews:
|
All listed review items are already present on current head
This pass required no additional code changes. I re-ran local validation ( |
|
@copilot This PR still needs another pass before maintainers can investigate quickly.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
logsrun summaries were over-classifying failed runs asdriver_exitwhensafe_outputsfailed after a successfulagentjob. This hid real in-scope safe-output incidents behind an infra-style failure tag.Failure-kind classification precedence
buildLogsDatato classify asagent_logicfirst when job metadata shows:agentjob concludedsuccesssafe_outputsjob concluded in a failure statedriver_exitremains a fallback for true zero-turn pre-agent failures.Job-name normalization for robust matching
safe_outputs/safe outputs/safe-outputsconsistently injob_detailsclassification.Regression coverage
agent=success+safe_outputs=failure, assertingfailure_kind=agent_logicand updated aggregate counters.run: https://github.com/github/gh-aw/actions/runs/30855215929