Skip to content

fix(desktop): reject fractional transfer sizes - #5527

Merged
wenshao merged 1 commit into
QwenLM:mainfrom
tt-a1i:fix/transfer-integer-counts
Jun 21, 2026
Merged

fix(desktop): reject fractional transfer sizes#5527
wenshao merged 1 commit into
QwenLM:mainfrom
tt-a1i:fix/transfer-integer-counts

Conversation

@tt-a1i

@tt-a1i tt-a1i commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Hardens the transfer:start RPC handler in the desktop server-core so that it rejects fractional chunkCount and totalBytes values. The metadata validation now requires both fields to be integers (via Number.isInteger) instead of merely being of type number, so invalid transfer metadata is rejected before any transfer state is allocated. Adds regression tests covering fractional chunk counts and fractional byte counts.

Why it's needed

The previous validation only checked typeof opts.chunkCount === 'number' / typeof opts.totalBytes === 'number', which accepts fractional values such as 1.5 or 10.5. Non-integer chunk counts and byte counts are not valid transfer metadata and could lead to allocating transfer state from malformed input. Requiring integers rejects these bad values up front.

Reviewer Test Plan

How to verify

  • bun test packages/desktop/packages/server-core/src/handlers/rpc/transfer.test.ts
  • cd packages/desktop/packages/server-core && bun run typecheck
  • npx prettier --check packages/desktop/packages/server-core/src/handlers/rpc/transfer.ts packages/desktop/packages/server-core/src/handlers/rpc/transfer.test.ts
  • git diff --check

Also ran bun test packages/desktop/packages/server-core/src; transfer coverage passed, but the full package currently has an unrelated existing failure in system.open-url.test.ts where the expected unsupported-protocol error text does not match the current blocked-scheme message.

Evidence (Before & After)

N/A — internal logic change covered by unit tests. Before: fractional chunkCount / totalBytes passed validation. After: they are rejected with Invalid chunkCount / Invalid totalBytes, verified by the new regression tests.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested locally; covered by CI
🐧 Linux ⚠️ not tested locally; covered by CI

Environment (optional)

Local macOS workspace; unit tests via bun.

Risk & Scope

  • Main risk or tradeoff: low; scope-limited to input validation in the transfer:start handler.
  • Not validated / out of scope: manual end-to-end transfer flow.
  • Breaking changes / migration notes: none.

Linked Issues

Fixes #5526

AI Assistance Disclosure

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

中文说明

本 PR 做了什么

加固 desktop server-core 中的 transfer:start RPC 处理器,使其拒绝分数(非整数)的 chunkCounttotalBytes 值。元数据校验现在要求这两个字段都是整数(通过 Number.isInteger),而不仅仅是 number 类型,因此无效的传输元数据会在分配任何传输状态之前就被拒绝。新增了针对分数 chunk 数和分数字节数的回归测试。

为什么需要

此前的校验只检查 typeof opts.chunkCount === 'number' / typeof opts.totalBytes === 'number',这会接受诸如 1.510.5 之类的分数值。非整数的 chunk 数和字节数不是合法的传输元数据,可能导致基于畸形输入分配传输状态。要求整数可以在前期就拒绝这些非法值。

审阅者测试计划

如何验证

  • bun test packages/desktop/packages/server-core/src/handlers/rpc/transfer.test.ts
  • cd packages/desktop/packages/server-core && bun run typecheck
  • npx prettier --check packages/desktop/packages/server-core/src/handlers/rpc/transfer.ts packages/desktop/packages/server-core/src/handlers/rpc/transfer.test.ts
  • git diff --check

同时运行了 bun test packages/desktop/packages/server-core/src;传输相关的测试通过,但整个包目前存在一个与本次改动无关的既有失败:system.open-url.test.ts 中期望的「不支持协议」错误文本与当前的「被屏蔽 scheme」消息不匹配。

证据(前后对比)

N/A —— 内部逻辑改动,已由单元测试覆盖。改动前:分数的 chunkCount / totalBytes 能通过校验。改动后:它们会被拒绝并抛出 Invalid chunkCount / Invalid totalBytes,由新增的回归测试验证。

测试平台

操作系统 状态
🍏 macOS ✅ 已测试
🪟 Windows ⚠️ 本地未测试;由 CI 覆盖
🐧 Linux ⚠️ 本地未测试;由 CI 覆盖

环境(可选)

本地 macOS 工作区;通过 bun 运行单元测试。

风险与范围

  • 主要风险或取舍:低;范围限定于 transfer:start 处理器的输入校验。
  • 未验证 / 范围之外:手动端到端传输流程。
  • 破坏性变更 / 迁移说明:无。

关联 Issue

Fixes #5526

AI 协助声明

我使用 Codex 来审查改动、对照既有模式核对实现,并帮助发现潜在的边界情况。

@tt-a1i
tt-a1i marked this pull request as ready for review June 20, 2026 20:03
@wenshao

wenshao commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

📌 Same maintainer note — full version on #5509.

This is the strict-numeric-parsing / reject-fractional theme again — same class as the merged #5500, #5491, #5496, now for desktop transfer sizes. Please consolidate the strict-parse work into a single PR and reuse a shared "strict positive integer" helper instead of another per-file copy. The change itself looks fine — this is purely grouping + DRY, not correctness.

中文说明

📌 同一条维护者说明 —— 完整版见 #5509

这又是严格数字解析 / 拒绝小数的主题 —— 和已合的 #5500#5491#5496 同类,现在用在 desktop transfer sizes 上。请把严格解析相关改动合并成一个 PR,并复用一个共享的"严格正整数"helper,而不是再写一份逐文件副本。改动本身没问题 —— 这条纯粹是归并 + DRY,不涉及正确性。

@tt-a1i

tt-a1i commented Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

macOS CI failed in src/services/cronScheduler.test.ts, not in the transfer-size path touched here. Looks like the lock-probe test can tick before takeover fully settles; I opened #5535 for that test race.

@wenshao

wenshao commented Jun 20, 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.

Thanks for the PR @tt-a1i! Before we dive into the code, this needs to follow the PR template so reviewers can evaluate it efficiently.

The PR body is missing several required sections:

  • ## What this PR does — currently "Summary", which is close but not the standard heading
  • ## Why it's needed — the motivation for the fix (the bug in #5526 is clear, but this section is required)
  • ## Reviewer Test Plan — the current "Test plan" lists commands but lacks the required sub-structure: "How to verify", "Evidence (Before & After)", and the "Tested on" OS table
  • ## Risk & Scope — risk, out-of-scope items, breaking changes
  • ## Linked IssuesFixes #5526 should go in this section
  • Chinese translation — the <details> block with a full translation is missing

Please restructure the PR body to match the template headings. The content you already have is good — it just needs to be placed under the right sections.

中文说明

感谢 @tt-a1i 的贡献!在审查代码之前,PR 描述需要按照 PR 模板 的格式来组织,方便 reviewer 高效评估。

当前 PR 描述缺少几个必需的部分:

  • ## What this PR does — 目前用的是 "Summary",接近但不是标准标题
  • ## Why it's needed — 修复的动机(#5526 的 bug 很清晰,但这个部分是必需的)
  • ## Reviewer Test Plan — 当前的 "Test plan" 列出了命令,但缺少必需的子结构:"How to verify"、"Evidence (Before & After)" 以及 "Tested on" 操作系统表格
  • ## Risk & Scope — 风险、不在范围内的内容、破坏性变更
  • ## Linked IssuesFixes #5526 应放在这个部分
  • 中文翻译 — 缺少包含完整翻译的 <details> 区块

请按照模板标题重新组织 PR 描述。现有内容质量不错,只需放到正确的章节下即可。

Qwen Code · qwen3.7-max

@wenshao

wenshao commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

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

On direction: this fixes a real correctness bug. Fractional chunkCount causes transfers to hang permanently because Set.size (always integer) can never equal a fractional chunkCount, so commit never succeeds. Linked issue #5526 describes it well. Solid hardening fix, well within scope.

On approach: minimal and focused — 2 lines of validation changed, 2 regression tests added, nothing extraneous. The change is exactly what the problem calls for.

I see @wenshao's existing CHANGES_REQUESTED review asking to consolidate this with the other strict-integer PRs (#5500, #5491, #5496) behind a shared helper. That's a valid DRY concern — the codebase currently has 30+ inline Number.isInteger calls with no shared utility. However, this PR correctly follows the existing inline pattern; creating a shared helper is a separate refactoring effort that would touch many files. The correctness fix shouldn't be blocked on that refactor.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ — 所有章节齐全,双语,测试计划清晰。

