fix(tests): skip config_input_test in cross env, fix Windows path in log#1348
Closed
fix(tests): skip config_input_test in cross env, fix Windows path in log#1348
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
… for HealthCheck Co-authored-by: Copilot <copilot@github.com>
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>
Contributor
📝 WalkthroughWalkthroughThe changes refactor CLI config input handling to support filesystem paths, base64-encoded strings, and stdin sources; improve health-check configuration with proper defaults and helper methods; add comprehensive integration tests; and strengthen error handling by converting panic-prone code to Result-based propagation across config conversion routines. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Parser as Argument Parser
participant ConfigInput as ConfigInput
participant Source as Config Source<br/>(File/Base64/Stdin)
participant Logger as Logger
participant App as Application
User->>Parser: clash-rs -f config.yaml<br/>(or --config-string=... or -)
Parser->>Parser: Normalize mihomo flags<br/>(-f, -config, etc.)
Parser->>ConfigInput: Create ConfigInput<br/>(path/base64/stdin)
ConfigInput->>Source: Resolve input source
Source-->>ConfigInput: Config bytes
ConfigInput->>ConfigInput: Parse and validate
alt Validation Success
ConfigInput-->>Logger: Timestamped success log
Logger->>App: Proceed with config
else Validation Failure
ConfigInput-->>Logger: Timestamped error log<br/>(decode/parse error)
Logger-->>User: "configuration file ... test failed"<br/>(stdout)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 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 docstrings
🧪 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: 2
🤖 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 329-356: The fallback for "-f -" currently returns
DEFAULT_CONFIG_FILE in the process CWD; change resolve_config_file and its
caller so the configured home directory is preserved: update resolve_config_file
to accept an extra directory: &Path parameter and, when stdin is empty, return
ConfigInput::from_file_path(directory.join(DEFAULT_CONFIG_FILE)) instead of
PathBuf::from(DEFAULT_CONFIG_FILE); then update resolve_config_path to call
resolve_config_file(config, directory) when config == Path::new("-"). Use the
existing symbols resolve_config_path, resolve_config_file, DEFAULT_CONFIG_FILE,
and ConfigInput::from_file_path to locate and implement the changes.
- Around line 321-326: The function decode_config currently uses
String::from_utf8_lossy which silently replaces invalid UTF-8 bytes; change
decode_config to validate UTF-8 strictly (e.g., use String::from_utf8 or
std::str::from_utf8 on the decoded byte Vec) and return an error via anyhow!
when UTF-8 validation fails so that ConfigInput::Bytes { content } only receives
valid UTF-8 content and invalid base64 payloads are rejected instead of being
repaired.
🪄 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: be79ee31-f660-49cf-b289-b5851343efeb
⛔ 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/config_input_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
+321
to
+326
| 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(); | ||
| Ok(ConfigInput::Bytes { content }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd clash-bin && find . -name "main.rs" -type fRepository: Watfaq/clash-rs
Length of output: 73
🏁 Script executed:
cat -n clash-bin/src/main.rs | sed -n '310,340p'Repository: Watfaq/clash-rs
Length of output: 1254
🏁 Script executed:
# Let's check the full context of decode_config and how ConfigInput is used
rg -A 20 "fn decode_config" clash-bin/src/main.rsRepository: Watfaq/clash-rs
Length of output: 701
🏁 Script executed:
# Check how ConfigInput is used in the codebase
rg "ConfigInput" clash-bin/src/main.rs | head -20Repository: Watfaq/clash-rs
Length of output: 932
🏁 Script executed:
# Check if there are any tests for decode_config
rg -B 5 -A 10 "decode_config\|ConfigInput" clash-bin/src/main.rsRepository: Watfaq/clash-rs
Length of output: 41
🏁 Script executed:
# Check ConfigInput enum definition to understand the structure better
rg -B 5 -A 15 "enum ConfigInput" clash-bin/src/main.rsRepository: Watfaq/clash-rs
Length of output: 653
🏁 Script executed:
# Check how STANDARD is defined (base64 standard)
rg "STANDARD" clash-bin/src/main.rs | head -10Repository: Watfaq/clash-rs
Length of output: 143
🏁 Script executed:
# Look for any tests or usages of decode_config
rg -B 3 -A 5 "decode_config" clash-bin/src/ --type rsRepository: Watfaq/clash-rs
Length of output: 86
🏁 Script executed:
# Check if there's documentation or examples showing what inputs are expected
rg -B 5 -A 5 "config-string\|config_string" clash-bin/src/main.rsRepository: Watfaq/clash-rs
Length of output: 41
🏁 Script executed:
# Check the full ConfigInput implementation to understand how the content is used
rg -A 30 "impl ConfigInput" clash-bin/src/main.rsRepository: Watfaq/clash-rs
Length of output: 1167
🏁 Script executed:
# Check if there are any validation or parsing steps after ConfigInput::Bytes is created
rg -B 5 -A 10 "config_input.load\|config_input\.load\|ConfigInput::" clash-bin/src/main.rs | head -40Repository: Watfaq/clash-rs
Length of output: 41
🏁 Script executed:
# Look at how the config content is parsed after being read
rg -B 3 -A 3 "Bytes.*content" clash-bin/src/main.rsRepository: Watfaq/clash-rs
Length of output: 940
🏁 Script executed:
# Check if there's a reason for using lossy conversion by looking at git history or comments
rg -B 10 "from_utf8_lossy" clash-bin/src/main.rsRepository: Watfaq/clash-rs
Length of output: 367
🏁 Script executed:
# Check how clash::Config::Str handles the content parsing
rg -B 5 -A 5 "try_parse\|Config::Str" clash-bin/src/ | head -40Repository: Watfaq/clash-rs
Length of output: 41
🏁 Script executed:
# Look for any documentation or comments about expected input format
rg -B 5 "config_string\|CLASH_CONFIG_STRING" clash-bin/src/main.rsRepository: Watfaq/clash-rs
Length of output: 41
🏁 Script executed:
# Check if there are integration tests or examples showing expected behavior
find clash-bin -name "*.rs" -type f | xargs rg -l "decode_config\|config_string" | head -5Repository: Watfaq/clash-rs
Length of output: 41
🏁 Script executed:
# Check if there are comments in the codebase about UTF-8 handling
rg "utf.?8\|UTF.?8" clash-bin/src/main.rsRepository: Watfaq/clash-rs
Length of output: 41
🏁 Script executed:
# Verify the base64 crate version to understand decode behavior
cat clash-bin/Cargo.toml | grep -A 5 "base64"Repository: Watfaq/clash-rs
Length of output: 265
Reject non-UTF-8 base64 configs instead of silently repairing them.
String::from_utf8_lossy replaces invalid bytes with U+FFFD, so the parsed YAML can differ from the caller's payload. This should fail fast with a UTF-8 error instead of continuing with modified content.
🩹 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 as 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 321 - 326, The function decode_config
currently uses String::from_utf8_lossy which silently replaces invalid UTF-8
bytes; change decode_config to validate UTF-8 strictly (e.g., use
String::from_utf8 or std::str::from_utf8 on the decoded byte Vec) and return an
error via anyhow! when UTF-8 validation fails so that ConfigInput::Bytes {
content } only receives valid UTF-8 content and invalid base64 payloads are
rejected instead of being repaired.
Comment on lines
+329
to
+356
| fn resolve_config_path( | ||
| config: &Path, | ||
| directory: &Path, | ||
| ) -> anyhow::Result<ConfigInput> { | ||
| if config == Path::new("-") { | ||
| return resolve_config_file(config); | ||
| } | ||
| if config.is_absolute() { | ||
| Ok(ConfigInput::from_file_path(config.to_path_buf())) | ||
| } else { | ||
| Ok(ConfigInput::from_file_path(directory.join(config))) | ||
| } | ||
| } | ||
|
|
||
| fn resolve_config_file(file: &Path) -> anyhow::Result<ConfigInput> { | ||
| if file == Path::new("-") { | ||
| let mut content = String::new(); | ||
| std::io::stdin() | ||
| .read_to_string(&mut content) | ||
| .context("read configuration from stdin")?; | ||
| if !content.is_empty() { | ||
| return Ok(ConfigInput::Bytes { content }); | ||
| } | ||
| return Ok(ConfigInput::from_file_path(PathBuf::from( | ||
| DEFAULT_CONFIG_FILE, | ||
| ))); | ||
| } | ||
| Ok(ConfigInput::from_file_path(file.to_path_buf())) |
Contributor
There was a problem hiding this comment.
Keep the configured home directory when -f - receives no stdin.
When stdin is empty, the fallback path becomes plain config.yaml, so --directory <dir> -f - (and CLASH_HOME_DIR with CLASH_CONFIG_FILE=-) creates the default file in the process cwd instead of the resolved config directory.
🩹 Proposed fix
fn resolve_config_path(
config: &Path,
directory: &Path,
) -> anyhow::Result<ConfigInput> {
if config == Path::new("-") {
- return resolve_config_file(config);
+ return resolve_config_file(config, Some(directory));
}
if config.is_absolute() {
Ok(ConfigInput::from_file_path(config.to_path_buf()))
} else {
Ok(ConfigInput::from_file_path(directory.join(config)))
@@
if let Some(file) = std::env::var_os("CLASH_CONFIG_FILE") {
- return resolve_config_file(&PathBuf::from(file));
+ return resolve_config_file(&PathBuf::from(file), Some(&directory));
}
@@
-fn resolve_config_file(file: &Path) -> anyhow::Result<ConfigInput> {
+fn resolve_config_file(
+ file: &Path,
+ directory: Option<&Path>,
+) -> anyhow::Result<ConfigInput> {
if file == Path::new("-") {
let mut content = String::new();
std::io::stdin()
.read_to_string(&mut content)
.context("read configuration from stdin")?;
if !content.is_empty() {
return Ok(ConfigInput::Bytes { content });
}
- return Ok(ConfigInput::from_file_path(PathBuf::from(
- DEFAULT_CONFIG_FILE,
- )));
+ let fallback = directory
+ .map(|dir| dir.join(DEFAULT_CONFIG_FILE))
+ .unwrap_or_else(|| PathBuf::from(DEFAULT_CONFIG_FILE));
+ return Ok(ConfigInput::from_file_path(fallback));
}
Ok(ConfigInput::from_file_path(file.to_path_buf()))
}🤖 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 329 - 356, The fallback for "-f -"
currently returns DEFAULT_CONFIG_FILE in the process CWD; change
resolve_config_file and its caller so the configured home directory is
preserved: update resolve_config_file to accept an extra directory: &Path
parameter and, when stdin is empty, return
ConfigInput::from_file_path(directory.join(DEFAULT_CONFIG_FILE)) instead of
PathBuf::from(DEFAULT_CONFIG_FILE); then update resolve_config_path to call
resolve_config_file(config, directory) when config == Path::new("-"). Use the
existing symbols resolve_config_path, resolve_config_file, DEFAULT_CONFIG_FILE,
and ConfigInput::from_file_path to locate and implement the changes.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Member
Author
|
Closing — the fix was pushed directly to the contributor's PR branch (feat/mirage-to-mihomo) since maintainer edits are enabled. |
Contributor
📊 Proxy Throughput ResultsShadowsocks
Trojan
VMess
VLESS
SOCKS5
AnyTLS
Hysteria2
TUIC
ShadowQUIC
SSH
Netem Tests (50 ms delay, 1% packet loss)Shadowsocks
Trojan
Hysteria2
TUIC
ShadowQUIC
Ran 34 variant(s) in parallel; each direction transfers the full payload. 🖥️ Test Environment
📎 View full workflow run and download artifacts Full test logDownload the |
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.
Context
Fixes the failing tests from PR #1331 (feat/mirage-to-mihomo).
Root causes
1. Cross-compiled targets (arm, aarch64, riscv, etc.)
All 9
config_input_testtests failed with exit code 127 or empty stdout. Incross/QEMU test environments the test binary runs inside an emulator, but when it spawns a child process (theclash-rsbinary),execveof the foreign-arch ELF is not intercepted by QEMU. glibc falls back to running it through/bin/sh, which exits with code 127.Fix: Add a
binary_can_be_spawned()probe (cached viaOnceLock) that runs the binary with-vand checks for exit code 127. Each test callsskip_on_cross!()at the top to bail out early.2. Windows (
empty_file_reports_empty_file_erroronly)escape_logrus_text()doubles backslashes in log lines (C:\\Users\\...), but the test checkedstdout.contains("configuration file C:\\Users\\...\\empty.yaml is empty")(single backslashes). The path in the log line didn't match.Fix: Restructure the 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.Changes
clash-bin/tests/config_input_test.rs: addsbinary_can_be_spawned()/skip_on_cross!()guard to all 9 tests; fixesempty_file_reports_empty_file_errorfor WindowsSummary by CodeRabbit
Release Notes
New Features
Improvements
Tests