feat(cli): align config validation with mihomo behavior#1331
Open
Tunglies wants to merge 8 commits intoWatfaq:masterfrom
Open
feat(cli): align config validation with mihomo behavior#1331Tunglies wants to merge 8 commits intoWatfaq:masterfrom
Tunglies wants to merge 8 commits intoWatfaq:masterfrom
Conversation
- accept mihomo-style config sources in test mode, including -config, -f -, and CLASH_* environment inputs - print validation errors and summaries on stdout with mihomo-compatible exit codes - create the mihomo default config for missing file-mode configs and cover the CLI behavior with integration tests
- allow proxy providers to omit health-check and apply mihomo-compatible defaults - default enabled health checks to interval 300 and lazy=true - return provider parse errors instead of panicking during config validation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds multi-source config loading to the CLI (file, stdin, base64), centralizes config resolution/error reporting, makes rule/provider parsing error-propagating, extends health-check config with effective-value helpers, and adds integration tests for config input behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant ConfigInput
participant FS as Filesystem
participant Decoder as Base64Decoder
participant Parser
participant App
CLI->>ConfigInput: build from args/env
ConfigInput->>ConfigInput: resolve() -> selects source
alt source is file
ConfigInput->>FS: check/ensure file (create default if missing)
FS-->>ConfigInput: file bytes
ConfigInput->>Parser: try_parse(file bytes)
else source is base64/string
ConfigInput->>Decoder: decode base64
Decoder-->>ConfigInput: decoded bytes
ConfigInput->>Parser: try_parse(decoded bytes)
end
Parser-->>ConfigInput: parse result or error
ConfigInput->>App: return parsed config or error
App->>CLI: print_cli_log(success|failure)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
clash-bin/src/main.rs (1)
332-338: Lossy UTF-8 conversion may silently corrupt config content.
String::from_utf8_lossyreplaces invalid UTF-8 sequences with the replacement character (�), which could silently corrupt configuration values (e.g., passwords or keys with non-UTF8 bytes). Consider usingString::from_utf8and returning a clear error on invalid UTF-8.♻️ Proposed fix
fn decode_config(config: &str) -> anyhow::Result<ConfigInput> { let content = STANDARD .decode(config) .map_err(|err| anyhow!("decode config: {err}"))?; - let content = String::from_utf8_lossy(&content).into_owned(); + let content = String::from_utf8(content) + .map_err(|err| anyhow!("decode config: invalid UTF-8: {err}"))?; Ok(ConfigInput::Bytes { content }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clash-bin/src/main.rs` around lines 332 - 338, The decode_config function currently uses String::from_utf8_lossy which can silently replace invalid UTF-8; change it to use String::from_utf8 and propagate a clear error when the decoded bytes are invalid UTF-8. Specifically, in decode_config where you build content for ConfigInput::Bytes, replace the lossy conversion with String::from_utf8(content_bytes) and map_err to an anyhow error (e.g., "invalid utf-8 in decoded config") so callers get a clear failure instead of corrupted config; keep the STANDARD.decode(...) error handling as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clash-bin/src/main.rs`:
- Around line 388-402: The format_description! invocation in print_mihomo_log
currently contains a line break inside the format string which breaks rustfmt;
replace it with a single-line format description (i.e., inline the entire
"[year]-[month]-[day]T[hour]:[minute]:[second].[subsecond digits:9][offset_hour
sign:mandatory]:[offset_minute]" string) used by format_description!, so
OffsetDateTime::now_local().format(...) receives a single-line literal; update
the format_description! call in print_mihomo_log accordingly to remove the
newline.
---
Nitpick comments:
In `@clash-bin/src/main.rs`:
- Around line 332-338: The decode_config function currently uses
String::from_utf8_lossy which can silently replace invalid UTF-8; change it to
use String::from_utf8 and propagate a clear error when the decoded bytes are
invalid UTF-8. Specifically, in decode_config where you build content for
ConfigInput::Bytes, replace the lossy conversion with
String::from_utf8(content_bytes) and map_err to an anyhow error (e.g., "invalid
utf-8 in decoded config") so callers get a clear failure instead of corrupted
config; keep the STANDARD.decode(...) error handling as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aa0145b2-a24c-4047-993b-1afc685fabb0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
clash-bin/Cargo.tomlclash-bin/src/main.rsclash-bin/tests/mihomo_config_test.rsclash-lib/src/app/outbound/manager.rsclash-lib/src/config/internal/convert/mod.rsclash-lib/src/config/internal/convert/rule_provider.rsclash-lib/src/config/internal/proxy.rs
Comment on lines
+388
to
+402
| fn print_mihomo_log(level: &str, msg: &str) { | ||
| let format = format_description!( | ||
| "[year]-[month]-[day]T[hour]:[minute]:[second].[subsecond digits:9][offset_hour sign:mandatory]:[offset_minute]" | ||
| ); | ||
| let timestamp = OffsetDateTime::now_local() | ||
| .unwrap_or_else(|_| OffsetDateTime::now_utc()) | ||
| .format(format) | ||
| .unwrap_or_else(|_| "1970-01-01T00:00:00.000000000+00:00".to_string()); | ||
| println!( | ||
| "time=\"{}\" level={} msg=\"{}\"", | ||
| timestamp, | ||
| level, | ||
| escape_logrus_text(msg) | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
CI formatting failure: line break in format_description! macro.
The pipeline failure indicates cargo fmt --check failed due to the format description string being broken across lines. The nightly rustfmt expects this to be on a single line.
🔧 Proposed fix
fn print_mihomo_log(level: &str, msg: &str) {
- let format = format_description!(
- "[year]-[month]-[day]T[hour]:[minute]:[second].[subsecond digits:9][offset_hour sign:mandatory]:[offset_minute]"
- );
+ let format = format_description!("[year]-[month]-[day]T[hour]:[minute]:[second].[subsecond digits:9][offset_hour sign:mandatory]:[offset_minute]");
let timestamp = OffsetDateTime::now_local()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn print_mihomo_log(level: &str, msg: &str) { | |
| let format = format_description!( | |
| "[year]-[month]-[day]T[hour]:[minute]:[second].[subsecond digits:9][offset_hour sign:mandatory]:[offset_minute]" | |
| ); | |
| let timestamp = OffsetDateTime::now_local() | |
| .unwrap_or_else(|_| OffsetDateTime::now_utc()) | |
| .format(format) | |
| .unwrap_or_else(|_| "1970-01-01T00:00:00.000000000+00:00".to_string()); | |
| println!( | |
| "time=\"{}\" level={} msg=\"{}\"", | |
| timestamp, | |
| level, | |
| escape_logrus_text(msg) | |
| ); | |
| } | |
| fn print_mihomo_log(level: &str, msg: &str) { | |
| let format = format_description!("[year]-[month]-[day]T[hour]:[minute]:[second].[subsecond digits:9][offset_hour sign:mandatory]:[offset_minute]"); | |
| let timestamp = OffsetDateTime::now_local() | |
| .unwrap_or_else(|_| OffsetDateTime::now_utc()) | |
| .format(format) | |
| .unwrap_or_else(|_| "1970-01-01T00:00:00.000000000+00:00".to_string()); | |
| println!( | |
| "time=\"{}\" level={} msg=\"{}\"", | |
| timestamp, | |
| level, | |
| escape_logrus_text(msg) | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clash-bin/src/main.rs` around lines 388 - 402, The format_description!
invocation in print_mihomo_log currently contains a line break inside the format
string which breaks rustfmt; replace it with a single-line format description
(i.e., inline the entire
"[year]-[month]-[day]T[hour]:[minute]:[second].[subsecond digits:9][offset_hour
sign:mandatory]:[offset_minute]" string) used by format_description!, so
OffsetDateTime::now_local().format(...) receives a single-line literal; update
the format_description! call in print_mihomo_log accordingly to remove the
newline.
ibigbug
reviewed
Apr 25, 2026
| pub name: String, | ||
| pub path: String, | ||
| pub interval: Option<u64>, | ||
| #[serde(default = "default_provider_health_check")] |
Member
There was a problem hiding this comment.
would impl Default for HealthCheck simplify it here a bit?
| pub url: String, | ||
| #[serde(default)] | ||
| pub interval: u64, | ||
| #[serde(default = "default_provider_health_check_lazy")] |
Member
There was a problem hiding this comment.
i think there's a default_bool_true in clash-lib can you use that instead so. you don't need to do Option here
| let config_input = match ConfigInput::resolve(&cli) { | ||
| Ok(config_input) => config_input, | ||
| Err(err) => { | ||
| print_mihomo_log("fatal", &err.to_string()); |
Member
There was a problem hiding this comment.
it looks like print_mihomo_log is default behavior now with no other option, maybe you can remove the "mihomo" from the function names everywhere, if that simplifies code and logic in any way.
or otherwise you can check the cli.compatiblity flag to enable regarding mihomo behaviors but i think that might be more involved
| } | ||
| } | ||
|
|
||
| fn preprocess_args(args: impl IntoIterator<Item = String>) -> Vec<String> { |
Member
There was a problem hiding this comment.
can you add a test for this so that
$ clash-rs -d DIR -f test.yml
$ clash-rs -d DIR # no -f so just find config.yaml under DIR
$ clash-rs -f test.yaml # find test.yaml in current dir
$ clash-rs # find config.yaml under current dir
all work?
… for HealthCheck Co-authored-by: Copilot <copilot@github.com>
Contributor
Author
|
It works > pwd
/Users/user/workspace/clash-rs
> cargo run -p clash-rs -- -d /Users/user/Library/Application\ Support/io.github.clash-verge-rev.clash-verge-rev -f clash-verge.yaml
Finished [`dev` profile [unoptimized + debuginfo]](https://doc.rust-lang.org/cargo/reference/profiles.html#default-profiles) target(s) in 0.44s
Running `target/debug/clash-rs -d '/Users/tunglies/Library/Application Support/io.github.clash-verge-rev.clash-verge-rev' -f clash-verge.yaml`
Compatibility mode enabled. This may cause some issues, but it is recommended to enable this if you are using clash verge.
26-04-25 15:11:51:132242 DEBUG ThreadId(01) clash_lib: clash-lib/src/lib.rs:366: tun enabled, initializing default outbound interface
26-04-25 15:11:51:139308 DEBUG ThreadId(01) clash_lib: clash-lib/src/lib.rs:372: initializing cache store |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
clash-bin/tests/config_input_test.rs (1)
53-295: Add regression coverage for the remaining resolver branches.The new suite never exercises
CLASH_HOME_DIR,CLASH_CONFIG_FILE, or the precedence between env-provided config and explicit CLI flags. Those are distinct branches inConfigInput::resolve(), and they are where path-resolution regressions will hide.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clash-bin/tests/config_input_test.rs` around lines 53 - 295, Add tests exercising ConfigInput::resolve() branches for CLASH_HOME_DIR and CLASH_CONFIG_FILE and their precedence: create new tests that spawn the binary via clash_cmd() while setting child env vars (use Command.env) for CLASH_HOME_DIR pointing to a temp dir (and verify a default config is created at that home path) and CLASH_CONFIG_FILE pointing to an explicit file path (and verify that file is read); additionally add a test where both an env var (CLASH_CONFIG_FILE or CLASH_HOME_DIR) and an explicit CLI flag (-f / -config) are provided and assert the CLI flag wins (explicit path/test output), referencing clash_cmd(), run_with_stdin() if needed, and ensuring assertions mirror existing patterns (status, stdout, stderr) so the resolver branches in ConfigInput::resolve() are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clash-bin/src/main.rs`:
- Around line 217-243: The resolve() logic currently checks CLASH_CONFIG_STRING
before honoring the CLI file flag, so update the precedence to prefer CLI flags:
keep checking cli.config_string first (decode_config), then if cli.config is
Some call resolve_config_path, then only afterwards check the environment
CLASH_CONFIG_STRING (decode_config) and CLASH_CONFIG_FILE (resolve_config_file);
ensure you reference and preserve calls to decode_config, resolve_config_path,
and resolve_config_file and maintain the directory resolution logic used for
resolve_config_path.
- Around line 241-243: The CLASH_CONFIG_FILE branch calls
resolve_config_file(&PathBuf::from(file)) which resolves relative paths against
the process CWD instead of the chosen --directory / CLASH_HOME_DIR context used
by the cli.config branch; change the logic so relative env paths are resolved
against the same directory context: either (A) update resolve_config_file to
accept a base directory parameter and pass cli.directory / CLASH_HOME_DIR into
it, or (B) before calling resolve_config_file, if PathBuf::from(file) is
relative, join it with the directory variable used elsewhere (the cli.directory
/ home dir used in the cli.config branch) and pass that absolute path; apply the
same fix to the other env-based branches mentioned (around lines 343-355) to
ensure consistent resolution.
- Around line 255-279: The current ensure_file implementation has a TOCTOU race:
it checks path.exists() then calls File::create(path) which can truncate a file
created by another process; replace the non-atomic logic with an atomic
create_new attempt (e.g. use
std::fs::OpenOptions::new().write(true).create_new(true).open(path)) to create
the file only if it does not already exist and treat ErrorKind::AlreadyExists as
success, or alternatively write the default content to a temporary file in the
same directory and atomically rename it into place; update the code paths around
ensure_file, the block that creates the parent dir, the OpenOptions/File::create
usage and the subsequent write_all(DEFAULT_CONFIG_CONTENT.as_bytes()) to use the
chosen atomic method so we never truncate an existing file.
In `@clash-bin/tests/config_input_test.rs`:
- Around line 16-23: clash_cmd() currently removes config-related env vars but
doesn't set the CI flag, causing tests to run a different startup path; update
the clash_cmd() function to set the environment variable CLASH_RS_CI to "true"
on the spawned Command (add a .env("CLASH_RS_CI", "true") call alongside the
existing .env_remove calls) so the binary runs in CI mode during tests.
---
Nitpick comments:
In `@clash-bin/tests/config_input_test.rs`:
- Around line 53-295: Add tests exercising ConfigInput::resolve() branches for
CLASH_HOME_DIR and CLASH_CONFIG_FILE and their precedence: create new tests that
spawn the binary via clash_cmd() while setting child env vars (use Command.env)
for CLASH_HOME_DIR pointing to a temp dir (and verify a default config is
created at that home path) and CLASH_CONFIG_FILE pointing to an explicit file
path (and verify that file is read); additionally add a test where both an env
var (CLASH_CONFIG_FILE or CLASH_HOME_DIR) and an explicit CLI flag (-f /
-config) are provided and assert the CLI flag wins (explicit path/test output),
referencing clash_cmd(), run_with_stdin() if needed, and ensuring assertions
mirror existing patterns (status, stdout, stderr) so the resolver branches in
ConfigInput::resolve() are covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b2ec52d7-37f0-4025-86ed-e32d1e0de877
📒 Files selected for processing (3)
clash-bin/src/main.rsclash-bin/tests/config_input_test.rsclash-lib/src/config/internal/proxy.rs
Comment on lines
+217
to
+243
| fn resolve(cli: &Cli) -> anyhow::Result<Self> { | ||
| if let Some(config) = cli.config_string.as_deref() { | ||
| return decode_config(config); | ||
| } | ||
| if let Ok(config) = std::env::var("CLASH_CONFIG_STRING") { | ||
| return decode_config(&config); | ||
| } | ||
|
|
||
| let current_dir = | ||
| std::env::current_dir().context("get current directory")?; | ||
| let home_dir = cli | ||
| .directory | ||
| .clone() | ||
| .or_else(|| std::env::var_os("CLASH_HOME_DIR").map(PathBuf::from)); | ||
| let directory = match home_dir { | ||
| Some(directory) if directory.is_absolute() => directory, | ||
| Some(directory) => current_dir.join(directory), | ||
| None => current_dir, | ||
| }; | ||
|
|
||
| if let Some(config) = cli.config.as_ref() { | ||
| return resolve_config_path(config, &directory); | ||
| } | ||
|
|
||
| if let Some(file) = std::env::var_os("CLASH_CONFIG_FILE") { | ||
| return resolve_config_file(&PathBuf::from(file)); | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve CLI-over-env precedence for config selection.
resolve() checks CLASH_CONFIG_STRING before cli.config, so an explicit -f/--config is ignored whenever that env var is set. That makes the file flag impossible to use in processes that export CLASH_CONFIG_STRING.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clash-bin/src/main.rs` around lines 217 - 243, The resolve() logic currently
checks CLASH_CONFIG_STRING before honoring the CLI file flag, so update the
precedence to prefer CLI flags: keep checking cli.config_string first
(decode_config), then if cli.config is Some call resolve_config_path, then only
afterwards check the environment CLASH_CONFIG_STRING (decode_config) and
CLASH_CONFIG_FILE (resolve_config_file); ensure you reference and preserve calls
to decode_config, resolve_config_path, and resolve_config_file and maintain the
directory resolution logic used for resolve_config_path.
Comment on lines
+241
to
+243
| if let Some(file) = std::env::var_os("CLASH_CONFIG_FILE") { | ||
| return resolve_config_file(&PathBuf::from(file)); | ||
| } |
Contributor
There was a problem hiding this comment.
Relative env-based config paths are resolved from the wrong base.
resolve_config_file() has no directory context, so CLASH_CONFIG_FILE=foo.yaml and the empty-stdin fallback from -f - both resolve from the process cwd instead of --directory / CLASH_HOME_DIR. That breaks the same path semantics the cli.config branch uses.
Also applies to: 343-355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clash-bin/src/main.rs` around lines 241 - 243, The CLASH_CONFIG_FILE branch
calls resolve_config_file(&PathBuf::from(file)) which resolves relative paths
against the process CWD instead of the chosen --directory / CLASH_HOME_DIR
context used by the cli.config branch; change the logic so relative env paths
are resolved against the same directory context: either (A) update
resolve_config_file to accept a base directory parameter and pass cli.directory
/ CLASH_HOME_DIR into it, or (B) before calling resolve_config_file, if
PathBuf::from(file) is relative, join it with the directory variable used
elsewhere (the cli.directory / home dir used in the cli.config branch) and pass
that absolute path; apply the same fix to the other env-based branches mentioned
(around lines 343-355) to ensure consistent resolution.
Comment on lines
+255
to
+279
| fn ensure_file(&self) -> anyhow::Result<()> { | ||
| let Self::File { path, .. } = self else { | ||
| return Ok(()); | ||
| }; | ||
| if path.exists() { | ||
| return Ok(()); | ||
| } | ||
| if let Some(parent) = path.parent() | ||
| && !parent.as_os_str().is_empty() | ||
| { | ||
| std::fs::create_dir_all(parent).with_context(|| { | ||
| format!( | ||
| "default profile directory cannot be created: {}", | ||
| parent.display() | ||
| ) | ||
| })?; | ||
| } | ||
| let mut config_file = std::fs::File::create(path).with_context(|| { | ||
| format!("default profile cannot be created: {}", path.display()) | ||
| })?; | ||
| config_file | ||
| .write_all(DEFAULT_CONFIG_CONTENT.as_bytes()) | ||
| .with_context(|| { | ||
| format!("default profile cannot be written: {}", path.display()) | ||
| })?; |
Contributor
There was a problem hiding this comment.
Create the default config atomically.
The exists() check at Line 259 and File::create() at Line 272 form a TOCTOU window. If another process creates the file in between, this path will truncate it back to the template content.
🔒 Proposed fix
fn ensure_file(&self) -> anyhow::Result<()> {
let Self::File { path, .. } = self else {
return Ok(());
};
- if path.exists() {
- return Ok(());
- }
if let Some(parent) = path.parent()
&& !parent.as_os_str().is_empty()
{
std::fs::create_dir_all(parent).with_context(|| {
format!(
@@
- let mut config_file = std::fs::File::create(path).with_context(|| {
- format!("default profile cannot be created: {}", path.display())
- })?;
+ let mut config_file = match std::fs::OpenOptions::new()
+ .write(true)
+ .create_new(true)
+ .open(path)
+ {
+ Ok(file) => file,
+ Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {
+ return Ok(());
+ }
+ Err(err) => {
+ return Err(err).with_context(|| {
+ format!("default profile cannot be created: {}", path.display())
+ });
+ }
+ };
config_file
.write_all(DEFAULT_CONFIG_CONTENT.as_bytes())
.with_context(|| {
format!("default profile cannot be written: {}", path.display())
})?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clash-bin/src/main.rs` around lines 255 - 279, The current ensure_file
implementation has a TOCTOU race: it checks path.exists() then calls
File::create(path) which can truncate a file created by another process; replace
the non-atomic logic with an atomic create_new attempt (e.g. use
std::fs::OpenOptions::new().write(true).create_new(true).open(path)) to create
the file only if it does not already exist and treat ErrorKind::AlreadyExists as
success, or alternatively write the default content to a temporary file in the
same directory and atomically rename it into place; update the code paths around
ensure_file, the block that creates the parent dir, the OpenOptions/File::create
usage and the subsequent write_all(DEFAULT_CONFIG_CONTENT.as_bytes()) to use the
chosen atomic method so we never truncate an existing file.
Comment on lines
+16
to
+23
| fn clash_cmd() -> Command { | ||
| let mut command = Command::new(env!("CARGO_BIN_EXE_clash-rs")); | ||
| command | ||
| .env_remove("CLASH_CONFIG_FILE") | ||
| .env_remove("CLASH_CONFIG_STRING") | ||
| .env_remove("CLASH_HOME_DIR"); | ||
| command | ||
| } |
Contributor
There was a problem hiding this comment.
Run the spawned binary in CI mode too.
clash_cmd() scrubs the config env vars, but it never sets CLASH_RS_CI=true, so these integration tests can execute a different startup path than the repo uses in CI.
🧪 Proposed fix
fn clash_cmd() -> Command {
let mut command = Command::new(env!("CARGO_BIN_EXE_clash-rs"));
command
.env_remove("CLASH_CONFIG_FILE")
.env_remove("CLASH_CONFIG_STRING")
- .env_remove("CLASH_HOME_DIR");
+ .env_remove("CLASH_HOME_DIR")
+ .env("CLASH_RS_CI", "true");
command
}Based on learnings: Use CLASH_RS_CI=true environment variable when running tests in CI mode.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn clash_cmd() -> Command { | |
| let mut command = Command::new(env!("CARGO_BIN_EXE_clash-rs")); | |
| command | |
| .env_remove("CLASH_CONFIG_FILE") | |
| .env_remove("CLASH_CONFIG_STRING") | |
| .env_remove("CLASH_HOME_DIR"); | |
| command | |
| } | |
| fn clash_cmd() -> Command { | |
| let mut command = Command::new(env!("CARGO_BIN_EXE_clash-rs")); | |
| command | |
| .env_remove("CLASH_CONFIG_FILE") | |
| .env_remove("CLASH_CONFIG_STRING") | |
| .env_remove("CLASH_HOME_DIR") | |
| .env("CLASH_RS_CI", "true"); | |
| command | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clash-bin/tests/config_input_test.rs` around lines 16 - 23, clash_cmd()
currently removes config-related env vars but doesn't set the CI flag, causing
tests to run a different startup path; update the clash_cmd() function to set
the environment variable CLASH_RS_CI to "true" on the spawned Command (add a
.env("CLASH_RS_CI", "true") call alongside the existing .env_remove calls) so
the binary runs in CI mode during tests.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
ibigbug
approved these changes
Apr 25, 2026
Member
|
please fix the failed tests |
In cross-compilation test runs (arm, aarch64, riscv, etc.) the clash-rs binary cannot be spawned as a child process from within a QEMU-emulated test runner -- glibc falls back to execing the foreign-arch ELF through /bin/sh, which exits 127. All nine config_input_test tests failed with exit code 127 or empty stdout. Fix: add a OnceLock-cached binary_can_be_spawned() probe that runs the binary with -v and checks for exit code 127. Each test calls skip_on_cross!() at the top to bail out early in such environments. On Windows, escape_logrus_text() doubles backslashes in log lines, so 'configuration file C:\Users\...' appears in stdout but the test was checking for 'C:\...' with single backslashes, causing empty_file_reports_empty_file_error to fail. Fix: restructure that test to check lines individually, mirroring file_failure_logs_error_then_summary_on_stdout: verify the first line contains 'is empty' (no path comparison) and check the unescaped summary line for the exact path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ibigbug
reviewed
Apr 28, 2026
Member
ibigbug
left a comment
ibigbug
left a comment
There was a problem hiding this comment.
Code review
�� High: Health check with empty URL will spam errors
When health-check: {enable: true} is set without a url, effective_interval() returns 300 (the default) and the health check kicks off every 5 minutes — but fires against an empty string URL, which fails to parse on every tick.
pub fn effective_interval(&self) -> u64 {
if !self.enable {
return 0;
}
if self.interval == 0 {
300 // <-- fires even when url is ""
} else {
self.interval
}
}Suggested fix — treat a missing URL the same as disabled:
pub fn effective_interval(&self) -> u64 {
if !self.enable || self.url.is_empty() {
return 0;
}
if self.interval == 0 { 300 } else { self.interval }
}⚠️ Medium: -config flag semantics differ from mihomo
In mihomo, -config specifies a config file path. Here it is remapped to --config-string (base64 input):
"-config" => "--config-string".to_string(),Anyone currently using -config /path/to/config.yaml would silently get a base64-decode error instead. If the goal is mihomo CLI compatibility, -config should map to --config (file path), not --config-string.
The test failures in CI are addressed by PR #1348 (skip tests in cross/QEMU envs where subprocess exec returns 127, fix Windows backslash escaping in empty_file_reports_empty_file_error).
Member
|
@Tunglies check the AI review comment? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
对其 mihomo 对 -t 测试行为输出格式
对其 mihomo 对字段
health-check填充和检测Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores