From efaaed9f7381f7d9d1d027193b870cf0596adb4e Mon Sep 17 00:00:00 2001 From: MuriloFP Date: Thu, 3 Jul 2025 15:26:24 -0300 Subject: [PATCH 01/10] 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 2ad4bf65fc..3e6619993e 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 e6251d67b2..b9a9b52db7 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 3b178f234f..213d7b8314 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 1024c373201c718509ecec7345ca6761897ed41c Mon Sep 17 00:00:00 2001 From: Murilo Date: Fri, 27 Jun 2025 00:33:08 -0300 Subject: [PATCH 02/10] The `list_files` tool with `recursive: true` was returning empty results when targeting directories that start with a dot (e.g., `.roo-memory`). This happened because the recursive mode was applying a blanket exclusion pattern `!**/.*//**` that excluded all files inside any hidden directory, even when the user explicitly requested to list that directory. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `src/services/glob/list-files.ts` - **Modified `buildRecursiveArgs()`**: Removed the problematic `!**/.*//**` exclusion pattern for hidden directories in recursive mode - **Enhanced `listFilteredDirectories()`**: Added `isTargetDir` parameter to distinguish between explicitly targeted directories and discovered subdirectories - **Updated `shouldIncludeDirectory()`**: Always include explicitly targeted directories (even if hidden), while still applying ignore rules to subdirectories found during traversal - **Before**: `list_files` with `path: ".roo-memory"` and `recursive: true` → Empty results - **After**: `list_files` with `path: ".roo-memory"` and `recursive: true` → Returns directory contents - **Preserved**: Hidden subdirectories discovered during traversal are still filtered out This maintains consistency with `.gitignore` and `.rooignore` mechanisms while ensuring explicitly targeted directories are always processed. Fixes #2992 --- src/services/glob/list-files.ts | 35 ++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/src/services/glob/list-files.ts b/src/services/glob/list-files.ts index 05fa8a1d7b..e7e78511d2 100644 --- a/src/services/glob/list-files.ts +++ b/src/services/glob/list-files.ts @@ -209,6 +209,14 @@ function buildRecursiveArgs(): string[] { // Apply directory exclusions for recursive searches for (const dir of DIRS_TO_IGNORE) { + // Special handling for hidden directories pattern + if (dir === ".*") { + // Don't exclude hidden directories at the ripgrep level for recursive mode + // This allows explicitly targeted hidden directories to be processed + // Hidden subdirectories will be filtered during directory traversal instead + continue + } + args.push("-g", `!**/${dir}/**`) } @@ -310,9 +318,10 @@ async function listFilteredDirectories( ignoreInstance: ReturnType, ): Promise { const absolutePath = path.resolve(dirPath) + const targetDirPath = absolutePath // Store the explicitly targeted directory const directories: string[] = [] - async function scanDirectory(currentPath: string): Promise { + async function scanDirectory(currentPath: string, isTargetDir: boolean = false): Promise { try { // List all entries in the current directory const entries = await fs.promises.readdir(currentPath, { withFileTypes: true }) @@ -324,14 +333,14 @@ async function listFilteredDirectories( const fullDirPath = path.join(currentPath, dirName) // Check if this directory should be included - if (shouldIncludeDirectory(dirName, fullDirPath, dirPath, ignoreInstance)) { + if (shouldIncludeDirectory(dirName, fullDirPath, dirPath, ignoreInstance, isTargetDir)) { // Add the directory to our results (with trailing slash) const formattedPath = fullDirPath.endsWith("/") ? fullDirPath : `${fullDirPath}/` directories.push(formattedPath) // If recursive mode and not a ignored directory, scan subdirectories if (recursive && !isDirectoryExplicitlyIgnored(dirName)) { - await scanDirectory(fullDirPath) + await scanDirectory(fullDirPath, false) // Subdirectories are not target dirs } } } @@ -342,8 +351,8 @@ async function listFilteredDirectories( } } - // Start scanning from the root directory - await scanDirectory(absolutePath) + // Start scanning from the root directory - this is the explicitly targeted directory + await scanDirectory(absolutePath, true) return directories } @@ -356,7 +365,23 @@ function shouldIncludeDirectory( fullDirPath: string, basePath: string, ignoreInstance: ReturnType, + isTargetDir: boolean = false, ): boolean { + // If this is the explicitly targeted directory, always include it + // (unless it's explicitly ignored by name, not by the .* pattern) + if (isTargetDir) { + // Only apply non-hidden-directory ignore rules to target directories + const nonHiddenIgnorePatterns = DIRS_TO_IGNORE.filter((pattern) => pattern !== ".*") + for (const pattern of nonHiddenIgnorePatterns) { + if (pattern === dirName || (pattern.includes("/") && pattern.split("/")[0] === dirName)) { + return false + } + } + return true + } + + // For non-target directories (subdirectories found during traversal), apply all ignore rules + // Skip hidden directories if configured to ignore them if (dirName.startsWith(".") && DIRS_TO_IGNORE.includes(".*")) { return false From 649a69e90d471151df879c8c4848e737858b2c40 Mon Sep 17 00:00:00 2001 From: Murilo Pires <50873657+MuriloFP@users.noreply.github.com> Date: Fri, 27 Jun 2025 00:46:51 -0300 Subject: [PATCH 03/10] Update src/services/glob/list-files.ts Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> --- src/services/glob/list-files.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/services/glob/list-files.ts b/src/services/glob/list-files.ts index e7e78511d2..7cf26c7528 100644 --- a/src/services/glob/list-files.ts +++ b/src/services/glob/list-files.ts @@ -318,7 +318,6 @@ async function listFilteredDirectories( ignoreInstance: ReturnType, ): Promise { const absolutePath = path.resolve(dirPath) - const targetDirPath = absolutePath // Store the explicitly targeted directory const directories: string[] = [] async function scanDirectory(currentPath: string, isTargetDir: boolean = false): Promise { From f71bbf0d34abe1accdd8184ef90af887e45453ba Mon Sep 17 00:00:00 2001 From: MuriloFP Date: Sat, 28 Jun 2025 13:46:30 -0300 Subject: [PATCH 04/10] fix: list_files recursive mode now works correctly for hidden directories - Fixed ripgrep exclusion pattern from `!**/.*//**` to `!**/.*/**` - Hidden directories (like .git/) now appear in recursive listings but their contents are excluded - Contents of hidden directories are only shown when explicitly targeting that directory - Prevents context window overflow from massive hidden directories like .git/ - Maintains consistent behavior: show directory exists, avoid flooding with contents Fixes #2992 --- src/services/glob/list-files.ts | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/services/glob/list-files.ts b/src/services/glob/list-files.ts index 7cf26c7528..027cb42766 100644 --- a/src/services/glob/list-files.ts +++ b/src/services/glob/list-files.ts @@ -192,7 +192,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] } @@ -201,25 +201,31 @@ 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 we're explicitly targeting a hidden directory + const targetDirName = path.basename(dirPath) + const isTargetingHiddenDir = targetDirName.startsWith(".") + // Apply directory exclusions for recursive searches for (const dir of DIRS_TO_IGNORE) { // Special handling for hidden directories pattern if (dir === ".*") { - // Don't exclude hidden directories at the ripgrep level for recursive mode + // Only exclude hidden directories if we're not explicitly targeting one // This allows explicitly targeted hidden directories to be processed - // Hidden subdirectories will be filtered during directory traversal instead + // while excluding them from general recursive searches + if (!isTargetingHiddenDir) { + args.push("-g", `!**/.*/**`) + } continue } args.push("-g", `!**/${dir}/**`) } - return args } @@ -338,7 +344,11 @@ async function listFilteredDirectories( directories.push(formattedPath) // If recursive mode and not a ignored directory, scan subdirectories - if (recursive && !isDirectoryExplicitlyIgnored(dirName)) { + // Don't recurse into hidden directories unless they are the explicit target + const isHiddenDir = dirName.startsWith(".") + const shouldRecurse = + recursive && !isDirectoryExplicitlyIgnored(dirName) && !(isHiddenDir && !isTargetDir) + if (shouldRecurse) { await scanDirectory(fullDirPath, false) // Subdirectories are not target dirs } } From d518f27ecc20ea41b13c074cb5a9b2e4cdbd9b79 Mon Sep 17 00:00:00 2001 From: MuriloFP Date: Tue, 8 Jul 2025 15:03:58 -0300 Subject: [PATCH 05/10] fix(glob): show top-level hidden directories in list_files - Modified list-files.ts to show hidden directories at top level - Added test coverage for hidden directory visibility behavior - Maintains existing functionality while fixing the .roo directory issue Addresses PR #5176 feedback with simplified implementation --- .../glob/__tests__/list-files.spec.ts | 121 ++++++++++++++++++ src/services/glob/constants.ts | 1 + src/services/glob/list-files.ts | 100 +++++++++++---- 3 files changed, 198 insertions(+), 24 deletions(-) diff --git a/src/services/glob/__tests__/list-files.spec.ts b/src/services/glob/__tests__/list-files.spec.ts index 6c133a732a..4e55f9558e 100644 --- a/src/services/glob/__tests__/list-files.spec.ts +++ b/src/services/glob/__tests__/list-files.spec.ts @@ -216,3 +216,124 @@ describe("list-files symlink support", () => { expect(hasCDir).toBe(true) }) }) + +describe("hidden directory exclusion", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("should exclude .git subdirectories from recursive directory listing", async () => { + // Mock filesystem structure with .git subdirectories + const mockReaddir = vi.fn() + vi.mocked(fs.promises).readdir = mockReaddir + + // Mock the directory structure: + // /test/ + // .git/ + // hooks/ + // objects/ + // src/ + // components/ + mockReaddir + .mockResolvedValueOnce([ + { name: ".git", isDirectory: () => true, isSymbolicLink: () => false }, + { name: "src", isDirectory: () => true, isSymbolicLink: () => false }, + ]) + .mockResolvedValueOnce([ + // src subdirectories (should be included) + { name: "components", isDirectory: () => true, isSymbolicLink: () => false }, + ]) + .mockResolvedValueOnce([]) // components/ is empty + + // Mock ripgrep to return no files + const mockSpawn = vi.mocked(childProcess.spawn) + const mockProcess = { + stdout: { + on: vi.fn((event, callback) => { + if (event === "data") { + // No files returned + } + }), + }, + stderr: { + on: vi.fn(), + }, + on: vi.fn((event, callback) => { + if (event === "close") { + setTimeout(() => callback(0), 10) + } + }), + kill: vi.fn(), + } + mockSpawn.mockReturnValue(mockProcess as any) + + // Call listFiles with recursive=true + const [result] = await listFiles("/test", true, 100) + + // Verify that .git subdirectories are NOT included + const directories = result.filter((item) => item.endsWith("/")) + + // More specific checks - look for exact paths + const hasSrcDir = directories.some((dir) => dir.endsWith("/test/src/") || dir.endsWith("src/")) + const hasComponentsDir = directories.some( + (dir) => + dir.endsWith("/test/src/components/") || dir.endsWith("src/components/") || dir.includes("components/"), + ) + const hasGitDir = directories.some((dir) => dir.includes(".git/")) + + // Should include src/ and src/components/ but NOT .git/ or its subdirectories + expect(hasSrcDir).toBe(true) + expect(hasComponentsDir).toBe(true) + + // Should NOT include .git (hidden directories are excluded) + expect(hasGitDir).toBe(false) + }) + + it("should allow explicit targeting of hidden directories", async () => { + // Mock filesystem structure for explicit .roo-memory targeting + const mockReaddir = vi.fn() + vi.mocked(fs.promises).readdir = mockReaddir + + // Mock .roo-memory directory contents + mockReaddir.mockResolvedValueOnce([ + { name: "tasks", isDirectory: () => true, isSymbolicLink: () => false }, + { name: "context", isDirectory: () => true, isSymbolicLink: () => false }, + ]) + + // Mock ripgrep to return no files + const mockSpawn = vi.mocked(childProcess.spawn) + const mockProcess = { + stdout: { + on: vi.fn((event, callback) => { + if (event === "data") { + // No files returned + } + }), + }, + stderr: { + on: vi.fn(), + }, + on: vi.fn((event, callback) => { + if (event === "close") { + setTimeout(() => callback(0), 10) + } + }), + kill: vi.fn(), + } + mockSpawn.mockReturnValue(mockProcess as any) + + // Call listFiles explicitly targeting .roo-memory directory + const [result] = await listFiles("/test/.roo-memory", true, 100) + + // When explicitly targeting a hidden directory, its subdirectories should be included + const directories = result.filter((item) => item.endsWith("/")) + + const hasTasksDir = directories.some((dir) => dir.includes(".roo-memory/tasks/") || dir.includes("tasks/")) + const hasContextDir = directories.some( + (dir) => dir.includes(".roo-memory/context/") || dir.includes("context/"), + ) + + expect(hasTasksDir).toBe(true) + expect(hasContextDir).toBe(true) + }) +}) diff --git a/src/services/glob/constants.ts b/src/services/glob/constants.ts index 1ddcc37df9..380e4afaf3 100644 --- a/src/services/glob/constants.ts +++ b/src/services/glob/constants.ts @@ -20,5 +20,6 @@ export const DIRS_TO_IGNORE = [ "deps", "pkg", "Pods", + ".git", ".*", ] diff --git a/src/services/glob/list-files.ts b/src/services/glob/list-files.ts index 027cb42766..2bb11cc4e7 100644 --- a/src/services/glob/list-files.ts +++ b/src/services/glob/list-files.ts @@ -245,8 +245,11 @@ function buildNonRecursiveArgs(): string[] { // Apply directory exclusions for non-recursive searches for (const dir of DIRS_TO_IGNORE) { if (dir === ".*") { - // For hidden files/dirs in non-recursive mode - args.push("-g", "!.*") + // For hidden directories in non-recursive mode, we want to show the directories + // themselves but not their contents. Since we're using --maxdepth 1, this + // naturally happens - we just need to avoid excluding the directories entirely. + // We'll let the directory scanning logic handle the visibility. + continue } else { // Direct children only args.push("-g", `!${dir}`) @@ -326,7 +329,11 @@ async function listFilteredDirectories( const absolutePath = path.resolve(dirPath) const directories: string[] = [] - async function scanDirectory(currentPath: string, isTargetDir: boolean = false): Promise { + async function scanDirectory( + currentPath: string, + isTargetDir: boolean = false, + insideExplicitHiddenTarget: boolean = false, + ): Promise { try { // List all entries in the current directory const entries = await fs.promises.readdir(currentPath, { withFileTypes: true }) @@ -338,19 +345,38 @@ async function listFilteredDirectories( const fullDirPath = path.join(currentPath, dirName) // Check if this directory should be included - if (shouldIncludeDirectory(dirName, fullDirPath, dirPath, ignoreInstance, isTargetDir)) { + // Subdirectories found during scanning are never target directories themselves + if (shouldIncludeDirectory(dirName, fullDirPath, dirPath, ignoreInstance, false, insideExplicitHiddenTarget)) { // Add the directory to our results (with trailing slash) const formattedPath = fullDirPath.endsWith("/") ? fullDirPath : `${fullDirPath}/` directories.push(formattedPath) + } - // If recursive mode and not a ignored directory, scan subdirectories - // Don't recurse into hidden directories unless they are the explicit target - const isHiddenDir = dirName.startsWith(".") - const shouldRecurse = - recursive && !isDirectoryExplicitlyIgnored(dirName) && !(isHiddenDir && !isTargetDir) - if (shouldRecurse) { - await scanDirectory(fullDirPath, false) // Subdirectories are not target dirs - } + // If recursive mode and not a ignored directory, scan subdirectories + // Don't recurse into hidden directories unless they are the explicit target + // or we're already inside an explicitly targeted hidden directory + const isHiddenDir = dirName.startsWith(".") + + // Use the same logic as shouldIncludeDirectory for recursion decisions + // When inside an explicitly targeted hidden directory, only block critical directories + let shouldRecurseIntoDir = true + if (insideExplicitHiddenTarget) { + // Only apply the most critical ignore patterns when inside explicit hidden target + const criticalIgnorePatterns = ["node_modules", ".git", "__pycache__", "venv", "env"] + shouldRecurseIntoDir = !criticalIgnorePatterns.includes(dirName) + } else { + shouldRecurseIntoDir = !isDirectoryExplicitlyIgnored(dirName) + } + + const shouldRecurse = + recursive && + shouldRecurseIntoDir && + !(isHiddenDir && DIRS_TO_IGNORE.includes(".*") && !isTargetDir && !insideExplicitHiddenTarget) + if (shouldRecurse) { + // If we're entering a hidden directory that's the target, or we're already inside one, + // mark that we're inside an explicitly targeted hidden directory + const newInsideExplicitHiddenTarget = insideExplicitHiddenTarget || (isHiddenDir && isTargetDir) + await scanDirectory(fullDirPath, false, newInsideExplicitHiddenTarget) } } } @@ -360,8 +386,12 @@ async function listFilteredDirectories( } } - // Start scanning from the root directory - this is the explicitly targeted directory - await scanDirectory(absolutePath, true) + // Start scanning from the root directory + // For environment details generation, we don't want to treat the root as a "target" + // if we're doing a general recursive scan, as this would include hidden directories + // Only treat as target if we're explicitly scanning a single hidden directory + const isExplicitHiddenTarget = path.basename(absolutePath).startsWith(".") + await scanDirectory(absolutePath, isExplicitHiddenTarget, isExplicitHiddenTarget) return directories } @@ -375,9 +405,10 @@ function shouldIncludeDirectory( basePath: string, ignoreInstance: ReturnType, isTargetDir: boolean = false, + insideExplicitHiddenTarget: boolean = false, ): boolean { - // If this is the explicitly targeted directory, always include it - // (unless it's explicitly ignored by name, not by the .* pattern) + // If this is the explicitly targeted directory, allow it even if it's hidden + // This preserves the ability to explicitly target hidden directories like .roo-memory if (isTargetDir) { // Only apply non-hidden-directory ignore rules to target directories const nonHiddenIgnorePatterns = DIRS_TO_IGNORE.filter((pattern) => pattern !== ".*") @@ -389,16 +420,32 @@ function shouldIncludeDirectory( return true } - // For non-target directories (subdirectories found during traversal), apply all ignore rules - - // Skip hidden directories if configured to ignore them - if (dirName.startsWith(".") && DIRS_TO_IGNORE.includes(".*")) { - return false + // If we're inside an explicitly targeted hidden directory, allow subdirectories + // even if they would normally be filtered out by the ".*" pattern or other ignore rules + if (insideExplicitHiddenTarget) { + // Only apply the most critical ignore patterns when inside explicit hidden target + // Allow temp, rules, etc. but still block node_modules, .git, etc. + const criticalIgnorePatterns = ["node_modules", ".git", "__pycache__", "venv", "env"] + for (const pattern of criticalIgnorePatterns) { + if (pattern === dirName || (pattern.includes("/") && pattern.split("/")[0] === dirName)) { + return false + } + } + // Check against gitignore patterns using the ignore library + const relativePath = path.relative(basePath, fullDirPath) + const normalizedPath = relativePath.replace(/\\/g, "/") + if (ignoreInstance.ignores(normalizedPath) || ignoreInstance.ignores(normalizedPath + "/")) { + return false + } + return true } - // Check against explicit ignore patterns - if (isDirectoryExplicitlyIgnored(dirName)) { - return false + // Check against explicit ignore patterns (excluding the ".*" pattern for now) + const nonHiddenIgnorePatterns = DIRS_TO_IGNORE.filter((pattern) => pattern !== ".*") + for (const pattern of nonHiddenIgnorePatterns) { + if (pattern === dirName || (pattern.includes("/") && pattern.split("/")[0] === dirName)) { + return false + } } // Check against gitignore patterns using the ignore library @@ -424,6 +471,11 @@ function isDirectoryExplicitlyIgnored(dirName: string): boolean { return true } + // Skip the ".*" pattern - it's handled specially to allow top-level visibility + if (pattern === ".*") { + continue + } + // Path patterns that contain / if (pattern.includes("/")) { const pathParts = pattern.split("/") From a1252f8994955dd8f101c59b96f41f717775b75d Mon Sep 17 00:00:00 2001 From: MuriloFP Date: Thu, 10 Jul 2025 16:54:32 -0300 Subject: [PATCH 06/10] fix(glob): include top-level files when recursively listing ignored directories - Fix issue where files at root level of directories in DIRS_TO_IGNORE were excluded - Add explicit include patterns (* and **/*) when targeting ignored directories - Modify exclusion pattern to use !*/dir/** instead of !**/dir/** for target directory - Add comprehensive test case for .roo/temp scenario Fixes #5176 --- .../glob/__tests__/list-files.spec.ts | 57 +++++++++++++++++++ src/services/glob/list-files.ts | 35 ++++++++++-- 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/src/services/glob/__tests__/list-files.spec.ts b/src/services/glob/__tests__/list-files.spec.ts index 4e55f9558e..8b8c1c84a2 100644 --- a/src/services/glob/__tests__/list-files.spec.ts +++ b/src/services/glob/__tests__/list-files.spec.ts @@ -336,4 +336,61 @@ describe("hidden directory exclusion", () => { expect(hasTasksDir).toBe(true) expect(hasContextDir).toBe(true) }) + + it("should include top-level files when recursively listing a hidden directory that's also in DIRS_TO_IGNORE", async () => { + // This test specifically addresses the bug where files at the root level of .roo/temp + // were being excluded when using recursive listing + const mockSpawn = vi.mocked(childProcess.spawn) + const mockProcess = { + stdout: { + on: vi.fn((event, callback) => { + if (event === "data") { + // Simulate files that should be found in .roo/temp + setTimeout(() => { + callback(".roo/temp/teste1.md\n") + callback(".roo/temp/22/test2.md\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) + + // Mock directory listing for .roo/temp + const mockReaddir = vi.fn() + vi.mocked(fs.promises).readdir = mockReaddir + mockReaddir.mockResolvedValueOnce([{ name: "22", isDirectory: () => true, isSymbolicLink: () => false }]) + + // Call listFiles targeting .roo/temp (which is both hidden and in DIRS_TO_IGNORE) + const [files] = await listFiles("/test/.roo/temp", true, 100) + + // Verify ripgrep was called with correct arguments + const [rgPath, args] = mockSpawn.mock.calls[0] + expect(args).toContain("--no-ignore-vcs") + expect(args).toContain("--no-ignore") + + // Check for the inclusion patterns that should be added + expect(args).toContain("-g") + const gIndex = args.indexOf("-g") + expect(args[gIndex + 1]).toBe("*") + + // Verify that both top-level and nested files are included + const fileNames = files.map((f) => path.basename(f)) + expect(fileNames).toContain("teste1.md") + expect(fileNames).toContain("test2.md") + + // Ensure the top-level file is actually included + const topLevelFile = files.find((f) => f.endsWith("teste1.md")) + expect(topLevelFile).toBeTruthy() + }) }) diff --git a/src/services/glob/list-files.ts b/src/services/glob/list-files.ts index 2bb11cc4e7..d26ea6c6aa 100644 --- a/src/services/glob/list-files.ts +++ b/src/services/glob/list-files.ts @@ -208,23 +208,48 @@ function buildRecursiveArgs(dirPath: string): string[] { // (ripgrep does this automatically) // Check if we're explicitly targeting a hidden directory + // We need to check all parts of the path, not just the basename + const pathParts = dirPath.split(path.sep).filter((part) => part !== "") + const isTargetingHiddenDir = pathParts.some((part) => part.startsWith(".")) + + // Get the target directory name to check if it's in the ignore list const targetDirName = path.basename(dirPath) - const isTargetingHiddenDir = targetDirName.startsWith(".") + const isTargetInIgnoreList = DIRS_TO_IGNORE.includes(targetDirName) + + // If targeting a hidden directory or a directory in the ignore list, + // use special handling to ensure all files are shown + if (isTargetingHiddenDir || isTargetInIgnoreList) { + args.push("--no-ignore-vcs") + args.push("--no-ignore") + + // When targeting an ignored directory, we need to be careful with glob patterns + // Add a pattern to explicitly include files at the root level + args.push("-g", "*") + args.push("-g", "**/*") + } // Apply directory exclusions for recursive searches for (const dir of DIRS_TO_IGNORE) { // Special handling for hidden directories pattern if (dir === ".*") { - // Only exclude hidden directories if we're not explicitly targeting one - // This allows explicitly targeted hidden directories to be processed - // while excluding them from general recursive searches + // If we're explicitly targeting a hidden directory, don't exclude hidden files/dirs + // This allows the target hidden directory and all its contents to be listed if (!isTargetingHiddenDir) { + // Not targeting hidden dir: exclude all hidden directories args.push("-g", `!**/.*/**`) } + // If targeting hidden dir: don't add any exclusion for hidden directories continue } - args.push("-g", `!**/${dir}/**`) + // When targeting a directory that's in the ignore list, modify the exclusion pattern + // to only exclude nested directories with the same name, not the root level + if (dir === targetDirName && isTargetInIgnoreList) { + // Only exclude subdirectories, not files at the root level + args.push("-g", `!*/${dir}/**`) + } else { + args.push("-g", `!**/${dir}/**`) + } } return args } From 299bce8cffb1c516a8e26d3013da8ecc7215aced Mon Sep 17 00:00:00 2001 From: MuriloFP Date: Mon, 14 Jul 2025 14:30:40 -0300 Subject: [PATCH 07/10] fix(list-files): improve handling of explicitly targeted ignored directories - Fixed issue where recursive listing would skip files in explicitly targeted directories that are in DIRS_TO_IGNORE - When targeting a directory like 'temp' that's in the ignore list, we now skip adding exclusion patterns for it - This ensures all files in the target directory are listed while still preventing recursion into nested directories with the same ignored name - Also fixed path resolution to convert relative paths from ripgrep to absolute paths --- src/services/glob/list-files.ts | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/services/glob/list-files.ts b/src/services/glob/list-files.ts index d26ea6c6aa..1b64ed312f 100644 --- a/src/services/glob/list-files.ts +++ b/src/services/glob/list-files.ts @@ -181,6 +181,7 @@ async function listFilesWithRipgrep( ): Promise { const absolutePath = path.resolve(dirPath) const rgArgs = buildRipgrepArgs(absolutePath, recursive) + return execRipgrep(rgPath, rgArgs, limit) } @@ -242,15 +243,21 @@ function buildRecursiveArgs(dirPath: string): string[] { continue } - // When targeting a directory that's in the ignore list, modify the exclusion pattern - // to only exclude nested directories with the same name, not the root level + // When explicitly targeting a directory that's in the ignore list (e.g., "temp"), + // we need special handling: + // - Don't add any exclusion pattern for the target directory itself + // - Only exclude nested subdirectories with the same name + // This ensures all files in the target directory are listed, while still + // preventing recursion into nested directories with the same ignored name if (dir === targetDirName && isTargetInIgnoreList) { - // Only exclude subdirectories, not files at the root level - args.push("-g", `!*/${dir}/**`) - } else { - args.push("-g", `!**/${dir}/**`) + // Skip adding any exclusion pattern - we want to see everything in the target directory + continue } + + // For all other cases, exclude the directory pattern globally + args.push("-g", `!**/${dir}/**`) } + return args } @@ -373,6 +380,7 @@ async function listFilteredDirectories( // Subdirectories found during scanning are never target directories themselves if (shouldIncludeDirectory(dirName, fullDirPath, dirPath, ignoreInstance, false, insideExplicitHiddenTarget)) { // Add the directory to our results (with trailing slash) + // fullDirPath is already absolute since it's built with path.join from absolutePath const formattedPath = fullDirPath.endsWith("/") ? fullDirPath : `${fullDirPath}/` directories.push(formattedPath) } @@ -543,6 +551,9 @@ function formatAndCombineResults(files: string[], directories: string[], limit: */ async function execRipgrep(rgPath: string, args: string[], limit: number): Promise { return new Promise((resolve, reject) => { + // Extract the directory path from args (it's the last argument) + const searchDir = args[args.length - 1] + const rgProcess = childProcess.spawn(rgPath, args) let output = "" let results: string[] = [] @@ -608,7 +619,9 @@ async function execRipgrep(rgPath: string, args: string[], limit: number): Promi // Process each complete line for (const line of lines) { if (line.trim() && results.length < limit) { - results.push(line) + // Convert relative path from ripgrep to absolute path + const absolutePath = path.resolve(searchDir, line) + results.push(absolutePath) } else if (results.length >= limit) { break } From 17d723a5398a5ee7e64d19da82ca920760c32b34 Mon Sep 17 00:00:00 2001 From: MuriloFP Date: Mon, 14 Jul 2025 14:41:34 -0300 Subject: [PATCH 08/10] fix(tests): add missing mocks for list-files tests --- src/services/glob/__tests__/list-files.spec.ts | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/services/glob/__tests__/list-files.spec.ts b/src/services/glob/__tests__/list-files.spec.ts index 8b8c1c84a2..c1c84ac451 100644 --- a/src/services/glob/__tests__/list-files.spec.ts +++ b/src/services/glob/__tests__/list-files.spec.ts @@ -3,6 +3,18 @@ import * as path from "path" import { listFiles } from "../list-files" import * as childProcess from "child_process" +vi.mock("child_process") +vi.mock("fs") +vi.mock("vscode", () => ({ + env: { + appRoot: "/mock/vscode/app/root", + }, +})) + +vi.mock("../../ripgrep", () => ({ + getBinPath: vi.fn().mockResolvedValue("/mock/path/to/rg"), +})) + vi.mock("../list-files", async () => { const actual = await vi.importActual("../list-files") return { From 82dfd28f1f63a99a0898dfd52cdcf8aa77a974e2 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Wed, 16 Jul 2025 18:30:04 -0500 Subject: [PATCH 09/10] refactor: improve list-files implementation - Remove redundant path resolution in listFilesWithRipgrep - Convert CRITICAL_IGNORE_PATTERNS to Set for better performance - Standardize error message format across all console.warn calls --- src/services/glob/list-files.ts | 222 +++++++++++++++++++++----------- 1 file changed, 145 insertions(+), 77 deletions(-) diff --git a/src/services/glob/list-files.ts b/src/services/glob/list-files.ts index 1b64ed312f..7347515784 100644 --- a/src/services/glob/list-files.ts +++ b/src/services/glob/list-files.ts @@ -8,6 +8,20 @@ import { arePathsEqual } from "../../utils/path" import { getBinPath } from "../../services/ripgrep" import { DIRS_TO_IGNORE } from "./constants" +/** + * Context object for directory scanning operations + */ +interface ScanContext { + /** Whether this is the explicitly targeted directory */ + isTargetDir: boolean + /** Whether we're inside an explicitly targeted hidden directory */ + insideExplicitHiddenTarget: boolean + /** The base path for the scan operation */ + basePath: string + /** The ignore instance for gitignore handling */ + ignoreInstance: ReturnType +} + /** * List files in a directory, with optional recursive traversal * @@ -70,7 +84,13 @@ async function getFirstLevelDirectories(dirPath: string, ignoreInstance: ReturnT for (const entry of entries) { if (entry.isDirectory() && !entry.isSymbolicLink()) { const fullDirPath = path.join(absolutePath, entry.name) - if (shouldIncludeDirectory(entry.name, fullDirPath, dirPath, ignoreInstance)) { + const context: ScanContext = { + isTargetDir: false, + insideExplicitHiddenTarget: false, + basePath: dirPath, + ignoreInstance, + } + if (shouldIncludeDirectory(entry.name, fullDirPath, context)) { const formattedPath = fullDirPath.endsWith("/") ? fullDirPath : `${fullDirPath}/` directories.push(formattedPath) } @@ -179,10 +199,14 @@ async function listFilesWithRipgrep( recursive: boolean, limit: number, ): Promise { - const absolutePath = path.resolve(dirPath) - const rgArgs = buildRipgrepArgs(absolutePath, recursive) + const rgArgs = buildRipgrepArgs(dirPath, recursive) + + const relativePaths = await execRipgrep(rgPath, rgArgs, limit) - return execRipgrep(rgPath, rgArgs, limit) + // Convert relative paths from ripgrep to absolute paths + // Resolve dirPath once here for the mapping operation + const absolutePath = path.resolve(dirPath) + return relativePaths.map((relativePath) => path.resolve(absolutePath, relativePath)) } /** @@ -209,8 +233,11 @@ function buildRecursiveArgs(dirPath: string): string[] { // (ripgrep does this automatically) // Check if we're explicitly targeting a hidden directory - // We need to check all parts of the path, not just the basename - const pathParts = dirPath.split(path.sep).filter((part) => part !== "") + // Normalize the path first to handle edge cases + const normalizedPath = path.normalize(dirPath) + // Split by separator and filter out empty parts + // This handles cases like trailing slashes, multiple separators, etc. + const pathParts = normalizedPath.split(path.sep).filter((part) => part.length > 0) const isTargetingHiddenDir = pathParts.some((part) => part.startsWith(".")) // Get the target directory name to check if it's in the ignore list @@ -310,7 +337,7 @@ async function createIgnoreInstance(dirPath: string): Promise { + // For environment details generation, we don't want to treat the root as a "target" + // if we're doing a general recursive scan, as this would include hidden directories + // Only treat as target if we're explicitly scanning a single hidden directory + const isExplicitHiddenTarget = path.basename(absolutePath).startsWith(".") + + // Create initial context for the scan + const initialContext: ScanContext = { + isTargetDir: isExplicitHiddenTarget, + insideExplicitHiddenTarget: isExplicitHiddenTarget, + basePath: dirPath, + ignoreInstance, + } + + async function scanDirectory(currentPath: string, context: ScanContext): Promise { try { // List all entries in the current directory const entries = await fs.promises.readdir(currentPath, { withFileTypes: true }) @@ -376,9 +412,15 @@ async function listFilteredDirectories( const dirName = entry.name const fullDirPath = path.join(currentPath, dirName) - // Check if this directory should be included + // Create context for subdirectory checks // Subdirectories found during scanning are never target directories themselves - if (shouldIncludeDirectory(dirName, fullDirPath, dirPath, ignoreInstance, false, insideExplicitHiddenTarget)) { + const subdirContext: ScanContext = { + ...context, + isTargetDir: false, + } + + // Check if this directory should be included + if (shouldIncludeDirectory(dirName, fullDirPath, subdirContext)) { // Add the directory to our results (with trailing slash) // fullDirPath is already absolute since it's built with path.join from absolutePath const formattedPath = fullDirPath.endsWith("/") ? fullDirPath : `${fullDirPath}/` @@ -393,10 +435,9 @@ async function listFilteredDirectories( // Use the same logic as shouldIncludeDirectory for recursion decisions // When inside an explicitly targeted hidden directory, only block critical directories let shouldRecurseIntoDir = true - if (insideExplicitHiddenTarget) { + if (context.insideExplicitHiddenTarget) { // Only apply the most critical ignore patterns when inside explicit hidden target - const criticalIgnorePatterns = ["node_modules", ".git", "__pycache__", "venv", "env"] - shouldRecurseIntoDir = !criticalIgnorePatterns.includes(dirName) + shouldRecurseIntoDir = !CRITICAL_IGNORE_PATTERNS.has(dirName) } else { shouldRecurseIntoDir = !isDirectoryExplicitlyIgnored(dirName) } @@ -404,94 +445,122 @@ async function listFilteredDirectories( const shouldRecurse = recursive && shouldRecurseIntoDir && - !(isHiddenDir && DIRS_TO_IGNORE.includes(".*") && !isTargetDir && !insideExplicitHiddenTarget) + !( + isHiddenDir && + DIRS_TO_IGNORE.includes(".*") && + !context.isTargetDir && + !context.insideExplicitHiddenTarget + ) if (shouldRecurse) { // If we're entering a hidden directory that's the target, or we're already inside one, // mark that we're inside an explicitly targeted hidden directory - const newInsideExplicitHiddenTarget = insideExplicitHiddenTarget || (isHiddenDir && isTargetDir) - await scanDirectory(fullDirPath, false, newInsideExplicitHiddenTarget) + const newInsideExplicitHiddenTarget = + context.insideExplicitHiddenTarget || (isHiddenDir && context.isTargetDir) + const newContext: ScanContext = { + ...context, + isTargetDir: false, + insideExplicitHiddenTarget: newInsideExplicitHiddenTarget, + } + await scanDirectory(fullDirPath, newContext) } } } } catch (err) { - // Silently continue if we can't read a directory + // Continue if we can't read a directory console.warn(`Could not read directory ${currentPath}: ${err}`) } } // Start scanning from the root directory - // For environment details generation, we don't want to treat the root as a "target" - // if we're doing a general recursive scan, as this would include hidden directories - // Only treat as target if we're explicitly scanning a single hidden directory - const isExplicitHiddenTarget = path.basename(absolutePath).startsWith(".") - await scanDirectory(absolutePath, isExplicitHiddenTarget, isExplicitHiddenTarget) + await scanDirectory(absolutePath, initialContext) return directories } /** - * Determine if a directory should be included in results based on filters + * Critical directories that should always be ignored, even inside explicitly targeted hidden directories + */ +const CRITICAL_IGNORE_PATTERNS = new Set(["node_modules", ".git", "__pycache__", "venv", "env"]) + +/** + * Check if a directory matches any of the given patterns */ -function shouldIncludeDirectory( - dirName: string, +function matchesIgnorePattern(dirName: string, patterns: string[]): boolean { + for (const pattern of patterns) { + if (pattern === dirName || (pattern.includes("/") && pattern.split("/")[0] === dirName)) { + return true + } + } + return false +} + +/** + * Check if a directory is ignored by gitignore + */ +function isIgnoredByGitignore( fullDirPath: string, basePath: string, ignoreInstance: ReturnType, - isTargetDir: boolean = false, - insideExplicitHiddenTarget: boolean = false, ): boolean { - // If this is the explicitly targeted directory, allow it even if it's hidden - // This preserves the ability to explicitly target hidden directories like .roo-memory - if (isTargetDir) { - // Only apply non-hidden-directory ignore rules to target directories - const nonHiddenIgnorePatterns = DIRS_TO_IGNORE.filter((pattern) => pattern !== ".*") - for (const pattern of nonHiddenIgnorePatterns) { - if (pattern === dirName || (pattern.includes("/") && pattern.split("/")[0] === dirName)) { - return false - } - } - return true - } + const relativePath = path.relative(basePath, fullDirPath) + const normalizedPath = relativePath.replace(/\\/g, "/") + return ignoreInstance.ignores(normalizedPath) || ignoreInstance.ignores(normalizedPath + "/") +} - // If we're inside an explicitly targeted hidden directory, allow subdirectories - // even if they would normally be filtered out by the ".*" pattern or other ignore rules - if (insideExplicitHiddenTarget) { - // Only apply the most critical ignore patterns when inside explicit hidden target - // Allow temp, rules, etc. but still block node_modules, .git, etc. - const criticalIgnorePatterns = ["node_modules", ".git", "__pycache__", "venv", "env"] - for (const pattern of criticalIgnorePatterns) { - if (pattern === dirName || (pattern.includes("/") && pattern.split("/")[0] === dirName)) { - return false - } - } - // Check against gitignore patterns using the ignore library - const relativePath = path.relative(basePath, fullDirPath) - const normalizedPath = relativePath.replace(/\\/g, "/") - if (ignoreInstance.ignores(normalizedPath) || ignoreInstance.ignores(normalizedPath + "/")) { - return false - } - return true +/** + * Check if a target directory should be included + */ +function shouldIncludeTargetDirectory(dirName: string): boolean { + // Only apply non-hidden-directory ignore rules to target directories + const nonHiddenIgnorePatterns = DIRS_TO_IGNORE.filter((pattern) => pattern !== ".*") + return !matchesIgnorePattern(dirName, nonHiddenIgnorePatterns) +} + +/** + * Check if a directory inside an explicitly targeted hidden directory should be included + */ +function shouldIncludeInsideHiddenTarget(dirName: string, fullDirPath: string, context: ScanContext): boolean { + // Only apply the most critical ignore patterns when inside explicit hidden target + if (CRITICAL_IGNORE_PATTERNS.has(dirName)) { + return false } - // Check against explicit ignore patterns (excluding the ".*" pattern for now) + // Check against gitignore patterns + return !isIgnoredByGitignore(fullDirPath, context.basePath, context.ignoreInstance) +} + +/** + * Check if a regular directory should be included + */ +function shouldIncludeRegularDirectory(dirName: string, fullDirPath: string, context: ScanContext): boolean { + // Check against explicit ignore patterns (excluding the ".*" pattern) const nonHiddenIgnorePatterns = DIRS_TO_IGNORE.filter((pattern) => pattern !== ".*") - for (const pattern of nonHiddenIgnorePatterns) { - if (pattern === dirName || (pattern.includes("/") && pattern.split("/")[0] === dirName)) { - return false - } + if (matchesIgnorePattern(dirName, nonHiddenIgnorePatterns)) { + return false } - // Check against gitignore patterns using the ignore library - // Calculate relative path from the base directory - const relativePath = path.relative(basePath, fullDirPath) - const normalizedPath = relativePath.replace(/\\/g, "/") + // Check against gitignore patterns + return !isIgnoredByGitignore(fullDirPath, context.basePath, context.ignoreInstance) +} - // Check if the directory is ignored by .gitignore - if (ignoreInstance.ignores(normalizedPath) || ignoreInstance.ignores(normalizedPath + "/")) { - return false +/** + * Determine if a directory should be included in results based on filters + */ +function shouldIncludeDirectory(dirName: string, fullDirPath: string, context: ScanContext): boolean { + // If this is the explicitly targeted directory, allow it even if it's hidden + // This preserves the ability to explicitly target hidden directories like .roo-memory + if (context.isTargetDir) { + return shouldIncludeTargetDirectory(dirName) + } + + // If we're inside an explicitly targeted hidden directory, allow subdirectories + // even if they would normally be filtered out by the ".*" pattern or other ignore rules + if (context.insideExplicitHiddenTarget) { + return shouldIncludeInsideHiddenTarget(dirName, fullDirPath, context) } - return true + // Regular directory inclusion logic + return shouldIncludeRegularDirectory(dirName, fullDirPath, context) } /** @@ -619,9 +688,8 @@ async function execRipgrep(rgPath: string, args: string[], limit: number): Promi // Process each complete line for (const line of lines) { if (line.trim() && results.length < limit) { - // Convert relative path from ripgrep to absolute path - const absolutePath = path.resolve(searchDir, line) - results.push(absolutePath) + // Keep the relative path as returned by ripgrep + results.push(line) } else if (results.length >= limit) { break } From 8860be7ad4fc00414d02a20a7c96d0266eb728e0 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Wed, 16 Jul 2025 18:38:17 -0500 Subject: [PATCH 10/10] fix: update tests to handle cross-platform path resolution --- .../glob/__tests__/list-files.spec.ts | 193 ++++++++++++++++-- 1 file changed, 173 insertions(+), 20 deletions(-) diff --git a/src/services/glob/__tests__/list-files.spec.ts b/src/services/glob/__tests__/list-files.spec.ts index c1c84ac451..d855388002 100644 --- a/src/services/glob/__tests__/list-files.spec.ts +++ b/src/services/glob/__tests__/list-files.spec.ts @@ -95,8 +95,11 @@ describe("list-files symlink support", () => { mockSpawn.mockReturnValue(mockProcess as any) + // Use a test directory path + const testDir = "/test/dir" + // Call listFiles to trigger ripgrep execution - await listFiles("/test/dir", false, 100) + await listFiles(testDir, false, 100) // Verify that spawn was called with --follow flag (the critical fix) const [rgPath, args] = mockSpawn.mock.calls[0] @@ -105,9 +108,12 @@ describe("list-files symlink support", () => { expect(args).toContain("--hidden") expect(args).toContain("--follow") // This is the critical assertion - the fix should add this flag - // Platform-agnostic path check - verify the last argument is the resolved path - const expectedPath = path.resolve("/test/dir") - expect(args[args.length - 1]).toBe(expectedPath) + // Platform-agnostic path check - verify the last argument ends with the expected path + const lastArg = args[args.length - 1] + // On Windows, the path might be resolved to something like D:\test\dir + // On Unix, it would be /test/dir + // So we just check that it ends with the expected segments + expect(lastArg).toMatch(/[/\\]test[/\\]dir$/) }) it("should include --follow flag for recursive listings too", async () => { @@ -136,8 +142,11 @@ describe("list-files symlink support", () => { mockSpawn.mockReturnValue(mockProcess as any) + // Use a test directory path + const testDir = "/test/dir" + // Call listFiles with recursive=true - await listFiles("/test/dir", true, 100) + await listFiles(testDir, true, 100) // Verify that spawn was called with --follow flag (the critical fix) const [rgPath, args] = mockSpawn.mock.calls[0] @@ -146,9 +155,12 @@ describe("list-files symlink support", () => { expect(args).toContain("--hidden") expect(args).toContain("--follow") // This should be present in recursive mode too - // Platform-agnostic path check - verify the last argument is the resolved path - const expectedPath = path.resolve("/test/dir") - expect(args[args.length - 1]).toBe(expectedPath) + // Platform-agnostic path check - verify the last argument ends with the expected path + const lastArg = args[args.length - 1] + // On Windows, the path might be resolved to something like D:\test\dir + // On Unix, it would be /test/dir + // So we just check that it ends with the expected segments + expect(lastArg).toMatch(/[/\\]test[/\\]dir$/) }) it("should ensure first-level directories are included when limit is reached", async () => { @@ -171,18 +183,19 @@ describe("list-files symlink support", () => { on: vi.fn((event, callback) => { if (event === "data") { // Return many file paths to trigger the limit + // Note: ripgrep returns relative paths const paths = [ - "/test/dir/a_dir/", - "/test/dir/a_dir/subdir1/", - "/test/dir/a_dir/subdir1/file1.txt", - "/test/dir/a_dir/subdir1/file2.txt", - "/test/dir/a_dir/subdir2/", - "/test/dir/a_dir/subdir2/file3.txt", - "/test/dir/a_dir/file4.txt", - "/test/dir/a_dir/file5.txt", - "/test/dir/file1.txt", - "/test/dir/file2.txt", + "a_dir/", + "a_dir/subdir1/", + "a_dir/subdir1/file1.txt", + "a_dir/subdir1/file2.txt", + "a_dir/subdir2/", + "a_dir/subdir2/file3.txt", + "a_dir/file4.txt", + "a_dir/file5.txt", + "file1.txt", + "file2.txt", // Note: b_dir and c_dir are missing from ripgrep output ].join("\n") + "\n" setTimeout(() => callback(paths), 10) @@ -358,9 +371,10 @@ describe("hidden directory exclusion", () => { on: vi.fn((event, callback) => { if (event === "data") { // Simulate files that should be found in .roo/temp + // Note: ripgrep returns relative paths setTimeout(() => { - callback(".roo/temp/teste1.md\n") - callback(".roo/temp/22/test2.md\n") + callback("teste1.md\n") + callback("22/test2.md\n") }, 10) } }), @@ -406,3 +420,142 @@ describe("hidden directory exclusion", () => { expect(topLevelFile).toBeTruthy() }) }) + +describe("buildRecursiveArgs edge cases", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it("should correctly detect hidden directories with trailing slashes", async () => { + const mockSpawn = vi.mocked(childProcess.spawn) + const mockProcess = { + stdout: { + on: vi.fn((event, callback) => { + if (event === "data") { + setTimeout(() => callback("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) + + // Test with trailing slash on hidden directory + await listFiles("/test/.hidden/", true, 100) + + const [rgPath, args] = mockSpawn.mock.calls[0] + // When targeting a hidden directory, these flags should be present + expect(args).toContain("--no-ignore-vcs") + expect(args).toContain("--no-ignore") + expect(args).toContain("-g") + const gIndex = args.indexOf("-g") + expect(args[gIndex + 1]).toBe("*") + }) + + it("should correctly detect hidden directories with redundant separators", async () => { + const mockSpawn = vi.mocked(childProcess.spawn) + const mockProcess = { + stdout: { + on: vi.fn((event, callback) => { + if (event === "data") { + setTimeout(() => callback("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) + + // Test with redundant separators before hidden directory + await listFiles("/test//.hidden", true, 100) + + const [rgPath, args] = mockSpawn.mock.calls[0] + // When targeting a hidden directory, these flags should be present + expect(args).toContain("--no-ignore-vcs") + expect(args).toContain("--no-ignore") + expect(args).toContain("-g") + const gIndex = args.indexOf("-g") + expect(args[gIndex + 1]).toBe("*") + }) + + it("should correctly detect nested hidden directories with mixed separators", async () => { + const mockSpawn = vi.mocked(childProcess.spawn) + const mockProcess = { + stdout: { + on: vi.fn((event, callback) => { + if (event === "data") { + setTimeout(() => callback("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) + + // Test with complex path including hidden directory + await listFiles("/test//normal/.hidden//subdir/", true, 100) + + const [rgPath, args] = mockSpawn.mock.calls[0] + // When targeting a path containing a hidden directory, these flags should be present + expect(args).toContain("--no-ignore-vcs") + expect(args).toContain("--no-ignore") + expect(args).toContain("-g") + const gIndex = args.indexOf("-g") + expect(args[gIndex + 1]).toBe("*") + }) + + it("should not detect hidden directories when path only has dots in filenames", async () => { + const mockSpawn = vi.mocked(childProcess.spawn) + const mockProcess = { + stdout: { + on: vi.fn((event, callback) => { + if (event === "data") { + setTimeout(() => callback("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) + + // Test with a path that has dots but no hidden directories + await listFiles("/test/file.with.dots/normal", true, 100) + + const [rgPath, args] = mockSpawn.mock.calls[0] + // Should NOT have the special flags for hidden directories + expect(args).not.toContain("--no-ignore-vcs") + expect(args).not.toContain("--no-ignore") + }) +})