fix: address verified review findings (cli-core-1)

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

internal/app/app.go

@@ -361,12 +361,21 @@ func builtins(root *cobra.Command) map[string]bool {
 // Execute expands aliases and executes the fresh root without printing errors.
 func Execute(ctx context.Context, deps Dependencies, args []string) error {
 	root := NewRoot(deps)
-	if at := commandIndex(root, args); at == len(args) || (args[at] != cobra.ShellCompRequestCmd && args[at] != cobra.ShellCompNoDescRequestCmd) {
-		args = joinJSONFields(root, args)
-	}
 	// Broken settings or aliases must not prevent help or recovery commands.
 	snapshot, loadErr := deps.Settings.Load()
 	names := builtins(root)
+	if loadErr == nil {
+		for name, expansion := range snapshot.Preferences.Aliases {
+			if aliases.Check(name, snapshot.Preferences.Aliases, names) == nil {
+				root.AddCommand(&cobra.Command{Use: name, Short: fmt.Sprintf("Alias for %q", expansion), GroupID: "alias", Annotations: map[string]string{"tea.alias": "true"}, RunE: func(*cobra.Command, []string) error {
+					return clierr.Wrap(clierr.Usage, errors.New("alias must be expanded before execution"))
+				}})
+			}
+		}
+	}
+	if at := commandIndex(root, args); at == len(args) || (args[at] != cobra.ShellCompRequestCmd && args[at] != cobra.ShellCompNoDescRequestCmd) {
+		args = joinJSONFields(root, args)
+	}
 	if commandAt := commandIndex(root, args); commandAt < len(args) {
 		name := args[commandAt]
 		completion := name == cobra.ShellCompRequestCmd || name == cobra.ShellCompNoDescRequestCmd
@@ -394,15 +403,6 @@ func Execute(ctx context.Context, deps Dependencies, args []string) error {
 			args = completeJSONValue(args)
 		}
 	}
-	if loadErr == nil {
-		for name, expansion := range snapshot.Preferences.Aliases {
-			if aliases.Check(name, snapshot.Preferences.Aliases, names) == nil {
-				root.AddCommand(&cobra.Command{Use: name, Short: fmt.Sprintf("Alias for %q", expansion), GroupID: "alias", Annotations: map[string]string{"tea.alias": "true"}, RunE: func(*cobra.Command, []string) error {
-					return clierr.Wrap(clierr.Usage, errors.New("alias must be expanded before execution"))
-				}})
-			}
-		}
-	}
 	root.SetArgs(args)
 	err := root.ExecuteContext(ctx)
 	if apiErr, ok := errors.AsType[*transport.APIError](err); ok && apiErr.Status == 401 {

internal/app/app_test.go

@@ -84,6 +84,14 @@ func TestAliasesAfterGlobalFlagsAndNestedHelp(t *testing.T) {
 	}
 }
 
+func TestBareJSONBeforeAnAliasKeepsTheAlias(t *testing.T) {
+	deps, out, _ := testDeps(t)
+	require.NoError(t, Execute(context.Background(), deps, []string{"alias", "set", "prefs", "config list"}))
+	out.Reset()
+	require.NoError(t, Execute(context.Background(), deps, []string{"--json", "prefs"}))
+	require.Contains(t, out.String(), "git_protocol")
+}
+
 func TestJoinJSONFields(t *testing.T) {
 	for _, tc := range []struct{ args, want []string }{
 		{[]string{"issue", "list", "--json", "number,title"}, []string{"issue", "list", "--json=number,title"}},
@@ -101,6 +109,7 @@ func TestJoinJSONFields(t *testing.T) {
 		// A leading --json keeps the command word that follows it.
 		{[]string{"--json", "repo", "view", "alice/project"}, []string{"--json", "repo", "view", "alice/project"}},
 		{[]string{"--json", "pr", "list"}, []string{"--json", "pr", "list"}},
+		{[]string{"issue", "--json", "list"}, []string{"issue", "--json", "list"}},
 		{[]string{"--json", "number", "my-alias"}, []string{"--json=number", "my-alias"}},
 		// A word that cannot be an argument is a misspelled field, which then lists the available ones.
 		{[]string{"issue", "list", "--json", "Number"}, []string{"issue", "list", "--json=Number"}},

internal/app/args.go

@@ -78,11 +78,14 @@ func oneOf(flag, value string, allowed ...string) error {
 // maxLimit bounds --limit; list services page through the server's smaller pages to reach it.
 const maxLimit = 1000
 
+// maxPage bounds --page so that the item offset of a page (page * limit) cannot overflow an int.
+const maxPage = math.MaxInt / (2 * maxLimit)
+
 // pageFlags registers -L/--limit and --page, which reject out-of-range values while parsing.
 func pageFlags(cmd *cobra.Command, limit, page *int) {
 	*limit = 30
 	cmd.Flags().VarP(positiveInt{limit, maxLimit}, "limit", "L", "Maximum number of results (1..1000)")
-	positiveIntVar(cmd, page, "page", 1, math.MaxInt, "Result page")
+	positiveIntVar(cmd, page, "page", 1, maxPage, "Result page")
 }
 
 // positiveIntVar registers an int flag that rejects values outside 1..upper while parsing.
@@ -103,7 +106,7 @@ func (f positiveInt) Type() string { return "int" }
 func (f positiveInt) Set(raw string) error {
 	n, err := strconv.Atoi(raw)
 	switch {
-	case (err != nil || n < 1) && f.upper == math.MaxInt:
+	case (err != nil || n < 1) && (f.upper == math.MaxInt || f.upper == maxPage):
 		return errors.New("must be a positive integer")
 	case err != nil || n < 1 || n > f.upper:
 		return fmt.Errorf("must be 1..%d", f.upper)
@@ -301,12 +304,25 @@ func argumentName(cmd *cobra.Command, argument string) string {
 		return text
 	}
 	noun := group.Name()
+	// The numbers under `time` and `milestone issue` are issue numbers.
+	if noun == "time" || slices.ContainsFunc(ancestors(cmd), func(c *cobra.Command) bool { return c.Name() == "issue" }) {
+		noun = "issue"
+	}
 	if noun == "pr" {
 		noun = "pull request"
 	}
 	return noun + " " + text
 }
 
+// ancestors returns the parents of cmd below the root command.
+func ancestors(cmd *cobra.Command) []*cobra.Command {
+	var parents []*cobra.Command
+	for parent := cmd.Parent(); parent != nil && parent.HasParent(); parent = parent.Parent() {
+		parents = append(parents, parent)
+	}
+	return parents
+}
+
 // localFlagsChanged reports whether a command-specific flag other than those in except was supplied.
 func localFlagsChanged(cmd *cobra.Command, except ...string) bool {
 	changed := false

internal/app/args_test.go

@@ -4,6 +4,8 @@
 package app
 
 import (
+	"math"
+	"strconv"
 	"testing"
 	"time"
 
@@ -93,6 +95,9 @@ func TestArgumentErrorsShowTheUsageLine(t *testing.T) {
 		{[]string{"pr", "checkout"}, "missing pull request number\nUsage: tea pr checkout {<number> | <url> | <branch>} [flags]"},
 		{[]string{"run", "view"}, "missing run ID\nUsage: tea run view [<run-id>] [flags]"},
 		{[]string{"release", "asset", "edit", "v1"}, "missing asset\nUsage: tea release asset edit <tag> <asset> [flags]"},
+		{[]string{"time", "create"}, "missing issue number\nUsage: tea time create {<number> | <url>} <duration> [flags]"},
+		{[]string{"milestone", "issue", "add", "v1.0"}, "missing issue number\nUsage: tea milestone issue add <milestone> {<number> | <url>}... [flags]"},
+		{[]string{"branch", "protect", "main", "--pattern", "release/*"}, "--pattern cannot be combined with branch names\nUsage: tea branch protect <branch>... [flags]"},
 		{[]string{"repo", "clone"}, "missing repository\nUsage: tea repo clone <repository> [<directory>] [-- <gitflags>...] [flags]"},
 	} {
 		requireUsageBeforeRequests(t, tc.want, tc.args...)
@@ -108,3 +113,12 @@ func TestLimitAcceptsShorthandUpToOneThousand(t *testing.T) {
 	require.Equal(t, 1000, limit)
 	require.ErrorContains(t, cmd.Flags().Parse([]string{"-L", "1001"}), "must be 1..1000")
 }
+
+func TestPageRejectsValuesThatOverflowTheOffset(t *testing.T) {
+	var limit, page int
+	cmd := &cobra.Command{Use: "list"}
+	pageFlags(cmd, &limit, &page)
+	require.NoError(t, cmd.Flags().Parse([]string{"--page", strconv.Itoa(maxPage)}))
+	require.Equal(t, maxPage, page)
+	require.ErrorContains(t, cmd.Flags().Parse([]string{"--page", strconv.Itoa(math.MaxInt)}), "--page")
+}

internal/app/auth.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"io"
 	"maps"
+	"net/http"
 	"os"
 	"path/filepath"
 	"slices"
@@ -22,6 +23,7 @@ import (
 	"gitea.dev/tea/internal/services"
 	"gitea.dev/tea/internal/settings"
 	"gitea.dev/tea/internal/terminal"
+	"gitea.dev/tea/internal/transport"
 	"github.com/spf13/cobra"
 )
 
@@ -267,7 +269,7 @@ $ tea auth login --host gitea.com --oauth --scopes read:repository,read:user`,
 			}
 			user, err := services.CurrentAccount(cmd.Context(), a.deps.Client(instance, credential.AccessToken, insecure))
 			if err != nil {
-				return err
+				return rejectedLogin(err, "the token", "check it or create a new one under Settings > Applications")
 			}
 			if selectedUser, _ := a.selectedUser(); !services.MatchAccount(selectedUser, user) {
 				return clierr.Wrap(clierr.Usage, errors.New("token account does not match --user or TEA_USER"))
@@ -304,6 +306,15 @@ $ tea auth login --host gitea.com --oauth --scopes read:repository,read:user`,
 	return login
 }
 
+// rejectedLogin words a 401 for the credential that tea auth login was given, so that Explain does
+// not blame GITEA_TOKEN or tell the user to run the command they are running.
+func rejectedLogin(err error, credential, advice string) error {
+	if apiErr, ok := errors.AsType[*transport.APIError](err); ok && apiErr.Status == http.StatusUnauthorized {
+		return explain(err, "%s was rejected by %s; %s", credential, cmp.Or(apiErr.Instance, "the instance"), advice)
+	}
+	return err
+}
+
 // confirmReplace asks before a login replaces the stored credential of an account that tea already
 // has, and reports whether it does.
 func (a *application) confirmReplace(cmd *cobra.Command, instance, user string) (bool, error) {
@@ -346,7 +357,7 @@ func (a *application) signedLogin(cmd *cobra.Command, snapshot settings.Snapshot
 	client.Signer = signer
 	user, err := services.CurrentAccount(cmd.Context(), client)
 	if err != nil {
-		return account, "", err
+		return account, "", rejectedLogin(err, "the SSH signature", "check that the key is registered for your account")
 	}
 	if selectedUser, _ := a.selectedUser(); !services.MatchAccount(selectedUser, user) {
 		return account, "", clierr.Wrap(clierr.Usage, errors.New("authenticated account does not match --user or TEA_USER"))

internal/app/branches.go

@@ -277,7 +277,10 @@ $ tea branch protect --pattern 'release/*' --allow-push acme/release`,
 		Annotations: textOutput("branch protect does not return a resource"),
 		Args: func(cmd *cobra.Command, args []string) error {
 			if pattern != "" {
-				return usageArgs(cobra.NoArgs)(cmd, args)
+				if len(args) > 0 {
+					return clierr.Wrap(clierr.Usage, &argumentError{problem: "--pattern cannot be combined with branch names", cmd: cmd})
+				}
+				return nil
 			}
 			return usageArgs(cobra.MinimumNArgs(1))(cmd, args)
 		},

internal/app/deadline.go

@@ -16,7 +16,7 @@ import (
 
 // numericDate matches dates such as 03/04/2026, which read as March 4 in the US and April 3
 // elsewhere.
-var numericDate = regexp.MustCompile(`^\d{1,2}[/.]\d{1,2}[/.]\d{2,4}$`)
+var numericDate = regexp.MustCompile(`^\d{1,2}[/.]\d{1,2}[/.]\d{2,4}(?:[ T].*)?$`)
 
 // parseDeadline parses a --deadline value; an empty value requests clearing.
 // Supplied timezone offsets are kept and Unix timestamps are UTC.

internal/app/deadline_test.go

@@ -43,7 +43,7 @@ func TestDeadlineInputAndExplicitClearing(t *testing.T) {
 }
 
 func TestAmbiguousNumericDeadlinesAreRejected(t *testing.T) {
-	for _, value := range []string{"03/04/2026", "04/03/2026", "01/15/2028", "15.01.2028", "3/4/26"} {
+	for _, value := range []string{"03/04/2026", "04/03/2026", "01/15/2028", "15.01.2028", "3/4/26", "04/03/2026 17:00", "4/3/2026 17:00:00", "04/03/2026 5:00 PM"} {
 		for _, command := range [][]string{{"issue", "edit", "1"}, {"issue", "create", "--title", "x"}, {"milestone", "create", "--title", "x"}} {
 			requireUsageBeforeRequests(t, fmt.Sprintf("invalid --deadline %q: the order of day and month is ambiguous; use YYYY-MM-DD", value), append(command, "--deadline="+value)...)
 		}

internal/app/editor.go

@@ -26,10 +26,27 @@ func finishDraft(draft *terminal.Draft, result *error) {
 		}
 	}
 	if *result != nil {
-		*result = fmt.Errorf("%w; draft saved at %s", *result, draft.Path)
+		*result = &draftError{err: *result, path: draft.Path}
 	}
 }
 
+// draftError reports where the text of a failed or canceled command was saved.
+type draftError struct {
+	err  error
+	path string
+}
+
+func (e *draftError) Error() string { return fmt.Sprintf("%v; draft saved at %s", e.err, e.path) }
+func (e *draftError) Unwrap() error { return e.err }
+
+// DraftPath returns the file that holds the unsubmitted text of a failed command, or "".
+func DraftPath(err error) string {
+	if draft, ok := errors.AsType[*draftError](err); ok {
+		return draft.path
+	}
+	return ""
+}
+
 func (a *application) editor() (terminal.Editor, string, error) {
 	preferences, err := a.preferences()
 	if err != nil {

internal/app/errors_test.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 	"time"
 
@@ -129,3 +130,14 @@ func TestTimeoutErrorsNameTheWaitAndKeepTheExitCode(t *testing.T) {
 	require.Equal(t, other, timeoutError(other, time.Second, "waiting"))
 	require.EqualError(t, Explain(context.Background(), Dependencies{}, err), "timed out before the operation finished")
 }
+
+func TestRejectedLoginTokenIsNotBlamedOnTheEnvironment(t *testing.T) {
+	rejecting := fakeGitea(t, map[string][2]string{"GET /api/v1/user": {"401", `{"message":"token is invalid"}`}})
+	deps, _, _ := testDeps(t)
+	deps.Getenv = envFunc(map[string]string{"GITEA_TOKEN": "valid-environment-token"})
+	deps.Streams.In = strings.NewReader("mistyped-token\n")
+	err := Execute(context.Background(), deps, []string{"auth", "login", "--host", rejecting.URL, "--with-token", "--credential-store", "file", "--no-input"})
+	explained := Explain(context.Background(), deps, err)
+	require.EqualError(t, explained, "the token was rejected by "+rejecting.URL+"; check it or create a new one under Settings > Applications")
+	require.Equal(t, clierr.Authentication, clierr.ExitCode(explained))
+}

main.go

@@ -18,6 +18,12 @@ import (
 
 func main() {
 	ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
+	// Restore the default handling once the first signal arrived, so that a second Ctrl-C or SIGTERM
+	// ends tea even while it blocks on something that ignores ctx, such as reading stdin.
+	go func() {
+		<-ctx.Done()
+		stop()
+	}()
 	code := run(ctx, app.System(), os.Args[1:])
 	stop()
 	os.Exit(code)
@@ -33,11 +39,21 @@ func run(ctx context.Context, deps app.Dependencies, args []string) int {
 	}
 	code := clierr.ExitCode(err)
 	switch {
-	case err == nil, code == clierr.Interrupted, clierr.IsQuiet(err):
+	case err == nil, clierr.IsQuiet(err):
+	case code == clierr.Interrupted:
+		reportDraft(deps, err)
 	case code == clierr.Canceled && !errors.Is(err, context.DeadlineExceeded):
 		fmt.Fprintln(deps.Streams.ErrOut, "Canceled.")
+		reportDraft(deps, err)
 	default:
 		fmt.Fprintf(deps.Streams.ErrOut, "Error: %v\n", app.Explain(ctx, deps, err))
 	}
 	return code
 }
+
+// reportDraft tells where the text of a canceled command was saved; the error is otherwise not shown.
+func reportDraft(deps app.Dependencies, err error) {
+	if path := app.DraftPath(err); path != "" {
+		fmt.Fprintf(deps.Streams.ErrOut, "Draft saved at %s\n", path)
+	}
+}

main_test.go

@@ -131,3 +131,65 @@ func TestRunReportsDeclinedConfirmationsAndExplainedFailures(t *testing.T) {
 	require.Equal(t, 1, run(context.Background(), deps, []string{"issue", "view", "9", "-R", "alice/project", "--no-input"}))
 	require.Equal(t, "Error: issue #9 not found in alice/project\n", stderr.String())
 }
+
+type reviewFormStub struct{}
+
+func (reviewFormStub) CreateReview(context.Context, terminal.ReviewFormChoices) (terminal.ReviewFormValues, error) {
+	return terminal.ReviewFormValues{Event: "COMMENT", Body: "Self note"}, nil
+}
+
+func TestRunReportsWhereADeclinedReviewWasSaved(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == "/api/v1/user" {
+			_, _ = w.Write([]byte(`{"login":"alice"}`))
+			return
+		}
+		_, _ = w.Write([]byte(`{"number":3,"title":"Add one","user":{"login":"alice"},"head":{"ref":"feat/one"},"base":{"ref":"main"}}`))
+	}))
+	defer server.Close()
+	tmp := t.TempDir()
+	t.Setenv("TMPDIR", tmp)
+	var stderr bytes.Buffer
+	store := settings.New(t.TempDir())
+	deps := app.Dependencies{
+		Settings: store, Credentials: &auth.Store{Dir: store.Dir}, Client: transport.New, Prompter: decliningPrompter{}, ReviewForm: reviewFormStub{},
+		Streams: terminal.Streams{In: &bytes.Buffer{}, Out: &bytes.Buffer{}, ErrOut: &stderr, InTTY: true, ErrTTY: true},
+		Getenv:  func(key string) string { return map[string]string{"GITEA_TOKEN": "token", "TEA_HOST": server.URL}[key] },
+	}
+	require.Equal(t, 3, run(context.Background(), deps, []string{"pr", "review", "3", "-R", "alice/project"}))
+	files, err := filepath.Glob(filepath.Join(tmp, "tea-draft-*.md"))
+	require.NoError(t, err)
+	require.Len(t, files, 1)
+	require.Equal(t, "Canceled.\nDraft saved at "+files[0]+"\n", stderr.String())
+}
+
+func TestSecondInterruptStopsACommandBlockedOnStdin(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("os.Interrupt delivery is not supported by Windows processes")
+	}
+	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
+	defer cancel()
+	binary := filepath.Join(t.TempDir(), "tea")
+	build := exec.CommandContext(ctx, "go", "build", "-o", binary, ".")
+	output, err := build.CombinedOutput()
+	require.NoError(t, err, "build: %s", output)
+	command := exec.CommandContext(ctx, binary, "auth", "login", "--host", "http://127.0.0.1:1", "--with-token", "--no-input")
+	command.Env = append(os.Environ(), "TEA_CONFIG_DIR="+t.TempDir(), "GITEA_TOKEN=")
+	stdin, err := command.StdinPipe()
+	require.NoError(t, err)
+	defer stdin.Close()
+	require.NoError(t, command.Start())
+	done := make(chan error, 1)
+	go func() { done <- command.Wait() }()
+	// Reading the token from stdin ignores the canceled context, so only a second signal ends tea.
+	time.Sleep(time.Second)
+	require.NoError(t, command.Process.Signal(os.Interrupt))
+	time.Sleep(500 * time.Millisecond)
+	require.NoError(t, command.Process.Signal(os.Interrupt))
+	select {
+	case err := <-done:
+		require.Error(t, err)
+	case <-time.After(10 * time.Second):
+		t.Fatal("tea ignored the second interrupt while waiting on stdin")
+	}
+}