Restore PR #5332 #8167

hannesrudolph · 2025-09-18T21:03:58Z

Restores commits from #5332 after the source branch was deleted. Preserves original commit authorship via refs/pull/5332 fetch. Supersedes #5332. Original PR: #5332

Important

Restores changes from a deleted branch, introducing safeReadJson for atomic JSON file reading and updating codebase and tests to use this utility.

Behavior:
- Introduces safeReadJson in safeReadJson.ts for atomic and safe JSON file reading using streams and locks.
- Replaces direct JSON reads with safeReadJson in modelCache.ts, modelEndpointCache.ts, importExport.ts, FileContextTracker.ts, apiMessages.ts, taskMessages.ts, ClineProvider.ts, extract-text.ts, MarketplaceManager.ts, SimpleInstaller.ts, McpHub.ts, MdmService.ts, and autoImportSettings.spec.ts.
- Handles JSON parsing errors and file access issues gracefully.
Tests:
- Updates tests in cache-manager.spec.ts, SimpleInstaller.spec.ts, McpHub.spec.ts, MdmService.spec.ts, autoImportSettings.spec.ts, and safeReadJson.spec.ts to mock and test safeReadJson.
- Adds comprehensive tests for safeReadJson in safeReadJson.spec.ts to cover various scenarios including errors and edge cases.
Misc:
- Adds .roo/rules/use-safeReadJson.md to enforce the use of safeReadJson for JSON file reading.

^{This description was created by}^{for b0ec3f4. You can customize this summary. It will automatically update as commits are pushed.}

RooCodeInc#5332

Implement safeReadJson function to complement the existing safeWriteJson functionality: - Uses stream-json for efficient processing of large JSON files - Supports both full object reading and selective path extraction - Provides file locking to prevent concurrent access - Includes comprehensive error handling - Adds complete test coverage - Passthrough all exceptions This enables efficient and safe JSON reading operations throughout the codebase. Signed-off-by: Eric Wheeler <[email protected]>

Replace manual file reading and JSON parsing with the safer safeReadJson utility across multiple files in the codebase. This change: - Provides atomic file access with proper locking to prevent race conditions - Streams file contents efficiently for better memory usage - Improves error handling consistency - Reduces code duplication Fixes: RooCodeInc#5331 Signed-off-by: Eric Wheeler <[email protected]>

Updated test files to properly mock and use safeReadJson/safeWriteJson: - Added proper imports for safeReadJson from safeWriteJson module - Updated mock implementations to mock both functions correctly - Replaced direct fs operations with calls to safe functions - Updated assertions to match the new behavior This fixes all failing tests after the conversion to safeReadJson. Signed-off-by: Eric Wheeler <[email protected]>

Copilot

Pull Request Overview

