Skip to content

Commit 65946e7

Browse files
committed
feat: implement condense journal for non-destructive rewind
- Add journal data structure to track removed messages during condense - Write journal entries before overwriting API conversation history - Implement restoration logic for delete/edit operations - Add comprehensive test coverage for journal functionality - Fixes #8295: Rewind after manual condense now preserves full history
1 parent 13534cc commit 65946e7

File tree

4 files changed

+680
-2
lines changed

4 files changed

+680
-2
lines changed
Lines changed: 387 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,387 @@
1+
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"
2+
import * as fs from "fs/promises"
3+
import * as path from "path"
4+
import * as os from "os"
5+
import {
6+
type CondenseJournalEntry,
7+
type CondenseJournal,
8+
readJournal,
9+
writeJournal,
10+
appendJournalEntry,
11+
createJournalEntry,
12+
restoreMessagesForTimestamp,
13+
findRemovedMessages,
14+
getJournalPath,
15+
} from "../journal"
16+
import { type ApiMessage } from "../../task-persistence"
17+
18+
// Mock safeWriteJson
19+
vi.mock("../../../utils/safeWriteJson", () => ({
20+
safeWriteJson: vi.fn(async (filePath: string, data: any) => {
21+
await fs.writeFile(filePath, JSON.stringify(data, null, 2), "utf-8")
22+
}),
23+
}))
24+
25+
describe("Condense Journal", () => {
26+
let testDir: string
27+
28+
beforeEach(async () => {
29+
// Create a temporary test directory
30+
testDir = path.join(os.tmpdir(), `journal-test-${Date.now()}`)
31+
await fs.mkdir(testDir, { recursive: true })
32+
})
33+
34+
afterEach(async () => {
35+
// Clean up test directory
36+
try {
37+
await fs.rm(testDir, { recursive: true, force: true })
38+
} catch (error) {
39+
// Ignore cleanup errors
40+
}
41+
})
42+
43+
describe("getJournalPath", () => {
44+
it("should return the correct journal path", () => {
45+
const journalPath = getJournalPath(testDir)
46+
expect(journalPath).toBe(path.join(testDir, "condense_journal.json"))
47+
})
48+
})
49+
50+
describe("readJournal", () => {
51+
it("should return null when journal doesn't exist", async () => {
52+
const journal = await readJournal(testDir)
53+
expect(journal).toBeNull()
54+
})
55+
56+
it("should read existing journal", async () => {
57+
const testJournal: CondenseJournal = {
58+
version: 1,
59+
entries: [
60+
{
61+
removed: [{ role: "user", content: "test", ts: 1000 }],
62+
boundary: { firstKeptTs: 900, lastKeptTs: 1100, summaryTs: 1050 },
63+
createdAt: Date.now(),
64+
type: "manual",
65+
},
66+
],
67+
}
68+
69+
const journalPath = getJournalPath(testDir)
70+
await fs.writeFile(journalPath, JSON.stringify(testJournal), "utf-8")
71+
72+
const journal = await readJournal(testDir)
73+
expect(journal).toEqual(testJournal)
74+
})
75+
76+
it("should handle corrupted journal file gracefully", async () => {
77+
const journalPath = getJournalPath(testDir)
78+
await fs.writeFile(journalPath, "invalid json", "utf-8")
79+
80+
const journal = await readJournal(testDir)
81+
expect(journal).toBeNull()
82+
})
83+
})
84+
85+
describe("writeJournal", () => {
86+
it("should write journal to disk", async () => {
87+
const testJournal: CondenseJournal = {
88+
version: 1,
89+
entries: [
90+
{
91+
removed: [{ role: "assistant", content: "response", ts: 2000 }],
92+
boundary: { firstKeptTs: 1900, lastKeptTs: 2100 },
93+
createdAt: Date.now(),
94+
type: "auto",
95+
},
96+
],
97+
}
98+
99+
await writeJournal(testDir, testJournal)
100+
101+
const journalPath = getJournalPath(testDir)
102+
const content = await fs.readFile(journalPath, "utf-8")
103+
const savedJournal = JSON.parse(content)
104+
expect(savedJournal).toEqual(testJournal)
105+
})
106+
})
107+
108+
describe("appendJournalEntry", () => {
109+
it("should create new journal if none exists", async () => {
110+
const entry: CondenseJournalEntry = {
111+
removed: [{ role: "user", content: "test message", ts: 3000 }],
112+
boundary: { firstKeptTs: 2900, summaryTs: 3050 },
113+
createdAt: Date.now(),
114+
type: "manual",
115+
}
116+
117+
await appendJournalEntry(testDir, entry)
118+
119+
const journal = await readJournal(testDir)
120+
expect(journal).not.toBeNull()
121+
expect(journal?.version).toBe(1)
122+
expect(journal?.entries).toHaveLength(1)
123+
expect(journal?.entries[0]).toEqual(entry)
124+
})
125+
126+
it("should append to existing journal", async () => {
127+
const existingEntry: CondenseJournalEntry = {
128+
removed: [{ role: "user", content: "old message", ts: 1000 }],
129+
boundary: { firstKeptTs: 900 },
130+
createdAt: Date.now() - 10000,
131+
type: "manual",
132+
}
133+
134+
const newEntry: CondenseJournalEntry = {
135+
removed: [{ role: "assistant", content: "new message", ts: 2000 }],
136+
boundary: { lastKeptTs: 2100 },
137+
createdAt: Date.now(),
138+
type: "auto",
139+
}
140+
141+
// Create initial journal
142+
await writeJournal(testDir, { version: 1, entries: [existingEntry] })
143+
144+
// Append new entry
145+
await appendJournalEntry(testDir, newEntry)
146+
147+
const journal = await readJournal(testDir)
148+
expect(journal?.entries).toHaveLength(2)
149+
expect(journal?.entries[0]).toEqual(existingEntry)
150+
expect(journal?.entries[1]).toEqual(newEntry)
151+
})
152+
})
153+
154+
describe("createJournalEntry", () => {
155+
it("should create journal entry with all fields", () => {
156+
const removed: ApiMessage[] = [
157+
{ role: "user", content: "message 1", ts: 1000 },
158+
{ role: "assistant", content: "message 2", ts: 1100 },
159+
]
160+
const firstKept: ApiMessage = { role: "user", content: "first kept", ts: 900 }
161+
const lastKept: ApiMessage = { role: "assistant", content: "last kept", ts: 1200 }
162+
const summary: ApiMessage = { role: "assistant", content: "summary", ts: 1150, isSummary: true }
163+
164+
const entry = createJournalEntry(removed, firstKept, lastKept, summary, "manual")
165+
166+
expect(entry.removed).toEqual(removed)
167+
expect(entry.boundary.firstKeptTs).toBe(900)
168+
expect(entry.boundary.lastKeptTs).toBe(1200)
169+
expect(entry.boundary.summaryTs).toBe(1150)
170+
expect(entry.type).toBe("manual")
171+
expect(entry.createdAt).toBeGreaterThan(0)
172+
})
173+
174+
it("should handle undefined boundary messages", () => {
175+
const removed: ApiMessage[] = [{ role: "user", content: "message", ts: 1000 }]
176+
177+
const entry = createJournalEntry(removed, undefined, undefined, undefined, "auto")
178+
179+
expect(entry.removed).toEqual(removed)
180+
expect(entry.boundary.firstKeptTs).toBeUndefined()
181+
expect(entry.boundary.lastKeptTs).toBeUndefined()
182+
expect(entry.boundary.summaryTs).toBeUndefined()
183+
expect(entry.type).toBe("auto")
184+
})
185+
})
186+
187+
describe("findRemovedMessages", () => {
188+
it("should identify removed messages correctly", () => {
189+
const original: ApiMessage[] = [
190+
{ role: "user", content: "msg1", ts: 1000 },
191+
{ role: "assistant", content: "msg2", ts: 1100 },
192+
{ role: "user", content: "msg3", ts: 1200 },
193+
{ role: "assistant", content: "msg4", ts: 1300 },
194+
]
195+
196+
const condensed: ApiMessage[] = [
197+
{ role: "user", content: "msg1", ts: 1000 },
198+
{ role: "assistant", content: "summary", ts: 1150, isSummary: true },
199+
{ role: "assistant", content: "msg4", ts: 1300 },
200+
]
201+
202+
const removed = findRemovedMessages(original, condensed)
203+
204+
expect(removed).toHaveLength(2)
205+
expect(removed[0]).toEqual({ role: "assistant", content: "msg2", ts: 1100 })
206+
expect(removed[1]).toEqual({ role: "user", content: "msg3", ts: 1200 })
207+
})
208+
209+
it("should handle messages without timestamps", () => {
210+
const original: ApiMessage[] = [
211+
{ role: "user", content: "msg1", ts: 1000 },
212+
{ role: "assistant", content: "no timestamp" }, // No ts field
213+
{ role: "user", content: "msg3", ts: 1200 },
214+
]
215+
216+
const condensed: ApiMessage[] = [
217+
{ role: "user", content: "msg1", ts: 1000 },
218+
{ role: "user", content: "msg3", ts: 1200 },
219+
]
220+
221+
const removed = findRemovedMessages(original, condensed)
222+
223+
expect(removed).toHaveLength(0) // Message without timestamp is not included
224+
})
225+
})
226+
227+
describe("restoreMessagesForTimestamp", () => {
228+
it("should return null if target timestamp already exists", async () => {
229+
const currentMessages: ApiMessage[] = [
230+
{ role: "user", content: "msg1", ts: 1000 },
231+
{ role: "assistant", content: "msg2", ts: 1100 },
232+
]
233+
234+
const result = await restoreMessagesForTimestamp(testDir, currentMessages, 1100)
235+
expect(result).toBeNull()
236+
})
237+
238+
it("should return null if no journal exists", async () => {
239+
const currentMessages: ApiMessage[] = [{ role: "user", content: "msg1", ts: 1000 }]
240+
241+
const result = await restoreMessagesForTimestamp(testDir, currentMessages, 2000)
242+
expect(result).toBeNull()
243+
})
244+
245+
it("should restore messages from single journal entry", async () => {
246+
const currentMessages: ApiMessage[] = [
247+
{ role: "user", content: "msg1", ts: 1000 },
248+
{ role: "assistant", content: "summary", ts: 1500, isSummary: true },
249+
{ role: "user", content: "msg5", ts: 1600 },
250+
]
251+
252+
const journal: CondenseJournal = {
253+
version: 1,
254+
entries: [
255+
{
256+
removed: [
257+
{ role: "assistant", content: "msg2", ts: 1100 },
258+
{ role: "user", content: "msg3", ts: 1200 },
259+
{ role: "assistant", content: "msg4", ts: 1300 },
260+
],
261+
boundary: { firstKeptTs: 1000, lastKeptTs: 1600, summaryTs: 1500 },
262+
createdAt: Date.now(),
263+
type: "manual",
264+
},
265+
],
266+
}
267+
268+
await writeJournal(testDir, journal)
269+
270+
const result = await restoreMessagesForTimestamp(testDir, currentMessages, 1200)
271+
272+
expect(result).not.toBeNull()
273+
expect(result).toHaveLength(6) // 3 current + 3 restored
274+
expect(result?.find((m) => m.ts === 1200)).toBeDefined()
275+
expect(result?.find((m) => m.ts === 1100)).toBeDefined()
276+
expect(result?.find((m) => m.ts === 1300)).toBeDefined()
277+
// Should be sorted by timestamp
278+
expect(result?.[0].ts).toBe(1000)
279+
expect(result?.[1].ts).toBe(1100)
280+
expect(result?.[2].ts).toBe(1200)
281+
expect(result?.[3].ts).toBe(1300)
282+
expect(result?.[4].ts).toBe(1500)
283+
expect(result?.[5].ts).toBe(1600)
284+
})
285+
286+
it("should handle nested condenses correctly", async () => {
287+
const currentMessages: ApiMessage[] = [
288+
{ role: "user", content: "msg1", ts: 1000 },
289+
{ role: "assistant", content: "summary2", ts: 2500, isSummary: true },
290+
{ role: "user", content: "msg9", ts: 2600 },
291+
]
292+
293+
const journal: CondenseJournal = {
294+
version: 1,
295+
entries: [
296+
// First condense
297+
{
298+
removed: [
299+
{ role: "assistant", content: "msg2", ts: 1100 },
300+
{ role: "user", content: "msg3", ts: 1200 },
301+
],
302+
boundary: { firstKeptTs: 1000, summaryTs: 1500 },
303+
createdAt: Date.now() - 10000,
304+
type: "manual",
305+
},
306+
// Second condense (nested) - this wouldn't contain msg2 and msg3 again since they were already condensed
307+
{
308+
removed: [
309+
{ role: "assistant", content: "summary1", ts: 1500, isSummary: true },
310+
{ role: "user", content: "msg5", ts: 1600 },
311+
{ role: "assistant", content: "msg6", ts: 1700 },
312+
],
313+
boundary: { firstKeptTs: 1000, summaryTs: 2500 },
314+
createdAt: Date.now(),
315+
type: "manual",
316+
},
317+
],
318+
}
319+
320+
await writeJournal(testDir, journal)
321+
322+
// Try to restore a message from the first condensed range
323+
const result = await restoreMessagesForTimestamp(testDir, currentMessages, 1100)
324+
325+
expect(result).not.toBeNull()
326+
// Should restore messages that contain the target timestamp
327+
expect(result?.find((m) => m.ts === 1100)).toBeDefined()
328+
expect(result?.find((m) => m.ts === 1200)).toBeDefined()
329+
// The restoration logic only restores messages needed to reach the target timestamp
330+
// It doesn't necessarily restore all messages from all entries
331+
})
332+
333+
it("should not restore messages that are already in current messages", async () => {
334+
const currentMessages: ApiMessage[] = [
335+
{ role: "user", content: "msg1", ts: 1000 },
336+
{ role: "assistant", content: "msg2", ts: 1100 }, // Already present
337+
{ role: "assistant", content: "summary", ts: 1500, isSummary: true },
338+
]
339+
340+
const journal: CondenseJournal = {
341+
version: 1,
342+
entries: [
343+
{
344+
removed: [
345+
{ role: "assistant", content: "msg2", ts: 1100 }, // Duplicate
346+
{ role: "user", content: "msg3", ts: 1200 },
347+
],
348+
boundary: {},
349+
createdAt: Date.now(),
350+
type: "manual",
351+
},
352+
],
353+
}
354+
355+
await writeJournal(testDir, journal)
356+
357+
const result = await restoreMessagesForTimestamp(testDir, currentMessages, 1200)
358+
359+
expect(result).not.toBeNull()
360+
expect(result).toHaveLength(4) // 3 current + 1 restored (msg3)
361+
// Should only have one msg2
362+
expect(result?.filter((m) => m.ts === 1100)).toHaveLength(1)
363+
})
364+
365+
it("should return null if target timestamp not found in journal", async () => {
366+
const currentMessages: ApiMessage[] = [{ role: "user", content: "msg1", ts: 1000 }]
367+
368+
const journal: CondenseJournal = {
369+
version: 1,
370+
entries: [
371+
{
372+
removed: [{ role: "assistant", content: "msg2", ts: 1100 }],
373+
boundary: {},
374+
createdAt: Date.now(),
375+
type: "manual",
376+
},
377+
],
378+
}
379+
380+
await writeJournal(testDir, journal)
381+
382+
// Try to restore a timestamp that doesn't exist in journal
383+
const result = await restoreMessagesForTimestamp(testDir, currentMessages, 9999)
384+
expect(result).toBeNull()
385+
})
386+
})
387+
})

0 commit comments

Comments
 (0)