fix(cli): reject partial session size values - #5475
Conversation
6cdd836 to
17fba4b
Compare
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hey @tt-a1i — thanks for the contribution! The fix itself looks well-scoped, but the PR body doesn't follow our pull request template. A few required sections are missing or renamed:
- What this PR does / Why it's needed — currently
## Summary; we need both headings separately so reviewers can understand the change and its motivation at a glance. - Reviewer Test Plan — currently
## Test plan; needs theHow to verify,Evidence (Before & After), andTested onsub-sections. - Risk & Scope — missing entirely (main risk, out-of-scope items, breaking changes).
- Linked Issues — missing as a section (you mention
Fixes #5474inline, which is great — just needs the formal section). - 中文说明 — missing the
<details>block with a Chinese translation of the body.
Could you restructure the PR description to match the template? Happy to answer questions if anything's unclear.
中文说明
@tt-a1i 你好——感谢贡献！修复本身范围合理，但 PR 正文没有按照我们的 PR 模板。缺少以下必要章节：
- What this PR does / Why it's needed — 目前只有
## Summary，需要分成两个独立标题。 - Reviewer Test Plan — 目前只有
## Test plan，需要包含How to verify、Evidence (Before & After)、Tested on子章节。 - Risk & Scope — 完全缺失。
- Linked Issues — 缺少独立章节（虽然在正文提到了
Fixes #5474）。 - 中文说明 — 缺少
<details>中文翻译。
请按照模板重新组织 PR 描述。有问题随时问！
— Qwen Code · qwen3.7-max
|
updated the PR description to match the template. thanks. |
✅ Local end-to-end verification (real
|
| Probe | BASE 56e76e7e |
PR 17fba4b9 |
|---|---|---|
HTTP ?size=1abc / 1.5 / 1e2 / 0x10 (3 stored) |
❌ 1 session + nextCursor (parseInt partial-accepts → 1/0) |
✅ 3 sessions, no cursor (malformed → default page) |
HTTP ?size=2 (valid control) |
✅ 2 + cursor | ✅ 2 + cursor |
HTTP ?size=9007199254740992 (2^53) (clamp control, 21 stored) |
✅ 21, clamped to max | ✅ 21, clamped to max |
helper size=1.5 / MAX_SAFE+1 / Infinity (non-HTTP caller, 21 stored) |
❌ 2 / 21 / 21 (raw value used) | ✅ 20 + cursor (default page) |
========== BASE (56e76e7e) ==========
HTTP ?size=1abc status=200 sessions=1 nextCursor=true
HTTP ?size=1.5 status=200 sessions=1 nextCursor=true
HTTP ?size=1e2 status=200 sessions=1 nextCursor=true
HTTP ?size=0x10 status=200 sessions=1 nextCursor=true
helper size=1.5 sessions=2 nextCursor=true
helper size=MAX_SAFE+1 sessions=21 nextCursor=false
helper size=Infinity sessions=21 nextCursor=false
========== PR (17fba4b9) ==========
HTTP ?size=1abc status=200 sessions=3 nextCursor=false
HTTP ?size=1.5 status=200 sessions=3 nextCursor=false
HTTP ?size=1e2 status=200 sessions=3 nextCursor=false
HTTP ?size=0x10 status=200 sessions=3 nextCursor=false
helper size=1.5 / MAX_SAFE+1 / Infinity sessions=20 nextCursor=true
So on BASE a malformed ?size=1abc is silently coerced to 1 (and 0x10 → 0 → 1), returning a single session with a misleading nextCursor; non-HTTP callers passing 1.5/Infinity leak a fractional/over-large page size. On PR both layers fall back to the default page size, while valid (2) and oversized-but-valid (2^53) values keep their existing behavior.
Unit test + revert-proof + static
vitest run server.test.ts -t "size"on the PR source → 5/5 pass.- Revert-proof: running the PR's new tests against the base source makes the two regression tests fail —
ignores malformed size query values❌ anduses the default page size for invalid non-HTTP size values❌ — while pre-existing controls (clamps size=0 to 1,clamps size=200 to max page size) pass. (clamps unsafe finite HTTP size values to the max page sizepasses on both — it is a behavior-preservation test, not a regression test.) - ESLint clean (exit 0); Prettier clean;
git diff --checkclean.
Note
parseSessionPageSizeQuery returns MAX_SESSION_PAGE_SIZE for valid-but-unsafe positive integers (e.g. 2^53) and 1 for unsafe negatives, preserving the existing HTTP clamp. The Number.isSafeInteger guard inside listWorkspaceSessionsForResponse independently protects non-HTTP callers (ACP / programmatic) that pass a raw number — confirmed above with 1.5 / MAX_SAFE+1 / Infinity.
Verdict: LGTM — the fix rejects partial/invalid session sizes at both the HTTP route and the shared helper, with no regression to valid or oversized-valid values.
🇨🇳 中文版本（点击展开）
✅ 本地端到端验证（真实 qwen serve 路由，tmux）
通过把真实编译产物中的 createServeApp 路由（GET /workspace/:id/sessions）挂到真实 socket 上，发起真实 HTTP 请求（带畸形/超大/小数的 size），并直接调用导出的 listWorkspaceSessionsForResponse 来验证。会话文件按测试套件相同方式写入磁盘（core Storage → chats/*.jsonl）；bridge 用一个返回空 live 列表的 no-op Proxy。两边都跑真实路由,只有 server.ts 不同：
- BASE = merge-base
56e76e7e（parseInt(sizeParam, 10)） - PR = head
17fba4b9（严格的parseSessionPageSizeQuery+Number.isSafeInteger守卫）
（DEFAULT_SESSION_PAGE_SIZE = 20，MAX_SESSION_PAGE_SIZE = 100。）
A/B 结果 —— 两个层面
| 探针 | BASE 56e76e7e |
PR 17fba4b9 |
|---|---|---|
HTTP ?size=1abc / 1.5 / 1e2 / 0x10（存 3 条） |
❌ 1 条 + nextCursor（parseInt 部分解析成 1/0） |
✅ 3 条,无 cursor（畸形 → 默认页大小） |
HTTP ?size=2（有效值对照） |
✅ 2 条 + cursor | ✅ 2 条 + cursor |
HTTP ?size=9007199254740992（2^53,clamp 对照,存 21 条） |
✅ 21 条,clamp 到 max | ✅ 21 条,clamp 到 max |
helper size=1.5 / MAX_SAFE+1 / Infinity（非 HTTP 调用方,存 21 条） |
❌ 2 / 21 / 21（直接使用原始值） | ✅ 20 + cursor（默认页大小） |
也就是说：BASE 上畸形的 ?size=1abc 被静默地强转成 1（0x10 → 0 → 1）,只返回 1 条会话并带有误导性的 nextCursor;非 HTTP 调用方传入 1.5/Infinity 时会泄漏出小数/超大的页大小。PR 在两个层面都回退到默认页大小,同时有效值（2）和超大但合法的值（2^53）保持原有行为。
单元测试 + 反向验证 + 静态检查
- 在 PR 源码上
vitest run server.test.ts -t "size"→ 5/5 通过。 - 反向验证：把 PR 新增测试放到 base 源码上跑,两个回归测试失败——
ignores malformed size query values❌ 与uses the default page size for invalid non-HTTP size values❌——而既有对照（clamps size=0 to 1、clamps size=200 to max page size）通过。（clamps unsafe finite HTTP size values to the max page size在两边都通过——它是行为保持型测试,而非回归测试。） - ESLint 通过（exit 0）；Prettier 通过；
git diff --check通过。
补充说明
parseSessionPageSizeQuery 对合法但非安全整数的正数（如 2^53）返回 MAX_SESSION_PAGE_SIZE、对非安全负数返回 1,从而保留既有的 HTTP clamp 行为。listWorkspaceSessionsForResponse 内部的 Number.isSafeInteger 守卫则独立保护传入原始数字的非 HTTP 调用方（ACP / 程序化调用）——上面用 1.5 / MAX_SAFE+1 / Infinity 已确认。
结论：LGTM —— 该修复在 HTTP 路由层和共享 helper 层都拒绝了部分/非法的 session size,且对合法值与超大合法值无回归。
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
@qwen-code /triage |
|
Thanks for the PR, @tt-a1i! Template looks good ✓ — all required sections filled in, bilingual, linked issue. On direction: this is a clean input-validation fix that aligns well with the project's recent trend toward stricter parsing (cf. #5491 on On approach: the scope is minimal and focused — exactly what's needed. The two-layer validation (regex in The Prettier-driven formatting changes in existing tests are noise but harmless. Moving on to code review. 🔍 中文说明感谢 PR，@tt-a1i！ 模板完整 ✓ — 所有必填部分都已填写，双语，关联了 issue。 方向：这是一个干净的输入校验修复，和项目最近趋严的解析方向一致（参考 #5491 对 方案：范围最小且聚焦——恰好是需要的。两层校验（HTTP 路径用 已有测试的 Prettier 格式化改动是噪音但无害。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
2a. Code ReviewMy independent proposal for strict session-size parsing: add a regex check at the HTTP boundary ( Reviewed both files carefully. The regex 2b. Real-Scenario TestingBefore (main branch — old
|
|
Stepping back: this is a textbook bug fix. The problem is real ( My independent proposal would have only added validation at the HTTP boundary. The PR's defense-in-depth approach — also tightening Every change in the diff is needed for the stated goal. The test formatting adjustments are Prettier-driven, not scope creep. All 16 tests pass, lint and typecheck are clean. Approving — this is ready to ship. ✅ 中文说明退一步看：这是一个教科书级的 bug 修复。问题是真实的（ 我的独立方案只在 HTTP 边界加校验。PR 的纵深防御方案——同时收紧 diff 中的每一处改动都是为了实现声明的目标。测试格式化调整是 Prettier 驱动的，不是范围蔓延。16 个测试全部通过，lint 和 typecheck 干净。 批准——可以合并 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
3 participants
What this PR does
This PR parses
GET /workspace/:id/sessions?size=only when the full query value is a decimal integer. Malformed values such as1abc,1.5,1e2, and0x10now stay on the default page-size path instead of being partially accepted. Valid oversized integers still use the existing HTTP clamp behavior, and fractional or non-safe numeric sizes from non-HTTP callers are ignored.Why it's needed
The sessions endpoint should not accept partial numeric query values. Partial parsing makes malformed inputs behave like valid sizes, which is surprising for callers and inconsistent with other stricter query parsing paths.
Reviewer Test Plan
How to verify
Run the focused server tests for
GET /workspace/:id/sessions. Confirm malformed size query values use the default size, valid oversized integers are clamped, and non-HTTP fractional/non-safe numeric inputs are ignored.Evidence (Before & After)
Before: a query such as
?size=1abccould be parsed as1by partial numeric parsing.After: only complete decimal integer strings are accepted from HTTP query values; malformed strings use the default behavior.
Tested on
Environment (optional)
Local validation used the CLI package test runner for
server.test.ts.Risk & Scope
sizevalues now get default page size behavior instead of accidental partial parsing.Linked Issues
Fixes #5474
Testing
npm --workspace packages/cli test -- server.test.ts -t "GET /workspace/:id/sessions" --coverage.enabled=falsenpx prettier --check packages/cli/src/serve/server.ts packages/cli/src/serve/server.test.tsnpx eslint packages/cli/src/serve/server.ts packages/cli/src/serve/server.test.tsnpm --workspace packages/cli run typechecknpm run build -- --cli-onlygit diff --checkAI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.
中文说明
这个 PR 做了什么
这个 PR 只在
GET /workspace/:id/sessions?size=的完整 query 值是十进制整数时才解析它。1abc、1.5、1e2、0x10这类畸形值会走默认 page-size 路径，不再被部分接受。合法但过大的整数仍保留现有 HTTP clamp 行为；非 HTTP 调用方传入的小数或非安全数字 size 会被忽略。为什么需要
Sessions endpoint 不应该接受部分数字 query 值。部分解析会让畸形输入表现得像合法 size，这对调用方来说很意外，也和其他更严格的 query 解析路径不一致。
Reviewer 测试计划
如何验证
运行
GET /workspace/:id/sessions的聚焦 server 测试。确认畸形 size query 值会使用默认 size，合法超大整数会被 clamp，非 HTTP 的小数/非安全数字输入会被忽略。前后证据
之前：
?size=1abc这类 query 可能通过部分数字解析变成1。之后：HTTP query 只接受完整十进制整数字符串；畸形字符串走默认行为。
测试平台
环境
本地验证使用 CLI package test runner，目标测试为
server.test.ts。风险与范围
size值的调用方现在会得到默认 page size 行为，不再走意外的部分解析。关联 Issue
Fixes #5474
测试
npm --workspace packages/cli test -- server.test.ts -t "GET /workspace/:id/sessions" --coverage.enabled=falsenpx prettier --check packages/cli/src/serve/server.ts packages/cli/src/serve/server.test.tsnpx eslint packages/cli/src/serve/server.ts packages/cli/src/serve/server.test.tsnpm --workspace packages/cli run typechecknpm run build -- --cli-onlygit diff --checkAI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.