[codex] fix Amp exported thread usage parsing - #19
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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.
Code Review
This pull request transitions the Amp client from scanning local JSON files to using the Amp CLI (amp threads list and amp threads export) for retrieving session data. Key changes include updating the Amp client configuration, modifying core parsing functions to return Result types for improved error handling, and implementing a new CLI-based parser in sessions/amp.rs. Feedback focuses on making the Amp CLI integration more robust by handling process failures gracefully, avoiding potential race conditions and resource leaks in temporary log file management, and optimizing performance by parallelizing thread exports.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/tokscale-core/src/clients.rs (1)
216-224: 💤 Low value建议添加注释说明 Amp 配置的特殊性。
Amp 的配置与其他客户端不同:
relative指向二进制文件而非数据目录,pattern为空。添加简短注释可以帮助未来维护者理解这是 CLI 采集方式的设计意图,而非配置错误。🤖 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-core/src/clients.rs` around lines 216 - 224, 在 clients.rs 中为 Amp = 5 的配置添加一行简短注释,说明该变体与其他客户端不同:relative 指向 CLI 二进制而不是数据目录,pattern 为空且 parse_local/headless/submit_default 的组合表示这是通过命令行采集(而非文件系统数据目录)的特殊配置;在 Amp、PathRoot::Home、relative 和 pattern 附近插入该注释以便未来维护者理解这是设计意图而非配置错误。crates/tokscale-core/src/sessions/amp.rs (1)
86-120: ⚖️ Poor tradeoff
run_amp缺少超时机制,可能导致无限阻塞。如果
ampCLI 因网络或服务端问题挂起,Command::output()会无限等待。考虑使用超时机制(例如通过wait_timeoutcrate 或tokio::time::timeout)来避免潜在的阻塞。🤖 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-core/src/sessions/amp.rs` around lines 86 - 120, run_amp currently uses Command::output() and can block indefinitely; change it to spawn the child (Command::spawn()) and implement a synchronous timeout (e.g. the wait_timeout crate) so the child is waited on only for a bounded duration, and if the timeout expires you kill the child and collect/return its output as an AmpCliError indicating a timeout. Specifically: replace the Command::output() usage in run_amp with Command::spawn(), use Child::wait_timeout(...) to wait for the configured duration, if it returns None call Child::kill() and then Child::wait() and read remaining stdout/stderr from the child's pipes, and finally return Err(AmpCliError::new(...)) describing the timeout (include AMP_COMMAND, args.join(" "), and available stdout/stderr); keep existing success path when Child exits with success. Ensure you enable/pipe stdout and stderr on spawn and add the wait_timeout dependency or equivalent.
🤖 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-core/src/clients.rs`:
- Around line 216-224: 在 clients.rs 中为 Amp = 5
的配置添加一行简短注释,说明该变体与其他客户端不同:relative 指向 CLI 二进制而不是数据目录,pattern 为空且
parse_local/headless/submit_default 的组合表示这是通过命令行采集(而非文件系统数据目录)的特殊配置;在
Amp、PathRoot::Home、relative 和 pattern 附近插入该注释以便未来维护者理解这是设计意图而非配置错误。
In `@crates/tokscale-core/src/sessions/amp.rs`:
- Around line 86-120: run_amp currently uses Command::output() and can block
indefinitely; change it to spawn the child (Command::spawn()) and implement a
synchronous timeout (e.g. the wait_timeout crate) so the child is waited on only
for a bounded duration, and if the timeout expires you kill the child and
collect/return its output as an AmpCliError indicating a timeout. Specifically:
replace the Command::output() usage in run_amp with Command::spawn(), use
Child::wait_timeout(...) to wait for the configured duration, if it returns None
call Child::kill() and then Child::wait() and read remaining stdout/stderr from
the child's pipes, and finally return Err(AmpCliError::new(...)) describing the
timeout (include AMP_COMMAND, args.join(" "), and available stdout/stderr); keep
existing success path when Child exits with success. Ensure you enable/pipe
stdout and stderr on spawn and add the wait_timeout dependency or equivalent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0046904f-2e3b-4958-8b99-611482899978
📒 Files selected for processing (5)
crates/tokscale-cli/src/main.rscrates/tokscale-core/src/clients.rscrates/tokscale-core/src/lib.rscrates/tokscale-core/src/scanner.rscrates/tokscale-core/src/sessions/amp.rs
|
Addressed the Amp client config nitpick by documenting that the configured path is only a local CLI marker and that usage is collected through I am intentionally not adding a timeout to |
junhoyeo#713) * feat(sessions): read Antigravity CLI usage from local SQLite databases The Antigravity CLI (the terminal agent that stores its data under `~/.gemini/antigravity-cli/`) was never counted. tokscale only knew two Gemini-family sources: the Gemini CLI (scans `~/.gemini/tmp/*.{json,jsonl}`) and Antigravity (pulls usage from a running IDE language server over RPC and caches it under the config dir). The Antigravity CLI fell into neither bucket, so its on-disk usage was invisible — `tokscale antigravity sync` found the filesystem candidates but cached zero because its only artifact path still requires a live language-server RPC connection. This adds Antigravity CLI as a first-class local scan source so its usage updates automatically like every other file-based source — no RPC, no `antigravity sync`. A new `antigravity-cli` client globs `~/.gemini/antigravity-cli/conversations/*.db` (honoring `GEMINI_CLI_HOME`) and a new parser reads each conversation database directly. Each `gen_metadata` row is one generation encoded as the same `GeneratorMetadata` protobuf the IDE returns over `GetCascadeTrajectoryGeneratorMetadata`. The repository has no `.proto`/prost decoder (the IDE path receives JSON because the language server does the proto-to-JSON conversion), so the parser ships a tiny dependency-free wire-format reader and pulls only the fields it needs. The field numbers were reverse-engineered from real databases and cross-checked across 6 sessions / 140 turns: `chatModel.#19` is the response model, `usage.#5`/`#9`/`#10` are cacheRead/output/thinking (verified by the invariant `#9 + #10 == #3`, the stored total output), `#11` is the responseId used for dedup, and input combines the fixed system-prompt count `#1` with the newly-processed input `#2`. The session timestamp and workspace come from `trajectory_metadata_blob`. Adding the new `ClientId` variant fans out to the usual registration points: the scanner gains a `*.db` glob arm (which naturally rejects `.db-wal`/`.db-shm` sidecars), both local-parse dispatch paths gain a branch, and the CLI `ClientFilter`, client labels, TUI picker, and frontend source maps gain entries. The deprecated per-client boolean flags intentionally do not, since `antigravity-cli` is reachable only via the canonical `--client antigravity-cli`. Closes junhoyeo#712. * fix(sessions): handle file:// authority/UNC paths and test Antigravity CLI wiring Addresses the cubic review on junhoyeo#713. `file_uri_to_path` previously stripped `file://` and only special-cased the leading slash before a Windows drive letter, so a non-empty authority (`file://host/share/...`, the UNC form) lost its host and collapsed into a bare path. It now treats an empty-authority remainder as before (`/C:/x` → `C:/x`, `/home/x` kept) and reconstructs a non-empty authority as a UNC path (`host/share/x` → `//host/share/x`) so `normalize_workspace_key` preserves the `//` prefix. A unit test covers the Windows-drive, POSIX, UNC, and percent-encoded-CJK cases. The new `AntigravityCli` client wiring is now asserted in `test_client_as_str`, `test_client_key`, and `test_client_from_key` (display name "Antigravity CLI", hotkey `f`, and the reverse hotkey mapping). * style: rustfmt antigravity_cli.rs * fix(antigravity-cli): add gemini-3-flash-a pricing alias and harden parser tests Map the raw #19 responseModel `gemini-3-flash-a` onto the priced `gemini-3-flash-preview` so Antigravity CLI cost no longer resolves to 0. Add alias-resolution, #9/#10==#3 field-mapping invariant, and malformed-protobuf bounds tests. Constraint: must not weaken the junhoyeo#707 brand-token fuzzy-match guard in lookup.rs Confidence: high Scope-risk: narrow --------- Co-authored-by: Junho Yeo <i@junho.io>
Summary
~/.local/share/amp/threads/T-*.jsonscanning withamp threads list/exportcollection.messages[].usagemodel/token/timestamp fields and let the existing pricing service calculate cost from tokens.tokscale clientsto show the Amp CLI export source instead of the removed local thread path.Root Cause
Amp no longer persists the token-bearing thread JSON files where tokscale expected them. The current usable data is available from
amp threads export, so the old scanner source reported zero sessions on machines with only the new Amp storage layout.Validation
cargo fmt --checkcargo test -q -p tokscale-corecargo test -q -p tokscale-clicargo run -q -p tokscale-cli -- --client amp --json --no-spinner-> 161 Amp messages, priced from model/token usagecargo run -q -p tokscale-cli -- clients-> Amp sourceamp threads list/export, 161 messagesSummary by CodeRabbit
发行说明
Bug 修复与改进
测试