fix(manifest): continue past unavailable directories - #309
Conversation
📝 WalkthroughWalkthroughManifest discovery now uses injectable filesystem enumerators and iterative safe traversal. Unauthorized or unavailable directories are logged and skipped while accessible manifests continue to be discovered. A deterministic test covers inaccessible and removed sibling directories. ChangesManifest discovery resilience
Estimated code review effort: 4 (Complex) | ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (1 file)
Note: The implementation correctly addresses the PR objectives of preventing recursive manifest discovery from aborting when directories become inaccessible. The exception handling, cancellation support, and stack-based traversal are well implemented. The production code uses Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit 2740efd)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 2740efd)Status: 1 Issue Found | Recommendation: Merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (1 file)
Note: The implementation correctly addresses the PR objectives of preventing recursive manifest discovery from aborting when directories become inaccessible. The exception handling, cancellation support, and stack-based traversal are well implemented. The production code uses Fix these issues in Kilo Cloud Previous review (commit cb8c001)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Note: The implementation correctly addresses the PR objectives of preventing recursive manifest discovery from aborting when directories become inaccessible. The exception handling, cancellation support, and stack-based traversal are well implemented. The previous issue about centralizing the directory wildcard pattern has been resolved by using the parameterless overload of Previous review (commit aff90d0)Status: 1 Existing Issue Found | Recommendation: Address existing review comments before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Note: The implementation correctly addresses the PR objectives of preventing recursive manifest discovery from aborting when directories become inaccessible. The exception handling, cancellation support, and stack-based traversal are well implemented. The only concern is the existing suggestion to centralize the directory wildcard pattern for consistency with the project's coding standards. Reviewed by glm-4.7 · Input: 36.5K · Output: 7K · Cached: 453.8K |
aff90d0 to
cb8c001
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai review The stacked branch was rebased onto the latest #305, and the explicit directory wildcard was removed by using |
|
✅ Action performedReview finished.
|
The base branch was changed.
cb8c001 to
2740efd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Manifest/ManifestDiscoveryServiceTests.cs`:
- Around line 146-196: Extend
DiscoverManifestsAsync_WithUnavailableDescendants_ContinuesDiscoveringAccessibleManifests
to make the injected directory enumerator throw an expected
UnauthorizedAccessException or IOException for one unavailable subdirectory,
while continuing to enumerate accessible siblings. Ensure the assertions still
verify the accessible manifest is discovered, thereby covering the
enumerateDirectories exception branch in EnumerateFilesSafely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 07a417a6-89c1-43b2-824f-058918dd514b
📒 Files selected for processing (2)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Manifest/ManifestDiscoveryServiceTests.csGenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| _loggerMock.Object, | ||
| _cacheMock.Object, | ||
| EnumerateFiles, | ||
| directory => Directory.EnumerateDirectories( |
There was a problem hiding this comment.
SUGGESTION: Test delegate should match production implementation
The test uses the old pattern with hardcoded "*" and SearchOption.TopDirectoryOnly, but the production code was updated to use the parameterless overload Directory.EnumerateDirectories(directory) to avoid hardcoding the directory wildcard pattern. For consistency and accuracy, the test should match the production implementation.
| directory => Directory.EnumerateDirectories( | |
| directory => Directory.EnumerateDirectories(directory)); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
@coderabbitai The coverage gap was real — the test injected failures only into the file enumerator, while the directory enumerator was the real Fixed in ace7451. The test now injects an Verified the test actually pins the branch: with the |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
| throw new UnauthorizedAccessException("Injected unlistable directory."); | ||
| } | ||
|
|
||
| return Directory.EnumerateDirectories(directory, "*", SearchOption.TopDirectoryOnly); |
There was a problem hiding this comment.
SUGGESTION: Test delegate should match production implementation
The test uses the old pattern with hardcoded "*" and SearchOption.TopDirectoryOnly, but the production code uses the parameterless overload Directory.EnumerateDirectories(directory) to avoid hardcoding the directory wildcard pattern. For consistency and accuracy, the test should match the production implementation.
| return Directory.EnumerateDirectories(directory, "*", SearchOption.TopDirectoryOnly); | |
| return Directory.EnumerateDirectories(directory); |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
Summary
Prevent recursive manifest discovery from aborting when a nested directory is inaccessible or removed during enumeration.
Traverse directories individually, preserve cancellation, log skipped paths, and continue discovering manifests in accessible siblings.
Add deterministic coverage for inaccessible and concurrently removed directories.
Testing
Dependency
Stacked on #305. Retarget to
developmentafter #305 merges.Closes #308
Greptile Summary
Updates manifest discovery to continue scanning accessible sibling directories when nested paths become unavailable.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Push manifest root] --> B{Pending directories?} B -->|No| Z[Discovery complete] B -->|Yes| C[Pop next directory] C --> D[Check cancellation] D --> E[Enumerate matching files] E -->|Unavailable| F[Log warning and skip files] E -->|Success| G[Process discovered manifests] F --> H[Enumerate child directories] G --> H H -->|Unavailable| I[Log warning and skip descendants] H -->|Success| J[Push child directories] I --> B J --> BReviews (4): Last reviewed commit: "test(manifest): cover directory enumerat..." | Re-trigger Greptile