feat: read evals from usage artifact instead of dedicated evals artifact#45797
Conversation
evals.jsonl is now included in the usage artifact (copied there by the conclusion job). Update the client-side code accordingly: - ArtifactSetEvals now resolves to the usage artifact name so that --artifacts evals downloads usage (which contains evals.jsonl) - applyEvalsArtifact no longer appends evals when usage is already in the artifacts list, since usage already provides evals - Thread evalsOnly bool through concurrentRunDownloadParams so the cache bypass in tryLoadCachedRunResult uses the explicit flag rather than inspecting the artifact filter for a name that no longer matches - Update --evals flag descriptions in audit and logs commands to reflect that evals are read from the usage artifact - Add TestResolveArtifactFilter case asserting evals→usage resolution Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Score breakdown: Impact 28/50 · Urgency 20/30 · Quality 17/20 Rationale: Fixes evals artifact resolution regression —
|
|
@copilot download evals if not found in usage artifact |
…tifact Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Added The fallback is triggered by a new |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45797 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. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Updates eval retrieval to use the usage artifact and adds legacy fallback handling.
Changes:
- Maps the evals artifact set to usage.
- Propagates eval-request context through download orchestration.
- Updates CLI descriptions and tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_run_processor.go |
Adds eval request tracking and legacy fallback. |
pkg/cli/logs_parallel_test.go |
Updates download helper calls. |
pkg/cli/logs_orchestrator.go |
Passes eval context to downloads. |
pkg/cli/logs_orchestrator_test.go |
Updates orchestrator test calls. |
pkg/cli/logs_orchestrator_stdin.go |
Propagates stdin eval options. |
pkg/cli/logs_command.go |
Updates the eval flag description. |
pkg/cli/logs_artifact_set.go |
Maps evals to usage and deduplicates selection. |
pkg/cli/logs_artifact_set_test.go |
Tests evals-to-usage resolution. |
pkg/cli/audit.go |
Updates audit messaging and comments. |
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: 2
- Review effort level: Medium
| ensureEvalsResultsFromBranch(ctx, summary.Run, runOutputDir, params.dlOwner, params.dlRepo, params.dlHost, params.verbose) | ||
| if evalsRequested && !hasEvals { | ||
| logsOrchestratorLog.Printf("Cache bypass for run %d: evals artifact requested but not present locally", run.DatabaseID) | ||
| if params.evalsOnly && !hasEvals { |
| // evals: evals results are now included in the usage artifact. | ||
| ArtifactSetEvals: {constants.UsageArtifactName}, |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat: read evals from usage artifact instead of dedicated evals artifact
The overall approach is correct — remapping ArtifactSetEvals to resolve to the usage artifact and introducing a fallback for older runs is well-designed. One blocking bug was found.
Blocking issue: In tryLoadCachedRunResult (logs_run_processor.go:271), the cache bypass condition was changed from the old evalsRequested (which fired for both --evals and --artifacts evals) to params.evalsOnly (only fires for --evals). The PR already introduces evalsArtifactRequested that covers both cases and uses it correctly in processSingleRunDownload. The same field should be used in tryLoadCachedRunResult for consistency and correctness.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 40.3 AIC · ⌖ 6.87 AIC · ⊞ 5K
| ensureEvalsResultsFromBranch(ctx, summary.Run, runOutputDir, params.dlOwner, params.dlRepo, params.dlHost, params.verbose) | ||
| if evalsRequested && !hasEvals { | ||
| logsOrchestratorLog.Printf("Cache bypass for run %d: evals artifact requested but not present locally", run.DatabaseID) | ||
| if params.evalsOnly && !hasEvals { |
There was a problem hiding this comment.
Bug: cache bypass uses evalsOnly instead of evalsArtifactRequested
The old code used evalsRequested := slices.Contains(params.artifactFilter, constants.EvalsArtifactName) which was true for both --evals and --artifacts evals. The new code replaces this with params.evalsOnly, which is only true when --evals is passed.
As a result, when a user passes --artifacts evals (without --evals), the cache bypass no longer fires, and a stale cache without evals.jsonl will be returned instead of triggering a fresh download.
The PR already introduces evalsArtifactRequested (which covers both cases) and uses it correctly in processSingleRunDownload line 218. The same field should be used here:
// before
if params.evalsOnly && !hasEvals {
// after
if params.evalsArtifactRequested && !hasEvals {@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (14 tests)
i️ Test Inflation Analysislogs_orchestrator_test.go: 14 lines added / 1 line in logs_orchestrator.go = 14:1 ratio (exceeds 2:1 threshold) Justification: All 13 new integration tests cover concurrent download behavior with careful attention to:
This is justified complexity: concurrency bugs are silent and hard to reproduce, requiring comprehensive testing of race conditions. Verdict
Strengths:
Notes:
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 /codebase-design and /tdd — requesting changes on one correctness bug and two test-coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Flag asymmetry (
/codebase-design):tryLoadCachedRunResultbypasses the cache only whenparams.evalsOnlyis true, butprocessSingleRunDownloadtriggers the fallback wheneverparams.evalsArtifactRequestedis true. These two flags differ when--artifacts evalsis passed without--evals, creating a silent correctness hole for cached runs. - Untested fallback (
/tdd):tryDownloadEvalsArtifactFallbackhas no unit test. It is a correctness-critical path whose "fail silently" contract should be pinned. - Missing deduplication test (
/tdd):evalsandusagenow both resolve toconstants.UsageArtifactName;["evals", "usage"]should be tested to ensure it does not trigger a double download.
Positive Highlights
- ✅ Clean redirection of
ArtifactSetEvals → UsageArtifactNamein the map — minimal blast radius - ✅ Backward-compatibility fallback for older runs is well-documented and error-swallowing intentionally
- ✅ All existing
downloadRunArtifactsConcurrentcall sites updated consistently - ✅ The new
applyEvalsArtifactguard forArtifactSetUsagecorrectly prevents double-appending
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 51.2 AIC · ⌖ 4.74 AIC · ⊞ 6.7K
Comment /matt to run again
| ensureEvalsResultsFromBranch(ctx, summary.Run, runOutputDir, params.dlOwner, params.dlRepo, params.dlHost, params.verbose) | ||
| if evalsRequested && !hasEvals { | ||
| logsOrchestratorLog.Printf("Cache bypass for run %d: evals artifact requested but not present locally", run.DatabaseID) | ||
| if params.evalsOnly && !hasEvals { |
There was a problem hiding this comment.
[/codebase-design] Cache-bypass uses params.evalsOnly (line 271) but the fallback download uses params.evalsArtifactRequested (line 218) — the two conditions are asymmetric. A user who passes --artifacts evals without --evals will trigger the fallback on a fresh download but not get a cache bypass, so a stale cached run that lacks evals will be served without attempting the dedicated-artifact fallback.
💡 Suggested fix
Change the cache-bypass guard to match the fallback condition:
// was: if params.evalsOnly && !hasEvals {
if params.evalsArtifactRequested && !hasEvals {
logsOrchestratorLog.Printf("Cache bypass for run %d: evals requested but not present locally", run.DatabaseID)
return nil, false
}Keeping these two branches in sync ensures a cached run without evals is never served when --artifacts evals was passed.
@copilot please address this.
| // evals: BinEval evaluation results uploaded by the evals job. | ||
| ArtifactSetEvals: {constants.EvalsArtifactName}, | ||
| // evals: evals results are now included in the usage artifact. | ||
| ArtifactSetEvals: {constants.UsageArtifactName}, |
There was a problem hiding this comment.
[/tdd] The evals and usage sets now resolve to the same artifact (constants.UsageArtifactName). There is no test asserting that ResolveArtifactFilter(["usage", "evals"]) deduplicates to ["usage"] (a single download) rather than downloading the usage artifact twice. The deduplication likely happens further downstream, but it should be explicitly covered.
💡 Suggested test
Add to TestResolveArtifactFilter:
{
name: "evals + usage deduplicates to single usage artifact download",
sets: []string{"evals", "usage"},
expected: []string{"usage"},
},This guards against double-downloading if callers pass both sets.
@copilot please address this.
| // that predate the conclusion-job copy), fall back to the dedicated evals | ||
| // artifact so those runs are not silently skipped. This applies both when | ||
| // --evals is set and when --artifacts evals was explicitly listed. | ||
| if params.evalsArtifactRequested && !runHasEvals(runOutputDir, params.verbose) { |
There was a problem hiding this comment.
[/tdd] tryDownloadEvalsArtifactFallback is called but never unit-tested. The fallback is a correctness-critical code path for older runs — if it silently fails, those runs are served without evals. A test that stubs downloadRunArtifacts to return an error (or succeed) would pin this behaviour.
💡 Suggested approach
Add a test in logs_run_processor_test.go (or a new file) that:
- Creates a run output dir without
evals.jsonl - Calls
tryDownloadEvalsArtifactFallbackwith a mock/stub - Asserts the log output and that errors are swallowed (not propagated)
This ensures the "fail silently, continue" contract is explicit and protected.
@copilot please address this.
There was a problem hiding this comment.
Two correctness issues warrant fixes before merge
1. Cache bypass uses a narrower predicate than the download fallback (logs_run_processor.go:271): tryLoadCachedRunResult bypasses the cache only when evalsOnly is true, but processSingleRunDownload triggers the evals fallback on the broader evalsArtifactRequested. These predicates must match or a cached run with missing evals is returned from cache without triggering the fallback when a user passes --artifacts evals without --evals.
2. applyEvalsArtifact called without the len > 0 guard in logs_command.go (lines 175 and 330): the function's own doc comment mandates this guard; audit.go applies it correctly; neither logs_command.go call site does. Passing --artifacts "" (empty slice) with --evals silently reduces the download to usage-only, dropping agent/activation/firewall artifacts.
Further details
Both issues are in the changed diff. The remainder of the change (artifact remapping, evalsOnly propagation, fallback download logic) is sound.
🔎 Code quality review by PR Code Quality Reviewer · 96.3 AIC · ⌖ 4.62 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/logs_run_processor.go:271
Cache bypass predicate is inconsistent with the download fallback condition: the cache bypass checks only params.evalsOnly but the fallback download (line 218) triggers on the broader params.evalsArtifactRequested. A user who passes --artifacts evals without --evals gets the fallback download after a fresh download, but if the run is already cached they get a stale cache hit with no evals — silently missing evals data.
<details>
<summary>💡 Suggested fix</summary>
Use the same pre…
pkg/cli/logs_command.go:175
Missing len(artifacts) > 0 guard mirrors the hazard documented in applyEvalsArtifact's own doc comment: the function's comment explicitly warns callers that treat empty as "all" to guard with len(artifacts) > 0. audit.go does this correctly. Both call sites in logs_command.go (lines 175 and 330) do not. If a user passes --artifacts "" (empty string, which StringSlice parses as an empty slice), applyEvalsArtifact will append "evals" and the effective filter becomes `["usage…
|
Run: https://github.com/github/gh-aw/actions/runs/29447716606
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in
Validation rerun:
|
|
@copilot please run the
|
Ran
This is ready for maintainer review from the agent side. Hand-off: please re-trigger CI from a maintainer context (close/reopen PR, |
|
🎉 This pull request is included in a new release. Release: |
evals.jsonlis now included in the usage artifact (conclusion job copies it from the evals job). The client-side--evalsflag was still requesting a dedicatedevalsartifact download and the cache bypass was keyed on the artifact name, which broke silently after the mapping change.Changes
ArtifactSetEvals→ resolves tousage(logs_artifact_set.go):--artifacts evalsnow downloads the usage artifact.runHasEvalsalready probesusage/evals.jsonl; this aligns the download side with detection.applyEvalsArtifactskips whenusagealready present (logs_artifact_set.go): Thelogsdefault is--artifacts usage; previously--evalsappended a redundantevalsentry. Now it short-circuits whenusageis already in the list.Thread
evalsOnlythrough the download pipeline (logs_run_processor.go,logs_orchestrator.go,logs_orchestrator_stdin.go): The cache bypass intryLoadCachedRunResultwas checkingslices.Contains(artifactFilter, "evals")— always false after the mapping change. Replaced with an explicitparams.evalsOnlyfield propagated from the callers.Flag descriptions and comments updated in
audit.goandlogs_command.goto reflect the usage artifact as the evals source.Test: Added
ResolveArtifactFilter(["evals"]) == ["usage"]assertion; updated alldownloadRunArtifactsConcurrentcall sites with the newevalsOnlyparameter.