Skip to content

Commit 38ed84b

Browse files
hannesrudolphhannesrudolph
committed
fix: consolidate command parsing logic and integrate security warnings
- Created shared command-parser.ts to eliminate duplicate parsing logic - Integrated detectSecurityIssues to display warnings in the UI - Made command suggestions configurable (only show when restrictions are enabled) - Added comprehensive tests for the new command parser - Updated existing tests to handle the new behavior
1 parent 4b257fc commit 38ed84b

File tree

15 files changed

+3638
-261
lines changed

15 files changed

+3638
-261
lines changed

‎.roo/temp/pr-5798/architecture-review.md‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
## Architecture Review for PR #5798
2+
3+
### Module Boundaries
4+
5+
**✅ GOOD: Clear separation of concerns**
6+
7+
- The command permission UI logic is properly separated into dedicated components:
8+
- `CommandExecution.tsx` - Handles command execution display and permission management
9+
- `CommandPatternSelector.tsx` - UI component for pattern selection
10+
- `commandPatterns.ts` - Business logic for pattern extraction and validation
11+
12+
**✅ GOOD: Proper layering**
13+
14+
- UI components (`CommandExecution`, `CommandPatternSelector`) depend on utility functions (`commandPatterns.ts`)
15+
- State management flows through proper channels (ExtensionStateContext → Components → VSCode messages)
16+
- No circular dependencies detected
17+
18+
**⚠️ CONCERN: Overlapping responsibilities**
19+
20+
- Both `command-validation.ts` and `commandPatterns.ts` handle command parsing
21+
- `command-validation.ts` uses shell-quote for validation logic
22+
- `commandPatterns.ts` also uses shell-quote for pattern extraction
23+
- This creates potential for divergent parsing behavior
24+
25+
### Dependency Analysis
26+
27+
**✅ GOOD: Appropriate dependency choice**
28+
29+
- `shell-quote` (v1.8.2) is a well-established library for shell command parsing
30+
- Already used in `command-validation.ts`, so no new dependency introduced
31+
- Lightweight and focused on a single responsibility
32+
33+
**⚠️ CONCERN: Dependency duplication**
34+
35+
- Both runtime dependencies and devDependencies include shell-quote types
36+
- Consider if `@types/shell-quote` should only be in devDependencies
37+
38+
### Architectural Concerns
39+
40+
**❌ ISSUE: Inconsistent command parsing**
41+
42+
- Two separate parsing implementations:
43+
1. `parseCommand()` in `command-validation.ts` - Complex parsing with subshell handling
44+
2. `parse()` usage in `commandPatterns.ts` - Simpler pattern extraction
45+
- Risk of commands being parsed differently for validation vs. pattern extraction
46+
47+
**✅ GOOD: State synchronization**
48+
49+
- Proper flow: UI → ExtensionState → VSCode messages → Backend persistence
50+
- Uses established patterns for state updates (`setAllowedCommands`, `setDeniedCommands`)
51+
- Backend properly validates and sanitizes command arrays
52+
53+
**⚠️ CONCERN: Security considerations**
54+
55+
- `commandPatterns.ts` removes subshells before pattern extraction (good)
56+
- However, the security warning detection (`detectSecurityIssues`) is not used in the UI
57+
- Pattern extraction might miss edge cases that the validation logic catches
58+
59+
**✅ GOOD: Internationalization support**
60+
61+
- All UI strings use i18n keys
62+
- 17 translation files updated consistently
63+
- Follows established i18n patterns
64+
65+
### Impact on System Architecture
66+
67+
**Integration with existing permission system:**
68+
69+
- ✅ Properly integrates with existing `allowedCommands` and `deniedCommands` state
70+
- ✅ Uses the same validation logic (`getCommandDecision`) for auto-approval/denial
71+
- ✅ Maintains backward compatibility with existing permission settings
72+
73+
**UI/UX consistency:**
74+
75+
- ✅ Follows existing UI patterns (VSCode toolkit components, Tailwind styling)
76+
- ✅ Integrates seamlessly into the command execution flow
77+
- ✅ Provides immediate visual feedback for permission states
78+
79+
**Performance considerations:**
80+
81+
- ✅ Pattern extraction is memoized with `useMemo`
82+
- ✅ No unnecessary re-renders (proper React optimization)
83+
- ⚠️ Pattern extraction runs on every command - consider caching for repeated commands
84+
85+
### Consistency with Architectural Patterns
86+
87+
**✅ GOOD: Follows established patterns**
88+
89+
- Component structure matches other chat components
90+
- State management through context follows app conventions
91+
- Message passing to extension follows established patterns
92+
93+
**✅ GOOD: Test coverage**
94+
95+
- Comprehensive unit tests for both components and utilities
96+
- Tests cover edge cases and user interactions
97+
- Follows existing test patterns
98+
99+
### Recommendations
100+
101+
1. **Consolidate command parsing logic**
102+
103+
- Extract common parsing logic into a shared utility
104+
- Ensure `command-validation.ts` and `commandPatterns.ts` use the same parser
105+
- This prevents divergent behavior between validation and pattern extraction
106+
107+
2. **Add pattern caching**
108+
109+
- Cache extracted patterns for recently executed commands
110+
- Reduces redundant parsing operations
111+
112+
3. **Enhance security integration**
113+
114+
- Use `detectSecurityIssues` from `commandPatterns.ts` to show warnings in UI
115+
- Ensure pattern extraction doesn't bypass security checks
116+
117+
4. **Consider extracting pattern management**
118+
119+
- Create a dedicated service/manager for command patterns
120+
- Would centralize pattern extraction, caching, and persistence
121+
122+
5. **Add integration tests**
123+
- Test the full flow: UI interaction → state update → backend persistence
124+
- Ensure pattern extraction and validation remain synchronized
125+
126+
### Overall Assessment
127+
128+
The PR demonstrates good architectural practices with clear module boundaries and proper separation of concerns. The main architectural concern is the duplication of command parsing logic, which could lead to inconsistent behavior. The integration with the existing permission system is well-designed and maintains backward compatibility. With the recommended improvements, particularly consolidating the parsing logic, this feature would be a solid addition to the codebase.

