detect cycle in cli custom commands #765
Conversation
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
Signed-off-by: Pulak Kanti Bhowmick <[email protected]>
|
This is not wrong, but we still need to catch when atmos is executing atmos in a free form command. Walking the tree won't help with that. The only practical way is to set an environment variable for the sub command and increment it. We should allow some level of recursion, just not infinite. |
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a cycle detection mechanism for custom CLI commands in the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| func detectCycle(commands []schema.Command) bool { | ||
| // Build a command graph | ||
| graph := make(map[string][]string) | ||
| for _, cmd := range commands { | ||
| for _, step := range cmd.Steps { | ||
| // Add an edge from command to each command it depends on | ||
| graph[cmd.Name] = append(graph[cmd.Name], parseCommandName(step)) | ||
| } | ||
| } | ||
|
|
||
| // To track visited nodes and detect cycles | ||
| visited := make(map[string]bool) | ||
| recStack := make(map[string]bool) | ||
|
|
||
| // Run DFS for each command to detect cycles | ||
| for cmd := range graph { | ||
| if detectCycleUtil(cmd, graph, visited, recStack) { | ||
| return true // Cycle detected | ||
| } | ||
| } | ||
| return false // No cycle detected | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for command existence and depth limiting
The cycle detection implementation is solid, but could be enhanced in a few ways:
- Validate that all commands referenced in steps exist in the command list
- Add a maximum depth limit as mentioned in the requirements
Here's a suggested enhancement:
func detectCycle(commands []schema.Command) bool {
+ // Create a map of valid commands for quick lookup
+ validCommands := make(map[string]bool)
+ for _, cmd := range commands {
+ validCommands[cmd.Name] = true
+ }
+
// Build a command graph
graph := make(map[string][]string)
for _, cmd := range commands {
for _, step := range cmd.Steps {
- graph[cmd.Name] = append(graph[cmd.Name], parseCommandName(step))
+ cmdName := parseCommandName(step)
+ if cmdName != "" && !validCommands[cmdName] {
+ return true // Invalid command reference creates a broken chain
+ }
+ graph[cmd.Name] = append(graph[cmd.Name], cmdName)
}
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Seems like a good recommendation
There was a problem hiding this comment.
@osterman, here's an updated detectCycle function incorporating command existence validation and depth limiting:
func detectCycle(commands []schema.Command, maxDepth int) bool {
// Create a map of valid commands for quick lookup
validCommands := make(map[string]bool)
for _, cmd := range commands {
validCommands[cmd.Name] = true
}
// Build a command graph
graph := make(map[string][]string)
for _, cmd := range commands {
for _, step := range cmd.Steps {
cmdName := parseCommandName(step)
if cmdName != "" {
if !validCommands[cmdName] {
return true // Invalid command reference creates a broken chain
}
graph[cmd.Name] = append(graph[cmd.Name], cmdName)
}
}
}
// To track visited nodes and detect cycles
visited := make(map[string]bool)
recStack := make(map[string]bool)
// Run DFS for each command to detect cycles with depth limit
for cmd := range graph {
if detectCycleUtil(cmd, graph, visited, recStack, 0, maxDepth) {
return true // Cycle detected or depth limit exceeded
}
}
return false // No cycle detected
}
func detectCycleUtil(command string, graph map[string][]string, visited, recStack map[string]bool, depth, maxDepth int) bool {
if depth > maxDepth {
return true // Depth limit exceeded
}
// If the current command is in the recursion stack, there's a cycle
if recStack[command] {
return true
}
// If already visited, no need to explore again
if visited[command] {
return false
}
// Mark as visited and add to recursion stack
visited[command] = true
recStack[command] = true
// Recurse for all dependencies
for _, dep := range graph[command] {
if detectCycleUtil(dep, graph, visited, recStack, depth+1, maxDepth) {
return true
}
}
// Remove from recursion stack before backtracking
recStack[command] = false
return false
}You'll need to define an appropriate maxDepth when calling detectCycle to enforce the recursion limit.
| // Helper function to parse command name from the step | ||
| func parseCommandName(step string) string { | ||
| // Split the step into parts | ||
| parts := strings.Split(step, " ") | ||
|
|
||
| // Check if the command starts with "atmos" and has additional parts | ||
| if len(parts) > 1 && parts[0] == "atmos" { | ||
| // Return everything after "atmos" as a single string | ||
| return strings.Join(parts[1:], " ") | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance command parsing robustness
The current parsing logic might miss cycles in complex command formats and could mask issues by returning empty strings.
Consider this enhanced implementation:
func parseCommandName(step string) string {
- // Split the step into parts
parts := strings.Split(step, " ")
- // Check if the command starts with "atmos" and has additional parts
if len(parts) > 1 && parts[0] == "atmos" {
- // Return everything after "atmos" as a single string
- return strings.Join(parts[1:], " ")
+ // Extract the actual command name, handling flags and arguments
+ cmdParts := []string{}
+ for _, part := range parts[1:] {
+ if strings.HasPrefix(part, "-") {
+ break // Stop at first flag
+ }
+ cmdParts = append(cmdParts, part)
+ }
+ return strings.Join(cmdParts, " ")
}
return ""
}📝 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.
| // Helper function to parse command name from the step | |
| func parseCommandName(step string) string { | |
| // Split the step into parts | |
| parts := strings.Split(step, " ") | |
| // Check if the command starts with "atmos" and has additional parts | |
| if len(parts) > 1 && parts[0] == "atmos" { | |
| // Return everything after "atmos" as a single string | |
| return strings.Join(parts[1:], " ") | |
| } | |
| return "" | |
| } | |
| // Helper function to parse command name from the step | |
| func parseCommandName(step string) string { | |
| parts := strings.Split(step, " ") | |
| if len(parts) > 1 && parts[0] == "atmos" { | |
| // Extract the actual command name, handling flags and arguments | |
| cmdParts := []string{} | |
| for _, part := range parts[1:] { | |
| if strings.HasPrefix(part, "-") { | |
| break // Stop at first flag | |
| } | |
| cmdParts = append(cmdParts, part) | |
| } | |
| return strings.Join(cmdParts, " ") | |
| } | |
| return "" | |
| } |
| if detectCycle(commands) { | ||
| return errors.New("cycle detected in custom cli commands") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error reporting and implement environment variable tracking
The current implementation could provide more helpful error messages and implement the environment variable tracking suggested in the PR comments.
Consider this enhancement:
+const AtmosCommandDepthEnv = "ATMOS_COMMAND_DEPTH"
+
+func getCommandDepth() int {
+ depth := os.Getenv(AtmosCommandDepthEnv)
+ if depth == "" {
+ return 0
+ }
+ val, _ := strconv.Atoi(depth)
+ return val
+}
+
if detectCycle(commands) {
- return errors.New("cycle detected in custom cli commands")
+ return fmt.Errorf("cycle detected in custom CLI commands - this could lead to infinite recursion")
+}
+
+// Implement depth tracking as suggested in PR comments
+depth := getCommandDepth()
+if depth > 10 { // Configure max depth as needed
+ return fmt.Errorf("maximum command depth exceeded (%d) - possible recursive loop", depth)
}
+os.Setenv(AtmosCommandDepthEnv, strconv.Itoa(depth+1))Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@pkbhowmick something like this 👆
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@pkbhowmick something like this 👆
got it. Thank you
Thanks for the info @osterman . I'll change accordingly |
what
why
references
Summary by CodeRabbit
New Features
Bug Fixes
Documentation