dev#204
Conversation
…late feat: add playbook support with run, stop, list, and inspect commands
WalkthroughThis PR introduces playbooks as a composable feature for orchestrating sequences of templates. It adds Playbook types with validation, implements repository-level loading for both templates and playbooks, wires them into the application dependency graph, and provides CLI commands to run, stop, and list playbooks alongside supporting documentation updates. ChangesPlaybook Feature Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
pkg/template/loader_test.go (1)
113-171: ⚡ Quick winAdd regressions for cross-category duplicates and symlink-file traversal.
Current tests don’t cover duplicate template IDs across different top-level categories, or the symlink-file case that can affect directory traversal. Adding both will protect loader behavior from regressions.
🤖 Prompt for 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. In `@pkg/template/loader_test.go` around lines 113 - 171, Add two new unit tests to pkg/template/loader_test.go to prevent regressions: one that creates templates with the same ID in two different top-level categories and asserts LoadRepo returns an error or deduplicates as expected (reference LoadRepo and createTestTemplate to set up categories and templates), and another that creates a symlink-to-file inside a category (or a symlinked directory) and verifies LoadRepo does not follow the symlink into unintended files/directories (reference createTestTemplate, playbooksDirName if needed, and LoadRepo); ensure the tests assert the correct template count and presence/absence of duplicate or symlinked entries to lock in desired behavior.
🤖 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 `@internal/cli/inspect.go`:
- Line 34: The flag description for the "id" flag (registered via
cmd.Flags().String("id", ... ) in internal/cli/inspect.go) is outdated — update
the help text to reflect that the command accepts both template and playbook IDs
(e.g., change "Specify a template ID for targeted vulnerable environment" to
something like "Specify a template or playbook ID for targeted vulnerable
environment") so users see the correct supported inputs.
In `@internal/cli/playbook.go`:
- Around line 77-81: The code currently logs partial failures for playbook
execution but still returns success; update the failure handling in the playbook
command so any non-empty failed slice causes a non-zero exit or returned error:
replace the log.Warn branch in the run/stop handlers (the blocks referencing
failed and pb.Info.Name) to log the error and then return an error (e.g.,
fmt.Errorf("playbook finished with errors: %v", failed) ) or call os.Exit(1)
depending on the function signature; apply the same change to both occurrences
(the blocks around failed, pb.Info.Name and the second block reported in the
comment) so CI/scripts see a non-zero exit on partial failures.
In `@pkg/template/loader.go`:
- Around line 58-60: The merge loop silently overwrites template IDs when
multiple categories contain the same id; update the loop that iterates over
categoryTemplates and writes into templates to first check if the id already
exists in templates and return (or propagate) an error instead of overwriting
it. Concretely, in the function containing the maps categoryTemplates and
templates, replace the blind assignment templates[id] = tmpl with a test like if
_, exists := templates[id]; exists { return fmt.Errorf("duplicate template id %q
found in category %q", id, categoryName) } and only assign when not present,
ensuring the duplicate detection references the categoryTemplates/ templates
variables and returns a clear error.
- Around line 120-127: The code currently allows absolute paths and relative
paths that can escape templateDir; update the loader logic that handles
providerConfig.Path (the composePath, templateDir check) to reject absolute
providerConfig.Path and to resolve the joined path using filepath.Clean and
filepath.Abs (or filepath.Rel) and then verify the resolved composePath is
inside templateDir (e.g. by computing rel, ensuring it does not start with ".."
and is not equal to ".." segments); if the path escapes or is absolute return a
descriptive error instead of returning the external path. Ensure you still stat
the validated composePath before returning.
- Around line 155-158: The symlink handling returns filepath.SkipDir
unconditionally when d.Type()&os.ModeSymlink != 0, which incorrectly skips
sibling entries; change the check so you only return filepath.SkipDir for
directory symlinks by combining the symlink test with d.IsDir() (i.e., if
d.Type()&os.ModeSymlink != 0 && d.IsDir() { return filepath.SkipDir }),
otherwise continue normally; make the identical change to the other
symlink-handling block that also uses d.Type()&os.ModeSymlink and
filepath.SkipDir.
In `@pkg/template/playbook.go`:
- Around line 129-143: The validation currently accepts whitespace-only
names/IDs; update the checks in the playbook validation (where pb.Info.Name and
loop over pb.Templates are validated) to trim whitespace (e.g.,
strings.TrimSpace) and treat empty-after-trim as invalid: reject pb.Info.Name if
strings.TrimSpace(pb.Info.Name) == "" and reject any template id if
strings.TrimSpace(id) == ""; continue using the same error messages but
reference the trimmed values when checking duplicates and storing into seen to
avoid treating " a " and "a" as different.
---
Nitpick comments:
In `@pkg/template/loader_test.go`:
- Around line 113-171: Add two new unit tests to pkg/template/loader_test.go to
prevent regressions: one that creates templates with the same ID in two
different top-level categories and asserts LoadRepo returns an error or
deduplicates as expected (reference LoadRepo and createTestTemplate to set up
categories and templates), and another that creates a symlink-to-file inside a
category (or a symlinked directory) and verifies LoadRepo does not follow the
symlink into unintended files/directories (reference createTestTemplate,
playbooksDirName if needed, and LoadRepo); ensure the tests assert the correct
template count and presence/absence of duplicate or symlinked entries to lock in
desired behavior.
🪄 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: CHILL
Plan: Pro
Run ID: d45d9495-3ca9-473a-b828-25237b3ec7d0
📒 Files selected for processing (15)
README.mdcmd/vt/main.gointernal/app/app.gointernal/cli/cli.gointernal/cli/inspect.gointernal/cli/playbook.gointernal/cli/template.gopkg/template/loader.gopkg/template/loader_test.gopkg/template/parser.gopkg/template/parser_test.gopkg/template/playbook.gopkg/template/playbook_test.gopkg/template/template.gopkg/template/template_test.go
| log.Fatal().Msgf("'%s' not found as a template or playbook", templateID) | ||
| }, | ||
| } | ||
| cmd.Flags().String("id", "", "Specify a template ID for targeted vulnerable environment") |
There was a problem hiding this comment.
Update --id help text to match new playbook support.
The flag description still says template-only, but the command now accepts both template and playbook IDs.
Proposed fix
- cmd.Flags().String("id", "", "Specify a template ID for targeted vulnerable environment")
+ cmd.Flags().String("id", "", "Specify a template or playbook ID for targeted vulnerable environment")📝 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.
| cmd.Flags().String("id", "", "Specify a template ID for targeted vulnerable environment") | |
| cmd.Flags().String("id", "", "Specify a template or playbook ID for targeted vulnerable environment") |
🤖 Prompt for 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.
In `@internal/cli/inspect.go` at line 34, The flag description for the "id" flag
(registered via cmd.Flags().String("id", ... ) in internal/cli/inspect.go) is
outdated — update the help text to reflect that the command accepts both
template and playbook IDs (e.g., change "Specify a template ID for targeted
vulnerable environment" to something like "Specify a template or playbook ID for
targeted vulnerable environment") so users see the correct supported inputs.
| if len(failed) > 0 { | ||
| log.Warn().Msgf("playbook finished with errors — failed templates: %s", strings.Join(failed, ", ")) | ||
| } else { | ||
| log.Info().Msgf("playbook '%s' completed successfully", pb.Info.Name) | ||
| } |
There was a problem hiding this comment.
Return non-zero exit when any playbook item fails.
Both run and stop currently report partial failures with warnings but still exit successfully. That masks operational failure in scripts and CI.
Proposed fix
if len(failed) > 0 {
- log.Warn().Msgf("playbook finished with errors — failed templates: %s", strings.Join(failed, ", "))
+ log.Fatal().Msgf("playbook finished with errors — failed templates: %s", strings.Join(failed, ", "))
} else {
log.Info().Msgf("playbook '%s' completed successfully", pb.Info.Name)
} if len(failed) > 0 {
- log.Warn().Msgf("playbook stop finished with errors — failed templates: %s", strings.Join(failed, ", "))
+ log.Fatal().Msgf("playbook stop finished with errors — failed templates: %s", strings.Join(failed, ", "))
} else {
log.Info().Msgf("playbook '%s' stopped successfully", pb.Info.Name)
}Also applies to: 140-144
🤖 Prompt for 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.
In `@internal/cli/playbook.go` around lines 77 - 81, The code currently logs
partial failures for playbook execution but still returns success; update the
failure handling in the playbook command so any non-empty failed slice causes a
non-zero exit or returned error: replace the log.Warn branch in the run/stop
handlers (the blocks referencing failed and pb.Info.Name) to log the error and
then return an error (e.g., fmt.Errorf("playbook finished with errors: %v",
failed) ) or call os.Exit(1) depending on the function signature; apply the same
change to both occurrences (the blocks around failed, pb.Info.Name and the
second block reported in the comment) so CI/scripts see a non-zero exit on
partial failures.
| for id, tmpl := range categoryTemplates { | ||
| templates[id] = tmpl | ||
| } |
There was a problem hiding this comment.
Prevent silent template ID overwrite across categories.
The current merge lets later categories overwrite an already-loaded template ID without error, which can make template resolution nondeterministic.
Proposed fix
for id, tmpl := range categoryTemplates {
+ if _, exists := templates[id]; exists {
+ return nil, nil, fmt.Errorf("duplicate template id '%s' found across categories", id)
+ }
templates[id] = tmpl
}
}📝 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.
| for id, tmpl := range categoryTemplates { | |
| templates[id] = tmpl | |
| } | |
| for id, tmpl := range categoryTemplates { | |
| if _, exists := templates[id]; exists { | |
| return nil, nil, fmt.Errorf("duplicate template id '%s' found across categories", id) | |
| } | |
| templates[id] = tmpl | |
| } |
🤖 Prompt for 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.
In `@pkg/template/loader.go` around lines 58 - 60, The merge loop silently
overwrites template IDs when multiple categories contain the same id; update the
loop that iterates over categoryTemplates and writes into templates to first
check if the id already exists in templates and return (or propagate) an error
instead of overwriting it. Concretely, in the function containing the maps
categoryTemplates and templates, replace the blind assignment templates[id] =
tmpl with a test like if _, exists := templates[id]; exists { return
fmt.Errorf("duplicate template id %q found in category %q", id, categoryName) }
and only assign when not present, ensuring the duplicate detection references
the categoryTemplates/ templates variables and returns a clear error.
| if filepath.IsAbs(providerConfig.Path) { | ||
| return providerConfig.Path, filepath.Dir(providerConfig.Path), nil | ||
| } | ||
|
|
||
| composePath = filepath.Join(templateDir, providerConfig.Path) | ||
| if _, statErr := os.Stat(composePath); statErr != nil { | ||
| return "", "", fmt.Errorf("template %q has invalid docker-compose path %q: %w", templateID, composePath, statErr) | ||
| } |
There was a problem hiding this comment.
Block docker-compose.path from escaping template directory.
Relative paths like ../../... can resolve outside templateDir, and absolute paths bypass repository boundaries entirely. This allows template metadata to point at arbitrary filesystem locations.
Proposed fix
- if filepath.IsAbs(providerConfig.Path) {
- return providerConfig.Path, filepath.Dir(providerConfig.Path), nil
- }
-
- composePath = filepath.Join(templateDir, providerConfig.Path)
+ if filepath.IsAbs(providerConfig.Path) {
+ return "", "", fmt.Errorf("template %q docker-compose.path must be relative", templateID)
+ }
+
+ composePath = filepath.Clean(filepath.Join(templateDir, providerConfig.Path))
+ rel, relErr := filepath.Rel(templateDir, composePath)
+ if relErr != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
+ return "", "", fmt.Errorf("template %q docker-compose.path escapes template directory", templateID)
+ }
if _, statErr := os.Stat(composePath); statErr != nil {
return "", "", fmt.Errorf("template %q has invalid docker-compose path %q: %w", templateID, composePath, statErr)
}📝 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 filepath.IsAbs(providerConfig.Path) { | |
| return providerConfig.Path, filepath.Dir(providerConfig.Path), nil | |
| } | |
| composePath = filepath.Join(templateDir, providerConfig.Path) | |
| if _, statErr := os.Stat(composePath); statErr != nil { | |
| return "", "", fmt.Errorf("template %q has invalid docker-compose path %q: %w", templateID, composePath, statErr) | |
| } | |
| if filepath.IsAbs(providerConfig.Path) { | |
| return "", "", fmt.Errorf("template %q docker-compose.path must be relative", templateID) | |
| } | |
| composePath = filepath.Clean(filepath.Join(templateDir, providerConfig.Path)) | |
| rel, relErr := filepath.Rel(templateDir, composePath) | |
| if relErr != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { | |
| return "", "", fmt.Errorf("template %q docker-compose.path escapes template directory", templateID) | |
| } | |
| if _, statErr := os.Stat(composePath); statErr != nil { | |
| return "", "", fmt.Errorf("template %q has invalid docker-compose path %q: %w", templateID, composePath, statErr) | |
| } |
🤖 Prompt for 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.
In `@pkg/template/loader.go` around lines 120 - 127, The code currently allows
absolute paths and relative paths that can escape templateDir; update the loader
logic that handles providerConfig.Path (the composePath, templateDir check) to
reject absolute providerConfig.Path and to resolve the joined path using
filepath.Clean and filepath.Abs (or filepath.Rel) and then verify the resolved
composePath is inside templateDir (e.g. by computing rel, ensuring it does not
start with ".." and is not equal to ".." segments); if the path escapes or is
absolute return a descriptive error instead of returning the external path.
Ensure you still stat the validated composePath before returning.
| if d.Type()&os.ModeSymlink != 0 { | ||
| log.Debug().Msgf("skipping symlink: %s", d.Name()) | ||
| return filepath.SkipDir | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify both symlink branches currently return SkipDir unconditionally.
rg -n -C2 'd\.Type\(\)&os\.ModeSymlink != 0' pkg/template/loader.goRepository: HappyHackingSpace/vt
Length of output: 301
🏁 Script executed:
# Get the full context of the functions containing lines 155-157 and 222-224
head -n 160 pkg/template/loader.go | tail -n 20Repository: HappyHackingSpace/vt
Length of output: 380
🏁 Script executed:
# Also check the function signature and surrounding context
sed -n '140,170p' pkg/template/loader.goRepository: HappyHackingSpace/vt
Length of output: 718
🏁 Script executed:
# Check the second occurrence around line 222
sed -n '210,235p' pkg/template/loader.goRepository: HappyHackingSpace/vt
Length of output: 599
🏁 Script executed:
# Find the function names to understand context
rg -B 15 'd\.Type\(\)&os\.ModeSymlink' pkg/template/loader.go | head -n 50Repository: HappyHackingSpace/vt
Length of output: 704
🌐 Web query:
Go filepath.SkipDir symlink files behavior WalkDir documentation
💡 Result:
In Go, the filepath.WalkDir function is documented as not following symbolic links [1][2][3]. This means that when the walker encounters a symbolic link, it reports the link itself as a file or directory entry rather than traversing into the target of the link [1][2]. There is one notable exception: if the root path provided to WalkDir is itself a symbolic link, the function will follow that link to its target and begin the walk from there [1]. Regarding the use of filepath.SkipDir: 1. When returned by the WalkDirFunc for a directory, SkipDir instructs the walker to skip the contents of that directory entirely [1]. 2. When returned for a non-directory file, SkipDir skips the remaining files in the current directory [4]. It is important to note that while WalkDir does not follow symlinks, it is still susceptible to time-of-check/time-of-use (TOCTOU) race conditions [5]. If a portion of the path being walked is replaced with a symbolic link while the walk is in progress, the behavior may be unpredictable [5]. For security-sensitive applications requiring protection against symlink traversal, developers are advised to use traversal-resistant APIs (such as those that operate on file descriptors) rather than relying on path-based walking functions [5]. Additionally, historical issues have been reported regarding platform-specific behavior, such as on Windows, where directory symlinks or junctions might sometimes be traversed [6][7]. However, the standard library documentation maintains that the function does not follow symbolic links [2][3].
Citations:
- 1: https://go.dev/src/io/fs/walk.go
- 2: https://pkg.go.dev/path/filepath@go1.22.4
- 3: https://pkg.go.dev/path/filepath
- 4: https://go.dev/issue/16280
- 5: path/filepath: Walk/WalkDir susceptible to symlink race golang/go#70007
- 6: path/filepath: Walk recurses on directory symlinks on Windows golang/go#17540
- 7: https://go.dev/issue/17540
Only return SkipDir for directory symlinks.
Returning filepath.SkipDir for non-directory symlink entries skips remaining siblings in the current directory and causes templates to be missed during scans. The dotfile handling in this function already demonstrates the correct pattern: check d.IsDir() before returning SkipDir.
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
} if d.Type()&os.ModeSymlink != 0 {
- return filepath.SkipDir
+ if d.IsDir() {
+ return filepath.SkipDir
+ }
+ return nil
}Also applies to: 222-224
🤖 Prompt for 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.
In `@pkg/template/loader.go` around lines 155 - 158, The symlink handling returns
filepath.SkipDir unconditionally when d.Type()&os.ModeSymlink != 0, which
incorrectly skips sibling entries; change the check so you only return
filepath.SkipDir for directory symlinks by combining the symlink test with
d.IsDir() (i.e., if d.Type()&os.ModeSymlink != 0 && d.IsDir() { return
filepath.SkipDir }), otherwise continue normally; make the identical change to
the other symlink-handling block that also uses d.Type()&os.ModeSymlink and
filepath.SkipDir.
| if pb.Info.Name == "" { | ||
| return fmt.Errorf("playbook '%s': info.name cannot be empty", pb.ID) | ||
| } | ||
| if len(pb.Templates) == 0 { | ||
| return fmt.Errorf("playbook '%s': templates list cannot be empty", pb.ID) | ||
| } | ||
| seen := make(map[string]bool, len(pb.Templates)) | ||
| for _, id := range pb.Templates { | ||
| if id == "" { | ||
| return fmt.Errorf("playbook '%s': template id cannot be empty", pb.ID) | ||
| } | ||
| if seen[id] { | ||
| return fmt.Errorf("playbook '%s': duplicate template id '%s'", pb.ID, id) | ||
| } | ||
| seen[id] = true |
There was a problem hiding this comment.
Reject whitespace-only info.name and template IDs in validation.
Current checks allow values like " ", which pass validation but fail later during lookup/use.
🔧 Proposed fix
func (pb Playbook) Validate() error {
@@
- if pb.Info.Name == "" {
+ if strings.TrimSpace(pb.Info.Name) == "" {
return fmt.Errorf("playbook '%s': info.name cannot be empty", pb.ID)
}
@@
- seen := make(map[string]bool, len(pb.Templates))
- for _, id := range pb.Templates {
- if id == "" {
+ seen := make(map[string]bool, len(pb.Templates))
+ for _, rawID := range pb.Templates {
+ id := strings.TrimSpace(rawID)
+ if id == "" {
return fmt.Errorf("playbook '%s': template id cannot be empty", pb.ID)
}
if seen[id] {
return fmt.Errorf("playbook '%s': duplicate template id '%s'", pb.ID, id)
}
seen[id] = true
}📝 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 pb.Info.Name == "" { | |
| return fmt.Errorf("playbook '%s': info.name cannot be empty", pb.ID) | |
| } | |
| if len(pb.Templates) == 0 { | |
| return fmt.Errorf("playbook '%s': templates list cannot be empty", pb.ID) | |
| } | |
| seen := make(map[string]bool, len(pb.Templates)) | |
| for _, id := range pb.Templates { | |
| if id == "" { | |
| return fmt.Errorf("playbook '%s': template id cannot be empty", pb.ID) | |
| } | |
| if seen[id] { | |
| return fmt.Errorf("playbook '%s': duplicate template id '%s'", pb.ID, id) | |
| } | |
| seen[id] = true | |
| if strings.TrimSpace(pb.Info.Name) == "" { | |
| return fmt.Errorf("playbook '%s': info.name cannot be empty", pb.ID) | |
| } | |
| if len(pb.Templates) == 0 { | |
| return fmt.Errorf("playbook '%s': templates list cannot be empty", pb.ID) | |
| } | |
| seen := make(map[string]bool, len(pb.Templates)) | |
| for _, rawID := range pb.Templates { | |
| id := strings.TrimSpace(rawID) | |
| if id == "" { | |
| return fmt.Errorf("playbook '%s': template id cannot be empty", pb.ID) | |
| } | |
| if seen[id] { | |
| return fmt.Errorf("playbook '%s': duplicate template id '%s'", pb.ID, id) | |
| } | |
| seen[id] = true | |
| } |
🤖 Prompt for 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.
In `@pkg/template/playbook.go` around lines 129 - 143, The validation currently
accepts whitespace-only names/IDs; update the checks in the playbook validation
(where pb.Info.Name and loop over pb.Templates are validated) to trim whitespace
(e.g., strings.TrimSpace) and treat empty-after-trim as invalid: reject
pb.Info.Name if strings.TrimSpace(pb.Info.Name) == "" and reject any template id
if strings.TrimSpace(id) == ""; continue using the same error messages but
reference the trimmed values when checking duplicates and storing into seen to
avoid treating " a " and "a" as different.
1 participant
Summary by CodeRabbit
Release Notes
New Features
playbook run,playbook stop, andplaybook listCLI commands to manage playbooksinspectcommand to support viewing both templates and playbooksDocumentation