fix(desktop): preserve glued automation history records#6344
Conversation
|
Thanks for the PR! (re-run) Template looks good ✓ — all required sections present, bilingual, linked issue #6343. Problem: This is a real bug with a linked issue (#6343) and a clear failure mode. Glued JSONL records occur when a partial write (process crash, power failure) leaves a line without its trailing newline — the next entry then gets concatenated onto the same physical line. Compaction drops both records as "malformed." The before/after demonstration confirms: old compaction keeps 2 of 4 entries, PR keeps all 4. Direction: Aligned. Narrow fix scoped to automation history compaction only — no changes to automation execution, scheduling, webhook retry, or root Approach: Scope feels right. The change is minimal — one new parsing function (~50 lines) and a refactor of the line-parsing loop in Moving on to code review. 🔍 中文说明感谢贡献！（re-run） 模板完整 ✓ — 所有必要部分齐全，双语，关联了 issue #6343。 问题：这是一个真实的 bug，有关联 issue（#6343）和清晰的失败模式。当部分写入（进程崩溃、断电）导致某行缺少尾部换行符时，下一条记录会被拼接到同一物理行上。压缩逻辑会把两条记录都当成 malformed line 丢弃。before/after 验证确认：旧压缩保留 4 条中的 2 条，PR 保留全部 4 条。 方向：对齐。改动范围窄，仅限于 automation history 压缩逻辑——不涉及 automation 执行、调度、webhook retry 或根目录 方案：范围合理。改动最小——一个新增的解析函数（~50 行）加上 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
2a. Code Review (re-run)Independent proposal: I would have added a character-by-character JSON object boundary scanner that tracks brace depth while correctly handling string literals and escape sequences. When The PR's implementation matches this approach exactly:
No correctness bugs, no security issues, no AGENTS.md violations. The code is focused, well-structured, and stays within the desktop package without introducing cross-workspace dependencies. 2b. Real-Scenario TestingUnit tests: 17 pass, 0 fail, 54 expect() calls — including 7 new tests covering glued-record recovery. Before/after demonstration with crafted The before/after confirms: old compaction silently drops glued records (ts 2 and 3 lost), PR's compaction recovers all 4 entries and normalizes them onto separate lines. 中文说明2a. 代码审查（re-run）独立方案：我会添加一个逐字符的 JSON 对象边界扫描器，在正确处理字符串字面量和转义序列的同时追踪花括号深度。当 PR 的实现与这个方案完全一致：
没有正确性 bug，没有安全问题，没有 AGENTS.md 违规。代码聚焦、结构清晰，保持在 desktop 包内，不引入跨 workspace 依赖。 2b. 真实场景测试单元测试：17 pass, 0 fail, 54 expect() calls ——包括 7 个新测试覆盖粘连记录恢复。 使用构造的 （见上方终端输出） before/after 确认：旧压缩默默丢弃粘连记录（ts 2 和 3 丢失），PR 的压缩恢复了全部 4 条记录并将它们规范化为单独的行。 — Qwen Code · qwen3.7-max |
|
Stepping back (re-run): this is a well-executed, narrow bug fix. The problem is real — partial writes to My independent proposal matched the PR's approach exactly. The implementation handles all stated edge cases, the test suite is thorough (17 tests, 54 assertions), and the before/after demonstration confirms the bug is fixed — 2 entries recovered where 2 were previously dropped. No scope creep, no cross-package dependencies, no unnecessary abstractions. The Approving. ✅ 中文说明回顾（re-run）：这是一个执行良好的、范围精确的 bug 修复。问题是真实的——对 我的独立方案与 PR 的方案完全一致。实现处理了所有声明的边界情况，测试套件充分（17 个测试，54 个断言），before/after 验证确认 bug 已修复——之前丢弃的 2 条记录现在被恢复。 没有范围蔓延，没有跨包依赖，没有不必要的抽象。 批准。✅ — 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. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Downgraded from Approve to Comment: CI still running. Suggestion-level recommendations are in the Suggestion summary comment below.
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/desktop/packages/shared/src/automations/history-store.ts |
The three recovery paths (recoverHistoryObjectsFromLine early returns, parseHistoryLine inner catch, non-object JSON rejection) silently discard data with no diagnostic logging. Peer modules in the same directory (retry-scheduler.ts, event-bus.ts) log error-recovery paths verbosely using the same createLogger utility. |
Add log.debug at bail-out points and the inner catch, e.g. log.debug('[HistoryStore] Recovery stopped at position %d, recovered %d objects', i, recovered.length) |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Downgraded from Approve to Comment: CI still running. No critical issues found — the implementation is correct for the documented use case. Suggestions below for robustness improvements.
— qwen3.7-max via Qwen Code /review
| return []; | ||
| } | ||
| } else if (depth === 0 && char.trim() !== '') { | ||
| return []; |
There was a problem hiding this comment.
[Suggestion] The in-loop guard return [] fires immediately on any non-whitespace at depth 0, even after valid objects have been accumulated in recovered. Similarly, recovered.length > 1 ? recovered : [] at the end discards a single valid object with trailing corruption.
In a crash-during-append scenario (e.g. {"id":"a","ts":1}{"id":"b","ts":2}partial-write), both valid entries are silently discarded despite the state machine having successfully identified them.
Consider breaking out of the loop (instead of return []) when trailing non-whitespace is encountered and returning whatever was already accumulated. Also adjust the final gate from > 1 to >= 1 — the caller already filters single-segment returns through the normal JSON.parse fast path.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks for catching this. Updated in 25f3bf586: the recovery state machine now preserves already recovered complete object records when it encounters trailing garbage/partial-write text, and the focused tests cover that exact prefix-preservation case.
| // Fall through to drop the whole glued line below. | ||
| } | ||
| return []; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The comment "Fall through to drop the whole glued line below" suggests continued execution, but return [] on the next line exits immediately. A future maintainer reading only the comment might conclude the return [] is redundant and remove it, silently changing behavior.
| } | |
| } catch { | |
| // Any sub-object failed — reject the entire glued line. | |
| } | |
| return []; |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Good point. Updated in 25f3bf586: the misleading fall-through comment is gone, and recovered segments are now validated through a shared helper while invalid recovered object segments are skipped without dropping other valid segments.
| expect(entries).toHaveLength(3); | ||
| }); | ||
|
|
||
| it('should preserve glued object records while dropping unrecoverable lines', async () => { |
There was a problem hiding this comment.
[Suggestion] The two new test cases cover the happy path well, but several edge cases for the recovery state machine are untested:
- Nested objects in glued lines (e.g.
{"a":{"b":1}}{"c":2}) — depth tracking is the core mechanism but only tested implicitly via string-embedded braces - 3+ glued objects — only 2-object lines are tested; cursor tracking across multiple boundaries is untested
- Trailing garbage (e.g.
{"a":1}{"b":2}garbage) — the rejection path is untested - needsRewrite triggers file rewrite — no test asserts the file is rewritten when glued lines exist but counts are within limits
- All-or-nothing drop — no test verifies what happens when one sub-object in a glued line is invalid
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks, added the requested edge coverage in 25f3bf586: nested objects, 3+ glued objects, trailing partial garbage, rewrite normalization when glued lines are within limits, and invalid recovered object segments preserving surrounding valid records.
Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
|
Thanks @wenshao for the careful suggestions. I updated the PR in
Validation re-run after rebasing on the updated remote branch: The commit includes your co-author trailer for the substantive review suggestion. Thanks again. |
Maintainer verification — real local build & tests ✅I built and ran this PR locally against the real desktop
1 · Real test suite + typecheck @ head
2 · A/B mutation — are the tests (and the fix) load-bearing?
3 · End-to-end driver on the real compaction pathA driver that imports the actual
4 · Note (non-blocking): the
|
Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
|
Thanks @wenshao, great catch on the surviving Re-run validation after the new test: PR body has also been updated with the new validation count and count-coincidence rewrite coverage. Thanks again for the precise local verification. |
| cursor = i + 1; | ||
| start = -1; | ||
| } else if (depth < 0) { | ||
| return []; |
There was a problem hiding this comment.
[Critical] return [] on depth < 0 discards all previously recovered valid objects. Every other early-exit path in this function (non-whitespace gap at depth 0, trailing non-whitespace, end-of-loop trailing content) correctly returns recovered — only this branch throws them away.
For input like {"id":"a1","ts":1}}garbage, the first object is fully parsed and pushed into recovered, then the stray } decrements depth to -1, and return [] silently drops the valid entry. This is the exact data-loss scenario the PR exists to prevent.
| return []; | |
| } else if (depth < 0) { | |
| return recovered; | |
| } |
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Thanks for catching this. Fixed in 221c2570b: the depth < 0 branch now returns the already recovered object segments instead of clearing them, so a later stray } cannot discard valid history objects that were parsed before it.
I also added focused coverage for this exact case: a valid recovered record followed by a stray }/garbage suffix is preserved alongside the following normal record.
| cursor = i + 1; | ||
| start = -1; | ||
| } else if (depth < 0) { | ||
| return []; |
There was a problem hiding this comment.
[Suggestion] Two defensive branches lack dedicated test coverage:
- The
depth < 0branch (line 186) — no test exercises a stray}after valid objects on a glued line. - The
isValidHistoryObjectfalse path inparseHistoryLine's try-block — whilenull/42/[]lines appear in the "nested and 3+ glued" test, they only assert final entry count, not that these values trigger file rewrite vianeedsRewrite.
Consider adding a test for each path, e.g.:
it('should recover valid objects before a stray closing brace', async () => {
writeFileSync(
join(tempDir, AUTOMATIONS_HISTORY_FILE),
JSON.stringify(makeEntry('a1', 1)) + '}garbage\n' + JSON.stringify(makeEntry('a1', 2)) + '\n',
);
await compactAutomationHistory(tempDir, 20, 1000);
const entries = readHistory(tempDir);
expect(entries.map((entry) => entry.ts)).toEqual([1, 2]);
});— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added both defensive coverage cases in 221c2570b:
- stray closing brace after a valid recovered object;
- non-object JSON lines (
null,42,[]) being normalized out of the rewritten history file.
Re-run validation:
D:\ZXY\Dev\nodejs\npx.cmd --yes bun test packages/shared/src/automations/history-store.test.ts
17 pass, 0 fail, 52 expect() calls
D:\ZXY\Dev\nodejs\npx.cmd --yes bun run typecheck:shared
passed
wsl -e bash -lc 'cd /mnt/d/ZXY/Github/qwen-code/packages/desktop && npx --yes bun test packages/shared/src/automations/history-store.test.ts'
17 pass, 0 fail, 52 expect() calls
git diff --check
passed
|
Thanks for the follow-up review. I pushed
Local validation passed on Windows and WSL/Linux, plus shared typecheck and |
| } | ||
| } | ||
|
|
||
| if (depth !== 0 || inString || line.slice(cursor).trim() !== '') { |
There was a problem hiding this comment.
[Suggestion] The final guard clause is dead code — both branches return the same recovered:
if (depth !== 0 || inString || line.slice(cursor).trim() !== '') {
return recovered; // ← returns recovered
}
return recovered; // ← also returns recoveredThe condition has no observable effect. A future maintainer may assume the condition gates different behavior and add logic to one branch, introducing a subtle bug. Either remove the if entirely (just return recovered;) or implement the intended distinction — e.g., return { objects: recovered, partial: true } to signal incomplete recovery.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Addressed in the latest follow-up commit: the final guard was removed because it returned recovered on both paths and had no observable effect. The recovery function now has a single explicit return recovered; at the end.
| await compactAutomationHistory(tempDir, 20, 1000); | ||
|
|
||
| const entries = readHistory(tempDir); | ||
| expect(entries.map((entry) => entry.ts)).toEqual([1, 2]); |
There was a problem hiding this comment.
[Suggestion] This test verifies both entries survive compaction but doesn't assert that the file was actually rewritten to remove }garbage. Sibling tests (e.g., "should preserve nested and 3+ glued object records") check the rewritten file content. Adding a file-content assertion would catch a regression where needsRewrite is accidentally bypassed:
const rewritten = readFileSync(join(tempDir, AUTOMATIONS_HISTORY_FILE), 'utf-8');
expect(rewritten).not.toContain('}garbage');
expect(rewritten.trim().split('\n')).toHaveLength(2);— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Added the requested rewrite assertion to the stray }garbage test. It now verifies both behavior surfaces:
- recovered entries still read back as
[1, 2]; - the rewritten file no longer contains
}garbageand has exactly two normalized JSONL records.
Re-run validation:
D:\ZXY\Dev\nodejs\npx.cmd --yes bun test packages/shared/src/automations/history-store.test.ts
17 pass, 0 fail, 54 expect() calls
D:\ZXY\Dev\nodejs\npx.cmd --yes bun run typecheck:shared
passed
wsl -e bash -lc 'cd /mnt/d/ZXY/Github/qwen-code/packages/desktop && npx --yes bun test packages/shared/src/automations/history-store.test.ts'
17 pass, 0 fail, 54 expect() calls
git diff --check
passed
|
Latest reviewer suggestions have been addressed and pushed:
Validation passed on Windows, WSL/Linux, shared typecheck, and |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
| } else if (depth < 0) { | ||
| return recovered; | ||
| } | ||
| } else if (depth === 0 && char.trim() !== '') { |
There was a problem hiding this comment.
[Suggestion] The recovery parser bails out (return recovered) on any non-whitespace character at depth 0, abandoning all subsequent objects on the same physical line. For a crash-interrupted write like {"id":"a1","ts":1,"ok":tr followed by the next entry {"id":"a1","ts":2,"ok":true} glued on the same line, the partial first object triggers a bail-out and the valid second object is silently lost. This is still strictly better than the old code (which dropped the entire line), but consider either resuming the scan after garbage or logging a warning when the early-return fires, so data loss is at least observable.
— qwen3.7-max via Qwen Code /review
| await compactAutomationHistory(tempDir, 20, 1000); | ||
|
|
||
| const rewritten = readFileSync(join(tempDir, AUTOMATIONS_HISTORY_FILE), 'utf-8'); | ||
| expect(rewritten).not.toContain('}garbage'); |
There was a problem hiding this comment.
[Suggestion] This assertion checks expect(rewritten).not.toContain('}garbage'), but the test input above uses 'partial-write' as the trailing garbage text. The substring }garbage never appears in the input, making this assertion vacuously true — it would pass regardless of whether the rewrite actually stripped the garbage. It should check expect(rewritten).not.toContain('partial-write') instead. (The sibling "stray closing brace" test correctly asserts against }garbage — this looks like a copy-paste from that test.)
| expect(rewritten).not.toContain('}garbage'); | |
| expect(rewritten).not.toContain('partial-write'); |
— qwen3.7-max via Qwen Code /review
|
|
||
| if (char === '{') { | ||
| if (depth === 0 && line.slice(cursor, i).trim() !== '') { | ||
| return recovered; |
There was a problem hiding this comment.
[Suggestion] This bail-out branch (depth === 0 && line.slice(cursor, i).trim() !== '') — which fires when non-whitespace garbage appears between two recovered objects before the next { — has no dedicated test coverage. An input like {"a":1}xxx{"b":2} would exercise this path and verify that the parser correctly stops recovery when inter-object garbage is present.
— qwen3.7-max via Qwen Code /review
|
Ok all dz 🧐 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
|
@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. ✅
qwen-code-ci-bot
added
category/core
4 participants
What this PR does
This PR updates desktop automation history compaction so it can preserve recoverable automation history records when multiple complete JSON objects are glued onto the same physical JSONL line.
The change is scoped to
automations-history.jsonlretention and compaction. It keeps ordinary malformed-line skipping unchanged, does not change automation execution, scheduling, webhook retry behavior, or the root qwen-code JSONL utilities, and avoids introducing a cross-workspace dependency from the desktop package into the root core package.Why it's needed
What Problem This Solves
Automation history is stored as JSONL: each completed automation history entry should be consumable as one JSON object record, normally separated by a newline.
Before this change, automation history compaction treated each physical line as either one valid JSON object or one malformed line. That works for ordinary records, but it loses recoverable data when a history file contains two complete objects glued together on the same line, for example:
{"id":"a1","ts":1,"ok":true} {"id":"a1","ts":2,"ok":true}{"id":"a1","ts":3,"ok":true}The second physical line is not random garbage. It contains two balanced top-level JSON objects, but a single
JSON.parse(line)cannot parse it because the newline separator between the two records is missing. The previous compaction path caught that parse failure, treated the whole physical line as malformed, and dropped both otherwise recoverable history entries.That matters because compaction rewrites
automations-history.jsonl. Once retention runs, recoverable glued records can become permanent automation history loss. The affected data is automation run history and last-executed evidence; automation execution itself is not changed.Changes
This PR keeps the parser deliberately narrow:
null,42, or[]as automation history entries;packages/coreJSONL helper.It also keeps the compaction rewrite decision aware of recovery. A glued line still forces normalization even when the recovered entry count happens to match the original physical line count.
Evidence
Focused validation passed on Windows PowerShell:
Observed result:
Focused validation also passed under WSL/Linux:
Observed result:
The new coverage exercises:
null,42, and[].Screenshots are not included because this is a non-UI persistence/parser fix.
Possible call chain / impact
Production readers consume automation history line by line. A glued physical line is therefore invisible to normal per-line JSON parsing until compaction normalizes it back into separate records.
This PR changes only the desktop automation history compaction path. It does not change automation execution, scheduling semantics, webhook retries, UI rendering, or the root
packages/coreJSONL utilities.Reviewer Test Plan
How to verify
Run the focused desktop automation history test and confirm the glued-record cases pass. The new coverage writes a temporary
automations-history.jsonlfile containing complete object records with a missing newline separator:{"id":"a1","ts":1,"ok":true} {"id":"a1","ts":2,"ok":true}{"id":"a1","ts":3,"ok":true}Expected behavior after compaction:
null,42, and[]are not treated as history entries and are normalized out on rewrite;packages/coreJSONL helper.Evidence (Before & After)
N/A for screenshots; this is a non-UI persistence/parser fix.
Before: a glued line such as
{"id":"a1","ts":2,"ok":true}{"id":"a1","ts":3,"ok":true}was treated as one malformed physical line and dropped during compaction.After: the two balanced object records are recovered as separate automation history entries before compaction rewrites the file. The latest follow-up also preserves valid recovered objects before a later stray
}branch.Tested on
Environment (optional)
Windows: Node.js v24.15.0 with
npx --yes bunusing Bun v1.3.14.WSL/Linux: Node.js v24.15.0 with
npx --yes bunusing Bun v1.3.14.Risk & Scope
packages/coreJSONL helper, retry scheduling semantics, automation execution, webhook request behavior, or UI rendering.Linked Issues
Fixes #6343
中文说明
What this PR does
这个 PR 调整 desktop automation history 的压缩逻辑，使其在多个完整 JSON object 被粘在同一条 JSONL 物理行上时，可以恢复并保留这些可恢复的 automation history 记录。
改动范围只限于
automations-history.jsonl的 retention/compaction。它不改变 automation 执行、调度、webhook retry 行为，也不跨 workspace 引入根目录packages/core的 JSONL helper。Why it's needed
What Problem This Solves
automation history 使用 JSONL 存储。正常情况下，每条完成的 automation history entry 都应该是一条 JSON object record，并通过换行分隔。
修改前，compaction 把每个物理行当成“一个合法 JSON object”或“一条 malformed line”。这对普通记录没问题，但如果文件里出现两个完整 object 粘在同一行的情况，例如：
{"id":"a1","ts":1,"ok":true} {"id":"a1","ts":2,"ok":true}{"id":"a1","ts":3,"ok":true}第二个物理行并不是完全不可恢复的坏数据，而是两个 balanced top-level JSON objects 少了中间的换行符。旧逻辑用单次
JSON.parse(line)解析失败后，会把整条物理行当成 malformed line 跳过。这会影响 automation history，因为 compaction 会重写
automations-history.jsonl。一旦 retention 跑过，原本可恢复的 glued records 就可能变成永久历史记录丢失。受影响的是 automation run history 和 last-executed evidence，不是 automation 执行本身。Changes
这个 PR 的恢复逻辑保持很窄：只恢复 glued line 里的 balanced top-level object records，继续跳过普通 malformed line、trailing partial write，并且不把
null、42、[]这类非 object JSON 值当成 history entry。最新 follow-up 也修复了 stray
}分支：如果前面已经恢复出了有效 object，后面再遇到多余的}或 garbage，不会把前面已经恢复的对象清空。同时，当 glued line 被恢复时，即使恢复出的记录数恰好等于原始物理行数量，也会强制 normalization rewrite，避免文件继续保留对逐行 reader 不可见的粘连记录。
Evidence
Windows focused test、shared typecheck、WSL/Linux focused test 均已通过：
该变更是非 UI 的持久化/parser 修复，因此截图或录屏不适用。
Possible call chain / impact
受影响的是 desktop automation history compaction 路径。automation 执行、调度、webhook retry、UI 渲染和根目录
packages/coreJSONL helper 不在本 PR 范围内。Reviewer Test Plan
见英文部分。
Risk & Scope
主要风险是恢复逻辑不能过宽，避免把任意坏 JSONL 误解释为合法 automation history。因此修复保持在 automation history compaction 的局部边界内，不引入跨 workspace 依赖，同时只保留可独立解析和验证的完整 object segment。