-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(desktop): preserve glued automation history records #6344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
a5bef75
517b942
25f3bf5
e6bd16b
c066bb3
221c257
77d0947
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -140,12 +140,102 @@ async function runCompaction( | |||||||||||
| // Pure compaction algorithm — shared by sync and async paths | ||||||||||||
| // ============================================================================ | ||||||||||||
|
|
||||||||||||
| function recoverHistoryObjectsFromLine(line: string): string[] { | ||||||||||||
| const recovered: string[] = []; | ||||||||||||
| let depth = 0; | ||||||||||||
| let start = -1; | ||||||||||||
| let cursor = 0; | ||||||||||||
| let inString = false; | ||||||||||||
| let escaped = false; | ||||||||||||
|
|
||||||||||||
| for (let i = 0; i < line.length; i++) { | ||||||||||||
| const char = line.charAt(i); | ||||||||||||
|
|
||||||||||||
| if (escaped) { | ||||||||||||
| escaped = false; | ||||||||||||
| continue; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (inString) { | ||||||||||||
| if (char === '\\') { | ||||||||||||
| escaped = true; | ||||||||||||
| } else if (char === '"') { | ||||||||||||
| inString = false; | ||||||||||||
| } | ||||||||||||
| continue; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (char === '"') { | ||||||||||||
| inString = true; | ||||||||||||
| continue; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (char === '{') { | ||||||||||||
| if (depth === 0 && line.slice(cursor, i).trim() !== '') { | ||||||||||||
| return recovered; | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] This bail-out branch ( — qwen3.7-max via Qwen Code /review |
||||||||||||
| } | ||||||||||||
| if (depth === 0) start = i; | ||||||||||||
| depth++; | ||||||||||||
| } else if (char === '}') { | ||||||||||||
| depth--; | ||||||||||||
| if (depth === 0 && start >= 0) { | ||||||||||||
| recovered.push(line.slice(start, i + 1)); | ||||||||||||
| cursor = i + 1; | ||||||||||||
| start = -1; | ||||||||||||
| } else if (depth < 0) { | ||||||||||||
| return recovered; | ||||||||||||
| } | ||||||||||||
| } else if (depth === 0 && char.trim() !== '') { | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The recovery parser bails out ( — qwen3.7-max via Qwen Code /review |
||||||||||||
| return recovered; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (depth !== 0 || inString || line.slice(cursor).trim() !== '') { | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The final guard clause is dead code — both branches return the same 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 — qwen3.7-max via Qwen Code /review
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in the latest follow-up commit: the final guard was removed because it returned |
||||||||||||
| return recovered; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| return recovered; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| function isValidHistoryObject(value: unknown): value is Record<string, unknown> { | ||||||||||||
| return value !== null && typeof value === 'object' && !Array.isArray(value); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| function getHistoryObjectId(value: Record<string, unknown>): string { | ||||||||||||
| return typeof value.id === 'string' ? value.id : ''; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| function parseHistoryLine(line: string): Array<{ raw: string; id: string }> { | ||||||||||||
| try { | ||||||||||||
| const parsed = JSON.parse(line); | ||||||||||||
| if (isValidHistoryObject(parsed)) { | ||||||||||||
| return [{ raw: line, id: getHistoryObjectId(parsed) }]; | ||||||||||||
| } | ||||||||||||
| return []; | ||||||||||||
| } catch { | ||||||||||||
| const recovered = recoverHistoryObjectsFromLine(line); | ||||||||||||
| const entries: Array<{ raw: string; id: string }> = []; | ||||||||||||
| for (const raw of recovered) { | ||||||||||||
| try { | ||||||||||||
| const parsed = JSON.parse(raw); | ||||||||||||
| if (isValidHistoryObject(parsed)) { | ||||||||||||
| entries.push({ raw, id: getHistoryObjectId(parsed) }); | ||||||||||||
| } | ||||||||||||
| } catch { | ||||||||||||
| // Skip only this recovered segment; keep any other valid objects. | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The comment "Fall through to drop the whole glued line below" suggests continued execution, but
Suggested change
— qwen3.7-max via Qwen Code /review
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Updated in |
||||||||||||
| return entries; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** | ||||||||||||
| * Apply two-tier retention to JSONL content: | ||||||||||||
| * 1. Per-automation cap: keep last `maxPerMatcher` entries per automation ID | ||||||||||||
| * 2. Global cap: keep last `maxTotal` entries overall | ||||||||||||
| * | ||||||||||||
| * Also drops malformed JSON lines. | ||||||||||||
| * Also drops malformed JSON lines, while preserving balanced object records | ||||||||||||
| * from glued history lines before rewriting the file. | ||||||||||||
| * | ||||||||||||
| * Returns the compacted output string, or `null` if no compaction was needed. | ||||||||||||
| */ | ||||||||||||
|
|
@@ -159,13 +249,13 @@ function compactEntries( | |||||||||||
|
|
||||||||||||
| // Parse all lines, dropping malformed ones | ||||||||||||
| const entries: Array<{ raw: string; id: string }> = []; | ||||||||||||
| let needsRewrite = false; | ||||||||||||
| for (const line of lines) { | ||||||||||||
| try { | ||||||||||||
| const parsed = JSON.parse(line); | ||||||||||||
| entries.push({ raw: line, id: parsed.id ?? '' }); | ||||||||||||
| } catch { | ||||||||||||
| // Drop malformed lines | ||||||||||||
| const parsedEntries = parseHistoryLine(line); | ||||||||||||
| if (parsedEntries.length !== 1 || parsedEntries[0]?.raw !== line) { | ||||||||||||
| needsRewrite = true; | ||||||||||||
| } | ||||||||||||
| entries.push(...parsedEntries); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // Track original line count (including malformed) for dirty-check | ||||||||||||
|
|
@@ -198,7 +288,7 @@ function compactEntries( | |||||||||||
| trimmed = trimmed.slice(-maxTotal); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if (trimmed.length === originalLineCount) return null; | ||||||||||||
| if (!needsRewrite && trimmed.length === originalLineCount) return null; | ||||||||||||
|
|
||||||||||||
| return trimmed.map(e => e.raw).join('\n') + '\n'; | ||||||||||||
| } | ||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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 whereneedsRewriteis accidentally bypassed:— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the requested rewrite assertion to the stray
}garbagetest. It now verifies both behavior surfaces:[1, 2];}garbageand has exactly two normalized JSONL records.Re-run validation: