Skip to content

feat(config): add --config and --dump-config for file-based configuration#759

Open
slin1237 wants to merge 2 commits intomainfrom
slin/config-file-support
Open

feat(config): add --config and --dump-config for file-based configuration#759
slin1237 wants to merge 2 commits intomainfrom
slin/config-file-support

Conversation

@slin1237
Copy link
Collaborator

@slin1237 slin1237 commented Mar 14, 2026

Summary

  • Add --config <path> flag to load configuration from YAML, JSON, or .env files instead of passing 130+ CLI arguments
  • Add --dump-config flag to serialize effective configuration as YAML for template generation
  • Wire config file support through Python bindings

What changed

File Change
model_gateway/Cargo.toml Add dotenvy dependency for .env file loading
model_gateway/src/config/loader.rs New — config file loader with format detection, YAML/JSON deserialization, .env loading, YAML serialization, and 6 unit tests
model_gateway/src/config/mod.rs Export loader module and public functions
model_gateway/src/config/types.rs Add #[serde(default)] at struct level on RouterConfig so partial configs work
model_gateway/src/main.rs Add --config/--dump-config flags, .env pre-loading before clap, build_router_config_from_file() via RouterConfigBuilder
bindings/python/src/smg/router_args.py Add config field and --config CLI argument
bindings/python/src/smg/router.py Pass config_path through to Rust Router
bindings/python/src/lib.rs Add config_path field, load config file or .env in start()

How it works

  • YAML/JSON: deserializes directly into RouterConfig via serde, then passes through RouterConfigBuilder for cert/MCP file loading
  • .env: loaded into process environment before clap parsing so existing env = "..." attributes pick up values automatically
  • --dump-config: serializes effective RouterConfig to YAML and exits
  • Precedence: Defaults → Config file → Env vars → CLI args

Example usage

# Generate a config template
cargo run -p smg -- --dump-config > gateway.yaml

# Run with config file
cargo run -p smg -- --config gateway.yaml

# Use .env file
cargo run -p smg -- --config .env

Test plan

  • 6 unit tests in loader.rs (YAML roundtrip, partial YAML defaults, JSON loading, format detection, missing file errors, .env detection)
  • All 858 existing tests pass (cargo test -p smg)
  • cargo clippy --all-targets --all-features -- -D warnings clean
  • Both smg and smg-python crates compile

Summary by CodeRabbit

  • New Features

    • Load configuration from YAML, JSON, or .env files; env files are applied early at startup.
    • New CLI option to specify a configuration file and a --dump-config option to print effective config as YAML.
    • Config file values are mergeable and can be overridden by explicit CLI arguments.
    • Python bindings: optional config path can be provided when constructing the Router.
  • Tests

    • Unit tests added for config loading and format detection.

… configuration

Add support for loading gateway configuration from YAML, JSON, or .env
files via a new --config flag, reducing the need to pass 130+ CLI args.

What changed:
- model_gateway/Cargo.toml: add dotenvy dependency for .env file loading
- model_gateway/src/config/loader.rs: new module with load_config_file(),
  load_env_file(), is_env_file(), dump_config_yaml() and format detection
  from file extension (.yaml/.yml/.json/.env)
- model_gateway/src/config/mod.rs: export loader module and public functions
- model_gateway/src/config/types.rs: add #[serde(default)] at struct level
  on RouterConfig so partial YAML/JSON files work (missing fields use defaults)
- model_gateway/src/main.rs: add --config and --dump-config to CliArgs,
  wire .env loading before clap parsing via pre_extract_config_path(),
  add build_router_config_from_file() that delegates to RouterConfigBuilder,
  auto-load .env from CWD via dotenvy::dotenv()
- bindings/python/src/smg/router_args.py: add config field and --config CLI arg
- bindings/python/src/smg/router.py: pass config_path through to Rust Router
- bindings/python/src/lib.rs: add config_path field, load config file or .env
  in start() before building RouterConfig

How it works:
- YAML/JSON files deserialize directly into RouterConfig via serde
- .env files are loaded into process env before clap parsing so existing
  env = "..." attributes pick up the values automatically
- --dump-config serializes effective RouterConfig to YAML for template generation
- Config precedence: Defaults -> Config file -> Env vars -> CLI args

Refs: config-file-support
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
@coderabbitai
Copy link

coderabbitai bot commented Mar 14, 2026

📝 Walkthrough

Walkthrough

Adds optional config file loading (YAML/JSON/.env) across Rust and Python bindings, a config loader module, CLI flags (--config, --dump-config), pre-arg environment loading, and wiring of an optional config_path from Python args into the Rust Router for conditional config ingestion.

Changes

Cohort / File(s) Summary
Python bindings
bindings/python/src/lib.rs, bindings/python/src/smg/router.py, bindings/python/src/smg/router_args.py
Added config_path: Option<String> to Rust Router and constructor; RouterArgs gains `config: str
Config loader module
model_gateway/src/config/loader.rs, model_gateway/src/config/mod.rs, model_gateway/src/config/types.rs
New loader supporting YAML/JSON/.env detection and loading, env-file population, YAML dumping, and struct-level serde(default) on RouterConfig.
CLI & main flow
model_gateway/src/main.rs
Pre-extracts config path from raw args, loads .env early, adds --config and --dump-config, merges file config with explicit CLI overrides, and integrates file-derived values into runtime/server config.
Dependency
model_gateway/Cargo.toml
Added dotenvy = "0.15" dependency for .env loading.

Sequence Diagram

sequenceDiagram
    participant User as CLI/User
    participant Pre as PreExtractor
    participant Env as EnvLoader
    participant Clap as Clap/ArgParser
    participant Loader as ConfigLoader
    participant CLIBuilder as CLI->RouterBuilder
    participant Router as Router/Server

    User->>Pre: raw args
    Pre->>Env: if path is .env -> load vars
    Pre->>Clap: filtered args
    Clap->>Loader: if --config provided -> load file
    Loader->>Loader: detect (yaml/json/.env) & parse
    Clap->>CLIBuilder: explicit CLI values
    Loader->>CLIBuilder: file config
    CLIBuilder->>CLIBuilder: merge (CLI overrides explicit)
    CLIBuilder->>Router: build RouterConfig
    Router->>Router: start with final RouterConfig
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

tests

Suggested reviewers

  • CatherineSue
  • key4ng

Poem

🐰 I nibble dots and YAML leaves,
I hop through JSON groves at ease,
A .env breeze whispers secrets near,
I stitch configs so startups cheer,
Hooray — the router's springing gears! 🎉

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding --config and --dump-config flags for file-based configuration loading, which is the primary focus of this changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch slin/config-file-support
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist
Copy link

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the router's configuration capabilities by introducing support for file-based configuration using YAML, JSON, and .env files. This change aims to simplify the setup process by centralizing configuration options, making it easier to manage complex deployments without relying solely on a large number of command-line arguments. Additionally, a new feature allows users to dump the current effective configuration, aiding in template generation and debugging. The new configuration system is also seamlessly integrated into the Python bindings, providing a consistent experience across different interfaces.

Highlights

  • File-based Configuration: Introduced a new --config <path> CLI flag that allows loading router configuration from YAML, JSON, or .env files, significantly reducing the need for numerous command-line arguments.
  • Configuration Dumping: Added a --dump-config CLI flag to serialize the effective router configuration into YAML format, which is useful for generating template configuration files.
  • Python Bindings Integration: Integrated the new configuration file loading mechanism into the Python bindings, allowing Python users to leverage --config for their router instances.
  • Configuration Precedence: Established a clear precedence order for configuration values: Defaults are overridden by Config file values, which are then overridden by Environment Variables, and finally by explicit CLI Arguments.
Changelog
  • bindings/python/src/lib.rs
    • Added config_path field to the Router struct to store the path to a configuration file.
    • Modified the start method to conditionally load configuration from the specified config_path, handling both .env and other config file types.
  • bindings/python/src/smg/router.py
    • Updated the from_args function to extract and pass the config_path argument to the Rust _Router constructor.
  • bindings/python/src/smg/router_args.py
    • Added a config field to the RouterArgs dataclass.
    • Introduced the --config CLI argument for Python users to specify a configuration file.
  • model_gateway/Cargo.toml
    • Added the dotenvy dependency to enable .env file loading.
  • model_gateway/src/config/loader.rs
    • Created a new module loader.rs to handle config file parsing, format detection (YAML, JSON, .env), and serialization of RouterConfig to YAML.
    • Implemented functions for loading YAML/JSON config files and .env files, along with utility functions like is_env_file.
  • model_gateway/src/config/mod.rs
    • Exported the new loader module and its public functions for configuration loading and dumping.
  • model_gateway/src/config/types.rs
    • Moved the #[serde(default)] attribute to the RouterConfig struct level to ensure partial configuration files can be deserialized correctly, inheriting defaults for unspecified fields.
  • model_gateway/src/main.rs
    • Imported RouterConfigBuilder for use in constructing configurations.
    • Added a pre_extract_config_path function to parse the --config argument early, allowing .env files to be loaded before clap processes environment variables.
    • Introduced --config and --dump-config CLI arguments to the main application.
    • Added a build_router_config_from_file method to CliArgs to construct a RouterConfig directly from a file.
    • Implemented logic in main to handle --dump-config by serializing the effective configuration to YAML.
    • Modified the main configuration loading flow to prioritize loading from a specified config file via --config, falling back to CLI arguments otherwise.
Activity
  • The pull request author, slin1237, provided a detailed summary of changes, including a file-by-file breakdown and examples.
  • The author outlined a comprehensive test plan, confirming unit tests, existing test suite pass, clippy cleanliness, and successful compilation of both smg and smg-python crates.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@github-actions github-actions bot added python-bindings Python bindings changes dependencies Dependency updates model-gateway Model gateway crate changes labels Mar 14, 2026
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a valuable feature for file-based configuration using --config and a --dump-config utility, supporting YAML, JSON, and .env formats. The implementation of the file loading and parsing is well-done. However, there are critical flaws in how the loaded configuration is merged with command-line or Python arguments. In both the main binary and the Python bindings, loading a YAML or JSON configuration file causes most other arguments to be ignored, violating the documented precedence where CLI/Python arguments should override file settings. Furthermore, the Python bindings incorrectly handle .env files, rendering them ineffective. The Python bindings also contain duplicated logic that should be refactored into a shared helper function. These issues will lead to unexpected and confusing behavior for users and need to be addressed.

Comment on lines +996 to +1003
fn build_router_config_from_file(&self, config_path: &str) -> ConfigResult<RouterConfig> {
let config = smg::config::load_config_file(config_path)?;

RouterConfigBuilder::from_config(config)
.maybe_server_cert_and_key(self.tls_cert_path.as_ref(), self.tls_key_path.as_ref())
.maybe_mcp_config_path(self.mcp_config_path.as_ref())
.build()
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This function incorrectly handles configuration precedence. It loads a configuration from a YAML/JSON file but then only applies a few specific overrides (tls_cert_path, mcp_config_path) from the command-line arguments. Most other CLI arguments are ignored, which violates the documented precedence order: Defaults → Config file → Env vars → CLI args. This means users cannot override most settings from the config file via the CLI, which can lead to unexpected behavior and confusion. The --dump-config feature is also affected by this, as it won't dump the true effective configuration.

Comment on lines +1025 to +1046
let router_config = if let Some(ref path) = self.config_path {
if config::is_env_file(path) {
config::load_env_file(path).map_err(|e| {
pyo3::exceptions::PyValueError::new_err(format!(
"Failed to load .env file: {e}"
))
})?;
self.to_router_config().map_err(|e| {
pyo3::exceptions::PyValueError::new_err(format!("Configuration error: {e}"))
})?
} else {
config::load_config_file(path).map_err(|e| {
pyo3::exceptions::PyValueError::new_err(format!(
"Failed to load config file: {e}"
))
})?
}
} else {
self.to_router_config().map_err(|e| {
pyo3::exceptions::PyValueError::new_err(format!("Configuration error: {e}"))
})?
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The configuration loading logic in the start method is flawed and does not respect the documented precedence, leading to incorrect behavior for Python users.

  • .env file handling: The .env file is loaded into the process environment, but the configuration is then built from the Python arguments passed during Router initialization. This completely ignores the environment variables from the file. .env files should be loaded in the Python layer before the Router object is created to be effective.
  • YAML/JSON file handling: The configuration is loaded from the file, but this discards all arguments passed from Python. Python arguments should override values from the configuration file.
  • Code Duplication: The call to self.to_router_config() and its error mapping is duplicated. This duplicated logic should be extracted into a shared helper function to improve maintainability and reduce redundancy.

These issues will cause significant confusion for users of the Python bindings.

References
  1. Extract duplicated logic into a shared helper function to improve maintainability and reduce redundancy.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c4371f32e9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +999 to +1002
RouterConfigBuilder::from_config(config)
.maybe_server_cert_and_key(self.tls_cert_path.as_ref(), self.tls_key_path.as_ref())
.maybe_mcp_config_path(self.mcp_config_path.as_ref())
.build()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Merge CLI/env overrides into file-loaded router config

When --config points to YAML/JSON, this code returns the deserialized file config and only reapplies TLS/MCP path handling, so explicit CLI/env values (for example --port, --policy, or other clap/env-backed fields) never override file values. Both startup and --dump-config route through this helper in main.rs, so the advertised precedence of config file < env < CLI is not actually honored.

Useful? React with 👍 / 👎.

Comment on lines +1036 to +1040
config::load_config_file(path).map_err(|e| {
pyo3::exceptions::PyValueError::new_err(format!(
"Failed to load config file: {e}"
))
})?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Apply Python Router args when config_path is YAML/JSON

In the non-.env branch, start() uses load_config_file(path) directly and skips self.to_router_config(), so constructor/CLI arguments are not merged into the loaded config; this breaks per-run overrides promised by the Python CLI help. It also creates inconsistent behavior because ServerConfig is still built later from self fields (e.g., host/port), while router-level settings come from the file.

Useful? React with 👍 / 👎.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@bindings/python/src/lib.rs`:
- Around line 1025-1046: The code currently constructs router_config differently
depending on config_path, causing .env loads to happen after Router construction
and config-file loads to skip builder-populated assets; fix by ensuring the
config file (either via config::load_env_file or config::load_config_file) is
applied to self before calling self.to_router_config() so that builder assets
(TLS/CA/MCP) and .env values are included; specifically, in the branch where
config::is_env_file(path) call config::load_env_file(path) and then always call
self.to_router_config(), and in the non-env branch load the config file into
self (or merge the parsed config into self) and then call
self.to_router_config() as well, and make server::startup(...) use the resulting
router_config rather than reading raw fields like self.host/self.port.

In `@bindings/python/src/smg/router_args.py`:
- Around line 232-245: The new CLI flag is always registered as "--config" which
ignores the add_cli_args(use_router_prefix=True) behavior and can collide with
parent parsers; change the registration in router_args.py so
config_group.add_argument uses the same prefixing logic as the rest of
add_cli_args (i.e., compute the flag name with the router prefix when
use_router_prefix is True instead of hardcoding "--config"), reusing the
existing prefix helper used elsewhere in add_cli_args and ensuring
from_cli_args() will still read the prefixed key.

In `@model_gateway/src/main.rs`:
- Around line 993-1003: build_router_config_from_file currently loads the config
file into RouterConfig but only applies TLS and MCP overrides, causing other
CLI/env values to neither be read from the file nor override it; update
build_router_config_from_file (and/or RouterConfigBuilder::from_config) so the
builder fully merges file values with CLI/env overrides: load the file into
RouterConfig, pass that into RouterConfigBuilder::from_config, then apply every
relevant CLI override (host/port/discovery/metrics/logging flags present on
CliArgs) via the builder’s methods before calling build(), and ensure
to_server_config() no longer re-reads values directly from CliArgs but uses the
merged RouterConfig produced by build_router_config_from_file (adjust
to_server_config or CliArgs usage accordingly).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 01b633db-e77c-4020-af75-fe417b582451

