Validation fixes

This commit is contained in:
Matt Rubens
2025-01-24 10:03:35 -05:00
parent 569e104bf2
commit 5f8a8887fb
6 changed files with 108 additions and 98 deletions

View File

@@ -1142,17 +1142,25 @@ export class Cline {
await this.browserSession.closeBrowser() await this.browserSession.closeBrowser()
} }
// Validate tool use before execution // Only validate complete tool uses
const { mode } = (await this.providerRef.deref()?.getState()) ?? {} if (!block.partial) {
const { customModes } = (await this.providerRef.deref()?.getState()) ?? {} const { mode } = (await this.providerRef.deref()?.getState()) ?? {}
try { const { customModes } = (await this.providerRef.deref()?.getState()) ?? {}
validateToolUse(block.name as ToolName, mode ?? defaultModeSlug, customModes ?? [], { try {
apply_diff: this.diffEnabled, validateToolUse(
}) block.name as ToolName,
} catch (error) { mode ?? defaultModeSlug,
this.consecutiveMistakeCount++ customModes ?? [],
pushToolResult(formatResponse.toolError(error.message)) {
break apply_diff: this.diffEnabled,
},
block.params,
)
} catch (error) {
this.consecutiveMistakeCount++
pushToolResult(formatResponse.toolError(error.message))
break
}
} }
switch (block.name) { switch (block.name) {

View File

@@ -22,7 +22,7 @@ const GroupOptionsSchema = z.object({
}, },
{ message: "Invalid regular expression pattern" }, { message: "Invalid regular expression pattern" },
), ),
fileRegexDescription: z.string().optional(), description: z.string().optional(),
}) })
// Schema for a group entry - either a tool group string or a tuple of [group, options] // Schema for a group entry - either a tool group string or a tuple of [group, options]

View File

@@ -135,7 +135,7 @@ describe("CustomModeSchema", () => {
roleDefinition: "Documentation editing mode", roleDefinition: "Documentation editing mode",
groups: [ groups: [
"read", "read",
["edit", { fileRegex: "\\.(md|txt)$", fileRegexDescription: "Documentation files only" }], ["edit", { fileRegex: "\\.(md|txt)$", description: "Documentation files only" }],
"browser", "browser",
], ],
} }

View File

@@ -1,4 +1,4 @@
import { Mode, isToolAllowedForMode, getModeConfig, ModeConfig } from "../shared/modes" import { Mode, isToolAllowedForMode, getModeConfig, ModeConfig, FileRestrictionError } from "../shared/modes"
import { ToolName } from "../shared/tool-groups" import { ToolName } from "../shared/tool-groups"
export { isToolAllowedForMode } export { isToolAllowedForMode }
@@ -9,8 +9,9 @@ export function validateToolUse(
mode: Mode, mode: Mode,
customModes?: ModeConfig[], customModes?: ModeConfig[],
toolRequirements?: Record<string, boolean>, toolRequirements?: Record<string, boolean>,
toolParams?: Record<string, unknown>,
): void { ): void {
if (!isToolAllowedForMode(toolName, mode, customModes ?? [], toolRequirements)) { if (!isToolAllowedForMode(toolName, mode, customModes ?? [], toolRequirements, toolParams)) {
throw new Error(`Tool "${toolName}" is not allowed in ${mode} mode.`) throw new Error(`Tool "${toolName}" is not allowed in ${mode} mode.`)
} }
} }

View File

@@ -29,59 +29,57 @@ describe("isToolAllowedForMode", () => {
describe("file restrictions", () => { describe("file restrictions", () => {
it("allows editing matching files", () => { it("allows editing matching files", () => {
// Test markdown editor mode // Test markdown editor mode
const mdResult = isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, "test.md") const mdResult = isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, {
path: "test.md",
})
expect(mdResult).toBe(true) expect(mdResult).toBe(true)
// Test CSS editor mode // Test CSS editor mode
const cssResult = isToolAllowedForMode("write_to_file", "css-editor", customModes, undefined, "styles.css") const cssResult = isToolAllowedForMode("write_to_file", "css-editor", customModes, undefined, {
path: "styles.css",
})
expect(cssResult).toBe(true) expect(cssResult).toBe(true)
}) })
it("rejects editing non-matching files", () => { it("rejects editing non-matching files", () => {
// Test markdown editor mode with non-markdown file // Test markdown editor mode with non-markdown file
const mdError = isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, "test.js") expect(() =>
expect(mdError).toBeInstanceOf(FileRestrictionError) isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, { path: "test.js" }),
expect((mdError as FileRestrictionError).message).toContain("\\.md$") ).toThrow(FileRestrictionError)
expect(() =>
isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, { path: "test.js" }),
).toThrow(/\\.md\$/)
// Test CSS editor mode with non-CSS file // Test CSS editor mode with non-CSS file
const cssError = isToolAllowedForMode("write_to_file", "css-editor", customModes, undefined, "test.js") expect(() =>
expect(cssError).toBeInstanceOf(FileRestrictionError) isToolAllowedForMode("write_to_file", "css-editor", customModes, undefined, { path: "test.js" }),
expect((cssError as FileRestrictionError).message).toContain("\\.css$") ).toThrow(FileRestrictionError)
}) expect(() =>
isToolAllowedForMode("write_to_file", "css-editor", customModes, undefined, { path: "test.js" }),
it("requires file path for restricted edit operations", () => { ).toThrow(/\\.css\$/)
const result = isToolAllowedForMode("write_to_file", "markdown-editor", customModes)
expect(result).toBeInstanceOf(FileRestrictionError)
expect((result as FileRestrictionError).message).toContain("\\.md$")
}) })
it("applies restrictions to both write_to_file and apply_diff", () => { it("applies restrictions to both write_to_file and apply_diff", () => {
// Test write_to_file // Test write_to_file
const writeResult = isToolAllowedForMode( const writeResult = isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, {
"write_to_file", path: "test.md",
"markdown-editor", })
customModes,
undefined,
"test.md",
)
expect(writeResult).toBe(true) expect(writeResult).toBe(true)
// Test apply_diff // Test apply_diff
const diffResult = isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, "test.md") const diffResult = isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, {
path: "test.md",
})
expect(diffResult).toBe(true) expect(diffResult).toBe(true)
// Test both with non-matching file // Test both with non-matching file
const writeError = isToolAllowedForMode( expect(() =>
"write_to_file", isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, { path: "test.js" }),
"markdown-editor", ).toThrow(FileRestrictionError)
customModes,
undefined,
"test.js",
)
expect(writeError).toBeInstanceOf(FileRestrictionError)
const diffError = isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, "test.js") expect(() =>
expect(diffError).toBeInstanceOf(FileRestrictionError) isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, { path: "test.js" }),
).toThrow(FileRestrictionError)
}) })
it("uses description in file restriction error for custom modes", () => { it("uses description in file restriction error for custom modes", () => {
@@ -99,60 +97,57 @@ describe("isToolAllowedForMode", () => {
] ]
// Test write_to_file with non-matching file // Test write_to_file with non-matching file
const writeError = isToolAllowedForMode( expect(() =>
"write_to_file", isToolAllowedForMode("write_to_file", "docs-editor", customModesWithDescription, undefined, {
"docs-editor", path: "test.js",
customModesWithDescription, }),
undefined, ).toThrow(FileRestrictionError)
"test.js", expect(() =>
) isToolAllowedForMode("write_to_file", "docs-editor", customModesWithDescription, undefined, {
expect(writeError).toBeInstanceOf(FileRestrictionError) path: "test.js",
expect((writeError as FileRestrictionError).message).toContain("Documentation files only") }),
).toThrow(/Documentation files only/)
// Test apply_diff with non-matching file // Test apply_diff with non-matching file
const diffError = isToolAllowedForMode( expect(() =>
"apply_diff", isToolAllowedForMode("apply_diff", "docs-editor", customModesWithDescription, undefined, {
"docs-editor", path: "test.js",
customModesWithDescription, }),
undefined, ).toThrow(FileRestrictionError)
"test.js", expect(() =>
) isToolAllowedForMode("apply_diff", "docs-editor", customModesWithDescription, undefined, {
expect(diffError).toBeInstanceOf(FileRestrictionError) path: "test.js",
expect((diffError as FileRestrictionError).message).toContain("Documentation files only") }),
).toThrow(/Documentation files only/)
// Test that matching files are allowed // Test that matching files are allowed
const mdResult = isToolAllowedForMode( expect(
"write_to_file", isToolAllowedForMode("write_to_file", "docs-editor", customModesWithDescription, undefined, {
"docs-editor", path: "test.md",
customModesWithDescription, }),
undefined, ).toBe(true)
"test.md",
)
expect(mdResult).toBe(true)
const txtResult = isToolAllowedForMode( expect(
"write_to_file", isToolAllowedForMode("write_to_file", "docs-editor", customModesWithDescription, undefined, {
"docs-editor", path: "test.txt",
customModesWithDescription, }),
undefined, ).toBe(true)
"test.txt",
)
expect(txtResult).toBe(true)
}) })
it("allows ask mode to edit markdown files only", () => { it("allows ask mode to edit markdown files only", () => {
// Should allow editing markdown files // Should allow editing markdown files
const mdResult = isToolAllowedForMode("write_to_file", "ask", [], undefined, "test.md") expect(isToolAllowedForMode("write_to_file", "ask", [], undefined, { path: "test.md" })).toBe(true)
expect(mdResult).toBe(true)
// Should allow applying diffs to markdown files // Should allow applying diffs to markdown files
const diffResult = isToolAllowedForMode("apply_diff", "ask", [], undefined, "readme.md") expect(isToolAllowedForMode("apply_diff", "ask", [], undefined, { path: "readme.md" })).toBe(true)
expect(diffResult).toBe(true)
// Should reject non-markdown files // Should reject non-markdown files
const jsResult = isToolAllowedForMode("write_to_file", "ask", [], undefined, "test.js") expect(() => isToolAllowedForMode("write_to_file", "ask", [], undefined, { path: "test.js" })).toThrow(
expect(jsResult).toBeInstanceOf(FileRestrictionError) FileRestrictionError,
expect((jsResult as FileRestrictionError).message).toContain("Markdown files only") )
expect(() => isToolAllowedForMode("write_to_file", "ask", [], undefined, { path: "test.js" })).toThrow(
/Markdown files only/,
)
// Should maintain read capabilities // Should maintain read capabilities
expect(isToolAllowedForMode("read_file", "ask", [])).toBe(true) expect(isToolAllowedForMode("read_file", "ask", [])).toBe(true)
@@ -176,14 +171,18 @@ describe("isToolAllowedForMode", () => {
describe("FileRestrictionError", () => { describe("FileRestrictionError", () => {
it("formats error message with pattern when no description provided", () => { it("formats error message with pattern when no description provided", () => {
const error = new FileRestrictionError("Markdown Editor", "\\.md$") const error = new FileRestrictionError("Markdown Editor", "\\.md$", undefined, "test.js")
expect(error.message).toBe("This mode (Markdown Editor) can only edit files matching the pattern: \\.md$") expect(error.message).toBe(
"This mode (Markdown Editor) can only edit files matching pattern: \\.md$. Got: test.js",
)
expect(error.name).toBe("FileRestrictionError") expect(error.name).toBe("FileRestrictionError")
}) })
it("formats error message with description when provided", () => { it("formats error message with description when provided", () => {
const error = new FileRestrictionError("Markdown Editor", "\\.md$", "Markdown files only") const error = new FileRestrictionError("Markdown Editor", "\\.md$", "Markdown files only", "test.js")
expect(error.message).toBe("This mode (Markdown Editor) can only edit files matching Markdown files only") expect(error.message).toBe(
"This mode (Markdown Editor) can only edit files matching pattern: \\.md$ (Markdown files only). Got: test.js",
)
expect(error.name).toBe("FileRestrictionError") expect(error.name).toBe("FileRestrictionError")
}) })
}) })

View File

@@ -146,9 +146,9 @@ export function isCustomMode(slug: string, customModes?: ModeConfig[]): boolean
// Custom error class for file restrictions // Custom error class for file restrictions
export class FileRestrictionError extends Error { export class FileRestrictionError extends Error {
constructor(mode: string, pattern: string, description?: string) { constructor(mode: string, pattern: string, description: string | undefined, filePath: string) {
super( super(
`This mode (${mode}) can only edit files matching ${description ? description : `the pattern: ${pattern}`}`, `This mode (${mode}) can only edit files matching pattern: ${pattern}${description ? ` (${description})` : ""}. Got: ${filePath}`,
) )
this.name = "FileRestrictionError" this.name = "FileRestrictionError"
} }
@@ -159,8 +159,8 @@ export function isToolAllowedForMode(
modeSlug: string, modeSlug: string,
customModes: ModeConfig[], customModes: ModeConfig[],
toolRequirements?: Record<string, boolean>, toolRequirements?: Record<string, boolean>,
filePath?: string, // Optional file path for checking regex patterns toolParams?: Record<string, any>, // All tool parameters
): boolean | FileRestrictionError { ): boolean {
// Always allow these tools // Always allow these tools
if (ALWAYS_AVAILABLE_TOOLS.includes(tool as any)) { if (ALWAYS_AVAILABLE_TOOLS.includes(tool as any)) {
return true return true
@@ -195,8 +195,10 @@ export function isToolAllowedForMode(
// For the edit group, check file regex if specified // For the edit group, check file regex if specified
if (groupName === "edit" && options.fileRegex) { if (groupName === "edit" && options.fileRegex) {
if (!filePath || !doesFileMatchRegex(filePath, options.fileRegex)) { const filePath = toolParams?.path
return new FileRestrictionError(mode.name, options.fileRegex, options.description) // Only validate regex if a path is provided
if (filePath && !doesFileMatchRegex(filePath, options.fileRegex)) {
throw new FileRestrictionError(mode.name, options.fileRegex, options.description, filePath)
} }
return true return true
} }