ronantakizawa/codereview-bench huggingface dataset:
Repository: claudiodangelis/qrcp
File: server/server.go (Go)

Code before:
	"gopkg.in/cheggaaa/pb.v1"
)

// Server is the server
type Server struct {
	// SendURL is the URL used to send the file
	SendURL string
	// ReceiveURL is the URL used to Receive the file
	ReceiveURL  string
	instance    *http.Server
	payload     payload.Payload
	outputDir   string
	stopChannel chan bool
	// expectParallelRequests is set to true when qrcp sends files, in order
	// to support downloading of parallel chunks
	expectParallelRequests bool
}

// ReceiveTo sets the output directory
func (s *Server) ReceiveTo(dir string) error {
	output, err := filepath.Abs(dir)
	if err != nil {
		return err
	}
	// Check if the output dir exists
	if dir, _ := os.Stat(output); dir == nil || !dir.IsDir() {
		return fmt.Errorf("%s is not a valid directory", output)
	}
	s.outputDir = output
	return nil
}

// Send adds a handler for sending the file
func (s *Server) Send(p payload.Payload) {
	s.payload = p
	s.expectParallelRequests = true
}

// Wait for transfer to be completed, it waits forever if kept awlive
func (s Server) Wait() error {
	<-s.stopChannel
	if err := s.instance.Shutdown(context.Background()); err != nil {
		log.Println(err)
	}
	if s.payload.DeleteAfterTransfer {
		s.payload.Delete()
	}
	return nil
}

// New instance of the server

Review comment (suggestion):
what about

```go
dir, err := os.Stat(output)
if err != nil {
    return err
}
if !dir.IsDir() {
    return fmt.Errorf("%s is not a valid directory", output)
}
```


Code after:
	"gopkg.in/cheggaaa/pb.v1"
)

// Server is the server
type Server struct {
	// SendURL is the URL used to send the file
	SendURL string
	// ReceiveURL is the URL used to Receive the file
	ReceiveURL  string
	instance    *http.Server
	payload     payload.Payload
	outputDir   string
	stopChannel chan bool
	// expectParallelRequests is set to true when qrcp sends files, in order
	// to support downloading of parallel chunks
	expectParallelRequests bool
}

// ReceiveTo sets the output directory
func (s *Server) ReceiveTo(dir string) error {
	output, err := filepath.Abs(dir)
	if err != nil {
		return err
	}
	// Check if the output dir exists
	fileinfo, err := os.Stat(output)
	if err != nil {
		return err
	}
	if !fileinfo.IsDir() {
		return fmt.Errorf("%s is not a valid directory", output)
	}
	s.outputDir = output
	return nil
}

// Send adds a handler for sending the file
func (s *Server) Send(p payload.Payload) {
	s.payload = p
	s.expectParallelRequests = true
}

