From efaaed9f7381f7d9d1d027193b870cf0596adb4e Mon Sep 17 00:00:00 2001 From: MuriloFP Date: Thu, 3 Jul 2025 15:26:24 -0300 Subject: [PATCH 1/5] feat: add Issue Fixer Orchestrator mode --- .../1_Workflow.xml | 402 +++++++++++++----- .../2_best_practices.xml | 31 +- .roomodes | 3 +- 3 files changed, 315 insertions(+), 121 deletions(-) diff --git a/.roo/rules-issue-fixer-orchestrator/1_Workflow.xml b/.roo/rules-issue-fixer-orchestrator/1_Workflow.xml index 2ad4bf65fc7..3e6619993ec 100644 --- a/.roo/rules-issue-fixer-orchestrator/1_Workflow.xml +++ b/.roo/rules-issue-fixer-orchestrator/1_Workflow.xml @@ -27,7 +27,7 @@ Delegate: Analyze Requirements & Explore Codebase - Launch a subtask in `code` mode to perform a detailed analysis of the issue and the codebase. The subtask will be responsible for identifying affected files and creating an implementation plan. + Launch a subtask in `architect` mode to perform a detailed analysis of the issue and the codebase. The subtask will be responsible for identifying affected files and creating an implementation plan. The context file `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/issue_context.json` will be the input for this subtask. The subtask should write its findings (the implementation plan) to a new file: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/implementation_plan.md`. @@ -36,19 +36,50 @@ **Task: Analyze Issue and Create Implementation Plan** - You are an expert software architect. Your task is to analyze the provided GitHub issue and the current codebase to create a detailed implementation plan. + You are an expert software architect. Your task is to analyze the provided GitHub issue and the current codebase to create a detailed implementation plan with a focus on understanding component interactions and dependencies. 1. **Read Issue Context**: The full issue details and comments are in `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/issue_context.json`. Read this file to understand all requirements, acceptance criteria, and technical discussions. - 2. **Explore Codebase**: Use `codebase_search`, `read_file`, and other tools to explore the codebase. Identify all files that will need to be modified or created to address the issue. Analyze existing patterns and conventions. - - 3. **Create Implementation Plan**: Based on your analysis, create a comprehensive implementation plan. The plan should be detailed enough for another developer to execute. It must include: - - A summary of the issue and the proposed solution. - - A list of all files to be created or modified. - - A step-by-step guide for the code changes required in each file. - - A plan for writing or updating tests. - - 4. **Save the Plan**: Write the complete implementation plan to `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/implementation_plan.md`. + 2. **Perform Architectural Analysis**: + - **Map Component Interactions**: Trace the complete data flow from entry points to outputs + - **Identify Paired Operations**: For any operation (e.g., export), find its counterpart (e.g., import) + - **Find Similar Patterns**: Search for existing implementations of similar features + - **Analyze Dependencies**: Identify all consumers of the functionality being modified + - **Assess Impact**: Determine how changes will affect other parts of the system + + 3. **Explore Codebase Systematically**: + - Use `codebase_search` FIRST to find all related functionality + - Search for paired operations (if modifying export, search for import) + - Find all files that consume or depend on the affected functionality + - Identify configuration files, tests, and documentation that need updates + - Study similar features to understand established patterns + + 4. **Create Comprehensive Implementation Plan**: The plan must include: + - **Issue Summary**: Clear description of the problem and proposed solution + - **Architectural Context**: + - Data flow diagram showing component interactions + - List of paired operations that must be updated together + - Dependencies and consumers of the affected functionality + - **Impact Analysis**: + - All files that will be affected (directly and indirectly) + - Potential breaking changes + - Performance implications + - **Implementation Steps**: + - Detailed, ordered steps for each file modification + - Specific code changes with context + - Validation and error handling requirements + - **Testing Strategy**: + - Unit tests for individual components + - Integration tests for component interactions + - Edge cases and error scenarios + + 5. **Save the Plan**: Write the complete implementation plan to `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/implementation_plan.md`. + + **Critical Requirements:** + - Always search for and analyze paired operations (import/export, save/load, etc.) + - Map the complete data flow before proposing changes + - Identify all integration points and dependencies + - Consider backward compatibility and migration needs **Completion Protocol:** - This is your only task. Do not deviate from these instructions. @@ -110,19 +141,43 @@ **Task: Implement Code Changes Based on Plan** - You are an expert software developer. Your task is to implement the code changes exactly as described in the provided implementation plan. + You are an expert software developer. Your task is to implement the code changes with full awareness of system interactions and dependencies. - 1. **Read the Plan**: The implementation plan is located at `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/implementation_plan.md`. Follow its instructions carefully. + 1. **Read the Plan**: The implementation plan is located at `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/implementation_plan.md`. Pay special attention to: + - The architectural context section + - Component interaction diagrams + - Identified dependencies and related operations + - Impact analysis - 2. **Implement Changes**: Use `apply_diff` and `write_to_file` to make the specified code changes. Adhere to all coding standards and patterns mentioned in the plan. + 2. **Validate Understanding**: Before coding, ensure you understand: + - How data flows through the system + - All related operations that must be updated together + - Dependencies that could be affected + - Integration points with other components - 3. **Implement Tests**: Write new unit and integration tests as specified in the plan to ensure quality and prevent regressions. + 3. **Implement Holistically**: + - **Update Related Operations Together**: If modifying one operation, update all related operations + - **Maintain Consistency**: Ensure data structures, validation, and error handling are consistent + - **Consider Side Effects**: Account for how changes propagate through the system + - **Follow Existing Patterns**: Use established patterns from similar features - 4. **Track Modified Files**: As you modify or create files, keep a running list. + 4. **Implement Tests**: + - Write tests that verify component interactions + - Test related operations together + - Include edge cases and error scenarios + - Verify data consistency across operations - 5. **Save Modified Files List**: After all changes are implemented and tested, save the list of all file paths you created or modified to `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/modified_files.json`. The format should be a JSON array of strings. + 5. **Track Modified Files**: As you modify or create files, keep a running list. + + 6. **Save Modified Files List**: After all changes are implemented and tested, save the list of all file paths you created or modified to `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/modified_files.json`. The format should be a JSON array of strings. Example: `["src/components/NewFeature.tsx", "src/__tests__/NewFeature.spec.ts"]` + **Critical Reminders:** + - Never implement changes in isolation - consider the full system impact + - Always update related operations together to maintain consistency + - Test component interactions, not just individual functions + - Follow the architectural analysis from the planning phase + Once the `modified_files.json` file is saved, your task is complete. @@ -325,122 +380,233 @@ - Create Pull Request + Delegate: Review Changes Before PR - This is the final step where the orchestrator takes all the prepared materials and creates the pull request. + Before creating the pull request, delegate to the PR reviewer mode to get feedback on the implementation and proposed changes. - 1. **Read PR Summary**: - - - - .roo/temp/issue-fixer-orchestrator/[TASK_ID]/pr_summary.json - - - - - 2. **Get Final Approval**: Present the PR title and body to the user for final approval, providing an option to request changes. + + pr-reviewer + + **Task: Review Implementation Before PR Creation** - - - I have prepared the pull request. Please review and confirm. + You are an expert code reviewer. Your task is to review the implementation for issue #[issue-number] and provide feedback before a pull request is created. - **Title**: [Insert title from pr_summary.json] + **Context Files:** + - **Issue Details**: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/issue_context.json` + - **Implementation Plan**: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/implementation_plan.md` + - **Modified Files**: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/modified_files.json` + - **Verification Results**: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/verification_results.md` + - **Translation Summary** (if exists): `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/translation_summary.md` + - **Draft PR Summary**: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/pr_summary.json` + + **Your Review Focus:** + 1. **Code Quality**: Review the actual code changes for: + - Adherence to project coding standards + - Proper error handling and edge cases + - Performance considerations + - Security implications + - Maintainability and readability + + 2. **Implementation Completeness**: Verify that: + - All requirements from the issue are addressed + - The solution follows the implementation plan + - No critical functionality is missing + - Proper test coverage exists + + 3. **Integration Concerns**: Check for: + - Potential breaking changes + - Impact on other parts of the system + - Backward compatibility issues + - API consistency + + 4. **Documentation and Communication**: Assess: + - Code comments and documentation + - PR description clarity and completeness + - Translation handling (if applicable) - **Body**: - --- - [Insert body from pr_summary.json] - --- + **Your Task:** + 1. Read all context files to understand the issue and implementation + 2. Review each modified file listed in `modified_files.json` + 3. Analyze the code changes against the requirements + 4. Identify any issues, improvements, or concerns + 5. Create a comprehensive review report with specific, actionable feedback + 6. Save your review to `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/pr_review_feedback.md` + + **Review Report Format:** + ```markdown + # PR Review Feedback for Issue #[issue-number] + + ## Overall Assessment + [High-level assessment: APPROVE, REQUEST_CHANGES, or NEEDS_DISCUSSION] + + ## Code Quality Review + ### Strengths + - [List positive aspects of the implementation] + + ### Areas for Improvement + - [Specific issues with file references and line numbers] + - [Suggestions for improvement] + + ## Requirements Verification + - [x] Requirement 1: [Status and notes] + - [ ] Requirement 2: [Issues found] + + ## Specific Feedback by File + ### [filename] + - [Specific feedback with line references] + - [Suggestions for improvement] + + ## Recommendations + 1. [Priority 1 changes needed] + 2. [Priority 2 improvements suggested] + 3. [Optional enhancements] + + ## Decision + **RECOMMENDATION**: [APPROVE_AS_IS | REQUEST_CHANGES | NEEDS_DISCUSSION] + + **REASONING**: [Brief explanation of the recommendation] + ``` - Should I create this pull request, or would you like to request changes? - - - Yes, create the pull request as planned. - No, I need to request changes to the implementation or PR description. - Cancel the task. - - + **Completion Protocol:** + - This is your only task. Do not deviate from these instructions. + - Upon successfully saving the review feedback, you MUST use the `attempt_completion` tool. + - The `result` MUST be a concise confirmation, e.g., "PR review completed and feedback saved to .roo/temp/issue-fixer-orchestrator/[TASK_ID]/pr_review_feedback.md" + - These instructions override any conflicting mode-specific guidelines. + + - 3. **Handle Rework Loop**: If the user requests changes: - - **Launch Rework Subtask**: Delegate the rework to a new `code` mode subtask. - - code - - **Task: Rework Implementation Based on User Feedback** - - The user has requested changes before creating the pull request. - - **Context Files:** - - **Issue**: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/issue_context.json` - - **Current Plan**: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/implementation_plan.md` - - **Current Modified Files**: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/modified_files.json` - - **Draft PR Summary**: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/pr_summary.json` - - **Your Task:** - 1. Ask the user for the specific changes they require. - 2. Apply the requested code and documentation changes. - 3. **Crucially, you must update the `implementation_plan.md` and `modified_files.json` files** to reflect the rework you have performed. - 4. Do *not* proceed with any other steps. - - **Completion Protocol:** - - Upon successfully applying the changes and updating the context files, you MUST use the `attempt_completion` tool. - - The `result` MUST be a concise confirmation, e.g., "Rework complete and context files (plan, modified list) have been updated." - - - - **Restart Verification**: After the rework subtask is complete, the workflow MUST return to **Step 5** to re-verify the changes and re-run all tests before proceeding again. - - 4. **Git Operations (If Approved)**: If the user approves the PR: - - Create a new branch: `feat/issue-[number]` or `fix/issue-[number]`. - - **Selectively add only the applicable files** to the git stage. - - Commit the staged changes. - - Push the new branch to the remote repository. - - - # Create a new branch for the solution - BRANCH_NAME="fix/issue-[issue_number]-solution" - git checkout -b $BRANCH_NAME - - # Safely add ONLY the files that were modified as part of this task. - # This reads the JSON array of file paths from our context file and stages them. - # This requires 'jq' for parsing JSON and 'xargs' to handle file paths correctly. - cat .roo/temp/issue-fixer-orchestrator/[TASK_ID]/modified_files.json | jq -r '.[]' | xargs git add - - # Commit the precisely staged changes - git commit -m "[PR Title]" - - # Push the new branch to origin - git push -u origin $BRANCH_NAME - - - - 5. **Create PR**: Use the `gh` CLI to create the pull request. - - gh pr create --repo [owner]/[repo] --base main --title "[PR Title from JSON]" --body "[PR Body from JSON]" - - - 6. **Link to Issue**: Comment on the original issue with the PR link. - - gh issue comment [issue_number] --repo [owner]/[repo] --body "PR #[new PR number] has been created." - + After the review subtask completes, read and process the feedback. - Monitor PR Checks and Cleanup + Process Review Feedback and Decide Next Steps - After creating the PR, monitor the CI checks and then clean up the temporary files. + After the PR review is complete, read the feedback and decide whether to make changes or proceed with PR creation. + + 1. **Read Review Feedback**: + + + + .roo/temp/issue-fixer-orchestrator/[TASK_ID]/pr_review_feedback.md + + + + + 2. **Present Feedback to User**: Show the review feedback and ask for direction. + + + The PR review has been completed. Here is the feedback: + + --- + [Insert content of pr_review_feedback.md here] + --- + + Based on this review, how would you like to proceed? + + + Implement the suggested changes before creating the PR + Create the PR as-is, ignoring the review feedback + Discuss specific feedback points before deciding + Cancel the task + + + + 3. **Handle User Decision**: + + **If user chooses to implement changes:** + - Launch a rework subtask to address the review feedback + + code + + **Task: Address PR Review Feedback** + + The PR review has identified areas for improvement. Your task is to address the feedback before creating the pull request. + + **Context Files:** + - **Issue**: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/issue_context.json` + - **Current Plan**: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/implementation_plan.md` + - **Current Modified Files**: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/modified_files.json` + - **Review Feedback**: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/pr_review_feedback.md` + - **Draft PR Summary**: `.roo/temp/issue-fixer-orchestrator/[TASK_ID]/pr_summary.json` + + **Your Task:** + 1. Read the review feedback carefully + 2. Address each point raised by the reviewer + 3. Make the necessary code changes + 4. Update tests if needed + 5. **Update the `modified_files.json` file** to reflect any new or changed files + 6. **Update the `implementation_plan.md`** if the approach has changed significantly + + **Important Notes:** + - Focus on the specific issues identified in the review + - Maintain the overall solution approach unless the review suggests otherwise + - Ensure all changes are properly tested + - Do not proceed with any other workflow steps + + **Completion Protocol:** + - Upon successfully addressing the feedback and updating context files, you MUST use the `attempt_completion` tool. + - The `result` MUST be a concise confirmation, e.g., "Review feedback addressed and context files updated." + + + - **After rework completion**: Return to **Step 5** (Verify and Test) to re-verify the changes + + **If user chooses to proceed as-is:** + - Continue to the next step (Create Pull Request) + + **If user wants to discuss or cancel:** + - Handle accordingly based on user input + + - 1. **Monitor Checks**: Use `--watch` to monitor CI status in real-time. - - gh pr checks [PR URL or number] --repo [owner]/[repo] --watch - + + Prepare Branch and Present PR Template + + This step prepares the branch and commits, then presents the PR template to the user for confirmation before creating the actual pull request. + + 1. Read Issue Context for Issue Number: + Use read_file to get the issue context from .roo/temp/issue-fixer-orchestrator/[TASK_ID]/issue_context.json + + 2. Git Operations - Create branch and commit changes: + - Create a new branch: feat/issue-[number] or fix/issue-[number] + - Selectively add only the applicable files to the git stage + - Commit the staged changes + - Push the new branch to the remote repository + + Use execute_command with: + BRANCH_NAME="fix/issue-[issue_number]-solution" + git checkout -b $BRANCH_NAME + cat .roo/temp/issue-fixer-orchestrator/[TASK_ID]/modified_files.json | jq -r '.[]' | xargs git add + git commit -m "[PR Title]" + git push -u origin $BRANCH_NAME + + 3. Present PR Template - Instead of creating the PR automatically, present the standardized PR template to the user: + Use ask_followup_question to ask: "The branch has been created and changes have been committed. I have prepared a standardized PR template for this issue. Would you like me to create the pull request using the standard Roo Code PR template, or would you prefer to make changes first?" + + Provide these options: + - Yes, create the pull request with the standard template + - No, I want to make changes to the implementation first + - No, I want to customize the PR template before creating it + - Cancel the task + + 4. Handle User Decision: + If user chooses to create the PR: Use gh CLI to create the pull request with the standard template + If user chooses to make changes: Launch a rework subtask using new_task with code mode + If user wants to customize the template: Ask for their preferred PR title and body + + 5. Link to Issue - After PR creation, comment on the original issue with the PR link using gh issue comment + + - 2. **Report Status**: Inform the user of the final status of the checks. + + Monitor PR Checks and Cleanup + + After creating the PR (if created), monitor the CI checks and then clean up the temporary files. - 3. **Cleanup**: Remove the temporary task directory. - - rm -rf .roo/temp/issue-fixer-orchestrator/[TASK_ID] - - + 1. Monitor Checks - Use gh pr checks with --watch to monitor CI status in real-time + 2. Report Status - Inform the user of the final status of the checks + 3. Cleanup - Remove the temporary task directory using rm -rf .roo/temp/issue-fixer-orchestrator/[TASK_ID] + This concludes the orchestration workflow. diff --git a/.roo/rules-issue-fixer-orchestrator/2_best_practices.xml b/.roo/rules-issue-fixer-orchestrator/2_best_practices.xml index e6251d67b23..b9a9b52db78 100644 --- a/.roo/rules-issue-fixer-orchestrator/2_best_practices.xml +++ b/.roo/rules-issue-fixer-orchestrator/2_best_practices.xml @@ -27,6 +27,17 @@ Always use `codebase_search` FIRST to understand the codebase structure and find all related files before using other tools like `read_file`. + + Critical: Understand Component Interactions + + Map the complete data flow from input to output + Identify ALL paired operations (import/export, save/load, encode/decode) + Find all consumers and dependencies of the affected code + Trace how data transformations occur throughout the system + Understand error propagation and handling patterns + + + Investigation Checklist for Bug Fixes Search for the specific error message or broken functionality. @@ -34,6 +45,8 @@ Locate related test files to understand expected behavior. Identify all dependencies and import/export patterns for the affected code. Find similar, working patterns in the codebase to use as a reference. + **CRITICAL**: For any operation being fixed, find and analyze its paired operations + Trace the complete data flow to understand all affected components @@ -42,10 +55,26 @@ Find potential integration points (e.g., API routes, UI component registries). Locate relevant configuration files that may need to be updated. Identify common patterns, components, and utilities that should be reused. + **CRITICAL**: Design paired operations together (e.g., both import AND export) + Map all data transformations and state changes + Identify all downstream consumers of the new functionality + + Always Implement Paired Operations Together + + When fixing export, ALWAYS check and update import + When modifying save, ALWAYS verify load handles the changes + When changing serialization, ALWAYS update deserialization + When updating create, consider read/update/delete operations + + + Paired operations must maintain consistency. Changes to one without the other leads to data corruption, import failures, or broken functionality. + + + - Always read multiple related files together to understand the full context, including coding conventions, testing patterns, and error handling approaches. + Always read multiple related files together to understand the full context. Never assume a change is isolated - trace its impact through the entire system. \ No newline at end of file diff --git a/.roomodes b/.roomodes index 3b178f234fa..213d7b8314a 100644 --- a/.roomodes +++ b/.roomodes @@ -9,7 +9,7 @@ customModes: - Ensuring modes have appropriate tool group permissions - Crafting clear whenToUse descriptions for the Orchestrator - Following XML structuring best practices for clarity and parseability - + You help users create new modes by: - Gathering requirements about the mode's purpose and workflow - Defining appropriate roleDefinition and whenToUse descriptions @@ -182,4 +182,3 @@ customModes: - edit - command source: project - description: Issue Fixer mode ported into an orchestrator From 46366054236f26da6bad15185328750744eb2d9c Mon Sep 17 00:00:00 2001 From: MuriloFP Date: Sat, 5 Jul 2025 00:46:58 -0300 Subject: [PATCH 2/5] fix: Add whitelist support for .roo/temp/ directory in list_files tool (#5409) --- .../glob/__tests__/list-files.spec.ts | 334 +++++++++++++++++- src/services/glob/constants.ts | 7 + src/services/glob/list-files.ts | 111 +++++- 3 files changed, 441 insertions(+), 11 deletions(-) diff --git a/src/services/glob/__tests__/list-files.spec.ts b/src/services/glob/__tests__/list-files.spec.ts index 2a154248307..049730f0056 100644 --- a/src/services/glob/__tests__/list-files.spec.ts +++ b/src/services/glob/__tests__/list-files.spec.ts @@ -1,6 +1,38 @@ -import { describe, it, expect, vi } from "vitest" +import { vi, describe, it, expect, beforeEach } from "vitest" +import * as path from "path" +import * as fs from "fs" +import * as childProcess from "child_process" import { listFiles } from "../list-files" +// Mock ripgrep to avoid filesystem dependencies +vi.mock("../../ripgrep", () => ({ + getBinPath: vi.fn().mockResolvedValue("/mock/path/to/rg"), +})) + +// Mock vscode +vi.mock("vscode", () => ({ + env: { + appRoot: "/mock/app/root", + }, +})) + +// Mock filesystem operations +vi.mock("fs", () => ({ + promises: { + access: vi.fn().mockRejectedValue(new Error("Not found")), + readFile: vi.fn().mockResolvedValue(""), + readdir: vi.fn().mockResolvedValue([]), + }, +})) + +vi.mock("child_process", () => ({ + spawn: vi.fn(), +})) + +vi.mock("../../path", () => ({ + arePathsEqual: vi.fn().mockReturnValue(false), +})) + vi.mock("../list-files", async () => { const actual = await vi.importActual("../list-files") return { @@ -15,4 +47,304 @@ describe("listFiles", () => { expect(result).toEqual([[], false]) }) + + describe("Whitelist functionality", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("should list files in .roo/temp even when .roo is gitignored", async () => { + const mockSpawn = vi.mocked(childProcess.spawn) + const mockFs = vi.mocked(fs.promises) + + // Mock .gitignore file with .roo + mockFs.access.mockImplementation((path) => { + if ((path as string).endsWith(".gitignore")) { + return Promise.resolve() + } + return Promise.reject(new Error("Not found")) + }) + mockFs.readFile.mockImplementation((path) => { + if ((path as string).endsWith(".gitignore")) { + return Promise.resolve(".roo") + } + return Promise.resolve("") + }) + + // Mock directory structure + mockFs.readdir.mockImplementation((dirPath) => { + const resolvedPath = path.resolve(dirPath as string) + if (resolvedPath === path.resolve(".")) { + return Promise.resolve([ + { name: ".roo", isDirectory: () => true, isSymbolicLink: () => false }, + { name: "src", isDirectory: () => true, isSymbolicLink: () => false }, + ] as any) + } + if (resolvedPath === path.resolve(".roo")) { + return Promise.resolve([ + { name: "temp", isDirectory: () => true, isSymbolicLink: () => false }, + ] as any) + } + if (resolvedPath === path.resolve(".roo/temp")) { + return Promise.resolve([ + { name: "test.txt", isDirectory: () => false, isSymbolicLink: () => false }, + ] as any) + } + return Promise.resolve([]) + }) + + // Mock ripgrep process + const mockProcess = { + stdout: { + on: vi.fn((event, callback) => { + if (event === "data") { + setTimeout(() => callback(".roo/temp/test.txt\n"), 10) + } + }), + }, + stderr: { + on: vi.fn(), + }, + on: vi.fn((event, callback) => { + if (event === "close") { + setTimeout(() => callback(0), 20) + } + }), + kill: vi.fn(), + } + mockSpawn.mockReturnValue(mockProcess as any) + + const [files] = await listFiles(".", true, 100) + + // Should include the whitelisted file - check with normalized paths + const hasRooTempFile = files.some((file) => file.replace(/\\/g, "/").includes(".roo/temp/test.txt")) + expect(hasRooTempFile).toBe(true) + + // The directories should be included in the results - check with normalized paths + const hasRooDir = files.some((file) => file.replace(/\\/g, "/").includes(".roo/")) + const hasRooTempDir = files.some((file) => file.replace(/\\/g, "/").includes(".roo/temp/")) + expect(hasRooDir).toBe(true) + expect(hasRooTempDir).toBe(true) + }) + + it("should handle nested hidden directories correctly", async () => { + const mockSpawn = vi.mocked(childProcess.spawn) + const mockFs = vi.mocked(fs.promises) + + // Mock directory structure with nested hidden directories + mockFs.readdir.mockImplementation((dirPath) => { + const resolvedPath = path.resolve(dirPath as string) + if (resolvedPath === path.resolve(".roo/temp")) { + return Promise.resolve([ + { name: "subdir", isDirectory: () => true, isSymbolicLink: () => false }, + ] as any) + } + if (resolvedPath === path.resolve(".roo/temp/subdir")) { + return Promise.resolve([ + { name: ".hidden", isDirectory: () => true, isSymbolicLink: () => false }, + ] as any) + } + if (resolvedPath === path.resolve(".roo/temp/subdir/.hidden")) { + return Promise.resolve([ + { name: "file.txt", isDirectory: () => false, isSymbolicLink: () => false }, + ] as any) + } + return Promise.resolve([]) + }) + + // Mock ripgrep process + const mockProcess = { + stdout: { + on: vi.fn((event, callback) => { + if (event === "data") { + setTimeout(() => callback(".roo/temp/subdir/.hidden/file.txt\n"), 10) + } + }), + }, + stderr: { + on: vi.fn(), + }, + on: vi.fn((event, callback) => { + if (event === "close") { + setTimeout(() => callback(0), 20) + } + }), + kill: vi.fn(), + } + mockSpawn.mockReturnValue(mockProcess as any) + + const [files] = await listFiles(".roo/temp", true, 100) + + // Should include files in nested hidden directories under whitelisted paths + expect(files).toContain(".roo/temp/subdir/.hidden/file.txt") + }) + + it("should not whitelist other hidden directories", async () => { + const mockSpawn = vi.mocked(childProcess.spawn) + const mockFs = vi.mocked(fs.promises) + + // Mock directory structure + mockFs.readdir.mockImplementation((dirPath) => { + const resolvedPath = path.resolve(dirPath as string) + if (resolvedPath === path.resolve(".")) { + return Promise.resolve([ + { name: ".other-hidden", isDirectory: () => true, isSymbolicLink: () => false }, + { name: ".roo", isDirectory: () => true, isSymbolicLink: () => false }, + ] as any) + } + return Promise.resolve([]) + }) + + // Mock ripgrep process - should not return .other-hidden files + const mockProcess = { + stdout: { + on: vi.fn((event, callback) => { + if (event === "data") { + // Only return non-hidden files + setTimeout(() => callback(""), 10) + } + }), + }, + stderr: { + on: vi.fn(), + }, + on: vi.fn((event, callback) => { + if (event === "close") { + setTimeout(() => callback(0), 20) + } + }), + kill: vi.fn(), + } + mockSpawn.mockReturnValue(mockProcess as any) + + const [files] = await listFiles(".", true, 100) + + // Should not include files from other hidden directories + expect(files).not.toContain(".other-hidden/") + expect(files).not.toContain(".other-hidden/file.txt") + }) + + it("should respect whitelist even when parent directory is gitignored", async () => { + const mockSpawn = vi.mocked(childProcess.spawn) + const mockFs = vi.mocked(fs.promises) + + // Mock .gitignore with parent directory + mockFs.access.mockImplementation((path) => { + if ((path as string).endsWith(".gitignore")) { + return Promise.resolve() + } + return Promise.reject(new Error("Not found")) + }) + mockFs.readFile.mockImplementation((path) => { + if ((path as string).endsWith(".gitignore")) { + return Promise.resolve(".roo/") + } + return Promise.resolve("") + }) + + // Mock directory structure + mockFs.readdir.mockImplementation((dirPath) => { + const resolvedPath = path.resolve(dirPath as string) + if (resolvedPath === path.resolve(".")) { + return Promise.resolve([ + { name: ".roo", isDirectory: () => true, isSymbolicLink: () => false }, + ] as any) + } + if (resolvedPath === path.resolve(".roo")) { + return Promise.resolve([ + { name: "temp", isDirectory: () => true, isSymbolicLink: () => false }, + { name: "other", isDirectory: () => true, isSymbolicLink: () => false }, + ] as any) + } + return Promise.resolve([]) + }) + + // Mock ripgrep process + const mockProcess = { + stdout: { + on: vi.fn((event, callback) => { + if (event === "data") { + setTimeout(() => callback(".roo/temp/workflow.json\n"), 10) + } + }), + }, + stderr: { + on: vi.fn(), + }, + on: vi.fn((event, callback) => { + if (event === "close") { + setTimeout(() => callback(0), 20) + } + }), + kill: vi.fn(), + } + mockSpawn.mockReturnValue(mockProcess as any) + + const [files] = await listFiles(".", true, 100) + + // Should include whitelisted paths even when parent is gitignored + expect(files).toContain(".roo/temp/workflow.json") + }) + + it("should handle case-sensitive paths correctly on different platforms", async () => { + const mockSpawn = vi.mocked(childProcess.spawn) + const mockFs = vi.mocked(fs.promises) + + // Mock directory structure with different case variations + mockFs.readdir.mockImplementation((dirPath) => { + const resolvedPath = path.resolve(dirPath as string) + if (resolvedPath === path.resolve(".")) { + return Promise.resolve([ + { name: ".roo", isDirectory: () => true, isSymbolicLink: () => false }, + { name: ".Roo", isDirectory: () => true, isSymbolicLink: () => false }, + ] as any) + } + if (resolvedPath === path.resolve(".roo")) { + return Promise.resolve([ + { name: "temp", isDirectory: () => true, isSymbolicLink: () => false }, + ] as any) + } + if (resolvedPath === path.resolve(".Roo")) { + return Promise.resolve([ + { name: "Temp", isDirectory: () => true, isSymbolicLink: () => false }, + ] as any) + } + return Promise.resolve([]) + }) + + // Mock ripgrep process - should only return files from whitelisted .roo/temp + const mockProcess = { + stdout: { + on: vi.fn((event, callback) => { + if (event === "data") { + setTimeout(() => { + // Only return files from the whitelisted .roo/temp directory + callback(".roo/temp/file1.txt\n") + }, 10) + } + }), + }, + stderr: { + on: vi.fn(), + }, + on: vi.fn((event, callback) => { + if (event === "close") { + setTimeout(() => callback(0), 20) + } + }), + kill: vi.fn(), + } + mockSpawn.mockReturnValue(mockProcess as any) + + const [files] = await listFiles(".", true, 100) + + // On case-sensitive systems, .roo and .Roo are different directories + // The whitelist is for ".roo/temp" specifically + expect(files).toContain(".roo/temp/file1.txt") + + // .Roo/Temp should not be whitelisted as it's a different path on case-sensitive systems + const hasUpperCaseRoo = files.some((file) => file.includes(".Roo/")) + expect(hasUpperCaseRoo).toBe(false) + }) + }) }) diff --git a/src/services/glob/constants.ts b/src/services/glob/constants.ts index 1ddcc37df91..ba30ba4b64a 100644 --- a/src/services/glob/constants.ts +++ b/src/services/glob/constants.ts @@ -22,3 +22,10 @@ export const DIRS_TO_IGNORE = [ "Pods", ".*", ] + +/** + * List of directories that should always be visible in file listings, + * even if they are included in .gitignore or are hidden directories. + * This is necessary for directories that contain workflow files used by various modes. + */ +export const GITIGNORE_WHITELIST = [".roo/temp"] diff --git a/src/services/glob/list-files.ts b/src/services/glob/list-files.ts index 3fb1b2e154c..97e793ae78a 100644 --- a/src/services/glob/list-files.ts +++ b/src/services/glob/list-files.ts @@ -5,7 +5,63 @@ import * as childProcess from "child_process" import * as vscode from "vscode" import { arePathsEqual } from "../../utils/path" import { getBinPath } from "../../services/ripgrep" -import { DIRS_TO_IGNORE } from "./constants" +import { DIRS_TO_IGNORE, GITIGNORE_WHITELIST } from "./constants" + +/** + * Check if a path is whitelisted or is a parent of a whitelisted path + */ +function isPathWhitelisted(absolutePath: string): boolean { + const normalizedPath = path.normalize(absolutePath) + + for (const whitelistedDir of GITIGNORE_WHITELIST) { + const whitelistedPath = path.normalize(whitelistedDir) + + // Check if this is the whitelisted path or a parent of it + if (normalizedPath === whitelistedPath || whitelistedPath.startsWith(path.join(normalizedPath, path.sep))) { + return true + } + } + + return false +} + +/** + * Check if a directory might contain whitelisted paths + */ +function mightContainWhitelistedPaths(dirPath: string): boolean { + const normalizedDirPath = path.normalize(dirPath) + + for (const whitelistedDir of GITIGNORE_WHITELIST) { + const whitelistedPath = path.normalize(whitelistedDir) + + // Check if the whitelisted path is under this directory + if ( + whitelistedPath.startsWith(path.join(normalizedDirPath, path.sep)) || + whitelistedPath === normalizedDirPath + ) { + return true + } + + // Also check if this directory is under the whitelisted path + if (normalizedDirPath.startsWith(path.join(whitelistedPath, path.sep))) { + return true + } + } + + return false +} + +/** + * Special check for .roo directory + */ +function isRooPath(dirPath: string): boolean { + const normalizedPath = path.normalize(dirPath) + const dirName = path.basename(normalizedPath) + + // Check if this is .roo or under .roo using path segments + const segments = normalizedPath.split(path.sep) + return dirName === ".roo" || segments.includes(".roo") +} /** * List files in a directory, with optional recursive traversal @@ -101,7 +157,7 @@ function buildRipgrepArgs(dirPath: string, recursive: boolean): string[] { const args = ["--files", "--hidden", "--follow"] if (recursive) { - return [...args, ...buildRecursiveArgs(), dirPath] + return [...args, ...buildRecursiveArgs(dirPath), dirPath] } else { return [...args, ...buildNonRecursiveArgs(), dirPath] } @@ -110,14 +166,26 @@ function buildRipgrepArgs(dirPath: string, recursive: boolean): string[] { /** * Build ripgrep arguments for recursive directory traversal */ -function buildRecursiveArgs(): string[] { +function buildRecursiveArgs(dirPath?: string): string[] { const args: string[] = [] - // In recursive mode, respect .gitignore by default - // (ripgrep does this automatically) + // Check if this path is whitelisted (with defensive null check) + const isWhitelisted = dirPath ? isPathWhitelisted(dirPath) : false + + if (isWhitelisted) { + // For whitelisted paths, don't respect .gitignore + args.push("--no-ignore-vcs") + } + // Otherwise, ripgrep respects .gitignore by default // Apply directory exclusions for recursive searches + // But be more selective about hidden directories for (const dir of DIRS_TO_IGNORE) { + if (dir === ".*") { + // Don't use the broad .* pattern + // Instead, we'll handle hidden directories in shouldIncludeDirectory + continue + } args.push("-g", `!**/${dir}/**`) } @@ -209,7 +277,7 @@ async function listFilteredDirectories( const fullDirPath = path.join(currentPath, dirName) // Check if this directory should be included - if (shouldIncludeDirectory(dirName, recursive, gitignorePatterns)) { + if (shouldIncludeDirectory(dirName, recursive, gitignorePatterns, fullDirPath)) { // Add the directory to our results (with trailing slash) const formattedPath = fullDirPath.endsWith("/") ? fullDirPath : `${fullDirPath}/` directories.push(formattedPath) @@ -236,20 +304,43 @@ async function listFilteredDirectories( /** * Determine if a directory should be included in results based on filters */ -function shouldIncludeDirectory(dirName: string, recursive: boolean, gitignorePatterns: string[]): boolean { - // Skip hidden directories if configured to ignore them +function shouldIncludeDirectory( + dirName: string, + recursive: boolean, + gitignorePatterns: string[], + fullPath: string, +): boolean { + // Always include whitelisted paths + if (isPathWhitelisted(fullPath)) { + return true + } + + // Special handling for hidden directories if (dirName.startsWith(".") && DIRS_TO_IGNORE.includes(".*")) { + // Check if this hidden directory might contain whitelisted paths + if (mightContainWhitelistedPaths(fullPath)) { + // Only allow if it's actually part of a whitelisted path + // This ensures we don't accidentally include similar-named directories + return true + } + // Block all other hidden directories return false } // Check against explicit ignore patterns if (isDirectoryExplicitlyIgnored(dirName)) { - return false + // But allow if it might contain whitelisted paths + if (!mightContainWhitelistedPaths(fullPath)) { + return false + } } // Check against gitignore patterns in recursive mode if (recursive && gitignorePatterns.length > 0 && isIgnoredByGitignore(dirName, gitignorePatterns)) { - return false + // But allow if it might contain whitelisted paths + if (!mightContainWhitelistedPaths(fullPath)) { + return false + } } return true From 02627aa4345ace739e4b712186b2dfd05e360a02 Mon Sep 17 00:00:00 2001 From: MuriloFP Date: Sat, 5 Jul 2025 01:37:55 -0300 Subject: [PATCH 3/5] refactor: use ignore package and RooIgnoreController for gitignore handling - Removed custom gitignore parsing logic from list-files.ts - Added gitignore support to RooIgnoreController using the ignore package - Moved helper functions to ignore-utils.ts for better code organization - Added comprehensive tests for gitignore and whitelist functionality - Maintains backward compatibility while aligning with established patterns This addresses the review feedback about using the centralized RooIgnoreController and the standard ignore package instead of manual parsing. --- src/core/ignore/RooIgnoreController.ts | 73 ++++++ .../RooIgnoreController.gitignore.spec.ts | 245 ++++++++++++++++++ src/services/glob/ignore-utils.ts | 69 ++++- src/services/glob/list-files.ts | 161 +++--------- 4 files changed, 415 insertions(+), 133 deletions(-) create mode 100644 src/core/ignore/__tests__/RooIgnoreController.gitignore.spec.ts diff --git a/src/core/ignore/RooIgnoreController.ts b/src/core/ignore/RooIgnoreController.ts index fda6c371757..6b6f7a22bf2 100644 --- a/src/core/ignore/RooIgnoreController.ts +++ b/src/core/ignore/RooIgnoreController.ts @@ -3,6 +3,7 @@ import { fileExistsAtPath } from "../../utils/fs" import fs from "fs/promises" import ignore, { Ignore } from "ignore" import * as vscode from "vscode" +import { GITIGNORE_WHITELIST } from "../../services/glob/constants" export const LOCK_TEXT_SYMBOL = "\u{1F512}" @@ -14,7 +15,9 @@ export const LOCK_TEXT_SYMBOL = "\u{1F512}" export class RooIgnoreController { private cwd: string private ignoreInstance: Ignore + private gitignoreInstance: Ignore | null = null private disposables: vscode.Disposable[] = [] + private whitelist = GITIGNORE_WHITELIST rooIgnoreContent: string | undefined constructor(cwd: string) { @@ -31,6 +34,7 @@ export class RooIgnoreController { */ async initialize(): Promise { await this.loadRooIgnore() + await this.loadGitignore() } /** @@ -79,6 +83,75 @@ export class RooIgnoreController { } } + /** + * Load patterns from .gitignore if it exists + */ + async loadGitignore(): Promise { + try { + this.gitignoreInstance = ignore() + const gitignorePath = path.join(this.cwd, ".gitignore") + if (await fileExistsAtPath(gitignorePath)) { + const content = await fs.readFile(gitignorePath, "utf8") + this.gitignoreInstance.add(content) + } + } catch (error) { + console.error("Error loading .gitignore:", error) + this.gitignoreInstance = null + } + } + + /** + * Check if a path is whitelisted + * @param filePath - Path to check (relative to cwd) + * @returns true if path is whitelisted + */ + isWhitelisted(filePath: string): boolean { + // Convert to relative path if absolute + let relativePath: string + if (path.isAbsolute(filePath)) { + relativePath = path.relative(this.cwd, filePath) + } else { + relativePath = filePath + } + + // Normalize the path + const normalizedPath = path.normalize(relativePath).replace(/\\/g, "/") + + return this.whitelist.some((pattern) => { + const normalizedPattern = path.normalize(pattern).replace(/\\/g, "/") + // Check if this is the whitelisted path or under it + return ( + normalizedPath === normalizedPattern || + normalizedPath.startsWith(normalizedPattern + "/") || + // Check if this is a parent of the whitelisted path + normalizedPattern.startsWith(normalizedPath + "/") + ) + }) + } + + /** + * Check if a path is ignored by gitignore (considering whitelist) + * @param filePath - Path to check (relative to cwd) + * @returns true if path is ignored by gitignore and not whitelisted + */ + isGitignored(filePath: string): boolean { + if (!this.gitignoreInstance) { + return false + } + + // Check whitelist first + if (this.isWhitelisted(filePath)) { + return false + } + + try { + const relativePath = path.relative(this.cwd, path.resolve(this.cwd, filePath)).replace(/\\/g, "/") + return this.gitignoreInstance.ignores(relativePath) + } catch (error) { + return false + } + } + /** * Check if a file should be accessible to the LLM * @param filePath - Path to check (relative to cwd) diff --git a/src/core/ignore/__tests__/RooIgnoreController.gitignore.spec.ts b/src/core/ignore/__tests__/RooIgnoreController.gitignore.spec.ts new file mode 100644 index 00000000000..7328b9756c3 --- /dev/null +++ b/src/core/ignore/__tests__/RooIgnoreController.gitignore.spec.ts @@ -0,0 +1,245 @@ +// npx vitest core/ignore/__tests__/RooIgnoreController.gitignore.spec.ts + +import type { Mock } from "vitest" +import { RooIgnoreController } from "../RooIgnoreController" +import * as path from "path" +import * as fs from "fs/promises" +import { fileExistsAtPath } from "../../../utils/fs" +import { GITIGNORE_WHITELIST } from "../../../services/glob/constants" + +// Mock dependencies +vi.mock("fs/promises") +vi.mock("../../../utils/fs") + +// Mock vscode +vi.mock("vscode", () => { + const mockDisposable = { dispose: vi.fn() } + const mockEventEmitter = { + event: vi.fn(), + fire: vi.fn(), + } + + return { + workspace: { + createFileSystemWatcher: vi.fn(() => ({ + onDidCreate: vi.fn(() => mockDisposable), + onDidChange: vi.fn(() => mockDisposable), + onDidDelete: vi.fn(() => mockDisposable), + dispose: vi.fn(), + })), + }, + RelativePattern: vi.fn().mockImplementation((base, pattern) => ({ + base, + pattern, + })), + EventEmitter: vi.fn().mockImplementation(() => mockEventEmitter), + Disposable: { + from: vi.fn(), + }, + } +}) + +describe("RooIgnoreController - Gitignore Support", () => { + const TEST_CWD = "/test/path" + let controller: RooIgnoreController + let mockFileExists: Mock + let mockReadFile: Mock + + beforeEach(() => { + // Reset mocks + vi.clearAllMocks() + + // Setup fs mocks + mockFileExists = fileExistsAtPath as Mock + mockReadFile = fs.readFile as Mock + + // Create controller + controller = new RooIgnoreController(TEST_CWD) + }) + + describe("loadGitignore", () => { + it("should load .gitignore patterns on initialization when file exists", async () => { + // Setup mocks to simulate existing .gitignore file + mockFileExists.mockImplementation(async (filePath) => { + const pathStr = filePath.toString() + if (pathStr.endsWith(".gitignore")) return true + return false + }) + mockReadFile.mockResolvedValue("node_modules/\n*.log\nbuild/") + + // Initialize controller + await controller.initialize() + + // Verify gitignore was loaded + expect(mockFileExists).toHaveBeenCalledWith(path.join(TEST_CWD, ".gitignore")) + expect(mockReadFile).toHaveBeenCalledWith(path.join(TEST_CWD, ".gitignore"), "utf8") + + // Test that gitignore patterns are applied + expect(controller.isGitignored("node_modules/package.json")).toBe(true) + expect(controller.isGitignored("app.log")).toBe(true) + expect(controller.isGitignored("build/output.js")).toBe(true) + expect(controller.isGitignored("src/app.ts")).toBe(false) + }) + + it("should handle missing .gitignore gracefully", async () => { + // Setup mocks to simulate missing .gitignore file + mockFileExists.mockResolvedValue(false) + + // Initialize controller + await controller.initialize() + + // Verify no gitignore patterns are applied + expect(controller.isGitignored("node_modules/package.json")).toBe(false) + expect(controller.isGitignored("build/output.js")).toBe(false) + }) + + it("should handle errors when loading .gitignore", async () => { + // Setup mocks to simulate error + mockFileExists.mockImplementation(async (filePath) => { + const pathStr = filePath.toString() + if (pathStr.endsWith(".gitignore")) return true + return false + }) + mockReadFile.mockRejectedValue(new Error("Test file read error")) + + // Spy on console.error + const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + // Initialize controller - shouldn't throw + await controller.initialize() + + // Verify error was logged + expect(consoleSpy).toHaveBeenCalledWith("Error loading .gitignore:", expect.any(Error)) + + // Cleanup + consoleSpy.mockRestore() + }) + }) + + describe("isWhitelisted", () => { + beforeEach(async () => { + await controller.initialize() + }) + + it("should correctly identify whitelisted paths", () => { + // Test exact matches + expect(controller.isWhitelisted(".roo/temp/file.txt")).toBe(true) + expect(controller.isWhitelisted(".roo/temp/")).toBe(true) + expect(controller.isWhitelisted(".roo/temp")).toBe(true) + + // Test subdirectories + expect(controller.isWhitelisted(".roo/temp/subdir/file.txt")).toBe(true) + expect(controller.isWhitelisted(".roo/temp/deep/nested/path/file.txt")).toBe(true) + + // Test non-whitelisted paths + expect(controller.isWhitelisted(".roo/other/file.txt")).toBe(false) + expect(controller.isWhitelisted("src/.roo/temp/file.txt")).toBe(false) + expect(controller.isWhitelisted("temp/file.txt")).toBe(false) + }) + + it("should handle absolute paths correctly", () => { + const absolutePath = path.join(TEST_CWD, ".roo/temp/file.txt") + expect(controller.isWhitelisted(absolutePath)).toBe(true) + + const nonWhitelistedAbsolute = path.join(TEST_CWD, "node_modules/file.txt") + expect(controller.isWhitelisted(nonWhitelistedAbsolute)).toBe(false) + }) + + it("should handle path normalization", () => { + // Test with different path separators and formats + expect(controller.isWhitelisted(".roo\\temp\\file.txt")).toBe(true) + expect(controller.isWhitelisted("./.roo/temp/file.txt")).toBe(true) + expect(controller.isWhitelisted(".roo/./temp/file.txt")).toBe(true) + }) + }) + + describe("isGitignored with whitelist", () => { + beforeEach(async () => { + // Setup both .gitignore and .rooignore + mockFileExists.mockImplementation(async (filePath) => { + const pathStr = filePath.toString() + if (pathStr.endsWith(".gitignore")) return true + if (pathStr.endsWith(".rooignore")) return true + return false + }) + mockReadFile.mockImplementation(async (filePath) => { + const pathStr = filePath.toString() + if (pathStr.endsWith(".gitignore")) { + return ".roo/\nnode_modules/\n*.log" + } + if (pathStr.endsWith(".rooignore")) { + return "secrets/" + } + throw new Error("Unexpected file") + }) + await controller.initialize() + }) + + it("should respect whitelist even if path is in .gitignore", () => { + // .roo/temp/ is whitelisted even though .roo/ is in .gitignore + expect(controller.isGitignored(".roo/temp/file.txt")).toBe(false) + + // .roo/other/ is NOT whitelisted, so it should be gitignored + expect(controller.isGitignored(".roo/other/file.txt")).toBe(true) + + // Other gitignored paths should still be ignored + expect(controller.isGitignored("node_modules/package.json")).toBe(true) + expect(controller.isGitignored("app.log")).toBe(true) + }) + + it("should handle nested whitelisted paths in gitignored directories", () => { + // Even though .roo/ is gitignored, .roo/temp/ is whitelisted + expect(controller.isGitignored(".roo/temp/nested/deep/file.txt")).toBe(false) + expect(controller.isGitignored(".roo/temp/")).toBe(false) + }) + + it("should return false for non-gitignored paths", () => { + expect(controller.isGitignored("src/app.ts")).toBe(false) + expect(controller.isGitignored("README.md")).toBe(false) + }) + }) + + describe("integration with validateAccess", () => { + beforeEach(async () => { + // Setup both .gitignore and .rooignore + mockFileExists.mockImplementation(async (filePath) => { + const pathStr = filePath.toString() + if (pathStr.endsWith(".gitignore")) return true + if (pathStr.endsWith(".rooignore")) return true + return false + }) + mockReadFile.mockImplementation(async (filePath) => { + const pathStr = filePath.toString() + if (pathStr.endsWith(".gitignore")) { + return ".roo/\nnode_modules/" + } + if (pathStr.endsWith(".rooignore")) { + return "secrets/" + } + throw new Error("Unexpected file") + }) + await controller.initialize() + }) + + it("should allow access to whitelisted paths even if gitignored", () => { + // Whitelisted paths should be accessible + expect(controller.validateAccess(".roo/temp/file.txt")).toBe(true) + expect(controller.validateAccess(".roo/temp/subdir/data.json")).toBe(true) + + // Rooignored paths should be blocked + expect(controller.validateAccess("secrets/api-key.txt")).toBe(false) + + // Regular paths should be accessible + expect(controller.validateAccess("src/app.ts")).toBe(true) + }) + }) + + describe("whitelist constants", () => { + it("should have the correct whitelist entries", () => { + // Verify the whitelist contains expected entries + expect(GITIGNORE_WHITELIST).toContain(".roo/temp") + expect(GITIGNORE_WHITELIST).toBeInstanceOf(Array) + expect(GITIGNORE_WHITELIST.length).toBeGreaterThan(0) + }) + }) +}) diff --git a/src/services/glob/ignore-utils.ts b/src/services/glob/ignore-utils.ts index 9c80375e667..b8d868778b3 100644 --- a/src/services/glob/ignore-utils.ts +++ b/src/services/glob/ignore-utils.ts @@ -1,4 +1,5 @@ -import { DIRS_TO_IGNORE } from "./constants" +import * as path from "path" +import { DIRS_TO_IGNORE, GITIGNORE_WHITELIST } from "./constants" /** * Checks if a file path should be ignored based on the DIRS_TO_IGNORE patterns. @@ -43,3 +44,69 @@ export function isPathInIgnoredDirectory(filePath: string): boolean { return false } + +/** + * Check if a path is whitelisted or is a parent of a whitelisted path + */ +export function isPathWhitelisted(absolutePath: string): boolean { + const normalizedPath = path.normalize(absolutePath) + + for (const whitelistedDir of GITIGNORE_WHITELIST) { + const whitelistedPath = path.normalize(whitelistedDir) + + // Check if this is the whitelisted path or a parent of it + if (normalizedPath === whitelistedPath || whitelistedPath.startsWith(path.join(normalizedPath, path.sep))) { + return true + } + } + + return false +} + +/** + * Check if a directory might contain whitelisted paths + */ +export function mightContainWhitelistedPaths(dirPath: string): boolean { + const normalizedDirPath = path.normalize(dirPath) + + for (const whitelistedDir of GITIGNORE_WHITELIST) { + const whitelistedPath = path.normalize(whitelistedDir) + + // Check if the whitelisted path is under this directory + if ( + whitelistedPath.startsWith(path.join(normalizedDirPath, path.sep)) || + whitelistedPath === normalizedDirPath + ) { + return true + } + + // Also check if this directory is under the whitelisted path + if (normalizedDirPath.startsWith(path.join(whitelistedPath, path.sep))) { + return true + } + } + + return false +} + +/** + * Check if a directory is in our explicit ignore list + */ +export function isDirectoryExplicitlyIgnored(dirName: string): boolean { + for (const pattern of DIRS_TO_IGNORE) { + // Exact name matching + if (pattern === dirName) { + return true + } + + // Path patterns that contain / + if (pattern.includes("/")) { + const pathParts = pattern.split("/") + if (pathParts[0] === dirName) { + return true + } + } + } + + return false +} diff --git a/src/services/glob/list-files.ts b/src/services/glob/list-files.ts index 97e793ae78a..ba74668cc65 100644 --- a/src/services/glob/list-files.ts +++ b/src/services/glob/list-files.ts @@ -3,65 +3,11 @@ import * as path from "path" import * as fs from "fs" import * as childProcess from "child_process" import * as vscode from "vscode" +import ignore, { Ignore } from "ignore" import { arePathsEqual } from "../../utils/path" import { getBinPath } from "../../services/ripgrep" -import { DIRS_TO_IGNORE, GITIGNORE_WHITELIST } from "./constants" - -/** - * Check if a path is whitelisted or is a parent of a whitelisted path - */ -function isPathWhitelisted(absolutePath: string): boolean { - const normalizedPath = path.normalize(absolutePath) - - for (const whitelistedDir of GITIGNORE_WHITELIST) { - const whitelistedPath = path.normalize(whitelistedDir) - - // Check if this is the whitelisted path or a parent of it - if (normalizedPath === whitelistedPath || whitelistedPath.startsWith(path.join(normalizedPath, path.sep))) { - return true - } - } - - return false -} - -/** - * Check if a directory might contain whitelisted paths - */ -function mightContainWhitelistedPaths(dirPath: string): boolean { - const normalizedDirPath = path.normalize(dirPath) - - for (const whitelistedDir of GITIGNORE_WHITELIST) { - const whitelistedPath = path.normalize(whitelistedDir) - - // Check if the whitelisted path is under this directory - if ( - whitelistedPath.startsWith(path.join(normalizedDirPath, path.sep)) || - whitelistedPath === normalizedDirPath - ) { - return true - } - - // Also check if this directory is under the whitelisted path - if (normalizedDirPath.startsWith(path.join(whitelistedPath, path.sep))) { - return true - } - } - - return false -} - -/** - * Special check for .roo directory - */ -function isRooPath(dirPath: string): boolean { - const normalizedPath = path.normalize(dirPath) - const dirName = path.basename(normalizedPath) - - // Check if this is .roo or under .roo using path segments - const segments = normalizedPath.split(path.sep) - return dirName === ".roo" || segments.includes(".roo") -} +import { DIRS_TO_IGNORE } from "./constants" +import { isPathWhitelisted, mightContainWhitelistedPaths, isDirectoryExplicitlyIgnored } from "./ignore-utils" /** * List files in a directory, with optional recursive traversal @@ -91,8 +37,8 @@ export async function listFiles(dirPath: string, recursive: boolean, limit: numb const files = await listFilesWithRipgrep(rgPath, dirPath, recursive, limit) // Get directories with proper filtering - const gitignorePatterns = await parseGitignoreFile(dirPath, recursive) - const directories = await listFilteredDirectories(dirPath, recursive, gitignorePatterns) + const gitignoreInstance = await loadGitignore(dirPath, recursive) + const directories = await listFilteredDirectories(dirPath, recursive, gitignoreInstance) // Combine and format the results return formatAndCombineResults(files, directories, limit) @@ -169,8 +115,8 @@ function buildRipgrepArgs(dirPath: string, recursive: boolean): string[] { function buildRecursiveArgs(dirPath?: string): string[] { const args: string[] = [] - // Check if this path is whitelisted (with defensive null check) - const isWhitelisted = dirPath ? isPathWhitelisted(dirPath) : false + // Check if this path is whitelisted + const isWhitelisted = dirPath ? isPathWhitelisted(path.resolve(dirPath)) : false if (isWhitelisted) { // For whitelisted paths, don't respect .gitignore @@ -221,11 +167,11 @@ function buildNonRecursiveArgs(): string[] { } /** - * Parse the .gitignore file if it exists and is relevant + * Load the .gitignore file if it exists and is relevant */ -async function parseGitignoreFile(dirPath: string, recursive: boolean): Promise { +async function loadGitignore(dirPath: string, recursive: boolean): Promise { if (!recursive) { - return [] // Only needed for recursive mode + return null // Only needed for recursive mode } const absolutePath = path.resolve(dirPath) @@ -239,18 +185,17 @@ async function parseGitignoreFile(dirPath: string, recursive: boolean): Promise< .catch(() => false) if (!exists) { - return [] + return null } - // Read and parse .gitignore file + // Read and parse .gitignore file using the ignore package const content = await fs.promises.readFile(gitignorePath, "utf8") - return content - .split("\n") - .map((line) => line.trim()) - .filter((line) => line && !line.startsWith("#")) + const ig = ignore() + ig.add(content) + return ig } catch (err) { console.warn(`Error reading .gitignore: ${err}`) - return [] // Continue without gitignore patterns on error + return null // Continue without gitignore on error } } @@ -260,7 +205,7 @@ async function parseGitignoreFile(dirPath: string, recursive: boolean): Promise< async function listFilteredDirectories( dirPath: string, recursive: boolean, - gitignorePatterns: string[], + gitignoreInstance: Ignore | null, ): Promise { const absolutePath = path.resolve(dirPath) const directories: string[] = [] @@ -277,7 +222,7 @@ async function listFilteredDirectories( const fullDirPath = path.join(currentPath, dirName) // Check if this directory should be included - if (shouldIncludeDirectory(dirName, recursive, gitignorePatterns, fullDirPath)) { + if (shouldIncludeDirectory(dirName, recursive, gitignoreInstance, fullDirPath, currentPath)) { // Add the directory to our results (with trailing slash) const formattedPath = fullDirPath.endsWith("/") ? fullDirPath : `${fullDirPath}/` directories.push(formattedPath) @@ -307,8 +252,9 @@ async function listFilteredDirectories( function shouldIncludeDirectory( dirName: string, recursive: boolean, - gitignorePatterns: string[], + gitignoreInstance: Ignore | null, fullPath: string, + basePath: string, ): boolean { // Always include whitelisted paths if (isPathWhitelisted(fullPath)) { @@ -336,68 +282,19 @@ function shouldIncludeDirectory( } // Check against gitignore patterns in recursive mode - if (recursive && gitignorePatterns.length > 0 && isIgnoredByGitignore(dirName, gitignorePatterns)) { - // But allow if it might contain whitelisted paths - if (!mightContainWhitelistedPaths(fullPath)) { - return false - } - } - - return true -} - -/** - * Check if a directory is in our explicit ignore list - */ -function isDirectoryExplicitlyIgnored(dirName: string): boolean { - for (const pattern of DIRS_TO_IGNORE) { - // Exact name matching - if (pattern === dirName) { - return true - } - - // Path patterns that contain / - if (pattern.includes("/")) { - const pathParts = pattern.split("/") - if (pathParts[0] === dirName) { - return true + if (recursive && gitignoreInstance) { + // Get relative path from the base directory for gitignore checking + const relativePath = path.relative(basePath, fullPath).replace(/\\/g, "/") + + if (gitignoreInstance.ignores(relativePath)) { + // But allow if it might contain whitelisted paths + if (!mightContainWhitelistedPaths(fullPath)) { + return false } } } - return false -} - -/** - * Check if a directory matches any gitignore patterns - */ -function isIgnoredByGitignore(dirName: string, gitignorePatterns: string[]): boolean { - for (const pattern of gitignorePatterns) { - // Directory patterns (ending with /) - if (pattern.endsWith("/")) { - const dirPattern = pattern.slice(0, -1) - if (dirName === dirPattern) { - return true - } - if (pattern.startsWith("**/") && dirName === dirPattern.slice(3)) { - return true - } - } - // Simple name patterns - else if (dirName === pattern) { - return true - } - // Wildcard patterns - else if (pattern.includes("*")) { - const regexPattern = pattern.replace(/\\/g, "\\\\").replace(/\./g, "\\.").replace(/\*/g, ".*") - const regex = new RegExp(`^${regexPattern}$`) - if (regex.test(dirName)) { - return true - } - } - } - - return false + return true } /** From 6e1a48dbdb60788e201f58b03a55f44e3844a286 Mon Sep 17 00:00:00 2001 From: MuriloFP Date: Sat, 5 Jul 2025 14:45:19 -0300 Subject: [PATCH 4/5] refactor: simplify whitelist implementation based on review feedback - Remove unnecessary complexity and manual gitignore parsing - Fix root cause: ripgrep's glob patterns filtering hidden directories - Delete unused files (ignore-utils.ts, RooIgnoreController.gitignore.spec.ts) - Simplify RooIgnoreController back to original implementation - Keep all whitelist logic contained in list-files.ts - Add isPathInIgnoredDirectory methods to scanner and file-watcher The implementation is now ~30 lines instead of ~100 lines, directly addressing the root cause identified by daniel-lxs where ripgrep's -g '!**/.*/**' pattern was filtering out hidden directories even when --no-ignore-vcs was used. --- src/core/ignore/RooIgnoreController.ts | 73 ------ .../RooIgnoreController.gitignore.spec.ts | 245 ------------------ .../code-index/processors/file-watcher.ts | 24 +- src/services/code-index/processors/scanner.ts | 25 +- src/services/glob/ignore-utils.ts | 112 -------- src/services/glob/list-files.ts | 105 +++++--- 6 files changed, 109 insertions(+), 475 deletions(-) delete mode 100644 src/core/ignore/__tests__/RooIgnoreController.gitignore.spec.ts delete mode 100644 src/services/glob/ignore-utils.ts diff --git a/src/core/ignore/RooIgnoreController.ts b/src/core/ignore/RooIgnoreController.ts index 6b6f7a22bf2..fda6c371757 100644 --- a/src/core/ignore/RooIgnoreController.ts +++ b/src/core/ignore/RooIgnoreController.ts @@ -3,7 +3,6 @@ import { fileExistsAtPath } from "../../utils/fs" import fs from "fs/promises" import ignore, { Ignore } from "ignore" import * as vscode from "vscode" -import { GITIGNORE_WHITELIST } from "../../services/glob/constants" export const LOCK_TEXT_SYMBOL = "\u{1F512}" @@ -15,9 +14,7 @@ export const LOCK_TEXT_SYMBOL = "\u{1F512}" export class RooIgnoreController { private cwd: string private ignoreInstance: Ignore - private gitignoreInstance: Ignore | null = null private disposables: vscode.Disposable[] = [] - private whitelist = GITIGNORE_WHITELIST rooIgnoreContent: string | undefined constructor(cwd: string) { @@ -34,7 +31,6 @@ export class RooIgnoreController { */ async initialize(): Promise { await this.loadRooIgnore() - await this.loadGitignore() } /** @@ -83,75 +79,6 @@ export class RooIgnoreController { } } - /** - * Load patterns from .gitignore if it exists - */ - async loadGitignore(): Promise { - try { - this.gitignoreInstance = ignore() - const gitignorePath = path.join(this.cwd, ".gitignore") - if (await fileExistsAtPath(gitignorePath)) { - const content = await fs.readFile(gitignorePath, "utf8") - this.gitignoreInstance.add(content) - } - } catch (error) { - console.error("Error loading .gitignore:", error) - this.gitignoreInstance = null - } - } - - /** - * Check if a path is whitelisted - * @param filePath - Path to check (relative to cwd) - * @returns true if path is whitelisted - */ - isWhitelisted(filePath: string): boolean { - // Convert to relative path if absolute - let relativePath: string - if (path.isAbsolute(filePath)) { - relativePath = path.relative(this.cwd, filePath) - } else { - relativePath = filePath - } - - // Normalize the path - const normalizedPath = path.normalize(relativePath).replace(/\\/g, "/") - - return this.whitelist.some((pattern) => { - const normalizedPattern = path.normalize(pattern).replace(/\\/g, "/") - // Check if this is the whitelisted path or under it - return ( - normalizedPath === normalizedPattern || - normalizedPath.startsWith(normalizedPattern + "/") || - // Check if this is a parent of the whitelisted path - normalizedPattern.startsWith(normalizedPath + "/") - ) - }) - } - - /** - * Check if a path is ignored by gitignore (considering whitelist) - * @param filePath - Path to check (relative to cwd) - * @returns true if path is ignored by gitignore and not whitelisted - */ - isGitignored(filePath: string): boolean { - if (!this.gitignoreInstance) { - return false - } - - // Check whitelist first - if (this.isWhitelisted(filePath)) { - return false - } - - try { - const relativePath = path.relative(this.cwd, path.resolve(this.cwd, filePath)).replace(/\\/g, "/") - return this.gitignoreInstance.ignores(relativePath) - } catch (error) { - return false - } - } - /** * Check if a file should be accessible to the LLM * @param filePath - Path to check (relative to cwd) diff --git a/src/core/ignore/__tests__/RooIgnoreController.gitignore.spec.ts b/src/core/ignore/__tests__/RooIgnoreController.gitignore.spec.ts deleted file mode 100644 index 7328b9756c3..00000000000 --- a/src/core/ignore/__tests__/RooIgnoreController.gitignore.spec.ts +++ /dev/null @@ -1,245 +0,0 @@ -// npx vitest core/ignore/__tests__/RooIgnoreController.gitignore.spec.ts - -import type { Mock } from "vitest" -import { RooIgnoreController } from "../RooIgnoreController" -import * as path from "path" -import * as fs from "fs/promises" -import { fileExistsAtPath } from "../../../utils/fs" -import { GITIGNORE_WHITELIST } from "../../../services/glob/constants" - -// Mock dependencies -vi.mock("fs/promises") -vi.mock("../../../utils/fs") - -// Mock vscode -vi.mock("vscode", () => { - const mockDisposable = { dispose: vi.fn() } - const mockEventEmitter = { - event: vi.fn(), - fire: vi.fn(), - } - - return { - workspace: { - createFileSystemWatcher: vi.fn(() => ({ - onDidCreate: vi.fn(() => mockDisposable), - onDidChange: vi.fn(() => mockDisposable), - onDidDelete: vi.fn(() => mockDisposable), - dispose: vi.fn(), - })), - }, - RelativePattern: vi.fn().mockImplementation((base, pattern) => ({ - base, - pattern, - })), - EventEmitter: vi.fn().mockImplementation(() => mockEventEmitter), - Disposable: { - from: vi.fn(), - }, - } -}) - -describe("RooIgnoreController - Gitignore Support", () => { - const TEST_CWD = "/test/path" - let controller: RooIgnoreController - let mockFileExists: Mock - let mockReadFile: Mock - - beforeEach(() => { - // Reset mocks - vi.clearAllMocks() - - // Setup fs mocks - mockFileExists = fileExistsAtPath as Mock - mockReadFile = fs.readFile as Mock - - // Create controller - controller = new RooIgnoreController(TEST_CWD) - }) - - describe("loadGitignore", () => { - it("should load .gitignore patterns on initialization when file exists", async () => { - // Setup mocks to simulate existing .gitignore file - mockFileExists.mockImplementation(async (filePath) => { - const pathStr = filePath.toString() - if (pathStr.endsWith(".gitignore")) return true - return false - }) - mockReadFile.mockResolvedValue("node_modules/\n*.log\nbuild/") - - // Initialize controller - await controller.initialize() - - // Verify gitignore was loaded - expect(mockFileExists).toHaveBeenCalledWith(path.join(TEST_CWD, ".gitignore")) - expect(mockReadFile).toHaveBeenCalledWith(path.join(TEST_CWD, ".gitignore"), "utf8") - - // Test that gitignore patterns are applied - expect(controller.isGitignored("node_modules/package.json")).toBe(true) - expect(controller.isGitignored("app.log")).toBe(true) - expect(controller.isGitignored("build/output.js")).toBe(true) - expect(controller.isGitignored("src/app.ts")).toBe(false) - }) - - it("should handle missing .gitignore gracefully", async () => { - // Setup mocks to simulate missing .gitignore file - mockFileExists.mockResolvedValue(false) - - // Initialize controller - await controller.initialize() - - // Verify no gitignore patterns are applied - expect(controller.isGitignored("node_modules/package.json")).toBe(false) - expect(controller.isGitignored("build/output.js")).toBe(false) - }) - - it("should handle errors when loading .gitignore", async () => { - // Setup mocks to simulate error - mockFileExists.mockImplementation(async (filePath) => { - const pathStr = filePath.toString() - if (pathStr.endsWith(".gitignore")) return true - return false - }) - mockReadFile.mockRejectedValue(new Error("Test file read error")) - - // Spy on console.error - const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {}) - - // Initialize controller - shouldn't throw - await controller.initialize() - - // Verify error was logged - expect(consoleSpy).toHaveBeenCalledWith("Error loading .gitignore:", expect.any(Error)) - - // Cleanup - consoleSpy.mockRestore() - }) - }) - - describe("isWhitelisted", () => { - beforeEach(async () => { - await controller.initialize() - }) - - it("should correctly identify whitelisted paths", () => { - // Test exact matches - expect(controller.isWhitelisted(".roo/temp/file.txt")).toBe(true) - expect(controller.isWhitelisted(".roo/temp/")).toBe(true) - expect(controller.isWhitelisted(".roo/temp")).toBe(true) - - // Test subdirectories - expect(controller.isWhitelisted(".roo/temp/subdir/file.txt")).toBe(true) - expect(controller.isWhitelisted(".roo/temp/deep/nested/path/file.txt")).toBe(true) - - // Test non-whitelisted paths - expect(controller.isWhitelisted(".roo/other/file.txt")).toBe(false) - expect(controller.isWhitelisted("src/.roo/temp/file.txt")).toBe(false) - expect(controller.isWhitelisted("temp/file.txt")).toBe(false) - }) - - it("should handle absolute paths correctly", () => { - const absolutePath = path.join(TEST_CWD, ".roo/temp/file.txt") - expect(controller.isWhitelisted(absolutePath)).toBe(true) - - const nonWhitelistedAbsolute = path.join(TEST_CWD, "node_modules/file.txt") - expect(controller.isWhitelisted(nonWhitelistedAbsolute)).toBe(false) - }) - - it("should handle path normalization", () => { - // Test with different path separators and formats - expect(controller.isWhitelisted(".roo\\temp\\file.txt")).toBe(true) - expect(controller.isWhitelisted("./.roo/temp/file.txt")).toBe(true) - expect(controller.isWhitelisted(".roo/./temp/file.txt")).toBe(true) - }) - }) - - describe("isGitignored with whitelist", () => { - beforeEach(async () => { - // Setup both .gitignore and .rooignore - mockFileExists.mockImplementation(async (filePath) => { - const pathStr = filePath.toString() - if (pathStr.endsWith(".gitignore")) return true - if (pathStr.endsWith(".rooignore")) return true - return false - }) - mockReadFile.mockImplementation(async (filePath) => { - const pathStr = filePath.toString() - if (pathStr.endsWith(".gitignore")) { - return ".roo/\nnode_modules/\n*.log" - } - if (pathStr.endsWith(".rooignore")) { - return "secrets/" - } - throw new Error("Unexpected file") - }) - await controller.initialize() - }) - - it("should respect whitelist even if path is in .gitignore", () => { - // .roo/temp/ is whitelisted even though .roo/ is in .gitignore - expect(controller.isGitignored(".roo/temp/file.txt")).toBe(false) - - // .roo/other/ is NOT whitelisted, so it should be gitignored - expect(controller.isGitignored(".roo/other/file.txt")).toBe(true) - - // Other gitignored paths should still be ignored - expect(controller.isGitignored("node_modules/package.json")).toBe(true) - expect(controller.isGitignored("app.log")).toBe(true) - }) - - it("should handle nested whitelisted paths in gitignored directories", () => { - // Even though .roo/ is gitignored, .roo/temp/ is whitelisted - expect(controller.isGitignored(".roo/temp/nested/deep/file.txt")).toBe(false) - expect(controller.isGitignored(".roo/temp/")).toBe(false) - }) - - it("should return false for non-gitignored paths", () => { - expect(controller.isGitignored("src/app.ts")).toBe(false) - expect(controller.isGitignored("README.md")).toBe(false) - }) - }) - - describe("integration with validateAccess", () => { - beforeEach(async () => { - // Setup both .gitignore and .rooignore - mockFileExists.mockImplementation(async (filePath) => { - const pathStr = filePath.toString() - if (pathStr.endsWith(".gitignore")) return true - if (pathStr.endsWith(".rooignore")) return true - return false - }) - mockReadFile.mockImplementation(async (filePath) => { - const pathStr = filePath.toString() - if (pathStr.endsWith(".gitignore")) { - return ".roo/\nnode_modules/" - } - if (pathStr.endsWith(".rooignore")) { - return "secrets/" - } - throw new Error("Unexpected file") - }) - await controller.initialize() - }) - - it("should allow access to whitelisted paths even if gitignored", () => { - // Whitelisted paths should be accessible - expect(controller.validateAccess(".roo/temp/file.txt")).toBe(true) - expect(controller.validateAccess(".roo/temp/subdir/data.json")).toBe(true) - - // Rooignored paths should be blocked - expect(controller.validateAccess("secrets/api-key.txt")).toBe(false) - - // Regular paths should be accessible - expect(controller.validateAccess("src/app.ts")).toBe(true) - }) - }) - - describe("whitelist constants", () => { - it("should have the correct whitelist entries", () => { - // Verify the whitelist contains expected entries - expect(GITIGNORE_WHITELIST).toContain(".roo/temp") - expect(GITIGNORE_WHITELIST).toBeInstanceOf(Array) - expect(GITIGNORE_WHITELIST.length).toBeGreaterThan(0) - }) - }) -}) diff --git a/src/services/code-index/processors/file-watcher.ts b/src/services/code-index/processors/file-watcher.ts index 9a1fc3c9af4..e263130ced7 100644 --- a/src/services/code-index/processors/file-watcher.ts +++ b/src/services/code-index/processors/file-watcher.ts @@ -22,7 +22,8 @@ import { import { codeParser } from "./parser" import { CacheManager } from "../cache-manager" import { generateNormalizedAbsolutePath, generateRelativeFilePath } from "../shared/get-relative-path" -import { isPathInIgnoredDirectory } from "../../glob/ignore-utils" +import { DIRS_TO_IGNORE } from "../../glob/constants" +import * as path from "path" /** * Implementation of the file watcher interface @@ -455,7 +456,7 @@ export class FileWatcher implements IFileWatcher { async processFile(filePath: string): Promise { try { // Check if file is in an ignored directory - if (isPathInIgnoredDirectory(filePath)) { + if (this.isPathInIgnoredDirectory(filePath)) { return { path: filePath, status: "skipped" as const, @@ -543,4 +544,23 @@ export class FileWatcher implements IFileWatcher { } } } + + /** + * Check if a path is within an ignored directory + */ + private isPathInIgnoredDirectory(filePath: string): boolean { + const normalizedPath = path.normalize(filePath) + const pathParts = normalizedPath.split(path.sep) + + return pathParts.some((part) => { + // Check if any part of the path matches an ignored directory + return DIRS_TO_IGNORE.some((dir) => { + if (dir === ".*") { + // Special case for hidden directories (starting with .) + return part.startsWith(".") && part !== "." + } + return part === dir + }) + }) + } } diff --git a/src/services/code-index/processors/scanner.ts b/src/services/code-index/processors/scanner.ts index f24650f4bcd..2144eac2ee7 100644 --- a/src/services/code-index/processors/scanner.ts +++ b/src/services/code-index/processors/scanner.ts @@ -23,7 +23,7 @@ import { PARSING_CONCURRENCY, BATCH_PROCESSING_CONCURRENCY, } from "../constants" -import { isPathInIgnoredDirectory } from "../../glob/ignore-utils" +import { DIRS_TO_IGNORE } from "../../glob/constants" export class DirectoryScanner implements IDirectoryScanner { constructor( @@ -68,8 +68,8 @@ export class DirectoryScanner implements IDirectoryScanner { const ext = path.extname(filePath).toLowerCase() const relativeFilePath = generateRelativeFilePath(filePath) - // Check if file is in an ignored directory using the shared helper - if (isPathInIgnoredDirectory(filePath)) { + // Check if file is in an ignored directory + if (this.isPathInIgnoredDirectory(filePath)) { return false } @@ -362,4 +362,23 @@ export class DirectoryScanner implements IDirectoryScanner { } } } + + /** + * Check if a path is within an ignored directory + */ + private isPathInIgnoredDirectory(filePath: string): boolean { + const normalizedPath = path.normalize(filePath) + const pathParts = normalizedPath.split(path.sep) + + return pathParts.some((part) => { + // Check if any part of the path matches an ignored directory + return DIRS_TO_IGNORE.some((dir) => { + if (dir === ".*") { + // Special case for hidden directories (starting with .) + return part.startsWith(".") && part !== "." + } + return part === dir + }) + }) + } } diff --git a/src/services/glob/ignore-utils.ts b/src/services/glob/ignore-utils.ts deleted file mode 100644 index b8d868778b3..00000000000 --- a/src/services/glob/ignore-utils.ts +++ /dev/null @@ -1,112 +0,0 @@ -import * as path from "path" -import { DIRS_TO_IGNORE, GITIGNORE_WHITELIST } from "./constants" - -/** - * Checks if a file path should be ignored based on the DIRS_TO_IGNORE patterns. - * This function handles special patterns like ".*" for hidden directories. - * - * @param filePath The file path to check - * @returns true if the path should be ignored, false otherwise - */ -export function isPathInIgnoredDirectory(filePath: string): boolean { - // Normalize path separators - const normalizedPath = filePath.replace(/\\/g, "/") - const pathParts = normalizedPath.split("/") - - // Check each directory in the path against DIRS_TO_IGNORE - for (const part of pathParts) { - // Skip empty parts (from leading or trailing slashes) - if (!part) continue - - // Handle the ".*" pattern for hidden directories - if (DIRS_TO_IGNORE.includes(".*") && part.startsWith(".") && part !== ".") { - return true - } - - // Check for exact matches - if (DIRS_TO_IGNORE.includes(part)) { - return true - } - } - - // Check if path contains any ignored directory pattern - for (const dir of DIRS_TO_IGNORE) { - if (dir === ".*") { - // Already handled above - continue - } - - // Check if the directory appears in the path - if (normalizedPath.includes(`/${dir}/`)) { - return true - } - } - - return false -} - -/** - * Check if a path is whitelisted or is a parent of a whitelisted path - */ -export function isPathWhitelisted(absolutePath: string): boolean { - const normalizedPath = path.normalize(absolutePath) - - for (const whitelistedDir of GITIGNORE_WHITELIST) { - const whitelistedPath = path.normalize(whitelistedDir) - - // Check if this is the whitelisted path or a parent of it - if (normalizedPath === whitelistedPath || whitelistedPath.startsWith(path.join(normalizedPath, path.sep))) { - return true - } - } - - return false -} - -/** - * Check if a directory might contain whitelisted paths - */ -export function mightContainWhitelistedPaths(dirPath: string): boolean { - const normalizedDirPath = path.normalize(dirPath) - - for (const whitelistedDir of GITIGNORE_WHITELIST) { - const whitelistedPath = path.normalize(whitelistedDir) - - // Check if the whitelisted path is under this directory - if ( - whitelistedPath.startsWith(path.join(normalizedDirPath, path.sep)) || - whitelistedPath === normalizedDirPath - ) { - return true - } - - // Also check if this directory is under the whitelisted path - if (normalizedDirPath.startsWith(path.join(whitelistedPath, path.sep))) { - return true - } - } - - return false -} - -/** - * Check if a directory is in our explicit ignore list - */ -export function isDirectoryExplicitlyIgnored(dirName: string): boolean { - for (const pattern of DIRS_TO_IGNORE) { - // Exact name matching - if (pattern === dirName) { - return true - } - - // Path patterns that contain / - if (pattern.includes("/")) { - const pathParts = pattern.split("/") - if (pathParts[0] === dirName) { - return true - } - } - } - - return false -} diff --git a/src/services/glob/list-files.ts b/src/services/glob/list-files.ts index ba74668cc65..fbd6a375faa 100644 --- a/src/services/glob/list-files.ts +++ b/src/services/glob/list-files.ts @@ -6,8 +6,7 @@ import * as vscode from "vscode" import ignore, { Ignore } from "ignore" import { arePathsEqual } from "../../utils/path" import { getBinPath } from "../../services/ripgrep" -import { DIRS_TO_IGNORE } from "./constants" -import { isPathWhitelisted, mightContainWhitelistedPaths, isDirectoryExplicitlyIgnored } from "./ignore-utils" +import { DIRS_TO_IGNORE, GITIGNORE_WHITELIST } from "./constants" /** * List files in a directory, with optional recursive traversal @@ -102,10 +101,15 @@ function buildRipgrepArgs(dirPath: string, recursive: boolean): string[] { // Base arguments to list files const args = ["--files", "--hidden", "--follow"] + // Add --no-ignore-vcs for whitelisted paths + if (isPathInWhitelist(dirPath)) { + args.push("--no-ignore-vcs") + } + if (recursive) { return [...args, ...buildRecursiveArgs(dirPath), dirPath] } else { - return [...args, ...buildNonRecursiveArgs(), dirPath] + return [...args, ...buildNonRecursiveArgs(dirPath), dirPath] } } @@ -113,25 +117,15 @@ function buildRipgrepArgs(dirPath: string, recursive: boolean): string[] { * Build ripgrep arguments for recursive directory traversal */ function buildRecursiveArgs(dirPath?: string): string[] { - const args: string[] = [] - - // Check if this path is whitelisted - const isWhitelisted = dirPath ? isPathWhitelisted(path.resolve(dirPath)) : false - - if (isWhitelisted) { - // For whitelisted paths, don't respect .gitignore - args.push("--no-ignore-vcs") + // Skip all exclusion patterns for whitelisted paths + if (dirPath && isPathInWhitelist(dirPath)) { + return [] } - // Otherwise, ripgrep respects .gitignore by default + + const args: string[] = [] // Apply directory exclusions for recursive searches - // But be more selective about hidden directories for (const dir of DIRS_TO_IGNORE) { - if (dir === ".*") { - // Don't use the broad .* pattern - // Instead, we'll handle hidden directories in shouldIncludeDirectory - continue - } args.push("-g", `!**/${dir}/**`) } @@ -141,7 +135,12 @@ function buildRecursiveArgs(dirPath?: string): string[] { /** * Build ripgrep arguments for non-recursive directory listing */ -function buildNonRecursiveArgs(): string[] { +function buildNonRecursiveArgs(dirPath?: string): string[] { + // Skip all exclusion patterns for whitelisted paths + if (dirPath && isPathInWhitelist(dirPath)) { + return ["-g", "*", "--maxdepth", "1"] + } + const args: string[] = [] // For non-recursive, limit to the current directory level @@ -256,29 +255,23 @@ function shouldIncludeDirectory( fullPath: string, basePath: string, ): boolean { - // Always include whitelisted paths - if (isPathWhitelisted(fullPath)) { - return true - } - - // Special handling for hidden directories - if (dirName.startsWith(".") && DIRS_TO_IGNORE.includes(".*")) { - // Check if this hidden directory might contain whitelisted paths - if (mightContainWhitelistedPaths(fullPath)) { - // Only allow if it's actually part of a whitelisted path - // This ensures we don't accidentally include similar-named directories + // Allow parent directories of whitelisted paths + const normalizedPath = path.normalize(fullPath) + for (const whitelisted of GITIGNORE_WHITELIST) { + const normalizedWhitelisted = path.normalize(path.resolve(basePath, whitelisted)) + if (normalizedWhitelisted.startsWith(normalizedPath + path.sep) || normalizedWhitelisted === normalizedPath) { return true } - // Block all other hidden directories - return false } // Check against explicit ignore patterns if (isDirectoryExplicitlyIgnored(dirName)) { - // But allow if it might contain whitelisted paths - if (!mightContainWhitelistedPaths(fullPath)) { - return false - } + return false + } + + // Special handling for hidden directories + if (dirName.startsWith(".") && DIRS_TO_IGNORE.includes(".*")) { + return false } // Check against gitignore patterns in recursive mode @@ -287,10 +280,7 @@ function shouldIncludeDirectory( const relativePath = path.relative(basePath, fullPath).replace(/\\/g, "/") if (gitignoreInstance.ignores(relativePath)) { - // But allow if it might contain whitelisted paths - if (!mightContainWhitelistedPaths(fullPath)) { - return false - } + return false } } @@ -400,3 +390,38 @@ async function execRipgrep(rgPath: string, args: string[], limit: number): Promi } }) } + +/** + * Check if a path is whitelisted + * @param dirPath - Path to check + * @returns true if path is whitelisted + */ +function isPathInWhitelist(dirPath: string): boolean { + const normalizedPath = path.normalize(dirPath) + return GITIGNORE_WHITELIST.some((whitelisted) => { + const normalizedWhitelisted = path.normalize(whitelisted) + return normalizedPath.includes(normalizedWhitelisted) || normalizedWhitelisted.includes(normalizedPath) + }) +} + +/** + * Check if a directory is in our explicit ignore list + */ +function isDirectoryExplicitlyIgnored(dirName: string): boolean { + for (const pattern of DIRS_TO_IGNORE) { + // Exact name matching + if (pattern === dirName) { + return true + } + + // Path patterns that contain / + if (pattern.includes("/")) { + const pathParts = pattern.split("/") + if (pathParts[0] === dirName) { + return true + } + } + } + + return false +} From 37d5041377f61bd52b4fc8c79298644aa478d1d5 Mon Sep 17 00:00:00 2001 From: MuriloFP Date: Sat, 5 Jul 2025 15:10:49 -0300 Subject: [PATCH 5/5] Added .roo/temp/ to the .gitignore. --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index 6f6bcd99dea..5a87ad65a6b 100644 --- a/.gitignore +++ b/.gitignore @@ -18,6 +18,9 @@ bin/ # Local prompts and rules /local-prompts +# Roo temp files +.roo/temp/ + # Test environment .test_env .vscode-test/