Repository navigation
Resolve remote workflow names for logs targets - #59454
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Three moderate workflow-resolution and reporting issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Resolves remote workflow IDs to GitHub Actions display names before listing runs.
Changes:
- Caches remote workflow metadata per repository.
- Preserves successful targets when other resolutions fail.
- Adds basic display-name resolution coverage.
File summaries
| File | Review |
|---|---|
pkg/cli/logs_command.go |
Moderate: repository-qualified local targets bypass resolution; remote fetch counts use local files; cross-repository workflow paths fail ID fallback. |
pkg/cli/logs_command_test.go |
Nit: add multi-target coverage for caching, unresolved targets, and preserved successes. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Balanced
| if target.workflowName == "" || target.repoOverride == "" || repoIsLocal(target.repoOverride) { | ||
| resolvedTargets = append(resolvedTargets, *target) | ||
| continue | ||
| } |
| continue | ||
| } | ||
| var err error | ||
| githubWorkflows, err = fetchWorkflows(ctx, target.repoOverride, verbose) |
| resolvedName := matchRemoteWorkflowName(target.workflowName, githubWorkflows) | ||
| if resolvedName == "" { |
| targets := []logsWorkflowTarget{ | ||
| {repoOverride: "githubnext/gh-aw-cao", workflowName: "uk-ai-advisory"}, | ||
| } | ||
| fetchWorkflows := func(context.Context, string, bool) (map[string]*GitHubWorkflow, error) { | ||
| return map[string]*GitHubWorkflow{ | ||
| "uk-ai-advisory": { | ||
| Name: "UK AI Advisory", | ||
| Path: ".github/workflows/uk-ai-advisory.lock.yml", | ||
| }, | ||
| }, nil | ||
| } | ||
|
|
||
| resolved, targetErrors := resolveRemoteLogsWorkflowTargetsWithFetcher( | ||
| context.Background(), targets, false, fetchWorkflows, | ||
| ) | ||
|
|
||
| require.Empty(t, targetErrors) | ||
| require.Len(t, resolved, 1) | ||
| assert.Equal(t, "UK AI Advisory", resolved[0].workflowName) | ||
| assert.Equal(t, "githubnext/gh-aw-cao", resolved[0].repoOverride) |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The remote workflow-name resolution logic is reasonable, but the new test coverage is too narrow for the failure modes this code adds.
Blocking theme
The implementation now caches remote workflow metadata per repository and intentionally keeps successful targets when a sibling target in the same batch cannot be resolved. That is exactly the kind of control flow that breaks later if we only test a single happy-path target. Right now there is no regression test proving that same-repo targets share one fetch or that an unresolved target does not poison the rest of the batch.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
registry.npmjs.org
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 25.2 AIC · ⌖ 7.41 AIC · ⊞ 23.5K
Comment /review to run again
| require.Empty(t, targetErrors) | ||
| require.Len(t, resolved, 1) | ||
| assert.Equal(t, "UK AI Advisory", resolved[0].workflowName) | ||
| assert.Equal(t, "githubnext/gh-aw-cao", resolved[0].repoOverride) |
There was a problem hiding this comment.
The new test only exercises a single happy-path target, so the repository-level cache and partial-failure behavior added in resolveRemoteLogsWorkflowTargetsWithFetcher can regress unnoticed. A second target in the same repo and an unresolved target in the same batch are the cases that prove this logic actually preserves successful targets while reusing the cached workflow list.
💡 Add a batch test that covers the new control flow
The new code introduces two behaviors that are easy to break independently:
targets := []logsWorkflowTarget{
{repoOverride: "githubnext/gh-aw-cao", workflowName: "uk-ai-advisory"},
{repoOverride: "githubnext/gh-aw-cao", workflowName: "missing-workflow"},
}
var fetchCalls int
fetchWorkflows := func(context.Context, string, bool) (map[string]*GitHubWorkflow, error) {
fetchCalls++
return map[string]*GitHubWorkflow{
"uk-ai-advisory": {Name: "UK AI Advisory", Path: ".github/workflows/uk-ai-advisory.lock.yml"},
}, nil
}
resolved, errs := resolveRemoteLogsWorkflowTargetsWithFetcher(context.Background(), targets, false, fetchWorkflows)
require.Len(t, resolved, 1)
require.Len(t, errs, 1)
assert.Equal(t, 1, fetchCalls)Without a test like this, a future refactor can accidentally re-fetch per target or drop all results after the first unresolved workflow, and this PR would still pass.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd. The fix correctly resolves the display-name mismatch for remote logs targets and reuses per-repo workflow metadata, but it reintroduces a regression for extension-suffixed workflow targets, and the new test doesn't cover the multi-target caching/skip behavior the PR describes.
📋 Key Findings
Confirmed Bug
matchRemoteWorkflowNamematches against the file-derived key (extension stripped), but cross-repo file-path targets (e.g.owner/repo/.github/workflows/daily-report.yml) retain their.ymlsuffix inworkflowName. I reproduced this with a standalone test — resolution now fails withworkflow not foundfor such targets, which worked prior to this PR since no remote lookup was previously performed on the raw name.
Test Coverage Gap
- The only new test (
TestResolveRemoteLogsWorkflowTargets) covers a single successful target. The PR body advertises repository-metadata reuse across multiple targets and skip-unresolved-while-preserving-successful behavior — neither is exercised, so both could regress silently.
Positive Highlights
- ✅ Per-repo caching of both successful (
workflowsByRepo) and failed (errorsByRepo) fetches avoids redundant API calls across targets. - ✅ Correctly short-circuits for local repos and empty names, avoiding unnecessary remote calls.
Note: this overlaps with and confirms one of the automated Copilot review comments already on this PR (extension/file-path target mismatch); I've added a concrete repro and a fix suggestion, plus a distinct test-coverage recommendation.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 88.1 AIC · ⌖ 14.9 AIC · ⊞ 10.3K
Comment /matt to run again
| } | ||
|
|
||
| resolvedName := matchRemoteWorkflowName(target.workflowName, githubWorkflows) | ||
| if resolvedName == "" { |
There was a problem hiding this comment.
[/diagnosing-bugs] Confirmed regression: matchRemoteWorkflowName matches on the file-derived key (no extension), but splitCrossRepoWorkflowTarget/resolveLogsWorkflowTarget leave file targets like owner/repo/.github/workflows/daily-report.yml with the .yml suffix intact, so this new resolution step now fails cross-repo file targets that worked before this PR.
💡 Repro + fix
I reproduced this with a standalone test:
targets := []logsWorkflowTarget{{repoOverride:"o/r", workflowName:"daily-report.yml"}}
fetch := func(context.Context,string,bool)(map[string]*GitHubWorkflow,error){
return map[string]*GitHubWorkflow{"daily-report": {Name:"Daily Report", Path:".github/workflows/daily-report.lock.yml"}}, nil
}
resolved, errs := resolveRemoteLogsWorkflowTargetsWithFetcher(context.Background(), targets, false, fetch)
// errs = ["o/r/daily-report.yml: workflow not found"]The root cause (mismatched display name) is fixed, but a second pre-existing normalization gap (extension not stripped for file-path targets before matching) is now newly exercised by this code path. Normalize target.workflowName (e.g. via normalizeWorkflowID) before calling matchRemoteWorkflowName, and add a regression test for a .yml/.md-suffixed file target.
@copilot please address this.
| } | ||
| logsCommandLog.Printf("Resolved remote workflow name: %s -> %s", target.displayName(), resolvedName) | ||
| target.workflowName = resolvedName | ||
| resolvedTargets = append(resolvedTargets, *target) |
There was a problem hiding this comment.
[/tdd] resolveRemoteLogsWorkflowTargetsWithFetcher has no test for the multi-target/repo-cache behavior it advertises (single fetch per repo, skip-on-error preserving other targets, per-repo error reuse). Only a single successful target is exercised.
💡 Suggested test cases
- Two targets, same
repoOverride, assertfetchWorkflowsis called exactly once (e.g. via a call counter closure). - One resolvable + one unresolvable workflow name in the same repo: assert the resolvable target is still returned and only the unresolvable one produces an error (
workflow not found). fetchWorkflowsreturning an error for a repo with two targets: assert both targets getfailed to resolve workflow nameerrors and the fetch is attempted only once (errorsByRepocache hit).- A target whose
workflowNamestill carries a.yml/.mdsuffix (file-path target), to catch the normalization gap noted in the other comment.
Without these, the repository-cache and skip-unresolved logic described in the PR body can silently regress.
@copilot please address this.
There was a problem hiding this comment.
This PR is a backend Go change (workflow-name resolution logic for logs targets), not a UI change, so Impeccable UI modes (audit/critique/harden/etc.) don't apply. Performed a standard correctness/security review instead.
I independently verified the existing unresolved Copilot review comments on this PR are all valid, high-confidence, and still unaddressed in the current diff:
logs_command.go:344— confirmed bug:normalizeWorkflowID("daily-report.yml")only strips.lock.yml/.mdsuffixes, so a cross-repo file target likeowner/repo/.github/workflows/daily-report.ymlkeepstarget.workflowName = "daily-report.yml". MeanwhilefetchGitHubWorkflows/extractWorkflowNameFromPathbuilds the remote map keyed by the extension-stripped name ("daily-report").matchRemoteWorkflowName's case-insensitive key comparison will therefore never match, and resolution silently falls through to "workflow not found" for exactly the documented cross-repo file-path use case.logs_command.go:325andlogs_command.go:334— as previously flagged, the local-checkout skip and the local-.md-file-based success counting infetchGitHubWorkflowsdon't generalize correctly to purely remote repos.logs_command_test.go:456— test coverage gap: only a single successful target is exercised; the advertised per-repo caching (workflowsByRepo/errorsByRepo) and skip-on-unresolved-target behavior have no test coverage and could regress silently.
These are blocking correctness issues in the core new logic (resolveRemoteLogsWorkflowTargetsWithFetcher) and should be fixed before merge. I'm not re-posting duplicate inline comments since the threads are already open; please address the existing comment threads.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 89.8 AIC · ⌖ 13.3 AIC · ⊞ 8.3K
|
🎉 This pull request is included in a new release. Release: |
Remote logs targets used the agentic workflow ID directly with
gh run list, which fails when the GitHub Actions workflow has a different display name.Remote resolution
Multi-target handling