// Wait for transfer to be completed, it waits forever if kept awlive
func (s Server) Wait() error {
	<-s.stopChannel
	if err := s.instance.Shutdown(context.Background()); err != nil {
		log.Println(err)
	}
	if s.payload.DeleteAfterTransfer {
		s.payload.Delete()
	}
Repository: continuedev/continue
File: core/autocomplete/templating/AutocompleteTemplate.ts (TypeScript)

Code before:
// https://github.com/QwenLM/Qwen2.5-Coder?tab=readme-ov-file#3-file-level-code-completion-fill-in-the-middle
// This issue asks about the use of 
 and 
 together with 
, 
 and 

// https://github.com/QwenLM/Qwen2.5-Coder/issues/343
const qwenCoderFimTemplate: AutocompleteTemplate = {
  template:
    "
{{{prefix}}}
{{{suffix}}}
",
  completionOptions: {
    stop: [
      "
",
      "
",
      "
",
      "
",
      "
",
      "
",
      "
",
      "
",
      "
",
    ],
  },
};

const seedCoderFimTemplate: AutocompleteTemplate = {
  template:
    "<[fim-prefix]>{{{prefix}}}<[fim-suffix]>{{{suffix}}}<[fim-middle]>",
  completionOptions: {
    stop: [
      "<[end▁of▁sentence]>",
      "<[fim-prefix]>",
      "<[fim-middle]>",
      "<[fim-suffix]>",
      "<[PAD▁TOKEN]>",
      "<[SEP▁TOKEN]>",
      "<[begin▁of▁sentence]>",
    ],
  },
};

const codestralFimTemplate: AutocompleteTemplate = {
  template: "[SUFFIX]{{{suffix}}}[PREFIX]{{{prefix}}}",
  completionOptions: {
    stop: ["[PREFIX]", "[SUFFIX]"],
  },
};

const codestralMultifileFimTemplate: AutocompleteTemplate = {
  compilePrefixSuffix: (
    prefix,
    suffix,
    filepath,
    reponame,
    snippets,

Review comment (suggestion):
It wasn't clear to me (as a total novice in this codebase) what the purpose of completionOptions.stop is w.r.t. the FIM template. I would assume that stop sequences are implemented on the model inference side, but I'm guessing this is some extra logic to stop the FIM task early? If they're not critical, then we can collapse this net-new template into the `qwenCoderFimTemplate` above.

Code after:
// https://github.com/QwenLM/Qwen2.5-Coder?tab=readme-ov-file#3-file-level-code-completion-fill-in-the-middle
// This issue asks about the use of 
 and 
 together with 
, 
 and 

// https://github.com/QwenLM/Qwen2.5-Coder/issues/343
const qwenCoderFimTemplate: AutocompleteTemplate = {
  template:
    "
{{{prefix}}}
{{{suffix}}}
",
  completionOptions: {
    stop: [
      "
",
      "
",
      "
",
      "
",
      "
",
      "
",
      "
",
      "
",
      "
",
    ],
  },
};

// https://www.ibm.com/granite/docs/models/granite#fim
const granite4FimTemplate: AutocompleteTemplate = {
  template:
    "
{{{prefix}}}
{{{suffix}}}
",
  completionOptions: {
    stop: [
      "
",
      "
",
      "
",
      "
",
      "
",
    ],
  },
};

const seedCoderFimTemplate: AutocompleteTemplate = {
  template:
    "<[fim-prefix]>{{{prefix}}}<[fim-suffix]>{{{suffix}}}<[fim-middle]>",
  completionOptions: {
    stop: [
      "<[end▁of▁sentence]>",
      "<[fim-prefix]>",
      "<[fim-middle]>",
      "<[fim-suffix]>",
      "<[PAD▁TOKEN]>",
      "<[SEP▁TOKEN]>",
      "<[begin▁of▁sentence]>",
    ],
  },
};
Repository: continuedev/continue
File: extensions/intellij/src/main/kotlin/com/github/continuedev/continueintellijextension/browser/ContinueBrowserService.kt (Kotlin)

Code before:
package com.github.continuedev.continueintellijextension.browser

import com.intellij.openapi.Disposable
import com.intellij.openapi.components.Service
import com.intellij.openapi.components.service
import com.intellij.openapi.project.Project
import com.intellij.openapi.util.Disposer
import com.intellij.ui.jcef.JBCefApp

@Service(Service.Level.PROJECT)
class ContinueBrowserService(project: Project): Disposable {

    private val browser: ContinueBrowser? =
        if (JBCefApp.isSupported())
            ContinueBrowser(project)
        else null

    override fun dispose() {
        if (browser != null)
            Disposer.dispose(browser)
    }

    companion object {

        fun Project.getBrowser(): ContinueBrowser? {
            if (isDisposed)
                return null
            return service<ContinueBrowserService>().browser
        }

    }
}


Review comment (suggestion):
Most of the logic is same to
```
private val browser: ContinueBrowser? =
        if (JBCefApp.isSupported())
            ContinueBrowser(project)
        else null
```

Code after:
package com.github.continuedev.continueintellijextension.browser

import com.intellij.openapi.Disposable
import com.intellij.openapi.application.ApplicationManager
import com.intellij.openapi.components.Service
import com.intellij.openapi.components.service
import com.intellij.openapi.project.Project
import com.intellij.openapi.util.Disposer
import com.intellij.ui.jcef.JBCefApp

@Service(Service.Level.PROJECT)
class ContinueBrowserService(val project: Project): Disposable {

    private var browser: ContinueBrowser? = null

    init {
        load()
    }

    override fun dispose() {
        browser?.let { Disposer.dispose(it) }
        browser = null
    }

    private fun load(): ContinueBrowser? {
        if (browser != null) {
            return browser
        }
        if (!JBCefApp.isSupported()) {
            return null
        }
        val newBrowser = ContinueBrowser(project)
        Disposer.register(this, newBrowser)

        this.browser = newBrowser
        return this.browser
    }

    /**
     * Reloads the browser by disposing the current one and creating a new one.
     * This method is intended for use when browser is frozen (unresponsive).
     */
    fun reload() {
        // Store the old browser instance to be disposed later
        val oldBrowser = browser
        browser = null

        // Dispose the old browser after the new one is loaded and UI is updated.
        // This avoids race conditions. We can do this on a background thread.
        oldBrowser?.let {
Repository: continuedev/continue
File: gui/src/redux/thunks/streamThunkWrapper.tsx (TypeScript)

Code before:
        saveCurrentSession({
          openNewSession: false,
          generateTitle: true,
        }),
      );
    }
  } catch (e) {
    await dispatch(cancelStream());
    dispatch(setDialogMessage(<StreamErrorDialog error={e} />));
    dispatch(setShowDialog(true));

    // Get the selected model from the state for error analysis
    const state = getState();
    const selectedModel = selectSelectedChatModel(state);

    const { parsedError, statusCode, modelTitle, providerName } = analyzeError(
      e,
      selectedModel,
    );

    const errorData = {
      error_type: statusCode ? `HTTP ${statusCode}` : "Unknown",
      error_message: parsedError,
      model_provider: providerName,
      model_title: modelTitle,
    };

    posthog.capture("gui_stream_error", errorData);
  }
});


Review comment (refactor):
I think checking `attempt < OVERLOADED_RETRIES` here has 2 issues
- it's duplicate of having the for loop with attempt <= OVERLOADED_RETRIES
- it wills top at e.g. 2 when OVERLOADED_RETRIES = 3

Let's change to either use recursive stop at depth OVERLOADED_RETRIES approach or just the for loop

Code after:
  void,
  () => Promise<void>,
  ThunkApiType
>("chat/streamWrapper", async (runStream, { dispatch, getState }) => {
  for (let attempt = 0; attempt <= OVERLOADED_RETRIES; attempt++) {
    try {
      await runStream();
      const state = getState();
      if (!state.session.isInEdit) {
        await dispatch(
          saveCurrentSession({
            openNewSession: false,
            generateTitle: true,
          }),
        );
      }
      return;
    } catch (e) {
      // Get the selected model from the state for error analysis
      const state = getState();
      const selectedModel = selectSelectedChatModel(state);
      const { parsedError, statusCode, message, modelTitle, providerName } =
        analyzeError(e, selectedModel);

      const shouldRetry =
        isOverloadedErrorMessage(message) && attempt < OVERLOADED_RETRIES;

      if (shouldRetry) {
        await dispatch(cancelStream());
        const delayMs = OVERLOADED_DELAY_MS * 2 ** attempt;
        await new Promise((resolve) => setTimeout(resolve, delayMs));
        await dispatch(cancelStream());
      } else {
        await dispatch(cancelStream());
        dispatch(setDialogMessage(<StreamErrorDialog error={e} />));
        dispatch(setShowDialog(true));

        const errorData = {
          error_type: statusCode ? `HTTP ${statusCode}` : "Unknown",
          error_message: parsedError,
          model_provider: providerName,
          model_title: modelTitle,
        };

        posthog.capture("gui_stream_error", errorData);
        return;
      }
    }
  }
});
Repository: continuedev/continue
File: core/index.d.ts (TypeScript)

Code before:
  group: string;
  originalFunctionName?: string;
  systemMessageDescription?: {
    prefix: string;
    exampleArgs?: Array<[string, string | number]>;
  };
  defaultToolPolicy?: ToolPolicy;
  toolCallIcon?: string;
  preprocessArgs?: (
    args: Record<string, unknown>,
    extras: {
      ide: IDE;
    },
  ) => Promise<Record<string, unknown>>;
  evaluateToolCallPolicy?: (
    basePolicy: ToolPolicy,
    parsedArgs: Record<string, unknown>,
    processedArgs?: Record<string, unknown>,
  ) => ToolPolicy;
}

