Skip to content

fix(desktop): validate list_sessions pagination params - #7162

Closed
VectorPeak wants to merge 7 commits into
QwenLM:mainfrom
VectorPeak:codex/list-sessions-pagination-validation
Closed

fix(desktop): validate list_sessions pagination params#7162
VectorPeak wants to merge 7 commits into
QwenLM:mainfrom
VectorPeak:codex/list-sessions-pagination-validation

Conversation

@VectorPeak

@VectorPeak VectorPeak commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

What this PR does

This PR tightens the list_sessions pagination contract so limit and offset are treated as discrete integer controls instead of arbitrary numbers.

It changes the canonical session tool schema to expose limit and offset as integers, and adds handler-level validation so direct registry calls cannot pass malformed pagination values into the session lister. Valid integer pagination still works, and high integer limit values are intentionally still passed to the backend so the existing max-100 clamp behavior stays unchanged. The focused desktop session-tools-core regression test for this handler is also wired into the full CI profile so this desktop-only package coverage is no longer local-only.

Why it's needed

list_sessions returns { total, returned, sessions } with pagination, and the system prompt tells agents to use filters plus pagination instead of fetching everything. Before this change, the schema accepted z.number() for both pagination fields, and handleListSessions() forwarded those values directly into ctx.listSessions().

The backend then computes pagination with Math.min(options?.limit ?? 20, 100), options?.offset ?? 0, and sessions.slice(offset, offset + limit). That leaves malformed numeric values with JavaScript slice semantics instead of pagination semantics. For example, limit: -1 becomes sessions.slice(0, -1), which returns every session except the last one instead of rejecting the invalid request; offset: 1.5 is also accepted even though a fractional session offset has no meaningful page boundary.

This PR rejects those malformed tool arguments at the session tool boundary before they reach the slice-based pagination path.

Reviewer Test Plan

How to verify

Reviewers can confirm that list_sessions.limit and list_sessions.offset are now emitted as integer fields in the converted JSON schema, and that direct handler calls reject limit: 0, limit: -1, limit: 1.5, offset: -1, and offset: 1.5 without invoking the backend session lister. Existing valid integer pagination, including the minimum boundary limit: 1, offset: 0, remains unchanged.

Evidence (Before & After)

Before this change, the malformed payload shape below could pass the list_sessions tool schema and reach backend pagination:

{
  "limit": -1,
  "offset": 0
}

The old backend pagination expression would evaluate the page as:

const limit = Math.min(-1, 100); // -1
const offset = 0;
sessions.slice(offset, offset + limit); // sessions.slice(0, -1)

After this change, the schema exposes integer pagination controls and the handler returns an error before calling ctx.listSessions() for negative or fractional values. The new regression test also checks that limit: 101 still passes through to the backend clamp, so this PR does not change the existing max-limit compatibility behavior.

Tested on

OS Status
macOS not tested
Windows tested
Linux tested

Environment (optional)

Windows local validation used Bun 1.3.14 from D:\ZXY\Dev\bun\bin\bun.exe:

D:\ZXY\Dev\bun\bin\bun.exe test packages/desktop/packages/session-tools-core/src/handlers/list-sessions.test.ts
cd packages/desktop/packages/session-tools-core
D:\ZXY\Dev\bun\bin\bun.exe run typecheck
git diff --check

Windows results: focused handler test 4 pass; tsc --noEmit passed; git diff --check passed.

WSL/Linux validation used native Bun through npm exec --yes bun@1.3.14:

npm exec --yes bun@1.3.14 -- test packages/desktop/packages/session-tools-core/src/handlers/list-sessions.test.ts
cd packages/desktop/packages/session-tools-core && npm exec --yes tsc -- --noEmit
cd /mnt/d/ZXY/Github/qwen-code && npm exec --yes bun@1.3.14 -- test packages/desktop/packages/session-tools-core/src

Linux results: focused handler test 4 pass; tsc --noEmit passed; session-tools-core full suite 65 pass.

The new GitHub Actions step intentionally runs the focused list-sessions.test.ts file. A broader ad-hoc root-level command that also included tool-defs-filtering.test.ts failed in remote CI because that sibling test imports package dependencies differently from the focused handler test; the full package suite still passes under WSL/Linux.

Risk & Scope

  • Main risk or tradeoff: Low. This tightens malformed pagination inputs only; callers that accidentally sent negative or fractional pagination values now receive a clear tool error instead of ambiguous JavaScript slice behavior.
  • Not validated / out of scope: macOS local validation and full repository preflight were not run. A Windows full packages/desktop/packages/session-tools-core/src run currently hits existing Darwin sandbox profile path assertions on Windows; the same full session-tools-core suite passes under WSL/Linux.
  • Breaking changes / migration notes: No intended change for valid callers. Positive integer limit and non-negative integer offset continue to work, and high integer limits still flow to the existing backend max-100 clamp.

Linked Issues

N/A

中文说明

What this PR does

这个 PR 收紧了 list_sessions 的分页参数契约,让 limitoffset 按离散整数分页控制来处理,而不是任意数字。

具体来说,canonical session tool schema 现在会把 limitoffset 暴露为 integer;同时 handler 增加了运行时防御,避免直接绕过 schema 的 registry 调用把非法分页值传给后端 session lister。合法整数分页保持不变,较大的整数 limit 仍然会传给后端,由既有的 max-100 clamp 处理。

Why it's needed

list_sessions 返回 { total, returned, sessions } 并支持分页,系统 prompt 也明确要求 agent 使用过滤和分页,而不是一次性扫描所有 session。修复前,这两个分页字段在 schema 中都是 z.number()handleListSessions() 会把值原样传给 ctx.listSessions()

后端分页随后使用 Math.min(options?.limit ?? 20, 100)options?.offset ?? 0sessions.slice(offset, offset + limit)。这意味着非法数字会落入 JavaScript slice 语义,而不是分页语义。比如 limit: -1 会变成 sessions.slice(0, -1),返回除最后一条外的全部 session;offset: 1.5 也会被接受,但小数 offset 没有合理的分页边界含义。

这个 PR 在 session tool 边界直接拒绝这些 malformed 参数,避免它们进入 slice-based pagination 路径。

Reviewer Test Plan

Reviewers 可以确认:list_sessions.limitlist_sessions.offset 在 JSON schema 中已经是 integer;直接调用 handler 时,limit: 0limit: -1limit: 1.5offset: -1offset: 1.5 都会被拒绝,并且不会调用后端 session lister。合法整数分页,包括最小边界 limit: 1, offset: 0,仍然正常。

Evidence (Before & After)

修复前,下面这种 payload 可以通过 list_sessions 工具 schema 并进入后端分页:

{
  "limit": -1,
  "offset": 0
}

旧逻辑会计算成:

const limit = Math.min(-1, 100); // -1
const offset = 0;
sessions.slice(offset, offset + limit); // sessions.slice(0, -1)

修复后,schema 和 handler 都会拒绝 0、负数或小数分页参数。新增测试还确认 limit: 1, offset: 0 的最小合法边界会被接受,且 limit: 101 仍然透传给后端 clamp,因此本 PR 不改变既有 max-100 兼容行为。

Risk & Scope

主要风险较低:这个 PR 只收紧 malformed pagination 输入。之前误传负数或小数的调用方会收到明确错误,而不是继续得到含混的 JavaScript slice 结果。合法调用不受影响;macOS 本地未验证;Windows 下同包全量测试存在既有 Darwin sandbox profile path 断言问题,但 WSL/Linux 同包全量测试已通过。

Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Qwen precheck requires maintainer approval before automated triage/review.

Head SHA: a99b0248c56ff15f155870c5bbec5bb60222c429

Reason:

  • prompt_injection:system_prompt

A maintainer with write access can inspect the PR and manually request a run with @qwen-code /triage or @qwen-code /review. A new push requires a fresh precheck.

@wenshao

wenshao commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /review

@github-actions

Copy link
Copy Markdown
Contributor
_Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/29666792600)._

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment on lines +31 to +32
{ args: { limit: -1 }, message: 'limit must be a positive integer.' },
{ args: { limit: 1.5 }, message: 'limit must be a positive integer.' },

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Missing boundary tests (limit: 0, limit: 1, offset: 0) for off-by-one regression coverage — Concrete cost: if < 1 were accidentally changed to <= 1, limit: 1 (a natural first-page pagination value) would be incorrectly rejected with no existing test detecting it.

