Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 133 additions & 0 deletions packages/desktop/packages/shared/src/automations/history-store.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,139 @@ describe('history-store', () => {
expect(entries).toHaveLength(3);
});

it('should preserve nested and 3+ glued object records while rewriting the file', async () => {
const gluedLine =
JSON.stringify(makeEntry('a1', 2)) +
JSON.stringify({ ...makeEntry('a1', 3), nested: { value: true } }) +
JSON.stringify(makeEntry('a1', 4));
const lines = [
JSON.stringify(makeEntry('a1', 1)),
gluedLine,
'not-json{{{',
'null',
'42',
'[]',
];
writeFileSync(
join(tempDir, AUTOMATIONS_HISTORY_FILE),
lines.join('\n') + '\n',
);

await compactAutomationHistory(tempDir, 20, 1000);

const entries = readHistory(tempDir);
expect(entries.map((entry) => entry.ts)).toEqual([1, 2, 3, 4]);
expect(entries[2]!.nested).toEqual({ value: true });

const rewritten = readFileSync(join(tempDir, AUTOMATIONS_HISTORY_FILE), 'utf-8');
expect(rewritten).not.toContain('}{');
expect(rewritten.trim().split('\n')).toHaveLength(4);
});

it('should preserve complete glued records before trailing garbage', async () => {
const first = makeEntry('a1', 1);
const second = makeEntry('a1', 2);
writeFileSync(
join(tempDir, AUTOMATIONS_HISTORY_FILE),
JSON.stringify(first) + JSON.stringify(second) + 'partial-write\n',
);

await compactAutomationHistory(tempDir, 20, 1000);

const rewritten = readFileSync(join(tempDir, AUTOMATIONS_HISTORY_FILE), 'utf-8');
expect(rewritten).not.toContain('}garbage');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.)

Suggested change
expect(rewritten).not.toContain('}garbage');
expect(rewritten).not.toContain('partial-write');

— qwen3.7-max via Qwen Code /review

expect(rewritten.trim().split('\n')).toHaveLength(2);

const entries = readHistory(tempDir);
expect(entries.map((entry) => entry.ts)).toEqual([1, 2]);
});

it('should recover valid objects before a stray closing brace', async () => {
const first = makeEntry('a1', 1);
const second = makeEntry('a1', 2);
writeFileSync(
join(tempDir, AUTOMATIONS_HISTORY_FILE),
JSON.stringify(first) + '}garbage\n' + JSON.stringify(second) + '\n',
);

await compactAutomationHistory(tempDir, 20, 1000);

const entries = readHistory(tempDir);
expect(entries.map((entry) => entry.ts)).toEqual([1, 2]);

Copy link
Copy Markdown
Collaborator

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 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

Copy link
Copy Markdown
Contributor Author

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 }garbage test. It now verifies both behavior surfaces:

  • recovered entries still read back as [1, 2];
  • the rewritten file no longer contains }garbage and 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

});

it('should rewrite glued records when recovered count matches physical line count', async () => {
const first = makeEntry('a1', 1);
const second = makeEntry('a1', 2);
writeFileSync(
join(tempDir, AUTOMATIONS_HISTORY_FILE),
JSON.stringify(first) + JSON.stringify(second) + '\nnot-json{{{\n',
);

await compactAutomationHistory(tempDir, 20, 1000);

const rewritten = readFileSync(join(tempDir, AUTOMATIONS_HISTORY_FILE), 'utf-8');
expect(rewritten).not.toContain('}{');
expect(rewritten.trim().split('\n')).toHaveLength(2);

const entries = readHistory(tempDir);
expect(entries.map((entry) => entry.ts)).toEqual([1, 2]);
});

it('should rewrite non-object JSON history lines out of the file', async () => {
writeFileSync(
join(tempDir, AUTOMATIONS_HISTORY_FILE),
[
JSON.stringify(makeEntry('a1', 1)),
'null',
'42',
'[]',
JSON.stringify(makeEntry('a1', 2)),
].join('\n') + '\n',
);

await compactAutomationHistory(tempDir, 20, 1000);

const rewritten = readFileSync(join(tempDir, AUTOMATIONS_HISTORY_FILE), 'utf-8');
expect(rewritten).not.toContain('null');
expect(rewritten).not.toContain('42');
expect(rewritten).not.toContain('[]');
expect(rewritten.trim().split('\n')).toHaveLength(2);

const entries = readHistory(tempDir);
expect(entries.map((entry) => entry.ts)).toEqual([1, 2]);
});

it('should skip invalid recovered object segments while preserving valid glued records', async () => {
const first = makeEntry('a1', 1);
const second = makeEntry('a1', 2);
writeFileSync(
join(tempDir, AUTOMATIONS_HISTORY_FILE),
JSON.stringify(first) + '{"id":}' + JSON.stringify(second) + '\n',
);

await compactAutomationHistory(tempDir, 20, 1000);

const entries = readHistory(tempDir);
expect(entries.map((entry) => entry.ts)).toEqual([1, 2]);
});

it('should preserve braces inside glued record strings', async () => {
const first = { ...makeEntry('a1', 1), error: 'failed with {"x":1}' };
const second = { ...makeEntry('a1', 2), error: 'escaped quote: \\"' };
writeFileSync(
join(tempDir, AUTOMATIONS_HISTORY_FILE),
JSON.stringify(first) + JSON.stringify(second) + '\n',
);

await compactAutomationHistory(tempDir, 20, 1000);

const entries = readHistory(tempDir);
expect(entries).toHaveLength(2);
expect(entries[0]!.error).toBe(first.error);
expect(entries[1]!.error).toBe(second.error);
});

it('should no-op when file does not exist', async () => {
// Should not throw
await compactAutomationHistory(tempDir, 20, 1000);
Expand Down
100 changes: 93 additions & 7 deletions packages/desktop/packages/shared/src/automations/history-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,98 @@ 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;

Copy link
Copy Markdown
Collaborator

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

}
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() !== '') {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

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.
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 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.

Suggested change
}
} catch {
// Any sub-object failed — reject the entire glued line.
}
return [];

— qwen3.7-max via Qwen Code /review

@VectorPeak VectorPeak Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.
*/
Expand All @@ -159,13 +245,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
Expand Down Expand Up @@ -198,7 +284,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';
}
Loading