Skip to content

fix(core): reject fractional LSP limit inputs#6455

Merged
wenshao merged 5 commits into
QwenLM:mainfrom
VectorPeak:codex/lsp-integer-params
Jul 8, 2026
Merged

fix(core): reject fractional LSP limit inputs#6455
wenshao merged 5 commits into
QwenLM:mainfrom
VectorPeak:codex/lsp-integer-params

Conversation

@VectorPeak

@VectorPeak VectorPeak commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What this PR does

This PR tightens the LSP tool's limit parameter so it only accepts positive integer result counts.

The change is intentionally narrow: it only affects Qwen's LSP result-limit extension. It does not change the Claude-compatible core LSP fields such as line or character, and it does not change the existing default result limits.

Why it's needed

What Problem This Solves

limit controls how many LSP results may be returned for operations such as definitions, implementations, references, workspace symbols, call hierarchy, diagnostics, and code actions.

Before this change, the schema exposed limit as number, and runtime validation only rejected non-positive values. A fractional value such as:

{
  "operation": "documentSymbol",
  "filePath": "src/app.ts",
  "limit": 1.5
}

could pass the LSP tool's positive-number validation path even though a fractional result count has no meaningful semantics. In other words, limit is a discrete count, not a continuous numeric measurement: returning 1, 2, or 50 results is meaningful, but returning 1.5 results is not.

Changes

This PR narrows only the Qwen LSP limit extension from number to integer in the tool schema and adds a runtime integer guard for the same field.

It intentionally leaves line and character unchanged because those fields are part of the existing Claude-compatible LSP core surface in this repository's tests. The PR also leaves default limits, LSP client behavior, result formatting, and other tools with limit parameters unchanged.

Evidence

The malformed input is small and deterministic:

{
  "operation": "documentSymbol",
  "filePath": "src/app.ts",
  "limit": 1.5
}

Before this change, the custom validation only checked limit <= 0, so a positive fractional value could pass that validation path. After this change, the schema declares limit as integer, so fractional limit inputs are rejected by schema validation before they reach execution; the runtime guard also rejects non-integer values if it is reached outside the normal schema-first path.

Focused validation now covers both sides of the contract: limit: 1.5 is rejected, limit: 0 remains rejected, limit: 5 continues to be passed to LSP client methods, and line / character remain schema-compatible as number.

Possible call chain / impact

A possible path for the bad input is:

LspTool.validateToolParams(...)
  -> schema validation / validateToolParamValues(...)
  -> operation-specific LSP execution
  -> result slicing such as slice(0, limit)
  -> LSP client calls that receive limit as a result cap

The user-visible impact is small but real: an invalid fractional result cap could previously flow into result slicing or client calls, leaving behavior to JavaScript/index coercion or downstream client handling. This PR moves that rejection to the tool boundary, where the contract is clearest.

This PR does not change source-position coordinates, Claude-compatible line / character behavior, default result caps, or LSP server interaction semantics for valid integer limits.

Reviewer Test Plan

How to verify

A reviewer can confirm that fractional limit values are rejected while existing positive integer limits continue to work.

Expected behavior after this change:

  • limit: 5 remains valid and is still passed through to LSP client methods.
  • limit: 0 remains invalid.
  • limit: 1.5 is now invalid because LSP result limits are discrete counts.
  • Omitted limit values keep the existing defaults, such as 20 or 50 depending on the operation.
  • line and character schema compatibility is intentionally unchanged in this PR.

Evidence (Before & After)

N/A — non-UI validation via focused unit tests.

Focused validation passed locally on Windows:

npm run test --workspace=packages/core -- src/tools/lsp.test.ts
npm run typecheck --workspace=packages/core
git diff --check

Focused validation also passed in WSL / Ubuntu-22.04:

wsl -d Ubuntu-22.04 -- bash -lc 'cd /mnt/d/ZXY/Github/qwen-code-lsp-integer && npm run test --workspace=packages/core -- src/tools/lsp.test.ts'
wsl -d Ubuntu-22.04 -- bash -lc 'cd /mnt/d/ZXY/Github/qwen-code-lsp-integer && npm run typecheck --workspace=packages/core'

Tested on

