fix: address verified review findings (actions-workflows-values-1)

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

internal/app/action_values.go

@@ -11,6 +11,7 @@ import (
 	"io"
 	"regexp"
 	"strings"
+	"unicode/utf8"
 
 	"gitea.dev/tea/internal/clierr"
 	"gitea.dev/tea/internal/services"
@@ -176,6 +177,10 @@ $ echo -n "$TOKEN" | tea secret set NPM_TOKEN --org acme
 # set the secrets of a dotenv file
 $ tea secret set --env-file .env`
 	}
+	descriptionNote := ""
+	if secret {
+		descriptionNote = "\n\nSetting an existing secret without --description clears its description."
+	}
 	cmd := &cobra.Command{
 		Use:   "set [<name>]",
 		Short: "Create or update a " + kind,
@@ -183,7 +188,7 @@ $ tea secret set --env-file .env`
 
 The value comes from --body, --body-file or standard input when it is not a terminal (one
 trailing line break is removed), or else from a prompt. --env-file sets every NAME=value of a
-dotenv file instead.`, kind),
+dotenv file instead.%[2]s`, kind, descriptionNote),
 		Example: example,
 		Args:    usageArgs(cobra.MaximumNArgs(1)),
 		RunE: func(cmd *cobra.Command, args []string) error {
@@ -193,6 +198,11 @@ dotenv file instead.`, kind),
 			if envFile != "" && (cmd.Flags().Changed("body") || cmd.Flags().Changed("body-file")) {
 				return clierr.Wrap(clierr.Usage, errors.New("--env-file cannot be combined with --body or --body-file"))
 			}
+			if envFile == "" {
+				if err := checkActionValueName(args[0]); err != nil {
+					return err
+				}
+			}
 			var entries []envEntry
 			if envFile != "" {
 				data, err := readFileOrStdin(a.deps.Streams.In, envFile)
@@ -216,8 +226,8 @@ dotenv file instead.`, kind),
 				if entry.value == "" {
 					return clierr.Wrap(clierr.Usage, fmt.Errorf("the value of %s is empty", entry.name))
 				}
-				if !secret && len(entry.value) > 65536 {
-					return clierr.Wrap(clierr.Usage, fmt.Errorf("the value of %s exceeds 64 KiB", entry.name))
+				if !secret && utf8.RuneCountInString(entry.value) > 65536 {
+					return clierr.Wrap(clierr.Usage, fmt.Errorf("the value of %s exceeds 65536 characters", entry.name))
 				}
 			}
 			selected, t, err := scope.resolve(a, cmd)
@@ -244,8 +254,12 @@ dotenv file instead.`, kind),
 				}, func(item services.ActionSecret) string { return report(item.Name) })
 			}
 			service := services.ActionVariables{Client: t.client, Scope: selected}
+			var newDescription *string
+			if cmd.Flags().Changed("description") {
+				newDescription = &description
+			}
 			return batchDone(a, cmd, entries, label, func(entry envEntry) (services.ActionVariable, error) {
-				return service.Set(cmd.Context(), entry.name, entry.value, description)
+				return service.Set(cmd.Context(), entry.name, entry.value, newDescription)
 			}, func(item services.ActionVariable) string { return report(item.Name) })
 		},
 	}
@@ -304,14 +318,20 @@ func parseEnvFile(data []byte) ([]envEntry, error) {
 		if !ok || name == "" {
 			return nil, fmt.Errorf("line %d: use NAME=value", number)
 		}
-		switch {
-		case len(value) >= 2 && value[0] == '"' && value[len(value)-1] == '"':
-			value = strings.ReplaceAll(value[1:len(value)-1], `\n`, "\n")
-		case len(value) >= 2 && value[0] == '\'' && value[len(value)-1] == '\'':
-			value = value[1 : len(value)-1]
-		default:
-			value, _, _ = strings.Cut(value, " #")
+		if quote := value[:min(len(value), 1)]; quote == `"` || quote == "'" {
+			// The value ends at the closing quote; only a # comment may follow it.
+			closing := strings.Index(value[1:], quote) + 1
+			rest := strings.TrimSpace(value[min(closing+1, len(value)):])
+			if closing > 0 && (rest == "" || strings.HasPrefix(rest, "#")) {
+				value = value[1:closing]
+				if quote == `"` {
+					value = strings.ReplaceAll(value, `\n`, "\n")
+				}
+				entries = append(entries, envEntry{name, value})
+				continue
+			}
 		}
+		value, _, _ = strings.Cut(value, " #")
 		entries = append(entries, envEntry{name, value})
 	}
 	if len(entries) == 0 {

internal/app/action_values_test.go

@@ -6,6 +6,7 @@ package app
 import (
 	"context"
 	"encoding/json"
+	"io"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -66,7 +67,7 @@ func TestActionValueSetReadsValuesAndReportsStoredNames(t *testing.T) {
 			[]string{"secret", "set", "token", "--body", "x", "--json"},
 			"",
 			[]string{`PUT actions/secrets/token {"data":"x","description":""}`},
-			`{"name":"TOKEN","description":""}`, "",
+			`{"name":"TOKEN","description":"","created_at":""}`, "",
 		},
 		{
 			"organization",
@@ -99,9 +100,9 @@ func TestActionValueSetReadsValuesAndReportsStoredNames(t *testing.T) {
 			server, requests := actionsAPI(t, map[string]string{
 				"PUT actions/secrets/lower_name": "204 ", "PUT actions/secrets/TOKEN": "204 ", "PUT actions/secrets/token": "204 ",
 				"PUT api/v1/orgs/acme/actions/secrets/TOKEN": "204 ", "PUT api/v1/user/actions/secrets/TOKEN": "204 ",
-				"GET api/v1/orgs/acme/actions/variables/API_KEY": `404 {}`, "POST api/v1/orgs/acme/actions/variables/API_KEY": "201 ",
-				"GET api/v1/orgs/acme/actions/variables/region": `404 {}`, "POST api/v1/orgs/acme/actions/variables/region": "201 ",
-				"GET api/v1/orgs/acme/actions/variables/MODE": `404 {}`, "POST api/v1/orgs/acme/actions/variables/MODE": "201 ",
+				"GET api/v1/orgs/acme/actions/variables/API_KEY": `404 {"message":"variable not found"}`, "POST api/v1/orgs/acme/actions/variables/API_KEY": "201 ",
+				"GET api/v1/orgs/acme/actions/variables/region": `404 {"message":"variable not found"}`, "POST api/v1/orgs/acme/actions/variables/region": "201 ",
+				"GET api/v1/orgs/acme/actions/variables/MODE": `404 {"message":"variable not found"}`, "POST api/v1/orgs/acme/actions/variables/MODE": "201 ",
 			})
 			deps, out, diagnostics := actionsDeps(t, server.URL, true)
 			deps.Streams.In = strings.NewReader(tc.stdin)
@@ -176,3 +177,35 @@ func TestActionValueDeleteConfirmsWithTheScope(t *testing.T) {
 		require.Equal(t, "✓ Deleted Actions secret DEPLOY_TOKEN from alice/project\n", diagnostics.String())
 	}
 }
+
+func TestParseEnvFileStripsQuotesBeforeAComment(t *testing.T) {
+	entries, err := parseEnvFile([]byte("A=\"s3cr3t\" # prod\nB='v' # c\nC=\"a # b\"\nD=plain # note\nE=\"x\\ny\"\nF=\"half\n"))
+	require.NoError(t, err)
+	require.Equal(t, []envEntry{{"A", "s3cr3t"}, {"B", "v"}, {"C", "a # b"}, {"D", "plain"}, {"E", "x\ny"}, {"F", "\"half"}}, entries)
+}
+
+func TestSecretSetChecksTheNameBeforeReadingTheValue(t *testing.T) {
+	deps, _, _ := actionsDeps(t, "http://127.0.0.1:1", true)
+	read := &stdinReads{}
+	deps.Streams.In = read
+	err := Execute(context.Background(), deps, []string{"secret", "set", "GITHUB_PAT", "-R", "alice/project"})
+	require.ErrorContains(t, err, "reserved")
+	require.Zero(t, read.reads, "the value must not be read for a refused name")
+}
+
+type stdinReads struct{ reads int }
+
+func (r *stdinReads) Read([]byte) (int, error) { r.reads++; return 0, io.EOF }
+
+func TestVariableSetCountsCharactersNotBytes(t *testing.T) {
+	server, requests := actionsAPI(t, map[string]string{
+		"GET api/v1/orgs/acme/actions/variables/WIDE": `404 {"message":"variable not found"}`, "POST api/v1/orgs/acme/actions/variables/WIDE": "201 ",
+	})
+	deps, _, _ := actionsDeps(t, server.URL, true)
+	deps.Streams.In = strings.NewReader(strings.Repeat("é", 40000))
+	require.NoError(t, Execute(context.Background(), deps, []string{"variable", "set", "WIDE", "-o", "acme"}))
+	require.Len(t, *requests, 2)
+	deps, _, _ = actionsDeps(t, server.URL, true)
+	deps.Streams.In = strings.NewReader(strings.Repeat("é", 65537))
+	require.ErrorContains(t, Execute(context.Background(), deps, []string{"variable", "set", "WIDE", "-o", "acme"}), "65536 characters")
+}

internal/app/alias_completion_test.go

@@ -347,3 +347,33 @@ func TestAccountCompletionUsesInstanceOfRepository(t *testing.T) {
 		require.Equal(t, []string{"adam\thttps://a.example.org", "alice\thttps://a.example.org (active)"}, values, command)
 	}
 }
+
+func TestActionValueCompletionOutsideARepository(t *testing.T) {
+	server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/orgs/acme/actions/variables":
+			_, _ = w.Write([]byte(`[{"name":"REGION","data":"eu"}]`))
+		case "/api/v1/user/actions/variables":
+			_, _ = w.Write([]byte(`[{"name":"HOME_REGION","data":"eu"}]`))
+		default:
+			t.Errorf("unexpected request %s", r.URL.Path)
+			w.WriteHeader(http.StatusTeapot)
+		}
+	}))
+	defer server.Close()
+	deps, _, _ := serverDeps(t, server.URL)
+	a := &application{deps: deps}
+	cmd := &cobra.Command{}
+	cmd.SetContext(context.Background())
+	for name, tc := range map[string]struct {
+		scope services.ActionScope
+		want  []string
+	}{
+		"organization": {services.ActionScope{Organization: "acme"}, []string{"REGION"}},
+		"account":      {services.ActionScope{Account: true}, []string{"HOME_REGION"}},
+		"repository":   {services.ActionScope{}, nil},
+	} {
+		values, _ := a.completeVariables(func() services.ActionScope { return tc.scope })(cmd, nil, "")
+		require.ElementsMatch(t, tc.want, values, name)
+	}
+}

internal/app/completion.go

@@ -490,16 +490,24 @@ func (a *application) completeRuns() cobra.CompletionFunc {
 
 // completeSecrets completes Actions secret names in the selected scope.
 func (a *application) completeSecrets(scope func() services.ActionScope) cobra.CompletionFunc {
-	return a.apiCompletion(true, func(ctx context.Context, t target, _ string) ([]cobra.Completion, error) {
-		items, err := services.ActionSecrets{Client: t.client, Scope: actionScope(scope, t)}.ListPage(ctx, completionLimit, 1)
+	return a.apiCompletion(false, func(ctx context.Context, t target, _ string) ([]cobra.Completion, error) {
+		selected := actionScope(scope, t)
+		if selected.Repository == "" && selected.Organization == "" && !selected.Account {
+			return nil, nil
+		}
+		items, err := services.ActionSecrets{Client: t.client, Scope: selected}.ListPage(ctx, completionLimit, 1)
 		return describe(items.Items, func(item services.ActionSecret) (string, string) { return item.Name, item.Description }), err
 	})
 }
 
 // completeVariables completes Actions variable names in the selected scope.
 func (a *application) completeVariables(scope func() services.ActionScope) cobra.CompletionFunc {
-	return a.apiCompletion(true, func(ctx context.Context, t target, _ string) ([]cobra.Completion, error) {
-		items, err := services.ActionVariables{Client: t.client, Scope: actionScope(scope, t)}.ListPage(ctx, completionLimit, 1)
+	return a.apiCompletion(false, func(ctx context.Context, t target, _ string) ([]cobra.Completion, error) {
+		selected := actionScope(scope, t)
+		if selected.Repository == "" && selected.Organization == "" && !selected.Account {
+			return nil, nil
+		}
+		items, err := services.ActionVariables{Client: t.client, Scope: selected}.ListPage(ctx, completionLimit, 1)
 		return describe(items.Items, func(item services.ActionVariable) (string, string) { return item.Name, item.Description }), err
 	})
 }

internal/app/validation_test.go

@@ -294,6 +294,7 @@ func TestPromptedCreateSubmitsWithoutConfirmation(t *testing.T) {
 			server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 				if r.Method == http.MethodGet {
 					w.WriteHeader(http.StatusNotFound)
+					_, _ = w.Write([]byte(`{"message":"variable not found"}`))
 					return
 				}
 				writes = append(writes, r.Method+" "+r.URL.Path)