Restore PR #5332 - 2 #8168

hannesrudolph · 2025-09-18T21:54:24Z

Important

Introduce safeReadJson for safe JSON file reading and update codebase to use it, enhancing file access safety and performance.

Behavior:
- Introduces safeReadJson in safeReadJson.ts for atomic and safe JSON file reading using streaming and locking.
- 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.
Tests:
- Adds safeReadJson.spec.ts to test safeReadJson functionality.
- Updates tests in SimpleInstaller.spec.ts, McpHub.spec.ts, MdmService.spec.ts, and autoImportSettings.spec.ts to mock safeReadJson.
Misc:
- Adds .roo/rules/use-safeReadJson.md to enforce the use of safeReadJson for JSON file reading.

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

#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: #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 the implementation of safeReadJson utility and integrates it throughout the codebase to provide atomic, streaming JSON file reading with proper locking. The changes replace manual fs.readFile + JSON.parse patterns with a unified, safer approach.

Implements safeReadJson function with streaming JSON parsing and file locking
Refactors all JSON file reading operations to use the new safe utility
Updates tests to mock the new utility instead of direct file system operations

Reviewed Changes

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

Show a summary per file

File	Description
src/utils/safeReadJson.ts	New safe JSON reading utility with streaming and locking
src/utils/safeWriteJson.ts	Extracts lock acquisition to shared function
src/utils/tests/safeReadJson.spec.ts	Comprehensive test suite for the new utility
Multiple service files	Replace manual JSON reading with safeReadJson
Multiple test files	Update mocks to use new utility
.roo/rules/use-safeReadJson.md	Documentation for the new pattern

Comments suppressed due to low confidence (1)

src/services/mdm/tests/MdmService.spec.ts:1

The property access error.code should be properly typed. The error parameter should be cast to the appropriate type or checked with hasOwnProperty before accessing the code property.

import * as path from "path"

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

Copilot · 2025-09-18T21:55:19Z

src/core/task-persistence/apiMessages.ts

+					`[Roo-Debug] readApiMessages: Error reading OLD API conversation history file (claude_messages.json). TaskId: ${taskId}, Path: ${oldPath}, Error: ${oldError}`,
 				)
-				// DO NOT unlink oldPath if parsing failed, throw error instead.
-				throw error
+				throw oldError
 			}
+		} else {
+			// For any other error with the main file, log and rethrow
+			console.error(
+				`[Roo-Debug] readApiMessages: Error reading API conversation history file. TaskId: ${taskId}, Path: ${filePath}, Error: ${error}`,


The error message should include the actual error message from the caught error rather than just ${error}, which may not provide meaningful information. Consider using error.message or similar.

Copilot · 2025-09-18T21:55:19Z

src/core/task-persistence/apiMessages.ts

+				// For any other error with the old file, log and rethrow
 				console.error(
-					`[Roo-Debug] readApiMessages: Error parsing OLD API conversation history file (claude_messages.json). TaskId: ${taskId}, Path: ${oldPath}, Error: ${error}`,
+					`[Roo-Debug] readApiMessages: Error reading OLD API conversation history file (claude_messages.json). TaskId: ${taskId}, Path: ${oldPath}, Error: ${oldError}`,


The error message should include the actual error message from the caught error rather than just ${oldError}, which may not provide meaningful information. Consider using oldError.message or similar.

Suggested change

`[Roo-Debug] readApiMessages: Error reading OLD API conversation history file (claude_messages.json). TaskId: ${taskId}, Path: ${oldPath}, Error: ${oldError}`,

`[Roo-Debug] readApiMessages: Error reading OLD API conversation history file (claude_messages.json). TaskId: ${taskId}, Path: ${oldPath}, Error: ${oldError.message}`,

Copilot · 2025-09-18T21:55:20Z

src/core/webview/ClineProvider.ts

 					apiConversationHistory,
 				}
+			} catch (error) {
+				if (error.code !== "ENOENT") {


The property access error.code should be properly typed. The error parameter should be cast to the appropriate type (e.g., error as any or error as NodeJS.ErrnoException) before accessing the code property.

Suggested change

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

if ((error as NodeJS.ErrnoException).code !== "ENOENT") {

roomote

Thank you for your contribution! I've reviewed the changes and found several issues that need attention. The implementation of safeReadJson is well-structured with proper streaming and locking, but there are some type safety and consistency concerns that should be addressed.

roomote · 2025-09-18T21:58:39Z

src/services/mdm/MdmService.ts

 		} catch (error) {
-			this.log(`[MDM] Error reading MDM config from ${configPath}:`, error)
+			// If file doesn't exist, return null
+			if ((error as any)?.code !== "ENOENT") {


Type safety issue: Using (error as any)?.code defeats TypeScript's type safety. Consider using a proper type guard instead:

Suggested change

if ((error as any)?.code !== "ENOENT") {

if (error && typeof error === 'object' && 'code' in error && (error as NodeJS.ErrnoException).code !== "ENOENT") {

This ensures type safety while checking for the ENOENT error code.

roomote · 2025-09-18T21:58:39Z

.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


Documentation mismatch: The function signature mentions jsonPath?: string | string[] parameter, but the actual implementation in safeReadJson.ts doesn't have this parameter. Could you update the documentation to match the implementation?

roomote · 2025-09-18T21:58:39Z

src/api/providers/fetchers/modelEndpointCache.ts

-	return exists ? JSON.parse(await fs.readFile(filePath, "utf8")) : undefined
+	try {
+		return await safeReadJson(filePath)
+	} catch (error) {


Is this intentional? The error is silently caught and returns undefined without any logging. Consider at least logging the error for debugging purposes, or add a comment explaining why all errors should be silently ignored here.

roomote · 2025-09-18T21:58:39Z

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"


Unused import: The Pick import from 'stream-json/filters/Pick' is not used anywhere in this file. Could we remove it?

roomote · 2025-09-18T21:58:39Z

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.


Technical debt indicator: Multiple comments mention "cannot use safeWriteJson because it does not (yet) support pretty printing". Should we create an issue to track adding pretty printing support to safeWriteJson? This would eliminate these workarounds.

roomote · 2025-09-18T21:58:39Z

src/utils/__tests__/safeReadJson.spec.ts

+	}
+
+	// Success Scenarios
+	test("should successfully read a JSON file", async () => {


Good test coverage! Consider adding a test for concurrent read scenarios to verify that the locking mechanism properly handles multiple simultaneous reads of the same file. This would ensure the atomic read guarantee is working as expected.

Eric Wheeler added 4 commits September 18, 2025 15:08

NOTICE: PR 5332 STARTS HERE

fe35442

#5332

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

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

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

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

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 added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Sep 18, 2025

hannesrudolph closed this Sep 19, 2025

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Restore PR #5332 - 2 #8168

Restore PR #5332 - 2 #8168

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!

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!

roomote bot Sep 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	`[Roo-Debug] readApiMessages: Error reading OLD API conversation history file (claude_messages.json). TaskId: ${taskId}, Path: ${oldPath}, Error: ${oldError}`,
	`[Roo-Debug] readApiMessages: Error reading OLD API conversation history file (claude_messages.json). TaskId: ${taskId}, Path: ${oldPath}, Error: ${oldError.message}`,

	if (error.code !== "ENOENT") {
	if ((error as NodeJS.ErrnoException).code !== "ENOENT") {

	if ((error as any)?.code !== "ENOENT") {
	if (error && typeof error === 'object' && 'code' in error && (error as NodeJS.ErrnoException).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

Restore PR #5332 - 2 #8168

Restore PR #5332 - 2 #8168

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!

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!

roomote bot Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hannesrudolph commented Sep 18, 2025 •

edited by ellipsis-dev bot

Loading