fix: populate engine_id from lock file metadata when aw_info.json is absent#43732
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…on is absent Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This PR fixes gh aw logs --engine <id> filtering and JSON reporting for historical workflow runs that pre-date the aw_info.json artifact by falling back to the engine identifier stored in the workflow lock file’s # gh-aw-metadata: comment.
Changes:
- Add lock-file metadata parsing (
agent_id→ engine id) as a fallback whenaw_info.jsonis missing or has an emptyengine_id. - Update engine filtering to accept a lock-file-derived engine id fallback.
- Add/extend tests covering lock-file fallback behavior for both filtering and report aggregation.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs_parsing_core.go | Adds extractEngineIDFromLockFile and lock-file path normalization helper. |
| pkg/cli/logs_orchestrator.go | Extends engine filtering logic to support lock-file fallback. |
| pkg/cli/logs_report.go | Falls back to lock-file metadata when building JSON report engine fields/counts. |
| pkg/cli/logs_report_test.go | Adds report test coverage for lock-file fallback when aw_info.json is absent. |
| pkg/cli/logs_engine_filter_test.go | Adds tests for lock-file engine-id extraction and filter precedence. |
| .github/workflows/workflow-skill-extractor.lock.yml | Generated lockfile formatting update. |
| .github/workflows/workflow-health-manager.lock.yml | Generated lockfile formatting update. |
| .github/workflows/weekly-blog-post-writer.lock.yml | Generated lockfile formatting update. |
| .github/workflows/unbloat-docs.lock.yml | Generated lockfile formatting update. |
| .github/workflows/uk-ai-operational-resilience.lock.yml | Generated lockfile formatting update. |
| .github/workflows/test-quality-sentinel.lock.yml | Generated lockfile formatting update. |
| .github/workflows/step-name-alignment.lock.yml | Generated lockfile formatting update. |
| .github/workflows/static-analysis-report.lock.yml | Generated lockfile formatting update. |
| .github/workflows/stale-repo-identifier.lock.yml | Generated lockfile formatting update. |
| .github/workflows/spec-extractor.lock.yml | Generated lockfile formatting update. |
| .github/workflows/schema-consistency-checker.lock.yml | Generated lockfile formatting update. |
| .github/workflows/repository-quality-improver.lock.yml | Generated lockfile formatting update. |
| .github/workflows/release.lock.yml | Generated lockfile formatting update. |
| .github/workflows/prompt-clustering-analysis.lock.yml | Generated lockfile formatting update. |
| .github/workflows/pr-description-caveman.lock.yml | Generated lockfile formatting update. |
| .github/workflows/objective-impact-report.lock.yml | Generated lockfile formatting update. |
| .github/workflows/mergefest.lock.yml | Generated lockfile formatting update. |
| .github/workflows/mattpocock-skills-reviewer.lock.yml | Generated lockfile formatting update. |
| .github/workflows/linter-miner.lock.yml | Generated lockfile formatting update. |
| .github/workflows/lint-monster.lock.yml | Generated lockfile formatting update. |
| .github/workflows/issue-arborist.lock.yml | Generated lockfile formatting update. |
| .github/workflows/impeccable-skills-reviewer.lock.yml | Generated lockfile formatting update. |
| .github/workflows/gpclean.lock.yml | Generated lockfile formatting update. |
| .github/workflows/glossary-maintainer.lock.yml | Generated lockfile formatting update. |
| .github/workflows/eslint-monster.lock.yml | Generated lockfile formatting update. |
| .github/workflows/docs-noob-tester.lock.yml | Generated lockfile formatting update. |
| .github/workflows/designer-drift-audit.lock.yml | Generated lockfile formatting update. |
| .github/workflows/design-decision-gate.lock.yml | Generated lockfile formatting update. |
| .github/workflows/dependabot-burner.lock.yml | Generated lockfile formatting update. |
| .github/workflows/delight.lock.yml | Generated lockfile formatting update. |
| .github/workflows/dataflow-pr-discussion-dataset.lock.yml | Generated lockfile formatting update. |
| .github/workflows/daily-spdd-spec-planner.lock.yml | Generated lockfile formatting update. |
| .github/workflows/daily-security-observability.lock.yml | Generated lockfile formatting update. |
| .github/workflows/daily-multi-device-docs-tester.lock.yml | Generated lockfile formatting update. |
| .github/workflows/daily-community-attribution.lock.yml | Generated lockfile formatting update. |
| .github/workflows/daily-ambient-context-optimizer.lock.yml | Generated lockfile formatting update. |
| .github/workflows/copilot-pr-nlp-analysis.lock.yml | Generated lockfile formatting update. |
| .github/workflows/copilot-centralization-drilldown.lock.yml | Generated lockfile formatting update. |
| .github/workflows/contribution-check.lock.yml | Generated lockfile formatting update. |
| .github/workflows/code-simplifier.lock.yml | Generated lockfile formatting update. |
| .github/workflows/cli-consistency-checker.lock.yml | Generated lockfile formatting update. |
| .github/workflows/ci-doctor.lock.yml | Generated lockfile formatting update. |
| .github/workflows/aw-failure-investigator.lock.yml | Generated lockfile formatting update. |
| .github/workflows/auto-triage-issues.lock.yml | Generated lockfile formatting update. |
| .github/workflows/architecture-guardian.lock.yml | Generated lockfile formatting update. |
| .github/workflows/agentic-token-trend-audit.lock.yml | Generated lockfile formatting update. |
| .github/workflows/agentic-token-optimizer.lock.yml | Generated lockfile formatting update. |
| .github/workflows/agentic-token-audit.lock.yml | Generated lockfile formatting update. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 53/53 changed files
- Comments generated: 3
- Review effort level: Low
| lockFileEngineID := extractEngineIDFromLockFile(result.Run.WorkflowPath, verbose) | ||
| engineMatches, detectedEngineID := matchEngineFilter(awInfo, awInfoErr, engine, lockFileEngineID) |
| lockFileEngineID := extractEngineIDFromLockFile(result.Run.WorkflowPath, opts.Verbose) | ||
| engineMatches, detectedEngineID := matchEngineFilter(awInfo, awInfoErr, opts.Engine, lockFileEngineID) |
| GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| name: Build Inventory | ||
| run: "set -euo pipefail\nmkdir -p /tmp/gh-aw/agent\n# Run compilation validation and capture output\ngh aw compile --validate > /tmp/gh-aw/agent/compile-validate.txt 2>&1 || true\n# List executable workflow files (exclude shared/ subdirectory)\nls .github/workflows/*.md 2>/dev/null > /tmp/gh-aw/agent/workflow-list.txt || true\necho \"Inventory complete: $(wc -l < /tmp/gh-aw/agent/workflow-list.txt | tr -d ' ') workflows found\"\n" | ||
| run: "set -euo pipefail\nmkdir -p /tmp/gh-aw/agent\n# Run compilation validation and capture output\ngh aw compile --validate > /tmp/gh-aw/agent/compile-validate.txt 2>&1 || true\n# List executable workflow files (exclude shared/ subdirectory)\nls .github/workflows/*.md 2>/dev/null > /tmp/gh-aw/agent/workflow-list.txt || true\necho \"Inventory complete: $(wc -l < /tmp/gh-aw/agent/workflow-list.txt | tr -d ' ') workflows found\"" |
ADR documents the two-tier precedence decision: aw_info.json is authoritative for engine_id resolution; the lock file gh-aw-metadata comment serves as fallback for pre-aw_info.json runs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (284 new lines in business logic directories) but does not have a linked Architecture Decision Record (ADR). 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report🔶 Test Quality Score: 68/100 — Acceptable (60-79 range)
📊 Metrics (18 tests)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Test Quality Sentinel: 68/100. Implementation tests comprise 38.9% of new tests (threshold: 30%). Core design coverage is strong (11/18 = 61%), but TestResolveToLockFilePath (7 utility-focused path normalization cases) counts against threshold. This test can be consolidated into TestExtractEngineIDFromLockFilePlainYml which already verifies path resolution end-to-end. See detailed analysis in the comment above.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on 5 issues: two correctness/observability gaps and three test quality issues.
📋 Key Themes & Highlights
Key Issues
-
CWD assumption (
logs_parsing_core.go:134) —os.ReadFileis given a relative path straight from the GitHub API. The fallback silently returns""when the process isn't run from the repo root, which means the original bug reappears in any non-standard invocation. No guard or diagnostic surfaced. -
Double lock-file I/O (
logs_report.go:249) — When--engineis active, each run reads its lock file twice: once in the filter loop and again inbuildLogsData. Propagating the already-computed value viaProcessedRunwould eliminate the second read entirely. -
verbosehardcoded tofalse(logs_report.go:249) — Inconsistent with both orchestrator call sites, which correctly pass the caller's verbose flag. The diagnostic"Detected engine from lock file metadata: ..."is silenced for this path. -
Missing precedence regression test (
logs_report_test.go:991) —TestBuildLogsDataEngineCountsFromLockFileFallbackhas no case that verifiesaw_info.jsonbeats the lock file when both are present with differingagent_id. The precedence rule could break silently. -
Fragile name-string sentinel in test (
logs_engine_filter_test.go:170) —tc.name != "empty workflow path returns empty"is a brittle guard; a struct flag likeskipFileCreate boolis refactor-safe.
Positive Highlights
- ✅ Precedence logic in
matchEngineFilteris clean and correct:aw_info.jsonwins when present, lock file is a true fallback. - ✅
resolveToLockFilePathswitch ordering correctly handles.lock.ymlbefore the plain.ymlcase. - ✅ Good coverage added for
matchEngineFilterfallback scenarios andresolveToLockFilePathpath-normalisation. - ✅ PR description is clear about the precedence hierarchy and the affected workflow (
daily-cli-tools-tester).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 89.1 AIC · ⌖ 5.84 AIC · ⊞ 6.7K
Comment /matt to run again
|
|
||
| // Use .lock.yml extension by default | ||
| workflowPath := tc.workflowPath | ||
| if workflowPath == "" && tc.name != "empty workflow path returns empty" { |
There was a problem hiding this comment.
[/tdd] Test control flow uses tc.name != "empty workflow path returns empty" as a string-match sentinel — this is fragile: a typo or rename silently breaks the guard without a compile error.
💡 Suggested fix
Add a skipFileCreate bool field to the test case struct:
cases := []struct {
name string
lockFileContent string
workflowPath string
skipFileCreate bool // pass workflowPath directly without creating a file
expectEngineID string
}{
...
{
name: "empty workflow path returns empty",
workflowPath: "",
skipFileCreate: true,
expectEngineID: "",
},
}Then the guard becomes:
if !tc.skipFileCreate {
// create lock file and set workflowPath
}This makes intent explicit and is refactor-safe.
@copilot please address this.
| return "" | ||
| } | ||
|
|
||
| content, err := os.ReadFile(filepath.Clean(lockPath)) |
There was a problem hiding this comment.
[/diagnosing-bugs] os.ReadFile(filepath.Clean(lockPath)) is called with a path that comes directly from the GitHub API — a relative path like .github/workflows/foo.lock.yml. This works only if the process CWD is the repository root, but there is no guard or assertion to enforce that assumption. If the tool is run from a different directory, the fallback silently returns "" (the lock file is unreadable), and the bug reappears without any error surfaced to the user.
💡 Suggested fix
Consider anchoring the path explicitly. If a repo-root concept exists in context, prefix lockPath with it:
if !filepath.IsAbs(lockPath) {
lockPath = filepath.Join(repoRoot, lockPath)
}Or at minimum, add a verbose log that reveals the resolved path when the file is missing, so operators can diagnose the mismatch:
if err != nil {
logsParsingCoreLog.Printf("Cannot read lock file %s (cwd: %s): %v", lockPath, cwdForLog(), err)
return ""
}This makes silent CWD-mismatch failures diagnosable.
@copilot please address this.
| // 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 = extractEngineIDFromLockFile(run.WorkflowPath, false) |
There was a problem hiding this comment.
[/diagnosing-bugs] When --engine is active, extractEngineIDFromLockFile is called twice per run: once in the filter loop (logs_orchestrator.go:520) and again here for report population. On a 200-run download this doubles the lock-file I/O, and the second call is entirely avoidable since the value was already computed (and the run already passed the filter).
💡 Suggested fix
Propagate the already-computed lockFileEngineID from the orchestrator into ProcessedRun (or pass it through buildLogsData), then skip the second read:
// In ProcessedRun or a new field:
LockFileEngineID string
// In orchestrator, before appending:
result.LockFileEngineID = lockFileEngineID
// In buildLogsData:
if engineID == "" {
engineID = pr.LockFileEngineID // already computed, no I/O
}Alternatively, a simple sync.Map-based per-path cache in extractEngineIDFromLockFile would also avoid repeated disk reads.
@copilot please address this.
| @@ -991,6 +991,63 @@ func TestBuildLogsDataEngineCountsFromAwInfo(t *testing.T) { | |||
| } | |||
There was a problem hiding this comment.
[/tdd] TestBuildLogsDataEngineCountsFromLockFileFallback covers the happy-path lock-file read but is missing a case that verifies aw_info.json takes precedence: when both a valid aw_info.json and a lock file with a different agent_id are present, the report's engine_id should reflect aw_info.json, not the lock file. Without this regression test, the precedence rule could silently break.
💡 Suggested test case
func TestBuildLogsDataAwInfoTakesPrecedenceOverLockFile(t *testing.T) {
dir := t.TempDir()
// aw_info.json says "claude"
awInfoContent := `{"engine_id":"claude"}`
require.NoError(t, os.WriteFile(filepath.Join(dir, "aw_info.json"), []byte(awInfoContent), 0600))
// lock file says "copilot"
lockContent := `# gh-aw-metadata: {"schema_version":"v3","frontmatter_hash":"x","agent_id":"copilot"}\nname: wf\n`
lockPath := filepath.Join(dir, "wf.lock.yml")
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)
assert.Equal(t, 1, data.Summary.EngineCounts["claude"], "aw_info should win")
assert.Equal(t, 0, data.Summary.EngineCounts["copilot"], "lock file should be ignored")
}@copilot please address this.
| // 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 = extractEngineIDFromLockFile(run.WorkflowPath, false) |
There was a problem hiding this comment.
[/diagnosing-bugs] extractEngineIDFromLockFile is called with a hardcoded false for verbose, unlike the two call sites in logs_orchestrator.go which correctly pass the caller's verbose flag. This means the "Detected engine from lock file metadata: ..." diagnostic message is never shown in buildLogsData's path, even when the user passes --verbose.
💡 Suggested fix
buildLogsData doesn't currently accept a verbose parameter, so the simplest fix is to add one and thread it through:
func buildLogsData(processedRuns []ProcessedRun, outputDir string, continuation *ContinuationData, verbose bool) LogsData {
...
engineID = extractEngineIDFromLockFile(run.WorkflowPath, verbose)
}Update the three call sites accordingly.
@copilot please address this.
There was a problem hiding this comment.
Review: fix: populate engine_id from lock file metadata when aw_info.json is absent
Summary: The fix is correct and well-tested. The fallback precedence (aw_info.json authoritative, lock file as fallback) is sound, and the new helper functions (extractEngineIDFromLockFile, resolveToLockFilePath) are clean and well-documented. Test coverage for the new paths is thorough.
One non-blocking issue found: In logs_orchestrator.go, extractEngineIDFromLockFile is called unconditionally on every run even when awInfo.EngineID is already set — triggering an unnecessary disk read in the common case. logs_report.go already uses the correct lazy pattern (if engineID == "" { ... }). The same guard should be applied at both call sites in the orchestrator.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 81.9 AIC · ⌖ 5.92 AIC · ⊞ 4.9K
| // Apply engine filtering if specified | ||
| if engine != "" { | ||
| engineMatches, detectedEngineID := matchEngineFilter(awInfo, awInfoErr, engine) | ||
| lockFileEngineID := extractEngineIDFromLockFile(result.Run.WorkflowPath, verbose) |
There was a problem hiding this comment.
The lock file is read unconditionally here — even when awInfo already carries a valid engine_id. This wastes a disk read per run in the common case. logs_report.go uses the correct lazy pattern:
if engineID == "" {
engineID = extractEngineIDFromLockFile(run.WorkflowPath, false)
}Same fix applies to line 1128. Consider only calling extractEngineIDFromLockFile when awInfo has no engine_id:
var lockFileEngineID string
if awInfoErr != nil || awInfo == nil || awInfo.EngineID == "" {
lockFileEngineID = extractEngineIDFromLockFile(result.Run.WorkflowPath, verbose)
}@copilot please address this.
|
@copilot please run the
|
There was a problem hiding this comment.
REQUEST_CHANGES — The fix is correct in intent but has three issues that need addressing before merge.
🔍 Blocking findings (3)
1. N+1 lock file reads across the pipeline
extractEngineIDFromLockFile is called independently at lines 520, 1128, and logs_report.go:249. For every run that passes the engine filter its lock file is read from disk at minimum three times. For a batch of N runs sharing the same workflow this is O(N) redundant disk reads. A map[string]string cache keyed on workflowPath would eliminate all redundant reads. See inline comment at line 520.
2. Filter and report can disagree on the engine ID
The orchestrator resolves and discards the lock-file engine ID after the filter decision. buildLogsData then independently re-reads the lock file. If the file is replaced between the two phases, --engine filtering and RunData.Agent/EngineCounts in the JSON report will silently reflect different values. The computed ID should be threaded through ProcessedRun rather than re-derived. See inline comment at logs_report.go:249.
3. .yaml extension silently returns empty — fallback never fires
resolveToLockFilePath has no case for .yaml. Any workflow file with that extension causes extractEngineIDFromLockFile to return "", silently defeating the entire fallback. See inline comment at logs_parsing_core.go:157.
⚠️ Non-blocking findings (2)
verbose=false hardcoded in report path
Orchestrator call sites forward the runtime verbose flag; the report call always passes false. Users running --verbose get incomplete engine-detection output.
Test sentinel using test name string
tc.name != "empty workflow path returns empty" is a magic string gate that silently breaks if the test case is renamed. Replace with an explicit skipFileCreate bool field.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 167.2 AIC · ⌖ 5.64 AIC · ⊞ 5.4K
Comment /review to run again
| // Apply engine filtering if specified | ||
| if engine != "" { | ||
| engineMatches, detectedEngineID := matchEngineFilter(awInfo, awInfoErr, engine) | ||
| lockFileEngineID := extractEngineIDFromLockFile(result.Run.WorkflowPath, verbose) |
There was a problem hiding this comment.
N+1 lock file reads in hot loop: the same lock file is read from disk on every run iteration.
💡 Details and suggested fix
extractEngineIDFromLockFile performs os.ReadFile + metadata parse on every loop iteration. For N runs sharing the same workflow path this is N redundant disk reads with no memoization. The same problem exists at the second call site (line 1128) and in buildLogsData (logs_report.go:249), meaning a run that passes the engine filter has its lock file read at minimum three times across the processing pipeline.
Suggested fix: introduce a map[string]string cache keyed on workflowPath and pass it through both call sites (or hoist it into a dedicated helper struct):
lockFileIDCache := make(map[string]string)
// inside loop:
if _, seen := lockFileIDCache[result.Run.WorkflowPath]; !seen {
lockFileIDCache[result.Run.WorkflowPath] = extractEngineIDFromLockFile(result.Run.WorkflowPath, verbose)
}
lockFileEngineID := lockFileIDCache[result.Run.WorkflowPath]This also eliminates the consistency hazard where filter and report phases could disagree if the lock file is modified between reads.
| // 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 = extractEngineIDFromLockFile(run.WorkflowPath, false) |
There was a problem hiding this comment.
Engine ID recomputed from disk in report phase instead of reusing value already resolved during filtering — filter and report can disagree.
💡 Details
The filtering code in logs_orchestrator.go (lines 520, 1128) already computes the lock-file engine ID and makes a pass/fail decision. That resolved value is then thrown away. buildLogsData independently re-reads the same lock file a third time. Two consequences:
- I/O waste — redundant disk read per run (see the N+1 comment at the filter call site).
- Consistency gap — if the lock file on disk is replaced between the filter phase and the report phase, the engine ID used to decide which runs pass the filter will differ from the engine ID recorded in
RunData.Agent/EngineCounts. This is unlikely in practice but makes the code brittle and harder to reason about.
The correct fix is to thread the already-computed engine ID through ProcessedRun (or pass it as a field on WorkflowRun) so buildLogsData consumes the pre-resolved value rather than re-reading the file.
| // 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 { |
There was a problem hiding this comment.
.yaml extension silently returns "", making the fallback silently fail for any GitHub Actions workflow named with .yaml.
💡 Details and fix
GitHub Actions accepts both .yml and .yaml as valid workflow file extensions. resolveToLockFilePath handles .yml but has no case for .yaml, so any WorkflowPath ending in .yaml falls through to default: return "". extractEngineIDFromLockFile then silently returns "" — the engine ID is never populated for those workflows, and --engine filter silently excludes all their runs.
Fix:
case strings.HasSuffix(workflowPath, ".yaml"):
return strings.TrimSuffix(workflowPath, ".yaml") + ".lock.yml"A corresponding test case in TestResolveToLockFilePath should be added.
| // 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 = extractEngineIDFromLockFile(run.WorkflowPath, false) |
There was a problem hiding this comment.
verbose is hardcoded to false here, making lock-file engine detection messages invisible when --verbose is set.
💡 Details
Both orchestrator call sites (lines 520, 1128) correctly forward the runtime verbose flag, so users running with --verbose see "Detected engine from lock file metadata: copilot" messages. This call site always passes false, silently suppressing those messages for engine IDs resolved during report building. The result is incomplete and inconsistent verbose output.
buildLogsData currently has no verbose parameter — that parameter should be threaded in from the caller, or false should be replaced with the appropriate flag value.
|
|
||
| // Use .lock.yml extension by default | ||
| workflowPath := tc.workflowPath | ||
| if workflowPath == "" && tc.name != "empty workflow path returns empty" { |
There was a problem hiding this comment.
Test setup is gated on a magic test-name string sentinel — renaming the case silently breaks its setup logic.
💡 Details and fix
The condition tc.name != "empty workflow path returns empty" uses the human-readable description as a run-time flag to suppress file creation. If anyone renames that test case the condition becomes vacuously false and the workflowPath == "" case will now try to create a lock file for an empty path, silently changing test semantics or panicking.
Replace with an explicit boolean field:
struct {
name string
lockFileContent string
workflowPath string
skipFileCreate bool // true when workflowPath is intentionally empty
expectEngineID string
}
// ...
{
name: "empty workflow path returns empty",
workflowPath: "",
skipFileCreate: true,
expectEngineID: "",
},
// in setup:
if !tc.skipFileCreate {
// create lock file ...
}
PR Triage
Fallback reads agent_id from lock file gh-aw-metadata when aw_info.json is absent. Fixes --engine filter silently excluding older runs. Explicit precedence rules, well-described.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Runs that pre-date the
aw_info.jsonartifact (e.g.daily-cli-tools-tester) returnengine_id: null, making the--enginefilter silently exclude them. The fix adds a fallback that readsagent_idfrom the# gh-aw-metadata:comment compiled into the lock file.Changes
logs_parsing_core.go— addsextractEngineIDFromLockFile(workflowPath, verbose)which reads the local.lock.yml, parses itsgh-aw-metadataJSON comment, and returnsagent_id. Also addsresolveToLockFilePathto normalise.yml/.md/.lock.ymlinputs.logs_orchestrator.go—matchEngineFiltergains alockFileEngineID stringparameter used as fallback whenaw_info.jsonis absent or carries noengine_id. Both call sites compute it viaextractEngineIDFromLockFile(result.Run.WorkflowPath, verbose).logs_report.go—buildLogsDatafalls back toextractEngineIDFromLockFilewhenaw_info.jsonyields noengine_id, soengine_id,engine, andengine_countsin the JSON output reflect the lock file metadata for older runs.Precedence
aw_info.jsonremains authoritative; the lock file is only consulted whenaw_info.jsonis missing or empty:pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/28776499241