fix: address verified review findings (completion-target-ref-1)

Signed-off-by: bircni <bircni@icloud.com>

docs/migration-1.0.md

@@ -150,9 +150,10 @@ tea 0.16 cloned over SSH when the login had an SSH key; tea 1.0 follows
 | `--debug` / `--vvv` | `--debug` |
 | `--confirm` / `-y`, `--force` / `-f` on destructive commands | `--yes` / `-y` |
 
-`-R` now means `--repo`: `tea issue list -R origin` fails with a usage error
-(`did you mean --remote origin?`) instead of selecting the `origin` remote. Use
-`--remote origin`.
+`-R` now means `--repo`, which takes `NAME`, `OWNER/REPO`, `HOST/OWNER/REPO` or
+a URL: `tea issue list -R origin` no longer selects the `origin` remote. Inside
+a checkout with that remote tea fails and suggests `--remote origin`; elsewhere
+`origin` is read as a repository of your account. Use `--remote origin`.
 
 New global flags: `--no-input` disables prompts, editors and browsers, `--yes`
 (`-y`) accepts confirmations, `--cwd` selects the directory used for repository

internal/app/action_runs.go

@@ -716,7 +716,7 @@ func (a *application) completeJobs() cobra.CompletionFunc {
 		if err != nil {
 			return nil, cobra.ShellCompDirectiveNoFileComp
 		}
-		return a.apiCompletion(true, func(ctx context.Context, t target, _ string) ([]cobra.Completion, error) {
+		return a.apiCompletionIn(run.Repo, true, func(ctx context.Context, t target, _ string) ([]cobra.Completion, error) {
 			jobs, err := services.Actions{Client: t.client, Repository: t.Repo}.Jobs(ctx, run.Number)
 			return describe(jobs, func(job services.Job) (string, string) {
 				return strconv.FormatInt(job.ID, 10), job.Name + " · " + cmp.Or(job.Conclusion, job.Status)

internal/app/alias_completion_test.go

@@ -116,6 +116,9 @@ func TestCompletionOffersPathsOnlyWherePathsAreExpected(t *testing.T) {
 		"issue list --state ":            cobra.ShellCompDirectiveNoFileComp,
 		"issue create --body-file ":      cobra.ShellCompDirectiveDefault,
 		"issue create --attachment ":     cobra.ShellCompDirectiveDefault,
+		"label export --output ":         cobra.ShellCompDirectiveDefault,
+		"run download 5 --output ":       cobra.ShellCompDirectiveDefault,
+		"config set ssh_key ":            cobra.ShellCompDirectiveDefault,
 		"api --input ":                   cobra.ShellCompDirectiveDefault,
 		"auth login --ssh-key ":          cobra.ShellCompDirectiveDefault,
 		"ssh-key create ":                cobra.ShellCompDirectiveDefault,
@@ -157,14 +160,14 @@ func TestCompletionOffersPathsOnlyWherePathsAreExpected(t *testing.T) {
 func TestOfflineCompletions(t *testing.T) {
 	deps, out, _ := testDeps(t)
 	deps.Git = &fakeGit{results: map[string]string{
-		`config --get-regexp ^remote\..*\.url$`:             "remote.origin.url https://gitea.example.com/alice/project.git\nremote.my.fork.url git@gitea.example.com:bob/project.git\n",
+		`config --get-regexp ^remote\..*\.url$`:             "remote.origin.url https://gitea.example.com/alice/project.git\nremote.my.fork.url git@gitea.example.com:bob/project.git\nremote.ci.url https://bot:s3cret@gitea.example.com/alice/project.git?token=x\n",
 		"for-each-ref --format=%(refname:short) refs/heads": "main\nfeature/login\n",
 	}}
 	configureAccounts(t, deps, "https://gitea.example.com", "alice", "alice", "bob")
 	for args, want := range map[string][]string{
 		"--host ":   {"https://gitea.example.com\talice, bob"},
 		"--user ":   {"alice\thttps://gitea.example.com (active)", "bob\thttps://gitea.example.com"},
-		"--remote ": {"origin\thttps://gitea.example.com/alice/project.git", "my.fork\tgit@gitea.example.com:bob/project.git"},
+		"--remote ": {"origin\thttps://gitea.example.com/alice/project.git", "my.fork\tgit@gitea.example.com:bob/project.git", "ci\thttps://gitea.example.com/alice/project.git"},
 	} {
 		values, directive := complete(t, deps, out, strings.Split(args, " ")...)
 		require.Equal(t, want, values, args)
@@ -217,6 +220,68 @@ func TestRepositoryCompletionUsesRemotesAndInstance(t *testing.T) {
 	require.Contains(t, tokens, "token alice-token")
 }
 
+func TestLabelCompletionDiscoversTheRepository(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/repos/alice/project/labels":
+			_, _ = w.Write([]byte(`[{"name":"bug","description":"Broken"},{"name":"docs"}]`))
+		case "/api/v1/users/alice":
+			_, _ = w.Write([]byte(`{"id":1,"login":"alice"}`))
+		default:
+			w.WriteHeader(http.StatusNotFound)
+		}
+	}))
+	defer server.Close()
+	deps, out, _ := testDeps(t)
+	deps.Git = &fakeGit{results: map[string]string{
+		"remote": "origin", "remote get-url origin": server.URL + "/alice/project.git",
+	}}
+	configureAccounts(t, deps, server.URL, "alice", "alice")
+	values, _ := complete(t, deps, out, "label", "delete", "")
+	require.Equal(t, []string{"bug\tBroken", "docs"}, values)
+}
+
+func TestJobCompletionUsesTheRepositoryOfTheRunURL(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/repos/bob/tool/actions/runs/44/jobs":
+			if r.URL.Query().Get("page") == "1" {
+				_, _ = w.Write([]byte(`{"jobs":[{"id":9,"name":"Build","status":"success"}]}`))
+			} else {
+				_, _ = w.Write([]byte(`{"jobs":[]}`))
+			}
+		case "/api/v1/users/alice":
+			_, _ = w.Write([]byte(`{"id":1,"login":"alice"}`))
+		default:
+			w.WriteHeader(http.StatusNotFound)
+		}
+	}))
+	defer server.Close()
+	deps, out, _ := testDeps(t)
+	deps.Git = &fakeGit{results: map[string]string{"remote": "origin", "remote get-url origin": server.URL + "/alice/project.git"}}
+	configureAccounts(t, deps, server.URL, "alice", "alice")
+	values, _ := complete(t, deps, out, "run", "logs", server.URL+"/bob/tool/actions/runs/44", "--job", "")
+	require.Equal(t, []string{"9\tBuild · success"}, values)
+}
+
+func TestActionValueCompletionNeedsNoRepositoryForAnOrganization(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/orgs/acme/actions/variables":
+			_, _ = w.Write([]byte(`[{"name":"MODE","description":"Build mode"}]`))
+		case "/api/v1/users/alice":
+			_, _ = w.Write([]byte(`{"id":1,"login":"alice"}`))
+		default:
+			w.WriteHeader(http.StatusNotFound)
+		}
+	}))
+	defer server.Close()
+	deps, out, _ := testDeps(t)
+	configureAccounts(t, deps, server.URL, "alice", "alice")
+	values, _ := complete(t, deps, out, "variable", "delete", "--org", "acme", "")
+	require.Equal(t, []string{"MODE\tBuild mode"}, values)
+}
+
 func TestAPICompletionNeverRefreshesOrFails(t *testing.T) {
 	requests := 0
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
@@ -297,7 +362,8 @@ func TestInstanceCompletions(t *testing.T) {
 		directive cobra.ShellCompDirective
 	}{
 		{"pulls by prefix", a.completePulls("open"), nil, "1", []string{"13\tAdd login support", "17\tImprove greeting"}, cobra.ShellCompDirectiveNoFileComp},
-		{"pulls skip given arguments and sanitize titles", a.completePulls("open"), []string{"13", "17"}, "", []string{"2\tFix]0;pwned docs"}, cobra.ShellCompDirectiveNoFileComp},
+		{"pulls skip given arguments and sanitize titles", withoutGiven(a.completePulls("open")), []string{"13", "17"}, "", []string{"2\tFix]0;pwned docs"}, cobra.ShellCompDirectiveNoFileComp},
+		{"flag values may equal a positional argument", a.completePulls("open"), []string{"13"}, "13", []string{"13\tAdd login support"}, cobra.ShellCompDirectiveNoFileComp},
 		{"labels are comma-separated", a.completeLabels(), nil, "bug,", []string{"bug,docs"}, cobra.ShellCompDirectiveNoSpace | cobra.ShellCompDirectiveNoFileComp},
 		{"labels with descriptions", a.completeLabels(), nil, "b", []string{"bug\tSomething is broken"}, cobra.ShellCompDirectiveNoSpace | cobra.ShellCompDirectiveNoFileComp},
 		{"users", a.completeUsers(), nil, "", []string{"alice\tAlice", "bob"}, cobra.ShellCompDirectiveNoSpace | cobra.ShellCompDirectiveNoFileComp},
@@ -331,7 +397,7 @@ func TestCompletionTargetAcceptsABareTokenHost(t *testing.T) {
 	require.NoError(t, testManager(t, deps).Register(instance, "alice", "file", "token", auth.HostOptions{}, auth.Credential{AccessToken: "stored-token"}))
 	deps.Getenv = envFunc(map[string]string{"TEA_HOST": "127.0.0.1:3999", "GITEA_TOKEN": "environment-token"})
 	a := &application{deps: deps}
-	resolved, err := a.completionTarget(context.Background(), false)
+	resolved, err := a.completionTarget(context.Background(), "", false)
 	require.NoError(t, err)
 	require.Equal(t, instance, resolved.Host)
 }

internal/app/artifacts.go

@@ -295,7 +295,7 @@ func (a *application) completeArtifactNames() cobra.CompletionFunc {
 		if err != nil {
 			return nil, cobra.ShellCompDirectiveNoFileComp
 		}
-		return a.apiCompletion(true, func(ctx context.Context, t target, _ string) ([]cobra.Completion, error) {
+		return a.apiCompletionIn(run.Repo, true, func(ctx context.Context, t target, _ string) ([]cobra.Completion, error) {
 			artifacts, err := services.Actions{Client: t.client, Repository: t.Repo}.AllArtifacts(ctx, run.Number)
 			return describe(artifacts, func(artifact services.Artifact) (string, string) { return artifact.Name, terminal.Bytes(artifact.Size) }), err
 		})(cmd, args, toComplete)

internal/app/completion.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"errors"
 	"maps"
+	"net/url"
 	"slices"
 	"strconv"
 	"strings"
@@ -228,7 +229,21 @@ func (a *application) gitRemotes(ctx context.Context) [][2]string {
 func (a *application) completeRemotes(cmd *cobra.Command, _ []string, toComplete string) ([]cobra.Completion, cobra.ShellCompDirective) {
 	ctx, cancel := completionContext(cmd)
 	defer cancel()
-	return matching(describe(a.gitRemotes(ctx), func(remote [2]string) (string, string) { return remote[0], remote[1] }), toComplete), cobra.ShellCompDirectiveNoFileComp
+	return matching(describe(a.gitRemotes(ctx), func(remote [2]string) (string, string) { return remote[0], withoutCredentials(remote[1]) }), toComplete), cobra.ShellCompDirectiveNoFileComp
+}
+
+// withoutCredentials drops the user info and query of a remote URL, which can embed a password or
+// token that the shell would print in its menu. An unparsable URL shows nothing.
+func withoutCredentials(raw string) string {
+	if !strings.Contains(raw, "://") {
+		return raw // scp-like addresses carry no password.
+	}
+	u, err := url.Parse(raw)
+	if err != nil {
+		return ""
+	}
+	u.User, u.RawQuery, u.Fragment = nil, "", ""
+	return u.String()
 }
 
 // completeLocalBranches completes the branches of the local checkout.
@@ -264,18 +279,25 @@ func (a *application) completeConfig(cmd *cobra.Command, args []string, toComple
 		return a.completeHosts(cmd, args, toComplete)
 	case args[0] == "remote":
 		return a.completeRemotes(cmd, args, toComplete)
+	case args[0] == "ssh_key":
+		return completeFiles(cmd, args, toComplete)
 	}
 	return matching(configValues[args[0]], toComplete), cobra.ShellCompDirectiveNoFileComp
 }
 
-// apiCompletion completes the values that list fetches from the selected instance, except those
-// already given. It never prompts, refreshes or retries, and failures complete nothing rather than
-// file names.
+// apiCompletion completes the values that list fetches from the selected instance. It never
+// prompts, refreshes or retries, and failures complete nothing rather than file names.
 func (a *application) apiCompletion(requireRepo bool, list func(ctx context.Context, t target, toComplete string) ([]cobra.Completion, error)) cobra.CompletionFunc {
+	return a.apiCompletionIn("", requireRepo, list)
+}
+
+// apiCompletionIn is apiCompletion for the repository that a command argument names, such as the
+// one of a run URL; an empty repo selects the repository like any other command.
+func (a *application) apiCompletionIn(repo string, requireRepo bool, list func(ctx context.Context, t target, toComplete string) ([]cobra.Completion, error)) cobra.CompletionFunc {
 	return func(cmd *cobra.Command, args []string, toComplete string) ([]cobra.Completion, cobra.ShellCompDirective) {
 		ctx, cancel := completionContext(cmd)
 		defer cancel()
-		t, err := a.completionTarget(ctx, requireRepo)
+		t, err := a.completionTarget(ctx, repo, requireRepo)
 		var values []cobra.Completion
 		if err == nil {
 			values, err = list(ctx, t, toComplete)
@@ -284,18 +306,14 @@ func (a *application) apiCompletion(requireRepo bool, list func(ctx context.Cont
 			cobra.CompDebugln("tea: "+err.Error(), false)
 			return nil, cobra.ShellCompDirectiveNoFileComp
 		}
-		values = slices.DeleteFunc(values, func(value cobra.Completion) bool {
-			name, _, _ := strings.Cut(value, "\t")
-			return slices.Contains(args, name)
-		})
 		return matching(values, toComplete), cobra.ShellCompDirectiveNoFileComp
 	}
 }
 
 // completionTarget resolves an API completion's target like resolve, but never prompts or rotates
 // credentials: an expired OAuth token or an encrypted SSH key completes nothing.
-func (a *application) completionTarget(ctx context.Context, requireRepo bool) (target, error) {
-	t, err := a.resolveTarget(ctx, requireRepo, false)
+func (a *application) completionTarget(ctx context.Context, repo string, requireRepo bool) (target, error) {
+	t, err := a.resolveRequest(ctx, targetRequest{requireRepo: requireRepo, repo: repo, repoSource: "argument"})
 	if err != nil {
 		return t, err
 	}
@@ -515,19 +533,27 @@ func (a *application) completeVariables(scope func() services.ActionScope) cobra
 // actionScope applies the target's repository to a scope without organization or account.
 func actionScope(scope func() services.ActionScope, t target) services.ActionScope {
 	selected := scope()
-	if selected.Organization == "" && !selected.Account {
+	if inRepository(selected) {
 		selected.Repository = t.Repo
 	}
 	return selected
 }
 
+// inRepository reports whether the scope is the repository's, which discovery must then select.
+func inRepository(scope services.ActionScope) bool {
+	return scope.Organization == "" && !scope.Account
+}
+
 // pathFlags take local file paths, like every flag whose name ends in -file.
-var pathFlags = []string{"asset", "attachment", "file", "input", "ssh-key"}
+var pathFlags = []string{"asset", "attachment", "file", "input", "output", "ssh-key"}
 
 // completeDefaults completes paths only where a command takes them: path flags and flags marked
 // with MarkFlagFilename. Everything else without a completer completes nothing, by the root's
 // default directive, rather than file names.
 func completeDefaults(cmd *cobra.Command) {
+	if cmd.ValidArgsFunction != nil {
+		cmd.ValidArgsFunction = withoutGiven(cmd.ValidArgsFunction)
+	}
 	for _, flags := range []*pflag.FlagSet{cmd.Flags(), cmd.PersistentFlags()} {
 		flags.VisitAll(func(flag *pflag.Flag) {
 			if _, registered := cmd.GetFlagCompletionFunc(flag.Name); registered {
@@ -546,6 +572,19 @@ func completeDefaults(cmd *cobra.Command) {
 	}
 }
 
+// withoutGiven drops the positional completions that the arguments already name. Flag values are not
+// filtered: Cobra passes the positional arguments to their completers as well, and a job ID may equal
+// its run ID.
+func withoutGiven(complete cobra.CompletionFunc) cobra.CompletionFunc {
+	return func(cmd *cobra.Command, args []string, toComplete string) ([]cobra.Completion, cobra.ShellCompDirective) {
+		values, directive := complete(cmd, args, toComplete)
+		return slices.DeleteFunc(values, func(value cobra.Completion) bool {
+			name, _, _ := strings.Cut(value, "\t")
+			return slices.Contains(args, name)
+		}), directive
+	}
+}
+
 // completeFiles leaves the completion of file paths to the shell.
 func completeFiles(*cobra.Command, []string, string) ([]cobra.Completion, cobra.ShellCompDirective) {
 	return nil, cobra.ShellCompDirectiveDefault

internal/app/labels.go

@@ -351,11 +351,10 @@ func (a *application) importLabels(cmd *cobra.Command, scope labelScope, file st
 // completeScopedLabels completes the label names of the repository or, with --org, of the
 // organization that *org names.
 func (a *application) completeScopedLabels(org *string) cobra.CompletionFunc {
-	return a.apiCompletion(false, func(ctx context.Context, t target, _ string) ([]cobra.Completion, error) {
-		if *org == "" && t.Repo == "" {
-			return nil, errRepositoryRequired
-		}
-		items, err := (services.Labels{Client: t.client, Repository: t.Repo, Organization: *org}).List(ctx, completionLimit, 1)
-		return describe(items, func(item services.Label) (string, string) { return item.Name, item.Description }), err
-	})
+	return func(cmd *cobra.Command, args []string, toComplete string) ([]cobra.Completion, cobra.ShellCompDirective) {
+		return a.apiCompletion(*org == "", func(ctx context.Context, t target, _ string) ([]cobra.Completion, error) {
+			items, err := (services.Labels{Client: t.client, Repository: t.Repo, Organization: *org}).List(ctx, completionLimit, 1)
+			return describe(items, func(item services.Label) (string, string) { return item.Name, item.Description }), err
+		})(cmd, args, toComplete)
+	}
 }

internal/app/ref_test.go

@@ -210,6 +210,10 @@ func TestResolveRefSelectsTheReferencedRepository(t *testing.T) {
 	_, _, err = a.resolveRef(commandContext(), serverURL+"/bob/tool/issues/1", issueRef)
 	require.Equal(t, clierr.Usage, clierr.ExitCode(err))
 	require.EqualError(t, err, serverURL+"/bob/tool/issues/1 points to bob/tool but --repo is bob/other")
+	a.options.repo = "not a repo"
+	_, _, err = a.resolveRef(commandContext(), serverURL+"/bob/tool/issues/1", issueRef)
+	require.Equal(t, clierr.Usage, clierr.ExitCode(err))
+	require.ErrorContains(t, err, `invalid repository "not a repo"`)
 }
 
 func TestExpandMeUsesTheLoginTeaActsAs(t *testing.T) {

internal/app/target.go

@@ -302,8 +302,11 @@ func (a *application) agreesWithRepoFlag(req targetRequest, hosts map[string]set
 	}
 	selected, selectedErr := settings.ParseRepository(req.repo, hosts)
 	explicit, explicitErr := settings.ParseRepository(a.options.repo, hosts)
-	if selectedErr != nil || explicitErr != nil {
-		return nil // Resolution reports the invalid one.
+	if explicitErr != nil {
+		return clierr.Wrap(clierr.Usage, explicitErr) // The argument's repository replaces --repo, so nothing else parses it.
+	}
+	if selectedErr != nil {
+		return nil // Resolution reports the invalid argument.
 	}
 	sameHost := selected.Host == "" || explicit.Host == "" || selected.Host == explicit.Host
 	sameRepo := strings.EqualFold(selected.Name, explicit.Name) && (explicit.Owner == "" || strings.EqualFold(selected.Owner, explicit.Owner))

internal/settings/remote.go

@@ -178,12 +178,21 @@ func ParseRepository(raw string, hosts map[string]Host) (RepositoryRef, error) {
 	}
 	// The longest configured instance prefix wins, so example.org/gitea/owner/name selects the
 	// instance installed below /gitea rather than example.org.
-	best := ""
+	best, tied := "", false
 	for instance := range hosts {
 		prefix := bareInstance(instance)
-		if len(prefix) > len(bareInstance(best)) && strings.HasPrefix(strings.ToLower(trimmed), strings.ToLower(prefix)+"/") && ValidSlug(trimmed[len(prefix)+1:]) {
-			best = instance
+		if !strings.HasPrefix(strings.ToLower(trimmed), strings.ToLower(prefix)+"/") || !ValidSlug(trimmed[len(prefix)+1:]) {
+			continue
 		}
+		switch longest := len(bareInstance(best)); {
+		case len(prefix) > longest:
+			best, tied = instance, false
+		case len(prefix) == longest && best != "":
+			tied = true // http and https of one host: the scheme decides, not map order.
+		}
+	}
+	if tied {
+		return RepositoryRef{}, fmt.Errorf("instance %q is configured for both http and https; include the scheme", bareInstance(best))
 	}
 	if best != "" {
 		owner, name, _ := strings.Cut(trimmed[len(bareInstance(best))+1:], "/")

internal/settings/remote_test.go

@@ -102,6 +102,17 @@ func TestParseRepositoryAcceptsShorthands(t *testing.T) {
 	require.Equal(t, "alice/project", RepositoryRef{Owner: "alice", Name: "project"}.Slug())
 }
 
+func TestParseRepositoryRejectsAnInstanceConfiguredForBothSchemes(t *testing.T) {
+	hosts := map[string]Host{"http://localhost:3001": {}, "https://localhost:3001": {}}
+	for range 20 { // Map iteration order is random.
+		_, err := ParseRepository("localhost:3001/alice/project", hosts)
+		require.ErrorContains(t, err, "include the scheme")
+	}
+	got, err := ParseRepository("https://localhost:3001/alice/project", hosts)
+	require.NoError(t, err)
+	require.Equal(t, "https://localhost:3001", got.Host)
+}
+
 func TestResolveHostAcceptsBareHosts(t *testing.T) {
 	hosts := map[string]Host{"http://localhost:3001": {}, "https://example.org/gitea": {}}
 	for raw, want := range map[string]string{