Skip to content

Conversation

@fuko2935
Copy link

@fuko2935 fuko2935 commented Aug 11, 2025

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.

Related GitHub Issue
Closes: #

Description
This PR 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.

Implementation details:

A new experimental feature flag COMMAND_FILTERING_MODE was added in src/shared/experiments.ts.
A new validation function getCommandDecisionAlternate was created in webview-ui/src/utils/command-validation.ts to implement the new logic.
The main getCommandDecision function was updated to check the feature flag and delegate to the appropriate validation function.
The ChatView.tsx component was updated to pass the experiments configuration to the validation logic.
Test Procedure
Added a new test file webview-ui/src/utils/tests/command-validation-alternate.spec.ts with unit tests for the new validation logic.
Updated existing tests in webview-ui/src/utils/tests/command-validation.spec.ts to account for the new experiments parameter.
Verification Steps:

Enable the "Command Filtering Mode" in the experimental settings.
Enter a command that is on the allowlist (e.g., npm install). It should be denied.
Enter a command that is on the denylist (e.g., rm -rf /). It should be denied.
Enter a command that is on neither list (e.g., git status). It should be approved.
Disable the feature flag and verify that the original command validation logic is used.
Note: I was unable to run the full test suite (bun test) due to a persistent build environment issue related to linting (ERR_REQUIRE_ESM). The code has been manually reviewed for correctness.

Pre-Submission Checklist
Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
Scope: My changes are focused on the linked issue (one major feature/fix per PR).
Self-Review: I have performed a thorough self-review of my code.
Testing: New and/or updated tests have been added to cover my changes (if applicable).
Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
Contribution Guidelines: I have read and agree to the Contributor Guidelines.
Screenshots / Videos
N/A

Documentation Updates
No documentation updates are required.
Additional Notes
The primary blocker during this task was a build failure in the testing environment, which prevented the test suite from running. The error appears to be a deep dependency issue related to ES Module resolution in a CommonJS-based tool (eslint). I've implemented the feature and tests, but they are unverified by the automated test suite.


Important

Introduces experimental command validation logic with a feature flag to toggle between new and old logic, including new tests and manual testing due to environment issues.

  • Behavior:
    • Introduces new experimental command validation logic in getCommandDecisionAlternate().
    • Commands are denied if they match any prefix in either allowlist or denylist.
    • Adds feature flag COMMAND_FILTERING_MODE to toggle between new and old logic.
  • Implementation:
    • Adds getCommandDecisionAlternate() and getSingleCommandDecisionAlternate() in command-validation.ts.
    • Updates getCommandDecision() to use new logic if feature flag is enabled.
    • Modifies ChatView.tsx to pass experiments configuration to validation logic.
  • Testing:
    • Adds command-validation-alternate.spec.ts for new logic tests.
    • Updates command-validation.spec.ts to include experiments parameter.
  • Misc:
    • Manual testing was done due to test suite environment issues.

This description was created by Ellipsis for 1bf956c. You can customize this summary. It will automatically update as commits are pushed.

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.
@fuko2935 fuko2935 requested review from cte, jr and mrubens as code owners August 11, 2025 13:42
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request labels Aug 11, 2025
Copy link
Contributor

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I've reviewed the changes and found several critical issues that need attention before this can be merged.

Critical Issues (Must Fix):

  1. Logical Error in Command Validation - The PR description states "A command is denied if it matches any prefix in the allowlist" which appears to be a typo or fundamental misunderstanding. The implementation in getSingleCommandDecisionAlternate (lines 611-613) denies commands matching the allowlist, which contradicts the expected behavior where allowlists should permit commands.

  2. Missing Dependency - The test file command-validation.spec.ts imports experimentDefault from @roo/experiments but this export doesn't exist in that module path. It's actually exported from src/shared/experiments.ts.

  3. Incomplete CommandValidator Integration - The CommandValidator class constructor and updateCommandLists method now require an experiments parameter, but the implementation doesn't actually use or store this parameter, making it a breaking change without proper implementation.

  4. Unintended File Addition - A bun.lock file (1124 lines) was added to the repository. This appears to be from your local environment and should not be committed.

Important Suggestions:

  1. Confusing Naming - The function name getCommandDecisionAlternate doesn't clearly indicate what makes it "alternate". Consider a more descriptive name like getCommandDecisionRestrictive or getCommandDecisionDenyBoth.

  2. Missing Documentation - The new experimental mode lacks user-facing documentation explaining when and why to use it.

Minor Improvements:

  1. Test Coverage - While tests were added for the alternate logic, there's no integration test verifying the feature flag actually switches between the two modes correctly.

Please address these issues, especially the critical ones, before this can be merged.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Aug 11, 2025
@daniel-lxs
Copy link
Member

Hey @fuko2935, Thank you for the contribution. We really appreciate the time you put into this. Unfortunately, I'm not really sure what problem this solves.

I'll be closing this PR, but feel free to ask any questions.

@daniel-lxs daniel-lxs closed this Aug 13, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Aug 13, 2025
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants