ronantakizawa/codereview-bench huggingface dataset:
Repository: microsoft/RD-Agent
File: rdagent/log/ui/ds_summary.py (Python)

Code before:
"""Data Science Experiment Summary Dashboard.

A Streamlit dashboard that visualizes and analyzes machine learning experiment results. 
Key features:

* Recent 3-day average performance metrics across competitions
* Detailed experiment curves showing test and validation scores over time
* Comprehensive experiment statistics with filtering by competition difficulty
* Visualizations of experiment metrics with color-coded categories

The dashboard provides insights into experiment performance through customizable views
and interactive data exploration capabilities.

Key metrics explained:

* Successful Final Decision: Percentage of experiment loops where code executed correctly 
  and produced expected output, as determined by evaluation feedback
  
* Best Result: The highest achievement level reached by any experiment throughout the entire 
  process, ranging from lowest to highest: made_submission, valid_submission, above_median,
  bronze, silver, gold
  
* SOTA Exp: Version found by working backward from the last attempt to find the most recent 
  successful experiment
  
* SOTA Exp (_to_submit): Version selected by LLM from all successful experiments for 
  competition submission, considering not only scores but also generalization ability
  and overfitting risk, totally decided by LLM
"""

import re
from pathlib import Path

import pandas as pd
import plotly.express as px
import plotly.graph_objects as go
import streamlit as st
from streamlit import session_state as state


Review comment (suggestion):
You don't have to replicate the docstring.

You can just leave words like "please refer to `rdagent/log/ui/utils.py:get_summary_df` for more detailed documents about metrics." 

Code after:
"""
Please refer to rdagent/log/ui/utils.py:get_summary_df for more detailed documents about metrics
"""

import re
from pathlib import Path

import pandas as pd
import plotly.express as px
import plotly.graph_objects as go
import streamlit as st
from streamlit import session_state as state

from rdagent.log.ui.utils import (
    ALL,
    HIGH,
    LITE,
    MEDIUM,
    get_statistics_df,
    get_summary_df,
    percent_df,
)
from rdagent.scenarios.kaggle.kaggle_crawler import get_metric_direction


def days_summarize_win():
    lfs1 = [re.sub(r"log\.srv\d*", "log.srv", folder) for folder in state.log_folders]
    lfs2 = [re.sub(r"log\.srv\d*", "log.srv2", folder) for folder in state.log_folders]
    lfs3 = [re.sub(r"log\.srv\d*", "log.srv3", folder) for folder in state.log_folders]

    _, df1 = get_summary_df(lfs1)
    _, df2 = get_summary_df(lfs2)
    _, df3 = get_summary_df(lfs3)

    df = pd.concat([df1, df2, df3], axis=0)

    def mean_func(x: pd.DataFrame):
        numeric_cols = x.select_dtypes(include=["int", "float"]).mean()
        string_cols = x.select_dtypes(include=["object"]).agg(lambda col: ", ".join(col.fillna("none").astype(str)))
Repository: microsoft/RD-Agent
File: rdagent/oai/backend/litellm.py (Python)

Code before:
            )

        prompt_tokens = token_counter(model=model, messages=messages)
        completion_tokens = token_counter(model=model, text=content)
        logger.log_object(
            {
                "model": model,
                "prompt_tokens": prompt_tokens,
                "completion_tokens": completion_tokens,
                "cost": cost,
                "accumulated_cost": ACC_COST,
            },
            tag="token_cost",
        )
        return content, finish_reason

    def supports_response_schema(self) -> bool:
        """
        Check if the backend supports function calling
        """
        return supports_response_schema(model=LITELLM_SETTINGS.chat_model) and LITELLM_SETTINGS.enable_response_schema

    @property
    def chat_token_limit(self) -> int:
        try:
            model_info = get_model_info(LITELLM_SETTINGS.chat_model)
            max_input_tokens = model_info["max_input_tokens"] - model_info["max_output_tokens"]
            if model_info is None:
                return super().chat_token_limit
            return max_input_tokens
        except Exception as e:
            return super().chat_token_limit


Review comment (suggestion):
```suggestion
        """Suggest an input token limit, ensuring enough space in the context window for the maximum output tokens.  """
        try:
```

Code after:
            )

        prompt_tokens = token_counter(model=model, messages=messages)
        completion_tokens = token_counter(model=model, text=content)
        logger.log_object(
            {
                "model": model,
                "prompt_tokens": prompt_tokens,
                "completion_tokens": completion_tokens,
                "cost": cost,
                "accumulated_cost": ACC_COST,
            },
            tag="token_cost",
        )
        return content, finish_reason

    def supports_response_schema(self) -> bool:
        """
        Check if the backend supports function calling
        """
        return supports_response_schema(model=LITELLM_SETTINGS.chat_model) and LITELLM_SETTINGS.enable_response_schema

    @property
    def chat_token_limit(self) -> int:
        """Suggest an input token limit, ensuring enough space in the context window for the maximum output tokens."""
        try:
            model_info = get_model_info(LITELLM_SETTINGS.chat_model)
            if model_info is None:
                return super().chat_token_limit

            max_input = model_info.get("max_input_tokens")
            max_output = model_info.get("max_output_tokens")

            if max_input is None or max_output is None:
                return super().chat_token_limit

            max_input_tokens = max_input - max_output
            return max_input_tokens
        except Exception as e:
            return super().chat_token_limit
Repository: microsoft/onefuzz
File: src/agent/onefuzz-agent/src/main.rs (Rust)

Code before:
        bail!("onefuzz-agent child failed: {:?}", exit_status);
    }

    Ok(())
}

fn run(opt: RunOpt) -> Result<()> {
    if opt.redirect_output.is_some() {
        return redirect(opt);
    }

    if done::is_agent_done()? {
        debug!(
            "agent is done, remove lock ({}) to continue",
            done::done_path()?.display()
        );
        return Ok(());
    }

    // We can't send telemetry if this fails.
    let rt = tokio::runtime::Runtime::new()?;
    let config = rt.block_on(load_config(opt));

    if let Err(err) = &config {
        error!("error loading supervisor agent config: {:?}", err);
    }

    let config = config?;

    let result = rt.block_on(run_agent(config));

    if let Err(err) = &result {
        error!("error running supervisor agent: {:?}", err);
        if let Err(err) = failure::save_failure(err) {
            error!("unable to save failure log: {:?}", err);
        }
    }

    rt.block_on(telemetry::try_flush_and_close());

    result
}

async fn load_config(opt: RunOpt) -> Result<StaticConfig> {
    info!("loading supervisor agent config");

    let config = match &opt.config_path {
        Some(config_path) => StaticConfig::from_file(config_path)?,
        None => StaticConfig::from_env()?,
    };


Review comment (bug):
Maybe we can add something like the following (which we probably should've had already):
```suggestion
    // We can't send telemetry, because we couldn't get a telemetry key from the config.
    // Instead, log to an assumed-redirected stdout for the sake of debugging.
    if let Err(err) = &config {
```

Code after:
        bail!("onefuzz-agent child failed: {:?}", exit_status);
    }

    Ok(())
}

fn run(opt: RunOpt) -> Result<()> {
    if opt.redirect_output.is_some() {
        return redirect(opt);
    }

    if done::is_agent_done()? {
        debug!(
            "agent is done, remove lock ({}) to continue",
            done::done_path()?.display()
        );
        return Ok(());
    }

    // We can't send telemetry if this fails.
    let rt = tokio::runtime::Runtime::new()?;
    let config = rt.block_on(load_config(opt));

    // We can't send telemetry, because we couldn't get a telemetry key from the config.
    // Instead, log to an assumed-redirected stdout for the sake of debugging.
    if let Err(err) = &config {
        error!("error loading supervisor agent config: {:?}", err);
    }

    let config = config?;

    let result = rt.block_on(run_agent(config));

    if let Err(err) = &result {
        error!("error running supervisor agent: {:?}", err);
        if let Err(err) = failure::save_failure(err) {
            error!("unable to save failure log: {:?}", err);
        }
    }

    rt.block_on(telemetry::try_flush_and_close());

    result
}

async fn load_config(opt: RunOpt) -> Result<StaticConfig> {
    info!("loading supervisor agent config");

    let config = match &opt.config_path {
        Some(config_path) => StaticConfig::from_file(config_path)?,
        None => StaticConfig::from_env()?,
Repository: microsoft/onefuzz
File: src/ApiService/ApiService/OneFuzzTypes/Requests.cs (C#)

Code before:
);

public record ScalesetSearch(
    Guid? ScalesetId = null,
    List<ScalesetState>? State = null,
    bool IncludeAuth = false
) : BaseRequest;

public record ScalesetStop(
    Guid ScalesetId,
    bool Now
) : BaseRequest;

public record ScalesetUpdate(
    Guid ScalesetId,
    [property: Range(1, long.MaxValue)]
    long? Size
) : BaseRequest;

public record TaskGet(Guid TaskId) : BaseRequest;

public record TaskCreate(
   Guid JobId,
   List<Guid>? PrereqTasks,
   TaskDetails Task,
   [property: Required]
   TaskPool Pool,
   List<TaskContainers>? Containers = null,
   Dictionary<string, string>? Tags = null,
   List<TaskDebugFlag>? Debug = null,
   bool? Colocate = null
) : BaseRequest;

public record TaskSearch(
    Guid? JobId,
    Guid? TaskId,
    List<TaskState> State) : BaseRequest;

public record PoolSearch(
    Guid? PoolId = null,
    PoolName? Name = null,
    List<PoolState>? State = null
) : BaseRequest;

public record PoolStop(
    PoolName Name,
    bool Now
) : BaseRequest;

public record PoolCreate(
    PoolName Name,

Review comment (suggestion):
in ASP.NET Core they make `[Required]` the default for any non-null properties, we should try to do the same

Code after:
    [property: Range(1, long.MaxValue)] long ScaleInAmount,
    [property: Range(1, long.MaxValue)] long ScaleInCooldown
);

public record ScalesetSearch(
    Guid? ScalesetId = null,
    List<ScalesetState>? State = null,
    bool IncludeAuth = false
) : BaseRequest;

public record ScalesetStop(
    [property: Required] Guid ScalesetId,
    [property: Required] bool Now
) : BaseRequest;

public record ScalesetUpdate(
    [property: Required] Guid ScalesetId,
    [property: Range(1, long.MaxValue)]
    long? Size
) : BaseRequest;

public record TaskGet(
    [property: Required] Guid TaskId
) : BaseRequest;

public record TaskCreate(
   [property: Required] Guid JobId,
   List<Guid>? PrereqTasks,
   [property: Required] TaskDetails Task,
   [property: Required] TaskPool Pool,
   List<TaskContainers>? Containers = null,
   Dictionary<string, string>? Tags = null,
   List<TaskDebugFlag>? Debug = null,
   bool? Colocate = null
) : BaseRequest;

public record TaskSearch(
    Guid? JobId,
    Guid? TaskId,
    [property: Required] List<TaskState> State) : BaseRequest;

public record PoolSearch(
    Guid? PoolId = null,
    PoolName? Name = null,
    List<PoolState>? State = null
) : BaseRequest;

public record PoolStop(
    [property: Required] PoolName Name,
    [property: Required] bool Now
) : BaseRequest;
Repository: microsoft/onefuzz
File: src/ApiService/ApiService/OneFuzzTypes/Enums.cs (C#)

Code before:
    TargetMustUseInput
}


[Flags]
public enum ContainerPermission {
    Read = 1 << 0,
    Write = 1 << 1,
    List = 1 << 2,
    Delete = 1 << 3,
}


public enum Compare {
    Equal,
    AtLeast,
    AtMost
}
public enum AgentMode {
    Fuzz,
    Repro,
    Proxy
}

public enum NodeState {
    New,
    Init,
    Free,
    SettingUp,
    Rebooting,
    Ready,
    Busy,
    Done,
    Shutdown,
    Halt,
}

public static class NodeStateHelper {
    private static readonly IReadOnlySet<NodeState> _needsWork =
        new HashSet<NodeState>(new[] { NodeState.Done, NodeState.Shutdown, NodeState.Halt });

    private static readonly IReadOnlySet<NodeState> _readyForReset
        = new HashSet<NodeState>(new[] { NodeState.Done, NodeState.Shutdown, NodeState.Halt });

    private static readonly IReadOnlySet<NodeState> _canProcessNewWork =
        new HashSet<NodeState>(new[] { NodeState.Free });

    private static readonly IReadOnlySet<NodeState> _busy =
        new HashSet<NodeState>(new[] { NodeState.Busy });

    public static IReadOnlySet<NodeState> BusyStates => _busy;

Review comment (suggestion):
I don't think we need the new state. 
The implementation of `NodeOperations.SetState` does not match the python code which probably causing the issue.
- [in python](https://github.com/chkeita/onefuzz/blob/3347e7e67b3c51dddf47eafc9320cfbc7bd609ae/src/api-service/__app__/onefuzzlib/workers/nodes.py#L486) the state is saved even if the state is the same (we don't send the event in that case).
- [in C#](https://github.com/chkeita/onefuzz/blob/4662df3e397c6bb1406647515a47b4fa868c4f26/src/ApiService/ApiService/onefuzzlib/NodeOperations.cs#L450) we don't save the node if the state is the same. 






Code after:
    TargetMustUseInput
}


[Flags]
public enum ContainerPermission {
    Read = 1 << 0,
    Write = 1 << 1,
    List = 1 << 2,
    Delete = 1 << 3,
}


public enum Compare {
    Equal,
    AtLeast,
    AtMost
}
public enum AgentMode {
    Fuzz,
    Repro,
    Proxy
}

public enum NodeState {
    Init,
    Free,
    SettingUp,
    Rebooting,
    Ready,
    Busy,
    Done,
    Shutdown,
    Halt,
}

public static class NodeStateHelper {
    private static readonly IReadOnlySet<NodeState> _needsWork =
        new HashSet<NodeState>(new[] { NodeState.Done, NodeState.Shutdown, NodeState.Halt });

    private static readonly IReadOnlySet<NodeState> _readyForReset
        = new HashSet<NodeState>(new[] { NodeState.Done, NodeState.Shutdown, NodeState.Halt });

    private static readonly IReadOnlySet<NodeState> _canProcessNewWork =
        new HashSet<NodeState>(new[] { NodeState.Free });

    private static readonly IReadOnlySet<NodeState> _busy =
        new HashSet<NodeState>(new[] { NodeState.Busy });

    public static IReadOnlySet<NodeState> BusyStates => _busy;
Repository: microsoft/onefuzz
File: src/ApiService/ApiService/onefuzzlib/Utils.cs (C#)

Code before:
    public static T EnsureNotNull<T>(this T? thisObject, string message) {
        if (thisObject == null) {
            throw new ArgumentException(message);
        }

        return thisObject;
    }

    // Explicitly discards the result value.
    // In general we should not do this; eventually all call-sites should
    // be updated.
    public static Async.Task IgnoreResult<T>(this Async.Task<T> task)
        => task;
}

public static class IAsyncEnumerableExtension {
    public static async IAsyncEnumerable<List<TSource>> Chunk<TSource>(this IAsyncEnumerable<TSource> source, int size) {

        var enumerator = source.GetAsyncEnumerator();
        List<TSource> result = new List<TSource>(size);
        while (await enumerator.MoveNextAsync()) {
            if (result.Count == size) {
                yield return result;
                result = new List<TSource>(size);
            }
            result.Add(enumerator.Current);
        }

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


Review comment (suggestion):
Do this in the other order so it `yield`s the result as soon as it is ready?

```suggestion
            result.Add(enumerator.Current);
            
            if (result.Count == size) {
                yield return result;
                result = new List<TSource>(size);
            }
```

Code after:
    public static T EnsureNotNull<T>(this T? thisObject, string message) {
        if (thisObject == null) {
            throw new ArgumentException(message);
        }

        return thisObject;
    }

    // Explicitly discards the result value.
    // In general we should not do this; eventually all call-sites should
    // be updated.
    public static Async.Task IgnoreResult<T>(this Async.Task<T> task)
        => task;
}

public static class IAsyncEnumerableExtension {
    public static async IAsyncEnumerable<List<TSource>> Chunk<TSource>(this IAsyncEnumerable<TSource> source, int size) {

        if (size <= 0) {
            throw new ArgumentException("size must be greater than 0");
        }

        var enumerator = source.GetAsyncEnumerator();
        List<TSource> result = new List<TSource>(size);
        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;
        }
    }
}
Repository: microsoft/onefuzz
File: src/ApiService/ApiService/OneFuzzTypes/Events.cs (C#)

Code before:
    TaskConfig? TaskConfig
) : BaseEvent();


[EventType(EventType.RegressionReported)]
public record EventRegressionReported(
    RegressionReport RegressionReport,
    Container Container,
    [property: JsonPropertyName("filename")] String FileName,
    TaskConfig? TaskConfig
) : BaseEvent();


[EventType(EventType.FileAdded)]
public record EventFileAdded(
    Container Container,
    [property: JsonPropertyName("filename")] String FileName
) : BaseEvent();


[EventType(EventType.InstanceConfigUpdated)]
public record EventInstanceConfigUpdated(
    InstanceConfig Config
) : BaseEvent();

[EventType(EventType.NotifcationFailed)]
public record EventNotifcationFailed(
    Guid NotificationId,
    Guid JobId,
    Error? Error
) : BaseEvent();

public record EventMessage(
    Guid EventId,
    EventType EventType,
    [property: TypeDiscrimnatorAttribute("EventType", typeof(EventTypeProvider))]
    [property: JsonConverter(typeof(BaseEventConverter))]
    BaseEvent Event,
    Guid InstanceId,
    String InstanceName
);

public class BaseEventConverter : JsonConverter<BaseEvent> {
    public override BaseEvent? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) {
        return null;
    }

    public override void Write(Utf8JsonWriter writer, BaseEvent value, JsonSerializerOptions options) {
        var eventType = value.GetType();
        JsonSerializer.Serialize(writer, value, eventType, options);
    }

Review comment (bug):
```suggestion
[EventType(EventType.NotificationFailed)]
```

Code after:
    TaskConfig? TaskConfig
) : BaseEvent(), ITruncatable<BaseEvent> {
    public BaseEvent Truncate(int maxLength) {
        return this with {
            Report = Report.Truncate(maxLength)
        };
    }
}

[EventType(EventType.RegressionReported)]
public record EventRegressionReported(
    RegressionReport RegressionReport,
    Container Container,
    [property: JsonPropertyName("filename")] String FileName,
    TaskConfig? TaskConfig
) : BaseEvent(), ITruncatable<BaseEvent> {
    public BaseEvent Truncate(int maxLength) {
        return this with {
            RegressionReport = RegressionReport.Truncate(maxLength)
        };
    }
}

[EventType(EventType.FileAdded)]
public record EventFileAdded(
    Container Container,
    [property: JsonPropertyName("filename")] String FileName
) : BaseEvent();


[EventType(EventType.InstanceConfigUpdated)]
public record EventInstanceConfigUpdated(
    InstanceConfig Config
) : BaseEvent();

[EventType(EventType.NotificationFailed)]
public record EventNotificationFailed(
    Guid NotificationId,
    Guid JobId,
    Error? Error
) : BaseEvent();

public record EventMessage(
    Guid EventId,
    EventType EventType,
    [property: TypeDiscrimnatorAttribute("EventType", typeof(EventTypeProvider))]
    [property: JsonConverter(typeof(BaseEventConverter))]
    BaseEvent Event,
    Guid InstanceId,
    String InstanceName
);
Repository: microsoft/onefuzz
File: src/ApiService/ApiService/Functions/NodeAddSshKey.cs (C#)

Code before:
                "NodeAddSshKey");
        }

        var node = await _context.NodeOperations.GetByMachineId(request.OkV.MachineId);

        if (node == null) {
            return await _context.RequestHandling.NotOk(
                req,
                new Error(ErrorCode.UNABLE_TO_FIND, new[] { "unable to find node" }),
                $"{request.OkV.MachineId}");
        }

        var result = await _context.NodeOperations.AddSshPublicKey(node, request.OkV.PublicKey);
        if (!result.IsOk) {
            return await _context.RequestHandling.NotOk(req, result.ErrorV, "NodeAddSshKey");
        }

        var response = req.CreateResponse(HttpStatusCode.OK);
        await response.WriteAsJsonAsync(new BoolResult(true));
        return response;


    }

    [Function("NodeAddSshKey")]
    public Async.Task<HttpResponseData> Run([HttpTrigger(AuthorizationLevel.Anonymous, "POST", Route = "node_add_ssh_key")] HttpRequestData req) {
        return _auth.CallIfUser(req, r => r.Method switch {
            "POST" => Post(r),
            _ => throw new InvalidOperationException("Unsupported HTTP method"),
        });
    }

}


Review comment (security):
Keep this one, we already had it explicitly specified

```suggestion
    public Async.Task<HttpResponseData> Run([HttpTrigger(AuthorizationLevel.Anonymous, "POST", Route = "node/add_ssh_key")] HttpRequestData req) {
```

Code after:
                "NodeAddSshKey");
        }

        var node = await _context.NodeOperations.GetByMachineId(request.OkV.MachineId);

        if (node == null) {
            return await _context.RequestHandling.NotOk(
                req,
                new Error(ErrorCode.UNABLE_TO_FIND, new[] { "unable to find node" }),
                $"{request.OkV.MachineId}");
        }

        var result = await _context.NodeOperations.AddSshPublicKey(node, request.OkV.PublicKey);
        if (!result.IsOk) {
            return await _context.RequestHandling.NotOk(req, result.ErrorV, "NodeAddSshKey");
        }

        var response = req.CreateResponse(HttpStatusCode.OK);
        await response.WriteAsJsonAsync(new BoolResult(true));
        return response;


    }

    [Function("NodeAddSshKey")]
    public Async.Task<HttpResponseData> Run([HttpTrigger(AuthorizationLevel.Anonymous, "POST", Route = "node/add_ssh_key")] HttpRequestData req) {
        return _auth.CallIfUser(req, r => r.Method switch {
            "POST" => Post(r),
            _ => throw new InvalidOperationException("Unsupported HTTP method"),
        });
    }

}