Fix scheduling arguments #423
Conversation
WalkthroughThe pull request primarily updates the version of the mock generation tool from Changes
Assessment against linked issues
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
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
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #423 +/- ##
==========================================
+ Coverage 72.17% 72.23% +0.06%
==========================================
Files 128 129 +1
Lines 13765 13796 +31
==========================================
+ Hits 9934 9965 +31
Misses 3392 3392
Partials 439 439
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
schtasks/config.go (1)
Line range hint
1-11: Consider adding documentation about argument handling.Since this is a breaking change in how arguments are handled, it would be helpful to add documentation about:
- How paths with spaces are handled
- Any escaping rules that users should be aware of
- Examples of valid argument strings
schedule/handler_test.go (1)
11-13: Consider documenting skip reasonThe platform abstraction change looks good. However, it would be helpful to document why these tests are skipped on Windows, as it's not immediately obvious to future maintainers.
Consider adding a comment like this:
func TestLookupExistingBinary(t *testing.T) { if platform.IsWindows() { + // Skip on Windows as this test verifies Unix-specific path lookup behaviour t.Skip() }Also applies to: 19-21
schedule/command_arguments.go (1)
5-13: Consider adding nil check in constructorWhile the constructor is clean and follows Go idioms, it might be worth adding a nil check to ensure
argsis not nil, returning an empty slice instead.func NewCommandArguments(args []string) CommandArguments { + if args == nil { + args = []string{} + } return CommandArguments{ args: args, } }schedule/config.go (1)
43-43: Consider documenting the argument handling behaviour.The change to use
NewCommandArgumentsis appropriate, but it would be helpful to document how this affects argument formatting, particularly for paths with spaces.Add a comment above the
SetCommandmethod explaining the argument handling behaviour:+// SetCommand sets the command details for scheduling. Arguments are automatically +// processed to ensure proper handling of paths with spaces and special characters. func (s *Config) SetCommand(wd, command string, args []string) {schedule/config_test.go (1)
38-38: Consider testing the string representation.The test only verifies
RawArgs(), but given the path quoting issues, it would be beneficial to also test the string representation of the arguments.Add an assertion to verify the string output:
assert.ElementsMatch(t, []string{"1", "2"}, schedule.Arguments.RawArgs()) + assert.Equal(t, `1 2`, schedule.Arguments.String())schedule/handler_windows.go (1)
61-61: Consider adding tests for path edge casesTo ensure robust handling of various path formats, consider adding tests for:
- Paths with multiple spaces
- Paths with special characters
- UNC paths (\server\share)
- Long paths
Would you like me to help generate comprehensive test cases for these scenarios?
schedule/handler_systemd.go (1)
110-110: Consider adding debug logging for command argumentsFor better troubleshooting of argument-related issues, consider adding debug logging to show how the arguments are being processed.
CommandLine: job.Command + " " + strings.Join(append([]string{"--no-prio"}, job.Arguments.RawArgs()...), " "), + // Log the constructed command line for debugging + _ = clog.Debugf("Generated command line: %s", CommandLine)schtasks/taskscheduler_test.go (1)
Line range hint
341-357: Consider adding test cases for paths with spaces.Given that this PR addresses an issue with paths containing spaces (issue #417), it would be beneficial to add test cases that verify this specific scenario. Consider adding test cases with:
- Arguments containing paths with spaces
- Working directory paths with spaces
Example test case to add:
scheduleConfig := &Config{ ProfileName: "test", CommandName: strconv.Itoa(count), Command: "echo", Arguments: "hello", WorkingDirectory: "C:\\", JobDescription: fixture.description, } + +// Add a new test case to fixtures +{ + "path with spaces", + []string{"2020-01-02 03:04"}, + `<TimeTrigger>\s*<StartBoundary>2020-01-02T03:04:00</StartBoundary>\s*(<ExecutionTimeLimit>PT0S</ExecutionTimeLimit>)?\s*</TimeTrigger>`, + 1, + &Config{ + ProfileName: "test", + CommandName: "path_test", + Command: "echo", + Arguments: `"C:\Program Files\My App\config.txt"`, + WorkingDirectory: `C:\Program Files\My App`, + JobDescription: "Test path with spaces", + }, +},schtasks/taskscheduler.go (1)
Line range hint
557-558: Consider updating the task status display format.The Status function concatenates the command and arguments with a space when displaying the task information, which might not accurately represent commands with quoted arguments or paths containing spaces.
Apply this diff to improve the display format:
- fmt.Fprintf(writer, "Exec:\t %v\n", action.Path+" "+action.Args) + fmt.Fprintf(writer, "Exec:\t %v\n", fmt.Sprintf("%s %s", action.Path, strings.Join(action.Args, " ")))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
- config/mocks/NamedPropertySet.go (1 hunks)
- config/mocks/ProfileInfo.go (1 hunks)
- config/mocks/PropertyInfo.go (1 hunks)
- config/mocks/SectionInfo.go (1 hunks)
- monitor/mocks/OutputAnalysis.go (1 hunks)
- schedule/command_arguments.go (1 hunks)
- schedule/command_arguments_test.go (1 hunks)
- schedule/config.go (2 hunks)
- schedule/config_test.go (2 hunks)
- schedule/handler_crond.go (2 hunks)
- schedule/handler_darwin.go (1 hunks)
- schedule/handler_systemd.go (1 hunks)
- schedule/handler_test.go (1 hunks)
- schedule/handler_windows.go (1 hunks)
- schedule/mocks/Handler.go (1 hunks)
- schedule_jobs.go (2 hunks)
- schedule_jobs_test.go (1 hunks)
- schtasks/config.go (1 hunks)
- schtasks/taskscheduler.go (6 hunks)
- schtasks/taskscheduler_test.go (1 hunks)
✅ Files skipped from review due to trivial changes (6)
- config/mocks/NamedPropertySet.go
- config/mocks/ProfileInfo.go
- config/mocks/PropertyInfo.go
- config/mocks/SectionInfo.go
- monitor/mocks/OutputAnalysis.go
- schedule/mocks/Handler.go
🧰 Additional context used
🔇 Additional comments (18)
schtasks/config.go (1)
7-7: LGTM! This change addresses the Windows Task Scheduler issue.This modification from
[]stringtostringaligns with fixing issue #417, where paths with spaces weren't being properly quoted in the Windows Task Scheduler.Let's verify the argument handling implementation:
✅ Verification successful
Let me gather more specific information about how the Windows Task Scheduler arguments are handled.
Let me search for the actual task creation and argument handling implementation.
Based on the code analysis, I can now provide a final response.
The change from
[]stringtostringis correctly implemented and safe.The codebase shows that:
- The
Argumentsfield inConfigstruct is properly used in the Windows Task Scheduler implementation- The task creation functions (
createUserTask,createSystemTask,createUserLoggedOnTask) all handle theArgumentsfield correctly as a single string- The Windows Task Scheduler API expects a single string for arguments, as evidenced by the
taskmaster.ExecActionstruct usageThis change aligns with Windows Task Scheduler's expectations and will properly handle paths with spaces, addressing issue #417.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the argument handling implementation to ensure proper quoting # Test: Search for the code that handles the argument concatenation rg -g '!vendor' -g '*.go' -A 10 'func.*Arguments.*string'Length of output: 6851
Script:
#!/bin/bash # Check how the schtasks package handles the Arguments field rg -g '!vendor' -g '*.go' -A 10 'type.*Config.*struct' schtasks/Length of output: 391
Script:
#!/bin/bash # Look for the actual implementation that uses the Arguments field rg -g '!vendor' -g '*.go' -A 10 'func.*Create.*Task' schtasks/Length of output: 64
Script:
#!/bin/bash # Check for any tests that verify argument handling rg -g '!vendor' -g '*_test.go' -A 10 'TestArguments|TestCreate.*Task' schtasks/Length of output: 81
Script:
#!/bin/bash # Look for the implementation files in schtasks fd . schtasks/ -e go --type f --exec cat {}Length of output: 34928
Script:
#!/bin/bash # Search for Windows-specific argument handling rg -g '!vendor' -g '*.go' -B 5 -A 10 'schtasks.*Arguments'Length of output: 60
schedule/handler_test.go (1)
6-6: LGTM: Clean platform package importThe addition of the platform package aligns well with the codebase's move towards better platform abstraction.
schedule/command_arguments.go (2)
1-4: LGTM: Clean package structure with minimal importsThe package name and imports are appropriate for the functionality.
40-48:⚠️ Potential issueHandle edge cases in argument quoting
The current implementation doesn't handle cases where:
- The string already contains quotes
- Windows paths with backslashes that might need escaping
- Arguments containing special characters
This could lead to issues with certain paths or arguments.
Let's check if there are any existing tests covering these edge cases:
Consider adding proper escaping:
func (ca CommandArguments) writeString(b *strings.Builder, str string) { + // Remove existing quotes if present + str = strings.Trim(str, `"`) + // Escape existing quotes + str = strings.ReplaceAll(str, `"`, `\"`) if strings.Contains(str, " ") { b.WriteString(`"`) b.WriteString(str) b.WriteString(`"`) } else { b.WriteString(str) } }schedule/command_arguments_test.go (1)
1-5: LGTM: Package structure and imports are appropriate.The package declaration and imports are minimal and correctly structured.
schedule/config.go (1)
17-17: Type change looks appropriate for handling complex arguments.This change from
[]stringtoCommandArgumentsappears to be a good foundation for addressing the path quoting issues mentioned in #417. The custom type should provide better control over argument formatting.Let's verify the impact of this type change:
✅ Verification successful
Let me analyze the results and generate additional verification scripts to check the impact of the type change.
This will help us understand:
- How the Arguments field is used in struct initializations
- Direct field access patterns
- Usage of the new CommandArguments type across the codebase
Based on the analysis of the shell script results, I can now provide a final response:
Type change is properly integrated and safe to use
The change from
[]stringtoCommandArgumentshas been consistently implemented across the codebase:
- All scheduler handlers (Windows, Darwin, SystemD, Crond) correctly use the new type through its interface methods (
String()orRawArgs()).- The type change is encapsulated within the scheduling subsystem and doesn't leak implementation details.
- Proper conversion is handled in
SetCommandmethod usingNewCommandArguments.- The change provides better argument handling whilst maintaining backward compatibility through the
RawArgs()method.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining direct []string argument handling # that might need updating after this type change # Look for potential argument handling patterns rg -g '!*.md' -g '!*_test.go' -A 3 'func.*\[\]string'Length of output: 37709
Script:
#!/bin/bash # Check for any direct usage of the Arguments field from Config struct ast-grep --pattern 'Config { $$$ Arguments $$$ }' # Check for any references to Arguments field rg -g '!*.md' -g '!*_test.go' "\.Arguments\b" -A 2 # Look for CommandArguments type usage rg -g '!*.md' -g '!*_test.go' "CommandArguments" -A 2Length of output: 13829
schedule/config_test.go (1)
17-17: Verify test coverage for path quoting scenarios.Whilst the change to use
NewCommandArgumentsis appropriate, the test data[]string{"1", "2"}doesn't validate the path quoting functionality mentioned in issue #417.Please add test cases that cover paths with spaces:
func TestScheduleProperties(t *testing.T) { + t.Run("handles paths with spaces", func(t *testing.T) { + schedule := Config{ + ConfigFile: `C:\Program Files\config.toml`, + Arguments: NewCommandArguments([]string{ + "--config", + `C:\Program Files\config.toml`, + "--name", + "my_task", + }), + } + assert.Contains(t, schedule.Arguments.String(), `"C:\Program Files\config.toml"`) + })schedule/handler_windows.go (1)
61-61: Verify argument formatting for paths with spacesThis change addresses issue #417 by using String() to format the arguments. Let's verify the fix handles all edge cases correctly.
✅ Verification successful
Let me gather more information about the implementation and tests.
Let me gather one final piece of information to verify the implementation.
Let me check one final detail about the String() method implementation.
String() method properly handles path quoting
The implementation of
String()inCommandArgumentscorrectly handles paths with spaces by:
- Automatically detecting arguments containing spaces
- Properly wrapping such arguments in double quotes
- Efficiently building the final string using
strings.Builder- Preserving arguments without spaces in their original form
The implementation is well-tested through:
TestShellCommandWithArgumentswhich verifies paths with spacesTestShellCommandwhich checks complete command string handling- Both tests cover Windows and non-Windows scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the String() method implementation handles paths with spaces # Expected: Find the String() method implementation to confirm proper quoting # Search for the String() method implementation ast-grep --pattern 'func ($x *CommandArguments) String() string { $$$ }' # Search for any tests covering paths with spaces rg -l 'TestCommandArguments.*String|test.*path.*space' -g '**/*_test.go'Length of output: 173
Script:
#!/bin/bash # Check the String() method implementation in the codebase rg -A 10 'String\(\) string' shell/command.go # Check the specific test cases in the test file cat shell/command_test.goLength of output: 15524
Script:
#!/bin/bash # Check the CommandArguments type and String() method rg -A 10 'type CommandArguments' schedule/Length of output: 576
Script:
#!/bin/bash # Check the String() method implementation in command_arguments.go cat schedule/command_arguments.goLength of output: 945
schedule/handler_crond.go (2)
93-93: LGTM: Consistent argument handlingThe change maintains consistency with the CreateJob method by using the same argument handling approach.
69-69: Verify space handling in pathsThe change to use
Arguments.String()looks good, but let's verify it properly handles paths with spaces as reported in issue #417.✅ Verification successful
Space handling in paths is properly implemented
The implementation of
CommandArguments.String()and its helper methodwriteString()correctly handles spaces in paths by:
- Detecting if an argument contains spaces using
strings.Contains(str, " ")- Wrapping such arguments in double quotes (
"argument with spaces")- Leaving arguments without spaces unquoted
- Joining all arguments with a space separator
This implementation addresses issue #417 by ensuring that paths containing spaces are properly quoted, preventing them from being interpreted as separate arguments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of CommandArguments.String() handles spaces correctly # Test: Search for the String() method implementation and its tests ast-grep --pattern 'func ($$_) String() string { $$$ }' # Test: Look for related test cases rg -A 5 'TestCommandArguments.*String'Length of output: 6515
Script:
#!/bin/bash # Let's check the implementation of writeString method in CommandArguments rg -A 10 'func.*writeString' schedule/command_arguments.go # Also check for any tests related to CommandArguments rg -l 'TestCommandArguments' --type go # If tests exist, let's see their content fd -e go -x cat {} \; -E vendor/ | rg -A 20 'TestCommandArguments'Length of output: 374
schedule_jobs.go (2)
144-144: Verify CommandArguments handling of paths with spacesThe switch to
schedule.NewCommandArguments(nil)suggests a more structured approach to argument handling. However, we should ensure this new type properly handles paths containing spaces.Let's examine the new CommandArguments implementation:
#!/bin/bash # Look for CommandArguments implementation echo "Searching for CommandArguments definition and methods:" ast-grep --pattern 'type CommandArguments $$$' # Check for quoting/escaping logic echo -e "\nSearching for quoting/escaping methods:" rg -A 5 'func.*Command.*Arguments.*String|func.*Quote|func.*Escape'
36-36:⚠️ Potential issueVerify if this change affects paths with spaces
This modification might be related to the issue described in #417 where paths containing spaces are not properly handled. The direct use of
scheduleConfig.ConfigFilewithout proper quoting could cause problems when the config file path contains spaces.Let's verify the argument handling:
Consider wrapping the config file path appropriately when constructing the arguments slice:
args := []string{ "--no-ansi", "--config", - scheduleConfig.ConfigFile, + fmt.Sprintf("%q", scheduleConfig.ConfigFile), "run-schedule", scheduleName, }schedule_jobs_test.go (2)
45-46: Excellent test coverage for path quoting issue.These assertions effectively verify the fix for issue #417 by ensuring:
- Raw arguments maintain the unquoted format internally
- String representation properly quotes paths containing spaces
This change directly addresses the Windows Task Scheduler issue where paths with spaces were not being handled correctly.
51-51: Consider additional path test cases.Whilst this test case covers paths with spaces, consider adding more test cases to verify handling of:
- Paths with special characters
- UNC paths (e.g.,
\\server\share\path with spaces)- Relative paths with spaces
Let's check if there are any existing test cases covering these scenarios:
schedule/handler_systemd.go (1)
110-110: Verify argument handling across different platformsThe change to use
RawArgs()method appears to be part of a broader solution to fix argument handling. While this change aligns with the newCommandArgumentstype, we should ensure it handles paths with spaces correctly across different platforms.Let's verify the argument handling:
schedule/handler_darwin.go (1)
167-167: Verify argument handling for paths with spacesThe change to use
RawArgs()appears to address the path quoting issue mentioned in #417. However, we should verify that launchd properly handles these arguments when they contain spaces.Let's verify the argument handling:
Consider adding a test case that specifically verifies the handling of paths with spaces in the launchd job creation. This would help prevent future regressions.
✅ Verification successful
Let me gather more specific information about the argument handling implementation and tests.
This will help us verify:
- How CommandArguments handles spaces in paths
- Whether there are specific tests for Darwin/launchd argument handling
- The actual implementation of argument parsing and handling
Based on the test files and implementation shown, I can now provide a final response:
Argument handling for paths with spaces is properly implemented
The
CommandArgumentsimplementation correctly handles raw arguments without any modification, which is exactly what launchd expects. The test suite demonstrates that:
- The
RawArgs()method returns unmodified arguments as received- The
String()method (used for display purposes) properly quotes arguments with spaces- Launchd's
ProgramArgumentsarray receives the raw arguments directly, which is the correct approach as launchd handles argument separation nativelyThe implementation is safe because:
- Arguments are passed through without modification via
RawArgs()- Launchd's plist format uses arrays for arguments, eliminating any shell-style parsing issues
- The original quoting issues are avoided by using the native array structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check how paths with spaces are handled in the codebase # Expected: Find instances where paths with spaces are handled, particularly in tests # Look for test cases involving paths with spaces rg -l "path.*with.*space|\".*\s+.*\"" --type go # Look for argument handling in CommandArguments implementation ast-grep --pattern 'func ($_) RawArgs() []string { $$$ }'Length of output: 4500
Script:
#!/bin/bash # Check the CommandArguments tests and implementation cat schedule/command_arguments_test.go cat schedule/command_arguments.go # Check Darwin-specific handler tests for argument handling cat schedule/handler_darwin_test.go # Look for any existing launchd plist examples or tests with spaces rg -A 5 -B 5 "ProgramArguments.*\[" schedule/handler_darwin_test.goLength of output: 7640
schtasks/taskscheduler_test.go (1)
352-352: LGTM! The type change aligns with the PR objectives.The modification of
Argumentsfrom slice to string is a step in the right direction for fixing the path quoting issue mentioned in #417.schtasks/taskscheduler.go (1)
117-117: LGTM: Consistent handling of command arguments across all task types.The transition from string concatenation to direct slice usage for command arguments ensures proper handling of paths containing spaces, addressing the issue reported in #417.
Also applies to: 157-157, 213-213, 247-247, 281-281, 310-310
Fixes regression bug introduced in #420
Fixes #417