📥 Commits

Reviewing files that changed from the base of the PR and between f687596 and c4371f3.

📒 Files selected for processing (8)
  • bindings/python/src/lib.rs
  • bindings/python/src/smg/router.py
  • bindings/python/src/smg/router_args.py
  • model_gateway/Cargo.toml
  • model_gateway/src/config/loader.rs
  • model_gateway/src/config/mod.rs
  • model_gateway/src/config/types.rs
  • model_gateway/src/main.rs

…dence

Replace field-by-field override approach with generic JSON deep merge
that requires zero changes when adding new flat config fields.

What changed:
- model_gateway/src/main.rs: Add merge_configs(), deep_merge(), and
  cli_arg_to_config_path() functions that serialize both the file-loaded
  and CLI-built RouterConfig to JSON, then recursively merge based on
  which CLI args were explicitly set via command line or env vars.
  Enum/structural fields (mode, policy, connection_mode) are handled
  separately since they can't be deep-merged. CLI arg naming mismatches
  (cb_* vs circuit_breaker_*, health_* vs health_check_*) are resolved
  via an alias table.
- model_gateway/src/main.rs: Change Cli::parse_from to
  Cli::command().get_matches_from() + from_arg_matches() to retain
  ArgMatches for value_source() checks.
- model_gateway/src/main.rs: Add sync_server_fields() to sync merged
  config values back to CliArgs for fields that to_server_config()
  reads directly.
- bindings/python/src/lib.rs: Fix YAML/JSON config loading to pass
  through RouterConfigBuilder for cert/MCP/CA loading instead of
  returning raw config. Extract ServerConfig fields from RouterConfig
  so config file values are used instead of Python defaults.
- bindings/python/src/smg/router_args.py: Fix --config arg to use
  f"--{prefix}config" like all other args.

Why:
PR review feedback identified that the previous field-by-field override
approach was not scalable — adding a new field required updating the
override list. The JSON deep merge uses naming convention (field_name
in config JSON matches field-name as clap arg ID) to automatically
handle any flat field. Only structural mismatches (enums, prefix
aliases) need explicit handling.

Refs: #759
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 92f908336c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

)*
};
}
sync!(host, port, max_payload_size, request_timeout_secs);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Sync discovery toggle from merged config into CliArgs

build_router_config_from_file() merges file values into RouterConfig, but sync_server_fields() only copies host/port/log-related fields, so self.service_discovery remains the CLI default (false) when discovery is set only in YAML/JSON. to_server_config() then skips building ServiceDiscoveryConfig because it gates on self.service_discovery, so a config file with discovery enabled is silently ignored unless --service-discovery is also passed.

Useful? React with 👍 / 👎.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (2)
model_gateway/src/main.rs (1)

1176-1203: ⚠️ Potential issue | 🟠 Major

Server-facing fields are still only partially synced from the merged config.

to_server_config() below still builds service discovery, Prometheus, and request-id handling from self.service_discovery, self.prometheus_*, and self.request_id_headers. Because this sync only backfills host/port/payload/timeout/logging, file values under discovery, metrics, and request_id_headers remain inert when --config is used.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@model_gateway/src/main.rs` around lines 1176 - 1203, sync_server_fields only
backfills host/port/max_payload_size/request_timeout_secs/logging but misses
other server-facing fields used by to_server_config such as service_discovery,
prometheus_* (e.g., prometheus_enabled, prometheus_listen_addr,
prometheus_namespace or whatever prometheus fields exist), and
request_id_headers; update sync_server_fields to also check is_explicitly_set
for the corresponding arg names (convert underscores to hyphens like the macro
does) and copy config values into self (cloning as needed) for
service_discovery, all prometheus-related fields, and request_id_headers so that
to_server_config reads the merged values instead of only the CLI ones.
bindings/python/src/lib.rs (1)

1025-1062: ⚠️ Potential issue | 🔴 Critical

config_path still doesn’t produce a merged effective config in the Python startup path.

The .env branch loads after Router is constructed, and self.to_router_config() never re-reads env, so the file is a no-op for router settings. The YAML/JSON branch does the opposite and builds straight from file_config, which drops explicit Router(...) overrides and leaves downstream server settings like discovery/metrics tied to self instead of the loaded config.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@bindings/python/src/lib.rs` around lines 1025 - 1062, The config_path
handling currently treats the .env branch as a no-op for Router settings and the
YAML/JSON branch as ignoring explicit Python Router(...) overrides; fix by (1)
in the env-file branch call config::load_env_file(path) first and then build the
router config from self (i.e. call self.to_router_config()) so
to_router_config() sees the newly loaded env, and (2) in the file-config branch
after loading file_config via config::load_config_file(path) use
config::RouterConfigBuilder::from_config(file_config) but then apply the
explicit overrides from self (the same overrides applied via
maybe_server_cert_and_key, maybe_client_cert_and_key, add_ca_certificates,
maybe_mcp_config_path, etc.) before calling build() so the resulting
RouterConfig merges file settings with any Python-specified fields instead of
discarding them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@model_gateway/src/main.rs`:
- Around line 91-127: The code currently replaces the whole "policy" or
"mode"/"connection_mode" object whenever any subfield listed in POLICY_ARGS or
MODE_ARGS is present in explicit; change the conditions so the entire enum is
only swapped when the top-level key itself was explicitly provided. Concretely,
in the block that handles MODE_ARGS, replace the MODE_ARGS.iter().any(...) check
with a direct check like explicit.contains("mode") ||
explicit.contains("connection_mode") before inserting
override_map["mode"/"connection_mode"]; and in the POLICY_ARGS block replace the
POLICY_ARGS.iter().any(...) check with explicit.contains("policy") before
inserting override_map["policy"]. Use the same variables shown (base_map,
override_map, explicit, "policy", "mode", "connection_mode") so only top-level
keys trigger full replacement while subfield flags continue to be handled
without overwriting the entire enum.

---

Duplicate comments:
In `@bindings/python/src/lib.rs`:
- Around line 1025-1062: The config_path handling currently treats the .env
branch as a no-op for Router settings and the YAML/JSON branch as ignoring
explicit Python Router(...) overrides; fix by (1) in the env-file branch call
config::load_env_file(path) first and then build the router config from self
(i.e. call self.to_router_config()) so to_router_config() sees the newly loaded
env, and (2) in the file-config branch after loading file_config via
config::load_config_file(path) use
config::RouterConfigBuilder::from_config(file_config) but then apply the
explicit overrides from self (the same overrides applied via
maybe_server_cert_and_key, maybe_client_cert_and_key, add_ca_certificates,
maybe_mcp_config_path, etc.) before calling build() so the resulting
RouterConfig merges file settings with any Python-specified fields instead of
discarding them.

In `@model_gateway/src/main.rs`:
- Around line 1176-1203: sync_server_fields only backfills
host/port/max_payload_size/request_timeout_secs/logging but misses other
server-facing fields used by to_server_config such as service_discovery,
prometheus_* (e.g., prometheus_enabled, prometheus_listen_addr,
prometheus_namespace or whatever prometheus fields exist), and
request_id_headers; update sync_server_fields to also check is_explicitly_set
for the corresponding arg names (convert underscores to hyphens like the macro
does) and copy config values into self (cloning as needed) for
service_discovery, all prometheus-related fields, and request_id_headers so that
to_server_config reads the merged values instead of only the CLI ones.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: f92b729e-e36f-41ae-9f8a-1a9ab22cae78

📥 Commits

Reviewing files that changed from the base of the PR and between c4371f3 and 92f9083.

📒 Files selected for processing (3)
  • bindings/python/src/lib.rs
  • bindings/python/src/smg/router_args.py
  • model_gateway/src/main.rs

Comment on lines +56 to +89
fn merge_configs(
file_config: RouterConfig,
cli_config: RouterConfig,
matches: &clap::ArgMatches,
) -> ConfigResult<RouterConfig> {
// Collect explicitly-set CLI arg IDs, normalized to underscore format.
// Also expand aliases for args whose names don't match config field paths.
let explicit: HashSet<String> = matches
.ids()
.filter(|id| is_explicitly_set(matches, id.as_str()))
.flat_map(|id| {
let key = id.as_str().replace('-', "_");
let mut keys = vec![key.clone()];
if let Some(alias) = cli_arg_to_config_path(&key) {
keys.push(alias);
}
keys
})
.collect();

let mut base = serde_json::to_value(&file_config).map_err(|e| {
ConfigError::ValidationFailed {
reason: format!("Failed to serialize file config: {e}"),
}
})?;
let overrides = serde_json::to_value(&cli_config).map_err(|e| {
ConfigError::ValidationFailed {
reason: format!("Failed to serialize CLI config: {e}"),
}
})?;

// Skip enum/structural fields — they need whole-value replacement, not deep merge
let skip_keys: &[&str] = &["mode", "policy", "connection_mode"];
deep_merge(&mut base, &overrides, &explicit, "", skip_keys);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Nested sections still miss the advertised file < env < CLI precedence.

explicit contains arg IDs like prometheus_port, service_discovery_port, retry_max_retries, or selector, but deep_merge() checks paths like metrics_port, discovery_port, retry_config_max_retries, and recurses past explicit parent maps. So explicit overrides in metrics, discovery, retry_config, and selector maps still lose to the file config.

Also applies to: 134-158, 166-195

Comment on lines +91 to +127
// Handle enum/structural fields: replace entirely when any constituent CLI arg is set
if let (Some(base_map), Some(override_map)) = (base.as_object_mut(), overrides.as_object()) {
const MODE_ARGS: &[&str] = &[
"worker_urls",
"pd_disaggregation",
"decode",
"backend",
"prefill_policy",
"decode_policy",
];
if MODE_ARGS.iter().any(|k| explicit.contains(*k)) {
for key in ["mode", "connection_mode"] {
if let Some(val) = override_map.get(key) {
base_map.insert(key.to_string(), val.clone());
}
}
}

const POLICY_ARGS: &[&str] = &[
"policy",
"cache_threshold",
"balance_abs_threshold",
"balance_rel_threshold",
"eviction_interval",
"max_tree_size",
"block_size",
"max_idle_secs",
"assignment_mode",
"prefix_token_count",
"prefix_hash_load_factor",
];
if POLICY_ARGS.iter().any(|k| explicit.contains(*k)) {
if let Some(val) = override_map.get("policy") {
base_map.insert("policy".to_string(), val.clone());
}
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Don’t replace policy/mode from CLI defaults when only subfields were explicit.

POLICY_ARGS / MODE_ARGS currently swap the entire enum with override_map["policy"] / ["mode"], but those values are rebuilt from CliArgs defaults. A file policy: manual plus --max-idle-secs 30 flips back to default cache_aware, and a file PD config plus --decode ... without --pd-disaggregation gets replaced by Regular.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@model_gateway/src/main.rs` around lines 91 - 127, The code currently replaces
the whole "policy" or "mode"/"connection_mode" object whenever any subfield
listed in POLICY_ARGS or MODE_ARGS is present in explicit; change the conditions
so the entire enum is only swapped when the top-level key itself was explicitly
provided. Concretely, in the block that handles MODE_ARGS, replace the
MODE_ARGS.iter().any(...) check with a direct check like
explicit.contains("mode") || explicit.contains("connection_mode") before
inserting override_map["mode"/"connection_mode"]; and in the POLICY_ARGS block
replace the POLICY_ARGS.iter().any(...) check with explicit.contains("policy")
before inserting override_map["policy"]. Use the same variables shown (base_map,
override_map, explicit, "policy", "mode", "connection_mode") so only top-level
keys trigger full replacement while subfield flags continue to be handled
without overwriting the entire enum.

@mergify
Copy link

mergify bot commented Mar 15, 2026

Hi @slin1237, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch:

git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease

@mergify mergify bot added the needs-rebase PR has merge conflicts that need to be resolved label Mar 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Dependency updates model-gateway Model gateway crate changes needs-rebase PR has merge conflicts that need to be resolved python-bindings Python bindings changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant