fix(cli): preserve partial remote input JSONL records#6317
Conversation
|
Thanks for the PR! Template looks good ✓ — all required sections present, bilingual. Problem: This is a real, observed bug. Issue #6316 describes the exact scenario — incremental JSONL writers splitting a record across multiple appends, causing the watcher to consume a partial record as malformed and lose the completed command. The reproduction script confirms it: on main, a split Direction: Aligned. The remote input watcher is a well-scoped CLI feature, and handling partial JSONL records correctly is core to its contract with external writers. This is the kind of edge case that bites integrations in practice. Approach: Tight and minimal. The diff is ~19 lines of production code — adds Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必要章节齐全，双语。 问题：这是一个真实存在的 bug。Issue #6316 描述了确切的场景——增量 JSONL 写入器把一条记录拆成多次 append，导致 watcher 把不完整的记录当作 malformed line 消费，最终丢失已补齐的命令。复现脚本验证了这一点：在 main 分支上，拆分的 方向：对齐。Remote input watcher 是一个范围明确的 CLI 功能，正确处理不完整的 JSONL 记录是其与外部写入器之间契约的核心。这类边界情况在实际集成中确实会造成问题。 方案：紧凑且最小化。diff 大约 19 行生产代码——新增 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe implementation is clean and correct. Independent proposal: I'd have taken the same approach — scan for the last newline in the new byte range, read up to it, defer the rest. The PR's implementation matches this exactly. Key correctness points verified:
No blockers, no AGENTS.md violations. The extra file I/O in Real-Scenario TestingRan a reproduction script that writes a partial JSONL submit record, polls, completes the record, polls again — on both main (before) and the PR branch (after). Before (main branch)The watcher consumed the unterminated After (this PR)The watcher deferred the unterminated partial record, then correctly consumed the completed Unit TestsAll 10 tests pass on the PR branch (7 existing + 3 new): The 3 new tests cover the exact scenarios described in the PR: split submit, split confirmation, and mixed complete+partial in the same poll. — Qwen Code · qwen3.7-max |
|
This is a tight, well-executed bug fix. The problem is real — incremental JSONL writers splitting records across appends is a legitimate edge case for the remote input watcher, and the bug is trivially reproducible on main (the reproduction script confirms the split submit is silently dropped). The implementation matches what I'd have written independently: scan the new byte range for the last newline, read only up to that boundary, defer the trailing unterminated bytes. It's 19 lines of production code that directly solve the stated problem without touching anything else. The truncate/rewrite detection path remains correct because the prefix hash now reflects the actually-consumed boundary rather than the raw file size. The before/after testing is conclusive — the same reproduction script fails on main and passes on the PR branch. Unit tests cover all three split-record paths (submit, confirmation, mixed) plus the existing regression suite. No concerns. Ships the fix cleanly. 中文说明这是一个紧凑、执行良好的 bug 修复。问题是真实的——增量 JSONL 写入器把记录拆成多次 append 是 remote input watcher 的一个合理边界情况，而且这个 bug 在 main 分支上可以轻松复现（复现脚本确认拆分的 submit 被静默丢弃）。 实现方案和我独立设想的完全一致：扫描新增字节范围找到最后一个换行符，只读取到该边界，将尾部未以换行结束的字节推迟处理。19 行生产代码直接解决既定问题，没有触碰其他任何东西。truncate/rewrite 检测路径仍然正确，因为前缀哈希现在反映的是实际已消费的边界，而不是原始文件大小。 Before/after 测试是结论性的——同一个复现脚本在 main 上失败，在 PR 分支上通过。单元测试覆盖了全部三种 split-record 路径（submit、confirmation、mixed），以及已有的回归测试套件。 没有顾虑。干净地交付了修复。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| await watcher.checkForNewInput(); | ||
|
|
||
| expect(handler).toHaveBeenCalledWith('req-7', true); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] Both new tests only cover the case where the entire appended content lacks a newline. The more common (and more interesting) real-world scenario is a single poll where the file contains one or more complete records followed by a trailing partial — e.g., a fast writer appends {"type":"submit","text":"first"}\n{"type":"submit","text":"sec between polls. This exercises the core findLastCompleteRecordEnd path where lastLineBreak is found mid-range, not absent entirely.
Consider adding a test:
it('consumes complete records but defers a trailing partial in the same poll', async () => {
watcher = new RemoteInputWatcher(inputFile);
const submitted: string[] = [];
watcher.setSubmitFn((text) => {
submitted.push(text);
});
fs.appendFileSync(
inputFile,
'{"type":"submit","text":"first"}\n{"type":"submit","text":"sec',
);
await watcher.checkForNewInput();
expect(submitted).toEqual(['first']);
fs.appendFileSync(inputFile, 'ond"}\n');
await watcher.checkForNewInput();
expect(submitted).toEqual(['first', 'second']);
});— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks for the suggestion. I added this mixed-record coverage in 68c9c95de: the new test appends one complete submit record followed by a trailing partial record in the same poll, verifies that only the complete record is consumed first, then appends the remaining bytes and verifies the deferred command is processed on the next poll.
I also re-ran the focused watcher suite on Windows and in an isolated Ubuntu-22.04 WSL2 clone:
npm --workspace packages/cli run test -- src/remoteInput/RemoteInputWatcher.test.ts
Test Files 1 passed (1)
Tests 10 passed (10)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM. The partial-record deferral is a clean, narrow fix: findLastCompleteRecordEnd correctly bounds createReadStream to the last complete \n-terminated record, and the three new split-record tests cover the key scenarios (split submit, mixed complete+partial, split confirmation). Deterministic analysis clean (eslint + tsc: 0 findings). All 10 tests in RemoteInputWatcher.test.ts pass.
Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| if (currentSize <= this.bytesRead) return Promise.resolve(); | ||
|
|
||
| const nextConsumedPrefixHash = this.hashFilePrefix(currentSize); | ||
| const consumeUntil = this.findLastCompleteRecordEnd( |
There was a problem hiding this comment.
[Suggestion] This now scans from bytesRead to currentSize before every read, but when the appended tail still has no newline the method returns without advancing any state. If a writer streams a large record in many newline-free chunks, each poll re-scans the whole pending tail synchronously in the TUI process, so the total work grows cumulatively and can stall the UI. Consider tracking the already-scanned pending offset, scanning backward from the newest bytes, or enforcing a maximum pending record size and resetting that state on truncate/prefix-change paths.
— GPT-5 via Qwen Code /review
| let fd: number | null = null; | ||
| try { | ||
| fd = openSync(this.filePath, 'r'); | ||
| const buffer = Buffer.allocUnsafe(Math.min(64 * 1024, end - start)); |
There was a problem hiding this comment.
[Suggestion] The scanner reads in 64 KiB chunks, but the new split-record tests only cover tiny appended ranges. A regression in the chunk-advance loop would still pass the current suite. It would be worth adding one test with a submit payload larger than 64 KiB, ideally followed by a trailing partial record, and asserting the large complete record is consumed once while the partial remains deferred until its newline arrives.
— GPT-5 via Qwen Code /review
3 participants
What this PR does
Prevents remote input from consuming a trailing incomplete JSONL record before it has been terminated by a newline. Once the writer finishes the record, the watcher processes the submit or confirmation command normally.
Why it's needed
What Problem This Solves
Remote input is consumed as a newline-delimited JSONL command stream: a command should only be read once its record has been terminated by
\n. That matters because external writers can append one JSONL record in multiple filesystem writes, leaving the file temporarily ending with a valid prefix of a command rather than a complete JSONL line.For example, a sidecar or integration can split one
submitcommand across two appends:Before this change,
readNewLines()read through the current file size. If the file ended with an unterminated fragment, the watcher could pass that fragment toJSON.parse, treat it as a malformed line, and advancebytesReadpast it. When the writer later appended the remaining bytes, the watcher resumed after the skipped fragment, so the completed command could no longer be reconstructed by the watcher.The practical impact is limited but real: incrementally written
submitcommands orconfirmation_responseapprovals could be missed when the final newline arrived after a polling cycle. This PR keeps trailing unterminated bytes pending until the newline completes the JSONL record, while preserving the existing behavior for complete malformed lines.Changes
The watcher now finds the last complete newline-terminated record in the newly appended byte range before creating the read stream. It reads only through that complete boundary and advances
bytesReadto the consumed boundary instead of the current file size.Bytes after the last newline remain pending for the next poll. The command schema, polling model, submit queue, confirmation dispatch path, truncate/rewrite detection, and complete-malformed-line handling are intentionally unchanged.
Evidence
The new split-submit test writes
{"type":"submit","text":"spl, confirms that no submit is queued yet, then appendsit"}\nand confirms thatsplitis submitted.The new split-confirmation test covers the same trailing-partial-record behavior for
confirmation_response: the handler is not called while the record is unterminated, then receives('req-7', true)after the remaining bytes and newline are appended. The mixed-record test covers the reviewer-requested path where one poll contains a completesubmitrecord followed by a trailing partial record; the complete command is consumed immediately and the partial command is deferred until its newline arrives.Focused validation:
The Linux run used a temporary clone under
/tmpwith a Linux Node.js tarball, so it did not reuse or rewrite the Windows checkout'snode_modules. The suite includes the three split-record tests plus the existing malformed-line, queued-submit, shutdown, and truncate/rewrite coverage.Possible call chain / impact
This PR only changes how the watcher decides the readable boundary for appended input. Incremental writers no longer miss a trailing partial command when a poll happens before the newline is appended.
Nearby behavior remains unchanged: complete malformed lines are still skipped, submits can still queue while the TUI is busy, confirmations still dispatch immediately once parsed, shutdown behavior is unchanged, and truncate/rewrite handling keeps its existing coverage.
Reviewer Test Plan
How to verify
Run
npm --workspace packages/cli run test -- src/remoteInput/RemoteInputWatcher.test.tsand confirm the split submit and split confirmation cases are both processed only after the newline completes the JSONL record. The existing malformed-line, queued-submit, shutdown, and truncate/rewrite cases should continue to pass.Evidence (Before & After)
Before: a trailing partial submit or confirmation record could be consumed as a malformed line before the writer finished it, causing the completed command to be missed by the watcher. After: the watcher leaves the partial record unread, then processes it successfully after the remaining bytes and newline arrive.
Tested on
Environment (optional)
Windows local validation used Node.js v24.15.0. Linux validation used an isolated
/tmpclone on Ubuntu-22.04 WSL2 with Linux Node.js v22.17.0, avoiding the Windows checkout and itsnode_modules.Risk & Scope
Linked Issues
Fixes #6316
中文说明
What this PR does
这个 PR 防止 remote input 在 JSONL 记录还没有以换行符结束前，就提前消费文件尾部的不完整记录。写入方补完整条记录并追加换行后，watcher 会正常处理
submit或confirmation_response命令。Why it's needed
What Problem This Solves
Remote input 是一条以换行符分隔的 JSONL 命令流：一条命令只有在记录以
\n结束后，才应该被当作完整命令读取。关键点是，外部写入器不一定一次性写完整条 JSONL 记录；它可能把同一条命令拆成多次文件 append，所以文件尾部会短暂出现“只写了一半”的命令片段。例如，边车进程或集成方可能把一条
submit命令拆成两次写入：修复前，
readNewLines()会直接读到当前文件大小。如果文件尾部正好是不完整 JSON 片段，watcher 可能会把这段片段交给JSON.parse，随后按 malformed line 跳过，并把bytesRead推进到这段字节之后。等写入方再追加剩余字节时，watcher 已经从后面的位置继续读取，无法再把前半段和后半段拼回原始命令。实际影响是窄范围但真实的：增量写入的
submit命令或confirmation_response确认响应，可能在最终换行到达后仍被 watcher 漏掉。这个 PR 会把尚未以换行结束的尾部字节保留到下一轮轮询，直到换行补齐 JSONL 记录；完整但格式错误的行仍按原有逻辑跳过。Changes
现在 watcher 会在新增字节范围内先找到最后一条完整的、以换行结束的记录。随后它只读取到这个完整边界，并把
bytesRead推进到实际已消费的位置，而不是直接推进到当前文件大小。最后一个换行符之后的尾部字节会留到下一轮轮询继续等待。命令 schema、轮询模型、submit 队列、confirmation 分发路径、truncate/rewrite 检测，以及完整 malformed line 的处理逻辑都保持不变。
Evidence
新增的 split-submit 测试会先写入半条
{"type":"submit","text":"spl，确认此时不会提交任何内容；再追加it"}\n，确认最终提交的是split。新增的 split-confirmation 测试对
confirmation_response做同样覆盖：记录尚未以换行结束时 handler 不会触发；追加剩余字节和换行后，handler 会收到('req-7', true)。新增的 mixed-record 测试覆盖 reviewer 指出的路径：同一次 poll 中同时包含一条完整的submit记录和一条尾部 partial 记录；完整命令会立即消费，partial 命令会等换行到达后再处理。聚焦验证：
Linux 侧验证使用
/tmp下的临时 clone 和 Linux Node.js tarball，没有复用或改写 Windows checkout 的node_modules。这 10 个测试包括三个 split-record 场景，以及已有 malformed line、queued submit、shutdown、truncate/rewrite 等回归覆盖。Possible call chain / impact
这个 PR 只改变 watcher 对“新增输入可读取边界”的判断。对于增量写入器来说，poll 发生在最终换行到达之前时，尾部 partial command 不会再被提前跳过。
相邻行为保持不变：完整但格式错误的行仍会跳过；TUI 忙碌时 submit 仍可排队；confirmation 解析后仍立即分发；shutdown 行为不变；truncate/rewrite 处理继续由现有测试覆盖。
Reviewer Test Plan
How to verify
运行
npm --workspace packages/cli run test -- src/remoteInput/RemoteInputWatcher.test.ts，确认 split submit 和 split confirmation 都只会在换行补齐 JSONL 记录后被处理。同时，已有 malformed line、queued submit、shutdown、truncate/rewrite 场景应继续通过。Evidence (Before & After)
修复前：尾部不完整的 submit 或 confirmation 记录可能在写入方完成前被 watcher 当作 malformed line 消费，导致补齐后的命令被 watcher 漏掉。
修复后：watcher 会保留未完成记录，等剩余字节和换行到达后再成功处理。
Tested on
本地 Windows 已验证；Linux 已通过 Ubuntu-22.04 WSL2 隔离临时 clone 验证；macOS 未在本地验证，留给项目 CI 覆盖。
Environment (optional)
Windows 本地验证使用 Node.js v24.15.0。Linux 验证使用 Ubuntu-22.04 WSL2 + Linux Node.js v22.17.0，并在
/tmp临时 clone 内安装依赖，避免污染 Windows checkout。Risk & Scope
主要风险或权衡是：watcher 现在会把尚未以换行结束的尾部记录延后处理，所以命令处理会等到写入方追加最终换行符之后再发生。
不改变 remote input 文件格式、轮询模型、写入方行为、命令 schema、submit 队列或 confirmation 分发语义。
没有破坏性变更或迁移要求。
Linked Issues
Fixes #6316