feat: add recursive subdirectory scanning for templates #103
Conversation
WalkthroughReplaces per-directory template loading with a recursive scanner using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/template/template_test.go (1)
121-140: Consider adding a test for symlink handling.The implementation skips symlinks for security, but there's no explicit test for this behavior. While creating symlinks in tests can be platform-dependent, it would strengthen the test coverage for this security feature.
pkg/template/template.go (1)
139-142: Symlink check is unreachable dead code.The check at line 135 (
!entry.IsDir()) filters out all symlinks before reaching the symlink check on line 139. Sinceos.ReadDirusesLstatinternally,DirEntry.IsDir()returnsfalsefor symlinks (including symlinks to directories), so the debug log message never executes. While symlinks are still correctly skipped, this check is redundant. Remove lines 139-142 or move the symlink check before theIsDir()filter if the debug log is important for observability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/template/template.gopkg/template/template_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/template/template_test.go (1)
pkg/template/template.go (1)
Template(17-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
pkg/template/template.go (4)
95-97: LGTM!The depth limit constant is appropriately defined and documented. A value of 10 provides reasonable flexibility for nested directory structures while preventing resource exhaustion from excessively deep hierarchies.
99-110: LGTM!The refactored function cleanly delegates to the new scanner and appropriately wraps errors with category context for better debugging.
112-118: LGTM!The helper correctly identifies template directories by checking for
index.yaml. Treating anyos.Staterror as "not a template directory" is reasonable defensive behavior for this use case.
146-162: LGTM!Solid implementation with proper validation:
- Template ID must match directory name (prevents misconfigurations)
- Duplicate ID detection prevents silent overwrites
- Informative error messages aid debugging
pkg/template/template_test.go (8)
36-63: LGTM!Well-designed test helper that creates realistic template structures. The YAML content includes all necessary fields for valid templates, and proper error handling with
t.Helper()ensures clean test output.
65-84: LGTM!Comprehensive test covering all three expected cases: directory without
index.yaml, directory withindex.yaml, and non-existent directory.
86-119: LGTM!Well-structured test with clear documentation of the expected directory hierarchy. Validates that recursive scanning correctly discovers templates at multiple nesting levels.
121-140: LGTM!Effectively tests that hidden directories (prefixed with
.) are properly skipped during scanning.
142-158: LGTM!Correctly tests the max depth enforcement by creating a directory structure that exceeds
maxScanDepthand verifying the expected error is returned.
160-176: LGTM!Properly tests duplicate template ID detection across different subdirectories.
178-202: LGTM!Good integration test verifying that
loadTemplatesFromCategorycorrectly uses the new scanner to find both direct and nested templates.
204-241: LGTM!Comprehensive end-to-end test that validates the full template loading pipeline with nested categories and subdirectories, mirroring realistic usage patterns.
|
What is the reason using recursion instead of https://pkg.go.dev/path/filepath#WalkDir |
|
No real advantage, updating. |
Implement recursive scanning of template directories using filepath.WalkDir to support nested organizational structures (e.g., templates organized by difficulty level or subcategory). Changes: - Use filepath.WalkDir for idiomatic directory traversal - Add isTemplateDirectory() helper to detect template directories - Skip symlinks to prevent path traversal attacks - Add max depth limit (10) to prevent excessive resource usage - Detect and error on duplicate template IDs - Add comprehensive tests for all new functionality Closes #102
dogancanbakir
force-pushed
the
feat/recursive-template-scanning
branch
from
0b024f6 to
e4f0028
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @pkg/template/template.go:
- Around line 114-125: The symlink branch never runs because d.IsDir() is
checked before symlink detection; move the explicit symlink handling (the
d.Type()&os.ModeSymlink check and the log.Debug().Msgf("skipping symlink: %s",
d.Name()) / return filepath.SkipDir) above the d.IsDir() check so symlinks are
detected, logged and skipped explicitly; ensure you still return
filepath.SkipDir for symlinks and retain the existing hidden-file/directory
check.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/template/template.gopkg/template/template_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/template/template_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/template/template.go (1)
pkg/template/parser.go (1)
LoadTemplate(12-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
pkg/template/template.go (3)
95-97: LGTM!A depth limit of 10 is reasonable for nested template structures. This safeguard prevents excessive resource consumption during directory traversal.
127-149: LGTM!The depth calculation, template ID validation against directory names, and duplicate detection logic are all correctly implemented. Returning
filepath.SkipDirafter loading a template properly prevents descending into template directories.
161-167: LGTM!Simple and correct implementation. Using
os.Statappropriately handles the case whereindex.yamlmight be a symlink to the actual file.
Move symlink detection before the d.IsDir() check so symlinks are properly detected and skipped. Previously, symlinks were filtered out by the !d.IsDir() check before the symlink handling could run.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @pkg/template/template.go:
- Around line 121-124: The symlink branch in the WalkDir handler (the
d.Type()&os.ModeSymlink check) currently returns filepath.SkipDir which causes
remaining siblings in the same directory to be skipped; change the handler so
symlinks are logged/skipped but the function returns nil instead of
filepath.SkipDir (i.e., remove the SkipDir return for symlinks) so WalkDir
continues processing sibling entries.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/template/template.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/template/template.go (1)
pkg/template/parser.go (1)
LoadTemplate(12-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
pkg/template/template.go (3)
95-97: LGTM! Appropriate depth limit.The depth limit of 10 is reasonable for nested template organization while preventing resource exhaustion and infinite loops from circular symlinks.
130-152: LGTM! Robust depth enforcement and template validation.The implementation correctly:
- Calculates directory depth relative to the category root
- Enforces the maximum depth limit with a clear error message
- Validates that template IDs match their directory names (preventing mismatched metadata)
- Detects duplicate template IDs across the hierarchy (preventing ID collisions)
- Returns
SkipDirafter loading a template to avoid scanning inside template directories- Provides comprehensive error context for all failure modes
157-159: LGTM! Clean helper and error handling.The
isTemplateDirectoryhelper provides a clear, single-purpose check for template directories by detectingindex.yaml. The error wrapping at line 158 adds valuable context about which category failed to scan.Also applies to: 164-170
| if d.Type()&os.ModeSymlink != 0 { | ||
| log.Debug().Msgf("skipping symlink: %s", d.Name()) | ||
| return filepath.SkipDir | ||
| } |
There was a problem hiding this comment.
Fix symlink handling to avoid skipping sibling entries.
According to filepath.WalkDir documentation, returning filepath.SkipDir for a non-directory entry skips all remaining files in the containing directory. For a symlink to a file (where d.IsDir() returns false), this causes sibling templates to be incorrectly skipped.
For example, if a directory contains:
template1/
symlink-to-file
template2/
When the symlink is encountered, template2/ would never be processed.
🔎 Proposed fix
if d.Type()&os.ModeSymlink != 0 {
log.Debug().Msgf("skipping symlink: %s", d.Name())
- return filepath.SkipDir
+ if d.IsDir() {
+ return filepath.SkipDir
+ }
+ return nil
}Alternatively, always return nil for symlinks since WalkDir won't descend into symlinked directories anyway:
if d.Type()&os.ModeSymlink != 0 {
log.Debug().Msgf("skipping symlink: %s", d.Name())
- return filepath.SkipDir
+ return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if d.Type()&os.ModeSymlink != 0 { | |
| log.Debug().Msgf("skipping symlink: %s", d.Name()) | |
| return filepath.SkipDir | |
| } | |
| if d.Type()&os.ModeSymlink != 0 { | |
| log.Debug().Msgf("skipping symlink: %s", d.Name()) | |
| if d.IsDir() { | |
| return filepath.SkipDir | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In @pkg/template/template.go around lines 121 - 124, The symlink branch in the
WalkDir handler (the d.Type()&os.ModeSymlink check) currently returns
filepath.SkipDir which causes remaining siblings in the same directory to be
skipped; change the handler so symlinks are logged/skipped but the function
returns nil instead of filepath.SkipDir (i.e., remove the SkipDir return for
symlinks) so WalkDir continues processing sibling entries.
Summary
This change supports the template organization needed for vt-templates PR #6 which adds 104 xbow security benchmarks.
Changes
scanDirectoryForTemplates()- Recursive directory traversal that detects templates by presence ofindex.yamlisTemplateDirectory()- Helper function to check if a directory is a templateTest plan
Closes #102
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.