Suggested change
{ args: { limit: -1 }, message: 'limit must be a positive integer.' },
{ args: { limit: 1.5 }, message: 'limit must be a positive integer.' },
{ args: { limit: 0 }, message: 'limit must be a positive integer.' },
{ args: { limit: -1 }, message: 'limit must be a positive integer.' },
{ args: { limit: 1.5 }, message: 'limit must be a positive integer.' },

Also consider adding a valid-boundary test:

it('accepts minimum valid pagination boundaries', async () => {
  const calls: Array<ListSessionsOptions | undefined> = [];
  const ctx = createCtx((options) => calls.push(options));
  const result = await handleListSessions(ctx, { limit: 1, offset: 0 });
  expect(result.isError).toBe(false);
  expect(calls).toEqual([{ limit: 1, offset: 0 }]);
});

— qwen3.7-max via Qwen Code /review

@VectorPeak VectorPeak Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, addressed. I added the invalid limit: 0 case and a valid minimum-boundary test for { limit: 1, offset: 0 }, so the regression suite now covers both sides of the off-by-one boundary. The latest remote Test (ubuntu-latest, Node 22.x) run is green.

Comment on lines +1 to +2
import { describe, expect, it } from 'bun:test';
import { handleListSessions } from './list-sessions.ts';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This test file (and the pre-existing tool-defs-filtering.test.ts) is outside every npm workspace and no CI workflow runs bun test in the desktop workspace — these tests never execute in CI — Concrete cost: a future change that weakens the validation guards would merge undetected.

Consider adding a CI step that runs bun test in the desktop workspace (e.g., in ci.yml gated on a full profile), or moving the critical validation tests into a root-workspace test file that npm run test:ci collects.

— qwen3.7-max via Qwen Code /review

@VectorPeak VectorPeak Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, addressed. I added a full-profile CI step that runs the focused desktop regression test with npm exec --yes bun@1.3.14 -- test packages/desktop/packages/session-tools-core/src/handlers/list-sessions.test.ts. I kept the CI command scoped to the list_sessions handler coverage from this PR; the broader session-tools-core suite still passes under WSL/Linux locally, and the latest remote Test (ubuntu-latest, Node 22.x) run is green.

@wenshao

wenshao commented Jul 19, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: This is a real validation gap. The current z.number() schema for limit and offset accepts any numeric value — negatives, fractions — which silently produce JavaScript slice semantics instead of pagination semantics (e.g. sessions.slice(0, -1) returning all-but-last). The PR body demonstrates this concretely with code examples. No live reproduction via the CLI, but the bug mechanism is clearly explained and the Zod schema confirms the gap.

Direction: Aligned. Tightening tool parameter validation to reject malformed inputs is squarely within the project's reliability goals. No CHANGELOG reference needed for this scope.

Size: Not applicable — no core paths touched. 17 production lines (handler + schema), 91 test lines across 4 files in packages/desktop/packages/session-tools-core/.

Approach: Scope feels right. Two layers of defense — Zod schema tightened to .int().min() and handler-level guard for direct calls bypassing schema validation. Both are clean and proportional. One observation: the handler validation exactly mirrors what the Zod schema already enforces. For direct handleListSessions() calls that skip schema validation this is a reasonable belt-and-suspenders pattern, but worth noting that in practice all tool invocations flow through the schema. Not a concern — just context for reviewers.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:这是一个真实的校验缺口。当前 limitoffsetz.number() schema 接受任意数值——负数、小数——会默默产生 JavaScript slice 语义而不是分页语义(如 sessions.slice(0, -1) 返回除最后一条外的全部结果)。PR 正文用代码示例具体说明了这一点。虽然没有通过 CLI 做实时复现,但 bug 机制解释清楚,Zod schema 也确认了这个缺口。

方向:对齐。收紧工具参数校验以拒绝畸形输入,完全符合项目的可靠性目标。此范围不需要 CHANGELOG 引用。

规模:不适用——未触及核心路径。17 行生产代码(handler + schema),91 行测试代码,共 4 个文件,位于 packages/desktop/packages/session-tools-core/