‎.roo/temp/pr-5798/comments.json‎

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
[
2+
{
3+
"author": { "login": "delve-auditor" },
4+
"authorAssociation": "NONE",
5+
"body": "✅ **No security or compliance issues detected.** Reviewed everything up to 47259df9547fe38e0b49d7fcb6e3eef84223212a.\n\n\n\u003cdetails\u003e\n\u003csummary\u003eSecurity Overview\u003c/summary\u003e\n\n- 🔎 **Scanned files:** 24 changed file(s)\n\n\n\u003c/details\u003e\n\n\u003cdetails\u003e\n\u003csummary\u003eDetected Code Changes\u003c/summary\u003e\n\nThe diff is too large to display a summary of code changes.\n\n\u003c/details\u003e\n\nReply to this PR with `@delve-auditor` followed by a description of what change you want and we'll auto-submit a change to this PR to implement it.",
6+
"createdAt": "2025-07-17T00:30:34Z",
7+
"id": "IC_kwDONIq5lM63sr_k",
8+
"includesCreatedEdit": true,
9+
"isMinimized": false,
10+
"minimizedReason": "",
11+
"reactionGroups": [],
12+
"url": "https://github.com/RooCodeInc/Roo-Code/pull/5798#issuecomment-3081945060",
13+
"viewerDidAuthor": false
14+
},
15+
{
16+
"author": { "login": "daniel-lxs" },
17+
"authorAssociation": "COLLABORATOR",
18+
"body": "@roomote-agent Use the switch_mode tool to switch to pr-fixer and address the reviews on this PR, commit the changes to this PR, do not create a new one",
19+
"createdAt": "2025-07-21T16:38:47Z",
20+
"id": "IC_kwDONIq5lM64oHNJ",
21+
"includesCreatedEdit": false,
22+
"isMinimized": false,
23+
"minimizedReason": "",
24+
"reactionGroups": [],
25+
"url": "https://github.com/RooCodeInc/Roo-Code/pull/5798#issuecomment-3097523017",
26+
"viewerDidAuthor": false
27+
},
28+
{
29+
"author": { "login": "roomote" },
30+
"authorAssociation": "NONE",
31+
"body": "Hi @daniel-lxs! I see your request to address the reviews on this PR. Ill now switch to pr-fixer mode and work on addressing the feedback from the code reviews. Ill commit the changes directly to this PR branch as requested.",
32+
"createdAt": "2025-07-21T17:03:24Z",
33+
"id": "IC_kwDONIq5lM64odzj",
34+
"includesCreatedEdit": false,
35+
"isMinimized": true,
36+
"minimizedReason": "RESOLVED",
37+
"reactionGroups": [],
38+
"url": "https://github.com/RooCodeInc/Roo-Code/pull/5798#issuecomment-3097615587",
39+
"viewerDidAuthor": false
40+
},
41+
{
42+
"author": { "login": "roomote" },
43+
"authorAssociation": "NONE",
44+
"body": "Hi @daniel-lxs! I've successfully addressed all the code review feedback on this PR:\n\n✅ **Fixed Issues:**\n1. Replaced the unused `showSuggestions` state variable with a `SHOW_SUGGESTIONS` constant\n2. Fixed the `parseCommandAndOutput` import name consistency issue\n3. Added test coverage for the fallback case where `enhanced.command === text`\n\n✅ **All CI checks are passing** - The changes have been committed and pushed to the PR branch.\n\nThe PR is now ready for re-review. Thanks for your patience!",
45+
"createdAt": "2025-07-21T17:27:39Z",
46+
"id": "IC_kwDONIq5lM64our9",
47+
"includesCreatedEdit": false,
48+
"isMinimized": true,
49+
"minimizedReason": "RESOLVED",
50+
"reactionGroups": [],
51+
"url": "https://github.com/RooCodeInc/Roo-Code/pull/5798#issuecomment-3097684733",
52+
"viewerDidAuthor": false
53+
},
54+
{
55+
"author": { "login": "daniel-lxs" },
56+
"authorAssociation": "COLLABORATOR",
57+
"body": "@roomote-agent please also address the review by Copilot by updating this PR, do not create a new PR",
58+
"createdAt": "2025-07-21T18:20:20Z",
59+
"id": "IC_kwDONIq5lM64pm7T",
60+
"includesCreatedEdit": false,
61+
"isMinimized": true,
62+
"minimizedReason": "RESOLVED",
63+
"reactionGroups": [],
64+
"url": "https://github.com/RooCodeInc/Roo-Code/pull/5798#issuecomment-3097915091",
65+
"viewerDidAuthor": false
66+
},
67+
{
68+
"author": { "login": "roomote" },
69+
"authorAssociation": "NONE",
70+
"body": "Hi @daniel-lxs! I see your request to address the Copilot review feedback. I'll work on fixing the following issues identified by Copilot:\n\n1. Convert the unused `showSuggestions` state variable to a constant\n2. Fix the `parseCommandAndOutput` import issue\n3. Add test coverage for the fallback case where `enhanced.command === text`\n\nI'll update this PR with the fixes shortly.",
71+
"createdAt": "2025-07-21T18:22:14Z",
72+
"id": "IC_kwDONIq5lM64poKm",
73+
"includesCreatedEdit": false,
74+
"isMinimized": true,
75+
"minimizedReason": "RESOLVED",
76+
"reactionGroups": [],
77+
"url": "https://github.com/RooCodeInc/Roo-Code/pull/5798#issuecomment-3097920166",
78+
"viewerDidAuthor": false
79+
},
80+
{
81+
"author": { "login": "roomote" },
82+
"authorAssociation": "NONE",
83+
"body": "Hi @daniel-lxs! I've reviewed the Copilot feedback and found that all the issues have already been addressed in previous commits:\n\n✅ **Issue 1**: The `showSuggestions` state variable has been converted to a constant `SHOW_SUGGESTIONS` (fixed in commit 94efb1a12)\n✅ **Issue 2**: The `parseCommandAndOutput` function is properly imported from `commandPatterns.ts` and used correctly\n✅ **Issue 3**: Test coverage for the fallback case already exists in the test file (lines 467-489)\n\nAll CI checks are passing. The PR appears to be ready for review. The Copilot review seems to be based on an older version of the code before these fixes were applied.",
84+
"createdAt": "2025-07-21T18:23:18Z",
85+
"id": "IC_kwDONIq5lM64po0H",
86+
"includesCreatedEdit": false,
87+
"isMinimized": true,
88+
"minimizedReason": "RESOLVED",
89+
"reactionGroups": [{ "content": "THUMBS_UP", "users": { "totalCount": 1 } }],
90+
"url": "https://github.com/RooCodeInc/Roo-Code/pull/5798#issuecomment-3097922823",
91+
"viewerDidAuthor": false
92+
},
93+
{
94+
"author": { "login": "daniel-lxs" },
95+
"authorAssociation": "COLLABORATOR",
96+
"body": "I am seeing some odd behavior with certain commands:\r\n\u003cimg width=\"413\" height=\"244\" alt=\"image\" src=\"https://github.com/user-attachments/assets/e76efbd8-40a6-4d6a-86f4-7c2298db92b8\" /\u003e\r\n\u003cimg width=\"411\" height=\"204\" alt=\"image\" src=\"https://github.com/user-attachments/assets/8d422a2e-e547-499c-86eb-a04aac3feefd\" /\u003e\r\n\r\nNot sure what is going on but it seems like for the first image the \"341\" appeared when the result of the command was returned, replacing the correct command prefix.\r\n\r\nShould the command be parsed from the actual tool call rather than the terminal context?\r\n\r\n",
97+
"createdAt": "2025-07-21T22:05:57Z",
98+
"id": "IC_kwDONIq5lM64vjai",
99+
"includesCreatedEdit": false,
100+
"isMinimized": true,
101+
"minimizedReason": "RESOLVED",
102+
"reactionGroups": [],
103+
"url": "https://github.com/RooCodeInc/Roo-Code/pull/5798#issuecomment-3099473570",
104+
"viewerDidAuthor": false
105+
},
106+
{
107+
"author": { "login": "hannesrudolph" },
108+
"authorAssociation": "MEMBER",
109+
"body": "## Critical Issues Found\r\n\r\nThis PR duplicates significant existing functionality and introduces architectural concerns that need to be addressed:\r\n\r\n### 1. Major Code Redundancy\r\n\r\n- **Pattern Extraction**: The new `extractCommandPatterns()` duplicates the existing `parseCommand()` function with inconsistent behavior\r\n",
110+
"createdAt": "2025-07-22T23:55:44Z",
111+
"id": "IC_kwDONIq5lM65FUFH",
112+
"includesCreatedEdit": true,
113+
"isMinimized": false,
114+
"minimizedReason": "",
115+
"reactionGroups": [],
116+
"url": "https://github.com/RooCodeInc/Roo-Code/pull/5798#issuecomment-3105177927",
117+
"viewerDidAuthor": true
118+
},
119+
{
120+
"author": { "login": "hannesrudolph" },
121+
"authorAssociation": "MEMBER",
122+
"body": "@roomote-agent the parser should be simplified to ```import { parse } from 'shell-quote';\r\n\r\nfunction extractPatterns(cmdStr) {\r\n const patterns = new Set();\r\n \r\n const parsed = parse(cmdStr);\r\n \r\n const commandSeparators = new Set(['|', '\u0026\u0026', '||', ';']);\r\n let current = [];\r\n for (const token of parsed) {\r\n if (typeof token === 'object' \u0026\u0026 token.op \u0026\u0026 commandSeparators.has(token.op)) {\r\n if (current.length) processCommand(current, patterns);\r\n current = [];\r\n } else {\r\n current.push(token);\r\n }\r\n }\r\n\r\n if (current.length) processCommand(current, patterns);\r\n \r\n return patterns;\r\n}``` and the filter should be simplified to ```function processCommand(cmd, patterns) {\r\n if (!cmd.length || typeof cmd[0] !== 'string') return;\r\n \r\n const mainCmd = cmd[0];\r\n patterns.add(mainCmd);\r\n \r\n const breakingExps = [ /^-/, /[\\\\/:.~ ]/, ];\r\n \r\n for (let i = 1; i \u003c cmd.length; i++) {\r\n const arg = cmd[i];\r\n\r\n if (typeof arg !== 'string' || breakingExps.some(re =\u003e re.test(arg))) break;\r\n \r\n const pattern = cmd.slice(0, i + 1).join(' ');\r\n patterns.add(pattern);\r\n }\r\n}```. That should be combined with the shell-quote library. ",
123+
"createdAt": "2025-07-23T14:37:00Z",
124+
"id": "IC_kwDONIq5lM65To9T",
125+
"includesCreatedEdit": false,
126+
"isMinimized": false,
127+
"minimizedReason": "",
128+
"reactionGroups": [],
129+
"url": "https://github.com/RooCodeInc/Roo-Code/pull/5798#issuecomment-3108933459",
130+
"viewerDidAuthor": true
131+
},
132+
{
133+
"author": { "login": "roomote" },
134+
"authorAssociation": "NONE",
135+
"body": "Hi @hannesrudolph, I see your request to simplify the parser and filter functions using shell-quote. I'll implement these changes now. Working on it!",
136+
"createdAt": "2025-07-23T14:38:54Z",
137+
"id": "IC_kwDONIq5lM65TqxD",
138+
"includesCreatedEdit": false,
139+
"isMinimized": false,
140+
"minimizedReason": "",
141+
"reactionGroups": [],
142+
"url": "https://github.com/RooCodeInc/Roo-Code/pull/5798#issuecomment-3108940867",
143+
"viewerDidAuthor": false
144+
},
145+
{
146+
"author": { "login": "roomote" },
147+
"authorAssociation": "NONE",
148+
"body": "Hi @hannesrudolph, I've implemented the simplified parser as requested! \n\n## Changes made:\n- ✅ Replaced custom parsing logic with the `shell-quote` library\n- ✅ Implemented the simplified `extractPatterns` function exactly as specified\n- ✅ Implemented the simplified `processCommand` helper function exactly as specified\n- ✅ Removed dependency on `parseCommand` from command-validation\n- ✅ All 60 existing tests pass without modification\n\n## Key improvements:\n- The parser now properly handles complex shell syntax using the battle-tested `shell-quote` library\n- The code is more concise and easier to understand\n- The breaking expressions have been simplified to `[ /^-/, /[\\\\/:.~ ]/ ]` as requested\n\nThe changes have been pushed to the PR branch and CI checks are currently running. The implementation maintains full backward compatibility while simplifying the codebase significantly.",
149+
"createdAt": "2025-07-23T14:47:04Z",
150+
"id": "IC_kwDONIq5lM65TyCz",
151+
"includesCreatedEdit": false,
152+
"isMinimized": false,
153+
"minimizedReason": "",
154+
"reactionGroups": [],
155+
"url": "https://github.com/RooCodeInc/Roo-Code/pull/5798#issuecomment-3108970675",
156+
"viewerDidAuthor": false
157+
},
158+
{
159+
"author": { "login": "hannesrudolph" },
160+
"authorAssociation": "MEMBER",
161+
"body": "@roomote-agent run pr-reviewer mode on this and then fix the critical issues with pr-fixer mode",
162+
"createdAt": "2025-07-23T16:25:44Z",
163+
"id": "IC_kwDONIq5lM65VJx8",
164+
"includesCreatedEdit": false,
165+
"isMinimized": false,
166+
"minimizedReason": "",
167+
"reactionGroups": [],
168+
"url": "https://github.com/RooCodeInc/Roo-Code/pull/5798#issuecomment-3109330044",
169+
"viewerDidAuthor": true
170+
},
171+
{
172+
"author": { "login": "roomote" },
173+
"authorAssociation": "NONE",
174+
"body": "Hi @hannesrudolph, I see your request. I will now run pr-reviewer mode on this PR to identify critical issues, then switch to pr-fixer mode to address them. Starting the review process now...",
175+
"createdAt": "2025-07-23T16:30:01Z",
176+
"id": "IC_kwDONIq5lM65VMvl",
177+
"includesCreatedEdit": false,
178+
"isMinimized": false,
179+
"minimizedReason": "",
180+
"reactionGroups": [],
181+
"url": "https://github.com/RooCodeInc/Roo-Code/pull/5798#issuecomment-3109342181",
182+
"viewerDidAuthor": false
183+
}
184+
]

0 commit comments

Comments
 (0)