Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,45 @@ describe('history-store', () => {
expect(entries).toHaveLength(3);
});

it('should preserve glued object records while dropping unrecoverable lines', async () => {

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

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

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.

const gluedLine =
JSON.stringify(makeEntry('a1', 2)) +
JSON.stringify(makeEntry('a1', 3));
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]);
});

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
98 changes: 91 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,96 @@ 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 [];
}
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 [];

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.

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

Suggested change
return [];
} else if (depth < 0) {
return recovered;
}

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

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.

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] Two defensive branches lack dedicated test coverage:

  1. The depth < 0 branch (line 186) — no test exercises a stray } after valid objects on a glued line.
  2. The isValidHistoryObject false path in parseHistoryLine's try-block — while null/42/[] lines appear in the "nested and 3+ glued" test, they only assert final entry count, not that these values trigger file rewrite via needsRewrite.

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

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

}
} 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 [];

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

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

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.

}
}

if (depth !== 0 || inString || line.slice(cursor).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 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 recovered

The 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

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.

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.

return [];
}

return recovered.length > 1 ? recovered : [];
}

function parseHistoryLine(line: string): Array<{ raw: string; id: string }> {
try {
const parsed = JSON.parse(line);
if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) {
return [{ raw: line, id: parsed.id ?? '' }];
}
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 (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) {
entries.push({ raw, id: parsed.id ?? '' });
continue;
}
} catch {
// Fall through to drop the whole glued line below.
}
return [];
}

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 +243,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 +282,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