ronantakizawa/codereview-bench huggingface dataset:
Repository: chatwoot/chatwoot
File: app/controllers/webhooks/shopify_controller.rb (Ruby)

Code before:
class Webhooks::ShopifyController < ActionController::API
  before_action :verify_hmac!

  def events
    case request.headers['X-Shopify-Topic']
    when 'shop/redact'
      handle_shop_redact
    end

    head :ok
  end

  private

  def verify_hmac!
    secret = GlobalConfigService.load('SHOPIFY_CLIENT_SECRET', nil)
    return head :unauthorized if secret.blank?

    data = request.body.read
    request.body.rewind

    hmac_header = request.headers['X-Shopify-Hmac-SHA256']
    return head :unauthorized if hmac_header.blank?

    computed = Base64.strict_encode64(OpenSSL::HMAC.digest('SHA256', secret, data))
    head :unauthorized unless ActiveSupport::SecurityUtils.secure_compare(computed, hmac_header)
  end

  def handle_shop_redact
    shop_domain = params[:shop_domain]
    return if shop_domain.blank?

    Integrations::Hook.where(app_id: 'shopify', reference_id: shop_domain).destroy_all
  end
end


Review comment (bug):
Let's do an explicit return here to halt the chain on failure
```suggestion
    return head :unauthorized unless ActiveSupport::SecurityUtils.secure_compare(computed, hmac_header)
```

Code after:
class Webhooks::ShopifyController < ActionController::API
  before_action :verify_hmac!

  def events
    case request.headers['X-Shopify-Topic']
    when 'shop/redact'
      handle_shop_redact
    end

    head :ok
  end

  private

  def verify_hmac!
    secret = GlobalConfigService.load('SHOPIFY_CLIENT_SECRET', nil)
    return head :unauthorized if secret.blank?

    data = request.body.read
    request.body.rewind

    hmac_header = request.headers['X-Shopify-Hmac-SHA256']
    return head :unauthorized if hmac_header.blank?

    computed = Base64.strict_encode64(OpenSSL::HMAC.digest('SHA256', secret, data))
    return head :unauthorized unless ActiveSupport::SecurityUtils.secure_compare(computed, hmac_header)
  end

  def handle_shop_redact
    shop_domain = params[:shop_domain]
    return if shop_domain.blank?

    Integrations::Hook.where(app_id: 'shopify', reference_id: shop_domain).destroy_all
  end
end
Repository: chatwoot/chatwoot
File: app/javascript/dashboard/components-next/Contacts/ContactsForm/ContactsForm.vue (Vue)

Code before:
const prepareStateBasedOnProps = () => {
  if (props.isNewContact) {
    return; // Added to prevent state update for new contact form
  }

  const {
    id,
    name = '',
    email: emailAddress,
    phoneNumber,
    additionalAttributes = {},
  } = props.contactData || {};
  const { firstName, lastName } = splitName(name || '');
  const {
    description = '',
    companyName = '',
    countryCode = '',
    country = '',
    city = '',
    socialTelegramUserName = '',
    social_telegram_user_name: socialTelegramUserNameLegacy = '',
    socialProfiles = {},
    social_profiles: socialProfilesLegacy = {},
  } = additionalAttributes || {};

  const telegramUsername =
    socialProfiles?.telegram ||
    socialProfilesLegacy?.telegram ||
    socialTelegramUserName ||
    socialTelegramUserNameLegacy ||
    '';

  Object.assign(state, {
    id,
    name,
    firstName,
    lastName,
    email: emailAddress,
    phoneNumber,
    additionalAttributes: {
      description,
      companyName,
      countryCode,
      country,
      city,
      socialProfiles: {
        ...(socialProfilesLegacy || {}),
        ...(socialProfiles || {}),
        telegram: telegramUsername,
      },
    },

Review comment (refactor):
@muhsin-k Do we need the full fallback chain here? `additionalAttributes` is already `camelCased` by `getContactById`, so I think we can simplify this and remove the legacy fields.

```suggestion
    socialTelegramUserName = '',
    socialProfiles = {},
  } = additionalAttributes || {};

  const telegramUsername =
    socialProfiles?.telegram || socialTelegramUserName || '';

```

Code after:
const prepareStateBasedOnProps = () => {
  if (props.isNewContact) {
    return; // Added to prevent state update for new contact form
  }

  const {
    id,
    name = '',
    email: emailAddress,
    phoneNumber,
    additionalAttributes = {},
  } = props.contactData || {};
  const { firstName, lastName } = splitName(name || '');
  const {
    description = '',
    companyName = '',
    countryCode = '',
    country = '',
    city = '',
    socialTelegramUserName = '',
    socialProfiles = {},
  } = additionalAttributes || {};

  const telegramUsername =
    socialProfiles?.telegram || socialTelegramUserName || '';

  Object.assign(state, {
    id,
    name,
    firstName,
    lastName,
    email: emailAddress,
    phoneNumber,
    additionalAttributes: {
      description,
      companyName,
      countryCode,
      country,
      city,
      socialProfiles: {
        ...socialProfiles,
        telegram: telegramUsername,
      },
    },
  });
};

const countryOptions = computed(() =>
  countries.map(({ name, id }) => ({ label: name, value: id }))
);
Repository: chatwoot/chatwoot
File: app/javascript/dashboard/components-next/Contacts/ContactsForm/ContactsForm.vue (Vue)

Code before:
    country = '',
    city = '',
    socialTelegramUserName = '',
    social_telegram_user_name: socialTelegramUserNameLegacy = '',
    socialProfiles = {},
    social_profiles: socialProfilesLegacy = {},
  } = additionalAttributes || {};

  const telegramUsername =
    socialProfiles?.telegram ||
    socialProfilesLegacy?.telegram ||
    socialTelegramUserName ||
    socialTelegramUserNameLegacy ||
    '';

  Object.assign(state, {
    id,
    name,
    firstName,
    lastName,
    email: emailAddress,
    phoneNumber,
    additionalAttributes: {
      description,
      companyName,
      countryCode,
      country,
      city,
      socialProfiles: {
        ...(socialProfilesLegacy || {}),
        ...(socialProfiles || {}),
        telegram: telegramUsername,
      },
    },
  });
};

const countryOptions = computed(() =>
  countries.map(({ name, id }) => ({ label: name, value: id }))
);

const editDetailsForm = computed(() =>
  Object.keys(FORM_CONFIG).map(key => ({
    key,
    placeholder: t(
      `CONTACTS_LAYOUT.CARD.EDIT_DETAILS_FORM.FORM.${key}.PLACEHOLDER`
    ),
  }))
);

const socialProfilesForm = computed(() =>

Review comment (suggestion):
```suggestion
      socialProfiles: {
        ...socialProfiles,
        telegram: telegramUsername,
      },
```

Code after:
    country = '',
    city = '',
    socialTelegramUserName = '',
    socialProfiles = {},
  } = additionalAttributes || {};

  const telegramUsername =
    socialProfiles?.telegram || socialTelegramUserName || '';

  Object.assign(state, {
    id,
    name,
    firstName,
    lastName,
    email: emailAddress,
    phoneNumber,
    additionalAttributes: {
      description,
      companyName,
      countryCode,
      country,
      city,
      socialProfiles: {
        ...socialProfiles,
        telegram: telegramUsername,
      },
    },
  });
};

const countryOptions = computed(() =>
  countries.map(({ name, id }) => ({ label: name, value: id }))
);

const editDetailsForm = computed(() =>
  Object.keys(FORM_CONFIG).map(key => ({
    key,
    placeholder: t(
      `CONTACTS_LAYOUT.CARD.EDIT_DETAILS_FORM.FORM.${key}.PLACEHOLDER`
    ),
  }))
);

const socialProfilesForm = computed(() =>
  Object.entries(SOCIAL_CONFIG).map(([key, icon]) => ({
    key,
    placeholder: t(`CONTACTS_LAYOUT.CARD.SOCIAL_MEDIA.FORM.${key}.PLACEHOLDER`),
    icon,
  }))
);
Repository: cli/cli
File: command/pr.go (Go)

Code before:
}

func printPrs(prs ...api.PullRequest) {
	for _, pr := range prs {
		prNumber := fmt.Sprintf("#%d", pr.Number)
		fmt.Printf("  %s  %s %s", utils.Yellow(prNumber), truncate(50, pr.Title), utils.Cyan("["+pr.HeadLabel()+"]"))

		checks := pr.ChecksStatus()
		reviews := pr.ReviewStatus()
		if checks.Total > 0 || reviews.ChangesRequested || reviews.Approved {
			fmt.Printf("\n  ")
		}

		if checks.Total > 0 {
			var ratio string
			if checks.Failing > 0 {
				ratio = fmt.Sprintf("%d/%d", checks.Passing, checks.Total)
				ratio = utils.Red(ratio)
			} else if checks.Pending > 0 {
				ratio = fmt.Sprintf("%d/%d", checks.Passing, checks.Total)
				ratio = utils.Yellow(ratio)
			} else if checks.Passing == checks.Total {
				ratio = fmt.Sprintf("%d", checks.Total)
				ratio = utils.Green(ratio)
			}
			fmt.Printf(" - checks: %s", ratio)
		}

		if reviews.ChangesRequested {
			fmt.Printf(" - %s", utils.Red("changes requested"))
		} else if reviews.ReviewRequired {
			fmt.Printf(" - %s", utils.Yellow("review required"))
		} else if reviews.Approved {
			fmt.Printf(" - %s", utils.Green("approved"))
		}

		fmt.Printf("\n")
	}
}

func printHeader(s string) {
	fmt.Println(utils.Bold(s))
}

func printMessage(s string) {
	fmt.Println(utils.Gray(s))
}

func truncate(maxLength int, title string) string {
	if len(title) > maxLength {
		return title[0:maxLength-3] + "..."

Review comment (suggestion):
I'm a little worried about this `if` block. It's hard to tell just from reading it if the summary will always be set. 

Code after:
}

func printPrs(prs ...api.PullRequest) {
	for _, pr := range prs {
		prNumber := fmt.Sprintf("#%d", pr.Number)
		fmt.Printf("  %s  %s %s", utils.Yellow(prNumber), truncate(50, pr.Title), utils.Cyan("["+pr.HeadLabel()+"]"))

		checks := pr.ChecksStatus()
		reviews := pr.ReviewStatus()
		if checks.Total > 0 || reviews.ChangesRequested || reviews.Approved {
			fmt.Printf("\n  ")
		}

		if checks.Total > 0 {
			var summary string
			if checks.Failing > 0 {
				if checks.Failing == checks.Total {
					summary = utils.Red("All checks failing")
				} else {
					summary = utils.Red(fmt.Sprintf("%d/%d checks failing", checks.Failing, checks.Total))
				}
			} else if checks.Pending > 0 {
				summary = utils.Yellow("Checks pending")
			} else if checks.Passing == checks.Total {
				summary = utils.Green("Checks passing")
			}
			fmt.Printf(" - %s", summary)
		}

		if reviews.ChangesRequested {
			fmt.Printf(" - %s", utils.Red("changes requested"))
		} else if reviews.ReviewRequired {
			fmt.Printf(" - %s", utils.Yellow("review required"))
		} else if reviews.Approved {
			fmt.Printf(" - %s", utils.Green("approved"))
		}

		fmt.Printf("\n")
	}
}

func printHeader(s string) {
	fmt.Println(utils.Bold(s))
}

func printMessage(s string) {
	fmt.Println(utils.Gray(s))
}

func truncate(maxLength int, title string) string {
	if len(title) > maxLength {
Repository: cli/cli
File: command/repo.go (Go)

Code before:
	Short: "Create a new repository",
	Long: `Create a new GitHub repository.

Use the "ORG/NAME" syntax to create a repository within your organization.`,
	RunE: repoCreate,
}

var repoForkCmd = &cobra.Command{
	Use:   "fork [<repository>]",
	Short: "Create a fork of a repository",
	Long: `Create a fork of a repository.

With no argument, creates a fork of the current repository. Otherwise, forks the specified repository.`,
	RunE: repoFork,
}

var repoViewCmd = &cobra.Command{
	Use:   "view [<repository>]",
	Short: "View a repository in the browser",
	Long: `View a GitHub repository.

With no argument, the repository for the current directory is displayed.`,
	RunE: repoView,
}

func parseExtraArgs(extraArgs []string) (args []string, target string) {
	args = extraArgs

	if len(args) > 0 {
		if !strings.HasPrefix(args[0], "-") {
			target, args = args[0], args[1:]
		}
	}
	return
}

func runClone(cloneURL string, args []string) (target string, err error) {
	cloneArgs, target := parseExtraArgs(args)

	cloneArgs = append(cloneArgs, cloneURL)

	// If the args contain an explicit target, pass it to clone
	//    otherwise, parse the URL to determine where git cloned it to so we can return it
	if target != "" {
		cloneArgs = append(cloneArgs, target)
	} else {
		target = path.Base(strings.TrimSuffix(cloneURL, ".git"))
	}

	cloneArgs = append([]string{"clone"}, cloneArgs...)


Review comment (suggestion):
Nit: this function has a generic-sounding name, but applies specifically to parsing clone arguments (it makes little sense otherwise). Since this function is shared across all `command` package, which is our biggest package, we could benefit from it having a more specific name, such as `parseCloneArgs`

Code after:

Use the "ORG/NAME" syntax to create a repository within your organization.`,
	RunE: repoCreate,
}

var repoForkCmd = &cobra.Command{
	Use:   "fork [<repository>]",
	Short: "Create a fork of a repository",
	Long: `Create a fork of a repository.

With no argument, creates a fork of the current repository. Otherwise, forks the specified repository.`,
	RunE: repoFork,
}

var repoViewCmd = &cobra.Command{
	Use:   "view [<repository>]",
	Short: "View a repository in the browser",
	Long: `View a GitHub repository.

With no argument, the repository for the current directory is displayed.`,
	RunE: repoView,
}

func parseCloneArgs(extraArgs []string) (args []string, target string) {
	args = extraArgs

	if len(args) > 0 {
		if !strings.HasPrefix(args[0], "-") {
			target, args = args[0], args[1:]
		}
	}
	return
}

func runClone(cloneURL string, args []string) (target string, err error) {
	cloneArgs, target := parseCloneArgs(args)

	cloneArgs = append(cloneArgs, cloneURL)

	// If the args contain an explicit target, pass it to clone
	//    otherwise, parse the URL to determine where git cloned it to so we can return it
	if target != "" {
		cloneArgs = append(cloneArgs, target)
	} else {
		target = path.Base(strings.TrimSuffix(cloneURL, ".git"))
	}

	cloneArgs = append([]string{"clone"}, cloneArgs...)

	cloneCmd := git.GitCommand(cloneArgs...)
	cloneCmd.Stdin = os.Stdin
Repository: cli/cli
File: pkg/cmd/repo/create/create.go (Go)

Code before:
		if !strings.Contains(viewURL, "/") {
			currentUser, err := api.CurrentLoginName(apiClient, ghinstance.Default())
			if err != nil {
				return err
			}
			viewURL = currentUser + "/" + viewURL
		}
		toView, err = ghrepo.FromFullName(viewURL)
		if err != nil {
			return fmt.Errorf("argument error: %w", err)
		}

		repo, err := api.GitHubRepo(apiClient, toView)
		if err != nil {
			return err
		}

		opts.Template = repo.ID
	}

	input := repoCreateInput{
		Name:             repoToCreate.RepoName(),
		Visibility:       visibility,
		OwnerID:          repoToCreate.RepoOwner(),
		TeamID:           opts.Team,
		RepositoryID:     opts.Template,
		Description:      opts.Description,
		HomepageURL:      opts.Homepage,
		HasIssuesEnabled: opts.EnableIssues,
		HasWikiEnabled:   opts.EnableWiki,
	}

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

	createLocalDirectory := opts.ConfirmSubmit
	if !opts.ConfirmSubmit {
		opts.ConfirmSubmit, err = confirmSubmission(input.Name, input.OwnerID, &opts.ConfirmSubmit)
		if err != nil {
			return err
		}
	}

	if opts.ConfirmSubmit {
		repo, err := repoCreate(httpClient, repoToCreate.RepoHost(), input)
		if err != nil {
			return err
		}


Review comment (refactor):
Since this type is intended to pass as input for the create repo mutation I'm not in love with using it to signal the desire to clone from a template.

I'd rather see:

- a new helper, `repoCreateFromTemplate`
- a change to `repoCreate`'s function signature to accept a potentially empty template argument

Code after:
			if err != nil {
				return err
			}
			cloneURL = currentUser + "/" + cloneURL
		}
		toClone, err = ghrepo.FromFullName(cloneURL)
		if err != nil {
			return fmt.Errorf("argument error: %w", err)
		}

		repo, err := api.GitHubRepo(apiClient, toClone)
		if err != nil {
			return err
		}

		opts.Template = repo.ID
	}

	input := repoCreateInput{
		Name:             repoToCreate.RepoName(),
		Visibility:       visibility,
		OwnerID:          repoToCreate.RepoOwner(),
		TeamID:           opts.Team,
		Description:      opts.Description,
		HomepageURL:      opts.Homepage,
		HasIssuesEnabled: opts.EnableIssues,
		HasWikiEnabled:   opts.EnableWiki,
	}

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

	createLocalDirectory := opts.ConfirmSubmit
	if !opts.ConfirmSubmit {
		opts.ConfirmSubmit, err = confirmSubmission(input.Name, input.OwnerID, &opts.ConfirmSubmit)
		if err != nil {
			return err
		}
	}

	if opts.ConfirmSubmit {
		repo, err := repoCreate(httpClient, repoToCreate.RepoHost(), input, opts.Template)
		if err != nil {
			return err
		}

		stderr := opts.IO.ErrOut
		stdout := opts.IO.Out
		greenCheck := utils.Green("✓")
Repository: cli/cli
File: pkg/cmd/codespace/ssh.go (Go)

Code before:
		err = os.Rename(privateKey, privateKeyNew)
		if err != nil {
			return nil
		}

		keyPair := &ssh.KeyPair{
			PublicKeyPath:  publicKeyNew,
			PrivateKeyPath: privateKeyNew,
		}

		return keyPair
	}

	return nil
}

// hasUploadedValidCustomKey checks which private keys are used in ssh configration and
// compares them to uploaded public keys to see if there is a match
func hasUploadedPublicKeyForConfig(
	ctx context.Context,
	sshContext ssh.Context,
	apiClient apiClient,
) bool {
	configuredPrivateKeyPaths, err := getConfiguredPrivateKeys(ctx)
	if err != nil {
		return false
	}

	configuredPublicKeys := []string{}
	for _, privateKeyPath := range configuredPrivateKeyPaths {
		if _, err := os.Stat(privateKeyPath); err != nil {
			// The default configuration includes standard keys like id_rsa,
			// id_ed25519, etc, but these may not actually exist
			continue
		}

		configuredPublicKey, err := sshContext.GetPublicKeyFromPrivateKey(privateKeyPath)
		if err != nil {
			continue
		}

		configuredPublicKeys = append(configuredPublicKeys, configuredPublicKey)
	}

	if len(configuredPublicKeys) == 0 {
		// There are no local private keys which ssh would use
		return false
	}

	user, err := apiClient.GetUser(ctx)
	if err != nil {

Review comment (suggestion):
A lot of the error handling is very basic/non-existent currently, once we're good on using this approach I'll circle back and polish it

Code after:
	if keyPair != nil {
		return keyPair, nil
	}

	keyPair, err := sshContext.GenerateSSHKey(automaticPrivateKeyName, "")
	if err != nil && !errors.Is(err, ssh.ErrKeyAlreadyExists) {
		return nil, err
	}

	return keyPair, nil
}

// checkAndUpdateOldKeyPair handles backward compatibility with the old keypair names.
// If the old public and private keys both exist they are renamed to the new name.
// The return value is non-nil only if the rename happens.
func checkAndUpdateOldKeyPair(sshContext ssh.Context) *ssh.KeyPair {
	publicKeys, err := sshContext.LocalPublicKeys()
	if err != nil {
		return nil
	}

	for _, publicKey := range publicKeys {
		if filepath.Base(publicKey) != automaticPrivateKeyNameOld+".pub" {
			continue
		}

		privateKey := strings.TrimSuffix(publicKey, ".pub")
		_, err := os.Stat(privateKey)
		if err != nil {
			continue
		}

		// Both old public and private keys exist, rename them to the new name

		sshDir := filepath.Dir(publicKey)

		publicKeyNew := filepath.Join(sshDir, automaticPrivateKeyName+".pub")
		err = os.Rename(publicKey, publicKeyNew)
		if err != nil {
			return nil
		}

		privateKeyNew := filepath.Join(sshDir, automaticPrivateKeyName)
		err = os.Rename(privateKey, privateKeyNew)
		if err != nil {
			return nil
		}

		keyPair := &ssh.KeyPair{
			PublicKeyPath:  publicKeyNew,
			PrivateKeyPath: privateKeyNew,
Repository: cli/cli
File: pkg/cmd/codespace/ssh.go (Go)

Code before:
	if err != nil {
		return false
	}

	configuredPublicKeys := []string{}
	for _, privateKeyPath := range configuredPrivateKeyPaths {
		publicKeyPath := privateKeyPath + ".pub"

		publicKeyContent, err := os.ReadFile(publicKeyPath)
		if err != nil {
			// The default configuration includes standard keys like id_rsa,
			// id_ed25519, etc, but these may not actually exist so just skip
			continue
		}

		configuredPublicKeys = append(configuredPublicKeys, string(publicKeyContent))
	}

	if len(configuredPublicKeys) == 0 {
		// There are no local private keys which ssh would use
		return false
	}

	user, err := apiClient.GetUser(ctx)
	if err != nil {
		return false
	}

	uploadedPublicKeys, err := apiClient.AuthorizedKeys(ctx, user.Login)
	if err != nil {
		return false
	}

	if len(uploadedPublicKeys) == 0 {
		return false
	}

	for _, configuredPublicKey := range configuredPublicKeys {
		for _, uploadedPublicKey := range uploadedPublicKeys {
			if configuredPublicKey == uploadedPublicKey {
				return true
			}
		}
	}

	return false
}

// getConfiguredPrivateKeys reads the effective configuration for a localhost
// connection and returns all private keys which would be tried for authentication
func getConfiguredPrivateKeys(

Review comment (bug):
If this fails (network blip, api issue, etc), we'll likely see a problem with the user being able to SSH somewhere downstream of this process, is it worth swallowing the error? Can't we return it? It may make it harder to figure out user reports otherwise.

Code after:
		if filepath.Base(publicKey) != automaticPrivateKeyNameOld+".pub" {
			continue
		}

		privateKey := strings.TrimSuffix(publicKey, ".pub")
		_, err := os.Stat(privateKey)
		if err != nil {
			continue
		}

		// Both old public and private keys exist, rename them to the new name

		sshDir := filepath.Dir(publicKey)

		publicKeyNew := filepath.Join(sshDir, automaticPrivateKeyName+".pub")
		err = os.Rename(publicKey, publicKeyNew)
		if err != nil {
			return nil
		}

		privateKeyNew := filepath.Join(sshDir, automaticPrivateKeyName)
		err = os.Rename(privateKey, privateKeyNew)
		if err != nil {
			return nil
		}

		keyPair := &ssh.KeyPair{
			PublicKeyPath:  publicKeyNew,
			PrivateKeyPath: privateKeyNew,
		}

		return keyPair
	}

	return nil
}

// hasUploadedValidCustomKey checks which private keys are used in ssh configration and
// compares them to uploaded public keys to see if there is a match
func hasUploadedPublicKeyForConfig(
	ctx context.Context,
	apiClient apiClient,
	customConfigFile string,
	customHost string,
) (bool, error) {
	configuredPrivateKeyPaths, err := getConfiguredPrivateKeys(ctx, customConfigFile, customHost)
	if err != nil {
		return false, fmt.Errorf("getting local ssh keys: %w", err)
	}

	configuredPublicKeys := make(map[string]bool)