diff --git a/docs/adr/43732-engine-id-lock-file-fallback.md b/docs/adr/43732-engine-id-lock-file-fallback.md new file mode 100644 index 00000000000..ce5fe34b890 --- /dev/null +++ b/docs/adr/43732-engine-id-lock-file-fallback.md @@ -0,0 +1,56 @@ +# ADR-43732: Two-Tier Fallback for Resolving engine_id in Workflow Run Logs + +**Date**: 2026-07-06 +**Status**: Draft +**Deciders**: pelikhan (PR author), copilot-swe-agent (implementer) + +--- + +### Context + +The `gh aw logs` command supports an `--engine` filter that matches workflow runs against an `engine_id`. This ID was previously sourced exclusively from an `aw_info.json` artifact uploaded during workflow execution. Older workflow runs (e.g., runs from the `daily-cli-tools-tester` workflow family that pre-date `aw_info.json` adoption) never upload this artifact, causing `engine_id` to resolve to `null`. When null, the `--engine` filter silently excludes these runs, making historical data invisible to users. The lock files (`.lock.yml`) for agentic workflows already embed a `# gh-aw-metadata:` JSON comment that contains an `agent_id` field equivalent to `engine_id`, so a fallback source already exists within the repository itself. + +### Decision + +We will establish a deterministic two-tier precedence for resolving `engine_id` when processing workflow run logs: + +1. **Authoritative**: `aw_info.json` artifact's `engine_id` field — used whenever the artifact is present and non-empty. +2. **Fallback**: the `agent_id` field parsed from the `# gh-aw-metadata:` comment embedded in the run's `.lock.yml` file — consulted only when `aw_info.json` is absent or carries an empty `engine_id`. +3. **Unknown**: if neither source yields a value, `engine_id` remains unresolvable and the run is excluded from `--engine` filter results. + +This precedence is implemented in `logs_parsing_core.go` (new `extractEngineIDFromLockFile` and `resolveToLockFilePath` helpers), `logs_orchestrator.go` (fallback parameter added to `matchEngineFilter`), and `logs_report.go` (fallback applied in `buildLogsData`). + +### Alternatives Considered + +#### Alternative 1: Require all workflows to adopt aw_info.json and do nothing for older runs + +Accept that older runs without `aw_info.json` are excluded from `--engine` filter results as a transitional cost. This is simpler — no fallback logic required. This was rejected because the `daily-cli-tools-tester` family represents a significant portion of historical run data, and silently dropping those runs from engine-scoped queries yields misleading results with no user-visible warning. + +#### Alternative 2: Back-fill aw_info.json artifacts for historical runs + +Retroactively upload `aw_info.json` artifacts for all pre-existing runs missing them. This would make the data consistent and avoid dual-source complexity. This was rejected because GitHub Actions artifacts cannot be added to completed runs after the fact via the public API; the approach is not technically feasible. + +#### Alternative 3: Introduce a separate metadata cache (e.g., a database or side-car file) + +Maintain a persistent mapping of workflow run ID → engine_id in a side-car cache (e.g., a JSON file in the repo-memory branch). This was rejected because it introduces operational complexity (cache invalidation, race conditions across concurrent runs) and offers no advantage over the lock file source, which is already co-located in the repository and read-only after merge. + +### Consequences + +#### Positive +- Older workflow runs that pre-date `aw_info.json` adoption now appear correctly in `--engine` filtered queries, restoring historical data visibility. +- The lock file's `gh-aw-metadata` comment becomes a formally recognized secondary source for engine identity, leveraging data that was already present. +- `aw_info.json` retains full authority; the fallback does not alter behavior for any run that already populates it. +- The precedence hierarchy is explicit and documented in code, reducing the risk of future contributors inadvertently changing resolution order. + +#### Negative +- The `engine_id` resolution path now has two code paths that must stay in sync — if the lock file format for `gh-aw-metadata` changes, the fallback parser (`extractEngineIDFromLockFile`) must be updated separately. +- Resolving engine_id now requires reading a local file (`.lock.yml`) for older runs, adding a file I/O dependency that was not present before; runs whose lock files are missing or malformed will silently fall back to `engine_id: unknown`. +- The `matchEngineFilter` function signature changed (added `lockFileEngineID` parameter), requiring all call sites to be updated — a small but non-zero refactoring cost for future callers. + +#### Neutral +- The `resolveToLockFilePath` helper normalises `.yml`, `.md`, and `.lock.yml` inputs to a canonical `.lock.yml` path; this normalization is reusable but currently only used in one context. +- The fallback only applies to `engine_id` — other metadata fields in `aw_info.json` do not gain a lock-file fallback, keeping the scope of this change narrow. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/logs_engine_filter_test.go b/pkg/cli/logs_engine_filter_test.go index 7dddd67e179..463c2ec0542 100644 --- a/pkg/cli/logs_engine_filter_test.go +++ b/pkg/cli/logs_engine_filter_test.go @@ -5,6 +5,7 @@ package cli import ( "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -12,13 +13,13 @@ import ( ) // TestMatchEngineFilter verifies that matchEngineFilter correctly compares -// awInfo.EngineID against the filter string. This is a regression test for the -// bug where the --engine filter was silently ignored and runs from all engines -// were returned regardless of the filter value. +// awInfo.EngineID against the filter string, and falls back to lockFileEngineID +// when aw_info.json is absent or has no engine_id. func TestMatchEngineFilter(t *testing.T) { cases := []struct { name string awInfoContent string // empty means no file + lockFileEngineID string filterEngine string expectMatch bool expectDetectedID string @@ -52,19 +53,52 @@ func TestMatchEngineFilter(t *testing.T) { expectDetectedID: "codex", }, { - name: "missing aw_info.json does not match any filter", + name: "missing aw_info.json does not match any filter without lock file fallback", awInfoContent: "", filterEngine: "claude", expectMatch: false, expectDetectedID: "", }, { - name: "empty engine_id does not match any filter", + name: "empty engine_id does not match any filter without lock file fallback", awInfoContent: `{"engine_id": ""}`, filterEngine: "claude", expectMatch: false, expectDetectedID: "", }, + // Lock file fallback cases + { + name: "missing aw_info.json matches filter via lock file fallback", + awInfoContent: "", + lockFileEngineID: "copilot", + filterEngine: "copilot", + expectMatch: true, + expectDetectedID: "copilot", + }, + { + name: "missing aw_info.json does not match different filter via lock file fallback", + awInfoContent: "", + lockFileEngineID: "copilot", + filterEngine: "claude", + expectMatch: false, + expectDetectedID: "copilot", + }, + { + name: "empty engine_id in aw_info.json falls back to lock file", + awInfoContent: `{"engine_id": ""}`, + lockFileEngineID: "copilot", + filterEngine: "copilot", + expectMatch: true, + expectDetectedID: "copilot", + }, + { + name: "aw_info.json engine_id takes precedence over lock file fallback", + awInfoContent: `{"engine_id": "claude"}`, + lockFileEngineID: "copilot", + filterEngine: "claude", + expectMatch: true, + expectDetectedID: "claude", + }, } for _, tc := range cases { @@ -77,10 +111,109 @@ func TestMatchEngineFilter(t *testing.T) { } awInfo, awInfoErr := parseAwInfo(awInfoPath, false) - gotMatch, gotDetectedID := matchEngineFilter(awInfo, awInfoErr, tc.filterEngine) + gotMatch, gotDetectedID := matchEngineFilter(awInfo, awInfoErr, tc.filterEngine, tc.lockFileEngineID) assert.Equal(t, tc.expectMatch, gotMatch, "match") assert.Equal(t, tc.expectDetectedID, gotDetectedID, "detectedEngineID") }) } } + +// TestExtractEngineIDFromLockFile verifies that extractEngineIDFromLockFile +// correctly reads the agent_id from the gh-aw-metadata comment in a lock file. +func TestExtractEngineIDFromLockFile(t *testing.T) { + cases := []struct { + name string + lockFileContent string // empty means no file + workflowPath string // relative path used in test + skipFileCreate bool + expectEngineID string + }{ + { + name: "copilot agent_id from lock file", + lockFileContent: `# gh-aw-metadata: {"schema_version":"v3","frontmatter_hash":"abc123","agent_id":"copilot"} +name: daily-cli-tools-tester +`, + expectEngineID: "copilot", + }, + { + name: "claude agent_id from lock file", + lockFileContent: `# gh-aw-metadata: {"schema_version":"v3","frontmatter_hash":"def456","agent_id":"claude"} +name: smoke-claude +`, + expectEngineID: "claude", + }, + { + name: "missing lock file returns empty", + lockFileContent: "", + expectEngineID: "", + }, + { + name: "lock file without agent_id returns empty", + lockFileContent: `# gh-aw-metadata: {"schema_version":"v1","frontmatter_hash":"xyz789"} +name: no-agent-id-workflow +`, + expectEngineID: "", + }, + { + name: "empty workflow path returns empty", + workflowPath: "", + skipFileCreate: true, + expectEngineID: "", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + tmpDir := t.TempDir() + + // Use .lock.yml extension by default + workflowPath := tc.workflowPath + if !tc.skipFileCreate { + // Create a lock file and use its path + lockFilePath := filepath.Join(tmpDir, "test-workflow.lock.yml") + if tc.lockFileContent != "" { + require.NoError(t, os.WriteFile(lockFilePath, []byte(tc.lockFileContent), 0644)) + } + workflowPath = lockFilePath + } + + gotEngineID := extractEngineIDFromLockFile(workflowPath, false) + assert.Equal(t, tc.expectEngineID, gotEngineID) + }) + } +} + +// TestExtractEngineIDFromLockFilePlainYml verifies that a plain .yml path is +// resolved to the corresponding .lock.yml file. +func TestExtractEngineIDFromLockFilePlainYml(t *testing.T) { + tmpDir := t.TempDir() + + lockContent := `# gh-aw-metadata: {"schema_version":"v3","frontmatter_hash":"abc","agent_id":"copilot"} +name: test +` + // Write the lock file with .lock.yml extension + lockFilePath := filepath.Join(tmpDir, "my-workflow.lock.yml") + require.NoError(t, os.WriteFile(lockFilePath, []byte(lockContent), 0644)) + + // Pass the .yml path (without .lock) — should resolve to the lock file + ymlPath := strings.TrimSuffix(lockFilePath, ".lock.yml") + ".yml" + got := extractEngineIDFromLockFile(ymlPath, false) + assert.Equal(t, "copilot", got) +} + +// TestExtractEngineIDFromLockFilePlainYAML verifies that a plain .yaml path is +// resolved to the corresponding .lock.yml file. +func TestExtractEngineIDFromLockFilePlainYAML(t *testing.T) { + tmpDir := t.TempDir() + + lockContent := `# gh-aw-metadata: {"schema_version":"v3","frontmatter_hash":"abc","agent_id":"claude"} +name: test +` + lockFilePath := filepath.Join(tmpDir, "my-workflow.lock.yml") + require.NoError(t, os.WriteFile(lockFilePath, []byte(lockContent), 0644)) + + yamlPath := strings.TrimSuffix(lockFilePath, ".lock.yml") + ".yaml" + got := extractEngineIDFromLockFile(yamlPath, false) + assert.Equal(t, "claude", got) +} diff --git a/pkg/cli/logs_models.go b/pkg/cli/logs_models.go index 7ef8093394f..76eb3dfe24b 100644 --- a/pkg/cli/logs_models.go +++ b/pkg/cli/logs_models.go @@ -94,6 +94,7 @@ type ToolCallInfo = workflow.ToolCallInfo // ProcessedRun represents a workflow run with its associated analysis type ProcessedRun struct { Run WorkflowRun + LockFileEngineID string AwContext *AwContext TaskDomain *TaskDomainInfo BehaviorFingerprint *BehaviorFingerprint diff --git a/pkg/cli/logs_orchestrator.go b/pkg/cli/logs_orchestrator.go index 1d3742eb61d..513eca22ae3 100644 --- a/pkg/cli/logs_orchestrator.go +++ b/pkg/cli/logs_orchestrator.go @@ -96,14 +96,22 @@ func getMaxConcurrentDownloads() int { return envutil.GetIntFromEnv("GH_AW_MAX_CONCURRENT_DOWNLOADS", MaxConcurrentDownloads, 1, 100, logsOrchestratorLog) } -// matchEngineFilter checks whether the run recorded in awInfo matches the -// requested engine filter string. It returns (matches, detectedEngineID). -// detectedEngineID is "" when awInfo is unavailable or carries no engine_id. -func matchEngineFilter(awInfo *AwInfo, awInfoErr error, filterEngine string) (bool, string) { - if awInfoErr != nil || awInfo == nil || awInfo.EngineID == "" { +// matchEngineFilter checks whether the run matches the requested engine filter string. +// It uses the engine_id from awInfo (authoritative) or falls back to lockFileEngineID +// (derived from the lock file's gh-aw-metadata) when aw_info.json is absent or carries +// no engine_id. Returns (matches, detectedEngineID). +// detectedEngineID is "" when the engine is unknown from both sources. +func matchEngineFilter(awInfo *AwInfo, awInfoErr error, filterEngine string, lockFileEngineID string) (bool, string) { + engineID := "" + if awInfoErr == nil && awInfo != nil && awInfo.EngineID != "" { + engineID = awInfo.EngineID + } else if lockFileEngineID != "" { + engineID = lockFileEngineID + } + if engineID == "" { return false, "" } - return awInfo.EngineID == filterEngine, awInfo.EngineID + return engineID == filterEngine, engineID } type LogsDownloadOptions struct { @@ -302,6 +310,7 @@ func DownloadWorkflowLogs(ctx context.Context, opts LogsDownloadOptions) error { } var processedRuns []ProcessedRun + lockFileEngineIDs := make(map[string]string) var beforeDate string iteration := 0 @@ -507,9 +516,19 @@ outerLoop: awInfo, awInfoErr = parseAwInfo(awInfoPath, verbose) } + lockFileEngineID := "" + if awInfoErr != nil || awInfo == nil || awInfo.EngineID == "" { + if cachedEngineID, ok := lockFileEngineIDs[result.Run.WorkflowPath]; ok { + lockFileEngineID = cachedEngineID + } else { + lockFileEngineID = extractEngineIDFromLockFile(result.Run.WorkflowPath, verbose) + lockFileEngineIDs[result.Run.WorkflowPath] = lockFileEngineID + } + } + // Apply engine filtering if specified if engine != "" { - engineMatches, detectedEngineID := matchEngineFilter(awInfo, awInfoErr, engine) + engineMatches, detectedEngineID := matchEngineFilter(awInfo, awInfoErr, engine, lockFileEngineID) if !engineMatches { if detectedEngineID == "" { detectedEngineID = "unknown" @@ -627,6 +646,7 @@ outerLoop: processedRun := ProcessedRun{ Run: run, + LockFileEngineID: lockFileEngineID, AwContext: result.AwContext, TaskDomain: result.TaskDomain, BehaviorFingerprint: result.BehaviorFingerprint, @@ -730,7 +750,7 @@ outerLoop: // When JSON output is requested, output JSON first to stdout before any stderr messages // This prevents stderr messages from corrupting JSON when both streams are redirected together if jsonOutput { - logsData := buildLogsData([]ProcessedRun{}, outputDir, nil) + logsData := buildLogsData([]ProcessedRun{}, outputDir, nil, verbose) logsData.Message = noRunsMessage(startDate, timeoutReached) if err := renderLogsJSON(logsData, verbose); err != nil { return fmt.Errorf("failed to render JSON output: %w", err) @@ -807,7 +827,7 @@ func renderLogsOutput(processedRuns []ProcessedRun, opts renderLogsOutputOptions // Build structured logs data logsOrchestratorLog.Printf("Building logs data from %d processed runs (continuation=%t)", len(processedRuns), opts.continuation != nil) - logsData := buildLogsData(processedRuns, opts.outputDir, opts.continuation) + logsData := buildLogsData(processedRuns, opts.outputDir, opts.continuation, opts.verbose) // When only the usage artifact was downloaded, add a hint so consumers know how // to fetch additional artifact sets (agent logs, firewall data, etc.). @@ -1080,7 +1100,7 @@ func DownloadWorkflowLogsFromStdin(ctx context.Context, opts StdinLogsOptions) e if len(runs) == 0 { if opts.JSONOutput { - logsData := buildLogsData([]ProcessedRun{}, opts.OutputDir, nil) + logsData := buildLogsData([]ProcessedRun{}, opts.OutputDir, nil, opts.Verbose) logsData.Message = "No runs found. No valid runs could be loaded from the provided input." if err := renderLogsJSON(logsData, opts.Verbose); err != nil { return fmt.Errorf("failed to render JSON output: %w", err) @@ -1095,6 +1115,7 @@ func DownloadWorkflowLogsFromStdin(ctx context.Context, opts StdinLogsOptions) e // Process download results applying the same filters as DownloadWorkflowLogs var processedRuns []ProcessedRun + lockFileEngineIDs := make(map[string]string) for _, result := range downloadResults { if result.Skipped { if opts.Verbose && result.Error != nil { @@ -1115,8 +1136,18 @@ func DownloadWorkflowLogsFromStdin(ctx context.Context, opts StdinLogsOptions) e awInfo, awInfoErr = parseAwInfo(awInfoPath, opts.Verbose) } + lockFileEngineID := "" + if awInfoErr != nil || awInfo == nil || awInfo.EngineID == "" { + if cachedEngineID, ok := lockFileEngineIDs[result.Run.WorkflowPath]; ok { + lockFileEngineID = cachedEngineID + } else { + lockFileEngineID = extractEngineIDFromLockFile(result.Run.WorkflowPath, opts.Verbose) + lockFileEngineIDs[result.Run.WorkflowPath] = lockFileEngineID + } + } + if opts.Engine != "" { - engineMatches, detectedEngineID := matchEngineFilter(awInfo, awInfoErr, opts.Engine) + engineMatches, detectedEngineID := matchEngineFilter(awInfo, awInfoErr, opts.Engine, lockFileEngineID) if !engineMatches { if detectedEngineID == "" { detectedEngineID = "unknown" @@ -1213,6 +1244,7 @@ func DownloadWorkflowLogsFromStdin(ctx context.Context, opts StdinLogsOptions) e processedRun := ProcessedRun{ Run: run, + LockFileEngineID: lockFileEngineID, AwContext: result.AwContext, TaskDomain: result.TaskDomain, BehaviorFingerprint: result.BehaviorFingerprint, @@ -1254,7 +1286,7 @@ func DownloadWorkflowLogsFromStdin(ctx context.Context, opts StdinLogsOptions) e if len(processedRuns) == 0 { if opts.JSONOutput { - logsData := buildLogsData([]ProcessedRun{}, opts.OutputDir, nil) + logsData := buildLogsData([]ProcessedRun{}, opts.OutputDir, nil, opts.Verbose) logsData.Message = "No runs found matching the specified criteria." if err := renderLogsJSON(logsData, opts.Verbose); err != nil { return fmt.Errorf("failed to render JSON output: %w", err) diff --git a/pkg/cli/logs_parsing_core.go b/pkg/cli/logs_parsing_core.go index da326106fa9..f12b123c435 100644 --- a/pkg/cli/logs_parsing_core.go +++ b/pkg/cli/logs_parsing_core.go @@ -4,6 +4,7 @@ // // Key responsibilities: // - Parsing aw_info.json to extract engine configuration +// - Extracting engine ID from lock file gh-aw-metadata as a fallback // - Registering the errWalkStop sentinel used by walk-based helpers in this package package cli @@ -14,13 +15,17 @@ import ( "fmt" "os" "path/filepath" + "strings" + "sync" "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/gitutil" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/workflow" ) var logsParsingCoreLog = logger.New("cli:logs_parsing_core") +var lockFileEngineIDCache sync.Map // errWalkStop is a sentinel returned from filepath.Walk callbacks to stop traversal early. // It is shared across all walk-based file-search functions in this package. @@ -107,3 +112,88 @@ func extractEngineFromAwInfo(infoFilePath string, verbose bool) workflow.CodingA logsParsingCoreLog.Printf("Successfully extracted engine: %s", engine.GetID()) return engine } + +// extractEngineIDFromLockFile reads the local lock file for the given workflow and +// returns the agent_id embedded in its gh-aw-metadata comment. This provides a +// fallback engine source when aw_info.json is absent (e.g. for older runs that +// pre-date the aw_info.json artifact). +// +// workflowPath is the workflow file path as returned by the GitHub API (e.g. +// ".github/workflows/daily-cli-tools-tester.lock.yml"). Plain ".yml" paths are +// also accepted and are automatically resolved to the ".lock.yml" variant. +// Returns "" when the path is empty, the lock file cannot be read, or no +// agent_id is present in the metadata. +func extractEngineIDFromLockFile(workflowPath string, verbose bool) string { + if workflowPath == "" { + return "" + } + + lockPath := resolveToLockFilePath(workflowPath) + if lockPath == "" { + logsParsingCoreLog.Printf("Cannot resolve lock file path from: %s", workflowPath) + return "" + } + + if !filepath.IsAbs(lockPath) { + repoRoot, err := gitutil.FindGitRoot() + if err == nil { + lockPath = filepath.Join(repoRoot, filepath.FromSlash(lockPath)) + } + } + + lockPath = filepath.Clean(lockPath) + if cachedEngineID, ok := lockFileEngineIDCache.Load(lockPath); ok { + engineID, _ := cachedEngineID.(string) + if engineID != "" { + logsParsingCoreLog.Printf("Using cached engine_id=%s from lock file %s", engineID, lockPath) + if verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Detected engine from lock file metadata: "+engineID)) + } + } + return engineID + } + + content, err := os.ReadFile(lockPath) + if err != nil { + lockFileEngineIDCache.Store(lockPath, "") + cwd, cwdErr := os.Getwd() + if cwdErr != nil { + cwd = "" + } + logsParsingCoreLog.Printf("Cannot read lock file %s (cwd: %s): %v", lockPath, cwd, err) + return "" + } + + metadata, _, err := workflow.ExtractMetadataFromLockFile(string(content)) + if err != nil || metadata == nil || metadata.AgentID == "" { + lockFileEngineIDCache.Store(lockPath, "") + logsParsingCoreLog.Printf("No agent_id in lock file metadata: %s", lockPath) + return "" + } + + lockFileEngineIDCache.Store(lockPath, metadata.AgentID) + logsParsingCoreLog.Printf("Extracted engine_id=%s from lock file %s", metadata.AgentID, lockPath) + if verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Detected engine from lock file metadata: "+metadata.AgentID)) + } + return metadata.AgentID +} + +// resolveToLockFilePath converts a workflow file path to its lock file path. +// Handles .lock.yml paths (returned as-is), plain .yml paths (converted by +// appending ".lock" before the ".yml" suffix), and .md source paths. +// Returns "" for unrecognised extensions. +func resolveToLockFilePath(workflowPath string) string { + switch { + case strings.HasSuffix(workflowPath, ".lock.yml"): + return workflowPath + case strings.HasSuffix(workflowPath, ".md"): + return strings.TrimSuffix(workflowPath, ".md") + ".lock.yml" + case strings.HasSuffix(workflowPath, ".yml"): + return strings.TrimSuffix(workflowPath, ".yml") + ".lock.yml" + case strings.HasSuffix(workflowPath, ".yaml"): + return strings.TrimSuffix(workflowPath, ".yaml") + ".lock.yml" + default: + return "" + } +} diff --git a/pkg/cli/logs_report.go b/pkg/cli/logs_report.go index 98aabc5926b..fcab01dbb8c 100644 --- a/pkg/cli/logs_report.go +++ b/pkg/cli/logs_report.go @@ -152,8 +152,9 @@ type RunData struct { } // buildLogsData creates structured logs data from processed runs -func buildLogsData(processedRuns []ProcessedRun, outputDir string, continuation *ContinuationData) LogsData { +func buildLogsData(processedRuns []ProcessedRun, outputDir string, continuation *ContinuationData, verboseFlags ...bool) LogsData { reportLog.Printf("Building logs data from %d processed runs", len(processedRuns)) + verbose := len(verboseFlags) > 0 && verboseFlags[0] // Build summary var totalDuration time.Duration @@ -243,13 +244,21 @@ func buildLogsData(processedRuns []ProcessedRun, outputDir string, continuation engineName = info.EngineName awContext = info.Context } + // Fall back to the lock file's gh-aw-metadata when aw_info.json is absent + // or does not carry an engine_id (e.g. older runs that pre-date the artifact). + if engineID == "" { + engineID = pr.LockFileEngineID + } + if engineID == "" { + engineID = extractEngineIDFromLockFile(run.WorkflowPath, verbose) + } if engineName == "" { engineName = engineID } if awContext == nil { awContext = pr.AwContext } - // Accumulate engine counts from aw_info.json data (authoritative source). + // Accumulate engine counts from aw_info.json / lock file metadata. if engineID != "" { engineCounts[engineID]++ } diff --git a/pkg/cli/logs_report_test.go b/pkg/cli/logs_report_test.go index 18f4c4fc657..da9af7fe593 100644 --- a/pkg/cli/logs_report_test.go +++ b/pkg/cli/logs_report_test.go @@ -10,6 +10,8 @@ import ( "github.com/github/gh-aw/pkg/setutil" "github.com/github/gh-aw/pkg/sliceutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) // TestRenderLogsConsoleUnified tests the unified console rendering @@ -991,6 +993,87 @@ func TestBuildLogsDataEngineCountsFromAwInfo(t *testing.T) { } } +// TestBuildLogsDataEngineCountsFromLockFileFallback verifies that engine_counts and +// RunData.Agent are populated from the lock file's gh-aw-metadata when aw_info.json +// is absent (i.e. for older runs that pre-date the aw_info.json artifact). +func TestBuildLogsDataEngineCountsFromLockFileFallback(t *testing.T) { + // Create a run dir WITHOUT aw_info.json to simulate an older run. + createRunDirNoAwInfo := func() string { + return t.TempDir() + } + + // Write a lock file containing gh-aw-metadata with agent_id. + createLockFile := func(engineID string) string { + dir := t.TempDir() + lockContent := `# gh-aw-metadata: {"schema_version":"v3","frontmatter_hash":"abc","agent_id":"` + engineID + `"} +name: test-workflow +` + lockPath := filepath.Join(dir, "test-workflow.lock.yml") + if err := os.WriteFile(lockPath, []byte(lockContent), 0600); err != nil { + t.Fatalf("Failed to write lock file: %v", err) + } + return lockPath + } + + copilotLockPath := createLockFile("copilot") + claudeLockPath := createLockFile("claude") + + copilotRunDir := createRunDirNoAwInfo() + claudeRunDir := createRunDirNoAwInfo() + + processedRuns := []ProcessedRun{ + {Run: WorkflowRun{DatabaseID: 1, WorkflowName: "wf-copilot", LogsPath: copilotRunDir, WorkflowPath: copilotLockPath}}, + {Run: WorkflowRun{DatabaseID: 2, WorkflowName: "wf-claude", LogsPath: claudeRunDir, WorkflowPath: claudeLockPath}}, + } + + data := buildLogsData(processedRuns, "/tmp/logs", nil) + + if data.Summary.EngineCounts == nil { + t.Fatal("EngineCounts should not be nil when runs have lock file metadata") + } + if got := data.Summary.EngineCounts["copilot"]; got != 1 { + t.Errorf("Expected 1 copilot run from lock file fallback, got %d", got) + } + if got := data.Summary.EngineCounts["claude"]; got != 1 { + t.Errorf("Expected 1 claude run from lock file fallback, got %d", got) + } + + agentsByID := make(map[int64]string) + for _, run := range data.Runs { + agentsByID[run.RunID] = run.Agent + } + if agentsByID[1] != "copilot" { + t.Errorf("Run 1: expected agent=copilot from lock file fallback, got %q", agentsByID[1]) + } + if agentsByID[2] != "claude" { + t.Errorf("Run 2: expected agent=claude from lock file fallback, got %q", agentsByID[2]) + } +} + +func TestBuildLogsDataAwInfoTakesPrecedenceOverLockFile(t *testing.T) { + dir := t.TempDir() + awInfo := `{"engine_id":"claude","engine_name":"Claude","workflow_name":"test","created_at":"2024-01-01T00:00:00Z"}` + require.NoError(t, os.WriteFile(filepath.Join(dir, "aw_info.json"), []byte(awInfo), 0600)) + + lockPath := filepath.Join(dir, "wf.lock.yml") + lockContent := `# gh-aw-metadata: {"schema_version":"v3","frontmatter_hash":"x","agent_id":"copilot"} +name: wf +` + require.NoError(t, os.WriteFile(lockPath, []byte(lockContent), 0600)) + + processedRuns := []ProcessedRun{ + {Run: WorkflowRun{DatabaseID: 1, WorkflowName: "wf", LogsPath: dir, WorkflowPath: lockPath}}, + } + + data := buildLogsData(processedRuns, "/tmp/logs", nil) + + require.Len(t, data.Runs, 1) + assert.Equal(t, "claude", data.Runs[0].EngineID) + assert.Equal(t, "Claude", data.Runs[0].Engine) + assert.Equal(t, 1, data.Summary.EngineCounts["claude"]) + assert.Equal(t, 0, data.Summary.EngineCounts["copilot"]) +} + func TestBuildLogsDataAggregatesSteeringEvents(t *testing.T) { processedRuns := []ProcessedRun{ {