Skip to content

Commit 1bf956c

Browse files
feat: Add experimental command filtering mode
This commit introduces a new experimental feature for command validation. When enabled via the experimental settings menu, this feature changes how commands are allowed or denied. The new logic is as follows: - A command is denied if it matches any prefix in the denylist. - A command is denied if it matches any prefix in the allowlist. - Otherwise, the command is approved. This provides a simpler, more restrictive validation mode compared to the default "longest prefix match" strategy. The implementation includes: - A new experimental feature flag `COMMAND_FILTERING_MODE`. - A new validation function `getCommandDecisionAlternate` that implements the new logic. - Integration of the feature flag to switch between validation logics. - New tests for the alternate validation logic and updates to existing tests. Note: I was unable to run the test suite for these changes. I have tested them manually to the extent possible.
1 parent f53fd39 commit 1bf956c

File tree

6 files changed

+1331
-45
lines changed

6 files changed

+1331
-45
lines changed

bun.lock

Lines changed: 1124 additions & 0 deletions
Large diffs are not rendered by default.

src/shared/experiments.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type { AssertEqual, Equals, Keys, Values, ExperimentId, Experiments } from "@roo-code/types"
22

33
export const EXPERIMENT_IDS = {
4+
COMMAND_FILTERING_MODE: "commandFilteringMode",
45
MULTI_FILE_APPLY_DIFF: "multiFileApplyDiff",
56
POWER_STEERING: "powerSteering",
67
PREVENT_FOCUS_DISRUPTION: "preventFocusDisruption",
@@ -16,6 +17,7 @@ interface ExperimentConfig {
1617
}
1718

1819
export const experimentConfigsMap: Record<ExperimentKey, ExperimentConfig> = {
20+
COMMAND_FILTERING_MODE: { enabled: false },
1921
MULTI_FILE_APPLY_DIFF: { enabled: false },
2022
POWER_STEERING: { enabled: false },
2123
PREVENT_FOCUS_DISRUPTION: { enabled: false },

webview-ui/src/components/chat/ChatView.tsx

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
117117
soundEnabled,
118118
soundVolume,
119119
cloudIsAuthenticated,
120+
experiments,
120121
} = useExtensionState()
121122

122123
const messagesRef = useRef(messages)
@@ -1056,9 +1057,14 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
10561057
const getCommandDecisionForMessage = useCallback(
10571058
(message: ClineMessage | undefined): CommandDecision => {
10581059
if (message?.type !== "ask") return "ask_user"
1059-
return getCommandDecision(message.text || "", allowedCommands || [], deniedCommands || [])
1060+
return getCommandDecision(
1061+
message.text || "",
1062+
allowedCommands || [],
1063+
deniedCommands || [],
1064+
experiments,
1065+
)
10601066
},
1061-
[allowedCommands, deniedCommands],
1067+
[allowedCommands, deniedCommands, experiments],
10621068
)
10631069

10641070
// Check if a command message should be auto-approved.
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
import { getCommandDecisionAlternate } from "../command-validation"
2+
3+
describe("getCommandDecisionAlternate", () => {
4+
const allowedCommands = ["npm install", "echo"]
5+
const deniedCommands = ["rm", "sudo"]
6+
7+
it("should deny a command on the denylist", () => {
8+
const decision = getCommandDecisionAlternate("rm -rf /", allowedCommands, deniedCommands)
9+
expect(decision).toBe("auto_deny")
10+
})
11+
12+
it("should deny a command on the allowlist", () => {
13+
const decision = getCommandDecisionAlternate("npm install", allowedCommands, deniedCommands)
14+
expect(decision).toBe("auto_deny")
15+
})
16+
17+
it("should approve a command not on any list", () => {
18+
const decision = getCommandDecisionAlternate("git status", allowedCommands, deniedCommands)
19+
expect(decision).toBe("auto_approve")
20+
})
21+
22+
it("should deny a command chain if one sub-command is on the denylist", () => {
23+
const decision = getCommandDecisionAlternate("git status && rm -rf /", allowedCommands, deniedCommands)
24+
expect(decision).toBe("auto_deny")
25+
})
26+
27+
it("should deny a command chain if one sub-command is on the allowlist", () => {
28+
const decision = getCommandDecisionAlternate("git status && npm install", allowedCommands, deniedCommands)
29+
expect(decision).toBe("auto_deny")
30+
})
31+
32+
it("should approve a command chain if no sub-commands are on any list", () => {
33+
const decision = getCommandDecisionAlternate("git status && git commit", allowedCommands, deniedCommands)
34+
expect(decision).toBe("auto_approve")
35+
})
36+
37+
it("should approve an empty command", () => {
38+
const decision = getCommandDecisionAlternate("", allowedCommands, deniedCommands)
39+
expect(decision).toBe("auto_approve")
40+
})
41+
42+
it("should approve a command if both lists are empty", () => {
43+
const decision = getCommandDecisionAlternate("any command", [], [])
44+
expect(decision).toBe("auto_approve")
45+
})
46+
47+
it("should handle undefined denylist", () => {
48+
const decision = getCommandDecisionAlternate("npm install", allowedCommands, undefined)
49+
expect(decision).toBe("auto_deny")
50+
})
51+
52+
it("should handle undefined allowlist", () => {
53+
const decision = getCommandDecisionAlternate("rm -rf /", [], deniedCommands)
54+
expect(decision).toBe("auto_deny")
55+
})
56+
})

webview-ui/src/utils/__tests__/command-validation.spec.ts

Lines changed: 80 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
createCommandValidator,
1414
containsSubshell,
1515
} from "../command-validation"
16+
import { experimentDefault } from "@roo/experiments"
1617

