-
Notifications
You must be signed in to change notification settings - Fork 220
fix(write-to-file): address partial filesystem error review #1066
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
0575a35
75b52e3
0966556
16c4d48
be0e154
224690b
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 |
|---|---|---|
|
|
@@ -2857,6 +2857,78 @@ describe("Cline", () => { | |
| saveSpy.mockRestore() | ||
| }) | ||
|
|
||
| it("finalizePartialToolAsk persists and updates a non-last partial tool ask", async () => { | ||
| const updateSpy = vi | ||
| .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") | ||
| .mockResolvedValue(undefined) | ||
| const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) | ||
|
|
||
| const task = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| const partialToolAsk = { | ||
| ts: Date.now() - 2, | ||
| type: "ask" as const, | ||
| ask: "tool" as const, | ||
| text: "partial tool message", | ||
| partial: true, | ||
| } | ||
|
|
||
| task.clineMessages.push(partialToolAsk) | ||
| task.clineMessages.push({ | ||
| ts: Date.now() - 1, | ||
| type: "say", | ||
| say: "error", | ||
| text: "intervening async message", | ||
| }) | ||
|
|
||
| await task.finalizePartialToolAsk("partial tool message") | ||
| await flushMicrotasks() | ||
|
|
||
| expect(partialToolAsk.partial).toBe(false) | ||
| expect(saveSpy).toHaveBeenCalled() | ||
| expect(updateSpy).toHaveBeenCalledWith(partialToolAsk) | ||
|
Contributor
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. Vitest stores spy arguments as live references. Since let snap: Record<string, unknown> | undefined
updateSpy.mockImplementation(async (m) => { snap = { ...m } })
// ... then:
expect(snap?.partial).toBe(false) |
||
|
|
||
| updateSpy.mockRestore() | ||
| saveSpy.mockRestore() | ||
| }) | ||
|
|
||
| it("finalizePartialToolAsk ignores non-matching partial tool asks when text is provided", async () => { | ||
| const updateSpy = vi | ||
| .spyOn(getTaskTestAccess(Task.prototype), "updateClineMessage") | ||
| .mockResolvedValue(undefined) | ||
| const saveSpy = vi.spyOn(getTaskTestAccess(Task.prototype), "saveClineMessages").mockResolvedValue(true) | ||
|
|
||
| const task = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| task.clineMessages.push({ | ||
| ts: Date.now() - 1, | ||
| type: "ask", | ||
| ask: "tool", | ||
| text: "other partial tool message", | ||
| partial: true, | ||
| }) | ||
|
|
||
| await task.finalizePartialToolAsk("target partial tool message") | ||
| await flushMicrotasks() | ||
|
|
||
| expect(task.clineMessages[0].partial).toBe(true) | ||
| expect(saveSpy).not.toHaveBeenCalled() | ||
| expect(updateSpy).not.toHaveBeenCalled() | ||
|
Contributor
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. Both tests here pass a text string. The |
||
|
|
||
| updateSpy.mockRestore() | ||
| saveSpy.mockRestore() | ||
| }) | ||
|
|
||
| it("logs (instead of crashing) when updateClineMessage rejects from the ask() ignore-partial path", async () => { | ||
| // Pins the .catch arm on the fire-and-forget updateClineMessage call | ||
| // in ask() when a new partial ask arrives while the previous partial | ||
|
|
||
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.
findLastis already exported from../../shared/array(same modulefindLastIndexis imported from on line 68). Would that be cleaner here?(Also add
findLastto the import on line 68.)