fix: populate engine_id from lock file metadata when aw_info.json is absent

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.*

pkg/cli/logs_engine_filter_test.go

@@ -5,20 +5,21 @@ package cli
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
 // 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)
+}

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

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)