feat: add --evals flag to logs and audit commands to filter by evals results#45373
Conversation
… with evals results Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…istency Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds --evals filtering to logs and audit commands and introduces an evals artifact set.
Changes:
- Adds eval artifact discovery and run filtering.
- Automatically requests eval artifacts from CLI commands.
- Threads eval filtering through download and audit options.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_run_processor.go |
Detects eval result files. |
pkg/cli/logs_orchestrator.go |
Enables eval filtering for downloaded runs. |
pkg/cli/logs_orchestrator_types.go |
Adds eval filter options. |
pkg/cli/logs_orchestrator_stdin.go |
Enables filtering for stdin runs. |
pkg/cli/logs_orchestrator_filters.go |
Skips runs without eval results. |
pkg/cli/logs_command.go |
Registers --evals and requests its artifact. |
pkg/cli/logs_artifact_set.go |
Defines the evals artifact set. |
pkg/cli/logs_artifact_set_test.go |
Updates valid artifact-set expectations. |
pkg/cli/audit.go |
Adds audit filtering and artifact selection. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 6
- Review effort level: Medium
| entries, err := os.ReadDir(runDir) | ||
| if err != nil { | ||
| return false | ||
| } |
| if opts.evalsOnly && !slices.Contains(opts.artifacts, string(ArtifactSetEvals)) && !slices.Contains(opts.artifacts, string(ArtifactSetAll)) { | ||
| opts.artifacts = append(opts.artifacts, string(ArtifactSetEvals)) | ||
| } |
|
|
||
| // Apply evals filtering if --evals flag is specified. | ||
| if opts.evalsOnly { | ||
| if !runHasEvals(result.LogsPath, verbose) { |
There was a problem hiding this comment.
Fixed in 04309d3 — downloadRunArtifactsConcurrent now bypasses the cached summary and falls through to a fresh download when the evals artifact is in the filter but not present locally, so the filter in applyRunFilters sees the just-downloaded artifact.
| if cfg.evalsOnly && !runHasEvals(cfg.outputDir, cfg.verbose) { | ||
| auditLog.Printf("Skipping run %d: no evals results found (filtered by --evals)", cfg.runID) | ||
| fmt.Fprintf(os.Stderr, "%s\n", console.FormatInfoMessage(fmt.Sprintf("Skipping run %d: workflow does not have evals results (filtered by --evals)", cfg.runID))) | ||
| return true, nil |
There was a problem hiding this comment.
Fixed in 04309d3 — renderCachedAuditIfAvailable returns (false, nil) (bypass cache) when --evals is set but evals are absent locally, so prepareAuditWorkflowRun fetches the artifact before shouldSkipForEvals applies the filter.
| opts.stdin, _ = cmd.Flags().GetBool("stdin") | ||
| opts.experimentFilter, _ = cmd.Flags().GetString("experiment") | ||
| opts.variantFilter, _ = cmd.Flags().GetString("variant") | ||
| opts.evalsOnly, _ = cmd.Flags().GetBool("evals") |
There was a problem hiding this comment.
Fixed in 04309d3 — runAuditCommand now returns a clear error message when --evals is combined with multiple run IDs, directing users to single-run mode.
| cmd.Flags().Bool("stdin", false, "Read workflow run IDs or URLs from stdin (one per line) instead of positional arguments") | ||
| cmd.Flags().String("experiment", "", "Filter to runs that include this experiment name") | ||
| cmd.Flags().String("variant", "", "Filter to runs with a specific variant value (requires --experiment)") | ||
| cmd.Flags().Bool("evals", false, "Include evals results in audit report; automatically downloads the evals artifact") |
There was a problem hiding this comment.
Fixed in 04309d3 — help text updated to "Skip runs that do not contain evals results (evals.jsonl); automatically downloads the evals artifact when --artifacts is narrowed", accurately describing the filtering behavior.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45373 does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (99 additions, threshold is 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat: add --evals flag to logs and audit commands
The overall approach is sound — the flag is cleanly threaded through both orchestrators, auto-artifact-inclusion is handled consistently, and the runHasEvals helper correctly handles both direct and workflow_call-prefixed artifact directories.
Two blocking issues:
-
Misleading flag description in
audit.go— The flag help text says "Include evals results" (implies display control) but the behaviour is "filter to runs that have evals" (a skip filter). Inconsistent with the logs variant wording. -
Unconditional stderr output in audit paths —
AuditWorkflowRunandrenderCachedAuditIfAvailableprint the skip message unconditionally, whileapplyRunFiltersin the logs path guards it withverbose. Non-verbose audit runs will receive noisy informational messages that other filters don't emit.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 26.6 AIC · ⌖ 4.45 AIC · ⊞ 4.8K
| cmd.Flags().Bool("stdin", false, "Read workflow run IDs or URLs from stdin (one per line) instead of positional arguments") | ||
| cmd.Flags().String("experiment", "", "Filter to runs that include this experiment name") | ||
| cmd.Flags().String("variant", "", "Filter to runs with a specific variant value (requires --experiment)") | ||
| cmd.Flags().Bool("evals", false, "Include evals results in audit report; automatically downloads the evals artifact") |
There was a problem hiding this comment.
The --evals flag description says "Include evals results in audit report" but the actual behaviour is to filter out runs that lack evals — it is a filter flag, not a display toggle.
Suggested wording (consistent with the logs variant):
"Filter to runs containing evals results; automatically downloads the evals artifact"
@copilot please address this.
There was a problem hiding this comment.
Fixed in 53a7bb7 — the --evals flag description in audit now reads "Skip runs that do not contain evals results (evals.jsonl); automatically downloads the evals artifact when --artifacts is narrowed", matching the filtering behavior.
| } | ||
| if cfg.evalsOnly && !runHasEvals(cfg.outputDir, cfg.verbose) { | ||
| auditLog.Printf("Skipping run %d: no evals results found (filtered by --evals)", cfg.runID) | ||
| fmt.Fprintf(os.Stderr, "%s\n", console.FormatInfoMessage(fmt.Sprintf("Skipping run %d: workflow does not have evals results (filtered by --evals)", cfg.runID))) |
There was a problem hiding this comment.
In AuditWorkflowRun, the skip message is unconditionally printed to stderr regardless of verbosity, while the parallel path in applyRunFilters (logs) only prints when verbose is true. This is inconsistent — noisy output appears even in non-verbose audit runs.
Consider guarding it with cfg.verbose:
if cfg.verbose {
fmt.Fprintf(os.Stderr, "%s\n", console.FormatInfoMessage(...))
}The same issue exists in renderCachedAuditIfAvailable (line ~493).
@copilot please address this.
There was a problem hiding this comment.
Fixed in 53a7bb7 — extracted shouldSkipForEvals(cfg auditRunConfig) bool which wraps the auditLog.Printf + fmt.Fprintf(os.Stderr, ...) pair. The stderr message is now guarded by cfg.verbose, consistent with the logs path.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (1 test)
| Test | File | Classification | Issues | Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /grill-with-docs — three targeted issues found; none blocking but worth addressing before this pattern spreads.
📋 Key Themes & Highlights
Key Issues
- No tests for
runHasEvals— the core helper driving both commands is untested. OtherrunHas*helpers in the same file have tests; this one should too. - Duplicated auto-include logic — the evals artifact guard appears in three places. A small helper removes drift risk.
- Duplicate skip blocks in
audit.go— identical log + stderr output in two functions; easy to extract into a shared helper. - Flag description mismatch —
audit --evalssays "Include evals results" but its behaviour is filtering (skipping runs without evals), consistent withlogs --evals.
Positive Highlights
- Clean, non-breaking feature: unchanged behaviour without the flag.
- Consistent threading of
EvalsOnlythrough all options types and orchestrators. - Hash-prefix support in
runHasEvalscorrectly handlesworkflow_call-prefixed artifacts. - Test updated for
ValidArtifactSetNamesto cover the new constant.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 40.7 AIC · ⌖ 4.79 AIC · ⊞ 6.6K
Comment /matt to run again
| logsOrchestratorLog.Printf("No evals results found in: %s", runDir) | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
[/tdd] runHasEvals is the core filtering function for this feature but has no unit tests — a future refactor could silently break evals filtering with no safety net.
💡 Suggested test cases
A table-driven test in logs_run_processor_test.go covering:
- directory contains
evals/evals.jsonl→ returnstrue - directory contains
abc123-evals/evals.jsonl(hash-prefixed) → returnstrue - directory has
evals/folder but noevals.jsonlinside → returnsfalse - empty directory → returns
false - non-existent directory → returns
false
These edge cases directly reflect the matching logic in the function and match the pattern used by other runHas* helpers in the same file.
@copilot please address this.
There was a problem hiding this comment.
Added in 53a7bb7 — pkg/cli/logs_run_processor_test.go now contains TestRunHasEvals with 6 table-driven cases: root-level evals.jsonl, evals/evals.jsonl, abc123-evals/evals.jsonl (hash-prefixed), evals/ folder without evals.jsonl, empty directory, and non-existent directory. All pass.
| @@ -170,6 +173,10 @@ Downloaded artifacts include (when using --artifacts all): | |||
| return err | |||
There was a problem hiding this comment.
[/codebase-design] The --evals auto-include logic (lines 173-175 and 333-335 in the stdin path) is duplicated verbatim. This pattern already exists once for the audit command too, so it will need to be kept in sync in three places.
💡 Suggestion
Extract a small helper or move the auto-include into getAuditCommandOptions-style resolution that happens once, for example:
func applyEvalsArtifact(artifacts []string, evalsOnly bool) []string {
if evalsOnly && !slices.Contains(artifacts, string(ArtifactSetEvals)) && !slices.Contains(artifacts, string(ArtifactSetAll)) {
return append(artifacts, string(ArtifactSetEvals))
}
return artifacts
}Then call artifacts = applyEvalsArtifact(artifacts, evalsOnly) in both command handlers and in getAuditCommandOptions.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 53a7bb7 — added applyEvalsArtifact(artifacts []string, evalsOnly bool) []string to logs_artifact_set.go. Both duplicate blocks in logs_command.go now call artifacts = applyEvalsArtifact(artifacts, evalsOnly), and getAuditCommandOptions in audit.go simplifies to opts.artifacts = applyEvalsArtifact(opts.artifacts, opts.evalsOnly) (guarded by the existing len > 0 check).
| if cfg.evalsOnly && !runHasEvals(cfg.outputDir, cfg.verbose) { | ||
| auditLog.Printf("Skipping run %d: no evals results found (filtered by --evals)", cfg.runID) | ||
| fmt.Fprintf(os.Stderr, "%s\n", console.FormatInfoMessage(fmt.Sprintf("Skipping run %d: workflow does not have evals results (filtered by --evals)", cfg.runID))) | ||
| return nil |
There was a problem hiding this comment.
[/codebase-design] The evals-skip block (lines 373-377) is copy-pasted verbatim into renderCachedAuditIfAvailable (lines 493-497), including the duplicate auditLog.Printf + fmt.Fprintf(os.Stderr, ...) pair. Divergence is likely as the feature evolves.
💡 Suggestion
Extract a small helper used by both:
// shouldSkipForEvals returns true and logs when no evals results are present.
func shouldSkipForEvals(cfg auditRunConfig) bool {
if !cfg.evalsOnly {
return false
}
if runHasEvals(cfg.outputDir, cfg.verbose) {
return false
}
auditLog.Printf("Skipping run %d: no evals results found (filtered by --evals)", cfg.runID)
fmt.Fprintf(os.Stderr, "%s\n", console.FormatInfoMessage(
fmt.Sprintf("Skipping run %d: workflow does not have evals results (filtered by --evals)", cfg.runID)))
return true
}@copilot please address this.
There was a problem hiding this comment.
Fixed in 53a7bb7 — extracted shouldSkipForEvals(cfg auditRunConfig) bool at the bottom of audit.go. Both AuditWorkflowRun and renderCachedAuditIfAvailable (which has a distinct "bypass cache" path) use this single helper. The helper includes the verbose guard for the stderr message.
| cmd.Flags().String("experiment", "", "Filter to runs that include this experiment name") | ||
| cmd.Flags().String("variant", "", "Filter to runs with a specific variant value (requires --experiment)") | ||
| cmd.Flags().Bool("evals", false, "Include evals results in audit report; automatically downloads the evals artifact") | ||
| RegisterDirFlagCompletion(cmd, "output") |
There was a problem hiding this comment.
[/grill-with-docs] The flag description says "Include evals results in audit report" but its primary effect is filtering (skipping runs without evals). This is more consistent with the logs --evals description ("Filter to runs containing evals results"). A misleading description can confuse users expecting the flag to add something to the report rather than exclude runs.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 53a7bb7 — the audit --evals flag description now accurately says "Skip runs that do not contain evals results (evals.jsonl); automatically downloads the evals artifact when --artifacts is narrowed".
There was a problem hiding this comment.
REQUEST_CHANGES — Several correctness issues need fixing before merge
The --evals feature has a cluster of bugs that will cause incorrect behavior at runtime.
### Blocking issues (raised by prior reviewer)
flattenSingleFileArtifactsmovesevals.jsonlbeforerunHasEvalschecks — the file will be relocated out of theevals/subdir, causing the filter to always return false.--evalsnot forwarded in multi-run / batch audit —EvalsOnlyis only threaded throughrunAuditSingle; other callers silently ignore it.- Cached-download false negatives in logs path — filter checks local filesystem before artifact is actually downloaded.
- Cached-summary false negatives in audit path —
renderCachedAuditIfAvailablechecks forevals.jsonllocally but the artifact may not have been downloaded when the summary was cached. - Misleading flag description — says "Include evals results" but actual behavior is "filter to runs that have evals results".
### Additional issues (new comments added)
- Unconditional stderr noise on audit path — skip message always printed to stderr in audit but gated on
verbosein logs path; breaks consistency. - Overly broad suffix match in
runHasEvals— any directory ending in-evalspasses, not just workflow_call hash-prefixed ones.
🔎 Code quality review by PR Code Quality Reviewer · 58.2 AIC · ⌖ 4.61 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/audit.go:370
Noisy stderr on every skipped run: the skip message is unconditionally written to os.Stderr on both audit paths, unlike the logs path which gates it on verbose.
<details>
<summary>💡 Details</summary>
In AuditWorkflowRun (and renderCachedAuditIfAvailable) the info message is always printed:
fmt.Fprintf(os.Stderr, "%s\n", console.FormatInfoMessage(...))But applyRunFilters in logs_orchestrator_filters.go only prints when verbose == true. When auditing many runs with…
pkg/cli/logs_run_processor.go:341
Overly broad suffix match allows any *-evals directory to satisfy the filter: the pattern strings.HasSuffix(name, "-"+constants.EvalsArtifactName) matches any directory name ending in "-evals", not just valid workflow_call hash-prefixed artifacts.
<details>
<summary>💡 Details</summary>
A directory named my-custom-evals or test-evals that happens to contain evals.jsonl would satisfy this check even if it is not a legitimate evals artifact. The comment says this matches `{hash}-e…
|
@copilot run pr-finisher skill |
- runHasEvals: also check root-level evals.jsonl (after flattenSingleFileArtifacts) - audit --evals artifact auto-include: only append evals when user narrowed artifacts - logs cache bypass: skip cache when evals requested but not present locally - audit cache bypass: bypass cache when --evals set but evals missing locally - reject --evals in multi-run diff mode with a clear error message - fix --evals help text to describe filtering behavior (not report content) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All six review issues have been addressed in commit
|
|
Run: https://github.com/github/gh-aw/actions/runs/29319775702
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all remaining review threads in commits
Local validation: |
Adds
--evalstogh aw logsandgh aw auditto filter workflow runs that contain BinEval evaluation results (evals.jsonl). When set, the evals artifact is automatically included in the download—no need to pass--artifacts evalsseparately.logs_artifact_set.go
ArtifactSetEvals = "evals"constant mapping toconstants.EvalsArtifactNamelogs_run_processor.go
runHasEvals(runDir, verbose)helper: walks run output looking forevals/evals.jsonlor{hash}-evals/evals.jsonllogs_orchestrator_filters.go / _types.go / _orchestrator.go / _stdin.go
evalsOnly booladded torunFilterOpts; runs without evals results are skipped with a verbose-mode info messageEvalsOnly boolthreaded throughLogsDownloadOptionsandStdinLogsOptionsinto both orchestratorslogs_command.go
--evalsflag registered; auto-appendsevalsto the artifact set; example added to usage textaudit.go
EvalsOnly booladded toAuditOptions,auditCommandOptions,auditRunConfig--evalsflag registered; evals artifact auto-included viaslices.Contains--evalsis set but noevals.jsonlis found