Skip to content

Conversation

@elianiva
Copy link
Contributor

@elianiva elianiva commented Oct 6, 2025

temporary, this is for Hannes to try out

Copy link
Contributor

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

I found some issues that need attention. See inline comments.

try {
const taskHistoryUri = vscode.Uri.joinPath(this.globalStorageUri, "taskHistory.jsonl")
const fileContent = await vscode.workspace.fs.readFile(taskHistoryUri)
const lines = fileContent.toString().split("\n").filter(Boolean)
Copy link
Contributor

Choose a reason for hiding this comment

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

P0: fileContent is a Uint8Array; calling toString() returns "[object Uint8Array]" in Node, breaking JSONL parsing and causing an empty history. Decode the buffer first.

Suggested change
const lines = fileContent.toString().split("\n").filter(Boolean)
const lines = new TextDecoder("utf-8").decode(fileContent).split("\n").filter(Boolean)

try {
const taskHistoryUri = vscode.Uri.joinPath(this.globalStorageUri, "taskHistory.jsonl")
const content = tasks.map((task) => JSON.stringify(task)).join("\n") + "\n"
await vscode.workspace.fs.writeFile(taskHistoryUri, Buffer.from(content))
Copy link
Contributor

Choose a reason for hiding this comment

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

P2: Atomicity/consistency: writing directly to the final file risks partial writes and bypasses our atomic write conventions. If we keep JSONL, write to a temp file then rename to the target to approximate an atomic replace.

Suggested change
await vscode.workspace.fs.writeFile(taskHistoryUri, Buffer.from(content))
const tmpUri = vscode.Uri.joinPath(this.globalStorageUri, "taskHistory.jsonl.tmp")
await vscode.workspace.fs.writeFile(tmpUri, Buffer.from(content))
await vscode.workspace.fs.rename(tmpUri, taskHistoryUri, { overwrite: true })

},
workspace: {
fs: {
readFile: vi.fn().mockRejectedValue(Object.assign(new Error("FileNotFound"), { code: "FileNotFound" })),
Copy link
Contributor

Choose a reason for hiding this comment

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

P2: Missing positive-path coverage: readFile is always mocked to reject, so readTaskHistoryFile's success path (decoding and parsing JSONL) is never exercised. Add a test case that resolves readFile with a valid Uint8Array (e.g., new TextEncoder().encode('{"id":"1"}\n')) and assert that taskHistory is populated.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Oct 6, 2025
@elianiva elianiva closed this Oct 17, 2025
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Oct 17, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Oct 17, 2025
@elianiva elianiva deleted the fix/task-history-storage branch October 17, 2025 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants