Skip to content

feat: add lrzsz ZMODEM transfers#309

Open
alkaid wants to merge 2 commits into
zerx-lab:mainfrom
alkaid:feat/rzsz
Open

feat: add lrzsz ZMODEM transfers#309
alkaid wants to merge 2 commits into
zerx-lab:mainfrom
alkaid:feat/rzsz

Conversation

@alkaid

@alkaid alkaid commented Jul 9, 2026

Copy link
Copy Markdown

Description

Adds lrzsz/ZMODEM file transfer support for terminal sessions.

  • Detects rz/sz ZMODEM handshakes and drives upload/download flows from the terminal.
  • Supports selecting files for rz, dragging files onto SSH terminals to start uploads, and choosing a local save directory for sz downloads.
  • Adds progress, transfer speed, cancellation handling, and temporary-file cleanup/commit behavior for incomplete transfers.
  • Adds Windows SSH direct side-channel handling so rz/sz does not depend on SFTP, including SSH Manager auth context reuse where available.
  • Enables the lrzsz feature for release bundle builds.

Testing

Added/updated unit and regression coverage around ZMODEM parsing, PTY event-loop transfer handling, SSH drag/drop upload routing, cancellation cleanup, duplicate upload names, and raw transfer bounds.

  • git diff --check upstream/main..HEAD -> pass
  • cargo check ->pass

Server API dependencies

N/A - this is client-side terminal transfer support and does not require a server API change.

  • Is this change necessary to make the client compatible with a desired server API breaking change?
  • Does this change rely on a new server API?
    • If so, is the use of this API restricted to client channels that rely on the staging server (e.g. WarpDev)?
  • Is this change enabling the use of a server API on client channels that rely on the production server (e.g. WarpStable)?
    • If so, has the new server API been stable on production for at least one server release cycle? See here for more details.

Agent Mode

  • Zap Agent Mode - This PR was created via Zap's AI Agent Mode

Changelog Entries for Stable

CHANGELOG-NEW-FEATURE: Add lrzsz/ZMODEM file transfers for terminal sessions, including rz uploads and sz downloads.

Review follow-up

  • Multi-file downloads use batch-atomic commit semantics: FileCompleted is emitted only after the final file path has been persisted. Cancellation or failure removes current and completed-but-uncommitted temporary files, so the UI no longer reports files that are absent on disk.
  • Terminal file drag/drop is intentionally handled before child dispatch. Child-first dispatch writes the dropped path into the active SSH shell, bypassing ZMODEM upload routing. Other event types continue to fall through to the child.
  • The zmodem2 single-file upload limit is now reported explicitly with the maximum supported byte count, source path, and actual file size.

Manual validation covered the Windows SSH drag/drop and rz upload path, including current remote working-directory selection, cancellation, and hidden helper-command echo. macOS and Linux drag/drop were not manually tested in this review round; they are not claimed as manually verified.

Review verification:

  • cargo test -p warp --features lrzsz zmodem -> 92 passed
  • cargo check -p warp --features lrzsz -> pass
  • git diff --check -> pass

@zerx-lab zerx-lab left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

整体判断倾向:Request Changes。ZMODEM 核心逻辑实现扎实,但发现一个会导致用户数据静默丢失的 bug,需要在合并前解决。Diff 超过 23000 行(大部分为 rustfmt 格式化),本次未逐行覆盖全部内容,专注在新增功能文件上抽样 review。


阻塞问题

1. DownloadSessionsz 中止时已完成的文件被静默删除(见 inline comment,zmodem.rs ~748 行)

详见 inline comment。核心问题:Event::Aborted 只清理了 current_file,但 completed_files 里已暂存(尚未 rename 到最终路径)的 NamedTempFileDownloadSession 析构时会被一并删除。用户会看到 UI 显示"文件 A/B 已接收完成",但磁盘上找不到这些文件。


建议

2. terminal_size_element.rsdispatch_event 顺序调换(见 inline comment,~97 行)

此处将拖放事件处理移到 child.dispatch_event 之前,并去掉了旧的 !handled_by_child 保护。逻辑上合理(让终端拦截文件拖放而不是让子元素消费),但属于行为变更,建议在 PR 描述里补充说明意图,并确认各平台的拖放行为均已测试。

3. 上传文件大小硬限 ~4 GB

UploadSession::start_current_fileu32::try_from(file.size) 意味着单文件超过约 4 GB 时直接报错。这是 zmodem2 crate 的 Position::new(u32) 限制,本身无误。建议在用户可见的错误信息或文档里明确说明此限制,避免用户困惑(报错信息当前是内部描述,用户体验不佳)。