方案:范围合理。两层防御——Zod schema 收紧为 .int().min(),handler 级别增加对绕过 schema 直接调用的防御。两者都干净且成比例。一个观察:handler 校验与 Zod schema 的约束完全重复。对于绕过 schema 直接调用 handleListSessions() 的场景,这是合理的纵深防御,但实际中所有工具调用都经过 schema。不是问题——仅供 reviewer 参考。

进入代码审查 🔍

Qwen Code · qwen3.7-max

Reviewed at 2e8746cbee462d2321f1acd00a854fa7410593f3 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

The implementation is clean and proportional to the problem. Two layers of defense:

Schema layerz.number().int().min(1) for limit and z.number().int().min(0) for offset in tool-defs.ts. This catches malformed values at the Zod validation boundary, which is where all tool invocations pass through. Standard zod-to-json-schema conversion will emit "type": "integer" for these fields, giving agents a correct schema signal.

Handler layerNumber.isInteger() + bounds check in handleListSessions() before forwarding to ctx.listSessions(). This is defense-in-depth for any direct handler call that bypasses schema validation. The validation logic exactly mirrors the Zod constraints, which is correct — any divergence between the two would be a bug.

No correctness issues found. Number.isInteger() correctly rejects NaN, Infinity, and fractional values. The limit < 1 check (not <= 0) correctly rejects zero — a limit of zero would be meaningless for pagination.

The new CI step in ci.yml runs the targeted tests as part of the full CI profile, following the existing guard pattern. Good addition — ensures these tests actually run on PRs.

Test Results

$ bun test src/handlers/list-sessions.test.ts
bun test v1.3.14 (0d9b296a)

✓ handleListSessions > rejects malformed pagination values before listing sessions [0.42ms]
✓ handleListSessions > accepts minimum valid pagination boundaries [0.19ms]
✓ handleListSessions > passes valid pagination values through to the session lister [0.13ms]
✓ handleListSessions > preserves high integer limits for the backend clamp [0.11ms]

 4 pass
 0 fail
 21 expect() calls
Ran 4 tests across 1 file. [10.00ms]

The tool-defs-filtering.test.ts schema type assertion (properties?.limit?.type === 'integer') couldn't be executed locally due to a pre-existing workspace dependency resolution issue (beautiful-mermaid not found in worktree), but the zod-to-json-schema behavior for z.number().int()"type": "integer" is well-established and the CI step will catch any regression.

TypeScript compilation errors in the worktree are all pre-existing (bun:test types, beautiful-mermaid, gray-matter) — none introduced by this PR.

中文说明

代码审查

实现干净且与问题成比例。两层防御:

Schema 层tool-defs.tslimit 使用 z.number().int().min(1)offset 使用 z.number().int().min(0)。在 Zod 校验边界拦截畸形值,所有工具调用都经过这个边界。标准的 zod-to-json-schema 转换会为这些字段输出 "type": "integer",给 agent 提供正确的 schema 信号。

Handler 层handleListSessions() 在转发给 ctx.listSessions() 之前做 Number.isInteger() + 边界检查。这是对绕过 schema 校验的直接调用的纵深防御。校验逻辑与 Zod 约束完全一致——任何偏差都会是 bug。

未发现正确性问题。Number.isInteger() 正确拒绝 NaNInfinity 和小数。limit < 1(不是 <= 0)正确拒绝零——零 limit 对分页没有意义。

ci.yml 新增的 CI 步骤在完整 CI profile 下运行目标测试,遵循现有的守卫模式。好的补充——确保这些测试在 PR 中实际运行。

测试结果

handler 测试 4/4 通过(见上方输出)。tool-defs-filtering.test.ts 的 schema 类型断言因预先存在的 workspace 依赖解析问题无法在本地执行,但 zod-to-json-schema 对 z.number().int()"type": "integer" 的行为是确定的,CI 步骤会捕获任何回归。

TypeScript 编译错误均为预先存在的问题,本 PR 未引入任何新错误。

Qwen Code · qwen3.7-max

Reviewed at 403c0d3c6507db4a10b00500d531058a93831184 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — focused fix with clean validation, good test coverage, and a CI step; handler-layer duplication with the schema is intentional defense-in-depth, not waste.

