Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
75 changes: 60 additions & 15 deletions src/core/prompts/sections/__tests__/custom-system-prompt.spec.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
// Mocks must come first, before imports

vi.mock("fs/promises")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mock setup pattern here is clean, but could benefit from a comment explaining why mocks must come before imports for future test writers. This is a TypeScript/Vitest specific requirement that might not be obvious.

vi.mock("os")

// Then imports
import type { Mock } from "vitest"
import path from "path"
import os from "os"
import { readFile } from "fs/promises"
import type { Mode } from "../../../../shared/modes" // Type-only import
import { loadSystemPromptFile, PromptVariables } from "../custom-system-prompt"
import { loadSystemPromptFile, PromptVariables, getGlobalSystemPromptFilePath } from "../custom-system-prompt"

// Cast the mocked readFile to the correct Mock type
const mockedReadFile = readFile as Mock<typeof readFile>
const mockedHomedir = os.homedir as Mock<typeof os.homedir>

describe("loadSystemPromptFile", () => {
// Corrected PromptVariables type and added mockMode
Expand All @@ -19,15 +22,18 @@ describe("loadSystemPromptFile", () => {
}
const mockCwd = "/mock/cwd"
const mockMode: Mode = "test" // Use Mode type, e.g., 'test'
const mockHomeDir = "/home/user"
// Corrected expected file path format
const expectedFilePath = path.join(mockCwd, ".roo", `system-prompt-${mockMode}`)
const expectedLocalFilePath = path.join(mockCwd, ".roo", `system-prompt-${mockMode}`)
const expectedGlobalFilePath = path.join(mockHomeDir, ".roo", `system-prompt-${mockMode}`)

beforeEach(() => {
// Clear mocks before each test
mockedReadFile.mockClear()
mockedHomedir.mockReturnValue(mockHomeDir)
})

it("should return an empty string if the file does not exist (ENOENT)", async () => {
it("should return an empty string if neither local nor global file exists (ENOENT)", async () => {
const error: NodeJS.ErrnoException = new Error("File not found")
error.code = "ENOENT"
mockedReadFile.mockRejectedValue(error)
Expand All @@ -36,8 +42,9 @@ describe("loadSystemPromptFile", () => {
const result = await loadSystemPromptFile(mockCwd, mockMode, mockVariables)

expect(result).toBe("")
expect(mockedReadFile).toHaveBeenCalledTimes(1)
expect(mockedReadFile).toHaveBeenCalledWith(expectedFilePath, "utf-8")
expect(mockedReadFile).toHaveBeenCalledTimes(2)
expect(mockedReadFile).toHaveBeenCalledWith(expectedLocalFilePath, "utf-8")
expect(mockedReadFile).toHaveBeenCalledWith(expectedGlobalFilePath, "utf-8")
})

// Updated test: should re-throw unexpected errors
Expand All @@ -50,18 +57,23 @@ describe("loadSystemPromptFile", () => {

// Verify readFile was still called correctly
expect(mockedReadFile).toHaveBeenCalledTimes(1)
expect(mockedReadFile).toHaveBeenCalledWith(expectedFilePath, "utf-8")
expect(mockedReadFile).toHaveBeenCalledWith(expectedLocalFilePath, "utf-8")
})

it("should return an empty string if the file content is empty", async () => {
mockedReadFile.mockResolvedValue("")
it("should return an empty string if the local file content is empty and check global", async () => {
const error: NodeJS.ErrnoException = new Error("File not found")
error.code = "ENOENT"

// Local file is empty, global file doesn't exist
mockedReadFile.mockResolvedValueOnce("").mockRejectedValueOnce(error)

// Added mockMode argument
const result = await loadSystemPromptFile(mockCwd, mockMode, mockVariables)

expect(result).toBe("")
expect(mockedReadFile).toHaveBeenCalledTimes(1)
expect(mockedReadFile).toHaveBeenCalledWith(expectedFilePath, "utf-8")
expect(mockedReadFile).toHaveBeenCalledTimes(2)
expect(mockedReadFile).toHaveBeenNthCalledWith(1, expectedLocalFilePath, "utf-8")
expect(mockedReadFile).toHaveBeenNthCalledWith(2, expectedGlobalFilePath, "utf-8")
})

// Updated test to only check workspace interpolation
Expand All @@ -74,7 +86,7 @@ describe("loadSystemPromptFile", () => {

expect(result).toBe("Workspace is: /path/to/workspace")
expect(mockedReadFile).toHaveBeenCalledTimes(1)
expect(mockedReadFile).toHaveBeenCalledWith(expectedFilePath, "utf-8")
expect(mockedReadFile).toHaveBeenCalledWith(expectedLocalFilePath, "utf-8")
})

// Updated test for multiple occurrences of workspace
Expand All @@ -87,7 +99,7 @@ describe("loadSystemPromptFile", () => {

expect(result).toBe("Path: /path/to/workspace//path/to/workspace")
expect(mockedReadFile).toHaveBeenCalledTimes(1)
expect(mockedReadFile).toHaveBeenCalledWith(expectedFilePath, "utf-8")
expect(mockedReadFile).toHaveBeenCalledWith(expectedLocalFilePath, "utf-8")
})

// Updated test for mixed used/unused
Expand All @@ -101,7 +113,7 @@ describe("loadSystemPromptFile", () => {
// Unused variables should remain untouched
expect(result).toBe("Workspace: /path/to/workspace, Unused: {{unusedVar}}, Another: {{another}}")
expect(mockedReadFile).toHaveBeenCalledTimes(1)
expect(mockedReadFile).toHaveBeenCalledWith(expectedFilePath, "utf-8")
expect(mockedReadFile).toHaveBeenCalledWith(expectedLocalFilePath, "utf-8")
})

// Test remains valid, just needs the mode argument and updated template
Expand All @@ -114,7 +126,7 @@ describe("loadSystemPromptFile", () => {

expect(result).toBe("Workspace: /path/to/workspace, Missing: {{missingPlaceholder}}")
expect(mockedReadFile).toHaveBeenCalledTimes(1)
expect(mockedReadFile).toHaveBeenCalledWith(expectedFilePath, "utf-8")
expect(mockedReadFile).toHaveBeenCalledWith(expectedLocalFilePath, "utf-8")
})

// Removed the test for extra keys as PromptVariables is simple now
Expand All @@ -129,6 +141,39 @@ describe("loadSystemPromptFile", () => {

expect(result).toBe("This is a static prompt.")
expect(mockedReadFile).toHaveBeenCalledTimes(1)
expect(mockedReadFile).toHaveBeenCalledWith(expectedFilePath, "utf-8")
expect(mockedReadFile).toHaveBeenCalledWith(expectedLocalFilePath, "utf-8")
})

it("should use global system prompt when local file doesn't exist", async () => {
const error: NodeJS.ErrnoException = new Error("File not found")
error.code = "ENOENT"
const globalTemplate = "Global system prompt: {{workspace}}"

// First call (local) fails, second call (global) succeeds
mockedReadFile.mockRejectedValueOnce(error).mockResolvedValueOnce(globalTemplate)

const result = await loadSystemPromptFile(mockCwd, mockMode, mockVariables)

expect(result).toBe("Global system prompt: /path/to/workspace")
expect(mockedReadFile).toHaveBeenCalledTimes(2)
expect(mockedReadFile).toHaveBeenNthCalledWith(1, expectedLocalFilePath, "utf-8")
expect(mockedReadFile).toHaveBeenNthCalledWith(2, expectedGlobalFilePath, "utf-8")
})

it("should prefer local system prompt over global when both exist", async () => {
const localTemplate = "Local system prompt: {{workspace}}"
mockedReadFile.mockResolvedValueOnce(localTemplate)

const result = await loadSystemPromptFile(mockCwd, mockMode, mockVariables)

expect(result).toBe("Local system prompt: /path/to/workspace")
// Should only read the local file, not the global one
expect(mockedReadFile).toHaveBeenCalledTimes(1)
expect(mockedReadFile).toHaveBeenCalledWith(expectedLocalFilePath, "utf-8")
})

it("should correctly generate global system prompt file path", () => {
const result = getGlobalSystemPromptFilePath(mockMode)
expect(result).toBe(expectedGlobalFilePath)
})
})
28 changes: 24 additions & 4 deletions src/core/prompts/sections/custom-system-prompt.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import fs from "fs/promises"
import path from "path"
import os from "os"
import { Mode } from "../../../shared/modes"
import { fileExistsAtPath } from "../../../utils/fs"

Expand Down Expand Up @@ -50,15 +51,34 @@ export function getSystemPromptFilePath(cwd: string, mode: Mode): string {
}

/**
* Loads custom system prompt from a file at .roo/system-prompt-[mode slug]
* If the file doesn't exist, returns an empty string
* Get the path to a global system prompt file for a specific mode
* Located in the user's home directory under .roo/system-prompt-[mode slug]
*/
export function getGlobalSystemPromptFilePath(mode: Mode): string {
return path.join(os.homedir(), ".roo", `system-prompt-${mode}`)
}

/**
* Loads custom system prompt from a file, checking in the following order:
* 1. Local project: .roo/system-prompt-[mode slug]
* 2. Global (home directory): ~/.roo/system-prompt-[mode slug]
* If neither file exists, returns an empty string
*/
export async function loadSystemPromptFile(cwd: string, mode: Mode, variables: PromptVariables): Promise<string> {
const filePath = getSystemPromptFilePath(cwd, mode)
const rawContent = await safeReadFile(filePath)
// First, check for local project-specific system prompt
const localFilePath = getSystemPromptFilePath(cwd, mode)
let rawContent = await safeReadFile(localFilePath)

// If no local file exists, check for global system prompt
if (!rawContent) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The implementation reads the local file even when it's empty before checking the global file. Could we optimize this by checking file existence first, then only reading if the file exists and is non-empty? This would avoid unnecessary file reads for empty files.

const globalFilePath = getGlobalSystemPromptFilePath(mode)
rawContent = await safeReadFile(globalFilePath)
}

if (!rawContent) {
return ""
}

const interpolatedContent = interpolatePromptContent(rawContent, variables)
return interpolatedContent
}
Expand Down
14 changes: 11 additions & 3 deletions src/core/webview/ClineProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ import { ContextProxy } from "../config/ContextProxy"
import { ProviderSettingsManager } from "../config/ProviderSettingsManager"
import { CustomModesManager } from "../config/CustomModesManager"
import { Task, TaskOptions } from "../task/Task"
import { getSystemPromptFilePath } from "../prompts/sections/custom-system-prompt"
import { getSystemPromptFilePath, getGlobalSystemPromptFilePath } from "../prompts/sections/custom-system-prompt"

import { webviewMessageHandler } from "./webviewMessageHandler"
import { getNonce } from "./getNonce"
Expand Down Expand Up @@ -1480,10 +1480,18 @@ export class ClineProvider

/**
* Checks if there is a file-based system prompt override for the given mode
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Consider adding a comment here explaining the precedence order (local > global) for future maintainers. Something like:

* Checks both local project directory and global home directory
*/
async hasFileBasedSystemPromptOverride(mode: Mode): Promise<boolean> {
const promptFilePath = getSystemPromptFilePath(this.cwd, mode)
return await fileExistsAtPath(promptFilePath)
// Check local project directory first
const localPromptFilePath = getSystemPromptFilePath(this.cwd, mode)
if (await fileExistsAtPath(localPromptFilePath)) {
return true
}

// Check global home directory
const globalPromptFilePath = getGlobalSystemPromptFilePath(mode)
return await fileExistsAtPath(globalPromptFilePath)
}

/**
Expand Down
Loading