/**
 * Configuration for overriding built-in tool prompts.
 * Allows customization of tool descriptions and behavior at the repo level.
 */
export interface ToolOverride {
  /** Tool name to override (matches function.name, e.g., "read_file", "run_terminal_command") */
  name: string;
  /** Override the tool's description shown to the LLM */
  description?: string;
  /** Override the display title shown in UI */
  displayTitle?: string;
  /** Override the action phrases */
  wouldLikeTo?: string;
  isCurrently?: string;
  hasAlready?: string;
  /** Override system message description for non-native tool calling */
  systemMessageDescription?: {
    prefix?: string;
    exampleArgs?: Array<[string, string | number]>;
  };
  /** Set to true to disable this tool */
  disabled?: boolean;
}

interface ToolChoice {
  type: "function";
  function: {
    name: string;
  };
}

Review comment (suggestion):
Ideally use `Partial<Pick<Tool, ....>>` or similar for this type

Code after:
  isCurrently?: string;
  hasAlready?: string;
  readonly: boolean;
  isInstant?: boolean;
  uri?: string;
  faviconUrl?: string;
  group: string;
  originalFunctionName?: string;
  systemMessageDescription?: {
    prefix: string;
    exampleArgs?: Array<[string, string | number]>;
  };
  defaultToolPolicy?: ToolPolicy;
  toolCallIcon?: string;
  preprocessArgs?: (
    args: Record<string, unknown>,
    extras: {
      ide: IDE;
    },
  ) => Promise<Record<string, unknown>>;
  evaluateToolCallPolicy?: (
    basePolicy: ToolPolicy,
    parsedArgs: Record<string, unknown>,
    processedArgs?: Record<string, unknown>,
  ) => ToolPolicy;
}

/**
 * Configuration for overriding built-in tool prompts.
 * Extends ToolOverrideConfig with required name for array usage.
 */
export type ToolOverride = ToolOverrideConfig & {
  /** Tool name to override (matches function.name, e.g., "read_file") */
  name: string;
};

interface ToolChoice {
  type: "function";
  function: {
    name: string;
  };
}

export interface ConfigDependentToolParams {
  rules: RuleWithSource[];
  enableExperimentalTools: boolean;
  isSignedIn: boolean;
  isRemote: boolean;
  modelName: string | undefined;
  ide: IDE;
}
Repository: continuedev/continue
File: core/index.d.ts (TypeScript)

Code before:
  group: string;
  originalFunctionName?: string;
  systemMessageDescription?: {
    prefix: string;
    exampleArgs?: Array<[string, string | number]>;
  };
  defaultToolPolicy?: ToolPolicy;
  toolCallIcon?: string;
  preprocessArgs?: (
    args: Record<string, unknown>,
    extras: {
      ide: IDE;
    },
  ) => Promise<Record<string, unknown>>;
  evaluateToolCallPolicy?: (
    basePolicy: ToolPolicy,
    parsedArgs: Record<string, unknown>,
    processedArgs?: Record<string, unknown>,
  ) => ToolPolicy;
}

/**
 * Configuration for overriding built-in tool prompts.
 * Allows customization of tool descriptions and behavior per model.
 */
export interface ToolOverride {
  /** Tool name to override (matches function.name, e.g., "read_file", "run_terminal_command") */
  name: string;
  /** Override the tool's description shown to the LLM */
  description?: string;
  /** Override the display title shown in UI */
  displayTitle?: string;
  /** Override the action phrases */
  wouldLikeTo?: string;
  isCurrently?: string;
  hasAlready?: string;
  /** Override system message description for non-native tool calling */
  systemMessageDescription?: {
    prefix?: string;
    exampleArgs?: Array<[string, string | number]>;
  };
  /** Set to true to disable this tool */
  disabled?: boolean;
}

interface ToolChoice {
  type: "function";
  function: {
    name: string;
  };
}

Review comment (suggestion):
Ideally use typescript `Pick` for this type to reduce duplication

