fix(cli): reject invalid session list cursors - #5709
Conversation
|
@qwen-code /triage |
|
Thanks for the PR, @tt-a1i! Template looks good ✓ — all required sections present with clear motivation and test plan. On direction: this is a focused defensive fix for session-list cursor validation, directly addressing #5708. It fits squarely within On approach: the scope is tight — four files, only cursor validation touched, no drive-by changes. The fix extracts parse functions in both the REST ( One minor observation: the ACP path calls Moving on to code review and testing. 🔍 中文说明感谢贡献,@tt-a1i! 模板完整 ✓ — 所有必填章节齐全,动机和测试计划清晰。 方向:这是对 session-list cursor 校验的防御性修复,直接对应 #5708。完全在 方案:范围紧凑——仅涉及 4 个文件,只修改 cursor 校验逻辑,无夹带改动。在 REST( 一个小的观察:ACP 路径先调用 进入代码审查和测试 🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe implementation is clean and focused. Two near-identical parse functions ( Unit TestsAll 14 cursor-specific tests pass (9 in Before (installed build — qwen 0.18.5)Live HTTP test against After (PR #5709 — cursor validation logic)
The PR's validation logic correctly rejects all three previously-accepted invalid cursor types (negative, unsafe integer, whitespace-only) while preserving valid fractional 中文说明代码审查实现干净且聚焦。两个近似相同的解析函数( 单元测试全部 14 个 cursor 相关测试通过( 修改前(已安装版本 — qwen 0.18.5)对 14170 端口的 修改后(PR #5709)
— Qwen Code · qwen3.7-max |
|
This is a clean, minimal defensive fix. The motivation is sound — three types of invalid cursor values (negative, unsafe integer, whitespace-only) slipped through the existing The implementation is well-scoped: four files, no drive-by refactors, every line serves the stated goal. The ~8 lines of duplicated validation logic between the REST and ACP paths is the pragmatic trade-off — different error types, different modules, and extracting would add cross-module coupling for a tiny gain. 14 unit tests pass, covering all edge cases. The live HTTP test against the installed build confirmed the bug ( Recommendation: approve. Ships cleanly, does what it says, no concerns. 中文说明这是一个干净、最小化的防御性修复。动机合理——三类非法 cursor 值(负数、unsafe 整数、纯空白)在现有仅检查 实现范围合理:4 个文件,无夹带重构,每行代码都服务于目标。REST 和 ACP 路径之间约 8 行的重复校验逻辑是务实的取舍——不同错误类型、不同模块,提取共享函数会增加跨模块耦合,收益不大。 14 个单元测试全部通过,覆盖所有边界。HTTP 实测确认了 bug( 建议:通过。改动干净,实现了承诺,无顾虑。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
✅ Maintainer verification — real local test (tmux)I verified this PR locally by booting the actual Setup
Test 1 — Real daemon over a real socket (the headline)Booted
Test 2 — REST path A/B (real Express app via supertest,
|
cursor |
HTTP | body |
|---|---|---|
| (省略) | 200 | 首页 |
''(空) |
200 | 首页 |
0 |
200 | 页(合法边界) |
1000123.456(小数) |
200 | 页 —— 小数 mtimeMs 被接受 |
1797860000000 |
200 | 页 |
abc |
400 | invalid_cursor(改之前也拒绝) |
Infinity |
400 | invalid_cursor(改之前也拒绝) |
-1 |
400 | invalid_cursor —— 本 PR 新增拒绝 |
9007199254740992(MAX_SAFE_INTEGER+1) |
400 | invalid_cursor —— 本 PR 新增拒绝 |
(仅空白) |
400 | invalid_cursor —— 本 PR 新增拒绝 |
测试 2 —— REST 路径 A/B(通过 supertest 跑真实 Express app,server.test.ts -t cursor)
- 在 PR 上:9/9 通过,包含全部五个
400 invalid_cursor边界 +accepts fractional mtime cursor values+passes fractional cursor values to SessionService without truncating。 - 只还原新增的拒绝条件(回到只有
!Number.isFinite)→ 恰好这 3 个新边界失败,其余保持绿色:恢复修复后又变绿。这个外科手术式的 toggle 证明测试精确守住了 PR 新增的内容——且小数精度在两种状态下都不受影响。× 400 invalid_cursor … : -1 → expected 200 to be 400 × 400 invalid_cursor … : 9007199254740992 → expected 200 to be 400 × 400 invalid_cursor … : ' ' → expected 200 to be 400 ✓ abc, ✓ Infinity, ✓ accepts fractional, ✓ passes fractional without truncating Tests 3 failed | 6 passed
测试 3 —— in-process ACP 路径 A/B(真实 QwenAgent,acpAgent.test.ts -t unstable_listSessions)
- 在 PR 上:5/5 通过,包含
rejects invalid cursors before listing sessions(遍历abc, Infinity, -Infinity, -1, 9007199254740992, ' ')和passes a finite non-negative cursor through to SessionService(小数1797860000000.5)。 - 还原 ACP 校验器 → 该 reject-loop 测试失败,而失败本身就是 bug:非法 cursor 不再被拒绝,反而流到了数据层:
也就是
expected [Function] to throw 'Invalid cursor: "-1" …' but got 'sessionService.listSessions is not a function'-1一路到了SessionService.listSessions,而不是在入口处被拒绝。恢复修复后变绿。
备注(不阻塞合并)
- 两条路径现在一致了。 REST(
parseSessionCursor)和 ACP(parseAcpSessionListCursor)应用相同规则,上面分别独立验证——对可能命中任一入口的 orchestrator 是好事。 - 空白曾是一个真实的潜在 bug。 修复前
cursor=" "会经Number(" ") → 0静默地返回 page-at-0 而不是报错;现在是干净的400。 - 没有精度回归。 小数
mtimeMscursor(1000123.456、1797860000000.5)在两种 toggle 状态下都原样透传、不截断——用parseInt/Math.floor的简单修法会破坏这一点,而本 PR 正确地没有这么做。 - 省略/空 cursor 的首页语义保持不变;
0仍是合法边界。
结论: 通过真实守护进程 + socket 端到端验证了行为,两条校验路径都在真实 app 和真实 agent 上做了 A/B 证明,小数 cursor 精度完好且无回归。👍
What this PR does
Tightens session-list cursor validation before calling
SessionService.listSessions. REST / ACP HTTP and in-process ACP now reject negative cursor values, whitespace-only cursor strings, and values aboveNumber.MAX_SAFE_INTEGERwhile preserving existing first-page handling for omitted or empty cursors.The change keeps non-negative fractional cursors valid because session pagination cursors come from filesystem
mtimeMs, which can include fractional milliseconds. Tests now cover the invalid edges and assert that fractional cursor values are forwarded without truncation.Why it's needed
Session-list cursors are generated by
SessionService.listSessions()from filemtimeMsvalues. The current handlers already reject non-numeric and non-finite cursors, but still accept finite values like-1or9007199254740992. Those are not valid page cursors: negative cursors can produce empty persisted pages, and unsafe numeric values can lose precision before filtering.Rejecting these values keeps the REST / ACP HTTP and in-process ACP paths aligned, while still accepting cursor tokens that can actually be returned by the service.
Reviewer Test Plan
How to verify
Run:
Expected behavior: REST session-list requests with invalid cursors such as
abc,-1,Infinity,9007199254740992, or whitespace-only strings return400 invalid_cursor. In-process ACP rejects the same invalid cursor values before constructingSessionService. Non-negative fractional cursors such as1000123.456remain valid and are forwarded without truncation.Known unrelated local check failure:
This still fails in
src/ui/components/BaseTextInput.tsxbecause local TypeScript cannot resolveink/domandink/components/CursorContext; those files are outside this PR.Evidence (Before & After)
N/A — non-UI API validation change. Unit tests cover the before/after behavior: invalid negative, unsafe, non-finite, non-numeric, and whitespace-only cursors are rejected; omitted and empty cursors keep first-page behavior; valid fractional
mtimeMscursors are preserved.Tested on
Environment (optional)
Local Node/npm workspace. I ran focused Vitest, Prettier check, targeted ESLint, workspace lint, and
git diff --check. CLI typecheck has the unrelatedink/domresolution failure noted above.Risk & Scope
mtimeMsvalues.Linked Issues
Fixes #5708
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.
中文说明
这个 PR 做了什么
在调用
SessionService.listSessions之前收紧 session-list cursor 校验。REST / ACP HTTP 和 in-process ACP 现在会拒绝负数 cursor、仅包含空白的 cursor 字符串,以及超过Number.MAX_SAFE_INTEGER的值;省略 cursor 或传空字符串时,仍保持原有的首页行为。这个改动保留非负小数 cursor 的合法性,因为 session 分页 cursor 来自文件系统
mtimeMs,它可能包含小数毫秒。测试现在覆盖非法边界,并断言小数 cursor 会原样转发、不被截断。为什么需要
Session-list cursor 由
SessionService.listSessions()基于文件mtimeMs生成。当前 handler 已经拒绝非数字和非有限 cursor,但仍会接受-1或9007199254740992这类有限数值。它们不是有效的分页 cursor:负数 cursor 可能产生空的持久化页面,unsafe 数值在过滤前可能发生精度丢失。拒绝这些值可以让 REST / ACP HTTP 与 in-process ACP 路径保持一致,同时继续接受 service 实际可能返回的 cursor token。
审核测试计划
如何验证
运行:
预期行为:带有
abc、-1、Infinity、9007199254740992或仅空白字符串等非法 cursor 的 REST session-list 请求返回400 invalid_cursor。In-process ACP 会在构造SessionService前拒绝相同的非法 cursor。1000123.456这类非负小数 cursor 仍然合法,并且不会被截断。已知的无关本地检查失败:
该命令仍然在
src/ui/components/BaseTextInput.tsx失败,因为本地 TypeScript 无法解析ink/dom和ink/components/CursorContext;这些文件不在本 PR 范围内。证据(修改前后)
N/A — 非 UI 的 API 校验变更。单元测试覆盖修改前后的行为:非法负数、unsafe、非有限、非数字和仅空白 cursor 会被拒绝;省略和空 cursor 仍然表示首页;合法的小数
mtimeMscursor 会被保留。测试平台
环境(可选)
本地 Node/npm workspace。我运行了 focused Vitest、Prettier check、targeted ESLint、workspace lint 和
git diff --check。CLI typecheck 存在上面提到的无关ink/dom解析失败。风险与范围
mtimeMs值。关联 Issue
Fixes #5708
AI 辅助披露
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.