fix(cli): reject malformed ACP timeout strings

[tt-a1i]

What this PR does

Makes ACP settings timeout normalization strict. The normalizeOptionalNumber helper used by the ACP settings paths previously ran parseInt(String(value), 10) on incoming timeout values, which silently truncated malformed strings — "10ms" was parsed as 10 and "1.5" as 1, then persisted. The helper now accepts a value only when it is a number or a string of pure digits (/^\d+$/); anything else (suffixes, fractional, non-numeric) becomes NaN. It then requires the result to be a positive integer (Number.isInteger(...) && value > 0) and rejects everything else with an invalidParams error reading Expected a positive integer. This applies to the MCP server timeout and hook command timeout settings handled over ACP (qwen/settings/setMcpServer, qwen/settings/setHook).

Why it's needed

Per #5313, ACP settings normalization accepted malformed timeout values because normalizeOptionalNumber used parseInt. Values like "10ms" or "1.5" were accepted and persisted as truncated numbers (10, 1) instead of being rejected as invalid params, silently corrupting MCP server and hook timeout settings. The nearby env parser already avoids this kind of truncation, so the ACP request/config path should be strict too. This rejects suffixed/partial numbers up front rather than persisting a wrong value.

Reviewer Test Plan

How to verify

Repro: send an ACP settings request with a malformed timeout. Before this change, { timeout: '10ms' } was accepted and persisted as 10; after it, the request is rejected with Expected a positive integer. A pure-digit string such as '1500' is still accepted and persisted as the number 1500.

Verification commands (run from repo root):

• npx vitest run src/acp-integration/acpAgent.test.ts -t "malformed timeout strings"
• npx vitest run src/acp-integration/acpAgent.test.ts
• npm run typecheck --workspace=packages/cli
• npm run lint
• npx prettier --experimental-cli --check packages/cli/src/acp-integration/acpAgent.ts packages/cli/src/acp-integration/acpAgent.test.ts
• git diff --check

Evidence (Before & After)

N/A — non-visible logic fix; covered by the unit tests above (new regression tests assert that '10ms' and '1.5' are rejected with Expected a positive integer, and that '1500' is persisted as 1500).

Tested on

OS	Status
🍏 macOS	✅ CI
🪟 Windows	✅ CI
🐧 Linux	✅ CI

✅ tested · ⚠️ not tested · N/A

Environment (optional)

Unit tests only (npm workspaces). Note: npm run build --workspace=packages/cli failed locally for the author because pre-existing ignored dist output under packages/core/dist is treated as TypeScript input (TS5055 overwrite-input errors), unrelated to this change; CI builds and tests pass on all three platforms.

Risk & Scope

• Main risk or tradeoff: Low; scoped to ACP settings timeout normalization (normalizeOptionalNumber). The behavior change is that previously-truncated malformed timeout strings are now rejected instead of silently persisted as wrong values; valid numeric/integer-string inputs are unaffected.
• Not validated / out of scope: No unrelated refactors, no public API changes, no UI changes.
• Breaking changes / migration notes: None. Inputs that were silently accepted as wrong values are now rejected with a clear error; any caller relying on the old truncation was already getting incorrect timeouts.

Linked Issues

Fixes #5313

中文说明

这个 PR 做了什么

让 ACP 设置中的超时（timeout）归一化变得严格。ACP 设置路径所用的 normalizeOptionalNumber 辅助函数此前对传入的超时值执行 parseInt(String(value), 10)，这会悄悄截断格式错误的字符串——"10ms" 被解析成 10，"1.5" 被解析成 1，随后被持久化保存。现在该辅助函数仅在值为数字、或为纯数字字符串（/^\d+$/）时才接受；其他任何情况（带后缀、带小数、非数字）都会变成 NaN。随后它要求结果必须是正整数（Number.isInteger(...) && value > 0），否则以 invalidParams 错误拒绝，错误信息为 Expected a positive integer。此逻辑作用于通过 ACP 处理的 MCP server 超时和 hook 命令超时设置（qwen/settings/setMcpServer、qwen/settings/setHook）。

为什么需要它

