fix(tokenless): fail compare on missing sessions - #2674
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45d5fe9e66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Forrest-ly
left a comment
There was a problem hiding this comment.
Code Review — commit 45d5fe9
Overall
The core fix is correct and well-tested. stats summary --compare now fails closed on missing sessions, aligned with stats diff --session (same No records found for … message style, exit code 1, plain-text stderr error even in --json mode) and with the Python TokenlessStats.compare (StatsNotFoundError). Verified locally at head 45d5fe9:
cargo fmt --all --check✅cargo clippy --workspace --locked -- -D warnings✅ (0 errors)cargo test --workspace --locked -- --test-threads=1✅ (588 passed, 2 ignored)- Manual binary check:
stats summary --compare missing-a missing-b→Error: No records found for baseline session "missing-a" and tokenless session "missing-b", exit=1, no 0% report on stdout ✅
Positives:
- The empty-side check is placed before
warn_mode_mismatch, so missing sessions don't also emit spurious mode warnings. - Session IDs are
{…:?}-debug-escaped in the error, preventing terminal escape-sequence injection — consistent withstats diff, and covered bymissing_compare_sessions_debug_escapes_control_chars. - Test coverage is thorough: helper unit tests (both-present / both-missing / one-side-missing / control chars),
run_commandtests (empty text+JSON now fail closed, one-side-missing, populated success), and binary-level integration tests against an isolated DB with numeric JSON assertions. - Docs updated consistently in both en and zh, matching the new behavior.
- No other in-repo callers rely on the old 0% behavior.
One issue (P2, corroborates the existing bot comment)
--limit 0 produces a false-positive "No records found" — src/tokenless/crates/tokenless-cli/src/main.rs:185
The Summary --limit arg is a bare Option<usize> without the parse_positive_usize value parser that Diff.limit uses (main.rs:224). Since records_by_session applies a SQL LIMIT ?, invoking with --limit 0 returns empty vectors for populated sessions, and the new check then reports them as missing. Reproduced at head:
$ tokenless stats summary --limit 0 --compare base-run active-run # both sessions populated
Error: No records found for baseline session "base-run" and tokenless session "active-run"
exit=1
This is exactly the failure class this PR eliminates (a misleading compare result), just gated behind an unusual flag combination. Suggested fix — reuse the existing parser, one line:
#[arg(long, value_parser = parse_positive_usize)]
limit: Option<usize>,(Alternatively, determine session existence with an unlimited query, but the parser matches stats diff and also fixes --limit 0 for the plain summary.)
I agree with the bot's P2 on this; recommend addressing it in this PR since the change is trivial, but it is non-blocking — the common paths are correct and well covered.
Verdict: COMMENT (non-blocking). Nice work on the fail-closed alignment and the test matrix.
Forrest-ly
left a comment
There was a problem hiding this comment.
总体评价
本 PR 将 stats summary --compare 对缺失 session 的处理从「静默输出 0% 报告」改为 fail-closed(exit 1 + No records found for …),与 stats diff --session 及 Python TokenlessStats.compare 的既有语义对齐。改动聚焦、测试与文档配套完整。已在本地基于 head 45d5fe9 验证:cargo fmt --all --check、cargo clippy --workspace --locked -- -D warnings、cargo test -p tokenless-cli --locked -- --test-threads=1(264 passed, 2 ignored)全部通过;手工端到端复现确认两侧缺失 / 单侧缺失时正确失败,两侧均有记录时比较输出不受影响。
审查结论
approve
详细意见
🔴 必须修改(阻塞合并)
- 无
🟡 建议修改(不阻塞但推荐)
- [src/tokenless/crates/tokenless-cli/src/main.rs:185, 702] (补充 Codex review) 已有 Codex P2 review 指出的
--limit 0误报问题,已在本地确认可复现:两个 session 均有记录时,tokenless stats summary --limit 0 --compare base-s act-s报Error: No records found for baseline session "base-s" and tokenless session "act-s"(exit 1)。根因是Summary的--limit(main.rs:185)未做正数校验,records_by_session执行 SQLLIMIT 0返回空集,导致新增检查误判;而同文件Diff的--limit已使用parse_positive_usize(main.rs:224、254)。建议本 PR 直接复用该 parser(或按原 review 建议独立探测 session 是否存在)。该问题已被已有 review 覆盖,此处仅为复现确认与具体修法补充,不作为新增问题、不影响本次结论;建议合并前处理。
🟢 值得肯定
- fail-closed 语义对齐到位:错误消息结构
No records found for …、exit code 1、session ID 的 debug 转义({:?})与stats diff --session(main.rs:776)一致;「baseline session … and tokenless session …」的缺失侧拼接方式也与 Python 端StatsNotFoundError(src/tokenless/python/tokenless/python/anolisa_tokenless/stats.py:377)同构,并附有控制字符注入防护的专项测试。 - 检查点放置在查询之后、
warn_mode_mismatch与格式化输出之前,保证--json模式下也不会泄漏 0% 报告,精确满足 issue #2673 的预期。 - 测试覆盖完整:单元测试覆盖 both-missing / baseline-only-missing / tokenless-only-missing / both-populated 四象限及控制字符转义;集成测试用隔离
TOKENLESS_STATS_DB驱动真实二进制并断言 exit code 与 stdout/stderr;两个既有空 session 测试被正确改写为期望失败,正常路径由新增 populated 测试接管,无覆盖损失。 - 新增检查复用已查询的记录,不引入任何额外 DB 查询;en/zh 用户文档同步准确、位置恰当。
Forrest-ly
left a comment
There was a problem hiding this comment.
Re-review of head 1ceb816 (incremental diff vs previously approved 45d5fe9).
Earlier P2 (--limit 0 false missing-session report) — verified fixed:
stats summary --limitnow usesparse_positive_usize(src/tokenless/crates/tokenless-cli/src/main.rs:184), the same parser asstats diff --limit(defined atmain.rs:254).--limit 0is rejected at clap parse time with "value must be greater than zero", so the SQLLIMIT 0empty-result path is no longer reachable.records_by_sessionappliesLIMIT nper session (tokenless-stats/src/recorder.rs:253) andn >= 1is now guaranteed, so an empty vector unambiguously means "no records for this session". The missing-session fail-closed contract is intact: the incremental diff touches only the arg attribute in main.rs; themissing_compare_sessionscheck ordering, control-char debug escaping, and exit-1 behavior are unchanged.- Coverage is present: parse-level
stats_summary_cli_rejects_zero_limit(plain summary, summary +--compare, plus a--limit 1positive control) and binary-levelstats_summary_compare_rejects_zero_limit, which asserts populated--compare+--limit 0fails with "greater than zero" and emits neither "No records found" nor a report.
Local gates re-run independently at head 1ceb816: cargo fmt --all --check clean; cargo clippy -p tokenless-cli --locked -- -D warnings clean; cargo test -p tokenless-cli --locked — 224 passed / 2 ignored (unit) + 42 passed (integration), 0 failed.
Note: no CI workflow runs had been triggered on this head at review time (only license/cla passing). Non-blocking for this review.
LGTM — approving.
Forrest-ly
left a comment
There was a problem hiding this comment.
Code review 通过(AI Code Review,Multica PR-Review AGE-3827,head 1ceb816)。
已在本地对 head 1ceb816 完整编译并运行 tokenless-cli 测试套件:266 tests passed / 0 failed(224 单元 + 42 集成,含本 PR 新增的 10 个 compare 用例与 2 个 zero-limit 用例)。
审查要点:
- 正确性:
missing_compare_sessions检查位于两次records_by_session查询之后、任何 text/JSON 输出与warn_mode_mismatch之前,text 与--json两条路径均不会在缺 session 时输出 0% 报告,fail-closed 契约成立。--compare的num_args = 2保证sessions[0]/sessions[1]索引安全。 --limit 0误报(此前 P2 意见):已通过复用parse_positive_usize在解析期拒绝,与stats diff一致。由于records_by_session使用 SQLLIMIT n(n≥1 或默认 10000),非空 session 不会再被误判为 missing。单元 + 集成测试均已覆盖并本地通过。- 安全性:session ID 使用
{:?}Debug 格式输出,控制字符被转义,避免构造 session ID 注入终端转义序列;与stats diff --session行为一致,并有专门测试验证。 - 一致性:错误文案
No records found for ...、exit code 1 与stats diff --session及 CLI 现有错误约定一致;EN/ZH 文档同步更新且表述准确。
无阻塞问题,approve。
ikunkun-sys
left a comment
There was a problem hiding this comment.
审查结论
REQUEST_CHANGES
必须修改
- 本 PR 的公开 CLI 行为变更尚未完整同步到仓库要求的文档入口,详见 inline comment。
合并前提交整理
- 当前第二个提交修复的是第一个提交在本 PR 内引入的
--limit 0边界回归。请按AGENTS.md§13 的 same-PR fixup 规则,在合并前将两个提交 squash 为一个原子提交。
验证
- Review anchor:
1ceb816c5a2c86256f4b1696175b05deec843036 cargo +1.89.0 fmt --all -- --checkcargo +1.89.0 clippy --workspace --locked -- -D warningscargo +1.89.0 test --workspace --locked -- --test-threads=1(590 passed,2 ignored)- 4 个
stats summary --compare二进制定向场景通过
远端当前仅有 license/cla 成功,尚无实际 CI workflow 结果。
Forrest-ly
left a comment
There was a problem hiding this comment.
总体评价
对 head 0a65c24 的 re-review:相对已审查通过的 1ceb816,增量改动为纯文档(6 个文件,无代码变更)。文档改动在 specs/documentation-standard.md §5 要求的两个入口(组件 README 摘要 + 用户指南完整参考)完整、准确地覆盖了本 PR 的两条公开 CLI 契约,上一轮 CHANGES_REQUESTED 中的文档缺口已解决;本地 fmt/clippy/test 全部通过。
审查结论
approve
详细意见
🔴 必须修改(阻塞合并)
- 无
🟡 建议修改(不阻塞但推荐)
- [提交历史](补充 ikunkun-sys 既有 review)AGENTS.md §13 same-PR fixup 规则与 §6 原子性要求("Squash fixup commits before merge"、"One commit = one logical change")对当前 3-commit 历史仍然适用:
1ceb816修复的是45d5fe9在本 PR 内引入的--limit 0边界回归,按规则应 fixup 进源提交;新增的0a65c24文档化的是同一用户可见行为变更,与前两个提交共同构成一个逻辑变更,建议合并前整理为一个原子提交。作者已声明本次运行不被授权 rebase/force-push 该 PR,并提议由 GitHub squash-on-merge 达成单提交——squash-on-merge 可使 main 上得到单一原子提交,但不改写分支历史,是否满足 §13 请人类 reviewer/维护者裁定;合并前请二选一(rebase --autosquash 整理提交,或确认接受 squash-on-merge)。
🟢 值得肯定
- 两条公开契约在全部要求入口完整覆盖且中英文对齐:
src/tokenless/README.md/README_zh.md新增stats summary用法示例并说明两条契约;docs/user-guide/{en,zh}/token-saving/tokenless/cli-reference.md与measuring-savings.md分别在--limit说明与--compare注意事项中补齐正整数契约,位置符合文档规范 §3.1/§5。 - 文档内容已逐条对照代码核实:
Summary.limit使用parse_positive_usize(src/tokenless/crates/tokenless-cli/src/main.rs:184,--limit 0解析期即拒绝);missing_compare_sessions检查位于任何 text/JSON 输出与warn_mode_mismatch之前(main.rs:702-709、994-1007);records_by_session的 LIMIT 恒为 n≥1 或默认 10,000(tokenless-stats/src/recorder.rs:189,260),空结果集必然表示该 session 无记录;"与stats diff --limit一致"属实(main.rs:223-224 同样使用parse_positive_usize)。 - 全库复查未发现其他与
--compare/--limit新行为冲突或过时的文档描述(framework-integration.md仅涉及 Python 客户端TokenlessStats.compare,其本就 fail-closed;user-manual.md为泛指表述)。 - 本地门禁在 head
0a65c24复跑:cargo fmt --all --check通过;cargo clippy -p tokenless-cli --locked --all-targets -- -D warnings零告警;cargo test -p tokenless-cli --locked -- --test-threads=1→ 266 passed / 2 ignored / 0 failed,含本 PR 新增的全部 compare 与 zero-limit 用例(7 个单元 + 4 个集成,与1ceb816轮次计数一致,确认文档提交未引入任何行为变化)。
Forrest-ly
left a comment
There was a problem hiding this comment.
Code review 通过(head 0a65c24)。本次新增 commit 为 docs-only(仅 6 个 markdown 文件),补齐了上一轮 REQUEST_CHANGES 指出的两项公开 CLI 契约文档缺口。
核对结果(docs 与 CLI 实际行为一致性):
--compare缺失 session fail-closed:在隔离环境实际运行tokenless stats summary --compare missing-a missing-b,输出Error: No records found for baseline session "missing-a" and tokenless session "missing-b",exit=1,与 README(en/zh)及 user-guide measuring-savings(en/zh)新增描述一致。--limit 0解析期拒绝:实际运行tokenless stats summary --limit 0,clap 报invalid value '0' for '--limit <LIMIT>': value must be greater than zero,exit=2(非零),与文档"解析阶段被拒绝 / rejected at parse time"描述一致。- 文档中"行为与
stats diff --session/stats diff --limit一致"的对照准确:stats diff --session无记录时报No records found for ...(main.rs:776),stats diff --limit同样使用parse_positive_usize(main.rs:224)。 - EN/ZH 页面语义等价、命令示例完全相同,符合 specs/documentation-standard.md 与 CONTRIBUTING.md 要求;CLI flag 变更所需的文档入口(component README + user-guide 页面)均已覆盖。CHANGELOG 按仓库惯例由 release version-bump PR 汇总,本 PR 无需添加。
验证(head 0a65c24):
cargo test -p tokenless-cli --locked:266 passed, 2 ignored(含新增的 compare fail-closed 与 zero-limit 单元/集成测试)cargo fmt --all --check:cleancargo clippy --workspace --locked -- -D warnings:通过
代码部分(45d5fe9、1ceb816)此前已审并获批准;missing_compare_sessions 位置正确(双侧查询后、mode-mismatch 告警与报表输出前),session ID 经 {:?} debug 转义防止控制字符注入,测试覆盖双侧缺失/单侧缺失/双侧有记录(text+JSON)/zero-limit 等场景。
0a65c24 to
72e92ed
Compare
|
@ikunkun-sys @Forrest-ly rebased this DIRTY/CONFLICTING PR onto current Conflict file: Local gates at the new head (
Please re-review this revision. |
Forrest-ly
left a comment
There was a problem hiding this comment.
Code review 通过(re-review @ 72e92ed)。
本轮是对 rebase 后新 head 的复核(上一轮 AGE-3841 已 APPROVE 0a65c24),重点核对 rebase 增量与遗留线程:
1. Rebase 增量无行为变化(已逐 hunk 核对)
对比 9f34d03..0a65c24(旧 patch)与 45306960..72e92ed(新 patch):除 tests/cli_integration.rs 的 import hunk 外,所有 hunk 完全一致。该 hunk 即 PR 描述中声明的冲突解决——同时保留本 PR 的 CompressionMode 与上游的 estimate_tokens_from_bytes,无其他改动。
2. 核心修复复核(src/tokenless/crates/tokenless-cli/src/main.rs)
missing_compare_sessions检查位于两侧查询之后、warn_mode_mismatch与任何报告输出之前,fail-closed(exit 1)时序正确。- 错误信息
No records found for ...前缀与{sid:?}debug 转义均与stats diff --session(main.rs:776)保持一致,控制字符注入有单测覆盖。 Summary.limit改用共享的parse_positive_usize(main.rs:254),与stats diff --limit一致,--limit 0在解析期即被拒绝("value must be greater than zero"),消除了 limit=0 伪装成 session 缺失的歧义。
3. 两条遗留 P2 线程在当前 head 已确认解决
- chatgpt-codex-connector 的
--limit 0P2:由 c015989 修复,stats_summary_cli_rejects_zero_limit/stats_summary_compare_rejects_zero_limit覆盖(含 populated compare 场景)。 - ikunkun-sys 的文档 P2:README(en/zh)与 user-guide(en/zh)四处均已写明两项公开契约(缺失 session fail-closed、
--limit必须为正整数),与 CLI 实际行为一致。
4. 本地验证(head 72e92ed)
cargo test -p tokenless-cli --locked:275 passed, 2 ignored(224 unit + 46 integration + 5 matcher contract),与 PR 描述一致;新增/更新的 7 个 compare 相关单测全部通过。cargo fmt --all --check通过。
既有测试中 run_command_stats_compare / run_command_stats_compare_json 由 is_ok() 改为 fail-closed 断言,属于本次有意的契约变更,改法正确。无新增问题,approve。
ikunkun-sys
left a comment
There was a problem hiding this comment.
审查结论
REQUEST_CHANGES
必须修改:与最新 main 存在代码冲突
当前 head 72e92ed2ad094147de313ed8820b7bdcc3a4d05f 无法合并到最新 main ec7c99ef2887d67a7e49d176c9ef319b47e97d48;GitHub 当前状态为 CONFLICTING / DIRTY。
三方合并证据显示,实际未解决的文本冲突在 src/tokenless/crates/tokenless-cli/src/tests/main_tests.rs:
- 上游
f79e8300在同一测试区域新增了stats_persist_snapshot_enable_keeps_file_compression_and_sls/stats_persist_snapshot_disable_keeps_file_compression_and_sls。 - 本 PR 在该区域新增了
missing_compare_sessions_*测试和seed_compare_recordhelper。
src/tokenless/crates/tokenless-cli/src/main.rs 也由双方修改,但当前三方合并可自动处理;真正产生 conflict markers 的是上述 main_tests.rs 测试块。请 rebase 到最新 main,保留两组测试及对应实现,然后重新运行 Tokenless 的 fmt、Clippy 和 workspace tests。
提交历史
当前分支仍有 3 个提交。处理 rebase 冲突时,请一并按此前 review 和 AGENTS.md §13 的 same-PR fixup 规则,将它们整理为一个原子提交。
CI 状态
当前 Tokenless、Docs、Pages 和 PR lint checks 均为成功,但这些结果不能消除当前分支与最新 main 的合并冲突。
Empty --compare sides used to print a successful 0% report, hiding typos. Fail closed like stats diff --session. Reject --limit 0 at parse time and document both public contracts. Assisted-by: Cursor:2026.08.11-e8db854 Signed-off-by: zyw02 <zyw02@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
72e92ed to
6278f1d
Compare
|
@ikunkun-sys rebased this DIRTY/CONFLICTING PR onto current Conflict file: The previous 3 commits were squashed into one atomic commit on this rebase. Local gates at the new head (
Please re-review this revision. |
Forrest-ly
left a comment
There was a problem hiding this comment.
Re-review APPROVE @ 6278f1d(rebase onto latest main a4cb23ea,单 squashed commit)
Rebase 增量复核(重点):逐 hunk 对比了上一轮已批准的累计 diff(45306960..72e92ed,3 commits)与本轮累计 diff(a4cb23ea..6278f1d,1 commit)。两者 PR 内容逐行一致(643 行 diff 完全等长),差异仅为:
- blob hash 与 hunk 行号偏移(base 变化所致,预期内);
main_tests.rs中本 PR 测试块的插入点上移:从run_command_stats_status之后移到 upstream 新增的run_command_stats_disable_does_not_persist_env_overrides(main_tests.rs:1218)之后,@@ -1072,26 +1104,174 @@→@@ -1197,26 +1229,174 @@,hunk 尺寸一致(+174),无任何内容行变化。
冲突解决确认:main_tests.rs(唯一冲突文件)同时保留了 upstream 的 stats_persist_snapshot_* / run_command_stats_{enable,disable}_does_not_persist_env_overrides 测试与本 PR 的 missing_compare_sessions_*、seed_compare_record、stats_summary_cli_rejects_zero_limit 测试;main.rs 中 upstream 的 persist_stats_enabled / stats_persist_snapshot 路径(enable/disable 命令分支)与本 PR 的 fail-closed --compare(summary 命令分支 main.rs:699)及 parse_positive_usize(main.rs:184)互不干扰。
本地验证(exact head 6278f1db):
cargo fmt --all --check✅cargo clippy --workspace --locked -- -D warnings✅cargo test --workspace --locked -- --test-threads=1✅ 606 passed, 2 ignored(与 PR 描述一致)
结论:rebase 增量无行为变化,维持此前 APPROVE 的全部结论(fail-closed --compare、--limit 0 解析期拒绝、文档与回归测试)。
Why
tokenless stats summary --comparetreated unknown session IDs as a successful 0% comparison. A mistyped--session-idor a dual-run that never recorded stats looked like "no savings" instead of "this session does not exist."stats diff --sessionand the PythonTokenlessStats.compareclient already fail closed on the same case.What changed
--compareempty-session contract: if either named session has no records, the CLI now exits 1 withNo records found for …and does not print a 0% text or JSON report.--limit 0:stats summary --limitnow usesparse_positive_usizelikestats diff, so--limit 0fails at parse time instead of looking like a missing compare session.--comparesessions fail closed, andstats summary --limitmust be a positive integer.--json), control-character session IDs (debug-escaped likestats diff --session), and--limit 0on populated--compare.tokenless-cliintegration tests run the same--compareinvocations against an isolatedTOKENLESS_STATS_DB.Related issue
fixes #2673
User / Agent impact
tokenless stats summary --compare <baseline> <active>now fails when a session was never recorded, matchingtokenless stats diff --sessionandTokenlessStats.compare. Dual-runs that actually wrote stats are unchanged.stats summary --limit 0is now rejected at parse time, matchingstats diff --limit 0.Risk and compatibility
CLI
--compareon an empty session now errors instead of exiting 0 with a zeroed report. That is the bugfix. Callers with two recorded sessions keep the same comparison output. Python compare already raisedStatsNotFoundErrorfor this case.--limit 0now fails at clap parse instead of querying.Validation
Current head:
6278f1db9cfcff64bb8cae83ff81009e09664410DIRTY rebase onto upstream
maina4cb23ea27c9645d7294b7afe65309262e6a0e4bfrom previous head72e92ed2ad094147de313ed8820b7bdcc3a4d05f. One conflict file:src/tokenless/crates/tokenless-cli/src/tests/main_tests.rs. Resolution kept upstream persist snapshot / enable-disable tests (stats_persist_snapshot_*,run_command_stats_{enable,disable}_does_not_persist_env_overrides) and this PR'smissing_compare_sessions_*tests plusseed_compare_record.main.rsauto-merged: upstreampersist_stats_enabled/load_from_filepath kept, with this PR's fail-closed--compareandparse_positive_usizeon--limitreplayed on top.The previous 3-commit history was squashed into one atomic commit on this rebase, per AGENTS.md §13 and review.
Rebased head
6278f1db9cfcff64bb8cae83ff81009e09664410: fmt/clippy clean; workspace tests 606 passed, 2 ignored. Focusedtokenless-cli: 9 unit tests (compare fail-closed, persist snapshot,--limit 0) + 2 integration tests (stats_summary_compare_rejects_missing_sessions,stats_summary_compare_rejects_zero_limit) passed.Exact-head proof (
TOKENLESS_STATS_DBpointed at an isolated file):Review follow-up (
--limit 0on populated compare sessions): clap rejects withvalue must be greater than zeroand does not emitNo records found. Covered bystats_summary_cli_rejects_zero_limitandstats_summary_compare_rejects_zero_limit.Review follow-up (docs): README and user-guide now document both public CLI contracts requested on
1ceb816.Documentation and rollback
User-guide notes in
docs/user-guide/{en,zh}/token-saving/tokenless/measuring-savings.mdandcli-reference.md, plussrc/tokenless/README.md/README_zh.md, state that a missing session fails instead of reporting 0%, and thatstats summary --limitmust be a positive integer. Revert the commits on this branch to restore the previous zeroed success report, unvalidated--limit 0, and prior docs.