From a2acd5666127c02f718952e67159a2b3da05309a Mon Sep 17 00:00:00 2001 From: schurchleycci Date: Fri, 26 Jun 2026 15:34:16 -0400 Subject: [PATCH 1/6] Use go-git to expand short SHAs in run watch Co-Authored-By: Claude Sonnet 4.6 (1M context) --- internal/cmd/run/watch.go | 4 +++- internal/gitremote/detect.go | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/internal/cmd/run/watch.go b/internal/cmd/run/watch.go index b8c94cf63..85a50d79d 100644 --- a/internal/cmd/run/watch.go +++ b/internal/cmd/run/watch.go @@ -157,7 +157,7 @@ func runWatch(ctx context.Context, client *apiclient.Client, args []string, proj if projectSlug == "" { projectSlug = info.Slug } - if branch == "" { + if branch == "" && sha == "" { branch = info.Branch } } @@ -234,6 +234,8 @@ func waitForRunBySHA(ctx context.Context, client *apiclient.Client, projectSlug, interval := 5 * time.Second printed := false + sha = gitremote.ExpandSHA(sha) + filter := fmt.Sprintf("pipeline.git.revision == %q", sha) if branch != "" { filter += fmt.Sprintf(" and pipeline.git.branch == %q", branch) diff --git a/internal/gitremote/detect.go b/internal/gitremote/detect.go index e5465c418..24486f8a7 100644 --- a/internal/gitremote/detect.go +++ b/internal/gitremote/detect.go @@ -283,6 +283,24 @@ func gitCurrentBranch(repo *git.Repository) (string, error) { return head.Name().Short(), nil } +// ExpandSHA attempts to resolve an abbreviated git SHA to its full 40-character +// form. Returns the input unchanged if the repo cannot be opened, the SHA isn't +// found, or the input is already 40 characters. +func ExpandSHA(sha string) string { + if len(sha) == 40 { + return sha + } + repo, err := openRepo() + if err != nil { + return sha + } + hash, err := repo.ResolveRevision(plumbing.Revision(sha)) + if err != nil { + return sha + } + return hash.String() +} + // gitDefaultBranch returns the short name of the remote default branch (e.g. // "main"), read from the symbolic ref refs/remotes/origin/HEAD. This is the // "origin/"-stripped equivalent of `git rev-parse --abbrev-ref origin/HEAD`. From 14429e376f15c8785cb28ccd90db3347c06896e0 Mon Sep 17 00:00:00 2001 From: schurchleycci Date: Fri, 26 Jun 2026 15:43:30 -0400 Subject: [PATCH 2/6] Fail fast when short SHA cannot be resolved locally Co-Authored-By: Claude Sonnet 4.6 (1M context) --- internal/cmd/run/watch.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/internal/cmd/run/watch.go b/internal/cmd/run/watch.go index 85a50d79d..12aeb02cc 100644 --- a/internal/cmd/run/watch.go +++ b/internal/cmd/run/watch.go @@ -234,7 +234,17 @@ func waitForRunBySHA(ctx context.Context, client *apiclient.Client, projectSlug, interval := 5 * time.Second printed := false - sha = gitremote.ExpandSHA(sha) + if expanded := gitremote.ExpandSHA(sha); expanded != sha { + sha = expanded + } else if len(sha) < 40 { + return nil, clierrors.New("run.invalid_sha", "Commit not found", + fmt.Sprintf("Commit %q does not exist in the local repository.", sha)). + WithSuggestions( + "Check the SHA is correct: git log --oneline", + "Pass the full 40-character SHA to skip local resolution", + ). + WithExitCode(clierrors.ExitNotFound) + } filter := fmt.Sprintf("pipeline.git.revision == %q", sha) if branch != "" { From 6f14504ef4d167dcda844c70a5fd9cac97494758 Mon Sep 17 00:00:00 2001 From: schurchleycci Date: Fri, 26 Jun 2026 16:05:35 -0400 Subject: [PATCH 3/6] Address review feedback on run watch SHA handling - ExpandSHA now returns (string, error) with distinct sentinel errors (ErrSHANotHex, ErrSHARepoInaccessible, ErrSHANotFound) so callers can give accurate messages for each failure mode - Hex validation guards against branch names / tags resolving silently - watch.go emits a specific error per case instead of one catch-all - --project suggestion no longer mentions --branch when --sha is set - Acceptance tests updated to use full 40-char SHAs, restoring the original intent of testing the API-level paths - Unit tests added for all five ExpandSHA branches Co-Authored-By: Claude Sonnet 4.6 (1M context) --- acceptance/watch_test.go | 4 +- internal/cmd/run/watch.go | 22 +++++++-- internal/gitremote/detect.go | 32 ++++++++++--- internal/gitremote/detect_test.go | 78 +++++++++++++++++++++++++++++++ 4 files changed, 124 insertions(+), 12 deletions(-) diff --git a/acceptance/watch_test.go b/acceptance/watch_test.go index d3522b86a..54f7afccb 100644 --- a/acceptance/watch_test.go +++ b/acceptance/watch_test.go @@ -222,7 +222,7 @@ func TestRunWatch_SHA(t *testing.T) { result := binary.RunCLI(t, binary.RunOpts{ Binary: binaryPath, - Args: []string{"run", "watch", "--sha", "abc1234", + Args: []string{"run", "watch", "--sha", "abc1234def5678abcdef1234567890abcdef1234", "--project", watchSlug, "--branch", "main"}, Env: env.Environ(), WorkDir: t.TempDir(), @@ -245,7 +245,7 @@ func TestRunWatch_SHA_NotFound(t *testing.T) { result := binary.RunCLI(t, binary.RunOpts{ Binary: binaryPath, - Args: []string{"run", "watch", "--sha", "deadbeef", + Args: []string{"run", "watch", "--sha", "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef", "--project", watchSlug, "--branch", "main"}, Env: env.Environ(), WorkDir: t.TempDir(), diff --git a/internal/cmd/run/watch.go b/internal/cmd/run/watch.go index 12aeb02cc..2e060c5e9 100644 --- a/internal/cmd/run/watch.go +++ b/internal/cmd/run/watch.go @@ -152,7 +152,11 @@ func runWatch(ctx context.Context, client *apiclient.Client, args []string, proj if needsGit { info, err := gitremote.Detect() if err != nil { - return cmdutil.GitDetectErr(err, "Or specify --project and --branch explicitly") + suggestion := "Or specify --project and --branch explicitly" + if sha != "" { + suggestion = "Or specify --project explicitly" + } + return cmdutil.GitDetectErr(err, suggestion) } if projectSlug == "" { projectSlug = info.Slug @@ -234,9 +238,21 @@ func waitForRunBySHA(ctx context.Context, client *apiclient.Client, projectSlug, interval := 5 * time.Second printed := false - if expanded := gitremote.ExpandSHA(sha); expanded != sha { + expanded, expandErr := gitremote.ExpandSHA(sha) + switch { + case expandErr == nil: sha = expanded - } else if len(sha) < 40 { + case errors.Is(expandErr, gitremote.ErrSHANotHex): + return nil, clierrors.New("run.invalid_sha_format", "Invalid SHA format", + fmt.Sprintf("%q does not look like a commit SHA; expected hex characters only.", sha)). + WithSuggestions("Pass a hex commit SHA, e.g. from 'git log --oneline'"). + WithExitCode(clierrors.ExitBadArguments) + case errors.Is(expandErr, gitremote.ErrSHARepoInaccessible): + return nil, clierrors.New("run.sha_unresolvable", "Could not resolve short SHA", + fmt.Sprintf("Cannot expand %q: local git repository is not accessible.", sha)). + WithSuggestions("Pass the full 40-character SHA to skip local resolution"). + WithExitCode(clierrors.ExitBadArguments) + case errors.Is(expandErr, gitremote.ErrSHANotFound): return nil, clierrors.New("run.invalid_sha", "Commit not found", fmt.Sprintf("Commit %q does not exist in the local repository.", sha)). WithSuggestions( diff --git a/internal/gitremote/detect.go b/internal/gitremote/detect.go index 24486f8a7..82affd66c 100644 --- a/internal/gitremote/detect.go +++ b/internal/gitremote/detect.go @@ -63,6 +63,20 @@ var ( sshProtoRemote = regexp.MustCompile(`^ssh://git@([^/]+)/([^/]+)/(.+?)(?:\.git)?$`) // matches https://github.com/org/repo.git httpsRemote = regexp.MustCompile(`^https?://([^/]+)/([^/]+)/(.+?)(?:\.git)?$`) + + hexRE = regexp.MustCompile(`^[0-9a-fA-F]+$`) +) + +var ( + // ErrSHANotHex is returned by ExpandSHA when the input contains non-hex + // characters (e.g. a branch name passed by mistake). + ErrSHANotHex = errors.New("input is not a valid hex SHA") + // ErrSHARepoInaccessible is returned by ExpandSHA when the local git + // repository cannot be opened, so a short SHA cannot be expanded. + ErrSHARepoInaccessible = errors.New("local git repository is not accessible") + // ErrSHANotFound is returned by ExpandSHA when the short SHA does not + // resolve to any object in the local repository. + ErrSHANotFound = errors.New("SHA not found in local repository") ) // DetectNamespace returns the organization name (namespace) from the git remote. @@ -284,21 +298,25 @@ func gitCurrentBranch(repo *git.Repository) (string, error) { } // ExpandSHA attempts to resolve an abbreviated git SHA to its full 40-character -// form. Returns the input unchanged if the repo cannot be opened, the SHA isn't -// found, or the input is already 40 characters. -func ExpandSHA(sha string) string { +// form. It returns the (possibly expanded) SHA and nil on success, or the +// original input and one of ErrSHANotHex, ErrSHARepoInaccessible, or +// ErrSHANotFound on failure. +func ExpandSHA(sha string) (string, error) { + if !hexRE.MatchString(sha) { + return sha, ErrSHANotHex + } if len(sha) == 40 { - return sha + return sha, nil } repo, err := openRepo() if err != nil { - return sha + return sha, ErrSHARepoInaccessible } hash, err := repo.ResolveRevision(plumbing.Revision(sha)) if err != nil { - return sha + return sha, ErrSHANotFound } - return hash.String() + return hash.String(), nil } // gitDefaultBranch returns the short name of the remote default branch (e.g. diff --git a/internal/gitremote/detect_test.go b/internal/gitremote/detect_test.go index 30b33c7c0..598e0fe42 100644 --- a/internal/gitremote/detect_test.go +++ b/internal/gitremote/detect_test.go @@ -23,9 +23,11 @@ package gitremote import ( + "errors" "os" "path/filepath" "testing" + "time" "github.com/go-git/go-git/v6" "github.com/go-git/go-git/v6/config" @@ -175,6 +177,82 @@ func TestDetect_SurfacesMalformedInfoYml(t *testing.T) { assert.Check(t, err != nil, "expected Detect to surface a malformed info.yml rather than fall back") } +func TestExpandSHA(t *testing.T) { + origDir, err := os.Getwd() + assert.NilError(t, err) + + t.Run("already 40 hex chars returns input unchanged", func(t *testing.T) { + full := "1234567890abcdef1234567890abcdef12345678" + got, err := ExpandSHA(full) + assert.NilError(t, err) + assert.Check(t, cmp.Equal(got, full)) + }) + + t.Run("non-hex input returns ErrSHANotHex", func(t *testing.T) { + _, err := ExpandSHA("main") + assert.Check(t, errors.Is(err, ErrSHANotHex), "got: %v", err) + }) + + t.Run("repo inaccessible returns ErrSHARepoInaccessible", func(t *testing.T) { + dir := t.TempDir() + assert.NilError(t, os.Chdir(dir)) + t.Cleanup(func() { _ = os.Chdir(origDir) }) + + _, err := ExpandSHA("abc1234") + assert.Check(t, errors.Is(err, ErrSHARepoInaccessible), "got: %v", err) + }) + + t.Run("SHA not found in repo returns ErrSHANotFound", func(t *testing.T) { + dir := t.TempDir() + _, err := git.PlainInit(dir, false) + assert.NilError(t, err) + assert.NilError(t, os.Chdir(dir)) + t.Cleanup(func() { _ = os.Chdir(origDir) }) + + _, err = ExpandSHA("deadbeef") + assert.Check(t, errors.Is(err, ErrSHANotFound), "got: %v", err) + }) + + t.Run("short SHA expands to full 40-char hash", func(t *testing.T) { + dir := t.TempDir() + fullHash := initRepoWithCommit(t, dir) + assert.NilError(t, os.Chdir(dir)) + t.Cleanup(func() { _ = os.Chdir(origDir) }) + + short := fullHash[:7] + got, err := ExpandSHA(short) + assert.NilError(t, err) + assert.Check(t, cmp.Equal(got, fullHash)) + }) +} + +// initRepoWithCommit initialises a new git repository in dir, adds one commit, +// and returns the full 40-character SHA of that commit. +func initRepoWithCommit(t *testing.T, dir string) string { + t.Helper() + repo, err := git.PlainInit(dir, false) + assert.NilError(t, err) + + err = os.WriteFile(filepath.Join(dir, "README"), []byte("test"), 0o644) + assert.NilError(t, err) + + wt, err := repo.Worktree() + assert.NilError(t, err) + + _, err = wt.Add("README") + assert.NilError(t, err) + + hash, err := wt.Commit("initial commit", &git.CommitOptions{ + Author: &object.Signature{ + Name: "Test", + Email: "test@example.com", + When: time.Now(), + }, + }) + assert.NilError(t, err) + return hash.String() +} + // Sanity check that DetectFromRemote does not consult info.yml — used by // `project link` to avoid short-circuiting against an existing entry. func TestDetectFromRemote_IgnoresInfoYml(t *testing.T) { From 9b5a7c0f27fab1e6ee7932177dc4077cbd604028 Mon Sep 17 00:00:00 2001 From: schurchleycci Date: Tue, 28 Jul 2026 10:50:26 -0400 Subject: [PATCH 4/6] Address review feedback on short-SHA error messaging and test coverage - Clarify the ErrSHANotFound secondary suggestion to make the shallow-clone use case explicit rather than presenting it as general advice - Add acceptance test for short SHA passed outside a git repo, covering the ErrSHARepoInaccessible error message and exit code end-to-end Co-Authored-By: Claude Sonnet 4.6 --- acceptance/watch_test.go | 22 ++++++++++++++++++++++ internal/cmd/run/watch.go | 2 +- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/acceptance/watch_test.go b/acceptance/watch_test.go index 54f7afccb..a86db8e18 100644 --- a/acceptance/watch_test.go +++ b/acceptance/watch_test.go @@ -255,6 +255,28 @@ func TestRunWatch_SHA_NotFound(t *testing.T) { assert.Check(t, cmp.Contains(result.Stderr, "No run found"), "stderr: %s", result.Stderr) } +// --- --sha: short SHA outside a git repo → exit 2 (bad arguments) --- + +func TestRunWatch_SHA_ShortOutsideGitRepo(t *testing.T) { + fake := fakes.NewCircleCI(t) + addProjectBySlug(fake, watchSlug, watchProjectID) + + env := testenv.New(t) + env.Token = testToken + env.CircleCIURL = fake.URL() + + result := binary.RunCLI(t, binary.RunOpts{ + Binary: binaryPath, + Args: []string{"run", "watch", "--sha", "abc1234", + "--project", watchSlug, "--branch", "main"}, + Env: env.Environ(), + WorkDir: t.TempDir(), + }) + + assert.Equal(t, result.ExitCode, 2, "stderr: %s", result.Stderr) // ExitBadArguments + assert.Check(t, cmp.Contains(result.Stderr, "Could not resolve short SHA"), "stderr: %s", result.Stderr) +} + // --- --failfast: exit immediately when a job fails, without waiting for the rest of the run --- func TestRunWatch_FailFast(t *testing.T) { diff --git a/internal/cmd/run/watch.go b/internal/cmd/run/watch.go index 2e060c5e9..575a530f4 100644 --- a/internal/cmd/run/watch.go +++ b/internal/cmd/run/watch.go @@ -257,7 +257,7 @@ func waitForRunBySHA(ctx context.Context, client *apiclient.Client, projectSlug, fmt.Sprintf("Commit %q does not exist in the local repository.", sha)). WithSuggestions( "Check the SHA is correct: git log --oneline", - "Pass the full 40-character SHA to skip local resolution", + "If using a shallow clone, pass the full 40-character SHA obtained from the remote", ). WithExitCode(clierrors.ExitNotFound) } From 8512b7f0bb0572e29f4aad533bfa7d28299ac2b2 Mon Sep 17 00:00:00 2001 From: schurchleycci Date: Tue, 28 Jul 2026 11:43:00 -0400 Subject: [PATCH 5/6] Assert on rendered error message in short-SHA acceptance test CLIError.Format only writes Message to stderr; Title is exposed via --json output. The assertion looked for the title text, which can never appear on stderr, so the test failed on all three platforms. Co-Authored-By: Claude Opus 5 (1M context) --- acceptance/watch_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acceptance/watch_test.go b/acceptance/watch_test.go index a86db8e18..4a00616b2 100644 --- a/acceptance/watch_test.go +++ b/acceptance/watch_test.go @@ -274,7 +274,7 @@ func TestRunWatch_SHA_ShortOutsideGitRepo(t *testing.T) { }) assert.Equal(t, result.ExitCode, 2, "stderr: %s", result.Stderr) // ExitBadArguments - assert.Check(t, cmp.Contains(result.Stderr, "Could not resolve short SHA"), "stderr: %s", result.Stderr) + assert.Check(t, cmp.Contains(result.Stderr, "local git repository is not accessible"), "stderr: %s", result.Stderr) } // --- --failfast: exit immediately when a job fails, without waiting for the rest of the run --- From 4c0c7407beec294d10938c96beb602befbec5eac Mon Sep 17 00:00:00 2001 From: schurchleycci Date: Tue, 28 Jul 2026 13:58:50 -0400 Subject: [PATCH 6/6] Validate SHA format in run watch and make ExpandSHA testable without chdir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the hex check out of gitremote and into `run watch`. A non-hex --sha is a bad-argument error rather than a git failure, so ErrSHANotHex is gone and the check runs before the project lookup — a malformed --sha now costs no API call. Add ExpandSHAIn, which takes an explicit starting directory, so the tests point at temporary checkouts instead of mutating the process working directory with os.Chdir. ExpandSHA delegates to it via the working directory. Every subtest can now run in parallel. Close the repository handle in ExpandSHAIn. openRepo's contract requires it and both other callers already did; without it the tests leave handles open on temp repos they then delete. Co-Authored-By: Claude Opus 5 (1M context) --- acceptance/watch_test.go | 25 +++++++++++ internal/cmd/run/watch.go | 49 +++++++++++++++------ internal/cmd/run/watch_test.go | 73 +++++++++++++++++++++++++++++++ internal/gitremote/detect.go | 44 +++++++++++++------ internal/gitremote/detect_test.go | 63 ++++++++++++++++---------- 5 files changed, 206 insertions(+), 48 deletions(-) create mode 100644 internal/cmd/run/watch_test.go diff --git a/acceptance/watch_test.go b/acceptance/watch_test.go index 4a00616b2..64cd35946 100644 --- a/acceptance/watch_test.go +++ b/acceptance/watch_test.go @@ -277,6 +277,31 @@ func TestRunWatch_SHA_ShortOutsideGitRepo(t *testing.T) { assert.Check(t, cmp.Contains(result.Stderr, "local git repository is not accessible"), "stderr: %s", result.Stderr) } +// --- --sha: a revision that is not a hex SHA → exit 2 without any API call --- + +// A branch name is the likely mistake here, and it must not reach local +// expansion: go-git would resolve it to that branch's tip and watch the wrong +// commit. No project is registered on the fake, so reaching the API at all would +// surface as a different exit code. +func TestRunWatch_SHA_NotHex(t *testing.T) { + fake := fakes.NewCircleCI(t) + + env := testenv.New(t) + env.Token = testToken + env.CircleCIURL = fake.URL() + + result := binary.RunCLI(t, binary.RunOpts{ + Binary: binaryPath, + Args: []string{"run", "watch", "--sha", "main", + "--project", watchSlug, "--branch", "main"}, + Env: env.Environ(), + WorkDir: t.TempDir(), + }) + + assert.Equal(t, result.ExitCode, 2, "stderr: %s", result.Stderr) // ExitBadArguments + assert.Check(t, cmp.Contains(result.Stderr, "does not look like a commit SHA"), "stderr: %s", result.Stderr) +} + // --- --failfast: exit immediately when a job fails, without waiting for the rest of the run --- func TestRunWatch_FailFast(t *testing.T) { diff --git a/internal/cmd/run/watch.go b/internal/cmd/run/watch.go index 575a530f4..9e253f826 100644 --- a/internal/cmd/run/watch.go +++ b/internal/cmd/run/watch.go @@ -225,28 +225,41 @@ func runWatch(ctx context.Context, client *apiclient.Client, args []string, proj return watchUntilDone(ctx, client, r.ID, timeout, failFast) } +// isHexSHA reports whether s has the form every git object ID takes: a non-empty +// run of hex characters. This lives here rather than in gitremote because a +// non-hex --sha is a bad-argument error, not a git failure — and because +// gitremote's expansion would otherwise happily resolve branch names and tags, +// silently watching the wrong commit. +func isHexSHA(s string) bool { + if s == "" { + return false + } + for _, r := range s { + switch { + case r >= '0' && r <= '9', r >= 'a' && r <= 'f', r >= 'A' && r <= 'F': + default: + return false + } + } + return true +} + // waitForRunBySHA searches for a run matching the given commit SHA via V3 search, // polling every 5 seconds for up to shaWaitDuration() if not immediately found. func waitForRunBySHA(ctx context.Context, client *apiclient.Client, projectSlug, branch, sha string) (*apiclient.RunV3, error) { - proj, err := client.GetProjectBySlug(ctx, projectSlug) - if err != nil { - return nil, apiErr(err, projectSlug) + // The SHA is resolved before the project lookup so a --sha that cannot work + // costs no API call. + if !isHexSHA(sha) { + return nil, clierrors.New("run.invalid_sha_format", "Invalid SHA format", + fmt.Sprintf("%q does not look like a commit SHA; expected hex characters only.", sha)). + WithSuggestions("Pass a hex commit SHA, e.g. from 'git log --oneline'"). + WithExitCode(clierrors.ExitBadArguments) } - waitDur := shaWaitDuration() - deadline := time.Now().Add(waitDur) - interval := 5 * time.Second - printed := false - expanded, expandErr := gitremote.ExpandSHA(sha) switch { case expandErr == nil: sha = expanded - case errors.Is(expandErr, gitremote.ErrSHANotHex): - return nil, clierrors.New("run.invalid_sha_format", "Invalid SHA format", - fmt.Sprintf("%q does not look like a commit SHA; expected hex characters only.", sha)). - WithSuggestions("Pass a hex commit SHA, e.g. from 'git log --oneline'"). - WithExitCode(clierrors.ExitBadArguments) case errors.Is(expandErr, gitremote.ErrSHARepoInaccessible): return nil, clierrors.New("run.sha_unresolvable", "Could not resolve short SHA", fmt.Sprintf("Cannot expand %q: local git repository is not accessible.", sha)). @@ -262,6 +275,16 @@ func waitForRunBySHA(ctx context.Context, client *apiclient.Client, projectSlug, WithExitCode(clierrors.ExitNotFound) } + proj, err := client.GetProjectBySlug(ctx, projectSlug) + if err != nil { + return nil, apiErr(err, projectSlug) + } + + waitDur := shaWaitDuration() + deadline := time.Now().Add(waitDur) + interval := 5 * time.Second + printed := false + filter := fmt.Sprintf("pipeline.git.revision == %q", sha) if branch != "" { filter += fmt.Sprintf(" and pipeline.git.branch == %q", branch) diff --git a/internal/cmd/run/watch_test.go b/internal/cmd/run/watch_test.go new file mode 100644 index 000000000..4cffce234 --- /dev/null +++ b/internal/cmd/run/watch_test.go @@ -0,0 +1,73 @@ +// Copyright (c) 2026 Circle Internet Services, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. +// +// SPDX-License-Identifier: MIT + +package run + +import ( + "strings" + "testing" + + "gotest.tools/v3/assert" + "gotest.tools/v3/assert/cmp" +) + +func TestIsHexSHA(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in string + want bool + }{ + {"full lowercase SHA", "1234567890abcdef1234567890abcdef12345678", true}, + {"abbreviated SHA", "abc1234", true}, + {"uppercase hex", "ABC1234", true}, + {"mixed case hex", "AbC1234", true}, + {"single character", "a", true}, + // Rejected so gitremote never sees a revision expression it would + // happily resolve to the wrong commit. + {"branch name", "main", false}, + {"HEAD", "HEAD", false}, + {"HEAD with offset", "HEAD~3", false}, + {"tag-like name", "v1.2.3", false}, + {"non-hex letter past f", "abcg123", false}, + {"leading whitespace", " abc1234", false}, + {"empty string", "", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Check(t, cmp.Equal(isHexSHA(tt.in), tt.want), "input: %q", tt.in) + }) + } +} + +// A full SHA must survive validation unchanged in either case, since the +// scripted path passes whatever git printed. +func TestIsHexSHA_FullSHACaseInsensitive(t *testing.T) { + t.Parallel() + + full := "1234567890abcdef1234567890abcdef12345678" + assert.Check(t, isHexSHA(full)) + assert.Check(t, isHexSHA(strings.ToUpper(full))) +} diff --git a/internal/gitremote/detect.go b/internal/gitremote/detect.go index 82affd66c..374f550a5 100644 --- a/internal/gitremote/detect.go +++ b/internal/gitremote/detect.go @@ -63,14 +63,9 @@ var ( sshProtoRemote = regexp.MustCompile(`^ssh://git@([^/]+)/([^/]+)/(.+?)(?:\.git)?$`) // matches https://github.com/org/repo.git httpsRemote = regexp.MustCompile(`^https?://([^/]+)/([^/]+)/(.+?)(?:\.git)?$`) - - hexRE = regexp.MustCompile(`^[0-9a-fA-F]+$`) ) var ( - // ErrSHANotHex is returned by ExpandSHA when the input contains non-hex - // characters (e.g. a branch name passed by mistake). - ErrSHANotHex = errors.New("input is not a valid hex SHA") // ErrSHARepoInaccessible is returned by ExpandSHA when the local git // repository cannot be opened, so a short SHA cannot be expanded. ErrSHARepoInaccessible = errors.New("local git repository is not accessible") @@ -270,7 +265,14 @@ func openRepo() (*git.Repository, error) { if err != nil { return nil, err } - return git.PlainOpenWithOptions(cwd, &git.PlainOpenOptions{DetectDotGit: true}) + return openRepoAt(cwd) +} + +// openRepoAt is openRepo for an explicit starting directory, letting tests point +// at a temporary checkout instead of mutating the process working directory. The +// same worktree resolution and handle-closing notes on openRepo apply. +func openRepoAt(dir string) (*git.Repository, error) { + return git.PlainOpenWithOptions(dir, &git.PlainOpenOptions{DetectDotGit: true}) } // gitOriginURL returns the first configured URL for the "origin" remote, @@ -297,21 +299,37 @@ func gitCurrentBranch(repo *git.Repository) (string, error) { return head.Name().Short(), nil } -// ExpandSHA attempts to resolve an abbreviated git SHA to its full 40-character -// form. It returns the (possibly expanded) SHA and nil on success, or the -// original input and one of ErrSHANotHex, ErrSHARepoInaccessible, or -// ErrSHANotFound on failure. +// ExpandSHA resolves an abbreviated git SHA against the repository containing +// the current working directory. See ExpandSHAIn for the contract. func ExpandSHA(sha string) (string, error) { - if !hexRE.MatchString(sha) { - return sha, ErrSHANotHex + cwd, err := os.Getwd() + if err != nil { + return sha, ErrSHARepoInaccessible } + return ExpandSHAIn(cwd, sha) +} + +// ExpandSHAIn attempts to resolve an abbreviated git SHA to its full +// 40-character form using the repository containing dir. It returns the +// (possibly expanded) SHA and nil on success, or the original input and either +// ErrSHARepoInaccessible or ErrSHANotFound on failure. A SHA that is already 40 +// characters is returned as-is without opening a repository, so callers holding +// a full SHA never depend on local git state. +// +// sha must already be known to be hex; callers validate that themselves, since a +// non-SHA argument is a bad-argument error rather than a git failure. +// ResolveRevision accepts any revision expression — branch names, tags, HEAD~3 — +// so passing unvalidated input here would silently resolve those instead. +func ExpandSHAIn(dir, sha string) (string, error) { if len(sha) == 40 { return sha, nil } - repo, err := openRepo() + repo, err := openRepoAt(dir) if err != nil { return sha, ErrSHARepoInaccessible } + defer func() { _ = repo.Close() }() + hash, err := repo.ResolveRevision(plumbing.Revision(sha)) if err != nil { return sha, ErrSHANotFound diff --git a/internal/gitremote/detect_test.go b/internal/gitremote/detect_test.go index 598e0fe42..56a262fd1 100644 --- a/internal/gitremote/detect_test.go +++ b/internal/gitremote/detect_test.go @@ -177,50 +177,68 @@ func TestDetect_SurfacesMalformedInfoYml(t *testing.T) { assert.Check(t, err != nil, "expected Detect to surface a malformed info.yml rather than fall back") } -func TestExpandSHA(t *testing.T) { - origDir, err := os.Getwd() - assert.NilError(t, err) +func TestExpandSHAIn(t *testing.T) { + t.Parallel() t.Run("already 40 hex chars returns input unchanged", func(t *testing.T) { + t.Parallel() + // Passed a directory that is not a repository to pin down that a full SHA + // never touches local git state — the property the scripted + // ($CIRCLE_SHA1) path relies on. full := "1234567890abcdef1234567890abcdef12345678" - got, err := ExpandSHA(full) + got, err := ExpandSHAIn(t.TempDir(), full) assert.NilError(t, err) assert.Check(t, cmp.Equal(got, full)) }) - t.Run("non-hex input returns ErrSHANotHex", func(t *testing.T) { - _, err := ExpandSHA("main") - assert.Check(t, errors.Is(err, ErrSHANotHex), "got: %v", err) - }) - t.Run("repo inaccessible returns ErrSHARepoInaccessible", func(t *testing.T) { - dir := t.TempDir() - assert.NilError(t, os.Chdir(dir)) - t.Cleanup(func() { _ = os.Chdir(origDir) }) - - _, err := ExpandSHA("abc1234") + t.Parallel() + _, err := ExpandSHAIn(t.TempDir(), "abc1234") assert.Check(t, errors.Is(err, ErrSHARepoInaccessible), "got: %v", err) }) t.Run("SHA not found in repo returns ErrSHANotFound", func(t *testing.T) { + t.Parallel() dir := t.TempDir() - _, err := git.PlainInit(dir, false) + repo, err := git.PlainInit(dir, false) assert.NilError(t, err) - assert.NilError(t, os.Chdir(dir)) - t.Cleanup(func() { _ = os.Chdir(origDir) }) + t.Cleanup(func() { _ = repo.Close() }) - _, err = ExpandSHA("deadbeef") + _, err = ExpandSHAIn(dir, "deadbeef") assert.Check(t, errors.Is(err, ErrSHANotFound), "got: %v", err) }) t.Run("short SHA expands to full 40-char hash", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + fullHash := initRepoWithCommit(t, dir) + + got, err := ExpandSHAIn(dir, fullHash[:7]) + assert.NilError(t, err) + assert.Check(t, cmp.Equal(got, fullHash)) + }) + + t.Run("resolves a subdirectory to its containing repo", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + fullHash := initRepoWithCommit(t, dir) + sub := filepath.Join(dir, "nested", "deeper") + assert.NilError(t, os.MkdirAll(sub, 0o755)) + + got, err := ExpandSHAIn(sub, fullHash[:7]) + assert.NilError(t, err) + assert.Check(t, cmp.Equal(got, fullHash)) + }) + + // Documents why callers must reject non-hex input before calling: go-git + // resolves any revision expression, so a branch name would expand to that + // branch's tip and silently watch the wrong commit. + t.Run("resolves branch names, which is why callers validate hex first", func(t *testing.T) { + t.Parallel() dir := t.TempDir() fullHash := initRepoWithCommit(t, dir) - assert.NilError(t, os.Chdir(dir)) - t.Cleanup(func() { _ = os.Chdir(origDir) }) - short := fullHash[:7] - got, err := ExpandSHA(short) + got, err := ExpandSHAIn(dir, "HEAD") assert.NilError(t, err) assert.Check(t, cmp.Equal(got, fullHash)) }) @@ -232,6 +250,7 @@ func initRepoWithCommit(t *testing.T, dir string) string { t.Helper() repo, err := git.PlainInit(dir, false) assert.NilError(t, err) + t.Cleanup(func() { _ = repo.Close() }) err = os.WriteFile(filepath.Join(dir, "README"), []byte("test"), 0o644) assert.NilError(t, err)