diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000000..98024f533c3 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,165 @@ +# AGENTS.md + +This is the GitHub CLI (`gh`), a command-line tool for interacting with GitHub. The module path is `github.com/cli/cli/v2`. + +## Build, Test, and Lint + +```bash +make # Build (Unix) — outputs bin/gh +go run script/build.go # Build (Windows) +go test ./... # All unit tests +go test ./pkg/cmd/issue/list/... -run TestIssueList_nontty # Single test +go test -tags acceptance ./acceptance # Acceptance tests +make lint # golangci-lint (same as CI) +``` + +## Architecture + +Entry point: `cmd/gh/main.go` → `internal/ghcmd.Main()` → `pkg/cmd/root.NewCmdRoot()`. + +Key packages: +- `pkg/cmd///` — CLI command implementations +- `pkg/cmdutil/` — Factory, error types, flag helpers (`NilStringFlag`, `NilBoolFlag`, `StringEnumFlag`) +- `pkg/iostreams/` — I/O abstraction with TTY detection, color, pager +- `pkg/httpmock/` — HTTP mocking for tests +- `api/` — GitHub API client (GraphQL + REST) +- `internal/featuredetection/` — GitHub.com vs GHES capability detection +- `internal/tableprinter/` — Table output for list commands + +## Command Structure + +A command `gh foo bar` lives in `pkg/cmd/foo/bar/` with `bar.go`, `bar_test.go`, and optionally `http.go`/`http_test.go`. + +### Canonical Examples + +- **Command + tests**: `pkg/cmd/issue/list/list.go` and `list_test.go` +- **Factory wiring**: `pkg/cmd/factory/default.go` +- **Unit tests**: `internal/agents/detect_test.go` + +### The Options + Factory Pattern + +Every command follows this structure (see `pkg/cmd/issue/list/list.go`): + +1. `Options` struct with `IO`, `HttpClient`, `Config`, `BaseRepo` + flags +2. `NewCmdFoo(f *cmdutil.Factory, runF func(*FooOptions) error)` constructor — `runF` is the test injection point +3. Separate `fooRun(opts)` function with the business logic + +Key rules: +- Lazy-init `BaseRepo`, `Remotes`, `Branch` inside `RunE`, not the constructor +- Commands register in `pkg/cmd/root/root.go`; subcommand groups use `cmdutil.AddGroup()` + +### Command Examples and Help Text + +Use `heredoc.Doc` for examples with `#` comment lines and `$ ` command prefixes: +```go +Example: heredoc.Doc(` + # Do the thing + $ gh foo bar --flag value +`), +``` + +### JSON Output + +Add `--json`, `--jq`, `--template` flags via `cmdutil.AddJSONFlags(cmd, &opts.Exporter, fieldNames)`. In the run function: `if opts.Exporter != nil { return opts.Exporter.Write(opts.IO, data) }`. See `pkg/cmd/pr/list/list.go`. + +## Testing + +### HTTP Mocking + +Use `httpmock.Registry` with `defer reg.Verify(t)` to ensure all stubs are called: + +```go +reg := &httpmock.Registry{} +defer reg.Verify(t) + +reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO"), + httpmock.JSONResponse(someData), +) +reg.Register( + httpmock.GraphQL(`query PullRequestList\b`), + httpmock.FileResponse("./fixtures/prList.json"), +) +client := &http.Client{Transport: reg} +``` + +Common: `REST(method, path)`, `GraphQL(pattern)`, `JSONResponse(body)`, `FileResponse(path)`. See `pkg/httpmock/` for all matchers/responders. + +### IOStreams in Tests + +```go +ios, stdin, stdout, stderr := iostreams.Test() +ios.SetStdoutTTY(true) // simulate terminal +``` + +### Assertions + +Use `testify`. Always use `require` (not `assert`) for error checks so the test halts immediately: + +```go +require.NoError(t, err) +require.Error(t, err) +assert.Equal(t, "expected", actual) +``` + +### Generated Mocks + +Interfaces use `moq`: `//go:generate moq -rm -out prompter_mock.go . Prompter`. Run `go generate ./...` after interface changes. + +### Table-Driven Tests + +Use table-driven tests for functions with multiple input/output scenarios. See `internal/agents/detect_test.go` or `pkg/cmd/issue/list/list_test.go` for examples: + +```go +tests := []struct { + name string + // inputs and expected outputs +}{ + {name: "descriptive case name", ...}, +} +for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // arrange, act, assert + }) +} +``` + +## Code Style + +- Add godoc comments to all exported functions, types, and constants +- Avoid unnecessary code comments — only comment when the *why* isn't obvious from the code +- Do not comment just to restate what the code does + +## Error Handling + +Error types in `pkg/cmdutil/errors.go`: +- `FlagErrorf(...)` — flag validation (prints usage) +- `cmdutil.SilentError` — exit 1, no message +- `cmdutil.CancelError` — user cancelled +- `cmdutil.PendingError` — outcome pending +- `cmdutil.NoResultsError` — empty results + +Use `cmdutil.MutuallyExclusive("message", cond1, cond2)` for mutually exclusive flags. + +## Feature Detection + +Commands using feature detection must include a `// TODO ` comment directly above the if-statement for linter compliance: + +```go +// TODO someFeatureCleanup +if features.SomeCapability { + // use new API +} else { + // fallback for older GHES +} +``` + +## API Patterns + +```go +client := api.NewClientFromHTTP(httpClient) +client.GraphQL(hostname, query, variables, &data) +client.REST(hostname, "GET", "repos/owner/repo", nil, &data) +``` + +For host resolution, use `cfg.Authentication().DefaultHost()` — not `ghinstance.Default()` which always returns `github.com`. \ No newline at end of file diff --git a/api/client.go b/api/client.go index 207fd86d368..8ee525df59d 100644 --- a/api/client.go +++ b/api/client.go @@ -15,7 +15,6 @@ import ( ) const ( - accept = "Accept" apiVersion = "X-GitHub-Api-Version" apiVersionValue = "2022-11-28" authorization = "Authorization" diff --git a/api/http_client.go b/api/http_client.go index 9957f6bc51d..532f79c7f9d 100644 --- a/api/http_client.go +++ b/api/http_client.go @@ -18,6 +18,7 @@ type tokenGetter interface { type HTTPClientOptions struct { AppVersion string + InvokingAgent string CacheTTL time.Duration Config tokenGetter EnableCache bool @@ -48,8 +49,13 @@ func NewHTTPClient(opts HTTPClientOptions) (*http.Client, error) { clientOpts.LogVerboseHTTP = opts.LogVerboseHTTP } + ua := fmt.Sprintf("GitHub CLI %s", opts.AppVersion) + if opts.InvokingAgent != "" { + ua = fmt.Sprintf("%s Agent/%s", ua, opts.InvokingAgent) + } + headers := map[string]string{ - userAgent: fmt.Sprintf("GitHub CLI %s", opts.AppVersion), + userAgent: ua, apiVersion: apiVersionValue, } clientOpts.Headers = headers diff --git a/api/http_client_test.go b/api/http_client_test.go index 824bc0f1b13..1c81b4aa7a5 100644 --- a/api/http_client_test.go +++ b/api/http_client_test.go @@ -20,6 +20,7 @@ func TestNewHTTPClient(t *testing.T) { type args struct { config tokenGetter appVersion string + invokingAgent string logVerboseHTTP bool skipDefaultHeaders bool } @@ -155,6 +156,18 @@ func TestNewHTTPClient(t *testing.T) { * Request took `), }, + { + name: "includes invoking agent in user-agent header", + args: args{ + appVersion: "v1.2.3", + invokingAgent: "copilot-cli", + }, + host: "github.com", + wantHeader: map[string][]string{ + "user-agent": {"GitHub CLI v1.2.3 Agent/copilot-cli"}, + }, + wantStderr: "", + }, } var gotReq *http.Request @@ -169,6 +182,7 @@ func TestNewHTTPClient(t *testing.T) { ios, _, _, stderr := iostreams.Test() client, err := NewHTTPClient(HTTPClientOptions{ AppVersion: tt.args.appVersion, + InvokingAgent: tt.args.invokingAgent, Config: tt.args.config, Log: ios.ErrOut, LogVerboseHTTP: tt.args.logVerboseHTTP, diff --git a/api/queries_issue.go b/api/queries_issue.go index d545ef59f85..f719e52f9ae 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -289,7 +289,8 @@ func IssueCreate(client *Client, repo *Repository, params map[string]interface{} switch key { case "assigneeIds", "body", "issueTemplate", "labelIds", "milestoneId", "projectIds", "repositoryId", "title": inputParams[key] = val - case "projectV2Ids": + case "projectV2Ids", "assigneeLogins": + // handled after issue creation default: return nil, fmt.Errorf("invalid IssueCreate mutation parameter %s", key) } @@ -310,6 +311,14 @@ func IssueCreate(client *Client, repo *Repository, params map[string]interface{} } issue := &result.CreateIssue.Issue + // Assign users using login-based mutation when ActorAssignees is true (github.com). + if assigneeLogins, ok := params["assigneeLogins"].([]string); ok && len(assigneeLogins) > 0 { + err := ReplaceActorsForAssignableByLogin(client, repo, issue.ID, assigneeLogins) + if err != nil { + return issue, err + } + } + // projectV2 parameters aren't supported in the `createIssue` mutation, // so add them after the issue has been created. projectV2Ids, ok := params["projectV2Ids"].([]string) diff --git a/api/queries_pr.go b/api/queries_pr.go index d0488c64b4b..01846a9e6d0 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -524,6 +524,14 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter } } + // Assign users using login-based mutation when ActorAssignees is true (github.com). + if assigneeLogins, ok := params["assigneeLogins"].([]string); ok && len(assigneeLogins) > 0 { + err := ReplaceActorsForAssignableByLogin(client, repo, pr.ID, assigneeLogins) + if err != nil { + return pr, err + } + } + // TODO requestReviewsByLoginCleanup // Request reviewers using either login-based (github.com) or ID-based (GHES) mutation. // The ID-based path can be removed once GHES supports requestReviewsByLogin. @@ -581,6 +589,35 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter return pr, nil } +// ReplaceActorsForAssignableByLogin calls the replaceActorsForAssignable mutation +// using actor logins. This avoids the need to resolve logins to node IDs. +func ReplaceActorsForAssignableByLogin(client *Client, repo ghrepo.Interface, assignableID string, logins []string) error { + type ReplaceActorsForAssignableInput struct { + AssignableID githubv4.ID `json:"assignableId"` + ActorLogins []githubv4.String `json:"actorLogins"` + } + + actorLogins := make([]githubv4.String, len(logins)) + for i, l := range logins { + actorLogins[i] = githubv4.String(l) + } + + var mutation struct { + ReplaceActorsForAssignable struct { + TypeName string `graphql:"__typename"` + } `graphql:"replaceActorsForAssignable(input: $input)"` + } + + variables := map[string]interface{}{ + "input": ReplaceActorsForAssignableInput{ + AssignableID: githubv4.ID(assignableID), + ActorLogins: actorLogins, + }, + } + + return client.Mutate(repo.RepoHost(), "ReplaceActorsForAssignable", &mutation, variables) +} + // SuggestedAssignableActors fetches up to 10 suggested actors for a specific assignable // (Issue or PullRequest) node ID. `assignableID` is the GraphQL node ID for the Issue/PR. // Returns the actors, the total count of available assignees in the repo, and an error. diff --git a/api/queries_repo.go b/api/queries_repo.go index d358255d8b9..e5806b91e90 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -1298,6 +1298,69 @@ func RepoAssignableActors(client *Client, repo ghrepo.Interface) ([]AssignableAc return actors, nil } +// SearchRepoAssignableActors searches assignable actors for a repository with an optional +// query string. Unlike RepoAssignableActors which fetches all actors with pagination, this +// returns up to 10 results matching the query, suitable for search-based selection. +func SearchRepoAssignableActors(client *Client, repo ghrepo.Interface, query string) ([]AssignableActor, int, error) { + type responseData struct { + Repository struct { + AssignableUsers struct { + TotalCount int + } + SuggestedActors struct { + Nodes []struct { + User struct { + ID string + Login string + Name string + TypeName string `graphql:"__typename"` + } `graphql:"... on User"` + Bot struct { + ID string + Login string + TypeName string `graphql:"__typename"` + } `graphql:"... on Bot"` + } + } `graphql:"suggestedActors(first: 10, query: $query, capabilities: CAN_BE_ASSIGNED)"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + + var q *githubv4.String + if query != "" { + v := githubv4.String(query) + q = &v + } + + variables := map[string]interface{}{ + "owner": githubv4.String(repo.RepoOwner()), + "name": githubv4.String(repo.RepoName()), + "query": q, + } + + var result responseData + if err := client.Query(repo.RepoHost(), "SearchRepoAssignableActors", &result, variables); err != nil { + return nil, 0, err + } + + var actors []AssignableActor + for _, node := range result.Repository.SuggestedActors.Nodes { + if node.User.TypeName == "User" { + actors = append(actors, AssignableUser{ + id: node.User.ID, + login: node.User.Login, + name: node.User.Name, + }) + } else if node.Bot.TypeName == "Bot" { + actors = append(actors, AssignableBot{ + id: node.Bot.ID, + login: node.Bot.Login, + }) + } + } + + return actors, result.Repository.AssignableUsers.TotalCount, nil +} + type RepoLabel struct { ID string Name string diff --git a/go.mod b/go.mod index a3a55c3cd68..76b90741c47 100644 --- a/go.mod +++ b/go.mod @@ -54,7 +54,7 @@ require ( golang.org/x/sync v0.20.0 golang.org/x/term v0.40.0 golang.org/x/text v0.34.0 - google.golang.org/grpc v1.79.2 + google.golang.org/grpc v1.79.3 google.golang.org/protobuf v1.36.11 gopkg.in/h2non/gock.v1 v1.1.2 gopkg.in/yaml.v3 v3.0.1 diff --git a/go.sum b/go.sum index af66f223f17..54b5f05c4a0 100644 --- a/go.sum +++ b/go.sum @@ -640,8 +640,8 @@ google.golang.org/genproto/googleapis/api v0.0.0-20251202230838-ff82c1b0f217 h1: google.golang.org/genproto/googleapis/api v0.0.0-20251202230838-ff82c1b0f217/go.mod h1:+rXWjjaukWZun3mLfjmVnQi18E1AsFbDN9QdJ5YXLto= google.golang.org/genproto/googleapis/rpc v0.0.0-20251222181119-0a764e51fe1b h1:Mv8VFug0MP9e5vUxfBcE3vUkV6CImK3cMNMIDFjmzxU= google.golang.org/genproto/googleapis/rpc v0.0.0-20251222181119-0a764e51fe1b/go.mod h1:j9x/tPzZkyxcgEFkiKEEGxfvyumM01BEtsW8xzOahRQ= -google.golang.org/grpc v1.79.2 h1:fRMD94s2tITpyJGtBBn7MkMseNpOZU8ZxgC3MMBaXRU= -google.golang.org/grpc v1.79.2/go.mod h1:KmT0Kjez+0dde/v2j9vzwoAScgEPx/Bw1CYChhHLrHQ= +google.golang.org/grpc v1.79.3 h1:sybAEdRIEtvcD68Gx7dmnwjZKlyfuc61Dyo9pGXXkKE= +google.golang.org/grpc v1.79.3/go.mod h1:KmT0Kjez+0dde/v2j9vzwoAScgEPx/Bw1CYChhHLrHQ= google.golang.org/protobuf v1.36.11 h1:fV6ZwhNocDyBLK0dj+fg8ektcVegBBuEolpbTQyBNVE= google.golang.org/protobuf v1.36.11/go.mod h1:HTf+CrKn2C3g5S8VImy6tdcUvCska2kB7j23XfzDpco= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/internal/agents/detect.go b/internal/agents/detect.go new file mode 100644 index 00000000000..fd330ed67e3 --- /dev/null +++ b/internal/agents/detect.go @@ -0,0 +1,99 @@ +package agents + +import ( + "fmt" + "os" + "regexp" +) + +// AgentName is a validated agent identifier safe for use in HTTP headers. +type AgentName string + +const ( + agentAmp AgentName = "amp" + agentClaudeCode AgentName = "claude-code" + agentCodex AgentName = "codex" + agentCopilotCLI AgentName = "copilot-cli" + agentGeminiCLI AgentName = "gemini-cli" + agentOpencode AgentName = "opencode" +) + +var validAgentName = regexp.MustCompile(`^[a-zA-Z0-9_-]+$`) + +// parseAgentName validates and returns an AgentName from a raw string. +// Only alphanumeric characters, hyphens, and underscores are allowed. +func parseAgentName(s string) (AgentName, error) { + if !validAgentName.MatchString(s) { + return "", fmt.Errorf("invalid agent name %q: must match [a-zA-Z0-9_-]+", s) + } + return AgentName(s), nil +} + +// Detect returns the name of the AI coding agent driving the CLI, +// or an empty AgentName if none is detected. +func Detect() AgentName { + return detectWith(os.LookupEnv) +} + +func detectWith(lookup func(string) (string, bool)) AgentName { + isSet := func(key string) bool { + v, ok := lookup(key) + return ok && v != "" + } + + valueOf := func(key string) string { + v, _ := lookup(key) + return v + } + + // Generic agent identifiers — checked first because they are the most specific signal. + if v, ok := lookup("AI_AGENT"); ok && v != "" { + if name, err := parseAgentName(v); err == nil { + return name + } + } + + // Tool-specific variables. + + // Check AGENT=amp before the more generic CLAUDECODE=1 since Amp sets both. + if valueOf("AGENT") == "amp" { + return agentAmp + } + + // OpenAI Codex CLI — https://github.com/openai/codex + // CODEX_SANDBOX: https://github.com/openai/codex/blob/95e1d5993985019ce0ce0d10689caf1375f95120/codex-rs/core/src/spawn.rs#L25 + // CODEX_THREAD_ID: https://github.com/openai/codex/blob/95e1d5993985019ce0ce0d10689caf1375f95120/codex-rs/core/src/exec_env.rs#L8 + // CODEX_CI: https://github.com/openai/codex/blob/95e1d5993985019ce0ce0d10689caf1375f95120/codex-rs/core/src/unified_exec/process_manager.rs#L64 + if isSet("CODEX_SANDBOX") || isSet("CODEX_CI") || isSet("CODEX_THREAD_ID") { + return agentCodex + } + + // Google Gemini CLI — https://github.com/google-gemini/gemini-cli + // GEMINI_CLI: https://github.com/google-gemini/gemini-cli/blob/46fd7b4864111032a1c7dfa1821b2000fc7531da/docs/tools/shell.md#L96-L97 + if isSet("GEMINI_CLI") { + return agentGeminiCLI + } + + // GitHub Copilot CLI + // No first-party docs + if isSet("COPILOT_CLI") { + return agentCopilotCLI + } + + // OpenCode — https://github.com/anomalyco/opencode + // OPENCODE: https://github.com/anomalyco/opencode/blob/fde201c286a83ff32dda9b41d61d734a4449fe70/packages/opencode/src/index.ts#L78-L80 + if isSet("OPENCODE") { + return agentOpencode + } + + // Anthropic Claude Code — https://docs.anthropic.com/en/docs/agents-and-tools/claude-code/overview + // CLAUDECODE: https://code.claude.com/docs/en/env-vars (CLAUDECODE section) + // Checked last because other agents (e.g. Amp) set CLAUDECODE=1 alongside their own vars. + if isSet("CLAUDECODE") { + // There is a CLAUDE_CODE_ENTRYPOINT env var that is set to `cli` or `desktop` etc, but it's not documented + // so we don't want to rely on it too heavily. We'll just return a generic claude-code agent name. + return agentClaudeCode + } + + return "" +} diff --git a/internal/agents/detect_test.go b/internal/agents/detect_test.go new file mode 100644 index 00000000000..6cb4f461195 --- /dev/null +++ b/internal/agents/detect_test.go @@ -0,0 +1,149 @@ +package agents + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func lookup(vars map[string]string) func(string) (string, bool) { + return func(key string) (string, bool) { + v, ok := vars[key] + return v, ok + } +} + +func TestParseAgentName(t *testing.T) { + tests := []struct { + name string + input string + want AgentName + wantErr bool + }{ + {name: "valid lowercase", input: "my-agent", want: "my-agent"}, + {name: "valid with underscore", input: "my_agent_v2", want: "my_agent_v2"}, + {name: "valid uppercase", input: "MyAgent", want: "MyAgent"}, + {name: "valid numbers", input: "agent123", want: "agent123"}, + {name: "spaces rejected", input: "my agent", wantErr: true}, + {name: "newline rejected", input: "my\nagent", wantErr: true}, + {name: "carriage return rejected", input: "my\ragent", wantErr: true}, + {name: "null byte rejected", input: "my\x00agent", wantErr: true}, + {name: "dot rejected", input: "my.agent", wantErr: true}, + {name: "slash rejected", input: "my/agent", wantErr: true}, + {name: "empty rejected", input: "", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := parseAgentName(tt.input) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + assert.Equal(t, tt.want, got) + } + }) + } +} + +func TestDetectWith(t *testing.T) { + tests := []struct { + name string + env map[string]string + wantAgent AgentName + }{ + { + name: "clean environment", + env: map[string]string{}, + wantAgent: "", + }, + { + name: "empty var is not detected", + env: map[string]string{"GEMINI_CLI": ""}, + wantAgent: "", + }, + { + name: "AGENT=amp detected as amp", + env: map[string]string{"AGENT": "amp"}, + wantAgent: "amp", + }, + { + name: "AGENT with non-amp value is ignored", + env: map[string]string{"AGENT": "other"}, + wantAgent: "", + }, + { + name: "AI_AGENT returns value as agent name", + env: map[string]string{"AI_AGENT": "some-agent"}, + wantAgent: "some-agent", + }, + { + name: "AI_AGENT with invalid characters is ignored", + env: map[string]string{"AI_AGENT": "bad\nagent"}, + wantAgent: "", + }, + { + name: "AI_AGENT with spaces is ignored", + env: map[string]string{"AI_AGENT": "bad agent"}, + wantAgent: "", + }, + { + name: "AI_AGENT takes priority over AGENT", + env: map[string]string{"AGENT": "amp", "AI_AGENT": "other"}, + wantAgent: "other", + }, + { + name: "CODEX_SANDBOX", + env: map[string]string{"CODEX_SANDBOX": "seatbelt"}, + wantAgent: "codex", + }, + { + name: "CODEX_CI", + env: map[string]string{"CODEX_CI": "1"}, + wantAgent: "codex", + }, + { + name: "CODEX_THREAD_ID", + env: map[string]string{"CODEX_THREAD_ID": "abc"}, + wantAgent: "codex", + }, + { + name: "GEMINI_CLI", + env: map[string]string{"GEMINI_CLI": "1"}, + wantAgent: "gemini-cli", + }, + { + name: "COPILOT_CLI", + env: map[string]string{"COPILOT_CLI": "1"}, + wantAgent: "copilot-cli", + }, + { + name: "OPENCODE", + env: map[string]string{"OPENCODE": "1"}, + wantAgent: "opencode", + }, + { + name: "CLAUDECODE", + env: map[string]string{"CLAUDECODE": "1"}, + wantAgent: "claude-code", + }, + { + name: "AGENT=amp takes priority over CLAUDECODE", + env: map[string]string{"AGENT": "amp", "CLAUDECODE": "1"}, + wantAgent: "amp", + }, + { + name: "invalid AI_AGENT falls through to tool-specific detection", + env: map[string]string{"AI_AGENT": "bad agent", "GEMINI_CLI": "1"}, + wantAgent: "gemini-cli", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := detectWith(lookup(tt.env)) + assert.Equal(t, tt.wantAgent, got) + }) + } +} diff --git a/internal/ghcmd/cmd.go b/internal/ghcmd/cmd.go index 9fc5bcefeaf..8690078c66e 100644 --- a/internal/ghcmd/cmd.go +++ b/internal/ghcmd/cmd.go @@ -15,6 +15,7 @@ import ( surveyCore "github.com/AlecAivazis/survey/v2/core" "github.com/AlecAivazis/survey/v2/terminal" "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/agents" "github.com/cli/cli/v2/internal/build" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/config/migration" @@ -44,7 +45,7 @@ func Main() exitCode { buildVersion := build.Version hasDebug, _ := utils.IsDebugEnabled() - cmdFactory := factory.New(buildVersion) + cmdFactory := factory.New(buildVersion, string(agents.Detect())) stderr := cmdFactory.IOStreams.ErrOut ctx := context.Background() diff --git a/pkg/cmd/attestation/verify/verify_integration_test.go b/pkg/cmd/attestation/verify/verify_integration_test.go index d77f21f70d0..ec64cefa7a3 100644 --- a/pkg/cmd/attestation/verify/verify_integration_test.go +++ b/pkg/cmd/attestation/verify/verify_integration_test.go @@ -26,7 +26,7 @@ func TestVerifyIntegration(t *testing.T) { TUFMetadataDir: o.Some(t.TempDir()), } - cmdFactory := factory.New("test") + cmdFactory := factory.New("test", "") hc, err := cmdFactory.HttpClient() if err != nil { @@ -143,7 +143,7 @@ func TestVerifyIntegrationCustomIssuer(t *testing.T) { TUFMetadataDir: o.Some(t.TempDir()), } - cmdFactory := factory.New("test") + cmdFactory := factory.New("test", "") hc, err := cmdFactory.HttpClient() if err != nil { @@ -217,7 +217,7 @@ func TestVerifyIntegrationReusableWorkflow(t *testing.T) { TUFMetadataDir: o.Some(t.TempDir()), } - cmdFactory := factory.New("test") + cmdFactory := factory.New("test", "") hc, err := cmdFactory.HttpClient() if err != nil { @@ -310,7 +310,7 @@ func TestVerifyIntegrationReusableWorkflowSignerWorkflow(t *testing.T) { TUFMetadataDir: o.Some(t.TempDir()), } - cmdFactory := factory.New("test") + cmdFactory := factory.New("test", "") hc, err := cmdFactory.HttpClient() if err != nil { diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index b4884038754..48ec0c8fe0c 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -26,23 +26,23 @@ import ( var ssoHeader string var ssoURLRE = regexp.MustCompile(`\burl=([^;]+)`) -func New(appVersion string) *cmdutil.Factory { +func New(appVersion string, invokingAgent string) *cmdutil.Factory { f := &cmdutil.Factory{ AppVersion: appVersion, Config: configFunc(), // No factory dependencies ExecutableName: "gh", } - f.IOStreams = ioStreams(f) // Depends on Config - f.HttpClient = httpClientFunc(f, appVersion) // Depends on Config, IOStreams, and appVersion - f.PlainHttpClient = plainHttpClientFunc(f, appVersion) // Depends on IOStreams, and appVersion - f.GitClient = newGitClient(f) // Depends on IOStreams, and Executable - f.Remotes = remotesFunc(f) // Depends on Config, and GitClient - f.BaseRepo = BaseRepoFunc(f) // Depends on Remotes - f.Prompter = newPrompter(f) // Depends on Config and IOStreams - f.Browser = newBrowser(f) // Depends on Config, and IOStreams - f.ExtensionManager = extensionManager(f) // Depends on Config, HttpClient, and IOStreams - f.Branch = branchFunc(f) // Depends on GitClient + f.IOStreams = ioStreams(f) // Depends on Config + f.HttpClient = httpClientFunc(f, appVersion, invokingAgent) // Depends on Config, IOStreams, appVersion, and invokingAgent + f.PlainHttpClient = plainHttpClientFunc(f, appVersion, invokingAgent) // Depends on IOStreams, appVersion, and invokingAgent + f.GitClient = newGitClient(f) // Depends on IOStreams, and Executable + f.Remotes = remotesFunc(f) // Depends on Config, and GitClient + f.BaseRepo = BaseRepoFunc(f) // Depends on Remotes + f.Prompter = newPrompter(f) // Depends on Config and IOStreams + f.Browser = newBrowser(f) // Depends on Config, and IOStreams + f.ExtensionManager = extensionManager(f) // Depends on Config, HttpClient, and IOStreams + f.Branch = branchFunc(f) // Depends on GitClient return f } @@ -186,7 +186,7 @@ func remotesFunc(f *cmdutil.Factory) func() (ghContext.Remotes, error) { return rr.Resolver() } -func httpClientFunc(f *cmdutil.Factory, appVersion string) func() (*http.Client, error) { +func httpClientFunc(f *cmdutil.Factory, appVersion string, invokingAgent string) func() (*http.Client, error) { return func() (*http.Client, error) { io := f.IOStreams cfg, err := f.Config() @@ -194,10 +194,11 @@ func httpClientFunc(f *cmdutil.Factory, appVersion string) func() (*http.Client, return nil, err } opts := api.HTTPClientOptions{ - Config: cfg.Authentication(), - Log: io.ErrOut, - LogColorize: io.ColorEnabled(), - AppVersion: appVersion, + Config: cfg.Authentication(), + Log: io.ErrOut, + LogColorize: io.ColorEnabled(), + AppVersion: appVersion, + InvokingAgent: invokingAgent, } client, err := api.NewHTTPClient(opts) if err != nil { @@ -208,13 +209,14 @@ func httpClientFunc(f *cmdutil.Factory, appVersion string) func() (*http.Client, } } -func plainHttpClientFunc(f *cmdutil.Factory, appVersion string) func() (*http.Client, error) { +func plainHttpClientFunc(f *cmdutil.Factory, appVersion string, invokingAgent string) func() (*http.Client, error) { return func() (*http.Client, error) { io := f.IOStreams opts := api.HTTPClientOptions{ - Log: io.ErrOut, - LogColorize: io.ColorEnabled(), - AppVersion: appVersion, + Log: io.ErrOut, + LogColorize: io.ColorEnabled(), + AppVersion: appVersion, + InvokingAgent: invokingAgent, // This is required to prevent automatic setting of auth and other headers. SkipDefaultHeaders: true, } diff --git a/pkg/cmd/factory/default_test.go b/pkg/cmd/factory/default_test.go index 7e0ed4af6f5..7d84caa8f26 100644 --- a/pkg/cmd/factory/default_test.go +++ b/pkg/cmd/factory/default_test.go @@ -66,7 +66,7 @@ func Test_BaseRepo(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - f := New("1") + f := New("1", "") rr := &remoteResolver{ readRemotes: func() (git.RemoteSet, error) { return tt.remotes, nil @@ -204,7 +204,7 @@ func Test_SmartBaseRepo(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - f := New("1") + f := New("1", "") rr := &remoteResolver{ readRemotes: func() (git.RemoteSet, error) { return tt.remotes, nil @@ -297,7 +297,7 @@ func Test_OverrideBaseRepo(t *testing.T) { if tt.envOverride != "" { t.Setenv("GH_REPO", tt.envOverride) } - f := New("1") + f := New("1", "") rr := &remoteResolver{ readRemotes: func() (git.RemoteSet, error) { return tt.remotes, nil @@ -375,7 +375,7 @@ func Test_ioStreams_pager(t *testing.T) { t.Setenv(k, v) } } - f := New("1") + f := New("1", "") f.Config = func() (gh.Config, error) { if tt.config == nil { return config.NewBlankConfig(), nil @@ -418,7 +418,7 @@ func Test_ioStreams_prompt(t *testing.T) { t.Setenv(k, v) } } - f := New("1") + f := New("1", "") f.Config = func() (gh.Config, error) { if tt.config == nil { return config.NewBlankConfig(), nil @@ -496,7 +496,7 @@ func Test_ioStreams_spinnerDisabled(t *testing.T) { for k, v := range tt.env { t.Setenv(k, v) } - f := New("1") + f := New("1", "") f.Config = func() (gh.Config, error) { if tt.config == nil { return config.NewBlankConfig(), nil @@ -564,7 +564,7 @@ func Test_ioStreams_accessiblePrompterEnabled(t *testing.T) { for k, v := range tt.env { t.Setenv(k, v) } - f := New("1") + f := New("1", "") f.Config = func() (gh.Config, error) { if tt.config == nil { return config.NewBlankConfig(), nil @@ -642,7 +642,7 @@ func Test_ioStreams_colorLabels(t *testing.T) { t.Setenv(k, v) } } - f := New("1") + f := New("1", "") f.Config = func() (gh.Config, error) { if tt.config == nil { return config.NewBlankConfig(), nil @@ -683,13 +683,13 @@ func TestSSOURL(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - f := New("1") + f := New("1", "") f.Config = func() (gh.Config, error) { return config.NewBlankConfig(), nil } ios, _, _, stderr := iostreams.Test() f.IOStreams = ios - client, err := httpClientFunc(f, "v1.2.3")() + client, err := httpClientFunc(f, "v1.2.3", "")() require.NoError(t, err) req, err := http.NewRequest("GET", ts.URL, nil) if tt.sso != "" { @@ -718,13 +718,13 @@ func TestPlainHttpClient(t *testing.T) { })) defer ts.Close() - f := New("1") + f := New("1", "") f.Config = func() (gh.Config, error) { return config.NewBlankConfig(), nil } ios, _, _, _ := iostreams.Test() f.IOStreams = ios - client, err := plainHttpClientFunc(f, "v1.2.3")() + client, err := plainHttpClientFunc(f, "v1.2.3", "")() require.NoError(t, err) req, err := http.NewRequest("GET", ts.URL, nil) @@ -759,7 +759,7 @@ func TestNewGitClient(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - f := New("1") + f := New("1", "") f.Config = func() (gh.Config, error) { if tt.config == nil { return config.NewBlankConfig(), nil diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index da3648c31b2..f9fab11a1c7 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -12,6 +12,7 @@ import ( fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/text" prShared "github.com/cli/cli/v2/pkg/cmd/pr/shared" "github.com/cli/cli/v2/pkg/cmdutil" @@ -178,18 +179,12 @@ func createRun(opts *CreateOptions) (err error) { // Replace special values in assignees // For web mode, @copilot should be replaced by name; otherwise, login. - assigneeSet := set.NewStringSet() - meReplacer := prShared.NewMeReplacer(apiClient, baseRepo.RepoHost()) - copilotReplacer := prShared.NewCopilotReplacer(!opts.WebMode) - assignees, err := meReplacer.ReplaceSlice(opts.Assignees) + assigneeReplacer := prShared.NewSpecialAssigneeReplacer(apiClient, baseRepo.RepoHost(), issueFeatures.ActorIsAssignable, !opts.WebMode) + assignees, err := assigneeReplacer.ReplaceSlice(opts.Assignees) if err != nil { return err } - - // TODO actorIsAssignableCleanup - if issueFeatures.ActorIsAssignable { - assignees = copilotReplacer.ReplaceSlice(assignees) - } + assigneeSet := set.NewStringSet() assigneeSet.AddValues(assignees) tb := prShared.IssueMetadataState{ @@ -313,7 +308,11 @@ func createRun(opts *CreateOptions) (err error) { Repo: baseRepo, State: &tb, } - err = prShared.MetadataSurvey(opts.Prompter, opts.IO, baseRepo, fetcher, &tb, projectsV1Support, nil) + var assigneeSearchFunc func(string) prompter.MultiSelectSearchResult + if issueFeatures.ActorIsAssignable { + assigneeSearchFunc = prShared.RepoAssigneeSearchFunc(apiClient, baseRepo) + } + err = prShared.MetadataSurvey(opts.Prompter, opts.IO, baseRepo, fetcher, &tb, projectsV1Support, nil, assigneeSearchFunc) if err != nil { return } diff --git a/pkg/cmd/issue/create/create_test.go b/pkg/cmd/issue/create/create_test.go index 80c8f76d35b..57de4c9b579 100644 --- a/pkg/cmd/issue/create/create_test.go +++ b/pkg/cmd/issue/create/create_test.go @@ -495,12 +495,18 @@ func Test_createRun(t *testing.T) { switch message { case "What would you like to add?": return prompter.IndexesFor(options, "Assignees") - case "Assignees": - return prompter.IndexesFor(options, "Copilot (AI)", "MonaLisa (Mona Display Name)") default: return nil, fmt.Errorf("unexpected multi-select prompt: %s", message) } } + pm.MultiSelectWithSearchFunc = func(message, searchPrompt string, defaults, persistentOptions []string, searchFunc func(string) prompter.MultiSelectSearchResult) ([]string, error) { + switch message { + case "Assignees": + return []string{"copilot-swe-agent", "MonaLisa"}, nil + default: + return nil, fmt.Errorf("unexpected multi-select-with-search prompt: %s", message) + } + } pm.SelectFunc = func(message, defaultValue string, options []string) (int, error) { switch message { case "What's next?": @@ -524,25 +530,25 @@ func Test_createRun(t *testing.T) { "viewerPermission": "WRITE" } } } `)) - r.Register( - httpmock.GraphQL(`query RepositoryAssignableActors\b`), - httpmock.StringResponse(` - { "data": { "repository": { "suggestedActors": { - "nodes": [ - { "login": "copilot-swe-agent", "id": "COPILOTID", "name": "Copilot (AI)", "__typename": "Bot" }, - { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name", "__typename": "User" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) r.Register( httpmock.GraphQL(`mutation IssueCreate\b`), httpmock.GraphQLMutation(` { "data": { "createIssue": { "issue": { + "id": "ISSUEID", "URL": "https://github.com/OWNER/REPO/issues/12" } } } } `, func(inputs map[string]interface{}) { - assert.Equal(t, []interface{}{"COPILOTID", "MONAID"}, inputs["assigneeIds"]) + if v, ok := inputs["assigneeIds"]; ok { + t.Errorf("did not expect assigneeIds: %v", v) + } + })) + r.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "ISSUEID", inputs["assignableId"]) + assert.Equal(t, []interface{}{"copilot-swe-agent", "MonaLisa"}, inputs["actorLogins"]) })) }, wantsStdout: "https://github.com/OWNER/REPO/issues/12\n", @@ -948,16 +954,6 @@ func TestIssueCreate_metadata(t *testing.T) { defer http.Verify(t) http.StubRepoInfoResponse("OWNER", "REPO", "main") - http.Register( - httpmock.GraphQL(`query RepositoryAssignableActors\b`), - httpmock.StringResponse(` - { "data": { "repository": { "suggestedActors": { - "nodes": [ - { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name", "__typename": "User" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) http.Register( httpmock.GraphQL(`query RepositoryLabelList\b`), httpmock.StringResponse(` @@ -1030,12 +1026,15 @@ func TestIssueCreate_metadata(t *testing.T) { httpmock.GraphQL(`mutation IssueCreate\b`), httpmock.GraphQLMutation(` { "data": { "createIssue": { "issue": { + "id": "NEWISSUEID", "URL": "https://github.com/OWNER/REPO/issues/12" } } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "TITLE", inputs["title"]) assert.Equal(t, "BODY", inputs["body"]) - assert.Equal(t, []interface{}{"MONAID"}, inputs["assigneeIds"]) + if v, ok := inputs["assigneeIds"]; ok { + t.Errorf("did not expect assigneeIds: %v", v) + } assert.Equal(t, []interface{}{"BUGID", "TODOID"}, inputs["labelIds"]) assert.Equal(t, []interface{}{"ROADMAPID"}, inputs["projectIds"]) assert.Equal(t, "BIGONEID", inputs["milestoneId"]) @@ -1043,6 +1042,14 @@ func TestIssueCreate_metadata(t *testing.T) { assert.NotContains(t, inputs, "teamIds") assert.NotContains(t, inputs, "projectV2Ids") })) + http.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "NEWISSUEID", inputs["assignableId"]) + assert.Equal(t, []interface{}{"monalisa"}, inputs["actorLogins"]) + })) output, err := runCommand(http, true, `-t TITLE -b BODY -a monalisa -l bug -l todo -p roadmap -m 'big one.oh'`, nil) if err != nil { @@ -1091,27 +1098,27 @@ func TestIssueCreate_AtMeAssignee(t *testing.T) { "hasIssuesEnabled": true } } } `)) - http.Register( - httpmock.GraphQL(`query RepositoryAssignableActors\b`), - httpmock.StringResponse(` - { "data": { "repository": { "suggestedActors": { - "nodes": [ - { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name", "__typename": "User" }, - { "login": "SomeOneElse", "id": "SOMEID", "name": "Someone else", "__typename": "User" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) http.Register( httpmock.GraphQL(`mutation IssueCreate\b`), httpmock.GraphQLMutation(` { "data": { "createIssue": { "issue": { + "id": "NEWISSUEID", "URL": "https://github.com/OWNER/REPO/issues/12" } } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "hello", inputs["title"]) assert.Equal(t, "cash rules everything around me", inputs["body"]) - assert.Equal(t, []interface{}{"MONAID", "SOMEID"}, inputs["assigneeIds"]) + if v, ok := inputs["assigneeIds"]; ok { + t.Errorf("did not expect assigneeIds: %v", v) + } + })) + http.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "NEWISSUEID", inputs["assignableId"]) + assert.Equal(t, []interface{}{"MonaLisa", "someoneelse"}, inputs["actorLogins"]) })) output, err := runCommand(http, true, `-a @me -a someoneelse -t hello -b "cash rules everything around me"`, nil) @@ -1134,26 +1141,27 @@ func TestIssueCreate_AtCopilotAssignee(t *testing.T) { "hasIssuesEnabled": true } } } `)) - http.Register( - httpmock.GraphQL(`query RepositoryAssignableActors\b`), - httpmock.StringResponse(` - { "data": { "repository": { "suggestedActors": { - "nodes": [ - { "login": "copilot-swe-agent", "id": "COPILOTID", "name": "Copilot (AI)", "__typename": "Bot" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) http.Register( httpmock.GraphQL(`mutation IssueCreate\b`), httpmock.GraphQLMutation(` { "data": { "createIssue": { "issue": { + "id": "NEWISSUEID", "URL": "https://github.com/OWNER/REPO/issues/12" } } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "hello", inputs["title"]) assert.Equal(t, "cash rules everything around me", inputs["body"]) - assert.Equal(t, []interface{}{"COPILOTID"}, inputs["assigneeIds"]) + if v, ok := inputs["assigneeIds"]; ok { + t.Errorf("did not expect assigneeIds: %v", v) + } + })) + http.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "NEWISSUEID", inputs["assignableId"]) + assert.Equal(t, []interface{}{"copilot-swe-agent"}, inputs["actorLogins"]) })) output, err := runCommand(http, true, `-a @copilot -t hello -b "cash rules everything around me"`, nil) diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index 11921096a1d..2fd632b5ef7 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -248,6 +248,13 @@ func editRun(opts *EditOptions) error { // Fetch editable shared fields once for all issues. apiClient := api.NewClientFromHTTP(httpClient) + + // Wire up search function for assignees when ActorIsAssignable is available. + // Interactive mode only supports a single issue, so we use its ID for the search query. + if issueFeatures.ActorIsAssignable && opts.Interactive && len(issues) == 1 { + editable.AssigneeSearchFunc = prShared.AssigneeSearchFunc(apiClient, baseRepo, issues[0].ID) + } + opts.IO.StartProgressIndicatorWithLabel("Fetching repository information") err = opts.FetchOptions(apiClient, baseRepo, &editable, opts.Detector.ProjectsV1()) opts.IO.StopProgressIndicator() diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index 2fcb85c569a..ef496d1ded8 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -527,17 +527,6 @@ func Test_editRun(t *testing.T) { }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { // Should only be one fetch of metadata. - reg.Register( - httpmock.GraphQL(`query RepositoryAssignableActors\b`), - httpmock.StringResponse(` - { "data": { "repository": { "suggestedActors": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, - { "login": "MonaLisa", "id": "MONAID", "__typename": "User" } - ], - "pageInfo": { "hasNextPage": false, "endCursor": "Mg" } - } } } } - `)) reg.Register( httpmock.GraphQL(`query RepositoryMilestoneList\b`), httpmock.StringResponse(` @@ -640,8 +629,9 @@ func Test_editRun(t *testing.T) { require.Equal(t, []string{"hubot"}, eo.Assignees.Default) require.Equal(t, []string{"hubot"}, eo.Assignees.DefaultLogins) - // Adding MonaLisa as PR assignee, should preserve hubot. - eo.Assignees.Value = []string{"hubot", "MonaLisa (Mona Display Name)"} + // Adding MonaLisa as issue assignee, should preserve hubot. + // MultiSelectWithSearch returns Keys (logins), not display names. + eo.Assignees.Value = []string{"hubot", "MonaLisa"} return nil }, FetchOptions: prShared.FetchOptions, @@ -649,27 +639,13 @@ func Test_editRun(t *testing.T) { }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockIsssueNumberGetWithAssignedActors(t, reg, 123) - reg.Register( - httpmock.GraphQL(`query RepositoryAssignableActors\b`), - httpmock.StringResponse(` - { "data": { "repository": { "suggestedActors": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, - { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name", "__typename": "User" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) mockIssueUpdate(t, reg) reg.Register( httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), httpmock.GraphQLMutation(` { "data": { "replaceActorsForAssignable": { "__typename": "" } } }`, func(inputs map[string]interface{}) { - // Checking that despite the display name being returned - // from the EditFieldsSurvey, the ID is still - // used in the mutation. - require.Subset(t, inputs["actorIds"], []string{"MONAID", "HUBOTID"}) + require.Subset(t, inputs["actorLogins"], []interface{}{"hubot", "MonaLisa"}) }), ) }, @@ -839,18 +815,6 @@ func mockIssueProjectItemsGet(_ *testing.T, reg *httpmock.Registry) { } func mockRepoMetadata(_ *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.GraphQL(`query RepositoryAssignableActors\b`), - httpmock.StringResponse(` - { "data": { "repository": { "suggestedActors": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID", "__typename": "Bot" }, - { "login": "monalisa", "id": "MONAID", "name": "Mona Display Name", "__typename": "User" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) - reg.Register( httpmock.GraphQL(`query RepositoryLabelList\b`), httpmock.StringResponse(` diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 0dbdb3987b9..d8230dc3c68 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -406,6 +406,7 @@ func createRun(opts *CreateOptions) error { return err } var reviewerSearchFunc func(string) prompter.MultiSelectSearchResult + var assigneeSearchFunc func(string) prompter.MultiSelectSearchResult if issueFeatures.ActorIsAssignable { reviewerSearchFunc = func(query string) prompter.MultiSelectSearchResult { candidates, moreResults, err := api.SuggestedReviewerActorsForRepo(client, ctx.PRRefs.BaseRepo(), query) @@ -420,6 +421,7 @@ func createRun(opts *CreateOptions) error { } return prompter.MultiSelectSearchResult{Keys: keys, Labels: labels, MoreResults: moreResults} } + assigneeSearchFunc = shared.RepoAssigneeSearchFunc(client, ctx.PRRefs.BaseRepo()) } state, err := NewIssueState(*ctx, *opts) @@ -429,6 +431,7 @@ func createRun(opts *CreateOptions) error { if issueFeatures.ActorIsAssignable { state.ActorReviewers = true + state.ActorAssignees = true } var openURL string @@ -597,7 +600,7 @@ func createRun(opts *CreateOptions) error { Repo: ctx.PRRefs.BaseRepo(), State: state, } - err = shared.MetadataSurvey(opts.Prompter, opts.IO, ctx.PRRefs.BaseRepo(), fetcher, state, projectsV1Support, reviewerSearchFunc) + err = shared.MetadataSurvey(opts.Prompter, opts.IO, ctx.PRRefs.BaseRepo(), fetcher, state, projectsV1Support, reviewerSearchFunc, assigneeSearchFunc) if err != nil { return err } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index ed2a9cf2e63..fd53a92fcc7 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -916,17 +916,6 @@ func Test_createRun(t *testing.T) { return func() {} }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { - reg.Register( - httpmock.GraphQL(`query RepositoryAssignableUsers\b`), - httpmock.StringResponse(` - { "data": { "repository": { "assignableUsers": { - "nodes": [ - { "login": "hubot", "id": "HUBOTID", "name": "" }, - { "login": "MonaLisa", "id": "MONAID", "name": "Mona Display Name" } - ], - "pageInfo": { "hasNextPage": false } - } } } } - `)) reg.Register( httpmock.GraphQL(`query RepositoryLabelList\b`), httpmock.StringResponse(` @@ -975,11 +964,21 @@ func Test_createRun(t *testing.T) { } } } `, func(inputs map[string]interface{}) { assert.Equal(t, "NEWPULLID", inputs["pullRequestId"]) - assert.Equal(t, []interface{}{"MONAID"}, inputs["assigneeIds"]) + if _, ok := inputs["assigneeIds"]; ok { + t.Error("did not expect assigneeIds in updatePullRequest when ActorAssignees is true") + } assert.Equal(t, []interface{}{"BUGID", "TODOID"}, inputs["labelIds"]) assert.Equal(t, []interface{}{"ROADMAPID"}, inputs["projectIds"]) assert.Equal(t, "BIGONEID", inputs["milestoneId"]) })) + reg.Register( + httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`), + httpmock.GraphQLMutation(` + { "data": { "replaceActorsForAssignable": { "__typename": "" } } } + `, func(inputs map[string]interface{}) { + assert.Equal(t, "NEWPULLID", inputs["assignableId"]) + assert.Equal(t, []interface{}{"monalisa"}, inputs["actorLogins"]) + })) reg.Register( httpmock.GraphQL(`mutation RequestReviewsByLogin\b`), httpmock.GraphQLMutation(` diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index 62811fe0c95..12c197b5af6 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -322,7 +322,7 @@ func editRun(opts *EditOptions) error { // to legacy reviewer/assignee fetching. // TODO actorIsAssignableCleanup if issueFeatures.ActorIsAssignable { - editable.AssigneeSearchFunc = assigneeSearchFunc(apiClient, repo, &editable, pr.ID) + editable.AssigneeSearchFunc = shared.AssigneeSearchFunc(apiClient, repo, pr.ID) editable.ReviewerSearchFunc = reviewerSearchFunc(apiClient, repo, &editable, pr.ID) } @@ -365,57 +365,6 @@ func editRun(opts *EditOptions) error { return nil } -// assigneeSearchFunc is intended to be an arg for MultiSelectWithSearch -// to return potential assignee actors. -// It also contains an important enclosure to update the editable's -// assignable actors metadata for later ID resolution - this is required -// while we continue to use IDs for mutating assignees with the GQL API. -func assigneeSearchFunc(apiClient *api.Client, repo ghrepo.Interface, editable *shared.Editable, assignableID string) func(string) prompter.MultiSelectSearchResult { - searchFunc := func(input string) prompter.MultiSelectSearchResult { - actors, availableAssigneesCount, err := api.SuggestedAssignableActors( - apiClient, - repo, - assignableID, - input) - if err != nil { - return prompter.MultiSelectSearchResult{ - Keys: nil, - Labels: nil, - MoreResults: 0, - Err: err, - } - } - - logins := make([]string, 0, len(actors)) - displayNames := make([]string, 0, len(actors)) - - for _, a := range actors { - if a.Login() != "" { - logins = append(logins, a.Login()) - } else { - continue - } - - if a.DisplayName() != "" { - displayNames = append(displayNames, a.DisplayName()) - } else { - displayNames = append(displayNames, a.Login()) - } - - // Update the assignable actors metadata in the editable struct - // so that updating the PR later can resolve the actor ID. - editable.Metadata.AssignableActors = append(editable.Metadata.AssignableActors, a) - } - return prompter.MultiSelectSearchResult{ - Keys: logins, - Labels: displayNames, - MoreResults: availableAssigneesCount, - Err: nil, - } - } - return searchFunc -} - // reviewerSearchFunc is intended to be an arg for MultiSelectWithSearch // to return potential reviewer candidates (users, bots, and teams). // It also updates the editable's metadata for later ID resolution. diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index e38fac3caa0..01acb7e0fba 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -415,7 +415,7 @@ func Test_editRun(t *testing.T) { httpStubs: func(t *testing.T, reg *httpmock.Registry) { // Non-interactive with Add/Remove doesn't need reviewers/assignees metadata // REST API accepts logins and team slugs directly - mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true}) + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: false, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestUpdateActorAssignees(reg) mockRequestReviewsByLogin(reg) @@ -473,7 +473,7 @@ func Test_editRun(t *testing.T) { Fetcher: testFetcher{}, }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { - mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: true, labels: true, projects: true, milestones: true}) + mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: false, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockPullRequestUpdateActorAssignees(reg) mockPullRequestUpdateLabels(reg) @@ -547,7 +547,7 @@ func Test_editRun(t *testing.T) { }, httpStubs: func(t *testing.T, reg *httpmock.Registry) { // Non-interactive with Remove doesn't need reviewers metadata - mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true}) + mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: false, labels: true, projects: true, milestones: true}) mockPullRequestUpdate(reg) mockRequestReviewsByLogin(reg) mockPullRequestUpdateLabels(reg) @@ -912,12 +912,8 @@ func Test_editRun(t *testing.T) { require.Equal(t, []string{"hubot"}, e.Assignees.DefaultLogins) // Adding monalisa as PR assignee, should preserve hubot. - e.Assignees.Value = []string{"hubot", "monalisa (Mona Display Name)"} - // Populate metadata to simulate what searchFunc would do during prompting - e.Metadata.AssignableActors = []api.AssignableActor{ - api.NewAssignableBot("HUBOTID", "hubot"), - api.NewAssignableUser("MONAID", "monalisa", "Mona Display Name"), - } + // MultiSelectWithSearch returns Keys (logins), not display names. + e.Assignees.Value = []string{"hubot", "monalisa"} return nil }, }, @@ -931,10 +927,7 @@ func Test_editRun(t *testing.T) { httpmock.GraphQLMutation(` { "data": { "replaceActorsForAssignable": { "__typename": "" } } }`, func(inputs map[string]interface{}) { - // Checking that despite the display name being returned - // from the EditFieldsSurvey, the ID is still - // used in the mutation. - require.Subset(t, inputs["actorIds"], []string{"MONAID", "HUBOTID"}) + require.Subset(t, inputs["actorLogins"], []interface{}{"hubot", "monalisa"}) }), ) }, diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 1b7c42be3b6..02fe5d33c50 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -95,22 +95,7 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str // If assignees came in from command line flags, we need to // curate the final list of assignees from the default list. if len(e.Assignees.Add) != 0 || len(e.Assignees.Remove) != 0 { - meReplacer := NewMeReplacer(client, repo.RepoHost()) - copilotReplacer := NewCopilotReplacer(true) - - replaceSpecialAssigneeNames := func(value []string) ([]string, error) { - replaced, err := meReplacer.ReplaceSlice(value) - if err != nil { - return nil, err - } - - // Only suppported for actor assignees. - if e.Assignees.ActorAssignees { - replaced = copilotReplacer.ReplaceSlice(replaced) - } - - return replaced, nil - } + replacer := NewSpecialAssigneeReplacer(client, repo.RepoHost(), e.Assignees.ActorAssignees, true) assigneeSet := set.NewStringSet() @@ -128,13 +113,13 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str assigneeSet.AddValues(e.Assignees.Default) } - add, err := replaceSpecialAssigneeNames(e.Assignees.Add) + add, err := replacer.ReplaceSlice(e.Assignees.Add) if err != nil { return nil, err } assigneeSet.AddValues(add) - remove, err := replaceSpecialAssigneeNames(e.Assignees.Remove) + remove, err := replacer.ReplaceSlice(e.Assignees.Remove) if err != nil { return nil, err } @@ -146,6 +131,70 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str return &a, err } +// AssigneeLogins computes the final list of assignee logins from the current +// defaults plus any Add/Remove operations. Unlike AssigneeIds, this does not +// resolve logins to node IDs, and is used on github.com where the +// ReplaceActorsForAssignable mutation accepts logins directly. +func (e Editable) AssigneeLogins(client *api.Client, repo ghrepo.Interface) ([]string, error) { + if !e.Assignees.Edited { + return nil, nil + } + + if len(e.Assignees.Add) != 0 || len(e.Assignees.Remove) != 0 { + replacer := NewSpecialAssigneeReplacer(client, repo.RepoHost(), true, true) + + assigneeSet := set.NewStringSet() + assigneeSet.AddValues(e.Assignees.DefaultLogins) + + add, err := replacer.ReplaceSlice(e.Assignees.Add) + if err != nil { + return nil, err + } + assigneeSet.AddValues(add) + + remove, err := replacer.ReplaceSlice(e.Assignees.Remove) + if err != nil { + return nil, err + } + assigneeSet.RemoveValues(remove) + + e.Assignees.Value = assigneeSet.ToSlice() + } + + return e.Assignees.Value, nil +} + +// SpecialAssigneeReplacer expands special assignee names (@me, Copilot actors) +// in login slices. Use NewSpecialAssigneeReplacer to create one. +type SpecialAssigneeReplacer struct { + meReplacer *MeReplacer + copilotReplacer *CopilotReplacer + actorAssignees bool +} + +// NewSpecialAssigneeReplacer creates a replacer that expands @me and (when +// actorAssignees is true) Copilot actor names in assignee slices. +// copilotUseLogin controls whether Copilot actors are replaced with their +// login (true) or display name (false, used for web mode). +func NewSpecialAssigneeReplacer(client *api.Client, host string, actorAssignees bool, copilotUseLogin bool) *SpecialAssigneeReplacer { + return &SpecialAssigneeReplacer{ + meReplacer: NewMeReplacer(client, host), + copilotReplacer: NewCopilotReplacer(copilotUseLogin), + actorAssignees: actorAssignees, + } +} + +func (r *SpecialAssigneeReplacer) ReplaceSlice(logins []string) ([]string, error) { + replaced, err := r.meReplacer.ReplaceSlice(logins) + if err != nil { + return nil, err + } + if r.actorAssignees { + replaced = r.copilotReplacer.ReplaceSlice(replaced) + } + return replaced, nil +} + // ProjectIds returns a slice containing IDs of projects v1 that the issue or a PR has to be linked to. func (e Editable) ProjectIds() (*[]string, error) { if !e.Projects.Edited { @@ -224,14 +273,16 @@ func (e Editable) MilestoneId() (*string, error) { // go routines. Fields that would be mutated will be copied. func (e *Editable) Clone() Editable { return Editable{ - Title: e.Title.clone(), - Body: e.Body.clone(), - Base: e.Base.clone(), - Reviewers: e.Reviewers.clone(), - Assignees: e.Assignees.clone(), - Labels: e.Labels.clone(), - Projects: e.Projects.clone(), - Milestone: e.Milestone.clone(), + Title: e.Title.clone(), + Body: e.Body.clone(), + Base: e.Base.clone(), + Reviewers: e.Reviewers.clone(), + ReviewerSearchFunc: e.ReviewerSearchFunc, + Assignees: e.Assignees.clone(), + AssigneeSearchFunc: e.AssigneeSearchFunc, + Labels: e.Labels.clone(), + Projects: e.Projects.clone(), + Milestone: e.Milestone.clone(), // Shallow copy since no mutation. Metadata: e.Metadata, } @@ -470,11 +521,10 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable, if len(editable.Assignees.Add) == 0 && len(editable.Assignees.Remove) == 0 && editable.AssigneeSearchFunc == nil { fetchAssignees = true } - // However, if we have Add/Remove operations (non-interactive flow), - // we do need to fetch the assignees. - // TODO: KW noninteractive assignees need to migrate to directly use - // new logins input with ReplaceActorsForAssignable to prevent fetching. - if len(editable.Assignees.Add) > 0 || len(editable.Assignees.Remove) > 0 { + // For non-interactive Add/Remove operations, we only need to fetch assignees + // on GHES where ID resolution is required. On github.com (ActorAssignees), + // logins are passed directly to the mutation. + if (len(editable.Assignees.Add) > 0 || len(editable.Assignees.Remove) > 0) && !editable.Assignees.ActorAssignees { fetchAssignees = true } } @@ -567,3 +617,50 @@ func milestoneSurvey(p EditPrompter, title string, opts []string) (result string result = opts[selected] return } + +// AssigneeSearchFunc returns a search function for MultiSelectWithSearch that +// dynamically fetches assignable actors for the given assignable (Issue/PR) node ID. +func AssigneeSearchFunc(apiClient *api.Client, repo ghrepo.Interface, assignableID string) func(string) prompter.MultiSelectSearchResult { + return func(input string) prompter.MultiSelectSearchResult { + actors, count, err := api.SuggestedAssignableActors(apiClient, repo, assignableID, input) + if err != nil { + return prompter.MultiSelectSearchResult{Err: err} + } + return actorsToSearchResult(actors, count) + } +} + +// RepoAssigneeSearchFunc returns a search function for MultiSelectWithSearch that +// dynamically fetches assignable actors at the repository level. Used during create +// flows where no issue/PR node ID exists yet. +func RepoAssigneeSearchFunc(apiClient *api.Client, repo ghrepo.Interface) func(string) prompter.MultiSelectSearchResult { + return func(input string) prompter.MultiSelectSearchResult { + actors, count, err := api.SearchRepoAssignableActors(apiClient, repo, input) + if err != nil { + return prompter.MultiSelectSearchResult{Err: err} + } + return actorsToSearchResult(actors, count) + } +} + +func actorsToSearchResult(actors []api.AssignableActor, totalCount int) prompter.MultiSelectSearchResult { + logins := make([]string, 0, len(actors)) + displayNames := make([]string, 0, len(actors)) + + for _, a := range actors { + if a.Login() == "" { + continue + } + logins = append(logins, a.Login()) + if a.DisplayName() != "" { + displayNames = append(displayNames, a.DisplayName()) + } else { + displayNames = append(displayNames, a.Login()) + } + } + return prompter.MultiSelectSearchResult{ + Keys: logins, + Labels: displayNames, + MoreResults: totalCount, + } +} diff --git a/pkg/cmd/pr/shared/editable_http.go b/pkg/cmd/pr/shared/editable_http.go index 8cd51c34942..bb63ceb0f62 100644 --- a/pkg/cmd/pr/shared/editable_http.go +++ b/pkg/cmd/pr/shared/editable_http.go @@ -68,12 +68,12 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR // https://github.com/cli/cli/pull/10960#discussion_r2086725348 if options.Assignees.Edited && options.Assignees.ActorAssignees { apiClient := api.NewClientFromHTTP(httpClient) - assigneeIds, err := options.AssigneeIds(apiClient, repo) + logins, err := options.AssigneeLogins(apiClient, repo) if err != nil { return err } - err = replaceActorAssigneesForEditable(apiClient, repo, id, assigneeIds) + err = api.ReplaceActorsForAssignableByLogin(apiClient, repo, id, logins) if err != nil { return err } @@ -90,32 +90,6 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR return wg.Wait() } -func replaceActorAssigneesForEditable(apiClient *api.Client, repo ghrepo.Interface, id string, assigneeIds *[]string) error { - type ReplaceActorsForAssignableInput struct { - AssignableID githubv4.ID `json:"assignableId"` - ActorIDs []githubv4.ID `json:"actorIds"` - } - - params := ReplaceActorsForAssignableInput{ - AssignableID: githubv4.ID(id), - ActorIDs: *ghIds(assigneeIds), - } - - var mutation struct { - ReplaceActorsForAssignable struct { - TypeName string `graphql:"__typename"` - } `graphql:"replaceActorsForAssignable(input: $input)"` - } - - variables := map[string]interface{}{"input": params} - err := apiClient.Mutate(repo.RepoHost(), "ReplaceActorsForAssignable", &mutation, variables) - if err != nil { - return err - } - - return nil -} - func replaceIssueFields(httpClient *http.Client, repo ghrepo.Interface, id string, isPR bool, options Editable) error { apiClient := api.NewClientFromHTTP(httpClient) diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index d5c168e5f7f..17d40d6395c 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -64,6 +64,9 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par // When ActorReviewers is true, we use login-based mutation and don't need to resolve reviewer IDs. needReviewerIDs := len(tb.Reviewers) > 0 && !tb.ActorReviewers + // When ActorAssignees is true, we use login-based mutation and don't need to resolve assignee IDs. + needAssigneeIDs := len(tb.Assignees) > 0 && !tb.ActorAssignees + // Retrieve minimal information needed to resolve metadata if this was not previously cached from additional metadata survey. if tb.MetadataResult == nil { input := api.RepoMetadataInput{ @@ -71,12 +74,11 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par TeamReviewers: needReviewerIDs && slices.ContainsFunc(tb.Reviewers, func(r string) bool { return strings.ContainsRune(r, '/') }), - Assignees: len(tb.Assignees) > 0, - ActorAssignees: tb.ActorAssignees, - Labels: len(tb.Labels) > 0, - ProjectsV1: len(tb.ProjectTitles) > 0 && projectV1Support == gh.ProjectsV1Supported, - ProjectsV2: len(tb.ProjectTitles) > 0, - Milestones: len(tb.Milestones) > 0, + Assignees: needAssigneeIDs, + Labels: len(tb.Labels) > 0, + ProjectsV1: len(tb.ProjectTitles) > 0 && projectV1Support == gh.ProjectsV1Supported, + ProjectsV2: len(tb.ProjectTitles) > 0, + Milestones: len(tb.Milestones) > 0, } metadataResult, err := api.RepoMetadata(client, baseRepo, input) @@ -86,11 +88,17 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par tb.MetadataResult = metadataResult } - assigneeIDs, err := tb.MetadataResult.MembersToIDs(tb.Assignees) - if err != nil { - return fmt.Errorf("could not assign user: %w", err) + // When ActorAssignees is true (github.com), pass logins directly for use with + // ReplaceActorsForAssignable mutation. The ID-based else branch is for GHES compatibility. + if tb.ActorAssignees { + params["assigneeLogins"] = tb.Assignees + } else { + assigneeIDs, err := tb.MetadataResult.MembersToIDs(tb.Assignees) + if err != nil { + return fmt.Errorf("could not assign user: %w", err) + } + params["assigneeIds"] = assigneeIDs } - params["assigneeIds"] = assigneeIDs labelIDs, err := tb.MetadataResult.LabelsToIDs(tb.Labels) if err != nil { diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index cc66bbe5c4d..71cfcd06343 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -154,7 +154,7 @@ type RepoMetadataFetcher interface { RepoMetadataFetch(api.RepoMetadataInput) (*api.RepoMetadataResult, error) } -func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface, fetcher RepoMetadataFetcher, state *IssueMetadataState, projectsV1Support gh.ProjectsV1Support, reviewerSearchFunc func(string) prompter.MultiSelectSearchResult) error { +func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface, fetcher RepoMetadataFetcher, state *IssueMetadataState, projectsV1Support gh.ProjectsV1Support, reviewerSearchFunc func(string) prompter.MultiSelectSearchResult, assigneeSearchFunc func(string) prompter.MultiSelectSearchResult) error { isChosen := func(m string) bool { for _, c := range state.Metadata { if m == c { @@ -184,11 +184,12 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface // When search-based reviewer selection is available, skip the expensive assignable-users // and teams fetch since reviewers are found dynamically via the search function. useReviewerSearch := state.ActorReviewers && reviewerSearchFunc != nil + useAssigneeSearch := state.ActorAssignees && assigneeSearchFunc != nil metadataInput := api.RepoMetadataInput{ Reviewers: isChosen("Reviewers") && !useReviewerSearch, TeamReviewers: isChosen("Reviewers") && !useReviewerSearch, - Assignees: isChosen("Assignees"), - ActorAssignees: isChosen("Assignees") && state.ActorAssignees, + Assignees: isChosen("Assignees") && !useAssigneeSearch, + ActorAssignees: isChosen("Assignees") && !useAssigneeSearch && state.ActorAssignees, Labels: isChosen("Labels"), ProjectsV1: isChosen("Projects") && projectsV1Support == gh.ProjectsV1Supported, ProjectsV2: isChosen("Projects"), @@ -212,24 +213,25 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface } // Populate the list of selectable assignees and their default selections. - // This logic maps the default assignees from `state` to the corresponding actors or users - // so that the correct display names are preselected in the prompt. + // When search-based selection is available, skip building the static list. var assignees []string var assigneesDefault []string - if state.ActorAssignees { - for _, u := range metadataResult.AssignableActors { - assignees = append(assignees, u.DisplayName()) + if !useAssigneeSearch { + if state.ActorAssignees { + for _, u := range metadataResult.AssignableActors { + assignees = append(assignees, u.DisplayName()) - if slices.Contains(state.Assignees, u.Login()) { - assigneesDefault = append(assigneesDefault, u.DisplayName()) + if slices.Contains(state.Assignees, u.Login()) { + assigneesDefault = append(assigneesDefault, u.DisplayName()) + } } - } - } else { - for _, u := range metadataResult.AssignableUsers { - assignees = append(assignees, u.DisplayName()) + } else { + for _, u := range metadataResult.AssignableUsers { + assignees = append(assignees, u.DisplayName()) - if slices.Contains(state.Assignees, u.Login()) { - assigneesDefault = append(assigneesDefault, u.DisplayName()) + if slices.Contains(state.Assignees, u.Login()) { + assigneesDefault = append(assigneesDefault, u.DisplayName()) + } } } } @@ -286,16 +288,23 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface } } if isChosen("Assignees") { - if len(assignees) > 0 { + if useAssigneeSearch { + selectedAssignees, err := p.MultiSelectWithSearch( + "Assignees", + "Search assignees", + state.Assignees, + []string{}, + assigneeSearchFunc) + if err != nil { + return err + } + values.Assignees = selectedAssignees + } else if len(assignees) > 0 { selected, err := p.MultiSelect("Assignees", assigneesDefault, assignees) if err != nil { return err } for _, i := range selected { - // Previously, this logic relied upon `assignees` being in `` or ` ()` form, - // however the inclusion of actors breaks this convention. - // Instead, we map the selected indexes to the source that populated `assignees` rather than - // relying on parsing the information out. if state.ActorAssignees { values.Assignees = append(values.Assignees, metadataResult.AssignableActors[i].Login()) } else { diff --git a/pkg/cmd/pr/shared/survey_test.go b/pkg/cmd/pr/shared/survey_test.go index 23ba96ceff7..384e5489500 100644 --- a/pkg/cmd/pr/shared/survey_test.go +++ b/pkg/cmd/pr/shared/survey_test.go @@ -71,7 +71,7 @@ func TestMetadataSurvey_selectAll(t *testing.T) { Assignees: []string{"hubot"}, Type: PRMetadata, } - err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported, nil) + err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported, nil, nil) assert.NoError(t, err) assert.Equal(t, "", stdout.String()) @@ -117,7 +117,7 @@ func TestMetadataSurvey_keepExisting(t *testing.T) { Assignees: []string{"hubot"}, } - err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported, nil) + err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported, nil, nil) assert.NoError(t, err) assert.Equal(t, "", stdout.String()) @@ -146,7 +146,7 @@ func TestMetadataSurveyProjectV1Deprecation(t *testing.T) { return []int{0}, nil }) - err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Supported, nil) + err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Supported, nil, nil) require.ErrorContains(t, err, "expected test error") require.True(t, fetcher.projectsV1Requested, "expected projectsV1 to be requested") @@ -167,7 +167,7 @@ func TestMetadataSurveyProjectV1Deprecation(t *testing.T) { return []int{0}, nil }) - err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Unsupported, nil) + err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Unsupported, nil, nil) require.ErrorContains(t, err, "expected test error") require.False(t, fetcher.projectsV1Requested, "expected projectsV1 not to be requested") diff --git a/pkg/cmd/search/shared/shared_test.go b/pkg/cmd/search/shared/shared_test.go index 0700a688e9b..c66e0908f52 100644 --- a/pkg/cmd/search/shared/shared_test.go +++ b/pkg/cmd/search/shared/shared_test.go @@ -15,7 +15,7 @@ import ( ) func TestSearcher(t *testing.T) { - f := factory.New("1") + f := factory.New("1", "") f.Config = func() (gh.Config, error) { return config.NewBlankConfig(), nil }