OS Status
🍏 macOS ⚠️ not tested locally
🪟 Windows ✅ tested
🐧 Linux ✅ tested via WSL Ubuntu-22.04

Environment (optional)

Windows local validation used Node.js v24.15.0 and npm 11.12.1 in the clean worktree. Linux validation used WSL Ubuntu-22.04 with Node.js v24.15.0 and npm 11.12.1; npm install was run in WSL first so Linux optional dependencies such as Rollup's native package were available.

Risk & Scope

  • Main risk or tradeoff: This tightens validation for Qwen's LSP-only limit extension, so invalid fractional values such as 1.5 are rejected instead of being accepted and passed into result slicing or client calls.
  • Not validated / out of scope: This PR does not change Claude-compatible core LSP properties such as line or character, source-position coordinates, default result limits, LSP client behavior, or other tools with limit parameters.
  • Breaking changes / migration notes: Invalid fractional limit inputs now fail validation. Existing positive integer limits and omitted limits keep the same behavior.

Linked Issues

N/A — independent narrowly scoped validation fix; no existing issue found.

中文说明

这个 PR 做了什么

这个 PR 收紧 LSP 工具的 limit 参数校验,让它只接受正整数结果数量。

改动范围刻意保持很小:这里只处理 Qwen 自己扩展的 LSP 结果数量上限,不修改 line / character 这类已有 Claude Code compatibility 断言覆盖的核心字段,也不修改现有默认返回数量。

为什么需要它

What Problem This Solves

limit 表示 LSP 操作最多返回多少条结果,例如 definitions、implementations、references、workspace symbols、call hierarchy、diagnostics 和 code actions。

在修改前,schema 把 limit 暴露为 number,运行时校验只拒绝非正数。因此下面这种小数值可能通过正数校验:

{
  "operation": "documentSymbol",
  "filePath": "src/app.ts",
  "limit": 1.5
}

但“返回 1.5 条结果”没有明确语义。limit 是离散计数,不是连续数值;返回 1250 条结果是有意义的,返回 1.5 条结果没有明确语义。

Changes

这个 PR 只把 Qwen LSP 扩展字段 limit 的 schema 从 number 收窄为 integer,并为同一个字段增加运行时整数校验。

本 PR 刻意不修改 linecharacter,因为它们属于当前测试中已有的 Claude-compatible LSP core surface。本 PR 也不修改默认 limit、LSP client 行为、结果格式化逻辑,或其他工具里的 limit 参数。

Evidence

触发输入很小且确定:

{
  "operation": "documentSymbol",
  "filePath": "src/app.ts",
  "limit": 1.5
}

修改前,自定义校验只检查 limit <= 0,所以正小数可能通过这一路校验。修改后,schema 将 limit 声明为 integer,小数 limit 会在进入执行前被 schema 校验拒绝;如果绕过正常 schema-first 路径,运行时 guard 也会拒绝非整数。

聚焦测试现在同时覆盖了这个契约的两边:limit: 1.5 会被拒绝,limit: 0 仍然会被拒绝,limit: 5 继续传给 LSP client,line / character 仍保持 number schema 兼容行为。

Possible call chain / impact

潜在调用链如下:

LspTool.validateToolParams(...)
  -> schema validation / validateToolParamValues(...)
  -> operation-specific LSP execution
  -> result slicing such as slice(0, limit)
  -> LSP client calls that receive limit as a result cap

用户可见影响很小但真实存在:非法的小数结果上限过去可能进入结果截断或 client 调用,让行为依赖 JavaScript/index coercion 或下游 client 的处理。这个 PR 把拒绝动作前移到工具边界,也就是契约最清楚的位置。

本 PR 不修改源码位置坐标、不修改 Claude-compatible line / character 行为、不修改默认结果数量,也不修改合法整数 limit 下的 LSP server 交互语义。

Reviewer 测试计划

如何验证

Reviewer 可以确认小数 limit 会被拒绝,同时已有正整数 limit 继续可用。

预期行为:

  • limit: 5 仍然合法,并继续传给 LSP client。
  • limit: 0 仍然非法。
  • limit: 1.5 现在会被拒绝,因为 LSP 结果数量上限是离散计数。
  • 不传 limit 时仍然使用现有默认值,例如不同操作下的 2050
  • 本 PR 不修改 line / character 的 schema compatibility 行为。