根据 #5313，ACP 设置归一化因为 normalizeOptionalNumber 使用了 parseInt 而接受了格式错误的超时值。像 "10ms" 或 "1.5" 这样的值被接受并被截断持久化为 10、1，而不是作为非法参数被拒绝，从而悄悄损坏 MCP server 和 hook 的超时设置。附近的环境变量解析器本就避免了这种截断，因此 ACP 的 request/config 路径也应当严格。本改动会提前拒绝带后缀/不完整的数字，而不是持久化一个错误的值。

审阅者测试计划

如何验证

复现：发送一个带有格式错误超时值的 ACP 设置请求。在本改动之前，{ timeout: '10ms' } 会被接受并持久化为 10；改动之后，该请求会被拒绝并报 Expected a positive integer。纯数字字符串（如 '1500'）仍会被接受并持久化为数字 1500。

验证命令（在仓库根目录执行）：

• npx vitest run src/acp-integration/acpAgent.test.ts -t "malformed timeout strings"
• npx vitest run src/acp-integration/acpAgent.test.ts
• npm run typecheck --workspace=packages/cli
• npm run lint
• npx prettier --experimental-cli --check packages/cli/src/acp-integration/acpAgent.ts packages/cli/src/acp-integration/acpAgent.test.ts
• git diff --check

证据（前后对比）

不适用 —— 这是非可见的逻辑修复；由上述单元测试覆盖（新增的回归测试断言 '10ms' 和 '1.5' 会被拒绝并报 Expected a positive integer，且 '1500' 会被持久化为 1500）。

测试平台

操作系统	状态
🍏 macOS	✅ CI
🪟 Windows	✅ CI
🐧 Linux	✅ CI

✅ 已测试 · ⚠️ 未测试 · 不适用

环境（可选）

仅单元测试（npm workspaces）。说明：作者本地运行 npm run build --workspace=packages/cli 失败，原因是 packages/core/dist 下已被忽略的旧 dist 产物被当作 TypeScript 输入（TS5055 覆盖输入错误），与本改动无关；CI 在三个平台上的构建与测试均通过。

风险与范围

• 主要风险或权衡：低；范围限于 ACP 设置超时归一化（normalizeOptionalNumber）。行为变化在于：此前被截断的格式错误超时字符串现在会被拒绝，而不是被悄悄持久化为错误值；合法的数字/整数字符串输入不受影响。
• 未验证 / 范围之外：无无关重构，无公共 API 变更，无 UI 变更。
• 破坏性变更 / 迁移说明：无。此前被悄悄接受为错误值的输入现在会以清晰的错误被拒绝；任何依赖旧截断行为的调用方此前本就拿到的是错误的超时值。

关联 Issue

Fixes #5313

AI Assistance Disclosure

I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.

[wenshao]

@qwen-code /triage

[qwen-code-ci-bot]

Hey @tt-a1i — thanks for the fix! The code change itself looks focused and reasonable, but the PR body doesn't follow the PR template. The template requires specific sections:

• What this PR does — prose description of the change
• Why it's needed — motivation / problem being solved
• Reviewer Test Plan — with "How to verify", "Evidence (Before & After)", and "Tested on" subsections
• Risk & Scope — main risk, out of scope, breaking changes
• Linked Issues — Fixes #N / Closes #N
• 中文说明 — Chinese translation in a <details> block

Your current body has "Summary", "Testing", and "AI Assistance Disclosure" which don't map to the template. Could you reformat the PR description to match? It helps reviewers (and the triage bot) evaluate the change efficiently.

Once that's updated I'll re-run triage. 🙏

中文说明

@tt-a1i 你好——感谢提交修复！代码改动本身很聚焦，但 PR 描述没有按照 PR 模板 填写。模板要求以下章节：

• What this PR does — 改动的文字描述
• Why it's needed — 动机/要解决的问题
• Reviewer Test Plan — 包含"如何验证"、"前后对比"和"测试平台"子节
• Risk & Scope — 主要风险、不在范围内的内容、破坏性变更
• Linked Issues — Fixes #N / Closes #N
• 中文说明 — <details> 块中的中文翻译

当前的 "Summary"、"Testing"、"AI Assistance Disclosure" 和模板对不上。请按模板重新格式化一下 PR 描述，方便审查。更新后我会重新跑 triage。🙏

