Skip to content

fix: add Zod schema validation to readModels disk cache #1099

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

Closed
daewoongoh wants to merge 2 commits into from
Closed

fix: add Zod schema validation to readModels disk cache #1099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d561cdc
refactor: add schema validation to readModels and extract common vali…
daewoongoh Aug 2, 2026
36b1355
test: fix ContextProxy mock path to ensure modelCache validation path…
daewoongoh Aug 2, 2026
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
56 changes: 51 additions & 5 deletions src/api/providers/fetchers/__tests__/modelCache.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ vi.mock("../moonshot")
vi.mock("../zoo-gateway")

// Mock ContextProxy with a simple static instance
vi.mock("../../../core/config/ContextProxy", () => ({
vi.mock("../../../../core/config/ContextProxy", () => ({
ContextProxy: {
instance: {
globalStorageUri: {
Expand All @@ -61,7 +61,8 @@ vi.mock("../../../core/config/ContextProxy", () => ({

// Then imports
import type { Mock, Mocked } from "vitest"
import { providerIdentifiers } from "@roo-code/types"
import { providerIdentifiers, modelInfoSchema } from "@roo-code/types"
import { z } from "zod"
import * as fsSync from "fs"
import NodeCache from "node-cache"
import { TelemetryService } from "@roo-code/telemetry"
Expand Down Expand Up @@ -320,9 +321,9 @@ describe("getModelsFromCache disk fallback", () => {

const result = getModelsFromCache(providerIdentifiers.openrouter)

// In the test environment, ContextProxy.instance may not be fully initialized,
// so getCacheDirectoryPathSync returns undefined and disk cache is not attempted
expect(result).toBeUndefined()
// With ContextProxy correctly mocked, getCacheDirectoryPathSync resolves
// properly and disk cache loading + validation is fully exercised.
expect(result).toEqual(diskModels)
})

it("handles disk read errors gracefully", () => {
Expand Down Expand Up @@ -356,6 +357,51 @@ describe("getModelsFromCache disk fallback", () => {
})
})

describe("validateModelRecord via getModelsFromCache", () => {
let mockCache: Mocked<NodeCache>

beforeEach(() => {
vi.clearAllMocks()
const MockedNodeCache = vi.mocked(NodeCache)
mockCache = vi.mocked(new MockedNodeCache())
// Always miss memory cache so disk path is exercised
mockCache.get.mockReturnValue(undefined)
vi.mocked(fsSync.existsSync).mockReturnValue(true)
})

it("returns validated data when disk cache contains a valid ModelRecord", () => {
const validModels = {
"test-model": {
maxTokens: 4096,
contextWindow: 128000,
supportsPromptCache: false,
},
}

vi.mocked(fsSync.readFileSync).mockReturnValue(JSON.stringify(validModels))

const result = getModelsFromCache(providerIdentifiers.openrouter)
expect(result).toEqual(validModels)
})

it("returns undefined and logs error when disk cache contains invalid schema data", () => {
const invalidData = [{ notAModelRecord: true }]

vi.mocked(fsSync.readFileSync).mockReturnValue(JSON.stringify(invalidData))
const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(function () {})

const result = getModelsFromCache(providerIdentifiers.openrouter)

expect(result).toBeUndefined()
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect.stringContaining("[MODEL_CACHE] Invalid disk cache for"),
expect.anything(),
)

consoleErrorSpy.mockRestore()
})
})

describe("empty cache protection", () => {
let mockCache: Mocked<NodeCache>
let mockGet: Mocked<NodeCache>["get"]
Expand Down
32 changes: 21 additions & 11 deletions src/api/providers/fetchers/modelCache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,12 +204,28 @@ async function writeModels(cacheKey: string, data: ModelRecord) {
await safeWriteJson(path.join(cacheDir, filename), data)
}

/**
* Validate parsed JSON data against the ModelRecord Zod schema.
* Returns the validated data on success, or undefined (with an error log) on failure.
*/
function validateModelRecord(data: unknown, cacheKey: string): ModelRecord | undefined {
const validation = modelRecordSchema.safeParse(data)
if (!validation.success) {
console.error(`[MODEL_CACHE] Invalid disk cache for ${cacheKey}:`, validation.error.format())
return undefined
}
return validation.data
}

async function readModels(cacheKey: string): Promise<ModelRecord | undefined> {
const filename = `${cacheKeyToFilename(cacheKey)}_models.json`
const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath)
const filePath = path.join(cacheDir, filename)
const exists = await fileExistsAtPath(filePath)
return exists ? JSON.parse(await fs.readFile(filePath, "utf8")) : undefined
if (!exists) return undefined

const data = JSON.parse(await fs.readFile(filePath, "utf8"))
return validateModelRecord(data, cacheKey)
Comment on lines 220 to +228

@coderabbitai coderabbitai Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Handle syntax errors in asynchronous cache reads.

JSON.parse at Line 227 throws for malformed or truncated _models.json files. The exception rejects readModels before validation runs. The synchronous path already catches this failure. Return undefined from both paths for corrupted disk data, as required by the PR objective.

Suggested error boundary 
 async function readModels(cacheKey: string): Promise<ModelRecord | undefined> {
+	try {
 		const filename = `${cacheKeyToFilename(cacheKey)}_models.json`
 		const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath)
 		const filePath = path.join(cacheDir, filename)
 		const exists = await fileExistsAtPath(filePath)
 		if (!exists) return undefined

 		const data = JSON.parse(await fs.readFile(filePath, "utf8"))
 		return validateModelRecord(data, cacheKey)
+	} catch (error) {
+		console.error(`[MODEL_CACHE] Error loading ${cacheKey} models from disk:`, error)
+		return undefined
+	}
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async function readModels(cacheKey: string): Promise<ModelRecord | undefined> {
const filename = `${cacheKeyToFilename(cacheKey)}_models.json`
const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath)
const filePath = path.join(cacheDir, filename)
const exists = await fileExistsAtPath(filePath)
return exists ? JSON.parse(await fs.readFile(filePath, "utf8")) : undefined
if (!exists) return undefined
const data = JSON.parse(await fs.readFile(filePath, "utf8"))
return validateModelRecord(data, cacheKey)
async function readModels(cacheKey: string): Promise<ModelRecord | undefined> {
try {
const filename = `${cacheKeyToFilename(cacheKey)}_models.json`
const cacheDir = await getCacheDirectoryPath(ContextProxy.instance.globalStorageUri.fsPath)
const filePath = path.join(cacheDir, filename)
const exists = await fileExistsAtPath(filePath)
if (!exists) return undefined
const data = JSON.parse(await fs.readFile(filePath, "utf8"))
return validateModelRecord(data, cacheKey)
} catch (error) {
console.error(`[MODEL_CACHE] Error loading ${cacheKey} models from disk:`, error)
return undefined
}
}
🧰 Tools
🪛 ast-grep (0.45.0)

[warning] 226-226: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(detect-non-literal-fs-filename-typescript)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/api/providers/fetchers/modelCache.ts` around lines 220 - 228, Update
readModels to catch JSON.parse or file-read syntax failures for malformed or
truncated cache data and return undefined instead of rejecting before
validateModelRecord runs. Match the existing synchronous cache-read behavior,
while preserving validation for successfully parsed data.

}

/**
Expand Down Expand Up @@ -533,21 +549,15 @@ export function getModelsFromCache(options: GetModelsOptions | ProviderName): Mo
const data = fsSync.readFileSync(filePath, "utf8")
const models = JSON.parse(data)

// Validate the disk cache data structure using Zod schema
// This ensures the data conforms to ModelRecord = Record<string, ModelInfo>
const validation = modelRecordSchema.safeParse(models)
if (!validation.success) {
console.error(
`[MODEL_CACHE] Invalid disk cache data structure for ${cacheKey}:`,
validation.error.format(),
)
const validated = validateModelRecord(models, cacheKey)
if (!validated) {
return undefined
}

// Populate memory cache for future fast access
memoryCache.set(cacheKey, validation.data)
memoryCache.set(cacheKey, validated)

return validation.data
return validated
}
} catch (error) {
console.error(`[MODEL_CACHE] Error loading ${cacheKey} models from disk:`, error)
Expand Down
Loading