-
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 5 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; | ||||||||||||
| } | ||||||||||||
| 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 []; | ||||||||||||
|
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. [Critical] For input like
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. Thanks for catching this. Fixed in I also added focused coverage for this exact case: a valid recovered record followed by a stray
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] Two defensive branches lack dedicated test coverage:
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
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. Added both defensive coverage cases in
Re-run validation: |
||||||||||||
| } | ||||||||||||
| } 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 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