diff --git a/pkg/cli/audit.go b/pkg/cli/audit.go index 8f26dc36ba8..7ce64fe3837 100644 --- a/pkg/cli/audit.go +++ b/pkg/cli/audit.go @@ -111,7 +111,7 @@ func registerAuditCommandFlags(cmd *cobra.Command) { 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, "Skip runs that do not contain evals results (evals.jsonl); automatically downloads the evals artifact when --artifacts is narrowed") + cmd.Flags().Bool("evals", false, "Skip runs that do not contain evals results (evals.jsonl); automatically downloads the usage artifact (which includes evals) when --artifacts is narrowed") RegisterDirFlagCompletion(cmd, "output") } @@ -155,9 +155,9 @@ func getAuditCommandOptions(cmd *cobra.Command) (auditCommandOptions, error) { []string{"Add --experiment to filter by experiment name alongside --variant"}, )) } - // Auto-include the evals artifact when --evals is specified and the user has - // narrowed the artifact set (non-empty --artifacts). When --artifacts is empty - // the default is "all", which already includes every artifact including evals, + // Auto-include the usage artifact (which now contains evals) when --evals is + // specified and the user has narrowed the artifact set (non-empty --artifacts). + // When --artifacts is empty the default is "all", which already includes usage, // so we must not append here: doing so would change the default from "all" to // "evals-only" and omit the activation/agent artifacts required for a full report. if len(opts.artifacts) > 0 { @@ -322,6 +322,10 @@ type auditRunConfig struct { experimentFilter string variantFilter string evalsOnly bool + // evalsArtifactRequested is true when evals were requested via --evals or + // explicit --artifacts evals, and is used to trigger legacy dedicated-evals + // fallback behavior for older runs. + evalsArtifactRequested bool } type auditAnalysisResults struct { @@ -386,20 +390,21 @@ func newAuditRunConfig(runID int64, opts AuditOptions) (auditRunConfig, error) { return auditRunConfig{}, err } return auditRunConfig{ - runID: runID, - owner: opts.Owner, - repo: opts.Repo, - hostname: resolveAuditHostname(opts.Hostname), - outputDir: resolveAuditOutputDir(opts.OutputDir, runID), - verbose: opts.Verbose, - parse: opts.Parse, - jsonOutput: opts.JSONOutput, - jobID: opts.JobID, - stepNumber: opts.StepNumber, - artifactFilter: ResolveArtifactFilter(opts.ArtifactSets), - experimentFilter: opts.ExperimentFilter, - variantFilter: opts.VariantFilter, - evalsOnly: opts.EvalsOnly, + runID: runID, + owner: opts.Owner, + repo: opts.Repo, + hostname: resolveAuditHostname(opts.Hostname), + outputDir: resolveAuditOutputDir(opts.OutputDir, runID), + verbose: opts.Verbose, + parse: opts.Parse, + jsonOutput: opts.JSONOutput, + jobID: opts.JobID, + stepNumber: opts.StepNumber, + artifactFilter: ResolveArtifactFilter(opts.ArtifactSets), + experimentFilter: opts.ExperimentFilter, + variantFilter: opts.VariantFilter, + evalsOnly: opts.EvalsOnly, + evalsArtifactRequested: isEvalsArtifactRequested(opts.EvalsOnly, opts.ArtifactSets), }, nil } @@ -494,13 +499,13 @@ func renderCachedAuditIfAvailable(ctx context.Context, cfg auditRunConfig) (bool if shouldSkipAuditRun(cfg.runID, cfg.outputDir, cfg.experimentFilter, cfg.variantFilter) { return true, nil } - // When --evals is set but the evals artifact is not present locally, the cache - // was created without evals (e.g., default usage-only download). Bypass the - // cache so prepareAuditWorkflowRun can fetch the evals artifact; the filter at + // When evals are requested but evals are not present locally (e.g., the run was + // cached before evals were included in the usage artifact), bypass the cache + // so prepareAuditWorkflowRun can fetch the usage artifact; the filter at // the post-download check will then correctly decide whether to skip the run. - if cfg.evalsOnly && !runHasEvals(cfg.outputDir, cfg.verbose) && + if cfg.evalsArtifactRequested && !runHasEvals(cfg.outputDir, cfg.verbose) && !ensureEvalsResultsFromBranch(ctx, summary.Run, cfg.outputDir, cfg.owner, cfg.repo, cfg.hostname, cfg.verbose) { - auditLog.Printf("Cache miss for run %d evals: evals artifact not present locally, bypassing cache", cfg.runID) + auditLog.Printf("Cache miss for run %d evals: evals not present locally, bypassing cache", cfg.runID) return false, nil } processedRun := processedRunFromSummary(summary, cfg.outputDir) @@ -588,6 +593,7 @@ func downloadAuditArtifactsIfNeeded(ctx context.Context, cfg auditRunConfig, run auditLog.Printf("Downloading artifacts for run %d", cfg.runID) err := downloadRunArtifacts(ctx, cfg.runID, cfg.outputDir, cfg.verbose, cfg.owner, cfg.repo, cfg.hostname, cfg.artifactFilter) if err == nil || errors.Is(err, ErrNoArtifacts) { + downloadLegacyEvalsArtifactIfNeeded(ctx, cfg) if errors.Is(err, ErrNoArtifacts) { auditLog.Printf("No artifacts found for run %d", cfg.runID) if cfg.verbose { @@ -606,6 +612,22 @@ func downloadAuditArtifactsIfNeeded(ctx context.Context, cfg auditRunConfig, run return false, fmt.Errorf("failed to download artifacts: %w", err) } +func downloadLegacyEvalsArtifactIfNeeded(ctx context.Context, cfg auditRunConfig) { + if !cfg.evalsArtifactRequested || runHasEvals(cfg.outputDir, cfg.verbose) { + return + } + auditLog.Printf("Evals not found in usage artifact for run %d, attempting fallback download of dedicated evals artifact", cfg.runID) + evalsArtifactFilter := []string{constants.EvalsArtifactName} + if err := downloadRunArtifacts(ctx, cfg.runID, cfg.outputDir, cfg.verbose, cfg.owner, cfg.repo, cfg.hostname, evalsArtifactFilter); err != nil { + auditLog.Printf("Fallback evals artifact download failed for run %d: %v", cfg.runID, err) + if cfg.verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Evals not found in usage artifact for run %d and fallback download failed: %v", cfg.runID, err))) + } + return + } + auditLog.Printf("Fallback evals artifact downloaded for run %d", cfg.runID) +} + func cacheRecoveryError(message string, runID int64, runOutputDir string, err error) error { return fmt.Errorf(message+"\n\n"+ "To download artifacts, use the GitHub MCP server:\n\n"+ diff --git a/pkg/cli/audit_evals_fallback_test.go b/pkg/cli/audit_evals_fallback_test.go new file mode 100644 index 00000000000..531daf9b74e --- /dev/null +++ b/pkg/cli/audit_evals_fallback_test.go @@ -0,0 +1,43 @@ +//go:build !integration + +package cli + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewAuditRunConfigMarksExplicitEvalsArtifactRequest(t *testing.T) { + cfg, err := newAuditRunConfig(123, AuditOptions{ + ArtifactSets: []string{"evals"}, + EvalsOnly: false, + }) + require.NoError(t, err) + assert.True(t, cfg.evalsArtifactRequested) +} + +func TestRenderCachedAuditIfAvailableBypassesCacheWhenExplicitEvalsArtifactRequested(t *testing.T) { + runOutputDir := t.TempDir() + summary := &RunSummary{ + CLIVersion: GetVersion(), + RunID: 123, + ProcessedAt: time.Now(), + Run: WorkflowRun{ + DatabaseID: 123, + }, + } + require.NoError(t, saveRunSummary(runOutputDir, summary, false)) + + done, err := renderCachedAuditIfAvailable(context.Background(), auditRunConfig{ + runID: 123, + outputDir: runOutputDir, + verbose: false, + evalsArtifactRequested: true, + }) + require.NoError(t, err) + assert.False(t, done, "cache should be bypassed so legacy evals fallback can run") +} diff --git a/pkg/cli/logs_artifact_set.go b/pkg/cli/logs_artifact_set.go index 97300ddda5f..6aa1f24851f 100644 --- a/pkg/cli/logs_artifact_set.go +++ b/pkg/cli/logs_artifact_set.go @@ -69,8 +69,8 @@ const ( // conclusion job (aw-info.jsonl, usage summaries, token usage JSONL). ArtifactSetUsage ArtifactSet = "usage" - // ArtifactSetEvals downloads the evals artifact containing BinEval evaluation - // results (evals.jsonl) produced by the evals job. + // ArtifactSetEvals downloads the usage artifact, which now includes evals.jsonl + // produced by the evals job (copied into usage by the conclusion job). ArtifactSetEvals ArtifactSet = "evals" ) @@ -91,8 +91,8 @@ var artifactSetArtifacts = map[ArtifactSet][]string{ ArtifactSetExperiment: {constants.ExperimentArtifactName}, // usage: compact conclusion artifact for lightweight reporting/forecasting. ArtifactSetUsage: {constants.UsageArtifactName}, - // 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}, } const maxArtifactHintExamples = 2 @@ -284,17 +284,29 @@ func findMissingFilterEntries(filter []string, outputDir string) []string { } // applyEvalsArtifact appends the evals artifact set to artifacts when evalsOnly is true -// and neither ArtifactSetEvals nor ArtifactSetAll is already present. This ensures -// evals.jsonl is downloaded without requiring the user to also pass --artifacts evals. +// and neither ArtifactSetEvals, ArtifactSetUsage, nor ArtifactSetAll is already present. +// Because evals results are now included in the usage artifact, this ensures evals.jsonl +// is downloaded without requiring the user to also pass --artifacts evals or --artifacts usage. // -// Note: callers that treat an empty artifacts slice as "all" (e.g., the audit command) -// should guard with len(artifacts) > 0 before calling this function, to avoid -// changing the empty/"all" default into an evals-only download. +// For callers that treat an empty artifacts slice as "all", the function returns +// the empty slice unchanged and does not append evals. func applyEvalsArtifact(artifacts []string, evalsOnly bool) []string { + if len(artifacts) == 0 { + return artifacts + } if evalsOnly && !slices.Contains(artifacts, string(ArtifactSetEvals)) && + !slices.Contains(artifacts, string(ArtifactSetUsage)) && !slices.Contains(artifacts, string(ArtifactSetAll)) { return append(artifacts, string(ArtifactSetEvals)) } return artifacts } + +// isEvalsArtifactRequested reports whether evals were explicitly requested, +// either via --evals or by including --artifacts evals. Callers use this to +// decide whether to bypass stale cache entries and trigger legacy dedicated-evals +// fallback downloads when evals.jsonl is missing from usage artifacts. +func isEvalsArtifactRequested(evalsOnly bool, artifactSets []string) bool { + return evalsOnly || slices.Contains(artifactSets, string(ArtifactSetEvals)) +} diff --git a/pkg/cli/logs_artifact_set_test.go b/pkg/cli/logs_artifact_set_test.go index 5da73f36c76..6de30a5058f 100644 --- a/pkg/cli/logs_artifact_set_test.go +++ b/pkg/cli/logs_artifact_set_test.go @@ -157,6 +157,11 @@ func TestResolveArtifactFilter(t *testing.T) { sets: []string{"usage"}, expected: []string{"usage"}, }, + { + name: "evals resolves to usage artifact (evals now included in usage)", + sets: []string{"evals"}, + expected: []string{"usage"}, + }, { name: "multiple sets are merged and deduplicated", sets: []string{"activation", "agent"}, @@ -256,6 +261,55 @@ func TestValidArtifactSetNames(t *testing.T) { assert.ElementsMatch(t, expected, names, "ValidArtifactSetNames should contain all known sets") } +func TestApplyEvalsArtifact(t *testing.T) { + t.Run("returns empty slice unchanged when artifact list is empty", func(t *testing.T) { + assert.Empty(t, applyEvalsArtifact(nil, true)) + assert.Empty(t, applyEvalsArtifact([]string{}, true)) + }) + + t.Run("appends evals when evals requested and artifact list narrowed", func(t *testing.T) { + assert.Equal(t, []string{"agent", "evals"}, applyEvalsArtifact([]string{"agent"}, true)) + }) + + t.Run("does not append evals when usage already present", func(t *testing.T) { + assert.Equal(t, []string{"usage"}, applyEvalsArtifact([]string{"usage"}, true)) + }) +} + +func TestIsEvalsArtifactRequested(t *testing.T) { + tests := []struct { + name string + evalsOnly bool + artifactSets []string + expected bool + }{ + { + name: "true when --evals is set", + evalsOnly: true, + artifactSets: nil, + expected: true, + }, + { + name: "true when explicit evals artifact set is requested", + evalsOnly: false, + artifactSets: []string{"evals"}, + expected: true, + }, + { + name: "false when evals are not requested", + evalsOnly: false, + artifactSets: []string{"usage"}, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, isEvalsArtifactRequested(tt.evalsOnly, tt.artifactSets)) + }) + } +} + func TestIsUsageOnlyArtifactFilter(t *testing.T) { tests := []struct { name string diff --git a/pkg/cli/logs_command.go b/pkg/cli/logs_command.go index 322cc6a2efa..6b6143d228c 100644 --- a/pkg/cli/logs_command.go +++ b/pkg/cli/logs_command.go @@ -172,7 +172,9 @@ Downloaded artifacts include (when using --artifacts all): return err } - artifacts = applyEvalsArtifact(artifacts, evalsOnly) + if len(artifacts) > 0 { + artifacts = applyEvalsArtifact(artifacts, evalsOnly) + } return DownloadWorkflowLogsFromStdin(cmd.Context(), StdinLogsOptions{ RunURLs: runURLs, @@ -327,7 +329,9 @@ Downloaded artifacts include (when using --artifacts all): logsCommandLog.Printf("Executing logs download: workflow=%s, count=%d, engine=%s, train=%v, cache_before=%s", workflowName, count, engine, train, cacheBefore) - artifacts = applyEvalsArtifact(artifacts, evalsOnly) + if len(artifacts) > 0 { + artifacts = applyEvalsArtifact(artifacts, evalsOnly) + } return DownloadWorkflowLogs(cmd.Context(), LogsDownloadOptions{ WorkflowName: workflowName, @@ -377,7 +381,7 @@ Downloaded artifacts include (when using --artifacts all): logsCmd.Flags().Bool("no-firewall", false, "Filter to only runs without firewall enabled") logsCmd.Flags().String("safe-output", "", "Filter to runs containing a specific safe output type (e.g., create-issue, missing-tool, missing-data, noop, report-incomplete)") logsCmd.Flags().Bool("filtered-integrity", false, "Filter to runs containing items that were filtered by gateway integrity checks") - logsCmd.Flags().Bool("evals", false, "Filter to runs containing evals results (evals.jsonl); automatically includes the evals artifact") + logsCmd.Flags().Bool("evals", false, "Filter to runs containing evals results (evals.jsonl); automatically includes the usage artifact (which contains evals)") logsCmd.Flags().Bool("parse", false, "Run JavaScript parsers on agent logs and firewall logs, writing Markdown to log.md and firewall.md") addJSONFlag(logsCmd) logsCmd.Flags().Int("timeout", 0, "Download timeout in minutes (0 = no timeout)") diff --git a/pkg/cli/logs_orchestrator.go b/pkg/cli/logs_orchestrator.go index 95a8a8e0271..5881efd515b 100644 --- a/pkg/cli/logs_orchestrator.go +++ b/pkg/cli/logs_orchestrator.go @@ -444,7 +444,7 @@ outerLoop: chunk := runsRemaining[:chunkSize] runsRemaining = runsRemaining[chunkSize:] - downloadResults := downloadRunArtifactsConcurrent(activeCtx, chunk, outputDir, verbose, remainingNeeded, repoOverride, artifactFilter) + downloadResults := downloadRunArtifactsConcurrent(activeCtx, chunk, outputDir, verbose, remainingNeeded, repoOverride, artifactFilter, evalsOnly, artifactSets) for _, result := range downloadResults { if result.Skipped { diff --git a/pkg/cli/logs_orchestrator_stdin.go b/pkg/cli/logs_orchestrator_stdin.go index e50dd7ca77a..119074ad964 100644 --- a/pkg/cli/logs_orchestrator_stdin.go +++ b/pkg/cli/logs_orchestrator_stdin.go @@ -147,7 +147,7 @@ func DownloadWorkflowLogsFromStdin(ctx context.Context, opts StdinLogsOptions) e } // Download artifacts for all runs concurrently. - downloadResults := downloadRunArtifactsConcurrent(ctx, runs, opts.OutputDir, opts.Verbose, len(runs), opts.RepoOverride, artifactFilter) + downloadResults := downloadRunArtifactsConcurrent(ctx, runs, opts.OutputDir, opts.Verbose, len(runs), opts.RepoOverride, artifactFilter, opts.EvalsOnly, opts.ArtifactSets) filters := runFilterOpts{ engine: opts.Engine, diff --git a/pkg/cli/logs_orchestrator_test.go b/pkg/cli/logs_orchestrator_test.go index 093b71498ef..833da9003d6 100644 --- a/pkg/cli/logs_orchestrator_test.go +++ b/pkg/cli/logs_orchestrator_test.go @@ -19,7 +19,7 @@ import ( // TestDownloadRunArtifactsConcurrent_EmptyRuns tests that empty runs slice returns empty results func TestDownloadRunArtifactsConcurrent_EmptyRuns(t *testing.T) { ctx := context.Background() - results := downloadRunArtifactsConcurrent(ctx, []WorkflowRun{}, "./test-logs", false, 5, "", nil) + results := downloadRunArtifactsConcurrent(ctx, []WorkflowRun{}, "./test-logs", false, 5, "", nil, false, nil) assert.Empty(t, results, "Expected empty results for empty runs slice") } @@ -38,7 +38,7 @@ func TestDownloadRunArtifactsConcurrent_ResultOrdering(t *testing.T) { } tmpDir := testutil.TempDir(t, "test-orchestrator-*") - results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 5, "", nil) + results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 5, "", nil, false, nil) // Verify we got all results require.Len(t, results, 5, "Expected 5 results") @@ -71,7 +71,7 @@ func TestDownloadRunArtifactsConcurrent_AllProcessed(t *testing.T) { tmpDir := testutil.TempDir(t, "test-orchestrator-*") // Pass maxRuns=3 as a hint, but all runs should still be processed - results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 3, "", nil) + results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 3, "", nil, false, nil) // All runs should be processed to account for caching/filtering require.Len(t, results, 5, "All runs should be processed regardless of maxRuns parameter") @@ -101,7 +101,7 @@ func TestDownloadRunArtifactsConcurrent_ContextCancellation(t *testing.T) { } tmpDir := testutil.TempDir(t, "test-orchestrator-*") - results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 5, "", nil) + results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 5, "", nil, false, nil) // Should still get results for all runs require.Len(t, results, 3, "Expected 3 results even with cancelled context") @@ -130,7 +130,7 @@ func TestDownloadRunArtifactsConcurrent_PartialCancellation(t *testing.T) { } tmpDir := testutil.TempDir(t, "test-orchestrator-*") - results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 20, "", nil) + results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 20, "", nil, false, nil) // Should get results for all runs (some may be skipped due to timeout) assert.Len(t, results, 20, "Should get results for all runs") @@ -165,7 +165,7 @@ func TestDownloadRunArtifactsConcurrent_NoResourceLeaks(t *testing.T) { } tmpDir := testutil.TempDir(t, "test-orchestrator-*") - results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 3, "", nil) + results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 3, "", nil, false, nil) require.Len(t, results, 3, "Expected 3 results") @@ -248,7 +248,7 @@ func TestDownloadRunArtifactsConcurrent_ConcurrencyLimit(t *testing.T) { // We can't directly test the pool's behavior without mocking, // but we can verify the limit is configured correctly tmpDir := testutil.TempDir(t, "test-orchestrator-*") - results := downloadRunArtifactsConcurrent(context.Background(), runs, tmpDir, false, tt.runs, "", nil) + results := downloadRunArtifactsConcurrent(context.Background(), runs, tmpDir, false, tt.runs, "", nil, false, nil) require.Len(t, results, tt.runs, "Expected %d results", tt.runs) @@ -271,7 +271,7 @@ func TestDownloadRunArtifactsConcurrent_LogsPath(t *testing.T) { } tmpDir := testutil.TempDir(t, "test-orchestrator-*") - results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 2, "", nil) + results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 2, "", nil, false, nil) require.Len(t, results, 2, "Expected 2 results") @@ -295,7 +295,7 @@ func TestDownloadRunArtifactsConcurrent_ErrorHandling(t *testing.T) { } tmpDir := testutil.TempDir(t, "test-orchestrator-*") - results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 2, "", nil) + results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 2, "", nil, false, nil) require.Len(t, results, 2, "Expected 2 results even with errors") @@ -324,7 +324,7 @@ func TestDownloadRunArtifactsConcurrent_MixedConclusions(t *testing.T) { } tmpDir := testutil.TempDir(t, "test-orchestrator-*") - results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 5, "", nil) + results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 5, "", nil, false, nil) require.Len(t, results, 5, "Expected 5 results") @@ -354,11 +354,11 @@ func TestDownloadRunArtifactsConcurrent_VerboseMode(t *testing.T) { tmpDir := testutil.TempDir(t, "test-orchestrator-*") // Test with verbose=false - resultsNonVerbose := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 2, "", nil) + resultsNonVerbose := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 2, "", nil, false, nil) require.Len(t, resultsNonVerbose, 2, "Non-verbose mode should return 2 results") // Test with verbose=true - resultsVerbose := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, true, 2, "", nil) + resultsVerbose := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, true, 2, "", nil, false, nil) require.Len(t, resultsVerbose, 2, "Verbose mode should return 2 results") // Verify both modes return the same set of IDs (regardless of order) @@ -392,7 +392,7 @@ func TestDownloadRunArtifactsConcurrent_ResultStructure(t *testing.T) { } tmpDir := testutil.TempDir(t, "test-orchestrator-*") - results := downloadRunArtifactsConcurrent(ctx, []WorkflowRun{run}, tmpDir, false, 1, "", nil) + results := downloadRunArtifactsConcurrent(ctx, []WorkflowRun{run}, tmpDir, false, 1, "", nil, false, nil) require.Len(t, results, 1, "Expected 1 result") @@ -437,7 +437,7 @@ func TestDownloadRunArtifactsConcurrent_PanicRecovery(t *testing.T) { } tmpDir := testutil.TempDir(t, "test-orchestrator-*") - results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 3, "", nil) + results := downloadRunArtifactsConcurrent(ctx, runs, tmpDir, false, 3, "", nil, false, nil) // Even if one download panicked, we should get results for all runs // (The actual panic recovery is tested by the conc pool library) diff --git a/pkg/cli/logs_parallel_test.go b/pkg/cli/logs_parallel_test.go index ce14d476e10..b683e0d1193 100644 --- a/pkg/cli/logs_parallel_test.go +++ b/pkg/cli/logs_parallel_test.go @@ -14,7 +14,7 @@ import ( func TestDownloadRunArtifactsParallel(t *testing.T) { // Test with empty runs slice - results := downloadRunArtifactsConcurrent(context.Background(), []WorkflowRun{}, "./test-logs", false, 5, "", nil) + results := downloadRunArtifactsConcurrent(context.Background(), []WorkflowRun{}, "./test-logs", false, 5, "", nil, false, nil) if len(results) != 0 { t.Errorf("Expected 0 results for empty runs, got %d", len(results)) } @@ -45,7 +45,7 @@ func TestDownloadRunArtifactsParallel(t *testing.T) { // This will fail since we don't have real GitHub CLI access, // but we can verify the structure and that no panics occur - results = downloadRunArtifactsConcurrent(context.Background(), runs, "./test-logs", false, 5, "", nil) + results = downloadRunArtifactsConcurrent(context.Background(), runs, "./test-logs", false, 5, "", nil, false, nil) // We expect 2 results even if they fail if len(results) != 2 { @@ -85,7 +85,7 @@ func TestDownloadRunArtifactsParallelMaxRuns(t *testing.T) { } // Pass maxRuns=3 as a hint that we need 3 results, but all runs should be processed - results := downloadRunArtifactsConcurrent(context.Background(), runs, "./test-logs", false, 3, "", nil) + results := downloadRunArtifactsConcurrent(context.Background(), runs, "./test-logs", false, 3, "", nil, false, nil) // All runs should be processed to account for potential caching/filtering if len(results) != 5 { @@ -284,7 +284,7 @@ func TestDownloadRunArtifactsParallelWithCancellation(t *testing.T) { } // Download with cancelled context - results := downloadRunArtifactsConcurrent(ctx, runs, "./test-logs", false, 5, "", nil) + results := downloadRunArtifactsConcurrent(ctx, runs, "./test-logs", false, 5, "", nil, false, nil) // Should get results for all runs if len(results) != 2 { diff --git a/pkg/cli/logs_run_processor.go b/pkg/cli/logs_run_processor.go index e82ae72807b..7171fd2ed44 100644 --- a/pkg/cli/logs_run_processor.go +++ b/pkg/cli/logs_run_processor.go @@ -16,7 +16,6 @@ import ( "math" "os" "path/filepath" - "slices" "strings" "sync/atomic" "time" @@ -38,11 +37,20 @@ type concurrentRunDownloadParams struct { dlOwner string dlRepo string artifactFilter []string + evalsOnly bool + // evalsArtifactRequested is true when the caller wants evals results, either + // because --evals was passed (evalsOnly) or because --artifacts evals was + // explicitly listed. This drives the fallback download of the dedicated evals + // artifact when evals.jsonl is absent from the usage artifact. + evalsArtifactRequested bool } // buildConcurrentDownloadParams constructs download parameters by parsing the optional // repoOverride ("owner/repo" or "HOST/owner/repo") once for the whole batch. -func buildConcurrentDownloadParams(outputDir string, verbose bool, repoOverride string, artifactFilter []string) concurrentRunDownloadParams { +// artifactSets is the original (pre-resolution) set list; it is used to detect whether +// the caller explicitly requested the evals artifact set so the fallback download can +// be triggered even when --evals was not set. +func buildConcurrentDownloadParams(outputDir string, verbose bool, repoOverride string, artifactFilter []string, evalsOnly bool, artifactSets []string) concurrentRunDownloadParams { var dlHost, dlOwner, dlRepo string if repoOverride != "" { // Accepted formats: "owner/repo" or "HOST/owner/repo". @@ -54,13 +62,16 @@ func buildConcurrentDownloadParams(outputDir string, verbose bool, repoOverride dlOwner, dlRepo = parts[0], parts[1] } } + evalsArtifactRequested := isEvalsArtifactRequested(evalsOnly, artifactSets) return concurrentRunDownloadParams{ - outputDir: outputDir, - verbose: verbose, - dlHost: dlHost, - dlOwner: dlOwner, - dlRepo: dlRepo, - artifactFilter: artifactFilter, + outputDir: outputDir, + verbose: verbose, + dlHost: dlHost, + dlOwner: dlOwner, + dlRepo: dlRepo, + artifactFilter: artifactFilter, + evalsOnly: evalsOnly, + evalsArtifactRequested: evalsArtifactRequested, } } @@ -87,8 +98,10 @@ func logConcurrentDownloadSummary(results []DownloadResult) { fmt.Sprintf("Completed parallel processing: %d successful, %d total", successCount, len(results)))) } -// downloadRunArtifactsConcurrent downloads artifacts for multiple workflow runs concurrently -func downloadRunArtifactsConcurrent(ctx context.Context, runs []WorkflowRun, outputDir string, verbose bool, maxRuns int, repoOverride string, artifactFilter []string) []DownloadResult { +// downloadRunArtifactsConcurrent downloads artifacts for multiple workflow runs concurrently. +// artifactSets is the original (pre-resolution) set list passed by the caller; it is used +// alongside evalsOnly to determine whether the evals artifact fallback should run. +func downloadRunArtifactsConcurrent(ctx context.Context, runs []WorkflowRun, outputDir string, verbose bool, maxRuns int, repoOverride string, artifactFilter []string, evalsOnly bool, artifactSets []string) []DownloadResult { logsOrchestratorLog.Printf("Starting concurrent artifact download: runs=%d, outputDir=%s, maxRuns=%d", len(runs), outputDir, maxRuns) if len(runs) == 0 { return []DownloadResult{} @@ -104,7 +117,7 @@ func downloadRunArtifactsConcurrent(ctx context.Context, runs []WorkflowRun, out progressBar := initDownloadProgressBar(verbose, totalRuns) var completedCount atomic.Int64 maxConcurrent := getMaxConcurrentDownloads() - params := buildConcurrentDownloadParams(outputDir, verbose, repoOverride, artifactFilter) + params := buildConcurrentDownloadParams(outputDir, verbose, repoOverride, artifactFilter, evalsOnly, artifactSets) // Configure concurrent download pool with bounded parallelism and context cancellation. // The conc pool automatically handles panic recovery and prevents goroutine leaks. @@ -195,6 +208,13 @@ func processSingleRunDownload( if err != nil { handleArtifactDownloadError(result, err, params.verbose) } else { + // When evals are requested but not found in the usage artifact (older runs + // 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) { + tryDownloadEvalsArtifactFallback(ctx, run.DatabaseID, runOutputDir, perRunParams) + } analyzeRunArtifacts(result, runOutputDir, params.verbose, params.artifactFilter) } } else { @@ -208,6 +228,24 @@ func processSingleRunDownload( return *result, nil } +// tryDownloadEvalsArtifactFallback attempts to download the dedicated evals artifact for +// a run that already has its primary artifacts on disk but does not contain evals.jsonl. +// This handles older runs where evals.jsonl was uploaded as a standalone artifact instead +// of being included in the usage artifact by the conclusion job. +// Errors are logged but not propagated — the caller proceeds with whatever was downloaded. +func tryDownloadEvalsArtifactFallback(ctx context.Context, runID int64, runOutputDir string, params concurrentRunDownloadParams) { + logsOrchestratorLog.Printf("evals not found in usage artifact for run %d, attempting fallback download of dedicated evals artifact", runID) + evalsFilter := []string{constants.EvalsArtifactName} + if err := downloadRunArtifacts(ctx, runID, runOutputDir, params.verbose, params.dlOwner, params.dlRepo, params.dlHost, evalsFilter); err != nil { + logsOrchestratorLog.Printf("Fallback evals artifact download failed for run %d: %v", runID, err) + if params.verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Evals not found in usage artifact for run %d and fallback download failed: %v", runID, err))) + } + } else { + logsOrchestratorLog.Printf("Fallback evals artifact downloaded for run %d", runID) + } +} + // tryLoadCachedRunResult attempts to return a pre-built DownloadResult from the on-disk // cache. Returns (result, true) on a valid cache hit; (nil, false) otherwise. func tryLoadCachedRunResult( @@ -221,15 +259,14 @@ func tryLoadCachedRunResult( return nil, false } - // When the caller requested the evals artifact and it is not present in the - // cached run directory, the cache was created without evals. Bypass the cache - // so the fresh download can fetch evals; the post-download filter decides whether - // to skip. - evalsRequested := slices.Contains(params.artifactFilter, constants.EvalsArtifactName) + // When --evals is requested but evals are not present in the cached run directory + // (e.g., the run was cached before evals were included in the usage artifact), + // bypass the cache so the fresh download can include the usage artifact with evals; + // the post-download filter decides whether to skip. hasEvals := runHasEvals(runOutputDir, params.verbose) || 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.evalsArtifactRequested && !hasEvals { + logsOrchestratorLog.Printf("Cache bypass for run %d: evals requested (--evals or --artifacts evals) but not present locally", run.DatabaseID) return nil, false } diff --git a/pkg/cli/logs_run_processor_test.go b/pkg/cli/logs_run_processor_test.go index 9e6c1c360a4..e76f907cfe6 100644 --- a/pkg/cli/logs_run_processor_test.go +++ b/pkg/cli/logs_run_processor_test.go @@ -3,9 +3,11 @@ package cli import ( + "context" "os" "path/filepath" "testing" + "time" "github.com/github/gh-aw/pkg/constants" "github.com/stretchr/testify/assert" @@ -134,3 +136,46 @@ func TestBackfillRunTokenUsageFromFirewall(t *testing.T) { assert.Equal(t, 123, result.Run.TokenUsage) }) } + +func TestTryLoadCachedRunResultBypassesForExplicitEvalsArtifactRequest(t *testing.T) { + runOutputDir := t.TempDir() + summary := &RunSummary{ + CLIVersion: GetVersion(), + RunID: 123, + ProcessedAt: time.Now(), + Run: WorkflowRun{ + DatabaseID: 123, + }, + } + require.NoError(t, saveRunSummary(runOutputDir, summary, false)) + + result, ok := tryLoadCachedRunResult(context.Background(), WorkflowRun{DatabaseID: 123}, runOutputDir, concurrentRunDownloadParams{ + evalsOnly: false, + evalsArtifactRequested: true, + verbose: false, + }) + assert.False(t, ok, "cache should be bypassed so fallback download can run for explicit --artifacts evals") + assert.Nil(t, result) +} + +func TestTryLoadCachedRunResultUsesCacheWhenEvalsNotRequested(t *testing.T) { + runOutputDir := t.TempDir() + summary := &RunSummary{ + CLIVersion: GetVersion(), + RunID: 124, + ProcessedAt: time.Now(), + Run: WorkflowRun{ + DatabaseID: 124, + }, + } + require.NoError(t, saveRunSummary(runOutputDir, summary, false)) + + result, ok := tryLoadCachedRunResult(context.Background(), WorkflowRun{DatabaseID: 124}, runOutputDir, concurrentRunDownloadParams{ + evalsOnly: false, + evalsArtifactRequested: false, + verbose: false, + }) + require.True(t, ok) + require.NotNil(t, result) + assert.True(t, result.Cached) +}