ronantakizawa/codereview-bench huggingface dataset:
Repository: envoyproxy/envoy
File: api/envoy/extensions/filters/http/mcp/v3/mcp.proto (Protocol Buffers)

Code before:
syntax = "proto3";

package envoy.extensions.filters.http.mcp.v3;

import "udpa/annotations/status.proto";
import "validate/validate.proto";

option java_package = "io.envoyproxy.envoy.extensions.filters.http.mcp.v3";
option java_outer_classname = "McpProto";
option java_multiple_files = true;
option go_package = "github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/mcp/v3;mcpv3";
option (udpa.annotations.file_status).package_version_status = ACTIVE;

// [#protodoc-title: MCP]
// MCP filter :ref:`configuration overview <config_http_filters_mcp>`.
// [#extension: envoy.filters.http.mcp]

message Mcp {
  // TODO: Add configuration fields
}

// Per-route configuration for MCP filter
message McpPerRoute {
  // TODO: Add configuration fields
}


Review comment (suggestion):
If we keep this, consider calling it something like `McpOverride`.  The `PerRoute` suffix is somewhat misleading, since the override config can also be set per-VirtualHost or per-ClusterWeight.

Code after:
syntax = "proto3";

package envoy.extensions.filters.http.mcp.v3;

import "udpa/annotations/status.proto";

option java_package = "io.envoyproxy.envoy.extensions.filters.http.mcp.v3";
option java_outer_classname = "McpProto";
option java_multiple_files = true;
option go_package = "github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/mcp/v3;mcpv3";
option (udpa.annotations.file_status).package_version_status = ACTIVE;

// [#protodoc-title: MCP]
// MCP filter :ref:`configuration overview <config_http_filters_mcp>`.
// [#extension: envoy.filters.http.mcp]

message Mcp {
  // TODO: Add configuration fields
}

// McpOverride for MCP filter
message McpOverride {
  // TODO: Add configuration fields
}
Repository: envoyproxy/envoy
File: source/common/upstream/locality_pool.cc (C++)

Code before:
#include "source/common/upstream/locality_pool.h"

#include "envoy/config/core/v3/base.pb.h"

namespace Envoy {
namespace Upstream {

SINGLETON_MANAGER_REGISTRATION(const_locality_shared_pool);

ConstLocalitySharedPoolSharedPtr
LocalityPool::getConstLocalitySharedPool(Singleton::Manager& manager,
                                         Event::Dispatcher& dispatcher) {
  return manager.getTyped<SharedPool::ObjectSharedPool<const envoy::config::core::v3::Locality,
                                                       LocalityHash, LocalityEqualTo>>(
      SINGLETON_MANAGER_REGISTERED_NAME(const_locality_shared_pool), [&dispatcher] {
        return std::make_shared<SharedPool::ObjectSharedPool<
            const envoy::config::core::v3::Locality, LocalityHash, LocalityEqualTo>>(dispatcher);
      });
}

} // namespace Upstream
} // namespace Envoy


Review comment (suggestion):
I think we should pin the singleton by set the `pin` as true. So, we needn't to worry the lifetime of the object

Code after:
#include "source/common/upstream/locality_pool.h"

#include "envoy/config/core/v3/base.pb.h"

namespace Envoy {
namespace Upstream {

SINGLETON_MANAGER_REGISTRATION(const_locality_shared_pool);

ConstLocalitySharedPoolSharedPtr
LocalityPool::getConstLocalitySharedPool(Singleton::Manager& manager,
                                         Event::Dispatcher& dispatcher) {
  // Creating a pinned localities pool.
  return manager.getTyped<ConstLocalitySharedPool>(
      SINGLETON_MANAGER_REGISTERED_NAME(const_locality_shared_pool),
      [&dispatcher] { return std::make_shared<ConstLocalitySharedPool>(dispatcher); }, true);
}

} // namespace Upstream
} // namespace Envoy
Repository: envoyproxy/envoy
File: source/common/upstream/locality_pool.h (C/C++)

Code before:
#pragma once

#include <memory>

#include "envoy/config/core/v3/base.pb.h"
#include "envoy/event/dispatcher.h"
#include "envoy/singleton/manager.h"
#include "envoy/upstream/locality.h"

#include "source/common/shared_pool/shared_pool.h"

namespace Envoy {
namespace Upstream {

using ConstLocalitySharedPoolSharedPtr =
    std::shared_ptr<SharedPool::ObjectSharedPool<const envoy::config::core::v3::Locality,
                                                 LocalityHash, LocalityEqualTo>>;

class LocalityPool {
public:
  /**
   * Returns an ObjectSharedPool to store const Locality
   * @param manager used to create singleton
   * @param dispatcher the dispatcher object reference to the thread that created the
   * ObjectSharedPool
   */
  static ConstLocalitySharedPoolSharedPtr getConstLocalitySharedPool(Singleton::Manager& manager,
                                                                     Event::Dispatcher& dispatcher);
};

} // namespace Upstream
} // namespace Envoy


Review comment (suggestion):
If we create the type alias, could we use a alias to represent the `SharedPool::ObjectSharedPool<const envoy::config::core::v3::Locality, LocalityHash, LocalityEqualTo>`

Code after:
#pragma once

#include <memory>

#include "envoy/config/core/v3/base.pb.h"
#include "envoy/event/dispatcher.h"
#include "envoy/singleton/manager.h"
#include "envoy/upstream/locality.h"

#include "source/common/shared_pool/shared_pool.h"

namespace Envoy {
namespace Upstream {

using ConstLocalitySharedPool =
    SharedPool::ObjectSharedPool<const envoy::config::core::v3::Locality, LocalityHash,
                                 LocalityEqualTo>;
using ConstLocalitySharedPoolSharedPtr = std::shared_ptr<ConstLocalitySharedPool>;

class LocalityPool {
public:
  /**
   * Returns an ObjectSharedPool to store const Locality
   * @param manager used to create singleton
   * @param dispatcher the dispatcher object reference to the thread that created the
   * ObjectSharedPool
   */
  static ConstLocalitySharedPoolSharedPtr getConstLocalitySharedPool(Singleton::Manager& manager,
                                                                     Event::Dispatcher& dispatcher);
};

} // namespace Upstream
} // namespace Envoy
Repository: envoyproxy/envoy
File: source/common/quic/envoy_quic_server_session.h (C/C++)

Code before:
  quic::HttpDatagramSupport LocalHttpDatagramSupport() override { return http_datagram_support_; }

  // QuicFilterManagerConnectionImpl
  bool hasDataToWrite() override;
  // Used by base class to access quic connection after initialization.
  const quic::QuicConnection* quicConnection() const override;
  quic::QuicConnection* quicConnection() override;

private:
  void setUpRequestDecoder(EnvoyQuicServerStream& stream);

  std::unique_ptr<EnvoyQuicServerConnection> quic_connection_;
  // These callbacks are owned by network filters and quic session should out live
  // them.
  Http::ServerConnectionCallbacks* http_connection_callbacks_{nullptr};

  envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
      headers_with_underscores_action_;

  EnvoyQuicCryptoServerStreamFactoryInterface& crypto_server_stream_factory_;
  absl::optional<ConnectionMapPosition> position_;
  QuicConnectionStats& connection_stats_;
  quic::HttpDatagramSupport http_datagram_support_ = quic::HttpDatagramSupport::kNone;
  std::unique_ptr<quic::QuicConnectionDebugVisitor> debug_visitor_;
  // Load shed points for H3 GoAway
  Server::LoadShedPoint* should_send_go_away_and_close_on_dispatch_{nullptr};
  Server::LoadShedPoint* should_send_go_away_on_dispatch_{nullptr};
  bool h3_go_away_sent_ = false;
};

} // namespace Quic
} // namespace Envoy


Review comment (bug):
nit: since this is just a raw pointer,  i thinks `= nullptr` is clearer than `{nullptr}`.

https://abseil.io/tips/88

Code after:
  quic::HttpDatagramSupport LocalHttpDatagramSupport() override { return http_datagram_support_; }

  // QuicFilterManagerConnectionImpl
  bool hasDataToWrite() override;
  // Used by base class to access quic connection after initialization.
  const quic::QuicConnection* quicConnection() const override;
  quic::QuicConnection* quicConnection() override;

private:
  void setUpRequestDecoder(EnvoyQuicServerStream& stream);

  std::unique_ptr<EnvoyQuicServerConnection> quic_connection_;
  // These callbacks are owned by network filters and quic session should out live
  // them.
  Http::ServerConnectionCallbacks* http_connection_callbacks_{nullptr};

  envoy::config::core::v3::HttpProtocolOptions::HeadersWithUnderscoresAction
      headers_with_underscores_action_;

  EnvoyQuicCryptoServerStreamFactoryInterface& crypto_server_stream_factory_;
  absl::optional<ConnectionMapPosition> position_;
  QuicConnectionStats& connection_stats_;
  quic::HttpDatagramSupport http_datagram_support_ = quic::HttpDatagramSupport::kNone;
  std::unique_ptr<quic::QuicConnectionDebugVisitor> debug_visitor_;
  // Load shed points for H3 GoAway
  Server::LoadShedPoint* should_send_go_away_and_close_on_dispatch_ = nullptr;
  Server::LoadShedPoint* should_send_go_away_on_dispatch_ = nullptr;
  bool h3_go_away_sent_ = false;
};

} // namespace Quic
} // namespace Envoy
Repository: envoyproxy/envoy
File: api/envoy/extensions/http/ext_proc/processing_request_modifiers/mapped_attribute_builder/v3/mapped_attribute_builder.proto (Protocol Buffers)

Code before:
//          value {
//            string_value: "/profile"
//          }
//        }
//        fields {
//          key: "source.country"
//          value {
//            string_value: "US"
//          }
//        }
//      }
//    }
//
// .. note::
//   Processing request modifiers are currently in alpha.
//
message MappedAttributeBuilder {
  // A map of request attributes to set in the attributes struct.
  // The key is the attribute name, the value is the attribute value,
  // interpretable by CEL. This allows for the re-mapping of attributes, which is not supported
  // by the native attribute building logic.
  map<string, string> mapped_request_attributes = 1;

  // Same as above, but for response attributes. This may contain values like request.host. The
  // response nomenclature here just indicates that the attributes, whatever they may be, are sent
  // with a response headers, body, or trailers ext_proc call.
  map<string, string> mapped_response_attributes = 2;
}


Review comment (suggestion):
```suggestion
  // Similar to ``mapped_request_attributes``, but for response attributes. The
  // response nomenclature here just indicates that the attributes, whatever they may be, are sent
  // with a response headers, body, or trailers ext_proc call.
  // If a value contains a request key, e.g., ``request.host``, then <CLARIFY BEHAVIOR>.
```

Code after:
//          value {
//            string_value: "/profile"
//          }
//        }
//        fields {
//          key: "source.country"
//          value {
//            string_value: "US"
//          }
//        }
//      }
//    }
//
// .. note::
//   Processing request modifiers are currently in alpha.
//
message MappedAttributeBuilder {
  // A map of request attributes to set in the attributes struct.
  // The key is the attribute name, the value is the attribute value,
  // interpretable by CEL. This allows for the re-mapping of attributes, which is not supported
  // by the native attribute building logic.
  map<string, string> mapped_request_attributes = 1;

  // Similar to ``mapped_request_attributes``, but for response attributes. The
  // response nomenclature here just indicates that the attributes, whatever they may be, are sent
  // with a response headers, body, or trailers ext_proc call.
  // If a value contains a request key, e.g., ``request.host``, then the attribute would
  // just be sent along in the response. This is useful if a given ext_proc extension is only
  // enabled for response handling, e.g., ``RESPONSE_HEADERS`` but the backend wants to access request
  // metadata.
  map<string, string> mapped_response_attributes = 2;
}
Repository: envoyproxy/envoy
File: api/envoy/extensions/filters/http/mcp/v3/mcp.proto (Protocol Buffers)

Code before:
// [#protodoc-title: MCP]
// MCP filter :ref:`configuration overview <config_http_filters_mcp>`.
// [#extension: envoy.filters.http.mcp]

// This filter will inspect and get attributes from MCP traffic.
message Mcp {
  // Traffic handling mode for non-MCP traffic.
  enum TrafficMode {
    // Proxies the HTTP request and response without MCP spec check.
    // This is the default mode.
    PASS_THROUGH = 0;

    // Reject requests that are not following MCP spec.
    // Valid MCP requests are:
    // - POST requests with JSON-RPC 2.0 messages
    // - GET requests for SSE streams (with Accept: text/event-stream)
    REJECT_NO_MCP = 1;
  }

  // Configures how the filter handles non-MCP traffic.
  TrafficMode traffic_mode = 1 [(validate.rules).enum = {defined_only: true}];

  // When set to true, the filter will clear the route cache after setting dynamic metadata.
  // This allows the route to be re-selected based on the MCP metadata (e.g., method, params).
  // If not set, defaults to true.
  google.protobuf.BoolValue clear_route_cache = 2;
}

// McpOverride for MCP filter
message McpOverride {
  // Optional per-route traffic mode override
  Mcp.TrafficMode traffic_mode = 1 [(validate.rules).enum = {defined_only: true}];
}


Review comment (performance):
Can we make it default false? I think there are also other use cases that we don't need to clear the route cache, like route to the same homogenous MCP clusters.

Code after:
// [#extension: envoy.filters.http.mcp]

// This filter will inspect and get attributes from MCP traffic.
message Mcp {
  // Traffic handling mode for non-MCP traffic.
  enum TrafficMode {
    // Proxies the HTTP request and response without MCP spec check.
    // This is the default mode.
    PASS_THROUGH = 0;

    // Reject requests that are not following MCP spec.
    // Valid MCP requests are:
    // - POST requests with JSON-RPC 2.0 messages
    // - GET requests for SSE streams (with Accept: text/event-stream)
    REJECT_NO_MCP = 1;
  }

  // Configures how the filter handles non-MCP traffic.
  TrafficMode traffic_mode = 1 [(validate.rules).enum = {defined_only: true}];

  // When set to true, the filter will clear the route cache after setting dynamic metadata.
  // This allows the route to be re-selected based on the MCP metadata (e.g., method, params).
  // Defaults to false.
  bool clear_route_cache = 2;
}

// McpOverride for MCP filter
message McpOverride {
  // Optional per-route traffic mode override
  Mcp.TrafficMode traffic_mode = 1 [(validate.rules).enum = {defined_only: true}];
}
Repository: envoyproxy/envoy
File: .devcontainer/setup.sh (Shell)

Code before:
#!/usr/bin/env bash

echo "build --config=clang" >> user.bazelrc

# Ideally we want this line so bazel doesn't pollute things outside of the devcontainer, but some of
# API tooling (proto_sync) depends on symlink like bazel-bin.
# TODO(lizan): Fix API tooling and enable this again
#echo "build --symlink_prefix=/" >> ~/.bazelrc

[[ -n "${BUILD_DIR}" ]] && sudo chown -R "$(id -u):$(id -g)" "${BUILD_DIR}"

echo "Fetching LLVM toolchain..."
bazel fetch @llvm_toolchain//...
sudo ln -s "$(bazel info output_base)"/external/llvm_toolchain_llvm/bin/{clang-format,clangd} /usr/local/bin/
sudo ln -s "$(bazel info output_base)"/external/llvm_toolchain/bin/cc_wrapper.sh /usr/local/bin/