方向:修复了一个真实的正确性 bug。分数的 chunkCount 会导致传输永久挂起(因为 Set.size 永远是整数,无法等于分数的 chunkCountcommit 永远不会成功)。关联 issue #5526 描述得很清楚。属于合理的加固修复。

方案:极简且聚焦 — 改动 2 行校验逻辑,新增 2 个回归测试,没有多余内容。

注意到 @wenshao 已有的 CHANGES_REQUESTED 评审,要求将此 PR 与其他严格整数校验 PR(#5500#5491#5496)合并并提取共享 helper。这个 DRY 建议合理 — 代码库目前有 30+ 处内联 Number.isInteger 调用,没有共享工具函数。但本 PR 正确遵循了现有的内联模式;创建共享 helper 是一个独立的、涉及多文件的重构工作。正确性修复不应被该重构阻塞。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

2a. Code Review

Independent proposal: Given "fractional chunk counts cause transfer hangs" — swap typeof === 'number' for Number.isInteger() on both chunkCount and totalBytes, add regression tests for fractional inputs. That's it.

Comparison with diff: The PR does exactly this. Two-line validation swap, two regression tests. No surprises, no scope creep.

Findings:

  • Correct swap. Number.isInteger() rejects fractions, NaN, and Infinity — all invalid for transfer metadata. The old typeof === 'number' accepted all of these.
  • totalBytes tightened appropriately. Old code accepted Infinity (it's typeof 'number' and >= 0). New code rejects it. Good — infinite byte count is meaningless.
  • !opts guard preserved. Still on the chunkCount line; totalBytes line doesn't need it since control never reaches there if opts is null.
  • Tests cover the new behavior. Fractional 1.5 for both chunkCount and totalBytes are explicitly tested. Existing tests still cover negative, cross-client, and abort scenarios.
  • ℹ️ Minor observation (not blocking): No explicit tests for NaN or Infinity inputs. Number.isInteger handles them correctly, but explicit edge-case tests would make the intent clearer. Low priority.

No correctness bugs, no security concerns, no AGENTS.md violations. The code follows the existing inline-validation pattern used in 30+ other locations in the codebase.

2b. Test Results

CI (GitHub Actions):

Job Result
Lint ✅ pass
Test (ubuntu-latest, Node 22.x) ✅ pass (17m50s)
Test (windows-latest, Node 22.x) ✅ pass (27m9s)
Test (macos-latest, Node 22.x) ❌ fail — cronScheduler.test.ts race condition, unrelated to this PR (author filed #5535)
CodeQL ✅ pass

Tmux real-scenario testing: N/A. This is an internal RPC handler input-validation change in packages/desktop/packages/server-core. It does not alter any CLI-facing or user-visible behavior — there is no prompt or command to drive in tmux. The behavioral change (rejecting fractional values at the RPC boundary) is fully exercised by the unit tests, which CI confirms pass on Linux and Windows.

中文说明

2a. 代码审查

独立方案: 针对"分数 chunk 计数导致传输挂起"—— 将 typeof === 'number' 替换为 Number.isInteger(),并添加回归测试。

与 diff 的对比: PR 完全按此方案实现。两行校验替换,两个回归测试。无意外,无范围蔓延。

审查发现:

  • 替换正确。 Number.isInteger() 拒绝分数、NaNInfinity —— 这些对传输元数据都是无效的。
  • totalBytes 适度收紧。 旧代码接受 Infinity(它是 typeof 'number'>= 0)。新代码拒绝了。合理。
  • !opts 守卫保留。 仍在 chunkCount 行;totalBytes 行不需要,因为如果 opts 为 null,控制流不会到达此处。
  • 测试覆盖新行为。chunkCounttotalBytes 的分数 1.5 均有显式测试。
  • ℹ️ 小观察(不阻塞): 缺少 NaNInfinity 输入的显式测试。Number.isInteger 能正确处理,但显式边界测试会更清晰。低优先级。

无正确性 bug,无安全隐患,无 AGENTS.md 违规。

2b. 测试结果

CI (GitHub Actions): Lint ✅ / Linux 测试 ✅ / Windows 测试 ✅ / macOS ❌(cronScheduler.test.ts 竞态条件,与本 PR 无关,作者已提交 #5535)/ CodeQL ✅

Tmux 真实场景测试:N/A。 这是 packages/desktop/packages/server-core 中的内部 RPC 处理器输入校验改动,不影响任何 CLI 或用户可见行为。行为变更(在 RPC 边界拒绝分数值)完全由单元测试覆盖,CI 已确认在 Linux 和 Windows 上通过。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Stepping back to look at the whole picture:

This is a clean, correct fix for a real bug. Fractional chunkCount causes transfers to hang forever because a Set.size can never equal a non-integer — the transfer just sits there until TTL cleanup. The fix is two lines: swap typeof === 'number' for Number.isInteger(). That's the obvious solution, and it's what the PR does.

The tests pass on Linux and Windows (macOS failure is an unrelated cronScheduler race, filed as #5535). The diff is focused — no drive-by refactors, no scope creep. The code follows the same inline validation pattern used in 30+ other places in the codebase.

The existing CHANGES_REQUESTED from @wenshao asks for consolidation with the other strict-integer PRs (#5500, #5491, #5496) behind a shared helper. That's a reasonable DRY goal, and I'd support it as a follow-up — but it's a cross-PR coordination concern, not a problem with this PR. Asking a contributor to hold a correct, minimal bug fix while a shared utility gets designed across four PRs feels like the wrong tradeoff. The fix ships value now; the helper can come later.

Approving. ✅

中文说明

回顾整体:

这是一个干净、正确的真实 bug 修复。分数的 chunkCount 导致传输永久挂起(Set.size 永远无法等于非整数),直到 TTL 清理。修复只需两行:把 typeof === 'number' 换成 Number.isInteger()。这是显而易见的方案,PR 也正是这样做的。

测试在 Linux 和 Windows 上通过(macOS 失败是不相关的 cronScheduler 竞态,已提交 #5535)。Diff 聚焦 — 没有顺手重构,没有范围蔓延。代码遵循了代码库中 30+ 处使用的相同内联校验模式。

@wenshao 已有的 CHANGES_REQUESTED 要求将此 PR 与其他严格整数校验 PR(#5500#5491#5496)合并并提取共享 helper。这个 DRY 目标合理,我支持作为后续工作 — 但这是跨 PR 的协调问题,不是本 PR 本身的问题。要求贡献者在一个正确的、极简的 bug 修复上等待四个 PR 的共享工具函数设计完成,是不合理的取舍。修复现在就能交付价值;helper 可以后续再做。

批准。✅

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 — correct minimal fix for the fractional chunk count hang. CI green on Linux/Windows; macOS failure is unrelated (#5535). ✅

@qwen-code-ci-bot qwen-code-ci-bot added category/core Core engine and logic type/bug Something isn't working as expected labels Jun 21, 2026
@wenshao

wenshao commented Jun 21, 2026

Copy link
Copy Markdown
Collaborator

✅ Local verification report — safe to merge

I reproduced this PR's full test plan locally on a clean isolated worktree checked out at the PR head (345d0ff25), Linux, bun 1.3.14 / Node 22, dependencies installed with bun install --frozen-lockfile (1554 packages, clean). All commands were run in a tmux session.

Verdict: the change does exactly what it claims, the regression tests genuinely guard it, and nothing else regresses. Every check documented in the PR reproduces.

Results

Check Command (as documented) Result
Targeted tests bun test …/rpc/transfer.test.ts 7/7 pass (incl. the 2 new regression tests)
Typecheck cd …/server-core && bun run typecheck ✅ pass (tsc --noEmit)
Format npx prettier --check … (from repo root) All matched files use Prettier code style!
Whitespace git diff --check (PR commit) ✅ clean
Broader suite bun test …/rpc/ 21/22 — the 1 failure is pre-existing & unrelated (see below)

The regression tests actually catch the bug

To confirm the new tests aren't vacuous, I reverted only transfer.ts back to the old typeof === 'number' validation (keeping the PR's new test file) and re-ran:

[Transfer:server] Started transfer …: 1.5 chunks, 0.0MB   ← old code ACCEPTED 1.5 chunks
(fail) chunked transfer handlers > rejects fractional chunk counts
       Expected promise that rejects / Received promise that resolved
(fail) chunked transfer handlers > rejects fractional total byte counts
 5 pass / 2 fail

With the fix restored, all 7 pass. So the change cleanly flips fractional chunkCount / totalBytes from accepted → rejected, and the tests fail without it. The Number.isInteger guard also correctly handles NaN / Infinity / undefined (all rejected) while still accepting valid integers (totalBytes: 0 and chunkCount: 1 remain valid).

One pre-existing failure in the broader suite — not caused by this PR

bun test …/rpc/ reports system.open-url.test.ts > rejects unsupported protocols failing:

Expected substring: "…Only http, https, mailto, craftdocs, craftagents URLs are allowed"
Received message:   "…Refused to open URL with blocked scheme: file:"

This is a stale expected-message string in a file this PR does not touch (the PR changes exactly 2 files: transfer.ts + transfer.test.ts). It is pre-existing and unrelated, and the author already disclosed it in the test plan. ✅ Good call flagging it.

Note on the prettier command

packages/desktop/ is listed in the repo-root .prettierignore, so the desktop subproject is intentionally excluded from the root Prettier config. The documented command (run from the repo root) therefore passes. Heads-up for future reviewers: running npx prettier from inside packages/desktop/ bypasses that ignore and picks up the root .prettierrc.json (semi: true), which then flags the whole — no-semicolon — desktop tree. Run it from the repo root as documented.

Recommendation: approve / merge. 👍

🇨🇳 中文版(点击展开)

✅ 本地验证报告 —— 可以合并

我在一个干净的独立 worktree(checkout 到 PR HEAD 345d0ff25)上,于 Linux、bun 1.3.14 / Node 22 环境中完整复现了本 PR 的测试计划;依赖通过 bun install --frozen-lockfile 安装(1554 个包,干净)。所有命令均在 tmux 会话中执行。

结论:改动确实实现了其声明的功能,回归测试能真正守护该行为,且没有引入任何其它回归。 PR 中记录的每一项检查都能复现。

结果

检查项 命令(按文档) 结果
目标测试 bun test …/rpc/transfer.test.ts 7/7 通过(含 2 个新增回归测试)
类型检查 cd …/server-core && bun run typecheck ✅ 通过(tsc --noEmit
格式化 npx prettier --check …(在仓库根目录) All matched files use Prettier code style!
空白字符 git diff --check(PR 提交) ✅ 干净
更大范围测试 bun test …/rpc/ 21/22 —— 唯一失败为既有且无关问题(见下)

回归测试确实能捕获该 bug

为确认新增测试并非「永真」摆设,我transfer.ts 回退到旧的 typeof === 'number' 校验(保留 PR 的新测试文件)后重跑:

[Transfer:server] Started transfer …: 1.5 chunks, 0.0MB   ← 旧代码接受了 1.5 个 chunk
(fail) rejects fractional chunk counts
       期望 Promise 拒绝 / 实际 Promise 已 resolve
(fail) rejects fractional total byte counts
 5 通过 / 2 失败

恢复修复后,7 个测试全部通过。因此该改动把分数 chunkCount / totalBytes接受 → 拒绝,且缺少修复时测试会失败。Number.isInteger 还能正确处理 NaN / Infinity / undefined(均拒绝),同时仍接受合法整数(totalBytes: 0chunkCount: 1 仍合法)。

更大范围测试中的一个既有失败 —— 与本 PR 无关

bun test …/rpc/system.open-url.test.ts > rejects unsupported protocols 失败:

期望子串:「…Only http, https, mailto, craftdocs, craftagents URLs are allowed」
实际消息:「…Refused to open URL with blocked scheme: file:」

这是一个位于本 PR 未触碰文件中的过期断言文本(本 PR 恰好只改了 2 个文件:transfer.ts + transfer.test.ts)。属于既有且无关问题,作者已在测试计划中如实声明。✅ 标注得很到位。

关于 prettier 命令的说明

仓库根目录的 .prettierignore 中包含 packages/desktop/,因此 desktop 子项目被有意排除在根 Prettier 配置之外。按文档在仓库根目录运行该命令即可通过。提醒后续审阅者:若在 packages/desktop/ 内部运行 npx prettier,会绕过该 ignore 并使用根 .prettierrc.jsonsemi: true),从而把整个(无分号风格的)desktop 目录都标红。请按文档在仓库根目录运行。

建议:批准 / 合并。 👍

@wenshao
wenshao merged commit 0880e34 into QwenLM:main Jun 21, 2026
52 of 54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category/core Core engine and logic type/bug Something isn't working as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chunked transfer accepts fractional chunk counts

3 participants