This is a small, well-scoped PR that does exactly what it says. The Zod schema was missing integer constraints on pagination params — a clear gap that let limit: -1 produce sessions.slice(0, -1) silently. The fix tightens the schema to .int().min() and adds a matching handler guard. Tests cover the malformed cases, the boundary values, and the high-limit passthrough to the backend clamp. A new CI step ensures these tests actually run.

The handler validation is technically redundant with the schema for normal tool invocations, but that's a deliberate belt-and-suspenders pattern for direct handler calls — reasonable at this scale. No scope creep, no drive-by refactors, no unnecessary abstractions. The kind of PR that's easy to review, revert, and reason about.

Approving. ✅

中文说明

置信度: 4/5 — 聚焦的修复,校验干净,测试覆盖好,增加了 CI 步骤;handler 层与 schema 的重复是有意为之的纵深防御,不是浪费。

这是一个小而聚焦的 PR,完全按照描述工作。Zod schema 缺少分页参数的整数约束——一个明显的缺口,让 limit: -1 默默产生 sessions.slice(0, -1)。修复将 schema 收紧为 .int().min() 并添加了匹配的 handler 防御。测试覆盖了畸形值、边界值和高 limit 透传到后端 clamp 的场景。新增 CI 步骤确保这些测试实际运行。

handler 校验在正常工具调用中与 schema 技术上重复,但这是为直接 handler 调用设计的纵深防御——在这个规模下合理。没有范围蔓延,没有顺手重构,没有不必要的抽象。容易审查、回滚和理解的 PR。

批准 ✅

Qwen Code · qwen3.7-max

Reviewed at 403c0d3c6507db4a10b00500d531058a93831184 · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

Co-authored-by: qwen-code-ci-bot <253268222+qwen-code-ci-bot@users.noreply.github.com>

Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
VectorPeak and others added 2 commits July 19, 2026 10:46
…-pagination-validation

Co-authored-by: qwen-code-ci-bot <253268222+qwen-code-ci-bot@users.noreply.github.com>

Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
@VectorPeak

VectorPeak commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Reviewer feedback addressed.

  • Added boundary coverage for malformed limit: 0 and valid minimum pagination { limit: 1, offset: 0 }.
  • Added a full-profile CI step for the focused desktop list-sessions.test.ts regression.
  • Scoped that CI step to the handler test from this PR, while keeping broader session-tools-core coverage as local/WSL validation.
  • Synced the branch with latest upstream/main so the VS Code companion NOTICES.txt drift from the previous run is resolved.

Validation:

  • Windows: D:\ZXY\Dev\bun\bin\bun.exe test packages/desktop/packages/session-tools-core/src/handlers/list-sessions.test.ts -> 4 pass
  • Windows: D:\ZXY\Dev\bun\bin\bun.exe run typecheck in packages/desktop/packages/session-tools-core -> pass
  • WSL/Linux: npm exec --yes bun@1.3.14 -- test packages/desktop/packages/session-tools-core/src/handlers/list-sessions.test.ts -> 4 pass
  • WSL/Linux: npm exec --yes bun@1.3.14 -- test packages/desktop/packages/session-tools-core/src -> 65 pass
  • WSL/Linux: npm run generate:notices --workspace=qwen-code-vscode-ide-companion -> clean
  • Remote CI: latest Test (ubuntu-latest, Node 22.x) and web-shell E2E Smoke (ubuntu-latest, Node 22.x) are green.

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed — no blockers. Suggestions are inline.

中文说明

已审查——无阻断问题。 建议见行内评论。

— qwen3.8-max-preview via Qwen Code /review

Comment thread .github/workflows/ci.yml
Comment on lines +285 to +289
- name: 'Run desktop session-tools-core tests'
if: "${{ needs.classify_pr.outputs.skip_ci != 'true' && steps.ci_profile.outputs.ci_profile == 'full' }}"
run: |-
npm exec --yes bun@1.3.14 -- test \
packages/desktop/packages/session-tools-core/src/handlers/list-sessions.test.ts

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The CI step is named "Run desktop session-tools-core tests" but hardcodes only list-sessions.test.ts. The tool-defs-filtering.test.ts change made in this same PR (the type === 'integer' schema assertion) is not covered by any CI step — Concrete cost: if a future change reverts z.number().int() to z.number(), the schema test catches it locally but CI stays green, and the regression ships undetected.