Evidence(修改前后)

N/A — 这是非 UI 改动,通过聚焦单测验证。

Windows 本地聚焦验证已通过:

npm run test --workspace=packages/core -- src/tools/lsp.test.ts
npm run typecheck --workspace=packages/core
git diff --check

WSL / Ubuntu-22.04 聚焦验证也已通过:

wsl -d Ubuntu-22.04 -- bash -lc 'cd /mnt/d/ZXY/Github/qwen-code-lsp-integer && npm run test --workspace=packages/core -- src/tools/lsp.test.ts'
wsl -d Ubuntu-22.04 -- bash -lc 'cd /mnt/d/ZXY/Github/qwen-code-lsp-integer && npm run typecheck --workspace=packages/core'

Tested on

OS Status
🍏 macOS ⚠️ 本地未测试
🪟 Windows ✅ 已测试
🐧 Linux ✅ 已通过 WSL Ubuntu-22.04 测试

Environment(可选)

Windows 本地验证使用 Node.js v24.15.0 和 npm 11.12.1,在干净 worktree 中执行。Linux 验证使用 WSL Ubuntu-22.04,Node.js v24.15.0,npm 11.12.1;测试前先在 WSL 中运行 npm install,以补齐 Rollup 等 Linux optional dependencies。

风险和范围

  • 主要风险或取舍:这个 PR 会更严格地拒绝以前被模糊接受的小数 limit,例如 1.5,避免它继续进入结果截断或 LSP client 调用。
  • 未验证 / 不在范围内:本 PR 不修改 line / character 等 Claude-compatible core LSP 字段,不修改源码坐标字段,不修改默认结果数量,不修改 LSP client 行为,也不修改其他工具里的 limit 参数。
  • Breaking changes / migration notes:非法的小数 limit 现在会校验失败。已有正整数 limit 和省略 limit 的调用保持原行为。

Linked Issues

N/A — 独立的小范围校验修复;暂未找到已有 issue。

VectorPeak and others added 2 commits July 7, 2026 20:24
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 7, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR! (Re-run after update 95dc4e2)

Template looks good ✓ — all required sections present, bilingual.

Problem: Real validation gap. The schema declared limit as number and the runtime only checked <= 0, so fractional values like 1.5 passed validation despite having no meaningful semantics as a result count. Narrow but genuine.

Direction: Aligned — tightening input validation at the tool boundary is the right place for it. The PR is careful to only touch the Qwen-specific limit extension, leaving line/character (Claude-compatible core) unchanged.

Size: 32 additions / 6 deletions across 2 files. Production: ~3 lines (schema change + removal of redundant runtime check). Tests: ~29 lines. Well within normal scope.

Approach: Clean and minimal. After the update, minimum: 1 was added to the schema (per prior feedback), and the now-redundant custom runtime check was removed — schema validation is the single source of truth. Good simplification.

Moving on to code review. 🔍

中文说明

感谢贡献!(更新 95dc4e2 后重新审查)

模板完整 ✓ — 所有必填章节齐全,双语。

问题: 真实的校验缺口。Schema 把 limit 声明为 number,运行时只检查 <= 0,因此 1.5 等小数可以通过校验,但"返回 1.5 条结果"没有实际语义。范围窄但问题真实存在。

方向: 对齐 — 在工具边界收紧输入校验是正确位置。PR 只处理 Qwen 扩展的 limit 字段,不动 line/character(Claude 兼容核心字段)。

规模: 2 个文件,32 行新增 / 6 行删除。生产代码约 3 行(schema 改动 + 移除冗余运行时检查),测试约 29 行。范围合理。

方案: 精简。更新后增加了 minimum: 1(根据之前反馈),并移除了冗余的自定义运行时校验——schema 校验成为唯一防线。很好的简化。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code Review