Code after:
  isInstant?: boolean;
  uri?: string;
  faviconUrl?: string;
  group: string;
  originalFunctionName?: string;
  systemMessageDescription?: {
    prefix: string;
    exampleArgs?: Array<[string, string | number]>;
  };
  defaultToolPolicy?: ToolPolicy;
  toolCallIcon?: string;
  preprocessArgs?: (
    args: Record<string, unknown>,
    extras: {
      ide: IDE;
    },
  ) => Promise<Record<string, unknown>>;
  evaluateToolCallPolicy?: (
    basePolicy: ToolPolicy,
    parsedArgs: Record<string, unknown>,
    processedArgs?: Record<string, unknown>,
  ) => ToolPolicy;
}

/**
 * Configuration for overriding built-in tool prompts.
 * Extends ToolOverrideConfig with required name for array usage.
 */
export type ToolOverride = ToolOverrideConfig & {
  /** Tool name to override (matches function.name, e.g., "read_file") */
  name: string;
};

interface ToolChoice {
  type: "function";
  function: {
    name: string;
  };
}

export interface ConfigDependentToolParams {
  rules: RuleWithSource[];
  enableExperimentalTools: boolean;
  isSignedIn: boolean;
  isRemote: boolean;
  modelName: string | undefined;
  ide: IDE;
}

export type GetTool = (params: ConfigDependentToolParams) => Promise<Tool>;
Repository: cookiecutter/cookiecutter
File: tests/test_main.py (Python)

Code before:
from cookiecutter.main import is_repo_url


def test_is_repo_url():
    """Verify is_repo_url works."""
    assert is_repo_url('gitolite@server:team/repo') is True
    assert is_repo_url('git@github.com:audreyr/cookiecutter.git') is True
    assert is_repo_url('https://github.com/audreyr/cookiecutter.git') is True
    assert is_repo_url('gh:audreyr/cookiecutter-pypackage') is True

    assert is_repo_url('/audreyr/cookiecutter.git') is False
    assert is_repo_url('/home/audreyr/cookiecutter') is False


Review comment (suggestion):
I am afraid this is not a test for `mercurial`, but for the abbreviations.

I was thinking of testing something like `https://bitbucket.org/pokoli/cookiecutter-trytonmodule.hg` :no_mouth: 


Code after:
from cookiecutter.main import is_repo_url


def test_is_repo_url():
    """Verify is_repo_url works."""
    assert is_repo_url('gitolite@server:team/repo') is True
    assert is_repo_url('git@github.com:audreyr/cookiecutter.git') is True
    assert is_repo_url('https://github.com/audreyr/cookiecutter.git') is True
    assert is_repo_url('gh:audreyr/cookiecutter-pypackage') is True
    assert is_repo_url('https://bitbucket.org/pokoli/cookiecutter.hg') is True

    assert is_repo_url('/audreyr/cookiecutter.git') is False
    assert is_repo_url('/home/audreyr/cookiecutter') is False
Repository: cookiecutter/cookiecutter
File: tests/test_main.py (Python)

Code before:
from cookiecutter.main import is_repo_url


def test_is_repo_url():
    """Verify is_repo_url works."""
    assert is_repo_url('gitolite@server:team/repo') is True
    assert is_repo_url('git@github.com:audreyr/cookiecutter.git') is True
    assert is_repo_url('https://github.com/audreyr/cookiecutter.git') is True
    assert is_repo_url('gh:audreyr/cookiecutter-pypackage') is True

    assert is_repo_url('/audreyr/cookiecutter.git') is False
    assert is_repo_url('/home/audreyr/cookiecutter') is False


Review comment (suggestion):
Someone may change this very method to only work for _github_ and we wouldn't notice a problem testing-wise, so IMHO it makes sense to have a simple test for it.


Code after:
from cookiecutter.main import is_repo_url


def test_is_repo_url():
    """Verify is_repo_url works."""
    assert is_repo_url('gitolite@server:team/repo') is True
    assert is_repo_url('git@github.com:audreyr/cookiecutter.git') is True
    assert is_repo_url('https://github.com/audreyr/cookiecutter.git') is True
    assert is_repo_url('gh:audreyr/cookiecutter-pypackage') is True
    assert is_repo_url('https://bitbucket.org/pokoli/cookiecutter.hg') is True

    assert is_repo_url('/audreyr/cookiecutter.git') is False
    assert is_repo_url('/home/audreyr/cookiecutter') is False
