Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 45 additions & 23 deletions pkg/cli/audit.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Expand Down Expand Up @@ -155,9 +155,9 @@ func getAuditCommandOptions(cmd *cobra.Command) (auditCommandOptions, error) {
[]string{"Add --experiment <name> 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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand All @@ -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"+
Expand Down
43 changes: 43 additions & 0 deletions pkg/cli/audit_evals_fallback_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
30 changes: 21 additions & 9 deletions pkg/cli/logs_artifact_set.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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},
Comment on lines +94 to +95

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

}

const maxArtifactHintExamples = 2
Expand Down Expand Up @@ -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))
}
54 changes: 54 additions & 0 deletions pkg/cli/logs_artifact_set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand Down Expand Up @@ -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
Expand Down
10 changes: 7 additions & 3 deletions pkg/cli/logs_command.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)")
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/logs_orchestrator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/logs_orchestrator_stdin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading
Loading