1718
describe("Command Validation", () => {
1819
describe("parseCommand", () => {
@@ -765,87 +766,114 @@ describe("Unified Command Decision Functions", () => {
765766
describe("getCommandDecision", () => {
766767
const allowedCommands = ["npm", "echo"]
767768
const deniedCommands = ["npm test"]
769+
const experiments = experimentDefault
768770

769771
it("returns auto_approve for commands with all sub-commands auto-approved", () => {
770-
expect(getCommandDecision("npm install", allowedCommands, deniedCommands)).toBe("auto_approve")
771-
expect(getCommandDecision("npm install && echo done", allowedCommands, deniedCommands)).toBe("auto_approve")
772+
expect(getCommandDecision("npm install", allowedCommands, deniedCommands, experiments)).toBe("auto_approve")
773+
expect(getCommandDecision("npm install && echo done", allowedCommands, deniedCommands, experiments)).toBe(
774+
"auto_approve",
775+
)
772776
})
773777

774778
it("returns auto_deny for commands with any sub-command auto-denied", () => {
775-
expect(getCommandDecision("npm test", allowedCommands, deniedCommands)).toBe("auto_deny")
776-
expect(getCommandDecision("npm install && npm test", allowedCommands, deniedCommands)).toBe("auto_deny")
779+
expect(getCommandDecision("npm test", allowedCommands, deniedCommands, experiments)).toBe("auto_deny")
780+
expect(getCommandDecision("npm install && npm test", allowedCommands, deniedCommands, experiments)).toBe(
781+
"auto_deny",
782+
)
777783
})
778784

779785
it("returns ask_user for commands with mixed or unknown sub-commands", () => {
780-
expect(getCommandDecision("dangerous", allowedCommands, deniedCommands)).toBe("ask_user")
781-
expect(getCommandDecision("npm install && dangerous", allowedCommands, deniedCommands)).toBe("ask_user")
786+
expect(getCommandDecision("dangerous", allowedCommands, deniedCommands, experiments)).toBe("ask_user")
787+
expect(getCommandDecision("npm install && dangerous", allowedCommands, deniedCommands, experiments)).toBe(
788+
"ask_user",
789+
)
782790
})
783791

784792
it("properly validates subshell commands by checking all parsed commands", () => {
785793
// Subshells without denied prefixes should be auto-approved if all commands are allowed
786-
expect(getCommandDecision("npm install $(echo test)", allowedCommands, deniedCommands)).toBe("auto_approve")
787-
expect(getCommandDecision("npm install `echo test`", allowedCommands, deniedCommands)).toBe("auto_approve")
794+
expect(getCommandDecision("npm install $(echo test)", allowedCommands, deniedCommands, experiments)).toBe(
795+
"auto_approve",
796+
)
797+
expect(getCommandDecision("npm install `echo test`", allowedCommands, deniedCommands, experiments)).toBe(
798+
"auto_approve",
799+
)
788800

789801
// Subshells with denied prefixes should be auto-denied
790-
expect(getCommandDecision("npm install $(npm test)", allowedCommands, deniedCommands)).toBe("auto_deny")
791-
expect(getCommandDecision("npm install `npm test --coverage`", allowedCommands, deniedCommands)).toBe(
802+
expect(getCommandDecision("npm install $(npm test)", allowedCommands, deniedCommands, experiments)).toBe(
792803
"auto_deny",
793804
)
805+
expect(
806+
getCommandDecision("npm install `npm test --coverage`", allowedCommands, deniedCommands, experiments),
807+
).toBe("auto_deny")
794808

795809
// Main command with denied prefix should also be auto-denied
796-
expect(getCommandDecision("npm test $(echo hello)", allowedCommands, deniedCommands)).toBe("auto_deny")
810+
expect(getCommandDecision("npm test $(echo hello)", allowedCommands, deniedCommands, experiments)).toBe(
811+
"auto_deny",
812+
)
797813
})
798814

799815
it("properly validates subshell commands when no denylist is present", () => {
800-
expect(getCommandDecision("npm install $(echo test)", allowedCommands)).toBe("auto_approve")
801-
expect(getCommandDecision("npm install `echo test`", allowedCommands)).toBe("auto_approve")
816+
expect(getCommandDecision("npm install $(echo test)", allowedCommands, undefined, experiments)).toBe(
817+
"auto_approve",
818+
)
819+
expect(getCommandDecision("npm install `echo test`", allowedCommands, undefined, experiments)).toBe(
820+
"auto_approve",
821+
)
802822
})
803823

804824
it("handles empty command", () => {
805-
expect(getCommandDecision("", allowedCommands, deniedCommands)).toBe("auto_approve")
825+
expect(getCommandDecision("", allowedCommands, deniedCommands, experiments)).toBe("auto_approve")
806826
})
807827

808828
it("handles complex chained commands", () => {
809829
// All sub-commands auto-approved
810-
expect(getCommandDecision("npm install && echo success && npm run build", ["npm", "echo"], [])).toBe(
811-
"auto_approve",
812-
)
830+
expect(
831+
getCommandDecision("npm install && echo success && npm run build", ["npm", "echo"], [], experiments),
832+
).toBe("auto_approve")
813833

814834
// One sub-command auto-denied
815-
expect(getCommandDecision("npm install && npm test && echo done", allowedCommands, deniedCommands)).toBe(
816-
"auto_deny",
817-
)
835+
expect(
836+
getCommandDecision("npm install && npm test && echo done", allowedCommands, deniedCommands, experiments),
837+
).toBe("auto_deny")
818838

819839
// Mixed decisions (some ask_user)
820-
expect(getCommandDecision("npm install && dangerous && echo done", allowedCommands, deniedCommands)).toBe(
821-
"ask_user",
822-
)
840+
expect(
841+
getCommandDecision(
842+
"npm install && dangerous && echo done",
843+
allowedCommands,
844+
deniedCommands,
845+
experiments,
846+
),
847+
).toBe("ask_user")
823848
})
824849

825850
it("demonstrates the three-tier system comprehensively", () => {
826851
const allowed = ["npm"]
827852
const denied = ["npm test"]
828853

829854
// Auto-approved: all sub-commands match allowlist, none match denylist
830-
expect(getCommandDecision("npm install", allowed, denied)).toBe("auto_approve")
831-
expect(getCommandDecision("npm install && npm run build", allowed, denied)).toBe("auto_approve")
855+
expect(getCommandDecision("npm install", allowed, denied, experiments)).toBe("auto_approve")
856+
expect(getCommandDecision("npm install && npm run build", allowed, denied, experiments)).toBe(
857+
"auto_approve",
858+
)
832859

833860
// Auto-denied: any sub-command matches denylist
834-
expect(getCommandDecision("npm test", allowed, denied)).toBe("auto_deny")
835-
expect(getCommandDecision("npm install && npm test", allowed, denied)).toBe("auto_deny")
861+
expect(getCommandDecision("npm test", allowed, denied, experiments)).toBe("auto_deny")
862+
expect(getCommandDecision("npm install && npm test", allowed, denied, experiments)).toBe("auto_deny")
836863

837864
// Ask user: commands that match neither list
838-
expect(getCommandDecision("dangerous", allowed, denied)).toBe("ask_user")
839-
expect(getCommandDecision("npm install && dangerous", allowed, denied)).toBe("ask_user")
865+
expect(getCommandDecision("dangerous", allowed, denied, experiments)).toBe("ask_user")
866+
expect(getCommandDecision("npm install && dangerous", allowed, denied, experiments)).toBe("ask_user")
840867
})
841868
})
842869

843870
describe("CommandValidator Integration Tests", () => {
871+
const experiments = experimentDefault
844872
describe("CommandValidator Class", () => {
845873
let validator: CommandValidator
846874

847875
beforeEach(() => {
848-
validator = new CommandValidator(["npm", "echo", "git"], ["npm test", "git push"])
876+
validator = new CommandValidator(["npm", "echo", "git"], ["npm test", "git push"], experiments)
849877
})
850878

851879
describe("Basic validation methods", () => {
@@ -872,7 +900,7 @@ describe("Unified Command Decision Functions", () => {
872900

873901
describe("Configuration management", () => {
874902
it("updates command lists", () => {
875-
validator.updateCommandLists(["echo"], ["echo hello"])
903+
validator.updateCommandLists(["echo"], ["echo hello"], experiments)
876904

877905
expect(validator.validateCommand("npm install")).toBe("ask_user")
878906
expect(validator.validateCommand("echo world")).toBe("auto_approve")
@@ -886,7 +914,7 @@ describe("Unified Command Decision Functions", () => {
886914
})
887915

888916
it("handles undefined denied commands", () => {
889-
const validatorNoDeny = new CommandValidator(["npm"])
917+
const validatorNoDeny = new CommandValidator(["npm"], undefined, experiments)
890918
const lists = validatorNoDeny.getCommandLists()
891919
expect(lists.allowedCommands).toEqual(["npm"])
892920
expect(lists.deniedCommands).toBeUndefined()
@@ -953,13 +981,13 @@ describe("Unified Command Decision Functions", () => {
953981
it("detects if rules are configured", () => {
954982
expect(validator.hasRules()).toBe(true)
955983

956-
const emptyValidator = new CommandValidator([], [])
984+
const emptyValidator = new CommandValidator([], [], experiments)
957985
expect(emptyValidator.hasRules()).toBe(false)
958986

959-
const allowOnlyValidator = new CommandValidator(["npm"], [])
987+
const allowOnlyValidator = new CommandValidator(["npm"], [], experiments)
960988
expect(allowOnlyValidator.hasRules()).toBe(true)
961989

962-
const denyOnlyValidator = new CommandValidator([], ["rm"])
990+
const denyOnlyValidator = new CommandValidator([], ["rm"], experiments)
963991
expect(denyOnlyValidator.hasRules()).toBe(true)
964992
})
965993

@@ -972,7 +1000,7 @@ describe("Unified Command Decision Functions", () => {
9721000
})
9731001

9741002
it("detects wildcard configuration", () => {
975-
const wildcardValidator = new CommandValidator(["*", "npm"], ["rm"])
1003+
const wildcardValidator = new CommandValidator(["*", "npm"], ["rm"], experiments)
9761004
const stats = wildcardValidator.getStats()
9771005
expect(stats.hasWildcard).toBe(true)
9781006
})
@@ -999,23 +1027,25 @@ describe("Unified Command Decision Functions", () => {
9991027
})
10001028

10011029
describe("Factory function", () => {
1030+
const experiments = experimentDefault
10021031
it("creates validator instances correctly", () => {
1003-
const validator = createCommandValidator(["npm"], ["rm"])
1032+
const validator = createCommandValidator(["npm"], ["rm"], experiments)
10041033
expect(validator).toBeInstanceOf(CommandValidator)
10051034
expect(validator.validateCommand("npm test")).toBe("auto_approve")
10061035
expect(validator.validateCommand("rm file")).toBe("auto_deny")
10071036
})
10081037

10091038
it("handles optional denied commands", () => {
1010-
const validator = createCommandValidator(["npm"])
1039+
const validator = createCommandValidator(["npm"], undefined, experiments)
10111040
expect(validator.validateCommand("npm test")).toBe("auto_approve")
10121041
expect(validator.validateCommand("dangerous")).toBe("ask_user")
10131042
})
10141043
})
10151044

10161045
describe("Subshell edge cases", () => {
1046+
const experiments = experimentDefault
10171047
it("handles multiple subshells correctly", () => {
1018-
const validator = createCommandValidator(["echo", "npm"], ["rm", "sudo"])
1048+
const validator = createCommandValidator(["echo", "npm"], ["rm", "sudo"], experiments)
10191049

10201050
// Multiple subshells, none with denied prefixes but subshell commands not in allowlist
10211051
// parseCommand extracts subshells as separate commands, so date and pwd are not allowed
@@ -1030,7 +1060,11 @@ describe("Unified Command Decision Functions", () => {
10301060
})
10311061

10321062
it("handles complex commands with subshells", () => {
1033-
const validator = createCommandValidator(["npm", "git", "echo"], ["git push", "npm publish"])
1063+
const validator = createCommandValidator(
1064+
["npm", "git", "echo"],
1065+
["git push", "npm publish"],
1066+
experiments,
1067+
)
10341068

10351069
// Subshell with allowed command - git status is extracted as separate command
10361070
// Since "git status" starts with "git" which is allowed, it's approved
@@ -1049,13 +1083,15 @@ describe("Unified Command Decision Functions", () => {
10491083
})
10501084

10511085
describe("Real-world integration scenarios", () => {
1086+
const experiments = experimentDefault
10521087
describe("Development workflow validation", () => {
10531088
let devValidator: CommandValidator
10541089

10551090
beforeEach(() => {
10561091
devValidator = createCommandValidator(
10571092
["npm", "git", "echo", "ls", "cat"],
10581093
["git push", "rm", "sudo", "npm publish"],
1094+
experiments,
10591095
)
10601096
})
10611097

@@ -1106,6 +1142,7 @@ describe("Unified Command Decision Functions", () => {
11061142
prodValidator = createCommandValidator(
11071143
["ls", "cat", "grep", "tail"],
11081144
["*"], // Deny everything by default
1145+
experiments,
11091146
)
11101147
})
11111148

@@ -1132,6 +1169,7 @@ describe("Unified Command Decision Functions", () => {
11321169
complexValidator = createCommandValidator(
11331170
["git", "git push", "git push --dry-run", "npm", "npm test"],
11341171
["git push", "npm test --coverage"],
1172+
experiments,
11351173
)
11361174
})
11371175

@@ -1166,7 +1204,7 @@ describe("Unified Command Decision Functions", () => {
11661204
const largeAllowList = Array.from({ length: 1000 }, (_, i) => `command${i}`)
11671205
const largeDenyList = Array.from({ length: 500 }, (_, i) => `dangerous${i}`)
11681206

1169-
const largeValidator = createCommandValidator(largeAllowList, largeDenyList)
1207+
const largeValidator = createCommandValidator(largeAllowList, largeDenyList, experiments)
11701208

11711209
// Should still work efficiently
11721210
expect(largeValidator.isAutoApproved("command500 --flag")).toBe(true)
@@ -1175,7 +1213,7 @@ describe("Unified Command Decision Functions", () => {
11751213
})
11761214

11771215
it("handles batch validation efficiently", () => {
1178-
const batchValidator = createCommandValidator(["npm"], ["rm"])
1216+
const batchValidator = createCommandValidator(["npm"], ["rm"], experiments)
11791217
const commands = Array.from({ length: 100 }, (_, i) => `npm test${i}`)
11801218
const results = batchValidator.validateCommands(commands)
11811219

0 commit comments

Comments
 (0)