Suggested change
- name: 'Run desktop session-tools-core tests'
if: "${{ needs.classify_pr.outputs.skip_ci != 'true' && steps.ci_profile.outputs.ci_profile == 'full' }}"
run: |-
npm exec --yes bun@1.3.14 -- test \
packages/desktop/packages/session-tools-core/src/handlers/list-sessions.test.ts
- name: 'Run desktop session-tools-core tests'
if: "${{ needs.classify_pr.outputs.skip_ci != 'true' && steps.ci_profile.outputs.ci_profile == 'full' }}"
run: |-
npm exec --yes bun@1.3.14 -- test \
packages/desktop/packages/session-tools-core/src/handlers/list-sessions.test.ts \
packages/desktop/packages/session-tools-core/src/tool-defs-filtering.test.ts
中文说明

CI 步骤名为 "Run desktop session-tools-core tests",但只硬编码了 list-sessions.test.ts。本 PR 同时修改的 tool-defs-filtering.test.ts(schema integer 类型断言)没有被任何 CI 步骤覆盖。具体代价:如果未来某个变更将 z.number().int() 回退为 z.number(),schema 测试在本地能捕获,但 CI 仍为绿色,回归会不被发现地合入。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +55 to +56
expect(properties?.limit?.type).toBe('integer');
expect(properties?.offset?.type).toBe('integer');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The schema test verifies only type === 'integer' (the .int() half) but not the .min(1) / .min(0) constraints this same diff introduces — Concrete cost: if .min(1)/.min(0) is later dropped from the schema, this test still passes and the schema metadata advertised to MCP clients silently regresses (runtime handling is independently protected by the handler validation, so the impact is bounded to client-side schema display).

Suggested change
expect(properties?.limit?.type).toBe('integer');
expect(properties?.offset?.type).toBe('integer');
expect(properties?.limit?.type).toBe('integer');
expect(properties?.offset?.type).toBe('integer');
expect((properties?.limit as Record<string, unknown>)?.minimum).toBe(1);
expect((properties?.offset as Record<string, unknown>)?.minimum).toBe(0);
中文说明

Schema 测试仅验证了 type === 'integer'(即 .int() 部分),但未断言本 diff 同时引入的 .min(1) / .min(0) 约束。具体代价:如果未来 .min(1)/.min(0) 被移除,此测试仍会通过,MCP 客户端看到的 schema 元数据会静默回归(运行时处理由 handler 层独立保护,因此影响仅限于客户端 schema 展示)。

— qwen3.8-max-preview via Qwen Code /review

@wenshao

wenshao commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Review — fix(desktop): validate list_sessions pagination params

No blockers. The schema tightening is the right fix in the right place, and I verified it end-to-end rather than by reading. Findings below are test-strength and CI-clarity only.

What I ran

Worktree at PR head a99b0248, bun 1.3.14 (same pin as the new CI step):

  • bun test .../handlers/list-sessions.test.ts4 pass, and notably it passes with no node_modules present at all — the new step is genuinely self-contained.
  • bun test .../tool-defs-filtering.test.ts8 pass (needed local dep stubs; see Finding 3).
  • Emitted JSON schema, printed from getToolDefsAsJsonSchema():
    limit = {"type":"integer","minimum":1,…} · offset = {"type":"integer","minimum":0,…}
  • A 13-mutant matrix across both files: 10 die, 3 survive.

Finding 1 — the schema's minimum is never asserted (3 surviving mutants)

tool-defs-filtering.test.ts:55-56 checks only type, so all of these keep the suite green:

mutant in tool-defs.ts:202-203 outcome
limit: .int().min(1).int() survived
offset: .int().min(0).int() survived
limit: .min(1).min(-100) survived

Two lines fix it — I applied them and re-ran the matrix: 13/13 mutants now die.

expect(properties?.limit).toMatchObject({ type: 'integer', minimum: 1 });
expect(properties?.offset).toMatchObject({ type: 'integer', minimum: 0 });

(widen the local cast to Record<string, { type?: string; minimum?: number }>)

Why this is the load-bearing assertion, not a nitpick → Finding 2.

Finding 2 — the handler guard is unreachable on both production paths today (context, not a defect)

There are exactly two dispatch sites for the canonical registry:

  1. In-process desktopshared/src/agent/session-scoped-tools.ts:253shared/src/mcp/local-tools.ts:50. The MCP SDK validates arguments against the zod schema before the callback (@modelcontextprotocol/sdk/dist/esm/server/mcp.jsvalidateToolInput()McpError(InvalidParams), downgraded to a graceful isError: true result). So limit: -1 is rejected by the schema and never reaches handleListSessions().
  2. Subprocess session-mcp-serversrc/index.ts:508 calls def.handler(ctx, toolArgs) with raw, unvalidated args. But createSessionMcpContext() (src/index.ts:113) never defines listSessions, so the handler returns list_sessions is not available in this context. before reaching the new guards.

So in production .int().min() is doing the entire job; the handler guards are cheap insurance for a future runtime that injects listSessionsFn into a non-validating path. Fine to keep — but two consequences worth knowing:

  • All 4 handler tests exercise the layer no production caller reaches, while the layer that actually enforces is the half-asserted one. That's why Finding 1's two lines are where the real regression protection lives.
  • An agent will never see [ERROR] limit must be a positive integer. — it gets the SDK's Input validation error: Invalid arguments for tool list_sessions: …. If the friendlier wording was a goal, that goal isn't reached on the live path.
  • Consider a one-line comment on the guards ("for runtimes that dispatch without schema validation"), otherwise the next reader sees dead code and deletes it.

Finding 3 — the CI step's name promises more than it runs, and it can't be broadened

I confirmed your stated constraint instead of taking it on faith. With root npm ci deps only, importing tool-defs.ts fails:

  • Cannot find package 'beautiful-mermaid' (via handlers/index.tsmermaid-validate.ts)
  • Cannot find package 'gray-matter' (via validation.ts)

Both are desktop-workspace-only deps that the root install deliberately excludes. And broadening the glob is not free either: bun test .../session-tools-core/src/handlers/2 of 6 files error out. Your single-file scope is correct.

That said, ci.yml:285 is named Run desktop session-tools-core tests while it runs 1 of the package's 12 test files — and tool-defs-filtering.test.ts, the test guarding the schema half of this PR, runs in no CI job at all. Suggest renaming (e.g. Run desktop list_sessions handler regression) and recording the reason inline so the next person doesn't broaden it into a red build or delete it as redundant:

      # Scoped to one file on purpose: the desktop tree is its own bun
      # workspace, so root `npm ci` has no beautiful-mermaid / gray-matter.
      # Anything importing tool-defs.ts cannot resolve under root CI.

Cost is a non-issue: 6s in this PR's own run (step 10:07:43Z → 10:07:49Z, job 89668475871).

Finding 4 — optional: the slice itself is still unguarded

SessionManager.ts:6024-6062 still computes Math.min(options?.limit ?? 20, 100) and sessions.slice(offset, offset + limit) with no floor. Nothing is exposed today (this handler is the only caller of listSessionsFn), but the invariant now lives two packages away from the slice it protects. A Math.max(1, Math.trunc(...)) there — or a shared bound constant imported by both the zod schema and the guard — would prevent exactly the schema/guard drift that Finding 1's mutants illustrate.

