fix(serve): reject fractional max connections - #5707
Conversation
|
@qwen-code /triage |
|
Thanks for the PR @tt-a1i! Template looks good ✓ On direction: clear bug fix — fractional connection counts are meaningless for a socket cap, and the existing validation was too permissive. Tightening this to reject fractional values matches the surrounding validation style for count-based serve options. No direction concerns. On approach: minimal and focused. Reuses the existing Moving on to code review. 🔍 中文说明感谢贡献 @tt-a1i! 模板完整 ✓ 方向:明确的验证逻辑修复——小数的连接数对 socket 上限没有意义,现有校验过于宽松。收紧为拒绝小数值与周围基于数量的 serve 选项校验风格一致,方向没有问题。 方案:最小化且聚焦。复用已有的 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewClean, focused change. The PR correctly replaces the inline The parameterized test consolidation (NaN → negative → fractional) is a good DRY improvement. Comment updates are accurate. No correctness bugs, no security issues, no regressions found. Unit TestsFocused max-connections tests: 6 passed (0, Infinity, valid integer 100, NaN, negative, fractional). Before (installed build)Fractional After (this PR)Fractional Before/after confirms the fix works as described. The daemon no longer silently accepts a meaningless fractional connection cap. 中文说明代码审查改动干净且聚焦。PR 正确地将内联的 参数化测试合并(NaN → 负数 → 小数)是一个好的 DRY 改进。注释更新准确。 未发现正确性 bug、安全问题或回归。 单元测试聚焦的 max-connections 测试:6 通过(0、Infinity、合法整数 100、NaN、负数、小数)。 对比测试修复前(已安装版本): 修复前/后对比确认修复符合描述。daemon 不再默默接受无意义的小数连接上限。 — Qwen Code · qwen3.7-max |
|
This is a clean, well-scoped validation fix. The PR does exactly what it says — tightens the The parameterized test consolidation is a nice touch — one Approving. ✅ 中文说明这是一个干净、范围恰当的验证逻辑修复。PR 做了它声称的事情——通过复用已有的辅助函数来收紧 参数化测试合并也很不错——一个 批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Same pattern as #5705. What scenario produces Feel free to reopen with a concrete reproduction. For future PRs, please show what actually breaks. 中文说明与 #5705 同一模式。 什么场景下会从环境变量或配置产生 欢迎带上具体复现案例重新打开。后续 PR 请展示实际会出什么问题。 |
What this PR does
Rejects fractional
maxConnectionsvalues inrunQwenServebefore the daemon listener starts.The existing default behavior is unchanged, and
0/Infinitystill keep the documented unlimited behavior by leavingserver.maxConnectionsunset.Why it's needed
maxConnectionsis a listener connection count cap. Fractional values like1.5are not meaningful for a socket count limit, but they were accepted and could be forwarded to Node'sserver.maxConnections.Failing at boot keeps the cap deterministic and matches the surrounding validation style for count-based serve options.
Reviewer Test Plan
How to verify
Run
npm test --workspace=packages/cli -- serve/server.test.ts -t max-connectionsand confirm the focused max-connections tests pass.Review
runQwenServevalidation and confirm fractionalmaxConnectionsvalues now throw while0,Infinity, and positive integers keep their existing behavior.Evidence (Before & After)
Before:
runQwenServe({ maxConnections: 1.5 })passed validation and forwarded a fractional connection cap toward the Node server.After: fractional
maxConnectionsthrows during boot. Tests cover0,Infinity, a valid integer,NaN, a negative value, and a fractional value.Tested on
Environment (optional)
Local validation on macOS:
npm test --workspace=packages/cli -- serve/server.test.ts -t max-connections✅npx prettier --check packages/cli/src/serve/run-qwen-serve.ts packages/cli/src/serve/server.test.ts✅npm run lint --workspace=packages/cli --if-present✅git diff --check✅npm run typecheck --workspace=packages/cli --if-presentsrc/ui/components/BaseTextInput.tsximports fromink/domandink/components/CursorContextRisk & Scope
maxConnectionswill now fail fast instead of relying on Node's handling of a fractional cap.0,Infinity, and the default behavior are unchanged.Linked Issues
Fixes #5706
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.
中文说明
What this PR does
这个 PR 会在 daemon listener 启动前,在
runQwenServe中拒绝小数形式的maxConnections。现有默认行为保持不变,
0/Infinity仍然保留文档中的无限制语义,也就是不设置server.maxConnections。Why it's needed
maxConnections是 listener 的连接数量上限。像1.5这样的小数对于 socket 数量限制没有意义,但之前会被接受,并可能继续传给 Node 的server.maxConnections。启动时直接失败能让这个 cap 的行为更确定,也和周围基于数量的 serve 选项校验风格一致。
Reviewer Test Plan
How to verify
运行
npm test --workspace=packages/cli -- serve/server.test.ts -t max-connections,确认 max-connections 的 focused tests 通过。检查
runQwenServe校验逻辑,确认小数maxConnections现在会抛错,同时0、Infinity和正整数保留现有行为。Evidence (Before & After)
Before:
runQwenServe({ maxConnections: 1.5 })可以通过校验,并把小数形式的连接 cap 继续传向 Node server。After:小数
maxConnections会在启动阶段抛错。测试覆盖了0、Infinity、合法整数、NaN、负数和小数。Tested on
Environment (optional)
本地 macOS 验证:
npm test --workspace=packages/cli -- serve/server.test.ts -t max-connections✅npx prettier --check packages/cli/src/serve/run-qwen-serve.ts packages/cli/src/serve/server.test.ts✅npm run lint --workspace=packages/cli --if-present✅git diff --check✅npm run typecheck --workspace=packages/cli --if-presentsrc/ui/components/BaseTextInput.tsx对ink/dom和ink/components/CursorContext的导入Risk & Scope
maxConnections的 embedded caller 现在会快速失败,不再依赖 Node 对小数 cap 的处理。0、Infinity和默认行为都保持不变。Linked Issues
Fixes #5706
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.