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
95 changes: 82 additions & 13 deletions src/shared/__tests__/modes.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,24 +93,37 @@ describe("isToolAllowedForMode", () => {
).toThrow(/\\.css\$/)
})

it("handles partial streaming cases (path only, no content/diff)", () => {
// Should allow path-only for matching files (no validation yet since content/diff not provided)
expect(
it("enforces file restrictions even when only path is provided", () => {
// File restrictions should be enforced immediately when path is provided, regardless of content/diff
expect(() =>
isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, {
path: "test.js",
path: "test.js", // Should be rejected - doesn't match .md pattern
}),
).toBe(true)
).toThrow(FileRestrictionError)

expect(
expect(() =>
isToolAllowedForMode("apply_diff", "markdown-editor", customModes, undefined, {
path: "test.js",
path: "test.js", // Should be rejected - doesn't match .md pattern
}),
).toThrow(FileRestrictionError)

// Should reject non-markdown files for architect mode too
expect(() =>
isToolAllowedForMode("write_to_file", "architect", [], undefined, {
path: "test.js", // Should be rejected - doesn't match .md pattern
}),
).toThrow(FileRestrictionError)

// But should allow matching files
expect(
isToolAllowedForMode("write_to_file", "markdown-editor", customModes, undefined, {
path: "test.md", // Should be allowed - matches .md pattern
}),
).toBe(true)

// Should allow path-only for architect mode too
expect(
isToolAllowedForMode("write_to_file", "architect", [], undefined, {
path: "test.js",
path: "README.md", // Should be allowed - matches .md pattern
}),
).toBe(true)
})
Expand Down Expand Up @@ -203,12 +216,12 @@ describe("isToolAllowedForMode", () => {
}),
).toBe(true)

// Test partial streaming cases
expect(
// Test that non-matching files are rejected even with path only
expect(() =>
isToolAllowedForMode("write_to_file", "docs-editor", customModesWithDescription, undefined, {
path: "test.js",
path: "test.js", // Should be rejected - doesn't match pattern
}),
).toBe(true)
).toThrow(FileRestrictionError)
})

it("allows architect mode to edit markdown files only", () => {
Expand Down Expand Up @@ -249,6 +262,62 @@ describe("isToolAllowedForMode", () => {
})
})

describe("file restriction bug reproduction", () => {
it("should enforce file restrictions even when only path is provided (reproduces issue #4732)", () => {
// This test reproduces the bug where mode permissions are ignored when only path is provided
// The current implementation incorrectly allows this, but it should be rejected

// Test with custom architect mode that has restricted file patterns
const customArchitectMode: ModeConfig[] = [
{
slug: "architect",
name: "🏗️ Architect",
roleDefinition: "You are an architect",
groups: [
"read",
[
"edit",
{
fileRegex: "\\.(md|yaml|json|toml|.+ignore|roomodes)$",
description: "Documentation and configs",
},
],
"browser",
"command",
"mcp",
],
},
]

// This should throw an error because PowerShell scripts (.ps1) don't match the allowed pattern
// But currently it returns true due to the bug
expect(() =>
isToolAllowedForMode("write_to_file", "architect", customArchitectMode, undefined, {
path: "script.ps1", // PowerShell script - should be rejected
}),
).toThrow(FileRestrictionError)

expect(() =>
isToolAllowedForMode("apply_diff", "architect", customArchitectMode, undefined, {
path: "script.ps1", // PowerShell script - should be rejected
}),
).toThrow(FileRestrictionError)

// These should still work (allowed file types)
expect(
isToolAllowedForMode("write_to_file", "architect", customArchitectMode, undefined, {
path: "README.md",
}),
).toBe(true)

expect(
isToolAllowedForMode("write_to_file", "architect", customArchitectMode, undefined, {
path: "config.yaml",
}),
).toBe(true)
})
})

it("handles non-existent modes", () => {
expect(isToolAllowedForMode("write_to_file", "non-existent", customModes)).toBe(false)
})
Expand Down
6 changes: 1 addition & 5 deletions src/shared/modes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -258,11 +258,7 @@ export function isToolAllowedForMode(
// For the edit group, check file regex if specified
if (groupName === "edit" && options.fileRegex) {
const filePath = toolParams?.path
if (
filePath &&
(toolParams.diff || toolParams.content || toolParams.operations) &&
!doesFileMatchRegex(filePath, options.fileRegex)
) {
if (filePath && !doesFileMatchRegex(filePath, options.fileRegex)) {
throw new FileRestrictionError(mode.name, options.fileRegex, options.description, filePath)
}
}
Expand Down