Checked, no concern

  • 20.0 from a model → JSON parses to 20; Number.isInteger(20) is true → no false rejection of float-formatted integers.
  • minimum survives core's schema converters (utils/schemaConverter.ts:117, openaiContentGenerator/converter.ts:292) → no provider-side breakage from the new keyword.
  • packages/desktop/ is in .prettierignore and eslint's ignore list, so the new files' formatting is outside root lint; the added trailing comma in the handler signature is harmless.
  • Profile classifier: any packages/desktop/** change ⇒ full, so the new step will fire on future desktop PRs.
  • Handler mutants H1–H7 all die: dropping both guards, < 1< 0, Number.isIntegertypeof, offset bound < 0< -1, swapping the two messages, and moving both guards below ctx.listSessions(). The "before listing sessions" claim in the test name is genuinely enforced, not incidental.
中文说明

无阻断问题。 schema 收紧是正确的修复,位置也对;我做了实际验证而非仅阅读代码。以下发现仅涉及测试强度与 CI 表述。

验证方式:在 PR head a99b0248 的独立 worktree 上用 bun 1.3.14(与新 CI 步骤同一 pin)运行。list-sessions.test.ts 4 pass,且在完全没有 node_modules 的情况下也能通过——说明新步骤确实自包含。tool-defs-filtering.test.ts 8 pass(需要本地 stub,见发现 3)。实际产出的 JSON schema 为 limit={"type":"integer","minimum":1}offset={"type":"integer","minimum":0}。13 个变异体:10 死 3 活。

发现 1(测试覆盖)tool-defs-filtering.test.ts:55-56 只断言 type,因此三个变异体存活——删掉 limit.min(1)、删掉 offset.min(0)、把 .min(1) 改成 .min(-100),测试全绿。改成 toMatchObject({ type: 'integer', minimum: 1 }) / minimum: 0 两行即可,我验证后 13/13 全部被杀死。

发现 2(背景,非缺陷):注册表只有两个分发路径。一是桌面进程内路径(session-scoped-tools.ts:253local-tools.ts:50),MCP SDK 在调用 callback 之前就用 zod schema 校验(validateToolInputInvalidParams,再降级为 isError: true),所以 limit: -1schema 拦下,根本到不了 handler。二是子进程 session-mcp-serverindex.ts:508 传原始参数、不做 zod 校验),但 createSessionMcpContext() 从未定义 listSessions,因此会在新守卫之前返回 "not available in this context"。结论:线上真正生效的是 .int().min(),handler 守卫属于面向未来运行时的廉价保险(可以保留)。两点影响:4 个 handler 测试覆盖的是线上到不了的那一层,而真正生效的那一层只被断言了一半——所以发现 1 的两行才是真正的回归保护;另外 agent 永远看不到 [ERROR] limit must be a positive integer.,它看到的是 SDK 的 Input validation error: …。建议给守卫加一行注释说明用途,否则下一个读者会当成死代码删掉。

发现 3(CI 步骤):我实测确认了作者的说法——只装根依赖时,导入 tool-defs.ts 会因 beautiful-mermaid(经 handlers/index.tsmermaid-validate.ts)和 gray-matter(经 validation.ts)失败;把 glob 扩到 src/handlers/ 则 6 个文件中有 2 个报错。所以只跑单文件是正确选择。但 ci.yml:285 的步骤名是 "Run desktop session-tools-core tests",实际只跑该包 12 个测试文件中的 1 个,而守护本 PR schema 那一半的 tool-defs-filtering.test.ts 在任何 CI job 中都不会运行。建议改名并把原因写成注释,避免后来者扩大范围导致 CI 变红、或误删该步骤。成本不是问题:本 PR 该步骤实测 6 秒(job 89668475871)。

发现 4(可选)SessionManager.ts:6024-6062Math.min(limit ?? 20, 100)slice(offset, offset + limit) 仍无下界保护。今天没有暴露面(该 handler 是唯一调用方),但不变量现在离它保护的 slice 隔了两个包。在那里加 Math.max(1, Math.trunc(...)),或让 schema 与守卫共用一个边界常量,可以避免发现 1 所揭示的漂移。

已核查、无需担心:模型给出 20.0 时 JSON 解析为 20Number.isInteger 为真,不会误拒;minimum 能通过 core 的 schema 转换器(schemaConverter.ts:117openaiContentGenerator/converter.ts:292),不会影响 provider;packages/desktop/.prettierignore 和 eslint ignore 中,新文件格式不受根 lint 约束,新增的尾随逗号无害;profile 分类器对任何 packages/desktop/** 变更都返回 full,新步骤在后续桌面 PR 上会执行;handler 侧 H1–H7 变异体全部死亡(删除两个守卫、< 1< 0Number.isIntegertypeof、offset 边界 < 0< -1、交换两条错误消息、把守卫移到 ctx.listSessions() 之后),测试名中 "before listing sessions" 的语义是真被断言到的。

@yiliang114

Copy link
Copy Markdown
Collaborator

Thanks for the contribution. Closing because the Electron desktop app (packages/desktop, including session-tools-core) was removed in #9085, so this PR targets code that no longer exists.

Session listing is now served by the qwen-code daemon (qwen serve) routes. If the pagination validation gap still exists there, please open a new issue pointing at the current route and we can re-target the fix.

@yiliang114 yiliang114 closed this Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants