ronantakizawa/codereview-bench huggingface dataset:
Repository: openai/codex
File: codex-rs/core/src/command_safety/is_dangerous_command.rs (Rust)

Code before:
                "branch",
                "-D",
                "feature",
            ])),
            true
        );
        assert_eq!(
            command_might_be_dangerous(&vec_str(&["bash", "-lc", "git -C . branch -d feature",])),
            true
        );
    }

    #[test]
    fn rm_rf_is_dangerous() {
        assert_eq!(
            command_might_be_dangerous(&vec_str(&["rm", "-rf", "/"])),
            true
        );
    }

    #[test]
    fn rm_f_is_dangerous() {
        assert_eq!(
            command_might_be_dangerous(&vec_str(&["rm", "-f", "/"])),
            true
        );
    }
}


Review comment (bug):
This may be an overeager rule about avoiding assert! triggering somewhere (model / AGENTS.md?). It's fine to use assert on bool props, but only for things that aren't going to fail and then hide evaluating assertions in the tests after the failure.

Code after:
    }

    #[test]
    fn usr_bin_git_is_dangerous() {
        assert!(command_might_be_dangerous(&vec_str(&[
            "/usr/bin/git",
            "reset",
            "--hard",
        ])));
    }

    #[test]
    fn git_branch_delete_is_dangerous() {
        assert!(command_might_be_dangerous(&vec_str(&[
            "git", "branch", "-d", "feature",
        ])));
        assert!(command_might_be_dangerous(&vec_str(&[
            "git", "branch", "-D", "feature",
        ])));
        assert!(command_might_be_dangerous(&vec_str(&[
            "bash",
            "-lc",
            "git branch --delete feature",
        ])));
    }

    #[test]
    fn git_branch_delete_with_stacked_short_flags_is_dangerous() {
        assert!(command_might_be_dangerous(&vec_str(&[
            "git", "branch", "-dv", "feature",
        ])));
        assert!(command_might_be_dangerous(&vec_str(&[
            "git", "branch", "-vd", "feature",
        ])));
        assert!(command_might_be_dangerous(&vec_str(&[
            "git", "branch", "-vD", "feature",
        ])));
        assert!(command_might_be_dangerous(&vec_str(&[
            "git", "branch", "-Dvv", "feature",
        ])));
    }

    #[test]
    fn git_branch_delete_with_global_options_is_dangerous() {
        assert!(command_might_be_dangerous(&vec_str(&[
            "git", "-C", ".", "branch", "-d", "feature",
        ])));
        assert!(command_might_be_dangerous(&vec_str(&[
            "git",
            "-c",
            "color.ui=false",
Repository: openai/codex
File: codex-rs/core/src/tasks/compact.rs (Rust)

Code before:
#[async_trait]
impl SessionTask for CompactTask {
    fn kind(&self) -> TaskKind {
        TaskKind::Compact
    }

    async fn run(
        self: Arc<Self>,
        session: Arc<SessionTaskContext>,
        ctx: Arc<TurnContext>,
        input: Vec<UserInput>,
        _cancellation_token: CancellationToken,
    ) -> Option<String> {
        let session = session.clone_session();
        if crate::compact::should_use_remote_compact_task(&ctx.provider) {
            let _ = session.services.otel_manager.counter(
                "codex.task.compact",
                1,
                &[("type", "remote")],
            );
            if crate::compact_remote::run_remote_compact_task(session, ctx)
                .await
                .is_err()
            {
                return None;
            }
        } else {
            let _ = session.services.otel_manager.counter(
                "codex.task.compact",
                1,
                &[("type", "local")],
            );
            if crate::compact::run_compact_task(session, ctx, input)
                .await
                .is_err()
            {
                return None;
            }
        }

        None
    }
}


Review comment (bug):
I think in this file you can actually just do `crate::compact_remote::run_remote_compact_task(session, ctx).await`, no need to return None upon error here because we just return None anyway.

Code after:
#[async_trait]
impl SessionTask for CompactTask {
    fn kind(&self) -> TaskKind {
        TaskKind::Compact
    }

    async fn run(
        self: Arc<Self>,
        session: Arc<SessionTaskContext>,
        ctx: Arc<TurnContext>,
        input: Vec<UserInput>,
        _cancellation_token: CancellationToken,
    ) -> Option<String> {
        let session = session.clone_session();
        if crate::compact::should_use_remote_compact_task(&ctx.provider) {
            let _ = session.services.otel_manager.counter(
                "codex.task.compact",
                1,
                &[("type", "remote")],
            );
            let _ = crate::compact_remote::run_remote_compact_task(session, ctx).await;
        } else {
            let _ = session.services.otel_manager.counter(
                "codex.task.compact",
                1,
                &[("type", "local")],
            );
            let _ = crate::compact::run_compact_task(session, ctx, input).await;
        }

        None
    }
}
Repository: openai/codex
File: codex-rs/utils/sleep-inhibitor/src/linux_inhibitor.rs (Rust)

Code before:
                "--who",
                APP_ID,
                "--why",
                ASSERTION_REASON,
                "--",
                "sleep",
                BLOCKER_SLEEP_SECONDS,
            ]);
            command
        }
        LinuxBackend::GnomeSessionInhibit => {
            let mut command = Command::new("gnome-session-inhibit");
            command.args([
                "--inhibit",
                "idle",
                "--reason",
                ASSERTION_REASON,
                "sleep",
                BLOCKER_SLEEP_SECONDS,
            ]);
            command
        }
    };

    unsafe {
        command.pre_exec(|| {
            if libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGTERM) == -1 {
                return Err(std::io::Error::last_os_error());
            }
            Ok(())
        });
    }

    command.spawn()
}

fn child_exited(error: &std::io::Error) -> bool {
    matches!(error.kind(), std::io::ErrorKind::InvalidInput)
}


Review comment (suggestion):
I think we have a race here. If the parent dies in pre-exec, the child can miss the dead signal and become orphan

We have a safer implementation in `utils/pty/src/process_group.rs` I think

Code after:
            let mut command = Command::new("systemd-inhibit");
            command.args([
                "--what=idle",
                "--mode=block",
                "--who",
                APP_ID,
                "--why",
                ASSERTION_REASON,
                "--",
                "sleep",
                BLOCKER_SLEEP_SECONDS,
            ]);
            command
        }
        LinuxBackend::GnomeSessionInhibit => {
            let mut command = Command::new("gnome-session-inhibit");
            command.args([
                "--inhibit",
                "idle",
                "--reason",
                ASSERTION_REASON,
                "sleep",
                BLOCKER_SLEEP_SECONDS,
            ]);
            command
        }
    };
    command
        .stdin(Stdio::null())
        .stdout(Stdio::null())
        .stderr(Stdio::null());

    // SAFETY: `pre_exec` must be registered before spawn. The closure only
    // performs libc setup for the child process and returns an `io::Error`
    // when parent-death signal setup fails.
    unsafe {
        command.pre_exec(move || {
            if libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGTERM) == -1 {
                return Err(std::io::Error::last_os_error());
            }
            if libc::getppid() != parent_pid {
                libc::raise(libc::SIGTERM);
            }
            Ok(())
        });
    }

    command.spawn()
}

fn child_exited(error: &std::io::Error) -> bool {
Repository: openai/codex
File: codex-rs/utils/sleep-inhibitor/src/linux_inhibitor.rs (Rust)

Code before:
    command
        .stdin(Stdio::null())
        .stdout(Stdio::null())
        .stderr(Stdio::null());

    // SAFETY: `pre_exec` must be registered before spawn. The closure only
    // performs libc setup for the child process and returns an `io::Error`
    // when parent-death signal setup fails.
    unsafe {
        command.pre_exec(move || {
            if libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGTERM) == -1 {
                return Err(std::io::Error::last_os_error());
            }
            if libc::getppid() != parent_pid {
                libc::raise(libc::SIGTERM);
            }
            Ok(())
        });
    }

    command.spawn()
}

fn child_exited(error: &std::io::Error) -> bool {
    matches!(error.kind(), std::io::ErrorKind::InvalidInput)
}


Review comment (suggestion):
please add a test for the invariant you've documented

```suggestion
}

#[cfg(test)]
mod tests {
    use super::BLOCKER_SLEEP_SECONDS;
    
    #[test]
    fn sleep_seconds_is_i32_max() {
        assert_eq!(BLOCKER_SLEEP_SECONDS, format!("{}", i32::MAX));
    }
}
```

Code after:
    command
        .stdin(Stdio::null())
        .stdout(Stdio::null())
        .stderr(Stdio::null());

    // SAFETY: `pre_exec` must be registered before spawn. The closure only
    // performs libc setup for the child process and returns an `io::Error`
    // when parent-death signal setup fails.
    unsafe {
        command.pre_exec(move || {
            if libc::prctl(libc::PR_SET_PDEATHSIG, libc::SIGTERM) == -1 {
                return Err(std::io::Error::last_os_error());
            }
            if libc::getppid() != parent_pid {
                libc::raise(libc::SIGTERM);
            }
            Ok(())
        });
    }

    command.spawn()
}

fn child_exited(error: &std::io::Error) -> bool {
    matches!(error.kind(), std::io::ErrorKind::InvalidInput)
}

#[cfg(test)]
mod tests {
    use super::BLOCKER_SLEEP_SECONDS;

    #[test]
    fn sleep_seconds_is_i32_max() {
        assert_eq!(BLOCKER_SLEEP_SECONDS, format!("{}", i32::MAX));
    }
}
Repository: openai/codex
File: codex-rs/core/src/skills/loader.rs (Rust)

Code before:
        )?;
    }

    let resolved_path = canonicalize_path(path).unwrap_or_else(|_| path.to_path_buf());

    Ok(SkillMetadata {
        name,
        description,
        short_description,
        interface,
        dependencies,
        policy,
        permission_profile,
        permissions,
        path_to_skills_md: resolved_path,
        scope,
    })
}

fn load_skill_metadata(
    skill_path: &Path,
) -> (
    Option<SkillInterface>,
    Option<SkillDependencies>,
    Option<SkillPolicy>,
    Option<PermissionProfile>,
    Option<Permissions>,
) {
    // Fail open: optional metadata should not block loading SKILL.md.
    let Some(skill_dir) = skill_path.parent() else {
        return (None, None, None, None, None);
    };
    let metadata_path = skill_dir
        .join(SKILLS_METADATA_DIR)
        .join(SKILLS_METADATA_FILENAME);
    if !metadata_path.exists() {
        return (None, None, None, None, None);
    }

    let contents = match fs::read_to_string(&metadata_path) {
        Ok(contents) => contents,
        Err(error) => {
            tracing::warn!(
                "ignoring {path}: failed to read {label}: {error}",
                path = metadata_path.display(),
                label = SKILLS_METADATA_FILENAME
            );
            return (None, None, None, None, None);
        }
    };


Review comment (security):
In a follow-up, we should figure out the story between `PermissionProfile` and `Permissions`.

Also, we should return a struct, this tuple is hard to work with...

Code after:
        interface,
        dependencies,
        policy,
        permission_profile,
        permissions,
    } = load_skill_metadata(path);

    validate_len(&name, MAX_NAME_LEN, "name")?;
    validate_len(&description, MAX_DESCRIPTION_LEN, "description")?;
    if let Some(short_description) = short_description.as_deref() {
        validate_len(
            short_description,
            MAX_SHORT_DESCRIPTION_LEN,
            "metadata.short-description",
        )?;
    }

    let resolved_path = canonicalize_path(path).unwrap_or_else(|_| path.to_path_buf());

    Ok(SkillMetadata {
        name,
        description,
        short_description,
        interface,
        dependencies,
        policy,
        permission_profile,
        permissions,
        path_to_skills_md: resolved_path,
        scope,
    })
}