— Qwen Code · qwen3.7-max

[wenshao]

@qwen-code /triage

[qwen-code-ci-bot]

Thanks for the PR!

Template looks good ✓

On direction: This fixes a real data-integrity bug — parseInt("10ms") silently truncating to 10 and persisting the wrong value is the kind of quiet corruption that bites users later. Clearly aligned with the project's settings/config path. Issue #5313 describes the exact problem.

On approach: The scope is tight and focused — one helper function tightened, two regression tests added, no drive-by changes. The move from Number.isFinite to Number.isInteger tightens validation appropriately for timeout values (which should be whole milliseconds). Regex /^\d+$/ for pure-digit string check is the right call — simple, readable, and rejects suffixed/fractional/non-numeric strings cleanly.

Moving on to code review. 🔍

中文说明

感谢贡献！

模板完整 ✓

方向：这是一个真实的数据完整性 bug——parseInt("10ms") 悄悄截断为 10 并持久化错误值，是那种日后会让用户踩坑的隐患。与项目的设置/配置路径完全对齐。Issue #5313 精确描述了这个问题。

方案：范围紧凑聚焦——收紧一个辅助函数，新增两个回归测试，没有夹带无关改动。从 Number.isFinite 改为 Number.isInteger 对超时值（应为整数毫秒）是合理的收紧。正则 /^\d+$/ 用于纯数字字符串校验，简单、可读，干净地拒绝带后缀/小数/非数字的字符串。

进入代码审查 🔍

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

Code Review

The change is a clean, minimal tightening of normalizeOptionalNumber. Two things I checked closely:

• Behavioral change from isFinite → isInteger: previously 1.5 was accepted as a valid timeout. Now it's rejected. This is a correct tightening — timeouts should be integer milliseconds — but it is technically a stricter contract. The PR description calls this out explicitly, so this is intentional.
• Error message change: "Expected a positive number" → "Expected a positive integer". Matches the new validation. This is an ACP internal error string, not a public API.

No correctness bugs, no security issues, no AGENTS.md violations. The code reads naturally and doesn't over-abstract.

Test Results

Full acpAgent.test.ts suite: 124 passed (including the 2 new malformed-timeout regression tests). Lint: clean.

 ✓ src/acp-integration/acpAgent.test.ts (124 tests) 5691ms

 Test Files  1 passed (1)
      Tests  124 passed (124)
   Start at  20:07:39
   Duration  11.84s

Before / After

=== BEFORE (main branch — parseInt behavior) ===

Testing normalizeOptionalNumber with '10ms' (before fix):
  Input: "10ms" -> Result: 10 (WRONG: silently truncated)
  Input: "1.5"  -> Result: 1 (WRONG: silently truncated)

=== AFTER (PR branch — strict regex behavior) ===

Testing normalizeOptionalNumber with '10ms' (after fix):
  Input: "10ms" -> Rejected: Expected a positive integer (CORRECT)
  Input: "1.5"  -> Rejected: Expected a positive integer (CORRECT)
  Input: "1500" -> Result: 1500 (CORRECT: valid digit string)
  Input: 5000    -> Result: 5000 (CORRECT: valid number)

The before/after confirms: malformed strings that were silently truncated are now correctly rejected, while valid inputs (numbers and digit-only strings) continue to work.

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

This is a textbook bug fix: small, focused, well-tested, and the before/after evidence confirms it does exactly what it says.

The normalizeOptionalNumber change is the right approach — reject malformed inputs at the boundary rather than silently persisting wrong values. My independent proposal would have been the same: regex for pure digits, then integer validation. The PR matches that exactly.

All 124 tests pass, lint is clean, and the diff is minimal (2 files, +66/-4). No scope creep, no drive-by refactors. The two regression tests are well-structured and cover both setMcpServer and setHook paths.

Approving. ✅

中文说明

这是一个教科书式的 bug 修复：小而聚焦、测试充分，before/after 证据确认了它完全达到了预期效果。

normalizeOptionalNumber 的改法是对的——在边界拒绝格式错误的输入，而不是悄悄持久化错误值。我的独立方案也是同样的思路：正则匹配纯数字，再做整数校验。PR 的实现完全吻合。

全部 124 个测试通过，lint 干净，diff 最小化（2 个文件，+66/-4）。没有范围蔓延，没有顺手重构。两个回归测试结构良好，覆盖了 setMcpServer 和 setHook 两条路径。

批准。✅

— Qwen Code · qwen3.7-max

[qwen-code-ci-bot]

LGTM, looks ready to ship. ✅

[wenshao]

✅ Maintainer verification — real ACP wire e2e A/B + tests + mutation

Verified on a clean build of the PR head in an isolated worktree. Verdict: correct, strictly safer, no regression — recommend merge. The bug is reproduced over the real ACP protocol against a live qwen --acp process, including the persisted‑settings corruption.

What the change does

normalizeOptionalNumber (ACP settings timeout normalization) now accepts a value only when it is a number or a pure‑digit string (/^\d+$/), then requires Number.isInteger(v) && v > 0; anything else is rejected with invalidParams: Expected a positive integer. Previously it ran parseInt(String(value),10), which silently truncated "10ms"→10 and "1.5"→1 and persisted the wrong value. Covers qwen/settings/setMcpServer and qwen/settings/setHook timeouts.

Root cause (#5313)

parseInt stops at the first non‑digit, so suffixed/fractional strings became truncated integers and were saved into MCP‑server / hook timeout settings instead of being rejected.

Evidence

Check	Result
Build real qwen binary (npm ci + build)	✅ exit 0
Declared test — … -t "malformed timeout strings"	✅ 2 passed
Mutation test (deterministic)	✅ revert acpAgent.ts→base → both new tests fail (the parseInt path accepts '10ms' so the promise no longer rejects)
Real ACP wire e2e A/B	✅ see below

Real ACP e2e — I spawned the actual qwen --acp process and drove it over the real ndjson JSON‑RPC stdio transport with a @agentclientprotocol/sdk ClientSideConnection (real initialize handshake, protocolVersion=1), then sent qwen/settings/setMcpServer / setHook and read back the persisted settings.json in an isolated HOME. Only cli/dist differs between runs (fixed vs rebuilt‑base, byte‑verified buggy before the run).

Request (over ACP)	PRE‑FIX (base)	FIXED (this PR)
setMcpServer timeout:'10ms'	ACCEPT ❌	REJECT Expected a positive integer [-32602] ✅
setMcpServer timeout:'1.5'	ACCEPT ❌	REJECT ✅
setMcpServer timeout:'1500'	ACCEPT ✅	ACCEPT ✅
setHook timeout:'10ms'	ACCEPT ❌	REJECT ✅
persisted settings.json	bad-ms→10, frac→1 ❌ (corrupted)	only good→1500 (number) ✅; malformed not persisted

PRE-FIX persisted mcpServers:                 FIXED persisted mcpServers:
  bad-ms: timeout=10  (from '10ms')  ❌         good: timeout=1500 (number) ✅
  frac:   timeout=1   (from '1.5')   ❌         (bad-ms / frac rejected up front, never written)
  good:   timeout=1500

The pre‑fix process silently wrote timeout=10 (from '10ms') and timeout=1 (from '1.5') — exactly the corruption #5313 describes; the fixed process rejects them at the wire with JSON‑RPC -32602 and persists only the valid 1500.

Correctness notes

• Stricter is correct: besides suffixed strings, the fix also rejects fractional number inputs (isInteger(1.5) is false) and scientific strings like '1e3' (old parseInt('1e3')→1); trim() still allows surrounding whitespace; 0/negative rejected as before. Valid integer / pure‑digit inputs are unchanged (1500 persists as 1500).
• All five normalizeOptionalNumber call sites (stdio/SSE/HTTP MCP timeouts + hook timeout) get the same stricter check — consistent with the PR's intent. Message text updated positive number→positive integer.

Verdict

A small, well‑scoped robustness fix that stops silent persistence of corrupted ACP timeout values, covered by a non‑vacuous regression test and confirmed end‑to‑end over the real ACP protocol. Recommend merge.

🇨🇳 中文版（点击展开）

✅ 维护者验证 —— 真实 ACP 协议端到端 A/B + 测试 + 变异测试

在隔离 worktree 中对 PR head 全新构建后验证。结论：正确、更严格更安全、无回归 —— 建议合并。 该 bug 在真实 ACP 协议下对一个运行中的 qwen --acp 进程被复现，包括对持久化 settings 的污染。

改动做了什么

normalizeOptionalNumber（ACP 设置超时归一化）现在仅在值为数字、或为纯数字字符串（/^\d+$/）时才接受，随后要求 Number.isInteger(v) && v > 0；其他一律以 invalidParams: Expected a positive integer 拒绝。此前它用 parseInt(String(value),10)，会把 "10ms"→10、"1.5"→1 静默截断并持久化错误值。覆盖 qwen/settings/setMcpServer 和 qwen/settings/setHook 的 timeout。

根因（#5313）

parseInt 在第一个非数字字符处停止，所以带后缀/小数的字符串被截断成整数并被写入 MCP server / hook 的 timeout 设置，而不是被拒绝。

证据

检查项	结果
构建真实 qwen 二进制（npm ci + build）	✅ exit 0
声明测试 —— … -t "malformed timeout strings"	✅ 2 通过
变异测试（确定性）	✅ 还原 acpAgent.ts→base → 两个新测试都失败（parseInt 接受 '10ms'，promise 不再 reject）
真实 ACP 协议端到端 A/B	✅ 见下

真实 ACP 端到端 —— 我 spawn 了真实的 qwen --acp 进程，用 @agentclientprotocol/sdk 的 ClientSideConnection 通过真实的 ndjson JSON‑RPC stdio 传输驱动它（真实 initialize 握手，protocolVersion=1），然后发送 qwen/settings/setMcpServer / setHook，并在隔离 HOME 中读回持久化的 settings.json。两次运行只有 cli/dist 不同（修复版 vs 重建的 base 版，运行前已逐字节确认 buggy）。

请求（经 ACP）	PRE‑FIX（base）	FIXED（本 PR）
setMcpServer timeout:'10ms'	ACCEPT ❌	REJECT Expected a positive integer [-32602] ✅
setMcpServer timeout:'1.5'	ACCEPT ❌	REJECT ✅
setMcpServer timeout:'1500'	ACCEPT ✅	ACCEPT ✅
setHook timeout:'10ms'	ACCEPT ❌	REJECT ✅
持久化的 settings.json	bad-ms→10、frac→1 ❌（已污染）	仅 good→1500（number）✅；畸形值未落盘

PRE-FIX 持久化的 mcpServers:                   FIXED 持久化的 mcpServers:
  bad-ms: timeout=10  （来自 '10ms'）  ❌         good: timeout=1500 (number) ✅
  frac:   timeout=1   （来自 '1.5'）   ❌         （bad-ms / frac 在前置校验即被拒，从未写入）
  good:   timeout=1500

pre‑fix 进程静默写入了 timeout=10（来自 '10ms'）和 timeout=1（来自 '1.5'）—— 正是 #5313 描述的污染；fixed 进程在 wire 层以 JSON‑RPC -32602 拒绝它们，只持久化合法的 1500。

正确性说明

• 更严格是对的：除带后缀字符串外，本修复还拒绝小数 number 输入（isInteger(1.5) 为 false）和科学计数字符串如 '1e3'（旧 parseInt('1e3')→1）；trim() 仍允许首尾空白；0/负数照旧拒绝。合法整数 / 纯数字输入不变（1500 持久化为 1500）。
• 全部 5 处 normalizeOptionalNumber 调用点（stdio/SSE/HTTP MCP 超时 + hook 超时）都获得同样更严的校验 —— 符合 PR 意图。错误信息由 positive number 改为 positive integer。

结论

一个小而界定清晰的健壮性修复，阻止了被污染的 ACP 超时值被静默持久化，有非空过场的回归测试覆盖，并在真实 ACP 协议下端到端确认。建议合并。

_{Verification method: worktree build + declared unit test + source‑revert mutation test + real ACP wire e2e (spawned qwen --acp, @agentclientprotocol/sdk ClientSideConnection over ndjson JSON‑RPC stdio, isolated HOME, persisted‑settings read‑back; fixed vs rebuilt‑base cli/dist).}