Skip to content

feat: Added file encoding detection and reading functions #6841

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: Added file encoding detection and reading functions #6841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f5ef6b0
feat: Added file encoding detection and reading functions
aheizi Aug 8, 2025
c7c0018
Refactor: use writeFileWithEncodingPreservation save the file to save…
aheizi Aug 12, 2025
cd41603
refactor: Reconstruct the file encoding detection logic to first atte…
aheizi Aug 15, 2025
ef0a5fa
feat(encoding): improve file encoding detection and error handling
aheizi Sep 26, 2025
48a1c62
fix(File Reading): Fixed the issue where the actual number of bytes r…
aheizi Nov 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/core/mentions/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import fs from "fs/promises"
import * as path from "path"

import * as vscode from "vscode"
import { isBinaryFile } from "isbinaryfile"
import { isBinaryFileWithEncodingDetection } from "../../utils/encoding"

import { mentionRegexGlobal, commandRegexGlobal, unescapeSpaces } from "../../shared/context-mentions"

Expand Down Expand Up @@ -314,7 +314,7 @@ async function getFileOrFolderContent(
fileContentPromises.push(
(async () => {
try {
const isBinary = await isBinaryFile(absoluteFilePath).catch(() => false)
const isBinary = await isBinaryFileWithEncodingDetection(absoluteFilePath)
if (isBinary) {
return undefined
}
Expand Down
50 changes: 26 additions & 24 deletions src/core/tools/__tests__/readFileTool.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { countFileLines } from "../../../integrations/misc/line-counter"
import { readLines } from "../../../integrations/misc/read-lines"
import { extractTextFromFile } from "../../../integrations/misc/extract-text"
import { parseSourceCodeDefinitionsForFile } from "../../../services/tree-sitter"
import { isBinaryFile } from "isbinaryfile"
import { isBinaryFileWithEncodingDetection } from "../../../utils/encoding"
import { ReadFileToolUse, ToolParamName, ToolResponse } from "../../../shared/tools"
import { readFileTool } from "../readFileTool"
import { formatResponse } from "../../prompts/responses"
Expand All @@ -23,7 +23,9 @@ vi.mock("path", async () => {

// Already mocked above with hoisted fsPromises

vi.mock("isbinaryfile")
vi.mock("../../../utils/encoding", () => ({
isBinaryFileWithEncodingDetection: vi.fn(),
}))

vi.mock("../../../integrations/misc/line-counter")
vi.mock("../../../integrations/misc/read-lines")
Expand Down Expand Up @@ -238,7 +240,7 @@ describe("read_file tool with maxReadFileLine setting", () => {
const mockedExtractTextFromFile = vi.mocked(extractTextFromFile)
const mockedParseSourceCodeDefinitionsForFile = vi.mocked(parseSourceCodeDefinitionsForFile)

const mockedIsBinaryFile = vi.mocked(isBinaryFile)
const mockedIsBinaryFileWithEncodingDetection = vi.mocked(isBinaryFileWithEncodingDetection)
const mockedPathResolve = vi.mocked(path.resolve)

let mockCline: any
Expand All @@ -249,7 +251,7 @@ describe("read_file tool with maxReadFileLine setting", () => {
// Clear specific mocks (not all mocks to preserve shared state)
mockedCountFileLines.mockClear()
mockedExtractTextFromFile.mockClear()
mockedIsBinaryFile.mockClear()
mockedIsBinaryFileWithEncodingDetection.mockClear()
mockedPathResolve.mockClear()
addLineNumbersMock.mockClear()
extractTextFromFileMock.mockClear()
Expand All @@ -264,7 +266,7 @@ describe("read_file tool with maxReadFileLine setting", () => {
setImageSupport(mockCline, false)

mockedPathResolve.mockReturnValue(absoluteFilePath)
mockedIsBinaryFile.mockResolvedValue(false)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(false)

mockInputContent = fileContent

Expand Down Expand Up @@ -502,7 +504,7 @@ describe("read_file tool with maxReadFileLine setting", () => {
describe("when file is binary", () => {
it("should always use extractTextFromFile regardless of maxReadFileLine", async () => {
// Setup
mockedIsBinaryFile.mockResolvedValue(true)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(true)
mockedCountFileLines.mockResolvedValue(3)
mockedExtractTextFromFile.mockResolvedValue("")

Expand Down Expand Up @@ -544,7 +546,7 @@ describe("read_file tool XML output structure", () => {

const mockedCountFileLines = vi.mocked(countFileLines)
const mockedExtractTextFromFile = vi.mocked(extractTextFromFile)
const mockedIsBinaryFile = vi.mocked(isBinaryFile)
const mockedIsBinaryFileWithEncodingDetection = vi.mocked(isBinaryFileWithEncodingDetection)
const mockedPathResolve = vi.mocked(path.resolve)
const mockedFsReadFile = vi.mocked(fsPromises.readFile)
const imageBuffer = Buffer.from(
Expand All @@ -560,7 +562,7 @@ describe("read_file tool XML output structure", () => {
// Clear specific mocks (not all mocks to preserve shared state)
mockedCountFileLines.mockClear()
mockedExtractTextFromFile.mockClear()
mockedIsBinaryFile.mockClear()
mockedIsBinaryFileWithEncodingDetection.mockClear()
mockedPathResolve.mockClear()
addLineNumbersMock.mockClear()
extractTextFromFileMock.mockClear()
Expand All @@ -580,7 +582,7 @@ describe("read_file tool XML output structure", () => {
setImageSupport(mockCline, true)

mockedPathResolve.mockReturnValue(absoluteFilePath)
mockedIsBinaryFile.mockResolvedValue(false)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(false)

// Set default implementation for extractTextFromFile
mockedExtractTextFromFile.mockImplementation((filePath) => {
Expand Down Expand Up @@ -617,7 +619,7 @@ describe("read_file tool XML output structure", () => {

mockProvider.getState.mockResolvedValue({ maxReadFileLine, maxImageFileSize: 20, maxTotalImageSize: 20 })
mockedCountFileLines.mockResolvedValue(totalLines)
mockedIsBinaryFile.mockResolvedValue(isBinary)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(isBinary)
mockCline.rooIgnoreController.validateAccess = vi.fn().mockReturnValue(validateAccess)

let argsContent = `<file><path>${testFilePath}</path></file>`
Expand Down Expand Up @@ -758,7 +760,7 @@ describe("read_file tool XML output structure", () => {

it("should allow multiple images under the total memory limit", async () => {
// Setup required mocks (don't clear all mocks - preserve API setup)
mockedIsBinaryFile.mockResolvedValue(true)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(true)
mockedCountFileLines.mockResolvedValue(0)
fsPromises.readFile.mockResolvedValue(
Buffer.from(
Expand Down Expand Up @@ -831,7 +833,7 @@ describe("read_file tool XML output structure", () => {

it("should skip images that would exceed the total memory limit", async () => {
// Setup required mocks (don't clear all mocks)
mockedIsBinaryFile.mockResolvedValue(true)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(true)
mockedCountFileLines.mockResolvedValue(0)
fsPromises.readFile.mockResolvedValue(
Buffer.from(
Expand Down Expand Up @@ -917,7 +919,7 @@ describe("read_file tool XML output structure", () => {
// Setup mocks (don't clear all mocks)

// Setup required mocks
mockedIsBinaryFile.mockResolvedValue(true)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(true)
mockedCountFileLines.mockResolvedValue(0)
fsPromises.readFile.mockResolvedValue(
Buffer.from(
Expand Down Expand Up @@ -990,7 +992,7 @@ describe("read_file tool XML output structure", () => {
// Setup mocks (don't clear all mocks)

// Setup required mocks
mockedIsBinaryFile.mockResolvedValue(true)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(true)
mockedCountFileLines.mockResolvedValue(0)
fsPromises.readFile.mockResolvedValue(
Buffer.from(
Expand Down Expand Up @@ -1084,7 +1086,7 @@ describe("read_file tool XML output structure", () => {
maxTotalImageSize: 20, // 20MB total
})

mockedIsBinaryFile.mockResolvedValue(true)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(true)
mockedCountFileLines.mockResolvedValue(0)
mockedFsReadFile.mockResolvedValue(imageBuffer)

Expand Down Expand Up @@ -1115,7 +1117,7 @@ describe("read_file tool XML output structure", () => {
// Setup mocks (don't clear all mocks)

// Setup required mocks for first batch
mockedIsBinaryFile.mockResolvedValue(true)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(true)
mockedCountFileLines.mockResolvedValue(0)
fsPromises.readFile.mockResolvedValue(
Buffer.from(
Expand Down Expand Up @@ -1161,7 +1163,7 @@ describe("read_file tool XML output structure", () => {
await executeReadMultipleImagesTool(firstBatch.map((img) => img.path))

// Setup second batch (don't clear all mocks)
mockedIsBinaryFile.mockResolvedValue(true)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(true)
mockedCountFileLines.mockResolvedValue(0)
fsPromises.readFile.mockResolvedValue(
Buffer.from(
Expand Down Expand Up @@ -1203,7 +1205,7 @@ describe("read_file tool XML output structure", () => {
// Clear and reset file system mocks for second batch
fsPromises.stat.mockClear()
fsPromises.readFile.mockClear()
mockedIsBinaryFile.mockClear()
mockedIsBinaryFileWithEncodingDetection.mockClear()
mockedCountFileLines.mockClear()

// Reset mocks for second batch
Expand All @@ -1214,7 +1216,7 @@ describe("read_file tool XML output structure", () => {
"base64",
),
)
mockedIsBinaryFile.mockResolvedValue(true)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(true)
mockedCountFileLines.mockResolvedValue(0)
mockedPathResolve.mockImplementation((cwd, relPath) => `/${relPath}`)

Expand All @@ -1241,7 +1243,7 @@ describe("read_file tool XML output structure", () => {
]

// Setup mocks
mockedIsBinaryFile.mockResolvedValue(true)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(true)
mockedCountFileLines.mockResolvedValue(0)
fsPromises.readFile.mockResolvedValue(imageBuffer)

Expand Down Expand Up @@ -1289,7 +1291,7 @@ describe("read_file tool XML output structure", () => {
// starts with fresh memory tracking

// Setup mocks
mockedIsBinaryFile.mockResolvedValue(true)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(true)
mockedCountFileLines.mockResolvedValue(0)
fsPromises.readFile.mockResolvedValue(imageBuffer)

Expand Down Expand Up @@ -1394,7 +1396,7 @@ describe("read_file tool with image support", () => {
const imageBuffer = Buffer.from(base64ImageData, "base64")

const mockedCountFileLines = vi.mocked(countFileLines)
const mockedIsBinaryFile = vi.mocked(isBinaryFile)
const mockedIsBinaryFileWithEncodingDetection = vi.mocked(isBinaryFileWithEncodingDetection)
const mockedPathResolve = vi.mocked(path.resolve)
const mockedFsReadFile = vi.mocked(fsPromises.readFile)
const mockedExtractTextFromFile = vi.mocked(extractTextFromFile)
Expand All @@ -1406,7 +1408,7 @@ describe("read_file tool with image support", () => {
beforeEach(() => {
// Clear specific mocks (not all mocks to preserve shared state)
mockedPathResolve.mockClear()
mockedIsBinaryFile.mockClear()
mockedIsBinaryFileWithEncodingDetection.mockClear()
mockedCountFileLines.mockClear()
mockedFsReadFile.mockClear()
mockedExtractTextFromFile.mockClear()
Expand All @@ -1425,7 +1427,7 @@ describe("read_file tool with image support", () => {
setImageSupport(localMockCline, true)

mockedPathResolve.mockReturnValue(absoluteImagePath)
mockedIsBinaryFile.mockResolvedValue(true)
mockedIsBinaryFileWithEncodingDetection.mockResolvedValue(true)
mockedCountFileLines.mockResolvedValue(0)
mockedFsReadFile.mockResolvedValue(imageBuffer)

Expand Down
3 changes: 2 additions & 1 deletion src/core/tools/applyDiffTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import path from "path"
import fs from "fs/promises"

import { TelemetryService } from "@roo-code/telemetry"
import { readFileWithEncodingDetection } from "../../utils/encoding"
import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"

import { ClineSayTool } from "../../shared/ExtensionMessage"
Expand Down Expand Up @@ -89,7 +90,7 @@ export async function applyDiffToolLegacy(
return
}

const originalContent: string = await fs.readFile(absolutePath, "utf-8")
const originalContent: string = await readFileWithEncodingDetection(absolutePath)

// Apply the diff to the original content
const diffResult = (await cline.diffStrategy?.applyDiff(
Expand Down
3 changes: 2 additions & 1 deletion src/core/tools/insertContentTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import fs from "fs/promises"
import path from "path"

import { getReadablePath } from "../../utils/path"
import { readFileWithEncodingDetection } from "../../utils/encoding"
import { Task } from "../task/Task"
import { ToolUse, AskApproval, HandleError, PushToolResult, RemoveClosingTag } from "../../shared/tools"
import { formatResponse } from "../prompts/responses"
Expand Down Expand Up @@ -93,7 +94,7 @@ export async function insertContentTool(
return
}
} else {
fileContent = await fs.readFile(absolutePath, "utf8")
fileContent = await readFileWithEncodingDetection(absolutePath)
}

cline.consecutiveMistakeCount = 0
Expand Down
9 changes: 5 additions & 4 deletions src/core/tools/multiApplyDiffTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import path from "path"
import fs from "fs/promises"

import { TelemetryService } from "@roo-code/telemetry"
import { readFileWithEncodingDetection } from "../../utils/encoding"
import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"

import { ClineSayTool } from "../../shared/ExtensionMessage"
Expand Down Expand Up @@ -300,7 +301,7 @@ Original error: ${errorMessage}`

let unified = ""
try {
const original = await fs.readFile(opResult.absolutePath!, "utf-8")
const original = await readFileWithEncodingDetection(opResult.absolutePath!)
const processed = !cline.api.getModel().id.includes("claude")
? (opResult.diffItems || []).map((item) => ({
...item,
Expand Down Expand Up @@ -457,7 +458,7 @@ Original error: ${errorMessage}`
const fileExists = opResult.fileExists!

try {
let originalContent: string | null = await fs.readFile(absolutePath, "utf-8")
let originalContent: string | null = await readFileWithEncodingDetection(absolutePath)
let beforeContent: string | null = originalContent
let successCount = 0
let formattedError = ""
Expand Down Expand Up @@ -611,7 +612,7 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""}
cline.diffViewProvider.scrollToFirstDiff()
} else {
// For direct save, we still need to set originalContent
cline.diffViewProvider.originalContent = await fs.readFile(absolutePath, "utf-8")
cline.diffViewProvider.originalContent = await readFileWithEncodingDetection(absolutePath)
}

// Ask for approval (same for both flows)
Expand Down Expand Up @@ -646,7 +647,7 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""}
if (isPreventFocusDisruptionEnabled) {
// Direct file write without diff view or opening the file
cline.diffViewProvider.editType = "modify"
cline.diffViewProvider.originalContent = await fs.readFile(absolutePath, "utf-8")
cline.diffViewProvider.originalContent = await readFileWithEncodingDetection(absolutePath)
Copy link
Contributor

@roomote roomote bot Aug 8, 2025

Choose a reason for hiding this comment

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

I noticed is called multiple times for the same file (here and line 560). Could we optimize this by reading the file once and reusing the content? This would improve performance, especially for large files.

Copy link
Contributor Author

@aheizi aheizi Aug 8, 2025

Choose a reason for hiding this comment

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

Good Review. Let me make some revisions

Copy link
Contributor Author

@aheizi aheizi Aug 8, 2025

Choose a reason for hiding this comment

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

Let's prioritize consistency with the past first. The issue of reading the file repeatedly can be addressed as an optimization for the next issue.

Copy link
Member

@daniel-lxs daniel-lxs Aug 12, 2025

Choose a reason for hiding this comment

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

Should we also ensure the encoding is preserved when the file is written back? The DiffViewProvider might need to track and use the original encoding when saving files.

await cline.diffViewProvider.saveDirectly(
relPath,
originalContent!,
Expand Down
7 changes: 5 additions & 2 deletions src/core/tools/readFileTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import path from "path"
import { isBinaryFile } from "isbinaryfile"

import { Task } from "../task/Task"
import { ClineSayTool } from "../../shared/ExtensionMessage"
Expand All @@ -14,6 +13,7 @@ import { readLines } from "../../integrations/misc/read-lines"
import { extractTextFromFile, addLineNumbers, getSupportedBinaryFormats } from "../../integrations/misc/extract-text"
import { parseSourceCodeDefinitionsForFile } from "../../services/tree-sitter"
import { parseXml } from "../../utils/xml"
import { isBinaryFileWithEncodingDetection } from "../../utils/encoding"
import {
DEFAULT_MAX_IMAGE_FILE_SIZE_MB,
DEFAULT_MAX_TOTAL_IMAGE_SIZE_MB,
Expand Down Expand Up @@ -456,7 +456,10 @@ export async function readFileTool(

// Process approved files
try {
const [totalLines, isBinary] = await Promise.all([countFileLines(fullPath), isBinaryFile(fullPath)])
const [totalLines, isBinary] = await Promise.all([
countFileLines(fullPath),
isBinaryFileWithEncodingDetection(fullPath),
])

// Handle binary files (but allow specific file types that extractTextFromFile can handle)
if (isBinary) {
Expand Down
3 changes: 2 additions & 1 deletion src/core/tools/writeToFileTool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as vscode from "vscode"
import fs from "fs/promises"

import { Task } from "../task/Task"
import { readFileWithEncodingDetection } from "../../utils/encoding"
import { ClineSayTool } from "../../shared/ExtensionMessage"
import { formatResponse } from "../prompts/responses"
import { ToolUse, AskApproval, HandleError, PushToolResult, RemoveClosingTag } from "../../shared/tools"
Expand Down Expand Up @@ -178,7 +179,7 @@ export async function writeToFileTool(
cline.diffViewProvider.editType = fileExists ? "modify" : "create"
if (fileExists) {
const absolutePath = path.resolve(cline.cwd, relPath)
cline.diffViewProvider.originalContent = await fs.readFile(absolutePath, "utf-8")
cline.diffViewProvider.originalContent = await readFileWithEncodingDetection(absolutePath)
} else {
cline.diffViewProvider.originalContent = ""
}
Expand Down
Loading