fn load_skill_metadata(skill_path: &Path) -> LoadedSkillMetadata {
    // Fail open: optional metadata should not block loading SKILL.md.
    let Some(skill_dir) = skill_path.parent() else {
        return LoadedSkillMetadata::default();
    };
    let metadata_path = skill_dir
        .join(SKILLS_METADATA_DIR)
        .join(SKILLS_METADATA_FILENAME);
    if !metadata_path.exists() {
        return LoadedSkillMetadata::default();
    }

    let contents = match fs::read_to_string(&metadata_path) {
        Ok(contents) => contents,
        Err(error) => {
            tracing::warn!(
                "ignoring {path}: failed to read {label}: {error}",
                path = metadata_path.display(),
Repository: openai/openai-python
File: openai/embeddings_utils.py (Python)

Code before:
import textwrap as tr
from typing import List, Optional

import matplotlib.pyplot as plt
import plotly.express as px
from scipy import spatial
from sklearn.decomposition import PCA
from sklearn.manifold import TSNE
from sklearn.metrics import average_precision_score, precision_recall_curve
from tenacity import retry, stop_after_attempt, wait_random_exponential

import openai
from openai.datalib import numpy as np
from openai.datalib import pandas as pd


@retry(wait=wait_random_exponential(min=1, max=20), stop=stop_after_attempt(6))
def get_embedding(text: str, engine="text-similarity-davinci-001") -> List[float]:

    # replace newlines, which can negatively affect performance.
    text = text.replace("\n", " ")

    return openai.Embedding.create(input=[text], engine=engine)["data"][0]["embedding"]


@retry(wait=wait_random_exponential(min=1, max=20), stop=stop_after_attempt(6))
def get_embeddings(
    list_of_text: List[str], engine="text-similarity-babbage-001"
) -> List[List[float]]:
    assert len(list_of_text) <= 2048, "The batch size should not be larger than 2048."

    # replace newlines, which can negatively affect performance.
    list_of_text = [text.replace("\n", " ") for text in list_of_text]

    data = openai.Embedding.create(input=list_of_text, engine=engine).data
    data = sorted(data, key=lambda x: x["index"])  # maintain the same order as input.
    return [d["embedding"] for d in data]



Review comment (suggestion):
I wonder if we should call `assert_has_numpy` and `assert_has_pandas` in each function these modules are used so that it's very clear to users what to do to fix the issue (rather than getting a generic `'NoneType' object has no attribute` Python exception).

Code after:
import textwrap as tr
from typing import List, Optional

import matplotlib.pyplot as plt
import plotly.express as px
from scipy import spatial
from sklearn.decomposition import PCA
from sklearn.manifold import TSNE
from sklearn.metrics import average_precision_score, precision_recall_curve
from tenacity import retry, stop_after_attempt, wait_random_exponential

import openai
from openai.datalib import numpy as np
from openai.datalib import pandas as pd


@retry(wait=wait_random_exponential(min=1, max=20), stop=stop_after_attempt(6))
def get_embedding(text: str, engine="text-similarity-davinci-001") -> List[float]:

    # replace newlines, which can negatively affect performance.
    text = text.replace("\n", " ")

    return openai.Embedding.create(input=[text], engine=engine)["data"][0]["embedding"]


@retry(wait=wait_random_exponential(min=1, max=20), stop=stop_after_attempt(6))
async def aget_embedding(
    text: str, engine="text-similarity-davinci-001"
) -> List[float]:

    # replace newlines, which can negatively affect performance.
    text = text.replace("\n", " ")

    return (await openai.Embedding.acreate(input=[text], engine=engine))["data"][0][
        "embedding"
    ]


@retry(wait=wait_random_exponential(min=1, max=20), stop=stop_after_attempt(6))
Repository: openai/openai-python
File: src/openai/_utils/_logs.py (Python)

Code before:
        format="[%(asctime)s - %(name)s:%(lineno)d - %(levelname)s] %(message)s",
        datefmt="%Y-%m-%d %H:%M:%S",
    )


def setup_logging() -> None:
    env = os.environ.get("OPENAI_LOG")
    if env == "debug":
        _basic_config()
        logger.setLevel(logging.DEBUG)
        httpx_logger.setLevel(logging.DEBUG)
    elif env == "info":
        _basic_config()
        logger.setLevel(logging.INFO)
        httpx_logger.setLevel(logging.INFO)


class APIKeyFilter(logging.Filter):
    @override
    def filter(self, record: logging.LogRecord) -> bool:
        if isinstance(record.args, dict) and "headers" in record.args:
            if isinstance(record.args["headers"], dict):
                logged_headers: Dict[str, Any] = record.args["headers"]
                for header in logged_headers:
                    if header.lower() in ["api-key", "authorization"]:
                        logged_headers[header] = "<redacted>"
        return True


Review comment (security):
nit: we have an `is_dict()` header that would make this slightly cleaner imo
```suggestion     
        if is_dict(record.args) and "headers" in record.args and is_dict(record.args["headers"]):
            headers = record.args["headers"]
            for header in headers:
                if str(header).lower() in ["api-key", "authorization"]:
                    headers[header] = "<redacted>"

```
and `from ._utils import is_dict`.

also a minor micro optimisation but I think we should extract the list of headers, e.g.
```py
SENSITIVE_HEADERS = {"api-key", "authorization"}
```

Code after:

def _basic_config() -> None:
    # e.g. [2023-10-05 14:12:26 - openai._base_client:818 - DEBUG] HTTP Request: POST http://127.0.0.1:4010/foo/bar "200 OK"
    logging.basicConfig(
        format="[%(asctime)s - %(name)s:%(lineno)d - %(levelname)s] %(message)s",
        datefmt="%Y-%m-%d %H:%M:%S",
    )


def setup_logging() -> None:
    env = os.environ.get("OPENAI_LOG")
    if env == "debug":
        _basic_config()
        logger.setLevel(logging.DEBUG)
        httpx_logger.setLevel(logging.DEBUG)
    elif env == "info":
        _basic_config()
        logger.setLevel(logging.INFO)
        httpx_logger.setLevel(logging.INFO)


class SensitiveHeadersFilter(logging.Filter):
    @override
    def filter(self, record: logging.LogRecord) -> bool:
        if is_dict(record.args) and "headers" in record.args and is_dict(record.args["headers"]):
            headers = record.args["headers"] = {**record.args["headers"]}
            for header in headers:
                if str(header).lower() in SENSITIVE_HEADERS:
                    headers[header] = "<redacted>"
        return True
Repository: openai/openai-python
File: src/openai/_utils/_logs.py (Python)

Code before:

logger: logging.Logger = logging.getLogger("openai")
httpx_logger: logging.Logger = logging.getLogger("httpx")


def _basic_config() -> None:
    # e.g. [2023-10-05 14:12:26 - openai._base_client:818 - DEBUG] HTTP Request: POST http://127.0.0.1:4010/foo/bar "200 OK"
    logging.basicConfig(
        format="[%(asctime)s - %(name)s:%(lineno)d - %(levelname)s] %(message)s",
        datefmt="%Y-%m-%d %H:%M:%S",
    )


def setup_logging() -> None:
    env = os.environ.get("OPENAI_LOG")
    if env == "debug":
        _basic_config()
        logger.setLevel(logging.DEBUG)
        httpx_logger.setLevel(logging.DEBUG)
    elif env == "info":
        _basic_config()
        logger.setLevel(logging.INFO)
        httpx_logger.setLevel(logging.INFO)


class APIKeyFilter(logging.Filter):
    @override
    def filter(self, record: logging.LogRecord) -> bool:
        if isinstance(record.args, dict) and "headers" in record.args:
            if isinstance(record.args["headers"], dict):
                logged_headers: Dict[str, Any] = record.args["headers"]
                for header in logged_headers:
                    if header.lower() in ["api-key", "authorization"]:
                        logged_headers[header] = "<redacted>"
        return True


Review comment (security):
nit: thoughts on this naming?
```suggestion
class SensitiveHeadersFilter(logging.Filter):
```

Code after:
from ._utils import is_dict

logger: logging.Logger = logging.getLogger("openai")
httpx_logger: logging.Logger = logging.getLogger("httpx")


SENSITIVE_HEADERS = {"api-key", "authorization"}


def _basic_config() -> None:
    # e.g. [2023-10-05 14:12:26 - openai._base_client:818 - DEBUG] HTTP Request: POST http://127.0.0.1:4010/foo/bar "200 OK"
    logging.basicConfig(
        format="[%(asctime)s - %(name)s:%(lineno)d - %(levelname)s] %(message)s",
        datefmt="%Y-%m-%d %H:%M:%S",
    )


def setup_logging() -> None:
    env = os.environ.get("OPENAI_LOG")
    if env == "debug":
        _basic_config()
        logger.setLevel(logging.DEBUG)
        httpx_logger.setLevel(logging.DEBUG)
    elif env == "info":
        _basic_config()
        logger.setLevel(logging.INFO)
        httpx_logger.setLevel(logging.INFO)


class SensitiveHeadersFilter(logging.Filter):
    @override
    def filter(self, record: logging.LogRecord) -> bool:
        if is_dict(record.args) and "headers" in record.args and is_dict(record.args["headers"]):
            headers = record.args["headers"] = {**record.args["headers"]}
            for header in headers:
                if str(header).lower() in SENSITIVE_HEADERS:
                    headers[header] = "<redacted>"
        return True