看起来不错的地方

  • sanitize_wire_file_name 正确拦截了路径遍历(split on /\\,拒绝 . / ..);raw_upload_file_specs 在客户端去重了同名文件,防止远端覆盖。
  • validate_raw_download_limits 对单文件大小(32 GB)和总量(128 GB)均有上界检查,且用 checked_add 防止溢出。
  • remote_raw_upload_command 中 shell 脚本通过 shell_words::quote 对文件名和路径做了正确转义,无注入风险;远端脚本里的 case "$name" in ""|.|..|*/*) exit 2;; 提供了第二道防御。
  • 超时机制(SSH_ZMODEM_INITIAL_OUTPUT_TIMEOUT = 15s,总超时 1h)覆盖了远端卡死的场景,取消信号通过原子 bool + try_kill 传播的方式也处理得比较完整。
  • ZmodemDetector 的"末尾前缀保留"逻辑(longest_zmodem_prefix_suffix_len)处理了 ZMODEM 握手跨 PTY 读取边界的情况,细节到位。

— 由 Claude Routine 自动生成;如需复评请 @ 维护者人工触发。


Generated by Claude Code

return Ok(true);
}
Event::Aborted => {
self.current_file.take();

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug:多文件 sz 下载中止时,已完成的文件被静默丢弃

sz 发送多个文件时,Event::FileCompleted 会把完成的 NamedTempFilecurrent_file 移入 completed_files,但不会立即 rename 到目标路径(rename 只在 Event::SessionCompleted 触发的 persist_completed_files() 里发生)。

这里的 Event::Aborted 分支只调用 self.current_file.take()(正确地清理了正在传输的文件),但没有处理 self.completed_files

  • DownloadSession 随即被 event loop 丢弃。
  • completed_files 里所有 NamedTempFile 的析构函数运行,临时文件被删除。
  • 用户在 UI 里看到"文件 A 已完成、文件 B 已完成、文件 C 失败",但 A 和 B 实际上不在磁盘上。

建议:在 Aborted 分支里决策:要么调用 persist_completed_files() 保留已完成的文件(更符合用户预期),要么在 UI/文档里明确说明"传输中止时所有文件均被丢弃"(当前行为则应去掉 FileCompleted 事件,否则误导用户)。若选择保留已完成文件,需处理 persist_completed_files 可能返回的错误。


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zerx-lab 感谢反馈。这里采用整批原子提交语义:标准 ZMODEM 下载和 Windows SSH raw download 都不再在临时文件接收完成时发出 ZmodemEvent::FileCompleted,而是在 SessionCompleted 后、persist_completed_files 成功写入最终路径时逐个发出。中止或失败时,当前文件和已完成但未提交的 NamedTempFile 都会被清理,UI 不再提前显示实际不存在的完成文件。

回归测试验证了 FileCompleted 触发时最终路径已经存在,以及取消/Drop 后最终文件与临时文件均不存在。修复在 ee283750;ZMODEM 测试 92 项通过,cargo check -p warp --features lrzsz 通过。

};
}
return true;
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question:dispatch_event 顺序调换是否有意为之?

旧逻辑:先调用 self.child.dispatch_event,若 child 未消费则处理拖放事件(DragFiles/DragFileExit/DragAndDropFiles)。

新逻辑:拖放事件优先处理(命中则 return true),之后才调用 self.child.dispatch_event

对于 DragAndDropFiles(触发 TerminalAction::DragAndDropFiles → ZMODEM 上传),child 现在不再收到这个事件。如果这是为了让终端本体不干扰文件拖放,逻辑上合理;但如果有子元素在旧代码中确实需要消费拖放事件(例如文件预览组件),此变更可能造成回归。

能否在 PR 描述里补充说明这个顺序调换的原因,以及确认各平台(macOS/Linux/Windows)下拖放行为均已人工测试过?


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zerx-lab 这个顺序调换是有意的。child-first 时,终端子元素的默认拖放处理会把本地路径写入当前 SSH shell,导致文件拖放退化为路径输入,无法进入 ZMODEM 上传路由;因此终端边界内的文件拖放需要先由 TerminalSizeElement 拦截,其他事件仍继续交给 child。代码和 PR 描述都补充了这一点。

人工验证范围是 Windows SSH 拖放/rz 上传,包括当前远端 cwd、取消和辅助命令回显隐藏。macOS/Linux 本轮未做人工测试,因此没有声明全平台手测;公共路由由现有单测和 cargo check 覆盖。

@alkaid

alkaid commented Jul 17, 2026

Copy link
Copy Markdown
Author

@zerx-lab review 反馈已在 ee283750 处理并推送:多文件下载完成事件改为落盘后发出,拖放优先级的设计原因与人工验证范围已补充到 PR 描述;zmodem2 的单文件上传上限错误也改为明确显示最大字节数、源文件路径和实际大小。

验证结果:cargo test -p warp --features lrzsz zmodem 为 92 passed,cargo check -p warp --features lrzszgit diff --check 通过。可以复评当前 head。

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