Repository: cookiecutter/cookiecutter
File: tests/test_cli.py (Python)

Code before:
        side_effect=cookiecutter
    )

    template_path = 'tests/fake-repo-pre/'
    result = runner.invoke(main, [
        template_path,
        '--no-input',
        '--replay',
        '-v'
    ])

    assert result.exit_code == 1

    expected_error_msg = (
        "You can not use both replay and no_input or extra_context "
        "at the same time."
    )
    assert expected_error_msg in result.output

    mock_cookiecutter.assert_called_once_with(
        template_path,
        None,
        True,
        replay=True
    )


Review comment (suggestion):
@hackebrot I left this test intact because I felt that it was good to test both the options together and verify that the command exits normally.


Code after:
    result = runner.invoke(main, [
        template_path,
        '--no-input',
        '--replay',
        '-v'
    ])

    assert result.exit_code == 1

    expected_error_msg = (
        "You can not use both replay and no_input or extra_context "
        "at the same time."
    )

    assert expected_error_msg in result.output

    mock_cookiecutter.assert_called_once_with(
        template_path,
        None,
        True,
        replay=True,
        overwrite_if_exists=False
    )

@pytest.mark.usefixtures('remove_fake_project_dir')
def test_cli_exit_on_overwrite_if_exists_and_replay(mocker):
    mock_cookiecutter = mocker.patch(
        'cookiecutter.cli.cookiecutter',
        side_effect=cookiecutter
    )

    template_path = 'tests/fake-repo-pre/'
    result = runner.invoke(main, [
        template_path,
        '--replay',
        '-v',
        '-f',
    ])

    assert result.exit_code == 0

    mock_cookiecutter.assert_called_once_with(
        template_path,
        None,
        False,
        replay=True,
        overwrite_if_exists=True
    )


@pytest.mark.usefixtures('remove_fake_project_dir')
Repository: cookiecutter/cookiecutter
File: tests/test_cli.py (Python)

Code before:
        side_effect=cookiecutter
    )

    template_path = 'tests/fake-repo-pre/'
    result = runner.invoke(main, [
        template_path,
        '--no-input',
        '--replay',
        '-v'
    ])

    assert result.exit_code == 1

    expected_error_msg = (
        "You can not use both replay and no_input or extra_context "
        "at the same time."
    )
    assert expected_error_msg in result.output

    mock_cookiecutter.assert_called_once_with(
        template_path,
        None,
        True,
        replay=True
    )


Review comment (suggestion):
I see. :no_mouth: 

The name is a bit misleading, I'm afraid. I thought it is supposed to test that **replay** and **overwrite** cause a the cli to exit without running **cookiecutter** (see `test_cli_exit_on_noinput_and_replay` which asserts for an error exit code).


Code after:
    result = runner.invoke(main, [
        template_path,
        '--no-input',
        '--replay',
        '-v'
    ])

    assert result.exit_code == 1

    expected_error_msg = (
        "You can not use both replay and no_input or extra_context "
        "at the same time."
    )

    assert expected_error_msg in result.output

    mock_cookiecutter.assert_called_once_with(
        template_path,
        None,
        True,
        replay=True,
        overwrite_if_exists=False
    )

@pytest.mark.usefixtures('remove_fake_project_dir')
def test_cli_exit_on_overwrite_if_exists_and_replay(mocker):
    mock_cookiecutter = mocker.patch(
        'cookiecutter.cli.cookiecutter',
        side_effect=cookiecutter
    )

    template_path = 'tests/fake-repo-pre/'
    result = runner.invoke(main, [
        template_path,
        '--replay',
        '-v',
        '-f',
    ])

    assert result.exit_code == 0

    mock_cookiecutter.assert_called_once_with(
        template_path,
        None,
        False,
        replay=True,
        overwrite_if_exists=True
    )


@pytest.mark.usefixtures('remove_fake_project_dir')