ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/onefuzz
File: src/runtime-tools/linux/run.sh (Shell)

Code before:
#!/bin/bash
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.

set -ex

export PATH=$PATH:/onefuzz/bin:/onefuzz/tools/linux:/onefuzz/tools/linux/afl:/onefuzz/tools/linux/radamsa
export DOTNET_ROOT=/onefuzz/tools/dotnet
export ONEFUZZ_TOOLS=/onefuzz/tools
export ONEFUZZ_ROOT=/onefuzz
export RUST_BACKTRACE=full
export RUST_LOG="${RUST_LOG:=info}"
export LLVM_SYMBOLIZER_PATH=/onefuzz/bin/llvm-symbolizer

logger "onefuzz: starting up onefuzz"

#check if we are running in docker
if [ -f /.dockerenv ]
then
    echo "Running in docker:
    to optimize the experience make sure the host os is stup propery. with the follwing command
    # use core files, not external crash handler
    echo core | sudo tee /proc/sys/kernel/core_pattern
    # disable ASLR
    echo 0 | sudo tee /proc/sys/kernel/randomize_va_space
    # set core dumping to default behavior
    echo 1 | sudo tee /proc/sys/fs/suid_dumpable"

else
    # use core files, not external crash handler
    echo core | sudo tee /proc/sys/kernel/core_pattern
    # disable ASLR
    echo 0 | sudo tee /proc/sys/kernel/randomize_va_space
    # set core dumping to default behavior
    echo 1 | sudo tee /proc/sys/fs/suid_dumpable
fi

cd /onefuzz
MODE=$(cat /onefuzz/etc/mode)
case ${MODE} in
    "proxy")
        logger "onefuzz: starting proxy"
        echo proxy
        onefuzz-proxy-manager --config /onefuzz/config.json
    ;;
    "fuzz")

Review comment (performance):
```suggestion
    to optimize the experience make sure the host os is setup properly. with the following command
```

Code after:
#!/bin/bash
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.

set -ex

export PATH=$PATH:/onefuzz/bin:/onefuzz/tools/linux:/onefuzz/tools/linux/afl:/onefuzz/tools/linux/radamsa
export DOTNET_ROOT=/onefuzz/tools/dotnet
export ONEFUZZ_TOOLS=/onefuzz/tools
export ONEFUZZ_ROOT=/onefuzz
export RUST_BACKTRACE=full
export RUST_LOG="${RUST_LOG:=info}"
export LLVM_SYMBOLIZER_PATH=/onefuzz/bin/llvm-symbolizer

logger "onefuzz: starting up onefuzz"

#check if we are running in docker
if [ -f /.dockerenv ]; then
    echo "Running in docker:
    to optimize the experience make sure the host os is setup properly. with the following command
    # use core files, not external crash handler
    echo core | sudo tee /proc/sys/kernel/core_pattern
    # disable ASLR
    echo 0 | sudo tee /proc/sys/kernel/randomize_va_space
    # set core dumping to default behavior
    echo 1 | sudo tee /proc/sys/fs/suid_dumpable"
else
    # use core files, not external crash handler
    echo core | sudo tee /proc/sys/kernel/core_pattern
    # disable ASLR
    echo 0 | sudo tee /proc/sys/kernel/randomize_va_space
    # set core dumping to default behavior
    echo 1 | sudo tee /proc/sys/fs/suid_dumpable
fi

cd /onefuzz
MODE=$(cat /onefuzz/etc/mode)
case ${MODE} in
    "proxy")
        logger "onefuzz: starting proxy"
        echo proxy
        onefuzz-proxy-manager --config /onefuzz/config.json
    ;;
    "fuzz")
        logger "onefuzz: starting fuzzing"
        echo fuzzing