This PR restores commits from a deleted branch (PR #5332) and implements the use of safe JSON file reading across the codebase. The main change introduces a new safeReadJson utility function to replace unsafe file reading patterns and ensures consistent error handling throughout the application.

Introduction of safeReadJson utility for atomic JSON file reading with proper locking
Refactoring of the _acquireLock function from safeWriteJson to be reusable
Comprehensive replacement of unsafe fs.readFile + JSON.parse patterns across the codebase

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/utils/safeWriteJson.ts	Extracts `_acquireLock` function for reuse and refactors existing code
src/utils/safeReadJson.ts	New utility providing atomic JSON reading with streaming and locking
src/utils/tests/safeReadJson.spec.ts	Comprehensive test suite for the new utility
.roo/rules/use-safeReadJson.md	Documentation rule requiring use of `safeReadJson`
Multiple files	Replacement of unsafe JSON reading patterns with `safeReadJson`

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-18T21:04:35Z

src/services/mdm/__tests__/MdmService.spec.ts

+
+		// Reset safeReadJson to reject with ENOENT by default (no MDM config)
+		vi.mocked(safeReadJson).mockClear()
+		vi.mocked(safeReadJson).mockRejectedValue({ code: "ENOENT" })


The mock is rejecting with a plain object { code: "ENOENT" } instead of an Error object. This should be mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" })) to properly simulate a file system error.

Suggested change

vi.mocked(safeReadJson).mockRejectedValue({ code: "ENOENT" })

vi.mocked(safeReadJson).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" }))

Copilot · 2025-09-18T21:04:36Z

src/services/code-index/cache-manager.ts

 		try {
-			const cacheData = await vscode.workspace.fs.readFile(this.cachePath)
-			this.fileHashes = JSON.parse(cacheData.toString())
+			this.fileHashes = await safeReadJson(this.cachePath.fsPath)


This code expects this.cachePath to have a fsPath property (suggesting it's a vscode.Uri), but safeReadJson expects a string path. Consider using this.cachePath.fsPath only if this.cachePath is indeed a Uri, or use this.cachePath directly if it's already a string.

roomote

Thank you for restoring PR #5332! I've reviewed the implementation of safeReadJson and found it addresses the memory usage issues well. However, there are some critical issues that need attention before merging.

Critical Issues:

The PR has merge conflicts that must be resolved
The test mock issue noted by Copilot is still present

Additional findings:

Documentation inconsistency with the jsonPath parameter
Inconsistent error handling patterns across files
An unused import that should be cleaned up

roomote · 2025-09-18T21:08:43Z

src/services/mdm/__tests__/MdmService.spec.ts

+
+		// Reset safeReadJson to reject with ENOENT by default (no MDM config)
+		vi.mocked(safeReadJson).mockClear()
+		vi.mocked(safeReadJson).mockRejectedValue({ code: "ENOENT" })


This issue from Copilot's review is still valid. The mock is rejecting with a plain object { code: "ENOENT" } instead of an Error object. Consider:

Suggested change

vi.mocked(safeReadJson).mockRejectedValue({ code: "ENOENT" })

vi.mocked(safeReadJson).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" }))

roomote · 2025-09-18T21:08:44Z

.roo/rules/use-safeReadJson.md

@@ -0,0 +1,33 @@
+# JSON File Reading Must Be Safe and Atomic
+
+- You MUST use `safeReadJson(filePath: string, jsonPath?: string | string[]): Promise<any>` from `src/utils/safeReadJson.ts` to read JSON files


The documentation mentions jsonPath?: string | string[] parameter but the actual implementation of safeReadJson doesn't support this parameter. Either update the documentation to match the implementation or add jsonPath support to the function.

roomote · 2025-09-18T21:08:44Z

src/utils/safeReadJson.ts

+import * as fsSync from "fs"
+import * as path from "path"
+import * as Parser from "stream-json/Parser"
+import * as Pick from "stream-json/filters/Pick"


The Pick import from stream-json is not used anywhere in this file. Consider removing it:

Suggested change

import * as Pick from "stream-json/filters/Pick"

import * as Parser from "stream-json/Parser"

import * as StreamValues from "stream-json/streamers/StreamValues"

roomote · 2025-09-18T21:08:44Z

src/services/mcp/McpHub.ts

-		const content = await fs.readFile(configPath, "utf-8")
-		const config = JSON.parse(content)
+		// This is a read-modify-write-operation, but we cannot
+		// use safeWriteJson because it does not (yet) support pretty printing.


Multiple comments mention "cannot use safeWriteJson because it does not (yet) support pretty printing". Should this be tracked as a feature request? Pretty printing support would eliminate these read-modify-write operations that bypass the safe write mechanism.

roomote · 2025-09-18T21:08:44Z

src/api/providers/fetchers/modelEndpointCache.ts

-	const exists = await fileExistsAtPath(filePath)
-	return exists ? JSON.parse(await fs.readFile(filePath, "utf8")) : undefined
+	try {
+		return await safeReadJson(filePath)


Is it intentional to silently return undefined for all errors, not just ENOENT? This could hide actual issues like permission errors or corrupted JSON. Consider:

Suggested change

return await safeReadJson(filePath)

try {

return await safeReadJson(filePath)

} catch (error: any) {

if (error.code === "ENOENT") {

return undefined

}

throw error

}

Eric Wheeler added 4 commits July 18, 2025 13:47

NOTICE: PR 5332 STARTS HERE

c9e13b0

RooCodeInc#5332

Copilot AI review requested due to automatic review settings September 18, 2025 21:03

hannesrudolph requested review from cte, jr and mrubens as code owners September 18, 2025 21:03

github-project-automation bot added this to Roo Code Roadmap and Roo Code Roadmap Sep 18, 2025

github-project-automation bot moved this to New in Roo Code Roadmap Sep 18, 2025

github-project-automation bot moved this to Triage in Roo Code Roadmap Sep 18, 2025

dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Sep 18, 2025

Copilot AI reviewed Sep 18, 2025

View reviewed changes

roomote bot reviewed Sep 18, 2025

View reviewed changes

hannesrudolph self-assigned this Sep 18, 2025

hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Sep 18, 2025

hannesrudolph closed this Sep 18, 2025

github-project-automation bot moved this from New to Done in Roo Code Roadmap Sep 18, 2025

github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Sep 18, 2025

hannesrudolph deleted the restore/pr-5332 branch September 18, 2025 23:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Restore PR #5332 #8167

Restore PR #5332 #8167

Uh oh!

hannesrudolph commented Sep 18, 2025 •

edited by ellipsis-dev bot

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 18, 2025

Uh oh!

Copilot AI Sep 18, 2025

Uh oh!

roomote bot left a comment

Uh oh!

roomote bot Sep 18, 2025

Uh oh!

roomote bot Sep 18, 2025

Uh oh!

roomote bot Sep 18, 2025

Uh oh!

roomote bot Sep 18, 2025

Uh oh!

roomote bot Sep 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	vi.mocked(safeReadJson).mockRejectedValue({ code: "ENOENT" })
	vi.mocked(safeReadJson).mockRejectedValue(Object.assign(new Error("ENOENT"), { code: "ENOENT" }))

		@@ -0,0 +1,33 @@
		# JSON File Reading Must Be Safe and Atomic

		- You MUST use `safeReadJson(filePath: string, jsonPath?: string \| string[]): Promise<any>` from `src/utils/safeReadJson.ts` to read JSON files

	import * as Pick from "stream-json/filters/Pick"
	import * as Parser from "stream-json/Parser"
	import * as StreamValues from "stream-json/streamers/StreamValues"

-		return await safeReadJson(filePath)
+	try {
+		return await safeReadJson(filePath)
+	} catch (error: any) {
+		if (error.code === "ENOENT") {
+			return undefined
+		}
+		throw error
+	}

Restore PR #5332 #8167

Restore PR #5332 #8167

Uh oh!

Conversation

hannesrudolph commented Sep 18, 2025 • edited by ellipsis-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

roomote bot left a comment

Choose a reason for hiding this comment

Uh oh!

roomote bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

roomote bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

roomote bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

roomote bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

roomote bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

hannesrudolph commented Sep 18, 2025 •

edited by ellipsis-dev bot

Loading