trial: fail and report rejected safe-output errors instead of always reporting success - #50723
Conversation
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
Detects rejected safe-output messages during gh aw trial and reports failed trials explicitly.
Changes:
- Extracts safe-output errors and records success status.
- Aggregates failures and returns a nonzero error.
- Adds unit tests for error extraction and status fields.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/trial_types.go |
Adds status fields and error extraction. |
pkg/cli/trial_helpers.go |
Reports and aggregates rejected messages. |
pkg/cli/trial_safe_output_errors_test.go |
Tests extraction and result fields. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Balanced
| Success: len(safeOutputErrors) == 0, | ||
| SafeOutputErrors: safeOutputErrors, |
There was a problem hiding this comment.
Fixed in latest commit: when --json is set, executeTrialRun now marshals the full WorkflowTrialResult (and CombinedTrialResult for multi-workflow runs) to stdout instead of the raw safe-outputs artifact, so success/safe_output_errors are exposed.
| if len(safeOutputErrors) > 0 { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("=== %d Safe Output Message(s) Rejected from %s ===", len(safeOutputErrors), parsedSpec.WorkflowName))) | ||
| for _, msg := range safeOutputErrors { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(msg)) |
There was a problem hiding this comment.
Added sanitizeControlChars which escapes control characters (e.g. ANSI escapes, newlines) before rejected messages are written to stderr.
| Success: false, | ||
| } | ||
| if result.Success { | ||
| t.Error("expected Success to be false when SafeOutputErrors is non-empty") |
There was a problem hiding this comment.
Factored the aggregation logic into aggregateTrialResults(results []WorkflowTrialResult) (bool, int, string), now used by executeTrialRun, and added TestAggregateTrialResults covering success/failure counts and first-error ordering across multiple workflows.
|
|
||
| if !overallSuccess { | ||
| fmt.Fprintln(os.Stderr, console.FormatErrorMessage(fmt.Sprintf("Trial completed with %d rejected safe-output message(s)", totalRejected))) | ||
| return fmt.Errorf("trial completed with %d rejected safe-output message(s); first error: %s", totalRejected, firstErrorMessage) |
There was a problem hiding this comment.
firstErrorMessage is now passed through sanitizeControlChars before being embedded in the returned error.
|
@copilot run pr-finisher skill |
…d aggregation tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
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. Great work on this fix, Copilot! 🎯 This PR directly addresses the critical issue where The implementation is solid:
The diff is focused, well-scoped to the single issue, and the PR body clearly explains detection, reporting, and exit behavior changes. Tests appear to be included. This looks ready for review and merge!
|
|
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. @copilot please resolve the remaining review threads, refresh this branch if needed, then run the
Run: https://github.com/github/gh-aw/actions/runs/31062758914
|
All four flagged review threads are already addressed in the current HEAD (0fb05e0): |
|
🎉 This pull request is included in a new release. Release: |
gh aw trialunconditionally printed "All trials completed successfully" and exited 0 even when a workflow's safe-outputs artifact contained a non-emptyerrorsarray from rejected messages, silently masking partial failures from CI gates and automation.Detection
extractSafeOutputErrorshelper to parse theerrorsarray out of a workflow's safe-outputs artifact.Result reporting
SuccessandSafeOutputErrorsfields toWorkflowTrialResult, andSuccesstoCombinedTrialResult, so the--jsonoutput exposes an explicit pass/fail signal instead of requiring consumers to inspect nestederrorsarrays.Exit behavior
executeTrialRunnow aggregates results across all workflows. If any workflow has safe-output errors, it returns a nonzero error reporting the total rejected count and the first error message, instead of the unconditional success message.{ "workflow_name": "example", "success": false, "safe_output_errors": [ "Line 1: set_issue_field requires at least one of: 'field_name', 'field_node_id' fields" ] }Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Run: https://github.com/github/gh-aw/actions/runs/31062758914> Generated by 👨🍳 PR Sous Chef · gpt54 · 20.1 AIC · ⊞ 8.3K · ◷