Repository: microsoft/onefuzz
File: src/ApiService/ApiService/OneFuzzTypes/Model.cs (C#)

Code before:
public record ADODuplicateTemplate(
    List<string> Increment,
    Dictionary<string, string> SetState,
    Dictionary<string, string> AdoFields,
    string? Comment = null
);

public record AdoTemplate(
    Uri BaseUrl,
    SecretData<string> AuthToken,
    string Project,
    string Type,
    List<string> UniqueFields,
    Dictionary<string, string> AdoFields,
    ADODuplicateTemplate OnDuplicate,
    string? Comment = null
    ) : NotificationTemplate {
    public async Task<OneFuzzResultVoid> Validate() {
        return await Ado.Validate(this);
    }
}

public record TeamsTemplate(SecretData<string> Url) : NotificationTemplate {
    public Task<OneFuzzResultVoid> Validate() {
        // The only way we can validate in the current state is to send a test webhook
        // MAybe there's a teams nuget package we can pull in to help validate
        return Async.Task.FromResult(OneFuzzResultVoid.Ok);
    }
}

public record GithubAuth(string User, string PersonalAccessToken);

public record GithubIssueSearch(
    List<GithubIssueSearchMatch> FieldMatch,
    [property: JsonPropertyName("string")] String str,
    string? Author = null,
    GithubIssueState? State = null
);

public record GithubIssueDuplicate(
    List<string> Labels,
    bool Reopen,
    string? Comment = null
);


public record GithubIssuesTemplate(
    SecretData<GithubAuth> Auth,
    string Organization,
    string Repository,
    string Title,

Review comment (suggestion):
```suggestion    
        // Maybe there's a teams nuget package we can pull in to help validate
````

Code after:

        return obj;
    }
}


public record ADODuplicateTemplate(
    List<string> Increment,
    Dictionary<string, string> SetState,
    Dictionary<string, string> AdoFields,
    string? Comment = null
);

public record AdoTemplate(
    Uri BaseUrl,
    SecretData<string> AuthToken,
    string Project,
    string Type,
    List<string> UniqueFields,
    Dictionary<string, string> AdoFields,
    ADODuplicateTemplate OnDuplicate,
    string? Comment = null
    ) : NotificationTemplate {
    public async Task<OneFuzzResultVoid> Validate() {
        return await Ado.Validate(this);
    }
}

public record TeamsTemplate(SecretData<string> Url) : NotificationTemplate {
    public Task<OneFuzzResultVoid> Validate() {
        // The only way we can validate in the current state is to send a test webhook
        // Maybe there's a teams nuget package we can pull in to help validate
        return Async.Task.FromResult(OneFuzzResultVoid.Ok);
    }
}

public record GithubAuth(string User, string PersonalAccessToken);

public record GithubIssueSearch(
    List<GithubIssueSearchMatch> FieldMatch,
    [property: JsonPropertyName("string")] String str,
    string? Author = null,
    GithubIssueState? State = null
);

public record GithubIssueDuplicate(
    List<string> Labels,
    bool Reopen,
    string? Comment = null
);
Repository: microsoft/onefuzz
File: src/agent/onefuzz-task/src/tasks/task_logger.rs (Rust)

Code before:
                        pending_logs: _,
                        state: LoopState::Done,
                        event: _,
                    }) => break,
                    Ok(c) => c,
                    Err(e) => {
                        error!("task logger failure {}", e);
                        return Err(e);
                    }
                };
            }
            Ok(())
        });

        Ok(SpawnedLogger { logger_handle })
    }
}

pub struct SpawnedLogger {
    logger_handle: tokio::task::JoinHandle<Result<()>>,
}

impl SpawnedLogger {
    pub async fn flush_and_stop(self, timeout: Duration) -> Result<()> {
        if let Ok(Err(e)) = tokio::time::timeout(timeout, self.logger_handle).await {
            error!("task logger failure {}", e);
            return Err(e.into());
        }
        Ok(())
    }
}

#[cfg(test)]
mod tests {
    use std::{
        collections::HashMap,
        sync::{Arc, RwLock},
    };

    use super::*;
    use onefuzz_telemetry::LogTrace;
    use reqwest::Url;

    fn create_log_trace(level: log::Level, message: String) -> LogTrace {
        LogTrace {
            timestamp: chrono::Utc::now(),
            level,
            message,
        }
    }


Review comment (bug):
NIT: maybe choose some different text to log so that when we see it it'll be easy to know exactly where it's coming from?

ex:
```suggestion
            error!("failed to flush and stop task logger {}", e);
```

Code after:
                        pending_logs: _,
                        state: LoopState::Done,
                        event: _,
                    }) => break,
                    Ok(c) => c,
                    Err(e) => {
                        error!("task logger failure {}", e);
                        return Err(e);
                    }
                };
            }
            Ok(())
        });

        Ok(SpawnedLogger { logger_handle })
    }
}

pub struct SpawnedLogger {
    logger_handle: tokio::task::JoinHandle<Result<()>>,
}

impl SpawnedLogger {
    pub async fn flush_and_stop(self, timeout: Duration) -> Result<()> {
        if let Ok(Err(e)) = tokio::time::timeout(timeout, self.logger_handle).await {
            error!("failed to flush and stop task logger {}", e);
            return Err(e.into());
        }
        Ok(())
    }
}

#[cfg(test)]
mod tests {
    use std::{
        collections::HashMap,
        sync::{Arc, RwLock},
    };

    use super::*;
    use onefuzz_telemetry::LogTrace;
    use reqwest::Url;

    fn create_log_trace(level: log::Level, message: String) -> LogTrace {
        LogTrace {
            timestamp: chrono::Utc::now(),
            level,
            message,
        }
    }
Repository: microsoft/onefuzz
File: src/ApiService/ApiService/onefuzzlib/Utils.cs (C#)

Code before:

            if (result.Count == size) {
                yield return result;
                result = new List<TSource>(size);
            }
        }

        if (result.Count > 0) {
            yield return result;
        }
    }
}

public static class TruncateUtils {
    public static List<string> TruncateList(List<string> data, int maxLength) {
        int currentLength = 0;
        return data.TakeWhile(curr => (currentLength += curr.Length) <= maxLength).ToList();
    }

    public static string TruncateString(string data, int maxLength) {
        return data[..Math.Min(data.Length, maxLength)];
    }

    public static string? TruncateNullableString(string? data, int maxLength) {
        return data?[..Math.Min(data.Length, maxLength)];
    }
}


Review comment (suggestion):
You could unify these with:

```suggestion
    [return: NotNullIfNotNull("data")]
    public static string? TruncateString(string? data, int maxLength) {
        return data?[..Math.Min(data.Length, maxLength)];
    }
```

Code after:
        while (await enumerator.MoveNextAsync()) {
            result.Add(enumerator.Current);

            if (result.Count == size) {
                yield return result;
                result = new List<TSource>(size);
            }
        }

        if (result.Count > 0) {
            yield return result;
        }
    }
}

public static class TruncateUtils {
    public static List<string> TruncateList(List<string> data, int maxLength) {
        int currentLength = 0;
        return data.TakeWhile(curr => (currentLength += curr.Length) <= maxLength).ToList();
    }

    [return: NotNullIfNotNull(nameof(data))]
    public static string? TruncateString(string? data, int maxLength) {
        return data?[..Math.Min(data.Length, maxLength)];
    }
}
Repository: microsoft/onefuzz
File: src/cli/onefuzz/azcopy.py (Python)

Code before:
import os
import shutil
import subprocess  # nosec


def find_azcopy() -> str:
    azcopy = os.environ.get("AZCOPY")

    if azcopy:
        if not os.path.exists(azcopy):
            raise Exception(f"AZCOPY environment variable is invalid: {azcopy}")
        elif os.path.isdir(azcopy):
            contains_azcopy = "azcopy.exe" in [
                f for f in os.listdir(azcopy) if os.path.isfile(os.path.join(azcopy, f))
            ]

            if contains_azcopy:
                azcopy = os.path.join(azcopy, "azcopy.exe")
            else:
                raise Exception(
                    f"The directory specified by AZCOPY doesn't contain the file 'azcopy.exe': {azcopy}"
                )
    else:
        azcopy = shutil.which("azcopy")

    if not azcopy:
        raise Exception(
            "unable to find 'azcopy' in path or AZCOPY environment variable"
        )

    return azcopy


def azcopy_sync(src: str, dst: str) -> None:
    """Expose azcopy for syncing existing files"""

    azcopy = find_azcopy()

    # security note: callers need to understand the src/dst for this.
    subprocess.check_output([azcopy, "sync", src, dst, "--recursive=true"])  # nosec

Review comment (refactor):
Suggest simplifying this:
```suggestion
            contains_azcopy = os.path.isfile(os.path.join(azcopy, "azcopy.exe"))
```

Code after:
import os
import shutil
import subprocess  # nosec


def find_azcopy() -> str:
    azcopy = os.environ.get("AZCOPY")
    binary_name = "azcopy" if os.name == "posix" else "azcopy.exe"

    if azcopy:
        if not os.path.exists(azcopy):
            raise Exception(f"AZCOPY environment variable is invalid: {azcopy}")
        elif os.path.isdir(azcopy):
            contains_azcopy = os.path.isfile(os.path.join(azcopy, binary_name))

            if contains_azcopy:
                azcopy = os.path.join(azcopy, binary_name)
            else:
                raise Exception(
                    f"The directory specified by AZCOPY doesn't contain the file '{binary_name}': {azcopy}"
                )
    else:
        azcopy = shutil.which("azcopy")

    if not azcopy:
        raise Exception(
            "unable to find 'azcopy' in path or AZCOPY environment variable"
        )

    return azcopy


def azcopy_sync(src: str, dst: str) -> None:
    """Expose azcopy for syncing existing files"""

    azcopy = find_azcopy()

    # security note: callers need to understand the src/dst for this.
    subprocess.check_output([azcopy, "sync", src, dst, "--recursive=true"])  # nosec
Repository: microsoft/onefuzz
File: src/agent/coverage/src/allowlist.rs (Rust)

Code before:
        // Deny rules are of the form `! <rule>`.
        if let Some(expr) = line.strip_prefix("! ") {
            let re = glob_to_regex(expr)?;
            return Ok(Self::Deny(re));
        }

        // Try to interpret as allow rule.
        let re = glob_to_regex(line)?;
        Ok(Self::Allow(re))
    }
}

#[allow(clippy::single_char_pattern)]
fn glob_to_regex(expr: &str) -> Result<Regex> {
    let expr = regex::escape(expr);

    // Translate escaped glob wildcards into quantified regexes.
    let expr = expr.replace(r"\*", ".*");

    // Anchor to line start and end.
    let expr = format!("^{expr}$");

    let mut rb = RegexBuilder::new(&expr);

    if cfg!(windows) {
        rb.case_insensitive(true);
    }

    Ok(rb.build()?)
}

#[cfg(test)]
mod tests;


Review comment (suggestion):
The allowlist matching is unit-tested. I suggest adding at least one test decorated with `[cfg(target_os = "windows")]` which exercises the new matching logic. Maybe also a Linux-only test that demonstrates case-sensitivity on that platform.

Code after:
        // Deny rules are of the form `! <rule>`.
        if let Some(expr) = line.strip_prefix("! ") {
            let re = glob_to_regex(expr)?;
            return Ok(Self::Deny(re));
        }

        // Try to interpret as allow rule.
        let re = glob_to_regex(line)?;
        Ok(Self::Allow(re))
    }
}

#[allow(clippy::single_char_pattern)]
fn glob_to_regex(expr: &str) -> Result<Regex> {
    let expr = regex::escape(expr);

    // Translate escaped glob wildcards into quantified regexes.
    let expr = expr.replace(r"\*", ".*");

    // Anchor to line start and end.
    // On Windows we should also ignore case.
    let expr = if cfg!(windows) {
        format!("(?i)^{expr}$")
    } else {
        format!("^{expr}$")
    };

    Ok(Regex::new(&expr)?)
}

#[cfg(test)]
mod tests;
Repository: microsoft/retina
File: cli/cmd/capture/capture.go (Go)

Code before:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

package capture

import (
	"github.com/microsoft/retina/cli/cmd"
	"github.com/spf13/cobra"
	"k8s.io/cli-runtime/pkg/genericclioptions"
)

var opts = struct {
	genericclioptions.ConfigFlags
	Name *string
}{}

var capture = &cobra.Command{
	Use:   "capture",
	Short: "capture network traffic",
}

func init() {
	cmd.Retina.AddCommand(capture)
	opts.ConfigFlags = *genericclioptions.NewConfigFlags(true)
	opts.AddFlags(capture.PersistentFlags())
	capture.PersistentFlags().StringVar(opts.Name, "name", "", "The name of the Retina Capture")
	_ = capture.MarkPersistentFlagRequired("name")
}


Review comment (suggestion):
I know it's a small thing here, but this might be better as a method of `opts`. You can inject whatever the value of `capture.PersistentFlags()` is and let opts add Flags to it. It'd make it a little more modular by letting the opts say "I know what my flags should be".

Not a blocker, but just an idea.

Code after:
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

package capture

import (
	"github.com/microsoft/retina/cli/cmd"
	"github.com/spf13/cobra"
	"k8s.io/cli-runtime/pkg/genericclioptions"
)

var opts = struct {
	genericclioptions.ConfigFlags
	Name *string
}{}

const defaultName = "retina-capture"

var capture = &cobra.Command{
	Use:   "capture",
	Short: "Capture network traffic",
}

func init() {
	cmd.Retina.AddCommand(capture)
	opts.ConfigFlags = *genericclioptions.NewConfigFlags(true)
	opts.AddFlags(capture.PersistentFlags())
	capture.PersistentFlags().StringVar(opts.Name, "name", defaultName, "The name of the Retina Capture")
}
Repository: microsoft/retina
File: test/e2e/framework/kubernetes/get-external-crd.go (Go)

Code before:
package kubernetes

import (
	"fmt"
	"io"
	"log"
	"net/http"
	"net/url"
	"os"
	"path"
	"path/filepath"
)

func downloadExternalCRDs(chartPath string) error {
	crdUrls := []string{
		"https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/main/example/prometheus-operator-crd/monitoring.coreos.com_servicemonitors.yaml",
	}

	for _, crdUrl := range crdUrls {
		crd, err := fetchYAML(crdUrl)
		if err == nil && crd != nil {
			crdName, err := extractFileName(crdUrl)
			if err == nil && crdName != "" {
				log.Printf("CRD exists %s", crdName)
				log.Printf("File path to be written to %s", filepath.Join(chartPath, "/crds/"+crdName))
				err = saveToFile(filepath.Join(chartPath, "/crds/"+crdName), crd)
				if err != nil {
					return fmt.Errorf("failed to save crd.yaml /crds dir: %w", err)
				}
			}
		}
		if err != nil {
			return fmt.Errorf("failed to fetch crd from %s: %w", crdUrl, err)
		}
	}
	return nil
}

func fetchYAML(url string) ([]byte, error) {
	resp, err := http.Get(url)
	if err != nil {
		return nil, fmt.Errorf("failed to get crd source code: %w", err)
	}
	defer resp.Body.Close()
	return io.ReadAll(resp.Body)
}

Review comment (suggestion):
error handling in go is normally `if err != nil {}` then happy path. Please remove `if err == nil`

Code after:
package kubernetes

import (
	"fmt"
	"io"
	"log"
	"net/http"
	"net/url"
	"os"
	"path"
	"path/filepath"
)

func downloadExternalCRDs(chartPath string) error {
	crdUrls := []string{
		"https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/main/example/prometheus-operator-crd/monitoring.coreos.com_servicemonitors.yaml",
	}

	for _, crdUrl := range crdUrls {
		crd, err := fetchYAML(crdUrl)
		if err != nil {
			return err
		}

		crdName, err := extractFileName(crdUrl)
		if err != nil {
			return err
		}

		log.Printf("CRD exists %s", crdName)
		log.Printf("File path to be written to %s", filepath.Join(chartPath, "/crds/"+crdName))
		err = saveToFile(filepath.Join(chartPath, "/crds/"+crdName), crd)
		if err != nil {
			return err
		}
	}
	return nil
}

func fetchYAML(url string) ([]byte, error) {
	resp, err := http.Get(url)
	if err != nil {
		return nil, fmt.Errorf("failed to get crd source code from %s: %w", url, err)
	}
	defer resp.Body.Close()
	return io.ReadAll(resp.Body)
Repository: microsoft/retina
File: test/e2e/framework/kubernetes/get-external-crd.go (Go)

Code before:
	"net/url"
	"os"
	"path"
	"path/filepath"
)

func downloadExternalCRDs(chartPath string) error {
	crdUrls := []string{
		"https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/main/example/prometheus-operator-crd/monitoring.coreos.com_servicemonitors.yaml",
	}

	for _, crdUrl := range crdUrls {
		crd, err := fetchYAML(crdUrl)
		if err == nil && crd != nil {
			crdName, err := extractFileName(crdUrl)
			if err == nil && crdName != "" {
				log.Printf("CRD exists %s", crdName)
				log.Printf("File path to be written to %s", filepath.Join(chartPath, "/crds/"+crdName))
				err = saveToFile(filepath.Join(chartPath, "/crds/"+crdName), crd)
				if err != nil {
					return fmt.Errorf("failed to save crd.yaml /crds dir: %w", err)
				}
			}
		}
		if err != nil {
			return fmt.Errorf("failed to fetch crd from %s: %w", crdUrl, err)
		}
	}
	return nil
}

func fetchYAML(url string) ([]byte, error) {
	resp, err := http.Get(url)
	if err != nil {
		return nil, fmt.Errorf("failed to get crd source code: %w", err)
	}
	defer resp.Body.Close()
	return io.ReadAll(resp.Body)
}

func extractFileName(rawURL string) (string, error) {
	parsedURL, err := url.Parse(rawURL)
	if err != nil {
		return "", fmt.Errorf("failed to parse url: %w", err)
	}
	return path.Base(parsedURL.Path), nil
}

func saveToFile(filename string, data []byte) error {
	err := os.WriteFile(filename, data, 0644)
	if err != nil {

Review comment (suggestion):
this block should be straight at the start, just after `crd, err := fetchYAML(crdUrl)` - (relates to previous comments)

Code after:
	"net/url"
	"os"
	"path"
	"path/filepath"
)

func downloadExternalCRDs(chartPath string) error {
	crdUrls := []string{
		"https://raw.githubusercontent.com/prometheus-operator/prometheus-operator/main/example/prometheus-operator-crd/monitoring.coreos.com_servicemonitors.yaml",
	}

	for _, crdUrl := range crdUrls {
		crd, err := fetchYAML(crdUrl)
		if err != nil {
			return err
		}

		crdName, err := extractFileName(crdUrl)
		if err != nil {
			return err
		}

		log.Printf("CRD exists %s", crdName)
		log.Printf("File path to be written to %s", filepath.Join(chartPath, "/crds/"+crdName))
		err = saveToFile(filepath.Join(chartPath, "/crds/"+crdName), crd)
		if err != nil {
			return err
		}
	}
	return nil
}

func fetchYAML(url string) ([]byte, error) {
	resp, err := http.Get(url)
	if err != nil {
		return nil, fmt.Errorf("failed to get crd source code from %s: %w", url, err)
	}
	defer resp.Body.Close()
	return io.ReadAll(resp.Body)
}

func extractFileName(rawURL string) (string, error) {
	parsedURL, err := url.Parse(rawURL)
	if err != nil {
		return "", fmt.Errorf("failed to parse url: %w", err)
	}
	return path.Base(parsedURL.Path), nil
}

func saveToFile(filename string, data []byte) error {
	err := os.WriteFile(filename, data, 0644)