fix(manifest): use JSON file pattern during discovery - #305
Conversation
📝 WalkthroughWalkthroughManifest discovery now safely enumerates nested JSON files, skips inaccessible filesystem entries and malformed manifests, honors cancellation, and uses injected filesystem delegates. Tests add isolated temporary-directory coverage for nested discovery, filtering, and malformed JSON. ChangesManifest discovery resilience
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant DiscoverManifestsAsync
participant EnumerateFilesSafely
participant FileSystem
participant Logger
DiscoverManifestsAsync->>EnumerateFilesSafely: enumerate JSON manifests
EnumerateFilesSafely->>FileSystem: enumerate files and directories
FileSystem-->>EnumerateFilesSafely: paths or enumeration failures
EnumerateFilesSafely->>Logger: log skipped failures
EnumerateFilesSafely-->>DiscoverManifestsAsync: matching manifest paths
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ 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: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous CRITICAL Issue: The previous review identified: "Recursive enumeration aborts discovery - when a configured content directory contains an inaccessible or concurrently removed descendant, recursive enumeration throws outside the per-file exception handler." Resolution: The issue was previously addressed in commit aff90d0. The incremental changes in this review (commit 1079387) focus on:
The core JSON filename-pattern fix remains validated, and unavailable-directory resilience is tracked separately in #308 and PR #309 as stated in the PR description. Previous Review Summaries (2 snapshots, latest commit aff90d0)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit aff90d0)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous CRITICAL Issue: The previous review identified: "Recursive enumeration aborts discovery - when a configured content directory contains an inaccessible or concurrently removed descendant, recursive enumeration throws outside the per-file exception handler." Resolution: The incremental changes in commit aff90d0 fix(manifest): continue past unavailable directories successfully address this issue by:
The added test Previous review (commit 0ddbd1d)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (2 files)
Reviewed by glm-4.7 · Input: 35.4K · Output: 2.2K · Cached: 149.2K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 273-277: Update Dispose to tolerate an already-removed
_tempDirectory and transient Directory.Delete failures by handling
DirectoryNotFoundException, IOException, and UnauthorizedAccessException without
propagating them to xUnit; remove the redundant GC.SuppressFinalize call while
keeping synchronous IDisposable cleanup.
In `@GenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs`:
- Around line 272-282: Update DiscoverFileSystemManifestsAsync to call the
existing LoadManifestAsync helper for each discovered manifest file instead of
duplicating file loading and deserialization logic. Preserve the current
scanning, cancellation, and manifest-processing behavior while reusing
LoadManifestAsync consistently with DiscoverManifestsAsync.
🪄 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: f138ae9f-28c2-4424-a9f1-146de7a832d5
📒 Files selected for processing (2)
GenHub/GenHub.Tests/GenHub.Tests.Core/Features/Manifest/ManifestDiscoveryServiceTests.csGenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs
aff90d0 to
0ddbd1d
Compare
|
@coderabbitai review Scope update: unavailable-directory resilience was split into #308 and PR #309. PR #305 now contains only the JSON filename-pattern fix and focused filesystem tests. The |
|
✅ Action performedReview finished.
|
1 participant
Summary
Fix manifest discovery to use the configured JSON filename pattern instead of a literal identifier, restoring recursive filesystem discovery.
Add coverage for nested manifests, non-JSON files, and malformed JSON.
Testing
Follow-up
Unavailable-directory resilience is tracked in #308 and implemented separately in #309.
Closes #302
Greptile Summary
Corrects filesystem manifest discovery and adds focused coverage.
FileTypes.JsonFilePattern.Confidence Score: 3/5
The manifest-pattern correction works as intended, but the outstanding recursive-enumeration failure should be fixed before merging.
Recursive enumeration remains outside the exception handler, so an inaccessible or concurrently removed descendant can still abort discovery instead of preserving manifests from accessible paths.
Files Needing Attention: GenHub/GenHub/Features/Manifest/ManifestDiscoveryService.cs
Important Files Changed
Reviews (4): Last reviewed commit: "test(manifest): harden temporary directo..." | Re-trigger Greptile