Skip to content

Resolve remote workflow names for logs targets #59454

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 1 commit into from
Sep 8, 2026
Merged

Resolve remote workflow names for logs targets #59454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
54 changes: 54 additions & 0 deletions pkg/cli/logs_command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
package cli

import (
"context"
"errors"
"fmt"
"os"
Expand Down Expand Up @@ -248,6 +249,8 @@ func loadLogsCommandValues(cmd *cobra.Command, args []string) (*logsCommandValue
return nil, err
}
targets, targetErrors := resolveLogsWorkflowTargets(cmd, args)
targets, remoteTargetErrors := resolveRemoteLogsWorkflowTargets(cmd.Context(), targets, options.Verbose)
targetErrors = append(targetErrors, remoteTargetErrors...)
if len(args) <= 1 && len(targetErrors) > 0 {
return nil, targetErrors[0]
}
Expand Down Expand Up @@ -299,6 +302,57 @@ func resolveLogsWorkflowTarget(cmd *cobra.Command, arg string) (logsWorkflowTarg
return logsWorkflowTarget{workflowName: workflowName}, err
}

func resolveRemoteLogsWorkflowTargets(ctx context.Context, targets []logsWorkflowTarget, verbose bool) ([]logsWorkflowTarget, []error) {
return resolveRemoteLogsWorkflowTargetsWithFetcher(ctx, targets, verbose, fetchGitHubWorkflows)
}

func resolveRemoteLogsWorkflowTargetsWithFetcher(
ctx context.Context,
targets []logsWorkflowTarget,
verbose bool,
fetchWorkflows func(context.Context, string, bool) (map[string]*GitHubWorkflow, error),
) ([]logsWorkflowTarget, []error) {
workflowsByRepo := make(map[string]map[string]*GitHubWorkflow)
errorsByRepo := make(map[string]error)
resolvedTargets := make([]logsWorkflowTarget, 0, len(targets))
var targetErrors []error

for i := range targets {
target := &targets[i]
if target.workflowName == "" || target.repoOverride == "" || repoIsLocal(target.repoOverride) {
resolvedTargets = append(resolvedTargets, *target)
continue
}
Comment on lines +322 to +325

githubWorkflows, ok := workflowsByRepo[target.repoOverride]
if !ok {
if err := errorsByRepo[target.repoOverride]; err != nil {
targetErrors = append(targetErrors, fmt.Errorf("%s: failed to resolve workflow name: %w", target.displayName(), err))
continue
}
var err error
githubWorkflows, err = fetchWorkflows(ctx, target.repoOverride, verbose)
if err != nil {
errorsByRepo[target.repoOverride] = err
targetErrors = append(targetErrors, fmt.Errorf("%s: failed to resolve workflow name: %w", target.displayName(), err))
continue
}
workflowsByRepo[target.repoOverride] = githubWorkflows
}

resolvedName := matchRemoteWorkflowName(target.workflowName, githubWorkflows)
if resolvedName == "" {
Comment on lines +343 to +344

@github-actions github-actions Bot Sep 8, 2026

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.

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

targetErrors = append(targetErrors, fmt.Errorf("%s: workflow not found", target.displayName()))
continue
}
logsCommandLog.Printf("Resolved remote workflow name: %s -> %s", target.displayName(), resolvedName)
target.workflowName = resolvedName
resolvedTargets = append(resolvedTargets, *target)

@github-actions github-actions Bot Sep 8, 2026

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] 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, assert fetchWorkflows is 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).
  • fetchWorkflows returning an error for a repo with two targets: assert both targets get failed to resolve workflow name errors and the fetch is attempted only once (errorsByRepo cache hit).
  • A target whose workflowName still carries a .yml/.md suffix (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.

}

return resolvedTargets, targetErrors
}

func splitCrossRepoWorkflowTarget(arg string) (string, string, bool) {
if isLocalWorkflowPath(arg) || strings.HasPrefix(filepath.ToSlash(arg), constants.GithubDir) {
return "", "", false
Expand Down
26 changes: 26 additions & 0 deletions pkg/cli/logs_command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package cli

import (
"context"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -430,6 +431,31 @@ func TestResolveLogsWorkflowTargetCrossRepo(t *testing.T) {
assert.Equal(t, "daily-report.yml", target.workflowName)
}

func TestResolveRemoteLogsWorkflowTargets(t *testing.T) {
t.Parallel()

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)
Comment on lines +437 to +456

@github-actions github-actions Bot Sep 8, 2026

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.

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.

}

func TestLogsCommandStdinFlag(t *testing.T) {
t.Parallel()
cmd := NewLogsCommand()
Expand Down
Loading