fix: address verified review findings (actions-runs-1)

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

internal/app/action_runs.go

@@ -8,6 +8,8 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io"
+	"path"
 	"slices"
 	"strconv"
 	"strings"
@@ -69,10 +71,15 @@ $ tea run list --since 24h --json id,status,conclusion,workflow`,
 			opts.Actor = t.expandMe(opts.Actor)
 			if opts.Workflow != "" {
 				workflow, err := service.FindWorkflow(cmd.Context(), opts.Workflow)
+				// The server also lists the runs of a workflow file that only exists on another branch or was removed.
+				if notFound, ok := errors.AsType[*services.NotFoundError](err); ok && notFound.Kind == "workflow" && isWorkflowFile(opts.Workflow) {
+					err = nil
+				} else if err == nil {
+					opts.Workflow = workflow.ID
+				}
 				if err != nil {
 					return err
 				}
-				opts.Workflow = workflow.ID
 			}
 			page, err := service.RunsPage(cmd.Context(), opts)
 			if errors.Is(err, services.ErrRunScanExhausted) {
@@ -230,6 +237,12 @@ $ tea run view 34 --json jobs --jq '.jobs[] | {name, conclusion}'`,
 	return cmd
 }
 
+// isWorkflowFile tells a workflow file name, which the server resolves itself, from a workflow name.
+func isWorkflowFile(raw string) bool {
+	ext := path.Ext(raw)
+	return !strings.ContainsAny(raw, " /") && (ext == ".yml" || ext == ".yaml")
+}
+
 // runHints are the next steps shown under a run: watching it while it runs, and reading the logs
 // of its jobs once one started.
 func (a *application) runHints(t target, run services.Run) []string {
@@ -250,9 +263,23 @@ func (a *application) suggestedRepo(t target) string {
 	if strings.HasPrefix(t.RepoSource, "remote ") || strings.HasPrefix(t.RepoSource, "git config") {
 		return ""
 	}
+	// An instance that --host or the repository itself selected is not the one a copied command finds.
+	if (t.HostSource == "--host" || (t.RepoSource != "" && t.HostSource == t.RepoSource)) && !a.isDefaultHost(t.Host) {
+		return " -R " + t.Host + "/" + t.Repo
+	}
 	return " -R " + t.Repo
 }
 
+// isDefaultHost reports whether host is the instance that commands use without --host or a repository URL.
+func (a *application) isDefaultHost(host string) bool {
+	s, err := a.settings()
+	if err != nil {
+		return false
+	}
+	var fallback target
+	return a.defaultHost(&fallback, s) == nil && fallback.Host == host
+}
+
 // renderRun shows a run with its jobs: on a terminal its state, trigger and jobs with their
 // steps, followed by hints and the run's URL; for scripts "key:\tvalue" lines and the jobs.
 func renderRun(run services.Run, d terminal.Display, hints []string) string {
@@ -338,11 +365,15 @@ func jobLines(jobs []services.Job, now time.Time) [][]terminal.Cell {
 	return rows
 }
 
-// waitingForRunner explains a run that waits for a runner: none of its jobs started yet.
+// waitingForRunner explains a run that waits for a runner: none of its jobs started yet. A run
+// that needs approval, such as one from a first-time contributor's fork, has the status "waiting".
 func waitingForRunner(run services.Run) string {
 	if run.Completed() || len(run.Jobs) == 0 || slices.ContainsFunc(run.Jobs, services.Job.Started) {
 		return ""
 	}
+	if run.Status == "waiting" {
+		return "Waiting for approval; approve the run in the web interface"
+	}
 	return "Waiting for a runner" + runnerLabels(run)
 }
 
@@ -515,7 +546,8 @@ func (a *application) watchRun(cmd *cobra.Command, t target, id int64, interval,
 	if err != nil {
 		if errors.Is(err, context.DeadlineExceeded) && run.ID != 0 {
 			if a.options.json == "" {
-				_ = a.writeHuman(cmd, renderRun(run, a.display(), nil))
+				// Not through the pager, which would stop the command at its end.
+				_, _ = io.WriteString(out, renderRun(run, a.display(), nil))
 			}
 			return timeoutError(err, timeout, "waiting for run %d (%s)", id, stillRunning(run))
 		}
@@ -525,12 +557,12 @@ func (a *application) watchRun(cmd *cobra.Command, t target, id int64, interval,
 		if err := a.writeJSON(cmd, run); err != nil {
 			return err
 		}
-	} else if err := a.writeHuman(cmd, renderRun(run, a.display(), nil)); err != nil {
+	} else if _, err := io.WriteString(out, renderRun(run, a.display(), nil)); err != nil {
 		return err
 	}
-	conclusion := cmp.Or(run.Conclusion, run.Status)
+	conclusion := terminal.SafeLine(cmp.Or(run.Conclusion, run.Status))
 	if !services.SuccessfulRun(run) {
-		return fmt.Errorf("run %d (%s) completed with %s", run.ID, run.Workflow, conclusion)
+		return fmt.Errorf("run %d (%s) completed with %s", run.ID, terminal.SafeLine(run.Workflow), conclusion)
 	}
 	took := ""
 	if d, ok := runElapsed(run.StartedAt, run.CompletedAt, a.deps.Now()); ok {
@@ -557,10 +589,13 @@ func runProgress(run services.Run) string {
 // stillRunning says how far a run got when a wait for it expired, suspecting a missing runner
 // when no job started.
 func stillRunning(run services.Run) string {
-	if waitingForRunner(run) != "" {
-		return "still " + run.Status + "; is a runner registered" + runnerLabels(run) + "?"
+	if run.Status == "waiting" && waitingForRunner(run) != "" {
+		return "still waiting for approval in the web interface"
 	}
-	return "still " + run.Status
+	if waitingForRunner(run) != "" {
+		return "still " + terminal.SafeLine(run.Status) + "; is a runner registered" + runnerLabels(run) + "?"
+	}
+	return "still " + terminal.SafeLine(run.Status)
 }
 
 func (a *application) runCancelCommand() *cobra.Command {
@@ -580,11 +615,15 @@ $ tea run cancel 34 --force`,
 			if err != nil {
 				return err
 			}
-			run, err := services.Actions{Client: t.client, Repository: t.Repo}.CancelRun(cmd.Context(), id, force)
+			service := services.Actions{Client: t.client, Repository: t.Repo}
+			run, err := service.CancelRun(cmd.Context(), id, force)
 			if err != nil {
 				return err
 			}
 			a.done(cmd, "Requested cancellation of run %d", id)
+			if a.options.json != "" {
+				run = service.NameRun(cmd.Context(), run)
+			}
 			return a.jsonResult(cmd, run)
 		},
 	}
@@ -643,6 +682,9 @@ $ tea run rerun 34 --job build`,
 				return err
 			}
 			a.done(cmd, "Requested rerun of run %d (attempt %d); watch it with: tea run watch %d%s", id, run.Attempt, id, a.suggestedRepo(t))
+			if a.options.json != "" {
+				run = service.NameRun(cmd.Context(), run)
+			}
 			return a.jsonResult(cmd, run)
 		},
 	}

internal/app/action_runs_test.go

@@ -16,6 +16,7 @@ import (
 	"time"
 
 	"gitea.dev/tea/internal/clierr"
+	"gitea.dev/tea/internal/services"
 	"gitea.dev/tea/internal/terminal"
 	"github.com/stretchr/testify/require"
 )
@@ -132,6 +133,22 @@ X       29  Nightly   Refactor the deployment pipeline  feature/x  push
 	}
 }
 
+func TestRunListAcceptsWorkflowFilesMissingFromTheDefaultBranch(t *testing.T) {
+	server, requests := actionsAPI(t, map[string]string{
+		"GET actions/workflows":               workflowsFixture,
+		"GET actions/workflows/old.yml/runs":  `{"total_count":1,"workflow_runs":[` + failedRunFixture + `]}`,
+		"GET actions/workflows/gone.yml/runs": `404 {"message":"workflow \"gone.yml\" not found"}`,
+	})
+	deps, out, _ := actionsDeps(t, server.URL, false)
+	require.NoError(t, Execute(context.Background(), deps, []string{"run", "list", "-R", "alice/project", "--workflow", "old.yml", "--json", "id"}))
+	require.JSONEq(t, `[{"id":29}]`, out.String())
+	err := Execute(context.Background(), deps, []string{"run", "list", "-R", "alice/project", "--workflow", "gone.yml"})
+	require.EqualError(t, err, `workflow "gone.yml" not found in alice/project`)
+	err = Execute(context.Background(), deps, []string{"run", "list", "-R", "alice/project", "--workflow", "Missing name"})
+	require.EqualError(t, err, `workflow "Missing name" not found in alice/project`)
+	require.NotContains(t, *requests, "GET actions/workflows/Missing name/runs")
+}
+
 func TestRunListResolvesWorkflowNamesAndReportsEmptyFilters(t *testing.T) {
 	server, requests := actionsAPI(t, map[string]string{
 		"GET actions/workflows":             workflowsFixture,
@@ -290,6 +307,34 @@ func TestRunWatchExitStatusAndTimeout(t *testing.T) {
 	}
 }
 
+func TestRunWatchSummaryBypassesThePager(t *testing.T) {
+	server, _ := actionsAPI(t, map[string]string{
+		"GET actions/workflows":   workflowsFixture,
+		"GET actions/runs/7":      strings.Replace(failedRunFixture, `"id":29`, `"id":7`, 1),
+		"GET actions/runs/7/jobs": failedJobsFixture,
+	})
+	deps, out, _ := actionsDeps(t, server.URL, true)
+	pager := &recordingPager{}
+	deps.Pager = pager
+	deps.Getenv = serverEnv(server.URL, map[string]string{"TERM": "dumb", "PAGER": "fake-pager"})
+	err := Execute(context.Background(), deps, []string{"run", "watch", "7", "-R", "alice/project", "--interval=1ms"})
+	require.EqualError(t, err, "run 7 (Nightly) completed with failure")
+	require.Zero(t, pager.calls, "the pager would hold the command back after the run completed")
+	require.Contains(t, out.String(), "Run tests")
+}
+
+func TestRunWatchErrorsDoNotCarryServerControlCharacters(t *testing.T) {
+	server, _ := actionsAPI(t, map[string]string{
+		"GET actions/workflows": `{"workflows":[]}`,
+		"GET actions/runs/7": `{"id":7,"path":"\u001b]0;pwned\u0007ci.yml@refs/heads/main","status":"completed","conclusion":"failure",
+			"created_at":"2028-01-01T11:59:00Z","started_at":"2028-01-01T11:59:00Z","completed_at":"2028-01-01T11:59:30Z"}`,
+		"GET actions/runs/7/jobs": `{"jobs":[]}`,
+	})
+	deps, _, _ := actionsDeps(t, server.URL, false)
+	err := Execute(context.Background(), deps, []string{"run", "watch", "7", "-R", "alice/project", "--interval=1ms"})
+	require.EqualError(t, err, "run 7 (]0;pwnedci) completed with failure")
+}
+
 func TestRunWatchRedrawsOnATerminalAndSummarizes(t *testing.T) {
 	server, _ := actionsAPI(t, map[string]string{
 		"GET actions/workflows": workflowsFixture,
@@ -323,10 +368,17 @@ func TestRunMutationsReportWhatHappened(t *testing.T) {
 		},
 		{
 			"cancel returns the run",
-			[]string{"run", "cancel", "47", "--json", "id,status"},
-			map[string]string{"POST actions/runs/47/cancel": queuedRunFixture},
-			[]string{"POST actions/runs/47/cancel"},
-			`{"id":47,"status":"queued"}`, "", nil,
+			[]string{"run", "cancel", "47", "--json", "id,status,workflow"},
+			map[string]string{"POST actions/runs/47/cancel": queuedRunFixture, "GET actions/workflows": workflowsFixture},
+			[]string{"POST actions/runs/47/cancel", "GET actions/workflows"},
+			`{"id":47,"status":"queued","workflow":"CI"}`, "", nil,
+		},
+		{
+			"cancel with fields the server may leave empty",
+			[]string{"run", "cancel", "47", "--json", "id,jobs,attempt,event"},
+			map[string]string{"POST actions/runs/47/cancel": `{"id":47}`, "GET actions/workflows": workflowsFixture},
+			[]string{"POST actions/runs/47/cancel", "GET actions/workflows"},
+			`{"id":47,"jobs":null,"attempt":0,"event":""}`, "", nil,
 		},
 		{
 			"rerun",
@@ -335,6 +387,13 @@ func TestRunMutationsReportWhatHappened(t *testing.T) {
 			[]string{"GET actions/runs/29", "POST actions/runs/29/rerun", "GET actions/runs/29"},
 			"", "✓ Requested rerun of run 29 (attempt 2); watch it with: tea run watch 29 -R alice/project\n", nil,
 		},
+		{
+			"rerun returns the run",
+			[]string{"run", "rerun", "29", "--json", "id,workflow"},
+			map[string]string{"GET actions/runs/29": completedRun, "POST actions/runs/29/rerun": completedRun, "GET actions/workflows": workflowsFixture},
+			[]string{"GET actions/runs/29", "POST actions/runs/29/rerun", "GET actions/runs/29", "GET actions/workflows"},
+			`{"id":29,"workflow":"Nightly"}`, "", nil,
+		},
 		{
 			"rerun a job by name",
 			[]string{"run", "rerun", "29", "--job", "TEST"},
@@ -395,3 +454,31 @@ func TestRunJobsListsStatusAndElapsedTime(t *testing.T) {
 	require.NoError(t, Execute(context.Background(), deps, []string{"run", "jobs", "#29", "-R", "alice/project"}))
 	require.Equal(t, "Showing 2 of 2 jobs in run 29\n\nSTATUS  ID  NAME   ELAPSED\n✓       30  build  45s\nX       31  test   27s\n", out.String())
 }
+
+func TestRunsAwaitingApprovalAreNotReportedAsWaitingForARunner(t *testing.T) {
+	jobs := []services.Job{{ID: 1, Name: "build", Status: "waiting", Labels: []string{"ubuntu-latest"}}}
+	approval := services.Run{ID: 7, Status: "waiting", Jobs: jobs}
+	require.Equal(t, "Waiting for approval; approve the run in the web interface", waitingForRunner(approval))
+	require.Equal(t, "still waiting for approval in the web interface", stillRunning(approval))
+	queued := services.Run{ID: 7, Status: "queued", Jobs: jobs}
+	require.Equal(t, `Waiting for a runner for "ubuntu-latest"`, waitingForRunner(queued))
+	require.Equal(t, `still queued; is a runner registered for "ubuntu-latest"?`, stillRunning(queued))
+}
+
+func TestRunSuggestionsNameTheInstanceThatWasSelectedExplicitly(t *testing.T) {
+	server, _ := actionsAPI(t, map[string]string{"GET actions/runs/47": queuedRunFixture})
+	host := strings.TrimPrefix(server.URL, "https://")
+	for _, tc := range []struct {
+		args []string
+		want string
+	}{
+		{[]string{"-R", "alice/project", "--host", server.URL}, " -R " + host + "/alice/project"},
+		{[]string{"-R", host + "/alice/project", "--host", server.URL}, " -R " + host + "/alice/project"},
+	} {
+		deps, _, _ := actionsDeps(t, server.URL, false)
+		// The instance that commands use by default is another one than the server.
+		deps.Getenv = serverEnv(server.URL, map[string]string{"TERM": "dumb", "GITEA_INSTANCE_URL": server.URL, "TEA_HOST": "https://default.test"})
+		err := Execute(context.Background(), deps, append([]string{"run", "rerun", "47"}, tc.args...))
+		require.EqualError(t, err, "run 47 is still queued; cancel it first: tea run cancel 47"+tc.want, tc.args)
+	}
+}

internal/app/artifacts.go

@@ -70,11 +70,10 @@ var artifactColumns = []terminal.Column{
 
 func artifactRow(item services.Artifact) []terminal.Cell {
 	expires := terminal.Cell{}
-	expiresAt := ""
 	if item.ExpiresAt != nil {
-		expiresAt = *item.ExpiresAt
-		expires.Time = terminal.ParseTime(expiresAt)
+		expires.Time = terminal.ParseTime(*item.ExpiresAt)
 	}
+	piped := terminal.Cell{Time: expires.Time}
 	if item.Expired {
 		expires = terminal.Cell{Text: "expired", Color: "red"}
 	}
@@ -84,7 +83,7 @@ func artifactRow(item services.Artifact) []terminal.Cell {
 		{Text: terminal.Bytes(item.Size)},
 		{Text: strconv.FormatInt(item.Size, 10)},
 		expires,
-		{Text: expiresAt},
+		piped,
 		{Text: strconv.FormatBool(item.Expired)},
 	}
 }
@@ -122,7 +121,7 @@ $ tea run download 34 --zip`,
 			if err != nil {
 				return err
 			}
-			selected, err := selectArtifacts(artifacts, names, run)
+			selected, err := selectArtifacts(latestArtifacts(artifacts), names, run)
 			if err != nil {
 				return err
 			}
@@ -153,6 +152,24 @@ $ tea run download 34 --zip`,
 	return cmd
 }
 
+// latestArtifacts keeps the newest artifact of each name: after a rerun, the run lists the artifacts
+// of every attempt, which may share names.
+func latestArtifacts(artifacts []services.Artifact) []services.Artifact {
+	index := map[string]int{}
+	latest := make([]services.Artifact, 0, len(artifacts))
+	for _, artifact := range artifacts {
+		if i, seen := index[artifact.Name]; seen {
+			if artifact.ID > latest[i].ID {
+				latest[i] = artifact
+			}
+			continue
+		}
+		index[artifact.Name] = len(latest)
+		latest = append(latest, artifact)
+	}
+	return latest
+}
+
 // selectArtifacts returns the artifacts of a run with the given names, or all of them.
 func selectArtifacts(artifacts []services.Artifact, names []string, run int64) ([]services.Artifact, error) {
 	if len(artifacts) == 0 {
@@ -167,7 +184,7 @@ func selectArtifacts(artifacts []services.Artifact, names []string, run int64) (
 		if index < 0 {
 			available := make([]string, len(artifacts))
 			for i, artifact := range artifacts {
-				available[i] = artifact.Name
+				available[i] = terminal.SafeLine(artifact.Name)
 			}
 			return nil, fmt.Errorf("artifact %q not found in run %d (artifacts: %s)", name, run, strings.Join(available, ", "))
 		}

internal/app/artifacts_test.go

@@ -7,10 +7,12 @@ import (
 	"archive/zip"
 	"bytes"
 	"context"
+	"fmt"
 	"net/http"
 	"os"
 	"path/filepath"
 	"runtime"
+	"strconv"
 	"strings"
 	"testing"
 
@@ -63,7 +65,6 @@ func TestRunDownloadExtractsArtifactsByName(t *testing.T) {
 		"GET actions/runs/29/artifacts": `{"total_count":3,"artifacts":[{"id":1,"name":"build","size_in_bytes":3},{"id":2,"name":"coverage"},{"id":3,"name":"old","expired":true}]}`,
 		"GET actions/artifacts/1/zip":   zipOf(t, map[string]string{"bin/tea": "tea", "README": "read me"}),
 		"GET actions/artifacts/2/zip":   zipOf(t, map[string]string{"cover.out": "100%"}),
-		"GET actions/artifacts/4/zip":   zipOf(t, map[string]string{"../escape": "x"}),
 	})
 	for _, tc := range []struct {
 		name  string
@@ -108,6 +109,39 @@ func TestRunDownloadExtractsArtifactsByName(t *testing.T) {
 			}
 		})
 	}
+	t.Run("newest artifact of a rerun name", func(t *testing.T) {
+		server, _ := actionsAPI(t, map[string]string{
+			"GET actions/runs/29/artifacts": `{"total_count":3,"artifacts":[{"id":1,"name":"dist"},{"id":2,"name":"docs"},{"id":3,"name":"dist"}]}`,
+			"GET actions/artifacts/2/zip":   zipOf(t, map[string]string{"index": "docs"}),
+			"GET actions/artifacts/3/zip":   zipOf(t, map[string]string{"app": "new"}),
+		})
+		dir := t.TempDir()
+		deps, _, _ := actionsDeps(t, server.URL, false)
+		require.NoError(t, Execute(context.Background(), deps, []string{"run", "download", "29", "-R", "alice/project", "-D", dir}))
+		data, err := os.ReadFile(filepath.Join(dir, "dist", "app"))
+		require.NoError(t, err)
+		require.Equal(t, "new", string(data))
+		require.NoError(t, Execute(context.Background(), deps, []string{"run", "download", "29", "-R", "alice/project", "-D", dir, "-n", "dist", "--clobber"}))
+	})
+	t.Run("unknown name lists artifacts without control characters", func(t *testing.T) {
+		server, _ := actionsAPI(t, map[string]string{"GET actions/runs/29/artifacts": `{"total_count":1,"artifacts":[{"id":4,"name":"build\u001b]0;pwned\u0007"}]}`})
+		deps, _, _ := actionsDeps(t, server.URL, false)
+		err := Execute(context.Background(), deps, []string{"run", "download", "29", "-R", "alice/project", "-D", t.TempDir(), "-n", "docs"})
+		require.EqualError(t, err, `artifact "docs" not found in run 29 (artifacts: build]0;pwned)`)
+	})
+	t.Run("unsafe artifact names", func(t *testing.T) {
+		for _, name := range []string{"../evil", "..", "a/b", `a\b`, "/abs"} {
+			server, requests := actionsAPI(t, map[string]string{"GET actions/runs/29/artifacts": `{"total_count":1,"artifacts":[{"id":4,"name":` + strconv.Quote(name) + `}]}`})
+			dir := t.TempDir()
+			deps, _, _ := actionsDeps(t, server.URL, false)
+			err := Execute(context.Background(), deps, []string{"run", "download", "29", "-R", "alice/project", "-D", filepath.Join(dir, "out")})
+			require.ErrorContains(t, err, fmt.Sprintf("cannot use %q as a directory name", name))
+			require.Equal(t, []string{"GET actions/runs/29/artifacts"}, *requests)
+			entries, err := os.ReadDir(dir)
+			require.NoError(t, err)
+			require.Empty(t, entries)
+		}
+	})
 	t.Run("existing files and unsafe paths", func(t *testing.T) {
 		dir := t.TempDir()
 		require.NoError(t, os.WriteFile(filepath.Join(dir, "cover.out"), []byte("mine"), 0o600))
@@ -156,7 +190,7 @@ func TestExtractZipKeepsPermissionBits(t *testing.T) {
 
 func TestArtifactListShowsSizesAndExpiry(t *testing.T) {
 	server, _ := actionsAPI(t, map[string]string{
-		"GET actions/runs/29/artifacts": `{"total_count":2,"artifacts":[{"id":1,"name":"build","size_in_bytes":3145728,"expires_at":"2028-03-01T12:00:00Z"},{"id":3,"name":"old","size_in_bytes":13,"expired":true,"expires_at":"2027-12-01T12:00:00Z"}]}`,
+		"GET actions/runs/29/artifacts": `{"total_count":2,"artifacts":[{"id":1,"name":"build","size_in_bytes":3145728,"expires_at":"2028-03-01T14:00:00+02:00"},{"id":3,"name":"old","size_in_bytes":13,"expired":true,"expires_at":"2027-12-01T12:00:00Z"}]}`,
 	})
 	for tty, want := range map[bool]string{
 		true:  "Showing 2 of 2 artifacts of run 29\n\nID  NAME   SIZE     EXPIRES\n1   build  3.0 MiB  in about 2 months\n3   old    13 B     expired\n",

internal/app/target.go

@@ -356,6 +356,11 @@ func (a *application) selectHost(t *target, s settings.Snapshot) error {
 		}
 		t.Host, t.HostSource = explicit, "--host"
 	}
+	return a.defaultHost(t, s)
+}
+
+// defaultHost picks the instance of a target that neither the repository nor --host named.
+func (a *application) defaultHost(t *target, s settings.Snapshot) error {
 	if t.Host == "" {
 		for _, key := range []string{"TEA_HOST", "GITEA_INSTANCE_URL"} {
 			if value := a.deps.Getenv(key); value != "" {