Review comment (suggestion):
I would suggest
```
sudo ln -s "$(bazel cquery --output=files @llvm_toolchain//:clangd)" /usr/local/bin/
sudo ln -s "$(bazel cquery --output=files @llvm_toolchain//:clang-format)" /usr/local/bin/
sudo ln -s "$(bazel cquery --output=files @llvm_toolchain//:bin/cc_wrapper.sh)" /usr/local/bin/
```
to minimize hardcoded bits of path (not sure if there's a better target for cc_wrapper.h).

Also possibly instead of the fetch, `bazel build @llvm_toolchain//:clangd` etc. would be more resilient to change, in case the target files at some point become [partially] generated rather than directly fetched.

Code after:
#!/usr/bin/env bash

echo "build --config=clang" >> user.bazelrc

# Ideally we want this line so bazel doesn't pollute things outside of the devcontainer, but some of
# API tooling (proto_sync) depends on symlink like bazel-bin.
# TODO(lizan): Fix API tooling and enable this again
#echo "build --symlink_prefix=/" >> ~/.bazelrc

[[ -n "${BUILD_DIR}" ]] && sudo chown -R "$(id -u):$(id -g)" "${BUILD_DIR}"

# Ensure that toolchain is downloaded.
./ci/do_ci.sh pre_refresh_compdb

sudo ln -sf /build/bazel_root/base-envoy-compdb/external/llvm_toolchain_llvm/bin/{clang-format,clangd} /usr/local/bin/
sudo ln -sf /build/bazel_root/base-envoy-compdb/external/llvm_toolchain/bin/cc_wrapper.sh /usr/local/bin/
Repository: envoyproxy/envoy
File: source/common/stream_info/stream_info_impl.h (C/C++)

Code before:
    MUTABLE_CONSTRUCT_ON_FIRST_USE(
        Network::ConnectionInfoProviderSharedPtr,
        std::make_shared<Network::ConnectionInfoSetterImpl>(nullptr, nullptr));
  }

  std::shared_ptr<UpstreamInfo> upstream_info_;
  const Network::ConnectionInfoProviderSharedPtr downstream_connection_info_provider_;
  const Http::RequestHeaderMap* request_headers_{};
  StreamIdProviderSharedPtr stream_id_provider_;
  absl::optional<DownstreamTiming> downstream_timing_;
  absl::optional<Upstream::ClusterInfoConstSharedPtr> upstream_cluster_info_;
  // Default construct the object because upstream stream is not constructed in some cases.
  BytesMeterSharedPtr upstream_bytes_meter_{std::make_shared<BytesMeter>()};
  BytesMeterSharedPtr downstream_bytes_meter_;
  std::string downstream_transport_failure_reason_;
  OptRef<const StreamInfo> parent_stream_info_;
  uint64_t bytes_received_{};
  uint64_t bytes_retransmitted_{};
  uint64_t packets_retransmitted_{};
  uint64_t bytes_sent_{};
  Tracing::Reason trace_reason_;
  bool health_check_request_{};
  bool should_scheme_match_upstream_{false};
  bool should_drain_connection_{false};
  bool is_shadow_{false};
  absl::optional<uint32_t> codec_stream_id_{0};
};

} // namespace StreamInfo
} // namespace Envoy


Review comment (suggestion):
I think this should be inited to {}? which is nullopt

Currently it is inited as 0.

Code after:
    MUTABLE_CONSTRUCT_ON_FIRST_USE(
        Network::ConnectionInfoProviderSharedPtr,
        std::make_shared<Network::ConnectionInfoSetterImpl>(nullptr, nullptr));
  }

  std::shared_ptr<UpstreamInfo> upstream_info_;
  const Network::ConnectionInfoProviderSharedPtr downstream_connection_info_provider_;
  const Http::RequestHeaderMap* request_headers_{};
  StreamIdProviderSharedPtr stream_id_provider_;
  absl::optional<DownstreamTiming> downstream_timing_;
  absl::optional<Upstream::ClusterInfoConstSharedPtr> upstream_cluster_info_;
  // Default construct the object because upstream stream is not constructed in some cases.
  BytesMeterSharedPtr upstream_bytes_meter_{std::make_shared<BytesMeter>()};
  BytesMeterSharedPtr downstream_bytes_meter_;
  std::string downstream_transport_failure_reason_;
  OptRef<const StreamInfo> parent_stream_info_;
  uint64_t bytes_received_{};
  uint64_t bytes_retransmitted_{};
  uint64_t packets_retransmitted_{};
  uint64_t bytes_sent_{};
  Tracing::Reason trace_reason_;
  bool health_check_request_{};
  bool should_scheme_match_upstream_{false};
  bool should_drain_connection_{false};
  bool is_shadow_{false};
  absl::optional<uint32_t> codec_stream_id_{};
};

} // namespace StreamInfo
} // namespace Envoy
Repository: envoyproxy/envoy
File: api/envoy/extensions/filters/http/router/v3/router.proto (Protocol Buffers)

Code before:
  // as not all failed gRPC requests charge HTTP status code metrics. See
  // :ref:`gRPC stats filter<config_http_filters_grpc_stats>` documentation
  // for more details.
  bool suppress_grpc_request_failure_code_stats = 7;

  // Optional HTTP filters for the upstream HTTP filter chain.
  //
  // .. note::
  //   Upstream HTTP filters are currently in alpha.
  //
  // These filters will be applied for all requests that pass through the router.
  // They will also be applied to shadowed requests.
  // Upstream HTTP filters cannot change route or cluster.
  // Upstream HTTP filters specified on the cluster will override these filters.
  //
  // If using upstream HTTP filters, please be aware that local errors sent by
  // upstream HTTP filters will not trigger retries, and local errors sent by
  // upstream HTTP filters will count as a final response if hedging is configured.
  // [#extension-category: envoy.filters.http.upstream]
  repeated network.http_connection_manager.v3.HttpFilter upstream_http_filters = 8;

  // If set to true, Envoy will reject ``CONNECT`` requests that send data before
  // receiving a ``200`` response from the upstream. This early data behavior
  // is common for latency reduction but can cause issues with some upstreams.
  // Defaults to false to allow early data and be compatible with common behavior.
  bool reject_connect_request_early_data = 10;
}


Review comment (suggestion):
Suggest using `google.protobuf.BoolValue` here, so that different data planes could have different defaults.

Code after:
  // as not all failed gRPC requests charge HTTP status code metrics. See
  // :ref:`gRPC stats filter<config_http_filters_grpc_stats>` documentation
  // for more details.
  bool suppress_grpc_request_failure_code_stats = 7;

  // Optional HTTP filters for the upstream HTTP filter chain.
  //
  // .. note::
  //   Upstream HTTP filters are currently in alpha.
  //
  // These filters will be applied for all requests that pass through the router.
  // They will also be applied to shadowed requests.
  // Upstream HTTP filters cannot change route or cluster.
  // Upstream HTTP filters specified on the cluster will override these filters.
  //
  // If using upstream HTTP filters, please be aware that local errors sent by
  // upstream HTTP filters will not trigger retries, and local errors sent by
  // upstream HTTP filters will count as a final response if hedging is configured.
  // [#extension-category: envoy.filters.http.upstream]
  repeated network.http_connection_manager.v3.HttpFilter upstream_http_filters = 8;

  // If set to true, Envoy will reject ``CONNECT`` requests that send data before
  // receiving a ``200`` response from the upstream. This early data behavior
  // is common for latency reduction but can cause issues with some upstreams.
  // Defaults to false to allow early data and be compatible with common behavior.
  google.protobuf.BoolValue reject_connect_request_early_data = 10;
}