refactor: split large modules into directories#36
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 58 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthrough将大型单体实现拆分为多个专用模块：admin CLI、nginx 迁移管道、配置编译、代理客户端与转发子模块，以及大量测试从内联迁移到独立测试文件。若干旧文件被删除并以模块化实现替换。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors the codebase by splitting previously large Rust modules and test files into topic-focused submodules/directories, while keeping thin entrypoints and reorganizing related tooling.
Changes:
- Split
proxy::forwardintoerror,grpc, andresponsesubmodules. - Split
proxy::clients(including TLS helpers) into a directory module layout and relocate unit tests. - Break up large integration test files (reload, gRPC proxy, downstream mTLS, admin) into smaller per-topic files; introduce new admin CLI module files.
Reviewed changes
Copilot reviewed 47 out of 110 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rginx-http/src/proxy/forward/response.rs | Extracts downstream response-building (incl. grpc-web + deadlines) into a dedicated module. |
| crates/rginx-http/src/proxy/forward/mod.rs | Converts forward into a directory module; wires new submodules and re-exports. |
| crates/rginx-http/src/proxy/forward/grpc.rs | Moves grpc/grpc-web header parsing and timeout parsing into a dedicated module. |
| crates/rginx-http/src/proxy/forward/error.rs | Moves upstream-stage timeout + HTTP/grpc error helpers into a dedicated module. |
| crates/rginx-http/src/proxy/clients/tls.rs | Extracts upstream TLS config building and certificate/CRL loading into a focused module. |
| crates/rginx-http/src/proxy/clients/tests.rs | Moves ProxyClients tests into the new proxy/clients module directory layout. |
| crates/rginx-http/src/proxy/clients/mod.rs | Replaces monolithic clients.rs with a directory module and delegates TLS logic to tls. |
| crates/rginx-http/src/proxy/clients.rs | Removes the old monolithic ProxyClients implementation file (now split into submodules). |
| crates/rginx-config/src/compile/path.rs | Adds a small helper for resolving relative vs absolute paths during config compilation. |
| crates/rginx-config/src/compile/mod.rs | Introduces a directory module structure for config compilation and centralizes defaults. |
| crates/rginx-app/tests/reload/restart_flow.rs | Splits reload tests: restart-related test cases. |
| crates/rginx-app/tests/reload/reload_flow.rs | Splits reload tests: successful reload behavior. |
| crates/rginx-app/tests/reload/reload_boundary.rs | Splits reload tests: rejection/boundary conditions. |
| crates/rginx-app/tests/reload.rs | Converts reload tests into a file that only defines shared fixtures + submodules. |
| crates/rginx-app/tests/grpc_proxy/timeout.rs | Splits grpc-proxy tests: timeout handling cases. |
| crates/rginx-app/tests/grpc_proxy/lifecycle.rs | Splits grpc-proxy tests: lifecycle/cancellation/health-check cases. |
| crates/rginx-app/tests/grpc_proxy/basic.rs | Splits grpc-proxy tests: baseline proxying behavior and routing cases. |
| crates/rginx-app/tests/downstream_mtls/validation.rs | Splits downstream mTLS tests: validation scenarios (verify depth, CRL). |
| crates/rginx-app/tests/downstream_mtls/observability.rs | Splits downstream mTLS tests: access log / observability assertions. |
| crates/rginx-app/tests/downstream_mtls/enforcement.rs | Splits downstream mTLS tests: required/optional enforcement functionality. |
| crates/rginx-app/tests/downstream_mtls.rs | Converts downstream mTLS tests into a shared-fixtures + submodules file. |
| crates/rginx-app/tests/admin/snapshot.rs | Splits admin tests: snapshot/revision snapshot scenarios. |
| crates/rginx-app/tests/admin/delta_wait.rs | Splits admin tests: delta + wait command behaviors. |
| crates/rginx-app/tests/admin/commands.rs | Splits admin tests: status/counters/traffic/peers/upstreams command behaviors. |
| crates/rginx-app/src/migrate_nginx/tokenize.rs | Introduces nginx config tokenizer used by the migration pipeline. |
| crates/rginx-app/src/migrate_nginx/tests.rs | Adds tests for nginx migration helpers and a basic end-to-end migration render check. |
| crates/rginx-app/src/migrate_nginx/render.rs | Adds a renderer that outputs RON config along with migration warnings. |
| crates/rginx-app/src/migrate_nginx/mod.rs | Adds the migrate-nginx module entrypoint and file/source migration wiring. |
| crates/rginx-app/src/admin_cli/upstreams.rs | Adds upstreams admin CLI command output implementation. |
| crates/rginx-app/src/admin_cli/traffic.rs | Adds traffic admin CLI command output implementation. |
| crates/rginx-app/src/admin_cli/status.rs | Adds status admin CLI command output implementation. |
| crates/rginx-app/src/admin_cli/socket.rs | Adds shared admin socket query/response handling for CLI commands. |
| crates/rginx-app/src/admin_cli/snapshot.rs | Adds snapshot, snapshot-version, delta, and wait CLI output implementations. |
| crates/rginx-app/src/admin_cli/render.rs | Adds record rendering helpers for CLI output (key=value with JSON escaping). |
| crates/rginx-app/src/admin_cli/peers.rs | Adds peers admin CLI command output implementation. |
| crates/rginx-app/src/admin_cli/mod.rs | Introduces the admin CLI submodule and routes CLI commands to it. |
| crates/rginx-app/src/admin_cli/counters.rs | Adds counters admin CLI command output implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
crates/rginx-config/src/compile/tests.rs (1)
826-949: 临时文件清理可以更健壮当前测试在函数末尾手动清理临时文件。如果测试断言失败或 panic，临时文件不会被清理。考虑使用
tempfilecrate 实现自动清理，或使用scopeguard确保清理逻辑始终执行。这是一个可选的改进，不影响测试正确性。
♻️ 可选的改进示例
// 使用 tempfile crate 的示例 use tempfile::TempDir; #[test] fn compile_resolves_custom_ca_relative_to_config_base() { let base_dir = TempDir::new().expect("temp base dir should be created"); let ca_path = base_dir.path().join("dev-ca.pem"); fs::write(&ca_path, b"placeholder").expect("temp CA file should be written"); // ... test logic ... // TempDir 在作用域结束时自动清理 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-config/src/compile/tests.rs` around lines 826 - 949, The test manually creates and removes a temp directory and CA file (base_dir, ca_path, fs::remove_file, fs::remove_dir) which won't run if the test panics; change the test (compile_resolves_custom_ca_relative_to_config_base / the block that calls compile_with_base) to use an owned TempDir (tempfile::TempDir) or a scopeguard so the temp dir and files are always cleaned up on drop, remove the explicit fs::remove_* calls, and update any path references to use TempDir::path() to locate dev-ca.pem.crates/rginx-http/src/proxy/clients/tls.rs (1)
135-140: CRL DER 回退缺少格式验证当 PEM 解析返回空结果时，代码直接将整个文件内容作为 DER 格式的 CRL 使用。如果文件既不是有效的 PEM 也不是有效的 DER，这可能会导致后续 TLS 握手时出现难以理解的错误。
建议添加基本的 DER 解析验证，或在错误消息中明确指出格式要求。
♻️ 可选的改进建议
if !crls.is_empty() { return Ok(crls); } - Ok(vec![rustls::pki_types::CertificateRevocationListDer::from(std::fs::read(path)?)]) + let raw = std::fs::read(path)?; + if raw.is_empty() { + return Err(Error::Server(format!( + "CRL file `{}` is empty or contains no valid CRLs", + path.display() + ))); + } + Ok(vec![rustls::pki_types::CertificateRevocationListDer::from(raw)])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/proxy/clients/tls.rs` around lines 135 - 140, When PEM parsing yields no CRLs (crls is empty) don't blindly wrap the file bytes into CertificateRevocationListDer::from; instead read the file bytes (path) and attempt to actually parse/validate them as a DER CRL (e.g., via a DER parse/try_from API or parser provided by rustls/pki or openssl) and return a clear Err if parsing fails, otherwise return the validated CertificateRevocationListDer in the vec; update the error message to state that the file must be a valid PEM or DER CRL and include the path or parse error for diagnostics.crates/rginx-app/tests/admin.rs (1)
2-2: 同前述文件，#![allow(unused_imports)]建议在重构完成后清理。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/tests/admin.rs` at line 2, 在 tests/admin.rs 中保留的 crate 属性 `#![allow(unused_imports)]` 已经是暂时用于重构阶段的抑制器，请在完成重构并移除或整理未使用的导入后删除该属性（或将其替换为更精确的 lint 允许如对单个模块或导入使用 #[allow(...)]），以确保未使用导入被清理并避免掩盖真实的警告；定位文件中的顶层属性 `#![allow(unused_imports)]` 并移除或缩小作用范围后重新编译并修复任何由此暴露的未使用导入。crates/rginx-app/tests/reload.rs (1)
2-2: 同前述文件，#![allow(unused_imports)]建议在重构完成后清理。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/tests/reload.rs` at line 2, 当前模块顶端的 crate 属性 `#![allow(unused_imports)]` 是临时为重构期间抑制警告添加的，重构完成后应移除或替换为更精确的允许项；请在文件中删除 `#![allow(unused_imports)]` 或将其替换为只允许确实需要的特定项（例如 `#![allow(dead_code)]` 等），然后运行测试/cargo clippy 确认没有未处理的未使用导入并修复相应导入或代码（关注 tests::reload 模块中的未使用导入）。crates/rginx-app/tests/grpc_proxy.rs (1)
1-2: 与downstream_mtls.rs相同的#![allow(unused_imports)]问题。建议在重构稳定后，将仅在子模块中使用的导入移到相应的子模块文件中，然后移除此属性。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/tests/grpc_proxy.rs` around lines 1 - 2, Remove the crate-level attribute "#![allow(unused_imports)]" from grpc_proxy.rs and instead move any imports that are only used by specific test submodules into those submodule files; mirror the same change made for downstream_mtls.rs: identify the unused imports currently silenced by "#![allow(unused_imports)]" in grpc_proxy.rs, relocate each import to the corresponding submodule where it is actually referenced, and then delete the attribute so the compiler will catch remaining unused imports.crates/rginx-app/src/admin_cli/render.rs (1)
8-14: 时间戳转换逻辑过于复杂，可以简化。当前实现将毫秒转换为秒后，又通过
UNIX_EPOCH.checked_add()和duration_since(UNIX_EPOCH)进行往返转换，最终结果仍是相同的秒数。这个链式调用主要用于溢出检查，但逻辑冗余。♻️ 建议简化
let finished_at = result .finished_at_unix_ms .checked_div(1000) - .and_then(|seconds| UNIX_EPOCH.checked_add(std::time::Duration::from_secs(seconds))) - .and_then(|time| time.duration_since(UNIX_EPOCH).ok()) - .map(|duration| duration.as_secs().to_string()) + .map(|seconds| seconds.to_string()) .unwrap_or_else(|| result.finished_at_unix_ms.to_string());如果需要验证时间戳有效性，可以添加显式的范围检查而非通过 SystemTime 往返。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/src/admin_cli/render.rs` around lines 8 - 14, The timestamp conversion in the finished_at computation is overly complex (uses checked_div + UNIX_EPOCH.checked_add + duration_since) to get seconds; replace it with a simple checked_div(1000) on result.finished_at_unix_ms and format that to string, and if you need validity checks add an explicit numeric range check instead of creating/round-tripping SystemTime; update the code around the finished_at binding (using result.finished_at_unix_ms and UNIX_EPOCH references) to use checked_div(1000).map(|s| s.to_string()).unwrap_or_else(|| result.finished_at_unix_ms.to_string()) or equivalent with an explicit range check.crates/rginx-app/tests/downstream_mtls.rs (1)
1-2:#![allow(unused_imports)]可能不是最佳方案。这个属性会抑制整个文件的未使用导入警告。如果这是因为重构后某些导入只在子模块中使用，可以考虑：
- 将这些导入移到实际使用它们的子模块中
- 或者使用
pub(super) use在子模块中重新导出如果这只是临时措施，可以保留；如果是最终状态，建议移除该属性并将未使用的导入移到相应的子模块。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/tests/downstream_mtls.rs` around lines 1 - 2, The file-level attribute #![allow(unused_imports)] is silencing unused-import warnings; remove this attribute and either move the imports that are only used by submodules into those submodules, or re-export them from the submodules using pub(super) use so the imports become used; if this was only temporary keep a local comment but prefer removing the attribute for final code and adjust the imports accordingly (look for the top-of-file attribute #![allow(unused_imports)] in downstream_mtls.rs and the unused imports in the test/submodule declarations).crates/rginx-app/tests/downstream_mtls/observability.rs (1)
14-21: 内联配置字符串较长，考虑提取为常量或辅助函数。当前配置字符串在单行中包含了大量格式化内容，降低了可读性。这是一个可选的重构建议，不影响功能正确性。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/tests/downstream_mtls/observability.rs` around lines 14 - 21, The large inline formatted config string should be extracted to improve readability; create a named helper (e.g., fn build_expected_config(listen: &str, cert: &str, key: &str, ca: &str) -> String or a const TEMPLATE: &str plus a small function to substitute values) and move the format! template body into it, then call that helper from the test instead of the long inline format! invocation that currently references READY_ROUTE_CONFIG, listen_addr.to_string(), cert_path.display().to_string(), key_path.display().to_string(), and ca_path.display().to_string().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rginx-app/src/admin_cli/status.rs`:
- Around line 60-68: Add the missing per-reason mTLS metric for
"missing_client_cert" to the CLI status output: include a tuple entry with key
"mtls_handshake_failures_missing_client_cert" and value
status.mtls.handshake_failures_missing_client_cert.to_string() alongside the
existing mtls entries (e.g., near the existing "mtls_handshake_failures",
"mtls_handshake_failures_certificate_revoked", and
"mtls_handshake_failures_verify_depth_exceeded" tuples) so the status command
shows this specific failure count.
In `@crates/rginx-app/src/migrate_nginx/parser.rs`:
- Around line 415-436: The loop currently assigns any non-whitelisted, non-`=`
token to address (candidate => address =
Some(normalize_listen_address(candidate)?)), letting unknown tokens overwrite a
previously parsed address (e.g. "quic" overwriting "443"); change this so the
first valid address wins: in the match arm handling candidate, only set address
if address.is_none(), otherwise push a warning (via warnings.push) that the
unknown listen token was ignored (include the candidate value) and do not
overwrite address; reference the address variable and the
normalize_listen_address(...) call when making this change.
- Around line 318-323: The parser currently overwrites tls.cert_path and
tls.key_path when encountering multiple Statement::Directive entries for
"ssl_certificate" / "ssl_certificate_key", losing earlier certificates; change
the parser to accumulate all occurrences instead of assigning: in
crates/rginx-app/src/migrate_nginx/parser.rs update the TLS struct fields
(tls.cert_path, tls.key_path) to collections (e.g. cert_paths: Vec<String>,
key_paths: Vec<String>) and, inside the match arm for Statement::Directive {
name, args } if name == "ssl_certificate" and similarly for
"ssl_certificate_key", push args.first().cloned() into the corresponding Vec
rather than assigning; also update any downstream code that consumes
tls.cert_path/tls.key_path to handle the Vec (choose how to prefer ordering or
pair cert/key entries, or retain all for later selection) so no certificates are
silently dropped.
In `@crates/rginx-app/src/migrate_nginx/render.rs`:
- Around line 210-218: The shorthand condition in render_upstream_tls
incorrectly omits verify_depth and crl_path, causing their values to be lost
when writing the shorthand "tls: Some(Insecure)"; update the shorthand check in
render_upstream_tls (which inspects ConvertedUpstreamTls) to also require
tls.verify_depth.is_none() and tls.crl_path.is_none() (in addition to the
existing checks for verify == Insecure, versions.is_empty(),
client_cert_path.is_none(), client_key_path.is_none()) so the function only
takes the shorthand path when those fields are absent.
In `@crates/rginx-app/tests/reload/reload_boundary.rs`:
- Around line 12-18: 这些拒绝路径在断言旧配置仍然生效前没有等待 reload 结果落定；在使用
server.send_signal(libc::SIGHUP) 后，不要直接用 server.wait_for_body(...) 或
assert_unreachable(...) 判断新 listener 是否未生效，而是先轮询并等待 server.status() 返回的
reload_attempts / reload_failures 发生预期变化（或达到超时），确认 reload 已完成/回滚后再调用
server.wait_for_body(initial_addr, "stable config\n", ...) 和
assert_unreachable(rejected_addr, ...); 对所有相同模式的测试块（当前文件中类似
server.write_return_config / server.send_signal / wait_for_body /
assert_unreachable 的段落）一并做相同调整。
In `@crates/rginx-app/tests/reload/restart_flow.rs`:
- Around line 127-143: The test currently uses server.wait_for_https_ready as
the synchronization point after server.send_signal(libc::SIGHUP), but that can
return immediately and not reflect a completed reload; change the sync to poll
the rginx status instead: after server.send_signal(SIGHUP) repeatedly call
run_cli_command(server.config_path(), ["status"]) (using
render_output(&status_output) for error context) until stdout contains
"reload_successes=1" (or until a certificate fingerprint value read from the
status output changes compared to a snapshot taken before the signal), with a
reasonable timeout; then proceed to assert last_reload_tls_certificate_changes.
This ensures the test waits for the actual reload completion rather than mere
HTTPS readiness.
In `@crates/rginx-http/src/proxy/forward/grpc.rs`:
- Around line 16-17: 当前用
normalized_mime.starts_with(GRPC_WEB_CONTENT_TYPE_PREFIX) 过宽，会把像
application/grpc-websocket 误判为 gRPC-Web；在检测时改为在匹配前缀后再检查边界：获取前缀之后的剩余字符串并确保它为空或以
'+' 或 ';' 开头 (允许 +proto 和参数)，例如替换 starts_with 检查为：检查
normalized_mime.starts_with(GRPC_WEB_CONTENT_TYPE_PREFIX) 然后 let rem =
&normalized_mime[GRPC_WEB_CONTENT_TYPE_PREFIX.len()..]; require rem.is_empty()
|| rem.starts_with('+') || rem.starts_with(';'); 并把相同修复应用到文件中其它相同的 starts_with
使用处 (涉及符号 normalized_mime 和 GRPC_WEB_CONTENT_TYPE_PREFIX).
---
Nitpick comments:
In `@crates/rginx-app/src/admin_cli/render.rs`:
- Around line 8-14: The timestamp conversion in the finished_at computation is
overly complex (uses checked_div + UNIX_EPOCH.checked_add + duration_since) to
get seconds; replace it with a simple checked_div(1000) on
result.finished_at_unix_ms and format that to string, and if you need validity
checks add an explicit numeric range check instead of creating/round-tripping
SystemTime; update the code around the finished_at binding (using
result.finished_at_unix_ms and UNIX_EPOCH references) to use
checked_div(1000).map(|s| s.to_string()).unwrap_or_else(||
result.finished_at_unix_ms.to_string()) or equivalent with an explicit range
check.
In `@crates/rginx-app/tests/admin.rs`:
- Line 2: 在 tests/admin.rs 中保留的 crate 属性 `#![allow(unused_imports)]`
已经是暂时用于重构阶段的抑制器，请在完成重构并移除或整理未使用的导入后删除该属性（或将其替换为更精确的 lint 允许如对单个模块或导入使用
#[allow(...)]），以确保未使用导入被清理并避免掩盖真实的警告；定位文件中的顶层属性 `#![allow(unused_imports)]`
并移除或缩小作用范围后重新编译并修复任何由此暴露的未使用导入。
In `@crates/rginx-app/tests/downstream_mtls.rs`:
- Around line 1-2: The file-level attribute #![allow(unused_imports)] is
silencing unused-import warnings; remove this attribute and either move the
imports that are only used by submodules into those submodules, or re-export
them from the submodules using pub(super) use so the imports become used; if
this was only temporary keep a local comment but prefer removing the attribute
for final code and adjust the imports accordingly (look for the top-of-file
attribute #![allow(unused_imports)] in downstream_mtls.rs and the unused imports
in the test/submodule declarations).
In `@crates/rginx-app/tests/downstream_mtls/observability.rs`:
- Around line 14-21: The large inline formatted config string should be
extracted to improve readability; create a named helper (e.g., fn
build_expected_config(listen: &str, cert: &str, key: &str, ca: &str) -> String
or a const TEMPLATE: &str plus a small function to substitute values) and move
the format! template body into it, then call that helper from the test instead
of the long inline format! invocation that currently references
READY_ROUTE_CONFIG, listen_addr.to_string(), cert_path.display().to_string(),
key_path.display().to_string(), and ca_path.display().to_string().
In `@crates/rginx-app/tests/grpc_proxy.rs`:
- Around line 1-2: Remove the crate-level attribute "#![allow(unused_imports)]"
from grpc_proxy.rs and instead move any imports that are only used by specific
test submodules into those submodule files; mirror the same change made for
downstream_mtls.rs: identify the unused imports currently silenced by
"#![allow(unused_imports)]" in grpc_proxy.rs, relocate each import to the
corresponding submodule where it is actually referenced, and then delete the
attribute so the compiler will catch remaining unused imports.
In `@crates/rginx-app/tests/reload.rs`:
- Line 2: 当前模块顶端的 crate 属性 `#![allow(unused_imports)]`
是临时为重构期间抑制警告添加的，重构完成后应移除或替换为更精确的允许项；请在文件中删除 `#![allow(unused_imports)]`
或将其替换为只允许确实需要的特定项（例如 `#![allow(dead_code)]` 等），然后运行测试/cargo clippy
确认没有未处理的未使用导入并修复相应导入或代码（关注 tests::reload 模块中的未使用导入）。
In `@crates/rginx-config/src/compile/tests.rs`:
- Around line 826-949: The test manually creates and removes a temp directory
and CA file (base_dir, ca_path, fs::remove_file, fs::remove_dir) which won't run
if the test panics; change the test
(compile_resolves_custom_ca_relative_to_config_base / the block that calls
compile_with_base) to use an owned TempDir (tempfile::TempDir) or a scopeguard
so the temp dir and files are always cleaned up on drop, remove the explicit
fs::remove_* calls, and update any path references to use TempDir::path() to
locate dev-ca.pem.
In `@crates/rginx-http/src/proxy/clients/tls.rs`:
- Around line 135-140: When PEM parsing yields no CRLs (crls is empty) don't
blindly wrap the file bytes into CertificateRevocationListDer::from; instead
read the file bytes (path) and attempt to actually parse/validate them as a DER
CRL (e.g., via a DER parse/try_from API or parser provided by rustls/pki or
openssl) and return a clear Err if parsing fails, otherwise return the validated
CertificateRevocationListDer in the vec; update the error message to state that
the file must be a valid PEM or DER CRL and include the path or parse error for
diagnostics.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: feac73e1-fdbc-4218-9a8f-18ded236903c
📒 Files selected for processing (110)
crates/rginx-app/src/admin_cli.rscrates/rginx-app/src/admin_cli/counters.rscrates/rginx-app/src/admin_cli/mod.rscrates/rginx-app/src/admin_cli/peers.rscrates/rginx-app/src/admin_cli/render.rscrates/rginx-app/src/admin_cli/snapshot.rscrates/rginx-app/src/admin_cli/socket.rscrates/rginx-app/src/admin_cli/status.rscrates/rginx-app/src/admin_cli/traffic.rscrates/rginx-app/src/admin_cli/upstreams.rscrates/rginx-app/src/migrate_nginx.rscrates/rginx-app/src/migrate_nginx/convert.rscrates/rginx-app/src/migrate_nginx/mod.rscrates/rginx-app/src/migrate_nginx/parser.rscrates/rginx-app/src/migrate_nginx/render.rscrates/rginx-app/src/migrate_nginx/tests.rscrates/rginx-app/src/migrate_nginx/tokenize.rscrates/rginx-app/tests/admin.rscrates/rginx-app/tests/admin/commands.rscrates/rginx-app/tests/admin/delta_wait.rscrates/rginx-app/tests/admin/snapshot.rscrates/rginx-app/tests/downstream_mtls.rscrates/rginx-app/tests/downstream_mtls/enforcement.rscrates/rginx-app/tests/downstream_mtls/observability.rscrates/rginx-app/tests/downstream_mtls/validation.rscrates/rginx-app/tests/grpc_proxy.rscrates/rginx-app/tests/grpc_proxy/basic.rscrates/rginx-app/tests/grpc_proxy/lifecycle.rscrates/rginx-app/tests/grpc_proxy/timeout.rscrates/rginx-app/tests/reload.rscrates/rginx-app/tests/reload/reload_boundary.rscrates/rginx-app/tests/reload/reload_flow.rscrates/rginx-app/tests/reload/restart_flow.rscrates/rginx-config/src/compile.rscrates/rginx-config/src/compile/mod.rscrates/rginx-config/src/compile/path.rscrates/rginx-config/src/compile/tests.rscrates/rginx-http/src/handler/mod.rscrates/rginx-http/src/handler/tests.rscrates/rginx-http/src/proxy/clients.rscrates/rginx-http/src/proxy/clients/mod.rscrates/rginx-http/src/proxy/clients/tests.rscrates/rginx-http/src/proxy/clients/tls.rscrates/rginx-http/src/proxy/forward/error.rscrates/rginx-http/src/proxy/forward/grpc.rscrates/rginx-http/src/proxy/forward/mod.rscrates/rginx-http/src/proxy/forward/response.rscrates/rginx-http/src/proxy/grpc_web/body.rscrates/rginx-http/src/proxy/grpc_web/codec.rscrates/rginx-http/src/proxy/grpc_web/mod.rscrates/rginx-http/src/proxy/mod.rscrates/rginx-http/src/proxy/tests.rscrates/rginx-http/src/proxy/tests/client_profiles.rscrates/rginx-http/src/proxy/tests/grpc.rscrates/rginx-http/src/proxy/tests/mod.rscrates/rginx-http/src/proxy/tests/peer_recovery.rscrates/rginx-http/src/proxy/tests/peer_selection.rscrates/rginx-http/src/proxy/tests/request_headers.rscrates/rginx-http/src/server.rscrates/rginx-http/src/server/accept.rscrates/rginx-http/src/server/connection.rscrates/rginx-http/src/server/graceful.rscrates/rginx-http/src/server/mod.rscrates/rginx-http/src/server/proxy_protocol.rscrates/rginx-http/src/server/tests.rscrates/rginx-http/src/state.rscrates/rginx-http/src/state/counters.rscrates/rginx-http/src/state/helpers.rscrates/rginx-http/src/state/mod.rscrates/rginx-http/src/state/tests.rscrates/rginx-http/src/state/tls_runtime.rscrates/rginx-http/src/state/tls_runtime/bindings.rscrates/rginx-http/src/state/tls_runtime/certificates.rscrates/rginx-http/src/state/tls_runtime/listeners.rscrates/rginx-http/src/state/tls_runtime/mod.rscrates/rginx-http/src/state/tls_runtime/ocsp.rscrates/rginx-http/src/state/tls_runtime/reload_boundary.rscrates/rginx-http/src/state/tls_runtime/upstreams.rscrates/rginx-http/src/timeout.rscrates/rginx-http/src/timeout/body.rscrates/rginx-http/src/timeout/io.rscrates/rginx-http/src/timeout/mod.rscrates/rginx-http/src/timeout/tests.rscrates/rginx-http/src/timeout/timers.rscrates/rginx-http/src/tls.rscrates/rginx-http/src/tls/acceptor.rscrates/rginx-http/src/tls/client_auth.rscrates/rginx-http/src/tls/mod.rscrates/rginx-http/src/tls/provider.rscrates/rginx-http/src/tls/session.rscrates/rginx-http/src/tls/sni.rscrates/rginx-http/src/tls/tests.rscrates/rginx-runtime/src/bootstrap/listeners.rscrates/rginx-runtime/src/bootstrap/mod.rscrates/rginx-runtime/src/bootstrap/reload.rscrates/rginx-runtime/src/bootstrap/restart.rscrates/rginx-runtime/src/bootstrap/shutdown.rsdocs/large-file-split-plan.mddocs/nginx-comparison-snapshots/2026-04-10-trixie-smoke.mddocs/nginx-comparison.mdscripts/nginx_compare.pyscripts/nginx_compare/__init__.pyscripts/nginx_compare/build.pyscripts/nginx_compare/checkout.pyscripts/nginx_compare/common.pyscripts/nginx_compare/configs.pyscripts/nginx_compare/launch.pyscripts/nginx_compare/main.pyscripts/nginx_compare/render.pyscripts/nginx_compare/scenarios.py
💤 Files with no reviewable changes (4)
- crates/rginx-http/src/proxy/clients.rs
- crates/rginx-app/src/admin_cli.rs
- crates/rginx-app/src/migrate_nginx.rs
- crates/rginx-config/src/compile.rs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/rginx-http/src/proxy/forward/grpc.rs (1)
62-63:⚠️ Potential issue | 🟠 Major不要用裸
starts_with("application/grpc")判定 gRPC 请求。这里和之前的 gRPC-Web 问题是同一类：
application/grpcweb、application/grpc-websocket这类无效 MIME 也会被当成 gRPC 请求，从而错误进入grpc-timeout的解析/裁剪路径。既然上面已经把 gRPC-Web 的匹配收紧了，这里最好显式匹配“原生 gRPC”以及两个合法的 gRPC-Web family，避免同类回归。🔧 建议修复
fn grpc_protocol_request(headers: &HeaderMap) -> bool { @@ let Ok(content_type) = content_type.to_str() else { return false; }; let (mime, _) = split_content_type(content_type); - mime.to_ascii_lowercase().starts_with(GRPC_CONTENT_TYPE_PREFIX) + let normalized_mime = mime.to_ascii_lowercase(); + let is_grpc = normalized_mime == GRPC_CONTENT_TYPE_PREFIX + || normalized_mime + .strip_prefix(GRPC_CONTENT_TYPE_PREFIX) + .is_some_and(|suffix| suffix.starts_with('+')); + is_grpc + || matches_grpc_web_mime(&normalized_mime, GRPC_WEB_CONTENT_TYPE_PREFIX) + || matches_grpc_web_mime(&normalized_mime, GRPC_WEB_TEXT_CONTENT_TYPE_PREFIX) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-http/src/proxy/forward/grpc.rs` around lines 62 - 63, The current check using split_content_type and starts_with(GRPC_CONTENT_TYPE_PREFIX) is too permissive and treats invalid MIME like "application/grpcweb" as gRPC; update the condition in the gRPC detection logic (the code using split_content_type and GRPC_CONTENT_TYPE_PREFIX) to explicitly match the native gRPC content-type and the allowed gRPC‑Web families instead of a bare starts_with: require mime == "application/grpc" (and optionally "application/grpc+proto"/"application/grpc+json" if you accept those variants) and explicitly allow the legitimate gRPC‑Web types such as "application/grpc-web", "application/grpc-web+proto" and "application/grpc-web+json"; replace the single starts_with check with these explicit equality/whitelist checks so invalid MIME like "application/grpcweb" are not treated as gRPC.
🧹 Nitpick comments (4)
crates/rginx-app/src/admin_cli/status.rs (1)
112-117: 建议抽出通用列表渲染助手，减少重复分支。当前同类
is_empty -> "-" / join逻辑在多个块重复，后续调整占位符或分隔符时容易漏改。♻️ 建议重构示例
AdminResponse::Status(status) => { + let render_list = |values: &[String], sep: &str| { + if values.is_empty() { + "-".to_string() + } else { + values.join(sep) + } + }; print_record( "status", [ @@ - ( - "san_dns_names", - if certificate.san_dns_names.is_empty() { - "-".to_string() - } else { - certificate.san_dns_names.join(",") - }, - ), + ("san_dns_names", render_list(&certificate.san_dns_names, ",")), @@ - ( - "server_names", - if binding.server_names.is_empty() { - "-".to_string() - } else { - binding.server_names.join(",") - }, - ), + ("server_names", render_list(&binding.server_names, ",")),Also applies to: 186-190, 228-232, 236-240, 244-248, 262-266, 270-274, 288-292, 296-300, 313-317, 321-325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/src/admin_cli/status.rs` around lines 112 - 117, Extract a small helper function (e.g., render_list_or_dash or fmt_list_or_dash) in crates/rginx-app/src/admin_cli/status.rs that takes a slice or iterator of strings and a separator and returns "-" if empty or the joined string otherwise; then replace the repeated patterns like certificate.san_dns_names.is_empty() ? "-" : certificate.san_dns_names.join(",") (and the other occurrences listed) with calls to that helper to centralize the empty-check and join logic and ensure consistent placeholder/separator behavior across the blocks.crates/rginx-app/tests/reload/reload_boundary.rs (1)
15-18: 可选：提取失败 reload 轮询辅助函数，减少重复断言字符串。当前相同 predicate 在多个测试重复，后续字段名调整时容易漏改。建议抽成一个小 helper。
♻️ 可选重构示例
+fn wait_for_single_failed_reload(server: &TestServer) -> String { + server.wait_for_status_output( + |stdout| stdout.contains("reload_attempts=1") && stdout.contains("reload_failures=1"), + Duration::from_secs(5), + ) +} + #[test] fn sighup_rejects_listen_address_changes() { @@ - let _stdout = server.wait_for_status_output( - |stdout| stdout.contains("reload_attempts=1") && stdout.contains("reload_failures=1"), - Duration::from_secs(5), - ); + let _stdout = wait_for_single_failed_reload(&server); @@ }Also applies to: 40-43, 63-66, 86-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/tests/reload/reload_boundary.rs` around lines 15 - 18, Extract the repeated predicate closure used with server.wait_for_status_output into a small helper (e.g., a function like assert_reload_stats or make_reload_predicate) that accepts expected attempts and failures and returns a closure or performs the assertion; replace the inline closures in tests (the ones passing |stdout| stdout.contains("reload_attempts=1") && stdout.contains("reload_failures=1") to server.wait_for_status_output) with calls to this helper so all tests (the occurrences around server.wait_for_status_output at the locations you noted) use the shared helper and avoid duplicating the literal field strings.crates/rginx-app/src/migrate_nginx/render.rs (1)
170-184:render_listener_tls与render_server_tls可合并，降低重复维护成本。两处实现完全一致，建议抽取公共函数，后续修改 TLS 输出时避免遗漏一处。
♻️ 建议重构
-fn render_listener_tls(out: &mut String, indent: &str, tls: Option<&ConvertedServerTls>) { - if let Some(tls) = tls { - let _ = writeln!(out, "{indent}tls: Some(ServerTlsConfig("); - render_server_tls_body(out, &format!("{indent} "), tls); - let _ = writeln!(out, "{indent})),"); - } -} - -fn render_server_tls(out: &mut String, indent: &str, tls: Option<&ConvertedServerTls>) { +fn render_tls_config(out: &mut String, indent: &str, tls: Option<&ConvertedServerTls>) { if let Some(tls) = tls { let _ = writeln!(out, "{indent}tls: Some(ServerTlsConfig("); render_server_tls_body(out, &format!("{indent} "), tls); let _ = writeln!(out, "{indent})),"); } } + +fn render_listener_tls(out: &mut String, indent: &str, tls: Option<&ConvertedServerTls>) { + render_tls_config(out, indent, tls); +} + +fn render_server_tls(out: &mut String, indent: &str, tls: Option<&ConvertedServerTls>) { + render_tls_config(out, indent, tls); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/src/migrate_nginx/render.rs` around lines 170 - 184, Both render_listener_tls and render_server_tls duplicate identical logic; extract a single helper like render_optional_server_tls(out: &mut String, indent: &str, tls: Option<&ConvertedServerTls>) that contains the writeln! calls and calls render_server_tls_body, then replace both render_listener_tls and render_server_tls to call that helper (or remove them and use the helper in their call sites). Ensure you reference render_server_tls_body inside the new helper and preserve the exact formatting and comma/paren placement currently emitted.crates/rginx-app/tests/admin.rs (1)
3-27: 把unused_imports的抑制范围再收窄一点。如果这些 import 主要是给拆分后的子模块通过
super::*共享作用域用的，建议只对那几个确实未在根文件直接使用的符号保留#[allow(unused_imports)]。现在像Command、Path、Duration、TcpListener、rcgen这些本文件 helper 已直接使用的 import 也一并被 suppress，后续真正的死 import 会更难暴露。也请顺手确认一下，哪些测试专属依赖其实可以直接下沉到对应子模块。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/rginx-app/tests/admin.rs` around lines 3 - 27, The current broad #[allow(unused_imports)] annotations hide real unused imports; narrow the suppression to only the truly unused symbols (e.g., apply #[allow(unused_imports)] directly to specific imports that are only referenced via super::* in child modules) and remove the attribute from imports that are actually used in this file such as Command, Path, Duration, TcpListener, rcgen items (e.g., CertificateParams, KeyPair) and UnixStream so the compiler will warn if they become dead; also consider moving test-only dependencies into their respective submodules instead of importing them at the root (inspect symbols like Command, Path, Duration, TcpListener, rcgen::{...}, UnixStream, admin_socket_path_for_config and only mark the few that remain unused with the attribute).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/rginx-app/src/migrate_nginx/render.rs`:
- Around line 21-23: 在将 self.warnings 输出为注释时当前直接写入 warning（writeln!(out, "// -
{}", warning)）会因为 warning 中包含 '\n' 导致生成未加注释前缀的新行，破坏输出格式；请在写入前对 warning
进行处理以防止换行逃逸注释块：要么以 '\n' 分割再逐行写入并在首行加 "// - "、后续行加 "// "，要么将所有 '\n' 替换为 "\n//
"（或等效前缀）再写入，从而保证每一行都被注释；修改位于 render.rs 中处理 self.warnings 的循环处（writeln!(out, "//
- {}", warning) 所在位置）。
In `@crates/rginx-app/src/migrate_nginx/tokenize.rs`:
- Around line 37-45: The '\\' escape arm currently discards the backslash for
non-newline escapes; change it so only recognized escapes (\\ -> backslash, '\"'
and '\'' -> respective quote) are converted, newline continues the line, and all
other cases preserve both the backslash and the following character. Update the
match arm that handles '\\' (the code using chars.next(), value, and line) to:
consume the next char, if it is '\n' increment line and continue; else if it is
'\\' push '\\'; else if it is '\"' or '\'' push that quote; otherwise push '\\'
then push the escaped char.
---
Duplicate comments:
In `@crates/rginx-http/src/proxy/forward/grpc.rs`:
- Around line 62-63: The current check using split_content_type and
starts_with(GRPC_CONTENT_TYPE_PREFIX) is too permissive and treats invalid MIME
like "application/grpcweb" as gRPC; update the condition in the gRPC detection
logic (the code using split_content_type and GRPC_CONTENT_TYPE_PREFIX) to
explicitly match the native gRPC content-type and the allowed gRPC‑Web families
instead of a bare starts_with: require mime == "application/grpc" (and
optionally "application/grpc+proto"/"application/grpc+json" if you accept those
variants) and explicitly allow the legitimate gRPC‑Web types such as
"application/grpc-web", "application/grpc-web+proto" and
"application/grpc-web+json"; replace the single starts_with check with these
explicit equality/whitelist checks so invalid MIME like "application/grpcweb"
are not treated as gRPC.
---
Nitpick comments:
In `@crates/rginx-app/src/admin_cli/status.rs`:
- Around line 112-117: Extract a small helper function (e.g.,
render_list_or_dash or fmt_list_or_dash) in
crates/rginx-app/src/admin_cli/status.rs that takes a slice or iterator of
strings and a separator and returns "-" if empty or the joined string otherwise;
then replace the repeated patterns like certificate.san_dns_names.is_empty() ?
"-" : certificate.san_dns_names.join(",") (and the other occurrences listed)
with calls to that helper to centralize the empty-check and join logic and
ensure consistent placeholder/separator behavior across the blocks.
In `@crates/rginx-app/src/migrate_nginx/render.rs`:
- Around line 170-184: Both render_listener_tls and render_server_tls duplicate
identical logic; extract a single helper like render_optional_server_tls(out:
&mut String, indent: &str, tls: Option<&ConvertedServerTls>) that contains the
writeln! calls and calls render_server_tls_body, then replace both
render_listener_tls and render_server_tls to call that helper (or remove them
and use the helper in their call sites). Ensure you reference
render_server_tls_body inside the new helper and preserve the exact formatting
and comma/paren placement currently emitted.
In `@crates/rginx-app/tests/admin.rs`:
- Around line 3-27: The current broad #[allow(unused_imports)] annotations hide
real unused imports; narrow the suppression to only the truly unused symbols
(e.g., apply #[allow(unused_imports)] directly to specific imports that are only
referenced via super::* in child modules) and remove the attribute from imports
that are actually used in this file such as Command, Path, Duration,
TcpListener, rcgen items (e.g., CertificateParams, KeyPair) and UnixStream so
the compiler will warn if they become dead; also consider moving test-only
dependencies into their respective submodules instead of importing them at the
root (inspect symbols like Command, Path, Duration, TcpListener, rcgen::{...},
UnixStream, admin_socket_path_for_config and only mark the few that remain
unused with the attribute).
In `@crates/rginx-app/tests/reload/reload_boundary.rs`:
- Around line 15-18: Extract the repeated predicate closure used with
server.wait_for_status_output into a small helper (e.g., a function like
assert_reload_stats or make_reload_predicate) that accepts expected attempts and
failures and returns a closure or performs the assertion; replace the inline
closures in tests (the ones passing |stdout|
stdout.contains("reload_attempts=1") && stdout.contains("reload_failures=1") to
server.wait_for_status_output) with calls to this helper so all tests (the
occurrences around server.wait_for_status_output at the locations you noted) use
the shared helper and avoid duplicating the literal field strings.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 528ca7ed-37c1-437e-9f13-0674f196417a
📒 Files selected for processing (14)
crates/rginx-app/src/admin_cli/status.rscrates/rginx-app/src/migrate_nginx/convert.rscrates/rginx-app/src/migrate_nginx/parser.rscrates/rginx-app/src/migrate_nginx/render.rscrates/rginx-app/src/migrate_nginx/tests.rscrates/rginx-app/src/migrate_nginx/tokenize.rscrates/rginx-app/tests/admin.rscrates/rginx-app/tests/downstream_mtls.rscrates/rginx-app/tests/grpc_proxy.rscrates/rginx-app/tests/reload.rscrates/rginx-app/tests/reload/reload_boundary.rscrates/rginx-app/tests/reload/restart_flow.rscrates/rginx-http/src/proxy/forward/grpc.rscrates/rginx-http/src/proxy/tests/grpc.rs
✅ Files skipped from review due to trivial changes (1)
- crates/rginx-app/tests/reload/restart_flow.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/rginx-app/src/migrate_nginx/convert.rs
- crates/rginx-app/tests/downstream_mtls.rs
- crates/rginx-app/src/migrate_nginx/tests.rs
Owner
Author
|
Addressed the remaining non-threaded review nits as well:
Verification run after the changes:
|
This was referenced Apr 11, 2026
Merged
This was referenced Apr 26, 2026
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
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.
Summary
Testing