The updated change is clean and correct:

  • Schema (lsp.ts:1084-1085): type: 'integer' + minimum: 1 — single source of truth for limit validation. Covers positive-only and integer constraints in one place.
  • Runtime (lsp.ts:1216-1219): The old params.limit <= 0 check is removed. Schema now handles both <= 0 and fractional rejection — no duplication.
  • Tests: Three new edge cases (zero → >= 1 message, negative → >= 1 message, fractional → integer message) plus a schema assertion test. Good coverage of the validation contract.
  • No unintended changes: line/character validation and their number type are untouched. Default limits unchanged. No scope creep.

No blockers. No AGENTS.md violations.

Test Results

Ran the LSP test suite and typecheck against the PR diff:

✓ src/tools/lsp.test.ts (73 tests) 361ms

 Test Files  1 passed (1)
      Tests  73 passed (73)

Typecheck: clean (no errors).

Real-Scenario Testing

N/A — this is a non-UI schema validation change. The behavior is fully covered by the focused unit tests above: limit: 0 rejected, limit: -1 rejected, limit: 1.5 rejected, limit: 5 still accepted. There is no user-visible TUI surface to test with tmux.

中文说明

代码审查

更新后的改动正确且干净:

  • Schemalsp.ts:1084-1085):type: 'integer' + minimum: 1 — limit 校验的唯一防线,同时覆盖正数和整数约束。
  • 运行时lsp.ts:1216-1219):移除了旧的 params.limit <= 0 检查。Schema 现在同时处理 <= 0 和小数拒绝——无重复。
  • 测试:三个新边界用例(零 → >= 1,负数 → >= 1,小数 → integer)加一个 schema 断言测试。覆盖了校验契约。
  • 无意外改动line/character 校验和 number 类型未动。默认 limit 不变。无范围蔓延。

无阻塞问题,无 AGENTS.md 违规。

测试结果

对 PR diff 运行了 LSP 测试和类型检查:

✓ src/tools/lsp.test.ts (73 tests) 361ms

 Test Files  1 passed (1)
      Tests  73 passed (73)

Typecheck:无错误。

真实场景测试

不适用 — 这是非 UI 的 schema 校验改动,上述聚焦单测已完整覆盖:limit: 0 被拒绝,limit: -1 被拒绝,limit: 1.5 被拒绝,limit: 5 仍被接受。没有可用 tmux 测试的用户可见 TUI 界面。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

This is a clean, minimal validation fix — exactly the kind of narrow, well-scoped contribution that's easy to ship.

The author's updated commit (95dc4e2) addressed prior feedback well: minimum: 1 was added to the schema, and the now-redundant custom runtime check was removed. The result is simpler than the original PR — schema validation as the single source of truth, no duplicated logic.

My independent proposal would have been identical: change type: 'number' to type: 'integer', add minimum: 1, remove the custom <= 0 check. The PR matches this exactly.

All 73 unit tests pass, typecheck is clean, and the test coverage is appropriate (zero, negative, fractional edge cases plus a schema assertion). The scope is tight — 2 files, ~3 production lines, ~29 test lines — with no drive-by refactors or scope creep.

The problem is real (fractional limit had no meaningful semantics and could leak into slice(0, limit) or LSP client calls), the fix is at the right layer (tool boundary), and the risk is negligible (only previously-invalid inputs now fail earlier with clearer messages).

Approving. ✅

中文说明

这是一个干净、最小化的校验修复——范围窄、质量好,容易合入。

作者的更新提交(95dc4e2)很好地回应了之前的反馈:在 schema 中增加了 minimum: 1,并移除了冗余的自定义运行时检查。结果比初始 PR 更简洁——schema 校验作为唯一防线,无重复逻辑。

我的独立方案完全一致:type: 'number' 改为 type: 'integer',加 minimum: 1,移除自定义 <= 0 检查。PR 与我的方案完全匹配。

73 个单测全部通过,typecheck 无错误,测试覆盖合理(零、负数、小数边界用例 + schema 断言)。范围紧凑——2 个文件、约 3 行生产代码、约 29 行测试——无顺手重构或范围蔓延。

问题真实存在(小数 limit 没有实际语义,可能泄漏到 slice(0, limit) 或 LSP client 调用),修复在正确的层(工具边界),风险可忽略(只有原本就非法的输入会更早失败,错误信息更清晰)。

批准 ✅

Qwen Code · qwen3.7-max

