fix(cli): handle truncated remote input files - #5473
Conversation
| private hasConsumedPrefixChanged(): boolean { | ||
| if (this.bytesRead === 0 || this.consumedPrefixHash === null) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[Critical] hasConsumedPrefixChanged() silently treats a hash failure as "prefix unchanged" — return currentHash !== null && ... returns false when currentHash is null. If the file is truncated-and-rewritten but a transient I/O error (file lock, NFS hiccup, permission change) causes hashFilePrefix() to return null, the truncation is not detected. checkForNewInput() then returns early at currentSize <= this.bytesRead, permanently skipping the new content.
| } | |
| const currentHash = this.hashFilePrefix(this.bytesRead); | |
| if (currentHash === null) { | |
| debugLogger.warn( | |
| 'RemoteInput: failed to hash consumed prefix, falling back to full re-read', | |
| ); | |
| return true; | |
| } | |
| return currentHash !== this.consumedPrefixHash; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
fixed, thanks. hash failures now fail open by resetting the read offset instead of pretending the prefix is unchanged.
| return new Promise<void>((resolve) => { | ||
| rl.on('close', () => { | ||
| this.bytesRead = currentSize; | ||
| this.consumedPrefixHash = nextConsumedPrefixHash; |
There was a problem hiding this comment.
[Critical] nextConsumedPrefixHash is computed by hashFilePrefix(currentSize) at line 161 which can return null on any I/O error. The close handler unconditionally assigns this.consumedPrefixHash = nextConsumedPrefixHash, permanently setting it to null. From that point forward, hasConsumedPrefixChanged() skips all hash checks (this.consumedPrefixHash === null guard returns false), permanently disabling hash-based truncation detection for the session lifetime — even if subsequent polls would have worked fine.
| this.consumedPrefixHash = nextConsumedPrefixHash; | |
| rl.on('close', () => { | |
| this.bytesRead = currentSize; | |
| if (nextConsumedPrefixHash !== null) { | |
| this.consumedPrefixHash = nextConsumedPrefixHash; | |
| } | |
| this.reading = false; | |
| this.processQueue(); | |
| resolve(); | |
| }); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
fixed this too. the close handler only updates the cached prefix hash/mtime when the next hash was computed successfully, so a transient hash failure no longer disables future detection.
| expect(submitted).toEqual(['after-bad-line']); | ||
| }); | ||
|
|
||
| it('reads commands written after the input file is truncated', async () => { |
There was a problem hiding this comment.
[Suggestion] The new test covers truncate-and-rewrite with a larger file, but not with a file of equal size. The equal-size case is where the old size-only check was most blind (it would skip the new content entirely), and where the hash-based fix is most critical. Adding a same-size test would catch a future regression where the hash logic is removed but the existing larger-size test still passes.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
added a same-size truncate/rewrite regression test. it uses two commands with equal JSONL length and verifies the rewritten command is still read.
| private hasConsumedPrefixChanged(): boolean { | ||
| if (this.bytesRead === 0 || this.consumedPrefixHash === null) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[Suggestion] hasConsumedPrefixChanged() reads and SHA256-hashes all this.bytesRead bytes of the file on every poll cycle (default 500ms), even when the file has not changed. As the input log accumulates, the per-poll cost grows without bound. For long-running sessions this wastes CPU and I/O. Consider caching the file's mtimeMs from statSync and only re-hashing when the mtime has changed since the last poll.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
yeah good point. added an mtime guard so unchanged files skip the prefix re-hash; changed files still verify the consumed prefix before deciding whether to reset.
There was a problem hiding this comment.
update: I removed the mtime fast-path after rechecking the same-size rewrite case. correctness is better here: we always verify the consumed prefix hash, including when size and mtime both look unchanged.
| hash.update(buffer.subarray(0, bytesRead)); | ||
| remaining -= bytesRead; | ||
| position += bytesRead; | ||
| } |
There was a problem hiding this comment.
[Suggestion] hashFilePrefix()'s catch block silently returns null on any I/O error without logging the failure reason. If hash-based detection degrades at runtime, there is zero observability into why — file-locking errors, permission changes, and transient faults all produce identical silent null returns. Adding a debugLogger.warn in the catch block would make this debuggable.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
added warning logs for hash failures as well, so this is visible instead of silently degrading.
82da42f to
d9a88b6
Compare
d9a88b6 to
dd4ce7f
Compare
|
updated the PR body to match the template too. |
qqqys
left a comment
There was a problem hiding this comment.
Critical issues from the previous review are resolved in the current head. I rechecked the remote-input truncation path and did not find any new critical issues.
|
@qwen-code /triage |
|
Thanks for the PR, @tt-a1i! Template looks good ✓ — all required sections present, bilingual, linked issue. On direction: this fixes a genuine bug in On approach: the scope is tight — two files, one concern. The core idea (hash the consumed prefix to detect rewrites instead of relying only on size) is the right minimal fix. The Moving on to code review. 🔍 中文说明感谢 @tt-a1i 的贡献! 模板完整 ✓ — 所有必填段落齐全,双语,关联了 issue。 方向:修复了 方案:范围紧凑——两个文件,一个问题。核心思路(对已消费前缀做 hash 来检测重写,而不是只看文件大小)是正确的最小修复。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe approach is sound. I wrote down my own proposal before reading the diff: track mtime + size to detect rewrites, reset offset on mismatch. The PR's hash-based approach is strictly better — mtime has second-level granularity and is easily fooled by fast rewrites, while a SHA-256 prefix hash is definitive. The The No correctness bugs, no security issues, no regressions spotted. Code follows project conventions — no over-abstraction, no unnecessary duplication, changes scoped to exactly what the fix needs. Test ResultsBefore fix (main branch + PR's test file)After fix (PR branch)Static checks
Real-Scenario TestingThis PR fixes internal file-watching behavior for the 中文说明代码审查方案合理。我在看 diff 之前写了自己的方案:跟踪 mtime + size 来检测重写,不一致时重置 offset。PR 的 hash 方案严格更优——mtime 精度只有秒级,快速重写容易漏检,而 SHA-256 前缀 hash 是确定性的。
无正确性 bug、无安全问题、无回归。代码遵循项目规范——没有过度抽象、没有不必要的重复、改动范围精确匹配修复需求。 测试结果修复前(main 分支 + PR 的测试文件):2/7 测试失败(两个截断回归测试),5/7 通过。 静态检查:Prettier ✅、ESLint ✅、TypeScript ✅、 真实场景测试此 PR 修复 — Qwen Code · qwen3.7-max |
ReflectionStepping back: this is a clean, focused bug fix that does exactly what it says. The problem is real — file truncation/rotation is a natural operation for external writers feeding What I'd watch for: the Every change in the diff is necessary for the fix. No drive-by refactors, no over-engineering. The tests are well-crafted — the same-size regression test is particularly thoughtful with its mtime pinning to ensure hash-based detection is what's actually being tested, not a size-based fallback. Verdict: approve. Clean fix, correct tests, ships the minimal change the bug needs. 中文说明总结这是一个干净、聚焦的 bug 修复,完全做到了它所承诺的。问题是真实的——文件截断/轮转是外部写入器向 需要注意的: diff 中的每一行改动都是修复所必需的。没有顺手重构,没有过度工程。测试设计精良——等大回归测试通过固定 mtime 确保真正测的是 hash 检测而非 size 回退,尤其用心。 结论:通过。 干净的修复,正确的测试,交付了 bug 所需的最小改动。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| @@ -179,13 +208,68 @@ export class RemoteInputWatcher { | |||
| return new Promise<void>((resolve) => { | |||
| rl.on('close', () => { | |||
| this.bytesRead = currentSize; | |||
There was a problem hiding this comment.
[Critical] Command replay on hash failure — state corruption causes false truncation detection
When hashFilePrefix(currentSize) returns null (I/O error), this.bytesRead is advanced to currentSize (line 210) but this.consumedPrefixHash is not updated (null-guarded at line 211). This breaks the invariant that consumedPrefixHash === hash(bytesRead). On the next poll, hasConsumedPrefixChanged() sees consumedPrefixHash === null with bytesRead > 0, returns true (line 222-226, new fail-open behavior), triggering a full file re-read from offset 0. Every command in the file is re-submitted.
If the I/O error persists (NFS hiccup, disk pressure, permission change), each poll cycle (500ms) re-submits all commands — an unbounded replay loop.
The same precondition exists in startWatching: if statSync succeeds but hashFilePrefix(this.bytesRead) returns null (line 108), bytesRead is set to stat.size but consumedPrefixHash stays null, setting up the same replay cycle.
| this.bytesRead = currentSize; | |
| // In readNewLines close handler, capture previous bytesRead: | |
| const previousBytesRead = this.bytesRead; | |
| this.bytesRead = currentSize; | |
| if (nextConsumedPrefixHash !== null) { | |
| this.consumedPrefixHash = nextConsumedPrefixHash; | |
| } else { | |
| // Hash failed — roll back to preserve invariant. | |
| this.bytesRead = previousBytesRead; | |
| } | |
| // In hasConsumedPrefixChanged, return false on null hash: | |
| if (currentHash === null) { | |
| debugLogger.warn('RemoteInput: failed to hash consumed prefix'); | |
| return false; | |
| } | |
| // In startWatching, fall back to offset 0 on hash failure: | |
| if (hashResult === null) { | |
| this.bytesRead = 0; | |
| this.consumedPrefixHash = null; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| remaining -= bytesRead; | ||
| position += bytesRead; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] Error object logged to debug may include stack traces
catch (err) passes the raw error object to debugLogger.warn(...). The debug logger's formatArgs() converts Error instances to their .stack property, writing full stack traces to the on-disk debug log.
| debugLogger.warn( | |
| 'RemoteInput: failed to hash file prefix:', | |
| err instanceof Error ? err.message : String(err), | |
| ); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
✅ Runtime E2E verification — truncated remote-input filesI built and ran the real The bug & the fix
// before — size only
if (currentSize <= this.bytesRead) return; // read [bytesRead, currentSize)Three failure modes on a truncate+rewrite:
The PR hashes the consumed prefix ( 1) Real compiled-watcher A/B —
|
| Rewrite size | pre-PR submitted | PR submitted |
|---|---|---|
| larger | [before] — after read mid-line → parse fail, lost ❌ |
[before, after] ✅ |
| same | [before] — early return, lost ❌ |
[before, after] ✅ |
| smaller | [before] — early return, lost ❌ |
[before, after] ✅ |
Pre-PR loses the post-truncate command in all three cases; the PR reads it in all three.
2) Real CLI E2E — --input-file (live in tmux)
Launched the real TUI with --input-file <f>, fed it a JSONL submit command, then truncated+rewrote the file to the same byte size with a different command:
| Arm | TUI user messages | Debug log |
|---|---|---|
| pre-PR | REMOTE_CMD_ALPHA only — OMEGA silently dropped |
no reset line |
| PR | REMOTE_CMD_ALPHA and REMOTE_CMD_OMEGA |
RemoteInput: input file prefix changed, resetting read offset → submitting: REMOTE_CMD_OMEGA |
# PR (new): # pre-PR (old):
> REMOTE_CMD_ALPHA > REMOTE_CMD_ALPHA
✦ ok ✦ ok
> REMOTE_CMD_OMEGA ← read (REMOTE_CMD_OMEGA never appears — dropped)
✦ ok
This is the nastiest case — same size and the rewrite would even keep the same mtime — which a size-only check can never catch; the prefix hash does.
3) Unit tests + revert-fix A/B
RemoteInputWatcher.test.tsis green: 7 passed, including the 2 new cases.- Reverting only the source reset block (keeping the PR's new tests) makes both new tests fail — e.g.
expected ['before-truncate'] to deeply equal ['before-truncate', 'after-truncate-with-a-longer-command']. So the new tests genuinely pin the bug; restoring the fix → 7/7 green.
Verdict
The fix makes the remote-input watcher robust to truncate+rewrite at any size (same / smaller / larger), verified through the real compiled watcher and the live --input-file TUI path; tests pin it; append-only behavior is unchanged. LGTM — good to merge. 👍
🇨🇳 中文版(点击展开)
✅ 运行时端到端验证 —— 被截断重写的 remote-input 文件
我从本 PR(dd4ce7ff,Node v22)构建并运行了真实的 qwen CLI,端到端确认修复:当 --input-file 被截断并重写(而非纯追加)时,watcher 以前会悄悄丢弃或错读新命令,现在能正确读取。作为 merge 参考。
Bug 与修复
RemoteInputWatcher 只跟踪字节偏移 bytesRead,假设写入都是纯追加:
// 改前 —— 只看大小
if (currentSize <= this.bytesRead) return; // 读取 [bytesRead, currentSize)截断重写时有三种失败模式:
- 相同大小 →
currentSize === bytesRead→ 提前返回 → 新命令从不被读取; - 更小 →
currentSize < bytesRead→ 提前返回 → 从不读取; - 更大 → 读取
[bytesRead, currentSize)= 重写内容的中间段 → 半行 →JSON.parse失败 → 命令丢失。
本 PR 对已消费前缀做哈希(consumedPrefixHash,SHA-256),每次轮询时若文件变小或已消费前缀的哈希变了,就把偏移重置为 0 —— 这样任意大小的重写都能被检测并重新读取。
1)真实编译产物 A/B —— 对真实磁盘文件跑 RemoteInputWatcher
先消费一个 before 命令,再用 after 命令在各种相对大小下截断重写,看实际提交了什么(dist 级 A/B,真实编译 watcher):
| 重写后大小 | 改前提交 | 本 PR 提交 |
|---|---|---|
| 更大 | [before] —— after 从半行读起 → 解析失败、丢失 ❌ |
[before, after] ✅ |
| 相同 | [before] —— 提前返回、丢失 ❌ |
[before, after] ✅ |
| 更小 | [before] —— 提前返回、丢失 ❌ |
[before, after] ✅ |
改前在三种情况下都丢掉了截断后的命令;本 PR 三种都能读到。
2)真实 CLI 端到端 —— --input-file(tmux 实时)
用 --input-file <f> 启动真实 TUI,喂给它一个 JSONL submit 命令,然后把文件截断重写成相同字节大小、内容不同的另一个命令:
| 分支 | TUI 用户消息 | 调试日志 |
|---|---|---|
| 改前 | 只有 REMOTE_CMD_ALPHA —— OMEGA 被悄悄丢弃 |
无 reset 行 |
| 本 PR | REMOTE_CMD_ALPHA 和 REMOTE_CMD_OMEGA |
RemoteInput: input file prefix changed, resetting read offset → submitting: REMOTE_CMD_OMEGA |
# 本 PR(新): # 改前(旧):
> REMOTE_CMD_ALPHA > REMOTE_CMD_ALPHA
✦ ok ✦ ok
> REMOTE_CMD_OMEGA ← 读到 (REMOTE_CMD_OMEGA 从不出现 —— 被丢弃)
✦ ok
这是最刁钻的情况 —— 大小相同、且重写后连 mtime 都可能一样 —— 只看大小的检查永远抓不到,而前缀哈希可以。
3)单测 + 撤销修复 A/B
RemoteInputWatcher.test.ts全绿:7 passed,含 2 个新增用例。- 只把源码里的 reset 块撤掉(保留 PR 的新测试),两个新测试都失败 —— 例如
expected ['before-truncate'] to deeply equal ['before-truncate', 'after-truncate-with-a-longer-command']。说明新测试确实钉住了这个 bug;恢复修复 → 重新 7/7 全绿。
结论
该修复让 remote-input watcher 对任意大小(相同 / 更小 / 更大)的截断重写都健壮,并通过真实编译 watcher 与真实 --input-file TUI 路径验证;测试钉死了它;纯追加行为不变。LGTM —— 可以合并。 👍
What this PR does
Detects when the remote input file has been truncated and rewritten, even if the new file is the same size or larger than the consumed prefix.
When the already-consumed prefix changes, the watcher resets its read offset so commands written after truncation are read normally. The update also covers hash failure handling and regression tests for larger and same-size rewrites.
Why it's needed
The previous remote input watcher mainly relied on file size. If a remote input file was truncated and rewritten to a same-size or larger JSONL payload, the watcher could keep its old offset and skip the new command forever.
Reviewer Test Plan
How to verify
Review
RemoteInputWatcherand its regression tests. The watcher should reset after a consumed-prefix mismatch, fail open on prefix hash errors, and continue to read rewritten commands after truncation.Run:
npm --workspace packages/cli run test -- src/remoteInput/RemoteInputWatcher.test.ts npx prettier --check packages/cli/src/remoteInput/RemoteInputWatcher.ts packages/cli/src/remoteInput/RemoteInputWatcher.test.ts npx eslint packages/cli/src/remoteInput/RemoteInputWatcher.ts packages/cli/src/remoteInput/RemoteInputWatcher.test.ts npx patch-package npm run build -- --cli-only npm --workspace packages/cli run typecheck git diff --checkEvidence (Before & After)
Before: after truncation and rewrite, the watcher could retain the old
bytesReadoffset and miss the rewritten command.After: the watcher compares the consumed prefix hash; if the prefix changed, it resets and reads the new JSONL command. Same-size and larger rewrite regressions are covered.
Tested on
Environment (optional)
Local Node/npm CLI workspace tests.
Risk & Scope
Linked Issues
Fixes #5471
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.
中文说明
这个 PR 做了什么
检测 remote input 文件被截断并重写的情况,即使新文件大小和已消费前缀相同或更大,也能发现。
当已消费前缀发生变化时,watcher 会重置读取 offset,让截断后写入的新命令可以被正常读取。这个更新也覆盖了 hash 失败处理,以及 larger/same-size rewrite 的回归测试。
为什么需要
旧的 remote input watcher 主要依赖文件大小。如果 remote input 文件被截断后重写成同大小或更大的 JSONL 内容,watcher 可能保留旧的
bytesReadoffset,导致新命令被永久跳过。Reviewer Test Plan
How to verify
检查
RemoteInputWatcher及其回归测试:消费前缀不一致时应重置;prefix hash 失败时应 fail open;截断重写后的命令应继续被读取。Evidence (Before & After)
修复前:截断重写后 watcher 可能保留旧
bytesRead,跳过新命令。修复后:watcher 比较已消费前缀 hash;前缀变化时重置并读取新的 JSONL 命令。同大小和更大 rewrite 都有回归测试覆盖。
Tested on
本地跑过 CLI workspace 相关 Node/npm 测试;Windows 和 Linux 由 CI 覆盖。
Risk & Scope
主要变化是通过 hash 已消费前缀来检测 truncation/rewrite,而不是只依赖文件大小。不改 remote input 文件格式或 polling 模型,也没有破坏性变更。