From e62891400468310bd8563c278ad18ae74983fc8e Mon Sep 17 00:00:00 2001 From: Rene Turcios <95189389+CloudCorpRecords@users.noreply.github.com> Date: Tue, 13 Oct 2026 10:07:54 -0700 Subject: [PATCH 1/2] Add --tag to release upload for parity with release create Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Rene Turcios <95189389+CloudCorpRecords@users.noreply.github.com> --- internal/cli/release.go | 35 ++++++++++++++++++++++++---- internal/cli/release_test.go | 44 ++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 5 deletions(-) diff --git a/internal/cli/release.go b/internal/cli/release.go index fb8d678..56c5697 100644 --- a/internal/cli/release.go +++ b/internal/cli/release.go @@ -312,14 +312,36 @@ func releaseDeleteCmd() *cobra.Command { return cmd } +// uploadTagAndFile resolves the release tag and file for "release upload". The tag comes from --tag with the file as +// the only argument, or from the first of two positional arguments. +func uploadTagAndFile(flagTag string, args []string) (tag, file string, err error) { + if flagTag != "" { + if len(args) != 1 { + return "", "", fmt.Errorf("with --tag, expected one argument, got %d", len(args)) + } + return flagTag, args[0], nil + } + if len(args) != releaseAssetArgs { + return "", "", fmt.Errorf("expected (or --tag ), got %d argument(s)", len(args)) + } + return args[0], args[1], nil +} + func releaseUploadCmd() *cobra.Command { - return &cobra.Command{ - Use: "upload ", + var flagTag string + + cmd := &cobra.Command{ + Use: "upload ( | --tag )", Short: "Upload an asset to a release", - Args: cobra.ExactArgs(releaseAssetArgs), + Args: func(cmd *cobra.Command, args []string) error { + _, _, err := uploadTagAndFile(flagTag, args) + return err + }, RunE: func(cmd *cobra.Command, args []string) error { - tag := args[0] - filePath := args[1] + tag, filePath, err := uploadTagAndFile(flagTag, args) + if err != nil { + return err + } forge, owner, repoName, _, err := resolve.Repo(flagRepo, flagForgeType) if err != nil { @@ -349,6 +371,9 @@ func releaseUploadCmd() *cobra.Command { return nil }, } + + cmd.Flags().StringVar(&flagTag, "tag", "", "Tag name of the release (alternative to the positional )") + return cmd } func releaseDownloadCmd() *cobra.Command { diff --git a/internal/cli/release_test.go b/internal/cli/release_test.go index ee682ad..0565b8f 100644 --- a/internal/cli/release_test.go +++ b/internal/cli/release_test.go @@ -68,3 +68,47 @@ func TestReleaseSubcommands(t *testing.T) { } } } + +func TestReleaseUploadTagFlag(t *testing.T) { + cmd := releaseUploadCmd() + + f := cmd.Flags().Lookup("tag") + if f == nil { + t.Fatal("expected --tag flag to exist") + } + if f.Value.Type() != "string" { + t.Errorf("expected --tag to be string, got %s", f.Value.Type()) + } +} + +func TestUploadTagAndFile(t *testing.T) { + tests := []struct { + name string + flagTag string + args []string + wantTag string + wantFile string + wantErr bool + }{ + {name: "positional tag and file", args: []string{"v1.0.0", "app.zip"}, wantTag: "v1.0.0", wantFile: "app.zip"}, + {name: "tag flag and file", flagTag: "v1.0.0", args: []string{"app.zip"}, wantTag: "v1.0.0", wantFile: "app.zip"}, + {name: "tag flag with positional tag too", flagTag: "v1.0.0", args: []string{"v1.0.0", "app.zip"}, wantErr: true}, + {name: "no tag at all", args: []string{"app.zip"}, wantErr: true}, + {name: "no arguments", wantErr: true}, + {name: "tag flag without a file", flagTag: "v1.0.0", wantErr: true}, + {name: "too many positional arguments", args: []string{"v1.0.0", "a.zip", "b.zip"}, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tag, file, err := uploadTagAndFile(tt.flagTag, tt.args) + if (err != nil) != tt.wantErr { + t.Fatalf("uploadTagAndFile(%q, %v) error = %v, wantErr %v", tt.flagTag, tt.args, err, tt.wantErr) + } + if tag != tt.wantTag || file != tt.wantFile { + t.Errorf("uploadTagAndFile(%q, %v) = (%q, %q), want (%q, %q)", + tt.flagTag, tt.args, tag, file, tt.wantTag, tt.wantFile) + } + }) + } +} From c87889d46d960bafdb5c627f913239f67325c177 Mon Sep 17 00:00:00 2001 From: Rene Turcios <95189389+CloudCorpRecords@users.noreply.github.com> Date: Sun, 18 Oct 2026 10:13:21 -0700 Subject: [PATCH 2/2] fix(cli): reject empty --tag on release upload, test through execution andrew (re-review on #172): --tag='' bypassed conflict validation and fell through to positional mode. uploadTagAndFile now takes Changed("tag") and rejects an explicitly empty flag value, so an unset script variable cannot silently select positional mode. Tests run the command end to end for both supported forms and the invalid combinations, asserting the tag and file contents passed to UploadAsset (and zero calls on refusal). Signed-off-by: Rene Turcios <95189389+CloudCorpRecords@users.noreply.github.com> --- internal/cli/pr_checkout_test.go | 7 +- internal/cli/release.go | 15 ++-- internal/cli/release_test.go | 148 ++++++++++++++++++++++++++----- 3 files changed, 142 insertions(+), 28 deletions(-) diff --git a/internal/cli/pr_checkout_test.go b/internal/cli/pr_checkout_test.go index 6c90862..8d960f6 100644 --- a/internal/cli/pr_checkout_test.go +++ b/internal/cli/pr_checkout_test.go @@ -84,8 +84,9 @@ func (m *mockPRService) ListURL(_ string) string { // mockForge implements forges.Forge for testing. type mockForge struct { - prService *mockPRService - repoService forges.RepoService + prService *mockPRService + repoService forges.RepoService + releaseService forges.ReleaseService } func (m *mockForge) Repos() forges.RepoService { return m.repoService } @@ -93,7 +94,7 @@ func (m *mockForge) Issues() forges.IssueService { return nil } func (m *mockForge) PullRequests() forges.PullRequestService { return m.prService } func (m *mockForge) Labels() forges.LabelService { return nil } func (m *mockForge) Milestones() forges.MilestoneService { return nil } -func (m *mockForge) Releases() forges.ReleaseService { return nil } +func (m *mockForge) Releases() forges.ReleaseService { return m.releaseService } func (m *mockForge) CI() forges.CIService { return nil } func (m *mockForge) Branches() forges.BranchService { return nil } func (m *mockForge) DeployKeys() forges.DeployKeyService { return nil } diff --git a/internal/cli/release.go b/internal/cli/release.go index 56c5697..c10e245 100644 --- a/internal/cli/release.go +++ b/internal/cli/release.go @@ -313,9 +313,14 @@ func releaseDeleteCmd() *cobra.Command { } // uploadTagAndFile resolves the release tag and file for "release upload". The tag comes from --tag with the file as -// the only argument, or from the first of two positional arguments. -func uploadTagAndFile(flagTag string, args []string) (tag, file string, err error) { - if flagTag != "" { +// the only argument, or from the first of two positional arguments. tagChanged must be +// cmd.Flags().Changed("tag"): an explicitly empty --tag is an error, not an unset flag, so an +// unset script variable cannot silently select positional mode. +func uploadTagAndFile(flagTag string, tagChanged bool, args []string) (tag, file string, err error) { + if tagChanged { + if flagTag == "" { + return "", "", fmt.Errorf("--tag value must not be empty") + } if len(args) != 1 { return "", "", fmt.Errorf("with --tag, expected one argument, got %d", len(args)) } @@ -334,11 +339,11 @@ func releaseUploadCmd() *cobra.Command { Use: "upload ( | --tag )", Short: "Upload an asset to a release", Args: func(cmd *cobra.Command, args []string) error { - _, _, err := uploadTagAndFile(flagTag, args) + _, _, err := uploadTagAndFile(flagTag, cmd.Flags().Changed("tag"), args) return err }, RunE: func(cmd *cobra.Command, args []string) error { - tag, filePath, err := uploadTagAndFile(flagTag, args) + tag, filePath, err := uploadTagAndFile(flagTag, cmd.Flags().Changed("tag"), args) if err != nil { return err } diff --git a/internal/cli/release_test.go b/internal/cli/release_test.go index 0565b8f..1527cc5 100644 --- a/internal/cli/release_test.go +++ b/internal/cli/release_test.go @@ -1,9 +1,16 @@ package cli import ( + "context" + "io" + "os" + "path/filepath" "testing" "github.com/spf13/cobra" + + "github.com/git-pkgs/forge" + "github.com/git-pkgs/forge/internal/resolve" ) const flagTypeBool = "bool" @@ -69,45 +76,146 @@ func TestReleaseSubcommands(t *testing.T) { } } -func TestReleaseUploadTagFlag(t *testing.T) { - cmd := releaseUploadCmd() +// mockReleaseService implements forges.ReleaseService, recording UploadAsset calls. +type mockReleaseService struct { + uploadCalls int + uploadTag string + uploadBody []byte +} - f := cmd.Flags().Lookup("tag") - if f == nil { - t.Fatal("expected --tag flag to exist") +func (m *mockReleaseService) List(_ context.Context, _, _ string, _ forges.ListReleaseOpts) ([]forges.Release, error) { + return nil, nil +} + +func (m *mockReleaseService) Get(_ context.Context, _, _, _ string) (*forges.Release, error) { + return nil, nil +} + +func (m *mockReleaseService) GetLatest(_ context.Context, _, _ string) (*forges.Release, error) { + return nil, nil +} + +func (m *mockReleaseService) Create(_ context.Context, _, _ string, _ forges.CreateReleaseOpts) (*forges.Release, error) { + return nil, nil +} + +func (m *mockReleaseService) Update(_ context.Context, _, _, _ string, _ forges.UpdateReleaseOpts) (*forges.Release, error) { + return nil, nil +} + +func (m *mockReleaseService) Delete(_ context.Context, _, _, _ string) error { + return nil +} + +func (m *mockReleaseService) UploadAsset(_ context.Context, _, _, tag string, file *os.File) (*forges.ReleaseAsset, error) { + m.uploadCalls++ + m.uploadTag = tag + body, err := io.ReadAll(file) + if err != nil { + return nil, err } - if f.Value.Type() != "string" { - t.Errorf("expected --tag to be string, got %s", f.Value.Type()) + m.uploadBody = body + return &forges.ReleaseAsset{Name: filepath.Base(file.Name()), Size: len(body)}, nil +} + +func (m *mockReleaseService) DownloadAsset(_ context.Context, _, _ string, _ int64) (io.ReadCloser, error) { + return nil, nil +} + +func TestReleaseUploadCommand(t *testing.T) { + const contents = "fake-zip-bytes" + + tests := []struct { + name string + args []string + wantTag string + wantErr bool + }{ + {name: "positional tag and file", args: []string{"v1.0.0", "{file}"}, wantTag: "v1.0.0"}, + {name: "tag flag and file", args: []string{"--tag", "v1.0.0", "{file}"}, wantTag: "v1.0.0"}, + {name: "tag flag with positional tag too", args: []string{"--tag", "v1.0.0", "v1.0.0", "{file}"}, wantErr: true}, + {name: "empty tag flag with positional tag", args: []string{"--tag", "", "v1.0.0", "{file}"}, wantErr: true}, + {name: "empty tag flag without positional tag", args: []string{"--tag", "", "{file}"}, wantErr: true}, + {name: "no tag at all", args: []string{"{file}"}, wantErr: true}, + {name: "tag flag without a file", args: []string{"--tag", "v1.0.0"}, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + file := filepath.Join(t.TempDir(), "app.zip") + if err := os.WriteFile(file, []byte(contents), 0644); err != nil { + t.Fatalf("writing test file: %v", err) + } + args := make([]string, len(tt.args)) + for i, a := range tt.args { + if a == "{file}" { + a = file + } + args[i] = a + } + + releases := &mockReleaseService{} + resolve.SetTestForge(&mockForge{releaseService: releases}, "owner", "repo", "example.com") + t.Cleanup(resolve.ResetTestForge) + + cmd := releaseUploadCmd() + cmd.SetArgs(args) + err := cmd.Execute() + if tt.wantErr { + if err == nil { + t.Fatalf("expected error, got success (tag %q uploaded)", releases.uploadTag) + } + if releases.uploadCalls != 0 { + t.Errorf("UploadAsset calls = %d, want 0", releases.uploadCalls) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if releases.uploadCalls != 1 { + t.Fatalf("UploadAsset calls = %d, want 1", releases.uploadCalls) + } + if releases.uploadTag != tt.wantTag { + t.Errorf("uploaded tag = %q, want %q", releases.uploadTag, tt.wantTag) + } + if string(releases.uploadBody) != contents { + t.Errorf("uploaded body = %q, want %q", releases.uploadBody, contents) + } + }) } } func TestUploadTagAndFile(t *testing.T) { tests := []struct { - name string - flagTag string - args []string - wantTag string - wantFile string - wantErr bool + name string + flagTag string + tagChanged bool + args []string + wantTag string + wantFile string + wantErr bool }{ {name: "positional tag and file", args: []string{"v1.0.0", "app.zip"}, wantTag: "v1.0.0", wantFile: "app.zip"}, - {name: "tag flag and file", flagTag: "v1.0.0", args: []string{"app.zip"}, wantTag: "v1.0.0", wantFile: "app.zip"}, - {name: "tag flag with positional tag too", flagTag: "v1.0.0", args: []string{"v1.0.0", "app.zip"}, wantErr: true}, + {name: "tag flag and file", flagTag: "v1.0.0", tagChanged: true, args: []string{"app.zip"}, wantTag: "v1.0.0", wantFile: "app.zip"}, + {name: "tag flag with positional tag too", flagTag: "v1.0.0", tagChanged: true, args: []string{"v1.0.0", "app.zip"}, wantErr: true}, + {name: "empty tag flag with positional tag", tagChanged: true, args: []string{"v1.0.0", "app.zip"}, wantErr: true}, + {name: "empty tag flag without positional tag", tagChanged: true, args: []string{"app.zip"}, wantErr: true}, {name: "no tag at all", args: []string{"app.zip"}, wantErr: true}, {name: "no arguments", wantErr: true}, - {name: "tag flag without a file", flagTag: "v1.0.0", wantErr: true}, + {name: "tag flag without a file", flagTag: "v1.0.0", tagChanged: true, wantErr: true}, {name: "too many positional arguments", args: []string{"v1.0.0", "a.zip", "b.zip"}, wantErr: true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - tag, file, err := uploadTagAndFile(tt.flagTag, tt.args) + tag, file, err := uploadTagAndFile(tt.flagTag, tt.tagChanged, tt.args) if (err != nil) != tt.wantErr { - t.Fatalf("uploadTagAndFile(%q, %v) error = %v, wantErr %v", tt.flagTag, tt.args, err, tt.wantErr) + t.Fatalf("uploadTagAndFile(%q, %v, %v) error = %v, wantErr %v", tt.flagTag, tt.tagChanged, tt.args, err, tt.wantErr) } if tag != tt.wantTag || file != tt.wantFile { - t.Errorf("uploadTagAndFile(%q, %v) = (%q, %q), want (%q, %q)", - tt.flagTag, tt.args, tag, file, tt.wantTag, tt.wantFile) + t.Errorf("uploadTagAndFile(%q, %v, %v) = (%q, %q), want (%q, %q)", + tt.flagTag, tt.tagChanged, tt.args, tag, file, tt.wantTag, tt.wantFile) } }) }