fix(core): reject fractional LSP limit inputs#6455
Conversation
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
|
Thanks for the PR! (Re-run after update Template looks good ✓ — all required sections present, bilingual. Problem: Real validation gap. The schema declared 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 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, Moving on to code review. 🔍 中文说明感谢贡献!(更新 模板完整 ✓ — 所有必填章节齐全,双语。 问题: 真实的校验缺口。Schema 把 方向: 对齐 — 在工具边界收紧输入校验是正确位置。PR 只处理 Qwen 扩展的 规模: 2 个文件,32 行新增 / 6 行删除。生产代码约 3 行(schema 改动 + 移除冗余运行时检查),测试约 29 行。范围合理。 方案: 精简。更新后增加了 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe updated change is clean and correct:
No blockers. No AGENTS.md violations. Test ResultsRan the LSP test suite and typecheck against the PR diff: Typecheck: clean (no errors). Real-Scenario TestingN/A — this is a non-UI schema validation change. The behavior is fully covered by the focused unit tests above: 中文说明代码审查更新后的改动正确且干净:
无阻塞问题,无 AGENTS.md 违规。 测试结果对 PR diff 运行了 LSP 测试和类型检查: Typecheck:无错误。 真实场景测试不适用 — 这是非 UI 的 schema 校验改动,上述聚焦单测已完整覆盖: — Qwen Code · qwen3.7-max |
|
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 ( My independent proposal would have been identical: change 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 Approving. ✅ 中文说明这是一个干净、最小化的校验修复——范围窄、质量好,容易合入。 作者的更新提交( 我的独立方案完全一致: 73 个单测全部通过,typecheck 无错误,测试覆盖合理(零、负数、小数边界用例 + schema 断言)。范围紧凑——2 个文件、约 3 行生产代码、约 29 行测试——无顺手重构或范围蔓延。 问题真实存在(小数 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| }, | ||
| limit: { | ||
| type: 'number', | ||
| type: 'integer', |
There was a problem hiding this comment.
[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.
| 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
There was a problem hiding this comment.
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.
| return 'limit must be a positive number.'; | ||
| if ( | ||
| params.limit !== undefined && | ||
| (!Number.isInteger(params.limit) || params.limit <= 0) |
There was a problem hiding this comment.
[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:
| (!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
There was a problem hiding this comment.
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', () => { |
There was a problem hiding this comment.
[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
There was a problem hiding this comment.
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.
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
|
@DragonnZhang Thanks again for the review. I addressed the CI bot's latest feedback in 95dc4e2:
Validation after rebasing on the latest PR branch:
Small note: Windows focused test/typecheck passed before the final rebase, but the shared Windows/WSL |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
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
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
What this PR does
This PR tightens the LSP tool's
limitparameter 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
lineorcharacter, and it does not change the existing default result limits.Why it's needed
What Problem This Solves
limitcontrols 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
limitasnumber, 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,
limitis a discrete count, not a continuous numeric measurement: returning1,2, or50results is meaningful, but returning1.5results is not.Changes
This PR narrows only the Qwen LSP
limitextension fromnumbertointegerin the tool schema and adds a runtime integer guard for the same field.It intentionally leaves
lineandcharacterunchanged 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 withlimitparameters 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 declareslimitasinteger, so fractionallimitinputs 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.5is rejected,limit: 0remains rejected,limit: 5continues to be passed to LSP client methods, andline/characterremain schema-compatible asnumber.Possible call chain / impact
A possible path for the bad input is:
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/characterbehavior, default result caps, or LSP server interaction semantics for valid integer limits.Reviewer Test Plan
How to verify
A reviewer can confirm that fractional
limitvalues are rejected while existing positive integer limits continue to work.Expected behavior after this change:
limit: 5remains valid and is still passed through to LSP client methods.limit: 0remains invalid.limit: 1.5is now invalid because LSP result limits are discrete counts.limitvalues keep the existing defaults, such as20or50depending on the operation.lineandcharacterschema 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 --checkFocused validation also passed in WSL / Ubuntu-22.04:
Tested on
Environment (optional)
Windows local validation used Node.js
v24.15.0and npm11.12.1in the clean worktree. Linux validation used WSL Ubuntu-22.04 with Node.jsv24.15.0and npm11.12.1;npm installwas run in WSL first so Linux optional dependencies such as Rollup's native package were available.Risk & Scope
limitextension, so invalid fractional values such as1.5are rejected instead of being accepted and passed into result slicing or client calls.lineorcharacter, source-position coordinates, default result limits, LSP client behavior, or other tools withlimitparameters.limitinputs 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是离散计数,不是连续数值;返回1、2或50条结果是有意义的,返回1.5条结果没有明确语义。Changes
这个 PR 只把 Qwen LSP 扩展字段
limit的 schema 从number收窄为integer,并为同一个字段增加运行时整数校验。本 PR 刻意不修改
line和character,因为它们属于当前测试中已有的 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仍保持numberschema 兼容行为。Possible call chain / impact
潜在调用链如下:
用户可见影响很小但真实存在:非法的小数结果上限过去可能进入结果截断或 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时仍然使用现有默认值,例如不同操作下的20或50。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 --checkWSL / Ubuntu-22.04 聚焦验证也已通过:
Tested on
Environment(可选)
Windows 本地验证使用 Node.js
v24.15.0和 npm11.12.1,在干净 worktree 中执行。Linux 验证使用 WSL Ubuntu-22.04,Node.jsv24.15.0,npm11.12.1;测试前先在 WSL 中运行npm install,以补齐 Rollup 等 Linux optional dependencies。风险和范围
limit,例如1.5,避免它继续进入结果截断或 LSP client 调用。line/character等 Claude-compatible core LSP 字段,不修改源码坐标字段,不修改默认结果数量,不修改 LSP client 行为,也不修改其他工具里的limit参数。limit现在会校验失败。已有正整数limit和省略limit的调用保持原行为。Linked Issues
N/A — 独立的小范围校验修复;暂未找到已有 issue。