@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. ✅

},
limit: {
type: 'number',
type: '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 declares type: 'integer' but omits minimum: 1. The positivity constraint is enforced only by the custom runtime check (params.limit <= 0). Adding minimum: 1 would make the constraint declarative and consistent with the type: 'integer' approach, letting AJV reject 0 and negative values at the schema layer.

Suggested change
type: 'integer',
limit: {
type: 'integer',
minimum: 1,
description: 'Optional maximum number of results to return.',
},

Note: if you add minimum: 1, the existing limit: 0 test's error message would change from 'limit must be a positive integer.' (custom) to the AJV message — the test assertion would need updating.

— qwen3.7-max via Qwen Code /review

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 in 95dc4e2. limit now declares minimum: 1 alongside type: 'integer', and the schema compatibility test now asserts both the integer type and minimum bound.

Comment thread packages/core/src/tools/lsp.ts Outdated
return 'limit must be a positive number.';
if (
params.limit !== undefined &&
(!Number.isInteger(params.limit) || params.limit <= 0)

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 !Number.isInteger(params.limit) check is dead code in the normal validation path. The base class validateToolParams runs AJV schema validation first (tools.ts:401-413), which rejects non-integers via type: 'integer' before this custom validator is ever called. The test at lsp.test.ts:258 confirms this — it asserts the AJV error 'params/limit must be integer', not the custom message.

Consider either removing the redundant check:

Suggested change
(!Number.isInteger(params.limit) || params.limit <= 0)
if (params.limit !== undefined && params.limit <= 0) {
return 'limit must be a positive integer.';
}

Or adding a comment explaining it's a defense-in-depth fallback.

— qwen3.7-max via Qwen Code /review

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 in 95dc4e2. Since schema validation now owns both integer and minimum: 1, I removed the redundant custom limit runtime check instead of keeping a second validation layer with competing messages.

expect(result).toBe('limit must be a positive integer.');
});

it('rejects fractional limit', () => {

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] Consider adding a test for a negative integer limit (e.g., limit: -1). The existing limit: 0 test exercises the <= 0 boundary, but a negative value would more thoroughly cover the rejection path:

it('rejects negative integer limit', () => {
  const result = tool.validateToolParams({
    operation: 'documentSymbol',
    filePath: 'src/app.ts',
    limit: -1,
  } as LspToolParams);
  expect(result).toBe('limit must be a positive integer.');
});

— qwen3.7-max via Qwen Code /review

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 in 95dc4e2. Added a negative integer limit test for limit: -1, and updated the non-positive limit expectation to the schema-layer AJV message: params/limit must be >= 1.

VectorPeak and others added 2 commits July 7, 2026 21:49
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
@VectorPeak

Copy link
Copy Markdown
Contributor Author

@DragonnZhang Thanks again for the review. I addressed the CI bot's latest feedback in 95dc4e2:

  • added minimum: 1 to the LSP limit schema
  • removed the now-redundant custom runtime limit validator so schema validation owns integer/minimum checks
  • added the negative integer limit: -1 test
  • extended the schema compatibility test to assert minimum: 1

Validation after rebasing on the latest PR branch:

  • WSL Ubuntu-22.04: npm run test --workspace=packages/core -- src/tools/lsp.test.ts passed, 73 tests
  • WSL Ubuntu-22.04: npm run typecheck --workspace=packages/core passed
  • git diff --check passed

Small note: Windows focused test/typecheck passed before the final rebase, but the shared Windows/WSL node_modules later flipped Rollup optional dependencies back to the Linux package, so I used the WSL post-rebase validation as the final clean proof. The code change itself remains scoped only to the limit result-count extension; line/character compatibility fields are still untouched.

@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.

No review findings. Downgraded from Approve to Comment: CI still running.

This is a clean, well-scoped validation fix. The schema change (type: 'integer' + minimum: 1) correctly subsumes the old runtime check, and the test coverage is thorough (zero, negative, fractional boundary cases plus schema property assertions). All 73 LSP tests pass.

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

@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. ✅

@wenshao
wenshao added this pull request to the merge queue Jul 8, 2026
Merged via the queue into QwenLM:main with commit faf7c43 Jul 8, 2026
35 checks passed
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.

3 participants