refactor(cli): split command modules - #106
Conversation
Walkthrough本次变更将原本集中在 tokscale-cli 的 main.rs 中的命令逻辑，按功能拆分为 commands/ 目录下的多个模块（cache、clients、graph、headless、hourly、integrations、models、monthly、pricing、render、shared、time_metrics），并新增大量单元测试覆盖这些函数的解析与格式化逻辑。 ChangesCLI 命令模块拆分
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as run_headless_command
participant Capture as run_capture_command
participant Child as 子进程(codex)
participant File as 输出文件
CLI->>Capture: 启动并传入超时
Capture->>Child: spawn 子进程
Child-->>Capture: stdout 流
Capture->>File: 写入捕获内容
Capture-->>CLI: CaptureCommandOutcome(exit_code, timed_out)
CLI-->>CLI: 超时则 exit(124)，否则按 exit_code 处理
sequenceDiagram
participant Command as run_*_report
participant Shared as shared.rs
participant Runtime as tokio Runtime
participant CursorSync as cursor::sync
Command->>Shared: should_auto_sync_cursor_for_local_report
Shared-->>Command: true/false
Command->>Shared: auto_sync_cursor_for_local_report
Shared->>Runtime: build_runtime
Runtime->>CursorSync: run sync
CursorSync-->>Shared: SyncCursorResult
Shared-->>Command: warnings via emit_cursor_sync_warning
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
makoMakoGo
left a comment
makoMakoGo
left a comment
There was a problem hiding this comment.
按 #97 的决策记录看了这次拆分，整体方向符合“函数级平移 / main.rs 保留 clap+dispatch / 不引入 trait 层”。
需要先修一个迁移遗漏：main_tests.rs 里两个 macOS-only 的 load_star_cache 测试被原样搬出来了，但新的 test module 只 import 了 cache/clients/integrations/render/shared，main.rs 也不再把 load_star_cache 带进 super::*。这些测试在 Linux 不会编译到，所以当前 cargo test --workspace 可能看起来是绿的；在 macOS target 上会因找不到 load_star_cache 直接编译失败。
处理方式建议保持边界干净：把测试移动到现在真正拥有 load_star_cache 的模块，或显式用现在的 owner 路径；如果 #102 已经删除 hosted/star-cache surface，那就删掉这两条陈旧测试和相关文档引用，不要为旧 surface 做兜底。
9e4839d to
faaa7b0
Compare
makoMakoGo
left a comment
makoMakoGo
left a comment
There was a problem hiding this comment.
Retarget 后重新看了一遍。
clients 拆分现在已经对齐 personal/local-clients baseline：用的是 count_local_client_messages，没有把旧的 parse_local_clients 路径带回来，这块 OK。
仍需要修一个 blocker：main_tests.rs 里还保留了两个 macOS-only load_star_cache 测试。这个 PR 的新 main.rs 不再拥有/导入 load_star_cache，所以 Linux 上会因为 cfg 没暴露而绿，macOS target 会在编译这些测试时找不到符号。既然 retarget 到 local-clients 后 hosted/star-cache surface 已经不是这个 PR 的职责，最干净的处理是删除这两条陈旧测试；不要为了它在 main.rs 或 main_tests.rs 重新兜底引入旧 surface。
faaa7b0 to
22b3221
Compare
makoMakoGo
left a comment
makoMakoGo
left a comment
There was a problem hiding this comment.
Re-reviewed after the cleanup.
The previous blocker is fixed: the obsolete macOS load_star_cache migration tests are gone from main_tests.rs, and there is no new import/fallback/compat bridge added for that old surface. I resolved the old inline thread.
The retargeted clients split still stays on count_local_client_messages, and the changed-file set remains scoped to the CLI split. From this review pass, this looks good to merge once you are ready to move the PR out of draft / after your normal validation gates pass.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 15 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
makoMakoGo
left a comment
makoMakoGo
left a comment
There was a problem hiding this comment.
Reviewed after moving out of draft.
LGTM for #97. The previous blocker is fixed: the obsolete macOS load_star_cache tests are gone, with no new import/fallback/compat bridge. clients remains on count_local_client_messages, main.rs stays at the intended clap + dispatch shape, and the PR remains scoped to the CLI split.
Cubic opened a few new threads after draft removal. I checked the important ones against the personal/local-clients base: they are pre-existing behavior moved out of main.rs, not regressions introduced by this split. They can be tracked separately if desired, but I would not block this refactor PR on them.
GitHub does not allow me to formally approve my own PR, so leaving this as a review comment rather than an approval.
makoMakoGo
left a comment
makoMakoGo
left a comment
There was a problem hiding this comment.
Cubic triage after ready-for-review:
-
commands/pricing.rs:107— reject. Theunwrap_or(0.0)behavior for base input/output pricing is intentional for the local CLI pricing contract: a resolved pricing record has numeric base pricing fields, and missing upstream values are normalized to zero for local cost math/reporting. Cache read/write pricing remains optional because it is an extra pricing lane, not the base lane. Changing input/output toOption<f64>would be a JSON output contract change, not a refactor, and contradicts the current design decision. I am resolving this thread. -
commands/pricing.rs:30— design decision, not a split regression. If we want to improve it, the clean choice is not a tiny branch patch; decide whether invalid--provideris a clap argument error or a JSON-mode command error, then make that contract explicit with tests. -
commands/clients.rs:14— real edge case, but Cubic's suggested fallback is the wrong shape.--home ""is an explicit invalid override, not an omitted flag. Clean fix: validate the global--homevalue at the CLI boundary and reject empty/whitespace-only values, instead of silently falling back to the real home. -
commands/render.rs:215— real bug in the formatter for negative values with digit counts divisible by three. Clean fix: handle the sign separately and add the missing-123,-123456, andi64::MIN-safe coverage. -
commands/integrations.rs:64— valid cleanup. Clean fix: construct the Tokio runtime only in branches that actually call async Trae operations; manual login/logout/status should not inherit runtime creation as a failure surface. -
main_tests.rs:311— valid cleanup. Delete the duplicate default-client test and keep one canonical assertion. -
commands/shared.rs:603— valid cleanup. Match the existing env-root convention: a blank/whitespace-onlyTOKSCALE_HEADLESS_DIRshould be treated like unset, with a focused unit test.
22b3221 to
212d84b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
crates/tokscale-cli/src/main_tests.rs (1)
778-793: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win环境变量清理未做异常安全处理
antigravity_cli_conversations_path_falls_back_for_blank_env和headless_roots_ignore_blank_env_override都是先set_var→ 执行断言 → 最后才恢复原值。如果中间的assert_eq!/assert!失败导致 panic，恢复逻辑不会执行，GEMINI_CLI_HOME/TOKSCALE_HEADLESS_DIR会保持被污染的状态，进而可能导致同一测试进程中后续测试（即使标了#[serial]，也只能防止并发写入，不能防止 panic 后状态泄漏）产生级联失败，增加排查难度。建议使用 RAII guard（在
Drop::drop中恢复）或std::panic::catch_unwind包裹断言部分，确保无论成功还是 panic 都能恢复环境变量。♻️ 建议修复方向（以 antigravity_cli_conversations_path_falls_back_for_blank_env 为例）
+struct EnvVarGuard { + key: &'static str, + previous: Option<String>, +} + +impl EnvVarGuard { + fn set(key: &'static str, value: &str) -> Self { + let previous = std::env::var(key).ok(); + unsafe { std::env::set_var(key, value) }; + Self { key, previous } + } +} + +impl Drop for EnvVarGuard { + fn drop(&mut self) { + match &self.previous { + Some(value) => unsafe { std::env::set_var(self.key, value) }, + None => unsafe { std::env::remove_var(self.key) }, + } + } +} + #[test] #[serial_test::serial] fn antigravity_cli_conversations_path_falls_back_for_blank_env() { - let previous = std::env::var("GEMINI_CLI_HOME").ok(); - unsafe { std::env::set_var("GEMINI_CLI_HOME", " ") }; + let _guard = EnvVarGuard::set("GEMINI_CLI_HOME", " "); assert_eq!( antigravity_cli_conversations_path("/tmp/home", true), PathBuf::from("/tmp/home/.gemini/antigravity-cli/conversations") ); - - match previous { - Some(value) => unsafe { std::env::set_var("GEMINI_CLI_HOME", value) }, - None => unsafe { std::env::remove_var("GEMINI_CLI_HOME") }, - } }Also applies to: 795-812
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tokscale-cli/src/main_tests.rs` around lines 778 - 793, The test setup in antigravity_cli_conversations_path_falls_back_for_blank_env and headless_roots_ignore_blank_env_override mutates process environment state and only restores it after the assertions, so a panic can leak GEMINI_CLI_HOME or TOKSCALE_HEADLESS_DIR into later tests. Update the test helpers around antigravity_cli_conversations_path and headless_roots_ignore_blank_env_override to restore the original env value via RAII (a guard with Drop) or another panic-safe cleanup approach, so the cleanup always runs even if an assertion fails.crates/tokscale-cli/src/commands/shared.rs (1)
115-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value建议用
ClientId::as_str()替代硬编码字符串比较。
client_filter_includes_cursor、client_filter_explicitly_requests_cursor、client_filter_explicitly_requests_warp三个函数都用字面量"cursor"/"warp"做比较，而不是复用ClientId::Cursor.as_str()/ClientId::Warp.as_str()。如果未来ClientId::as_str()的输出发生变化，这里会静默失效而不报编译错误。♻️ 建议改动示例
pub(crate) fn client_filter_includes_cursor(clients: &Option<Vec<String>>) -> bool { clients .as_ref() - .is_none_or(|sources| sources.iter().any(|source| source == "cursor")) + .is_none_or(|sources| sources.iter().any(|source| source == ClientId::Cursor.as_str())) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tokscale-cli/src/commands/shared.rs` around lines 115 - 131, The three client filter helpers in shared.rs currently compare against hardcoded "cursor" and "warp" strings, so update client_filter_includes_cursor, client_filter_explicitly_requests_cursor, and client_filter_explicitly_requests_warp to use ClientId::Cursor.as_str() and ClientId::Warp.as_str() instead. Keep the existing Option<Vec<String>> logic intact, but route all matching through the ClientId enum’s as_str() so the behavior stays aligned with ClientId if its string representation changes.crates/tokscale-cli/src/commands/integrations.rs (1)
62-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win多处重复创建
tokio::runtime::Runtime::new()执行单个异步调用Line 101 与 Line 180 各自新建了一个完整的多线程
Runtime仅用于执行一次block_on调用，commands/graph.rs（Line 233-248）和commands/pricing.rs（Line 32-36）中也是同样的模式。这是跨文件重复的样板代码，且默认的多线程 runtime 对单次异步调用来说存在不必要的线程池创建开销。可以在共享模块中提取一个fn block_on_async<F: Future>(fut: F) -> Result<F::Output>之类的辅助函数，内部使用Builder::new_current_thread().enable_all().build()?，供各命令模块复用。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tokscale-cli/src/commands/integrations.rs` around lines 62 - 193, Repeated ad hoc creation of tokio::runtime::Runtime in run_trae_command and similar command handlers is unnecessary and duplicated across modules. Extract a shared helper such as block_on_async in a common commands/util module, and have run_trae_command, graph.rs, and pricing.rs call it instead of building a new multi-thread runtime for each single block_on use. Implement the helper with a current-thread runtime via Builder::new_current_thread().enable_all().build()? and return the wrapped async result consistently.crates/tokscale-cli/src/commands/clients.rs (1)
299-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value基于字符串硬编码判断
amp客户端，建议改用枚举比较
row.client == "amp"依赖ClientId::as_str()的具体字符串值来判断展示文案（"source" vs "sessions"），一旦该字符串常量发生变化容易悄悄失效且不会有编译期提示。可以在构造ClientRow时（Line 117-260 循环内，此时仍持有client: ClientId）直接按枚举比较并把结果存入结构体字段，而不是在渲染阶段对字符串做二次判断。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tokscale-cli/src/commands/clients.rs` around lines 299 - 306, The display-label logic is still branching on the string value of row.client, which should be replaced with enum-based selection. Move this decision into the ClientRow construction path in the clients loop where ClientId is still available, store the derived label field on ClientRow, and have the rendering code only read that field instead of checking row.client == "amp". Use ClientId and ClientRow as the key symbols to update.crates/tokscale-cli/src/commands/pricing.rs (1)
15-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
model_id参数复用为子命令分发标记（"list-overrides"）将
list-overrides作为特殊的model_id值来分发到run_pricing_list_overrides，这种基于魔法字符串的函数内部分发方式略显隐晦，容易与真实模型 ID 冲突（虽然概率很低）。如果 CLI 层已有独立的子命令定义这只是别名兼容处理，可以保留；否则更清晰的做法是在 clap 子命令层面区分。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tokscale-cli/src/commands/pricing.rs` around lines 15 - 17, The current `pricing` command handler uses the `model_id` parameter in `run_pricing` as a magic dispatch flag for `"list-overrides"`, which can conflict with real model IDs. Update the CLI flow so `run_pricing_list_overrides` is selected through a dedicated clap subcommand or explicit command variant instead of branching inside `run_pricing`; if this value must remain for backward compatibility, keep it only as an alias path and make the main dispatch use the named command structure.crates/tokscale-cli/src/commands/models.rs (2)
19-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win四个报表命令存在大量重复的启动样板代码。
run_models_report中 spinner 启动、auto_sync_cursor_for_local_report、setup_warnings_for_report、use_env_roots的调用顺序，与monthly.rs、hourly.rs、time_metrics.rs中几乎完全一致（仅细节参数不同）。这种跨四个文件的重复逻辑意味着未来任何一处 cursor 同步/警告逻辑变更都需要同步修改四处，容易产生行为分叉风险。建议将这段样板逻辑抽取为
commands/shared.rs中的一个统一辅助函数（例如返回一个包含 spinner、cursor_sync_result、setup_warnings、use_env_roots 的上下文结构体），供四个命令复用。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tokscale-cli/src/commands/models.rs` around lines 19 - 52, The startup boilerplate in run_models_report is duplicated across the report commands, including spinner setup, auto_sync_cursor_for_local_report, setup_warnings_for_report, and use_env_roots. Extract this shared sequence into a reusable helper in commands/shared.rs that returns a small context object (for example, with spinner, cursor_sync_result, setup_warnings, and use_env_roots), and update run_models_report plus the matching monthly, hourly, and time_metrics command paths to call that helper instead of repeating the same initialization logic.
36-38: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win将一次性 CLI runtime 改成 current_thread
run_models_report这里只做一次block_on，monthly.rs/hourly.rs/time_metrics.rs也一样；统一改用tokio::runtime::Builder::new_current_thread().enable_all().build()?，可以减少线程池初始化开销。tokscale_core里的tokio::join!和tokio::spawn不会阻止这个切换。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tokscale-cli/src/commands/models.rs` around lines 36 - 38, The one-shot CLI runtime setup in run_models_report still uses Runtime, but it only performs a single block_on; switch this and the similar monthly.rs, hourly.rs, and time_metrics.rs paths to tokio::runtime::Builder::new_current_thread().enable_all().build()? to avoid thread-pool overhead. Update the runtime creation code in the affected command entrypoints while keeping the existing block_on logic and leaving tokscale_core usage (including tokio::join! and tokio::spawn) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/tokscale-cli/src/commands/clients.rs`:
- Around line 299-306: The display-label logic is still branching on the string
value of row.client, which should be replaced with enum-based selection. Move
this decision into the ClientRow construction path in the clients loop where
ClientId is still available, store the derived label field on ClientRow, and
have the rendering code only read that field instead of checking row.client ==
"amp". Use ClientId and ClientRow as the key symbols to update.
In `@crates/tokscale-cli/src/commands/integrations.rs`:
- Around line 62-193: Repeated ad hoc creation of tokio::runtime::Runtime in
run_trae_command and similar command handlers is unnecessary and duplicated
across modules. Extract a shared helper such as block_on_async in a common
commands/util module, and have run_trae_command, graph.rs, and pricing.rs call
it instead of building a new multi-thread runtime for each single block_on use.
Implement the helper with a current-thread runtime via
Builder::new_current_thread().enable_all().build()? and return the wrapped async
result consistently.
In `@crates/tokscale-cli/src/commands/models.rs`:
- Around line 19-52: The startup boilerplate in run_models_report is duplicated
across the report commands, including spinner setup,
auto_sync_cursor_for_local_report, setup_warnings_for_report, and use_env_roots.
Extract this shared sequence into a reusable helper in commands/shared.rs that
returns a small context object (for example, with spinner, cursor_sync_result,
setup_warnings, and use_env_roots), and update run_models_report plus the
matching monthly, hourly, and time_metrics command paths to call that helper
instead of repeating the same initialization logic.
- Around line 36-38: The one-shot CLI runtime setup in run_models_report still
uses Runtime, but it only performs a single block_on; switch this and the
similar monthly.rs, hourly.rs, and time_metrics.rs paths to
tokio::runtime::Builder::new_current_thread().enable_all().build()? to avoid
thread-pool overhead. Update the runtime creation code in the affected command
entrypoints while keeping the existing block_on logic and leaving tokscale_core
usage (including tokio::join! and tokio::spawn) unchanged.
In `@crates/tokscale-cli/src/commands/pricing.rs`:
- Around line 15-17: The current `pricing` command handler uses the `model_id`
parameter in `run_pricing` as a magic dispatch flag for `"list-overrides"`,
which can conflict with real model IDs. Update the CLI flow so
`run_pricing_list_overrides` is selected through a dedicated clap subcommand or
explicit command variant instead of branching inside `run_pricing`; if this
value must remain for backward compatibility, keep it only as an alias path and
make the main dispatch use the named command structure.
In `@crates/tokscale-cli/src/commands/shared.rs`:
- Around line 115-131: The three client filter helpers in shared.rs currently
compare against hardcoded "cursor" and "warp" strings, so update
client_filter_includes_cursor, client_filter_explicitly_requests_cursor, and
client_filter_explicitly_requests_warp to use ClientId::Cursor.as_str() and
ClientId::Warp.as_str() instead. Keep the existing Option<Vec<String>> logic
intact, but route all matching through the ClientId enum’s as_str() so the
behavior stays aligned with ClientId if its string representation changes.
In `@crates/tokscale-cli/src/main_tests.rs`:
- Around line 778-793: The test setup in
antigravity_cli_conversations_path_falls_back_for_blank_env and
headless_roots_ignore_blank_env_override mutates process environment state and
only restores it after the assertions, so a panic can leak GEMINI_CLI_HOME or
TOKSCALE_HEADLESS_DIR into later tests. Update the test helpers around
antigravity_cli_conversations_path and headless_roots_ignore_blank_env_override
to restore the original env value via RAII (a guard with Drop) or another
panic-safe cleanup approach, so the cleanup always runs even if an assertion
fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 127259f9-c885-409d-add7-92832c0bda08
📒 Files selected for processing (15)
crates/tokscale-cli/src/commands/cache.rscrates/tokscale-cli/src/commands/clients.rscrates/tokscale-cli/src/commands/graph.rscrates/tokscale-cli/src/commands/headless.rscrates/tokscale-cli/src/commands/hourly.rscrates/tokscale-cli/src/commands/integrations.rscrates/tokscale-cli/src/commands/mod.rscrates/tokscale-cli/src/commands/models.rscrates/tokscale-cli/src/commands/monthly.rscrates/tokscale-cli/src/commands/pricing.rscrates/tokscale-cli/src/commands/render.rscrates/tokscale-cli/src/commands/shared.rscrates/tokscale-cli/src/commands/time_metrics.rscrates/tokscale-cli/src/main.rscrates/tokscale-cli/src/main_tests.rs
makoMakoGo
left a comment
makoMakoGo
left a comment
There was a problem hiding this comment.
Reviewed head 212d84b4.
LGTM. The Cubic cleanup pass is now resolved cleanly:
--homeempty/blank is rejected at the clap boundary instead of falling through to path handling.pricing --provideris now a clap-level enum-like parser error, so invalid providers do not enter runtime lookup or JSON command-error handling.format_tokens_with_commasnow handles the sign separately withunsigned_abs(), includingi64::MINcoverage.- Trae runtime creation is no longer eager for manual login/logout/status.
- The duplicate default-client test is gone.
- Blank
TOKSCALE_HEADLESS_DIRfollows the existing blank-env semantics and falls back to default roots.
All Cubic threads are resolved. This still reads as a scoped #97 CLI split, with the review cleanups folded in without compatibility bridges or fallback behavior.
1 participant
Summary
commands/modules for models, monthly, hourly, clients, pricing, and related command surfaces.main.rs.main.rsunit tests intomain_tests.rswhile leavingcrates/tokscale-cli/tests/cli_tests.rsunchanged.clientscommand on the currentcount_local_client_messagespath from R2: 删除 ParsedMessage 遗留管线 #93/perf(cli): route wrapped and clients through cached folds #101.--home, pricing provider parsing, signed token formatting, Trae runtime creation, duplicate tests, and blankTOKSCALE_HEADLESS_DIR.Closes #97
Built on the current
personal/local-clientsbaseline.Validation
cargo fmt --all --checkcargo clippy --workspace --all-targetscargo test --workspace(1886 passed, 4 ignored)cargo test -p tokscale-cli --bin tokscale main_tests(96 passed, 602 filtered out)wc -l crates/tokscale-cli/src/main.rs-> 881git diff -- crates/tokscale-cli/tests/cli_tests.rs-> no diffrg -n "parse_local_clients|ParsedMessage|ParsedMessages" crates/tokscale-cli/src crates/tokscale-core/srcreturns no matchesrg -n "load_star_cache|star-cache|star cache|star_cache" crates/tokscale-cli/src/main_tests.rsreturns no matchesSummary by CodeRabbit
新功能
修复