ronantakizawa/codereview-bench huggingface dataset:
Repository: cli/cli
File: pkg/cmd/pr/edit/edit.go (Go)

Code before:
			if err != nil {
				return nil, cobra.ShellCompDirectiveError
			}
			results, err := shared.RequestableReviewersForCompletion(httpClient, baseRepo)
			if err != nil {
				return nil, cobra.ShellCompDirectiveError
			}
			return results, cobra.ShellCompDirectiveNoFileComp
		})
	}

	return cmd
}

func editRun(opts *EditOptions) error {
	findOptions := shared.FindOptions{
		Selector: opts.SelectorArg,
		Fields:   []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone"},
		Detector: opts.Detector,
	}

	if opts.Detector == nil {
		httpClient, err := opts.HttpClient()
		if err != nil {
			return err
		}

		baseRepo, err := opts.BaseRepo()
		if err != nil {
			return err
		}

		cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
		opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost())
	}

	issueFeatures, err := opts.Detector.IssueFeatures()
	if err != nil {
		return err
	}

	if issueFeatures.ActorIsAssignable {
		findOptions.Fields = append(findOptions.Fields, "assignedActors")
	} else {
		findOptions.Fields = append(findOptions.Fields, "assignees")
	}

	pr, repo, err := opts.Finder.Find(findOptions)
	if err != nil {
		return err
	}

Review comment (suggestion):
**suggest:** since this is needed later down, perhaps we pull this out above the detector and remove the redundant section below

https://github.com/cli/cli/blob/54f48cfe46e231a7f1d45bdab0706ed999690f60/pkg/cmd/pr/edit/edit.go#L265-L268

Code after:
			if err != nil {
				return nil, cobra.ShellCompDirectiveError
			}
			results, err := shared.RequestableReviewersForCompletion(httpClient, baseRepo)
			if err != nil {
				return nil, cobra.ShellCompDirectiveError
			}
			return results, cobra.ShellCompDirectiveNoFileComp
		})
	}

	return cmd
}

func editRun(opts *EditOptions) error {
	findOptions := shared.FindOptions{
		Selector: opts.SelectorArg,
		Fields:   []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone"},
		Detector: opts.Detector,
	}

	httpClient, err := opts.HttpClient()
	if err != nil {
		return err
	}

	if opts.Detector == nil {
		baseRepo, err := opts.BaseRepo()
		if err != nil {
			return err
		}

		cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
		opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost())
	}

	issueFeatures, err := opts.Detector.IssueFeatures()
	if err != nil {
		return err
	}

	if issueFeatures.ActorIsAssignable {
		findOptions.Fields = append(findOptions.Fields, "assignedActors")
	} else {
		findOptions.Fields = append(findOptions.Fields, "assignees")
	}

	pr, repo, err := opts.Finder.Find(findOptions)
	if err != nil {
		return err
	}
Repository: cli/cli
File: pkg/cmd/pr/shared/params.go (Go)

Code before:
	if handle == "@me" {
		return r.currentLogin()
	}
	return handle, nil
}

func (r *MeReplacer) ReplaceSlice(handles []string) ([]string, error) {
	res := make([]string, len(handles))
	for i, h := range handles {
		var err error
		res[i], err = r.Replace(h)
		if err != nil {
			return nil, err
		}
	}
	return res, nil
}

// CopilotReplacer resolves usages of `@copilot` to Copilot's login.
type CopilotReplacer struct{}

func NewCopilotReplacer() *CopilotReplacer {
	return &CopilotReplacer{}
}

func (r *CopilotReplacer) replace(handle string) (string, error) {
	if strings.EqualFold(handle, "@copilot") {
		return "copilot-swe-agent", nil
	}
	return handle, nil
}

// Replace replaces usages of `@copilot` in a slice with Copilot's login.
func (r *CopilotReplacer) ReplaceSlice(handles []string) ([]string, error) {
	res := make([]string, len(handles))
	for i, h := range handles {
		var err error
		res[i], err = r.replace(h)
		if err != nil {
			return nil, err
		}
	}
	return res, nil
}


Review comment (bug):
I know this is for consistency with `MeReplacer`, but why we need to return an `error`? It's just a lookup and there's no failure if it's not a match.

Feel free to ignore this if you think it's out of scope.

Code after:
	if handle == "@me" {
		return r.currentLogin()
	}
	return handle, nil
}

func (r *MeReplacer) ReplaceSlice(handles []string) ([]string, error) {
	res := make([]string, len(handles))
	for i, h := range handles {
		var err error
		res[i], err = r.Replace(h)
		if err != nil {
			return nil, err
		}
	}
	return res, nil
}

// CopilotReplacer resolves usages of `@copilot` to Copilot's login.
type CopilotReplacer struct{}

func NewCopilotReplacer() *CopilotReplacer {
	return &CopilotReplacer{}
}

func (r *CopilotReplacer) replace(handle string) string {
	if strings.EqualFold(handle, "@copilot") {
		return "copilot-swe-agent"
	}
	return handle
}

// Replace replaces usages of `@copilot` in a slice with Copilot's login.
func (r *CopilotReplacer) ReplaceSlice(handles []string) []string {
	res := make([]string, len(handles))
	for i, h := range handles {
		res[i] = r.replace(h)
	}
	return res
}
Repository: cli/cli
File: script/gen-winres.ps1 (PowerShell)

Code before:
    Write-Host "error: path to winres.json is missing"
    exit 1
}

if (-not (Test-Path $_winresJson)) {
    Write-Host "error: winres.json file not found at '$_winresJson'"
    exit 1
}

$_output = $args[3]
if ([string]::IsNullOrEmpty($_output)) {
    Write-Host "error: output path is missing"
    exit 1
}

if (-not (Test-Path $_output -PathType Container)) {
    Write-Host "error: output path '$_output' is not a directory"
    exit 1
}

# Note that we intentionally leave the `--file-version` option in the command
# below, because it's meant to be a 4-component version, while ours is a semver
# (3-component). If we populate the `--file-version` with our semver value, then
# a zero component will be added to the end, which is not what we want.

go run github.com/tc-hib/go-winres@v0.3.3 make `
    --arch "$_arch" `
    --product-version "$_version" `
    --in "$_winresJson" `
    --out rsrc

Move-Item -Path ".\rsrc_*.syso" -Destination "$_output" -Force


Review comment (security):
**issue:** this project does not appear to be actively maintained if even `go.mod` hasn't been changed in a year.

I know this was the tool that `@iamazeem` used in experimenting on this issue.  The only other suggestion I could offer is https://github.com/josephspurrier/goversioninfo, which has more recent releases and appears to keep up with Dependabot security updates.

Code after:
}

$_versioninfo_path = $args[2]
if ([string]::IsNullOrEmpty($_versioninfo_path)) {
    Write-Host "error: path to versioninfo.json is missing"
    Write-Host $_usage
    exit 1
}

if (-not (Test-Path $_versioninfo_path)) {
    Write-Host "error: path to versioninfo.json '$_versioninfo_path' is not a file"
    Write-Host $_usage
    exit 1
}

$_output = $args[3]
if ([string]::IsNullOrEmpty($_output)) {
    Write-Host "error: output path is missing"
    Write-Host $_usage
    exit 1
}

if (-not (Test-Path $_output -PathType Container)) {
    Write-Host "error: output path '$_output' is not a directory"
    Write-Host $_usage
    exit 1
}

go install github.com/josephspurrier/goversioninfo/cmd/goversioninfo@v1.5.0

goversioninfo `
    -64 -arm -platform-specific `
    -file-version "$_file_version" `
    -product-version "$_product_version" `
    "$_versioninfo_path"

Move-Item -Path "resource_windows_*.syso" -Destination "$_output" -Force
Repository: cli/cli
File: script/gen-winres.ps1 (PowerShell)

Code before:

$ErrorActionPreference = "Stop"

$_file_version = $args[0]
if ([string]::IsNullOrEmpty($_file_version)) {
    Write-Host "error: file-version argument is missing"
    Write-Host $_usage
    exit 1
}

$_product_version = $args[1]
if ([string]::IsNullOrEmpty($_product_version)) {
    Write-Host "error: product-version argument is missing"
    Write-Host $_usage
    exit 1
}

$_versioninfo_path = $args[2]
if ([string]::IsNullOrEmpty($_versioninfo_path)) {
    Write-Host "error: path to versioninfo.json is missing"
    Write-Host $_usage
    exit 1
}

if (-not (Test-Path $_versioninfo_path)) {
    Write-Host "error: file not found at '$_versioninfo_path'"
    Write-Host $_usage
    exit 1
}

$_output = $args[3]
if ([string]::IsNullOrEmpty($_output)) {
    Write-Host "error: output path is missing"
    Write-Host $_usage
    exit 1
}

if (-not (Test-Path $_output -PathType Container)) {
    Write-Host "error: output path '$_output' is not a directory"
    Write-Host $_usage
    exit 1
}

go install github.com/josephspurrier/goversioninfo/cmd/goversioninfo@v1.5.0

goversioninfo `
    -64 -arm -platform-specific `
    -file-version "$_file_version" `
    -product-version "$_product_version" `
    "$_versioninfo_path"


Review comment (suggestion):
**nit:** providing a consistent error message with other tests

```suggestion
    Write-Host "error: path to versioninfo.json '$_versioninfo_path' is not a file"
```

Code after:

$ErrorActionPreference = "Stop"

$_file_version = $args[0]
if ([string]::IsNullOrEmpty($_file_version)) {
    Write-Host "error: file-version argument is missing"
    Write-Host $_usage
    exit 1
}

$_product_version = $args[1]
if ([string]::IsNullOrEmpty($_product_version)) {
    Write-Host "error: product-version argument is missing"
    Write-Host $_usage
    exit 1
}

$_versioninfo_path = $args[2]
if ([string]::IsNullOrEmpty($_versioninfo_path)) {
    Write-Host "error: path to versioninfo.json is missing"
    Write-Host $_usage
    exit 1
}

if (-not (Test-Path $_versioninfo_path)) {
    Write-Host "error: path to versioninfo.json '$_versioninfo_path' is not a file"
    Write-Host $_usage
    exit 1
}

$_output = $args[3]
if ([string]::IsNullOrEmpty($_output)) {
    Write-Host "error: output path is missing"
    Write-Host $_usage
    exit 1
}

if (-not (Test-Path $_output -PathType Container)) {
    Write-Host "error: output path '$_output' is not a directory"
    Write-Host $_usage
    exit 1
}

go install github.com/josephspurrier/goversioninfo/cmd/goversioninfo@v1.5.0

goversioninfo `
    -64 -arm -platform-specific `
    -file-version "$_file_version" `
    -product-version "$_product_version" `
    "$_versioninfo_path"
Repository: cli/cli
File: .github/workflows/scripts/check-help-wanted.sh (Shell)

Code before:
#!/bin/bash

set -e

PR_URL="$1"

if [ -z "$PR_URL" ]; then
    echo "Usage: $0 <pr_url>"
    exit 1
fi

# Extract PR number from URL for logging
PRNUM=$(basename "$PR_URL")

# Get PR details including closing issues references
PR_DATA=$(gh pr view "$PR_URL" --json closingIssuesReferences)

# Extract closing issues references
CLOSING_ISSUES=$(echo "$PR_DATA" | jq -r '.closingIssuesReferences[]?.number // empty')

if [ -z "$CLOSING_ISSUES" ]; then
    echo "No closing issues found for PR #$PRNUM"
    exit 0
fi

# Check each closing issue for help-wanted label
ISSUES_WITHOUT_HELP_WANTED=()


Review comment (bug):
I'd rather go with:

```sh
set -euxo pipefail
```

We can remove `x` when we're confident the script is doing okay.

Code after:
#!/bin/bash

set -e

PR_URL="$1"

if [ -z "$PR_URL" ]; then
    echo "Usage: $0 <PR_URL>"
    echo ""
    echo "Check if the PR references any non-help-wanted issues and, if so, comment"
    echo "on it explaining why the team might close/dismiss it."
    exit 1
fi

# Extract PR number from URL for logging
PR_NUM="$(basename "$PR_URL")"

# Extract cli/cli closing issues references from PR
CLOSING_ISSUES="$(gh pr view "$PR_URL" --json closingIssuesReferences --jq '.closingIssuesReferences[] | select(.repository.name == "cli" and .repository.owner.login == "cli") | .number')"

if [ -z "$CLOSING_ISSUES" ]; then
    echo "No closing issues found for PR #$PR_NUM"
    exit 0
fi

# Check each closing issue for 'help-wanted' label
ISSUES_WITHOUT_HELP_WANTED=()
Repository: cli/cli
File: internal/codespaces/connection/tunnels_api_server_mock.go (Go)

Code before:
			Proxy: http.ProxyURL(url),
		},
	}, nil
}

type relayServer struct {
	httpServer  *httptest.Server
	errc        chan error
	sshConfig   *ssh.ServerConfig
	channels    map[string]channelHandler
	accessToken string

	serverConn *ssh.ServerConn
}

type relayServerOption func(*relayServer)
type channelHandler func(context.Context, ssh.NewChannel) error

func newMockrelayServer(opts ...relayServerOption) (*relayServer, error) {
	server := &relayServer{
		errc: make(chan error),
		sshConfig: &ssh.ServerConfig{
			NoClientAuth: true,
		},
	}

	// Create a private key with the crypto package
	key, err := rsa.GenerateKey(rand.Reader, 2048)
	if err != nil {
		return nil, fmt.Errorf("failed to generate key: %w", err)
	}

	privateKeyPEM := pem.EncodeToMemory(
		&pem.Block{
			Type:  "RSA PRIVATE KEY",
			Bytes: x509.MarshalPKCS1PrivateKey(key),
		},
	)

	// Parse the private key
	sshPrivateKey, err := ssh.ParsePrivateKey(privateKeyPEM)
	if err != nil {
		return nil, fmt.Errorf("failed to parse private key: %w", err)
	}

	server.sshConfig.AddHostKey(ssh.Signer(sshPrivateKey))

	server.httpServer = httptest.NewServer(http.HandlerFunc(makeConnection(server)))

	for _, opt := range opts {
		opt(server)

Review comment (suggestion):
Now I **did** change this to a PUT.

This is now a PUT in https://github.com/microsoft/dev-tunnels/blob/06ba8571935cd11a5384cb7d597fa1b796c59282/go/tunnels/manager.go#L465

But previously was a POST: https://github.com/microsoft/dev-tunnels/blob/97233d20448e1c3cb0e0fd9114acf68c7e5c0249/go/tunnels/manager.go#L370

Code after:
							log.Fatalf("strconv.Atoi returned an error: %v", err)
						}

						tunnelPort, ok := specifiedPorts[port]
						if !ok {
							w.WriteHeader(http.StatusNotFound)
							return
						}

						response, err := json.Marshal(tunnelPort)

						if err != nil {
							log.Fatalf("json.Marshal returned an error: %v", err)
						}

						_, _ = w.Write(response)
						return
					}
				}

				// Else this is an unexpected request, fall through to 404 at the bottom
			}

			// If it's a PUT request, we assume it's for creating a new port so we'll do some validation
			// and then return a stub.
			if r.Method == http.MethodPut {
				// If a port was already configured with this number, and the protocol has changed, return a 400 Bad Request.
				if specifiedPorts != nil {
					port, err := strconv.Atoi(matches[1])
					if err != nil {
						log.Fatalf("strconv.Atoi returned an error: %v", err)
					}

					var portRequest tunnels.TunnelPort
					if err := json.NewDecoder(r.Body).Decode(&portRequest); err != nil {
						log.Fatalf("json.NewDecoder returned an error: %v", err)
					}

					tunnelPort, ok := specifiedPorts[port]
					if ok {
						if tunnelPort.Protocol != portRequest.Protocol {
							w.WriteHeader(http.StatusBadRequest)
							return
						}
					}

					// Create or update the new port entry.
					specifiedPorts[port] = portRequest
				}

				response, err := json.Marshal(tunnels.TunnelPort{
Repository: cli/cli
File: pkg/search/query.go (Go)

Code before:
	}
	return s
}

func formatQualifiers(qs Qualifiers) []string {
	var all []string
	for k, vs := range qs.Map() {
		for _, v := range vs {
			all = append(all, fmt.Sprintf("%s:%s", k, quote(v)))
		}
	}
	sort.Strings(all)
	return all
}

func formatKeywords(ks []string) []string {
	for i, k := range ks {
		before, after, found := strings.Cut(k, ":")
		if !found {
			ks[i] = quote(k)
		} else {
			ks[i] = fmt.Sprintf("%s:%s", before, quote(after))
		}
	}
	return ks
}

// CamelToKebab returns a copy of the string s that is converted from camel case form to '-' separated form.
func camelToKebab(s string) string {
	var output []rune
	var segment []rune
	for _, r := range s {
		if !unicode.IsLower(r) && string(r) != "-" && !unicode.IsNumber(r) {
			output = addSegment(output, segment)
			segment = nil
		}
		segment = append(segment, unicode.ToLower(r))
	}
	output = addSegment(output, segment)
	return string(output)
}

func addSegment(inrune, segment []rune) []rune {
	if len(segment) == 0 {
		return inrune
	}
	if len(inrune) != 0 {
		inrune = append(inrune, '-')
	}
	inrune = append(inrune, segment...)
	return inrune

Review comment (refactor):
**note:** just to clarify, this function had a side effect of modifying `Query.Keywords` field which is used else where.  Refactoring this logic to not modify the input avoids the redundant formatting issue.

Code after:
	}
	return s
}

func formatQualifiers(qs Qualifiers) []string {
	var all []string
	for k, vs := range qs.Map() {
		for _, v := range vs {
			all = append(all, fmt.Sprintf("%s:%s", k, quote(v)))
		}
	}
	sort.Strings(all)
	return all
}

func formatKeywords(ks []string) []string {
	result := make([]string, len(ks))
	for i, k := range ks {
		before, after, found := strings.Cut(k, ":")
		if !found {
			result[i] = quote(k)
		} else {
			result[i] = fmt.Sprintf("%s:%s", before, quote(after))
		}
	}
	return result
}

// CamelToKebab returns a copy of the string s that is converted from camel case form to '-' separated form.
func camelToKebab(s string) string {
	var output []rune
	var segment []rune
	for _, r := range s {
		if !unicode.IsLower(r) && string(r) != "-" && !unicode.IsNumber(r) {
			output = addSegment(output, segment)
			segment = nil
		}
		segment = append(segment, unicode.ToLower(r))
	}
	output = addSegment(output, segment)
	return string(output)
}

func addSegment(inrune, segment []rune) []rune {
	if len(segment) == 0 {
		return inrune
	}
	if len(inrune) != 0 {
		inrune = append(inrune, '-')
	}
	inrune = append(inrune, segment...)
Repository: cli/cli
File: .github/workflows/scripts/spam-detection/process-issue.sh (Shell)

Code before:
if [[ -z "$_issue_url" ]]; then
    echo "error: issue URL is empty" >&2
    exit 1
fi

_result="$("$SPAM_DIR/check-issue.sh" "$_issue_url")"

if [[ "$_result" == "PASS" ]]; then
    echo "detected as not-spam: $_issue_url"
    exit 0
fi

echo "detected as spam: $_issue_url"

cat << EOF | gh issue comment "$_issue_url" --body-file -
Thank you for taking the time to create this issue.

We've automatically reviewed this issue and suspect it as potentially inauthentic or spam-like content. As a result, we're closing this issue.

**If this was closed by mistake**, please don't hesitate to reach out to us by commenting on this issue with additional context.

We appreciate your understanding and apologize if this action was taken in error. Our automated systems help us manage the large volume of issues we receive, but we know they're not perfect.
EOF

gh issue edit --add-label "suspected-spam" --add-label "invalid" "$_issue_url"
gh issue close "$_issue_url"

echo "issue labelled as suspected spam"


Review comment (suggestion):
We should close the issue as "not planned", like how we're doing it:


```suggestion
gh issue close --reason 'not planned' "$_issue_url"
```

Code after:
if [[ -z "$_issue_url" ]]; then
    echo "error: issue URL is empty" >&2
    exit 1
fi

_result="$("$SPAM_DIR/check-issue.sh" "$_issue_url")"

if [[ "$_result" == "PASS" ]]; then
    echo "detected as not-spam: $_issue_url"
    exit 0
fi

echo "detected as spam: $_issue_url"

cat << EOF | gh issue comment "$_issue_url" --body-file -
Thank you for taking the time to create this issue.

We've automatically reviewed this issue and suspect it as potentially inauthentic or spam-like content. As a result, we're closing this issue.

**If this was closed by mistake**, please don't hesitate to reach out to us by commenting on this issue with additional context.

We appreciate your understanding and apologize if this action was taken in error. Our automated systems help us manage the large volume of issues we receive, but we know they're not perfect.
EOF

gh issue edit --add-label "suspected-spam" --add-label "invalid" "$_issue_url"
gh issue close --reason 'not planned' "$_issue_url"

echo "issue processed as suspected spam: commented, closed, and labeled"
Repository: cli/cli
File: pkg/cmd/release/shared/fetch_test.go (Go)

Code before:
package shared

import (
	"context"
	"net/http"
	"testing"

	"github.com/cli/cli/v2/internal/ghrepo"
	"github.com/cli/cli/v2/pkg/httpmock"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
)

func TestFetchRefSHA(t *testing.T) {
	tests := []struct {
		name           string
		tagName        string
		responseStatus int
		responseBody   string
		expectedSHA    string
		errorMessage   string
	}{
		{
			name:           "full semver tag",
			tagName:        "v1.2.3",
			responseStatus: 200,
			responseBody:   `{"object": {"sha": "1234567890abcdef1234567890abcdef12345678"}}`,
			expectedSHA:    "1234567890abcdef1234567890abcdef12345678",
		},
		{
			name:           "partial semver - major only",
			tagName:        "v1",
			responseStatus: 200,
			responseBody:   `{"object": {"sha": "abcdef1234567890abcdef1234567890abcdef12"}}`,
			expectedSHA:    "abcdef1234567890abcdef1234567890abcdef12",
		},
		{
			name:           "partial semver - major.minor",
			tagName:        "v1.2",
			responseStatus: 200,
			responseBody:   `{"object": {"sha": "fedcba0987654321fedcba0987654321fedcba09"}}`,
			expectedSHA:    "fedcba0987654321fedcba0987654321fedcba09",
		},
		{
			name:           "prerelease tag",
			tagName:        "v1.2.3-alpha.1",
			responseStatus: 200,

Review comment (suggestion):
I like the unit test here! 🙏 

However, I think the current test cases are no longer needed/relevant. The endpoint we're now calling (i.e. `repos/onwer/repo/git/ref/tags/[tag]`) does not perform any partial matching. The tag is also not required to be in semver format. 

So, I can imagine we only need these test cases:
- a match (`200`)
- a non-match (`404`)
- a `500`
- a malformed JSON with `200`
  - This never happens, but it should be there for covering all code paths.


Code after:
package shared

import (
	"context"
	"net/http"
	"testing"

	"github.com/cli/go-gh/v2/pkg/api"

	"github.com/cli/cli/v2/internal/ghrepo"
	"github.com/cli/cli/v2/pkg/httpmock"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
)

func TestFetchRefSHA(t *testing.T) {
	tests := []struct {
		name            string
		tagName         string
		responseStatus  int
		responseBody    string
		responseMessage string
		expectedSHA     string
		errorMessage    string
	}{
		{
			name:           "match (200)",
			tagName:        "v1.2.3",
			responseStatus: 200,
			responseBody:   `{"object": {"sha": "1234567890abcdef1234567890abcdef12345678"}}`,
			expectedSHA:    "1234567890abcdef1234567890abcdef12345678",
		},
		{
			name:            "non-match (404)",
			tagName:         "v1.2.3",
			responseStatus:  404,
			responseMessage: `Not found`,
			errorMessage:    "release not found",
		},
		{
			name:            "server error (500)",
			tagName:         "v1.2.3",
			responseStatus:  500,
			responseMessage: `arbitrary error"`,
			errorMessage:    "HTTP 500: arbitrary error\" (https://api.github.com/repos/owner/repo/git/ref/tags/v1.2.3)",
		},
		{