Use schtasks.exe CLI to schedule jobs on Windows #459
Conversation
WalkthroughThe pull request updates the repository by adding a new ignore pattern for Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant TS as Taskscheduler
participant TD as Task Definition
participant EX as schtasks.exe
C->>TS: CreateTask(config, schedules, permission)
TS->>TD: Build task definition
TS->>EX: Execute task creation (generate & pass XML)
EX-->>TS: Return task creation result
TS-->>C: Return status (success/failure)
sequenceDiagram
participant U as User
participant PC as PermissionModule
PC->>PC: Check cached credentials
alt Credentials not cached
PC->>U: Prompt for Windows password
U-->>PC: Provide password
else Credentials cached
PC-->>U: Return cached credentials
end
PC-->>U: Return username and password
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (22)
schtasks/trigger_test.go (1)
16-38: Consider extracting helper functions for better readability.The date handling logic is robust, but the string generation for
everyDayandeveryMonthcould be moved to separate helper functions to improve code organisation.Consider this refactoring:
+func generateEveryDayString() string { + var everyDay string + for day := 1; day <= 31; day++ { + everyDay += `<Day>` + strconv.Itoa(day) + `</Day>\s*` + } + return everyDay +} + +func generateEveryMonthString() string { + var everyMonth string + for month := 1; month <= 12; month++ { + everyMonth += `<` + time.Month(month).String() + `></` + time.Month(month).String() + `>\s*` + } + return everyMonth +} + func TestTriggerCreationFromXML(t *testing.T) { // ... existing date handling code ... - everyDay := "" - for day := 1; day <= 31; day++ { - everyDay += `<Day>` + strconv.Itoa(day) + `</Day>\s*` - } - everyMonth := "" - for month := 1; month <= 12; month++ { - everyMonth += `<` + time.Month(month).String() + `></` + time.Month(month).String() + `>\s*` - } + everyDay := generateEveryDayString() + everyMonth := generateEveryMonthString()schtasks/schtasks.go (3)
69-77: Validation for optional username and passwordThe documentation states that username and password must be specified together, but the code only checks for a non-empty password. If the password is given but the username remains empty, this might lead to a silently malformed command.
You could refine the logic, for instance:
if len(password) > 0 && len(username) == 0 { return "", errors.New("username is required when specifying a password") }
29-30: Check for command injection or sanitise sensitive parametersThough
exec.Commandis generally safe from shell interpretation, user-provided arguments such asfilenameor CSV data could be manipulated by an attacker in certain scenarios. Verify that these inputs cannot include malicious paths or arguments that might cause unexpected behaviours ofschtasks.exe.Also applies to: 51-54, 97-107
142-156: Clearer error messagesWhile the function correctly maps known conditions to custom errors, the fallback simply returns the original error string. Consider including any relevant context (e.g., arguments used) to facilitate debugging.
schtasks/settings.go (2)
39-46: Clarify repeated “Task Scheduler 2.0” version referencesAll enumerations from
TaskCompatibilityV1toTaskCompatibilityV24have the same comment stating “The task is compatible with Task Scheduler 2.0.” Even if these constants are functionally the same, this could be confusing for maintainers. Clarify if there are practical differences or if they are indeed identical implementations.
7-25: Struct defaulting strategyThese settings are generally optional and might require default values. Currently, there is no mechanism to set sensible defaults if a field is omitted. For example,
UseUnifiedSchedulingEngineor time-based fields could default to a safer value. Evaluate providing helper constructors or checks to ensure robust default behaviour.schtasks/taskscheduler_test.go (3)
18-25: Assert error type for unknown tasksThe test only asserts that an error is returned for an unknown task. Checking the error type or message would ensure that it correctly identifies missing tasks, rather than failing for unrelated reasons.
27-84: Test coverage for scheduling conflictsExisting tests create tasks and then validate their presence. However, there is no negative test verifying behaviour when tasks with duplicate names are created in quick succession. Consider adding a scenario that tries to create the same task name multiple times to confirm that the system gracefully handles or rejects duplicates.
160-164: Robustness of dynamic date logic in testsThese integration tests adjust the current day if it is the first day of the month or a Monday. This manual approach might cause confusion if run at other boundary conditions (e.g., month end on a Sunday). Consider a more robust approach or clarifying documentation for these date constraints.
Also applies to: 201-255
schtasks/taskscheduler.go (4)
59-64: Improve the user-facing error message
This error message could be more explicit about what is missing and why it matters.- return fmt.Errorf("it doesn't look like %s is installed on your system", binaryPath) + return fmt.Errorf("cannot find %s on PATH; please ensure 'schtasks.exe' is installed and available on this system", binaryPath)
70-125: Consider adding a success log or extra error context
Deleting and recreating tasks is a valid approach. For debugging, you might find it useful to log additional information when deletion or creation completes successfully.
129-132: Distinguish between non-existent tasks and general deletion errors
Handling a “task not found” error more gracefully could prevent confusion for end-users. A separate check might be valuable.
159-195: Robustness when parsing command lines
Currently, command parsing is based on splitting strings. Consider scenarios where the command path or arguments might contain spaces. A more robust approach ensures accurate parsing.schtasks/task.go (1)
307-315: Typo in function name
Rename “getRepetionDuration” to “getRepetitionDuration” for clarity and to match standard spelling.-func getRepetionDuration(start time.Time, recurrences []time.Time) period.Period { +func getRepetitionDuration(start time.Time, recurrences []time.Time) period.Period {schtasks/permission.go (2)
20-25: Consider security implications of storing password in memory.Storing the password in memory as a package-level variable could pose a security risk. Consider using a more secure approach, such as:
- Encrypting the password in memory
- Using Windows Credential Manager
- Clearing the password after use
28-46: LGTM! Well-implemented credential management.The function effectively manages user credentials with secure password input. Consider adding a method to clear the password from memory when it's no longer needed.
func userCredentials() (string, string, error) { if userName != "" { // we've been here already: we don't check for blank password as it's a valid password return userName, userPassword, nil } currentUser, err := user.Current() if err != nil { return "", "", err } userName = currentUser.Username fmt.Printf("\nCreating task for user %s\n", userName) fmt.Printf("Task Scheduler requires your Windows password to validate the task: ") userPassword, err = term.ReadPassword() if err != nil { return "", "", err } return userName, userPassword, nil } +// clearCredentials clears the cached credentials from memory +func clearCredentials() { + userName = "" + for i := range userPassword { + userPassword[i] = 0 + } + userPassword = "" +}schtasks/task_test.go (1)
13-33: Enhance test coverage with more test cases.Consider adding:
- Test case descriptions using the
namefield- Edge cases (e.g., invalid formats, boundary conditions)
- More schedule formats (e.g., specific months, years)
testData := []struct { + name string input string differences []time.Duration unique []time.Duration }{ { + "weekday range with specific time", "1..4,6,8:00", []time.Duration{1 * time.Hour, 1 * time.Hour, 1 * time.Hour, 2 * time.Hour, 2 * time.Hour}, []time.Duration{1 * time.Hour, 2 * time.Hour}, }, // ... existing test cases ... + { + "invalid format", + "invalid", + nil, + nil, + }, + { + "specific months", + "jan,feb,mar 1:00", + []time.Duration{24 * time.Hour, 24 * time.Hour}, + []time.Duration{24 * time.Hour}, + }, }schtasks/schtasks_test.go (2)
16-21: Enhance test coverage with field value assertions.The test only verifies the number of lines and fields. Consider adding assertions for specific field values to ensure correct parsing of task properties like TaskName, Status, etc.
func TestReadingCSVOutput(t *testing.T) { output, err := getCSV(bytes.NewBufferString(csvOutput)) require.NoError(t, err) assert.Len(t, output, 3) // lines assert.Len(t, output[0], 28) // fields + // Verify specific fields + assert.Equal(t, "TaskName", output[0][1], "Header field mismatch") + assert.Equal(t, "\\resticprofile backup\\self backup", output[1][1], "Task name mismatch") + assert.Equal(t, "Ready", output[1][3], "Status mismatch") }
46-63: Document error message patterns.Consider adding comments to explain the error message patterns from schtasks.exe. This would help maintainers understand which variations of error messages are handled.
testCases := []struct { message string expected error }{ + // Standard "file not found" error from schtasks.exe {`ERROR: The system cannot find the file specified.\r\n`, ErrNotRegistered}, + // Alternative "path not found" error {`ERROR: The system cannot find the path] specified.\r\n`, ErrNotRegistered}, + // Task-specific "not found" error {`ERROR: The specified task name "\resticprofile backup\toto" does not exist in the system.\r\n`, ErrNotRegistered}, // ... more cases with commentsschedule/handler_windows.go (1)
24-26: Document why Close() does nothing.Please add a comment explaining why the Close() method no longer needs to call schtasks.Close(). This will help future maintainers understand the design decision.
-// Close does nothing with this implementation +// Close does nothing with this implementation as schtasks.exe is executed +// as a separate process for each operation and doesn't maintain a connection func (h *HandlerWindows) Close() { }schtasks/trigger.go (2)
12-17: Consider using time.Duration for RandomDelay.The RandomDelay field might be better represented using time.Duration instead of period.Period, as it's a simple duration rather than a complex period.
type TimeTrigger struct { Enabled *bool `xml:"Enabled"` // indicates whether the trigger is enabled StartBoundary string `xml:"StartBoundary"` ExecutionTimeLimit *period.Period `xml:"ExecutionTimeLimit"` - RandomDelay *period.Period `xml:"RandomDelay,omitempty"` // a delay time that is randomly added to the start time of the trigger + RandomDelay *time.Duration `xml:"RandomDelay,omitempty"` // a delay time that is randomly added to the start time of the trigger }
70-78: Consider using a map for DaysOfWeek.The current structure uses individual string pointers for each day. A map might be more maintainable and reduce repetition.
-type DaysOfWeek struct { - Monday *string `xml:"Monday"` - Tuesday *string `xml:"Tuesday"` - Wednesday *string `xml:"Wednesday"` - Thursday *string `xml:"Thursday"` - Friday *string `xml:"Friday"` - Sunday *string `xml:"Sunday"` - Saturday *string `xml:"Saturday"` -} +type DaysOfWeek struct { + Days map[string]*string `xml:",any"` +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
.gitignore(1 hunks)examples/windows.yaml(1 hunks)go.mod(4 hunks)schedule/handler_windows.go(1 hunks)schtasks/actions.go(1 hunks)schtasks/errors.go(1 hunks)schtasks/permission.go(1 hunks)schtasks/principal.go(1 hunks)schtasks/schtasks.go(1 hunks)schtasks/schtasks_test.go(1 hunks)schtasks/settings.go(1 hunks)schtasks/task.go(1 hunks)schtasks/task_test.go(1 hunks)schtasks/taskscheduler.go(1 hunks)schtasks/taskscheduler_test.go(3 hunks)schtasks/trigger.go(1 hunks)schtasks/trigger_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build and test (1.23, macos-latest)
- GitHub Check: Build and test (1.23, windows-latest)
- GitHub Check: Build and test (1.23, ubuntu-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (41)
schtasks/trigger_test.go (4)
1-14: LGTM! Well-organised imports.The imports are appropriate and all are utilised within the test suite.
39-179: LGTM! Comprehensive test coverage.The test fixtures provide excellent coverage of various scheduling scenarios including single occurrences, daily, weekly, and monthly triggers.
127-132: Address the commented-out test case.Either implement or remove the commented test case for "twice on fridays". Commented code can lead to confusion and should be addressed.
Would you like me to help implement this test case or should it be removed?
181-216: LGTM! Robust test execution logic.The test execution is well-implemented with proper error handling and helpful debug output for failed tests.
schtasks/taskscheduler_test.go (1)
222-224: Potential double file closeOverall logic is sound, but the file is closed explicitly in line 231 and also deferred in line 223. Although closing an already closed file is typically harmless, it may result in a redundant error that is ignored. Confirm that this approach is intentional.
Also applies to: 231-231
schtasks/taskscheduler.go (7)
1-18: Well-written documentation block
These comments provide a clear overview of possible schedule types on Windows.
23-25: Confirm minimum Go version for 'slices' package
Theslicespackage is part of the Go standard library starting in Go 1.21. Ensure that your module’s go.mod mandates Go 1.21 or higher.
35-57: Security descriptor usage
The security descriptor string grants specific permissions to builtin admins, system, local service, and authenticated users. It looks correct for typical usage.
137-155: Status function reads cleanly
The status information is laid out in a user-friendly format, and the checks for an unregistered task are clearly handled.
197-199: Validate path sanitisation
IfprofileNameorcommandNamecould include special characters, you might consider sanitising or validating them before constructing the task path.
201-211: createTaskDefinition appears correct
The definition is built neatly, and the method calls to add actions and schedules align well with the rest of the code.
213-219: Potential out-of-range scenario
If the CSV data structure on the second row has fewer columns than the index, this might panic. Consider an explicit bounds check for safe handling.schtasks/task.go (16)
1-12: Initial declarations
No issues noted. Dependencies appear consistent with the rest of the package.
14-21: Constants alignment
These constants are straightforward for scheduling tasks, including a sensible maxTriggers limit.
23-29: RegistrationInfo struct
Looks coherent for storing XML-based task registration metadata.
31-40: Task struct
The XML schema references and structure appear correct for Windows task scheduler definitions.
42-81: NewTask constructor
Sets up a sensible default configuration, including principal logon type and scheduling engine settings.
83-89: AddExecAction
The method correctly appends new actions, with no logical concerns.
91-113: AddSchedules
Good handling of multiple schedule types, falling back to a warning if unknown.
115-124: addTimeTrigger
This function is compact and handles one-time triggers effectively.
126-132: addCalendarTrigger
Straightforward usage to store trigger definitions in a slice.
134-184: addDailyTrigger
Logical structure for daily triggers, with robust checks for a single trigger or multiple recurrences.
186-239: addWeeklyTrigger
Similar to daily triggers, with no evident issues.
241-279: addMonthlyTrigger
Handles potential complexity in monthly recurrences clearly and warns if day-of-month and day-of-week conflict.
281-305: compileDifferences
Generates difference arrays for scheduling calculations effectively.
317-364: convertMonths
Adequately maps month indexes to valid XML-based schedule parameters.
366-375: convertDaysOfMonth
Fills days of the month with a default range or uses an explicit list—no issues here.
377-401: convertWeekdays
Maps integer-based weekdays to appropriate fields, with Sunday covering both 0 and 7.schtasks/actions.go (1)
1-12: Actions and ExecAction structures
These definitions appear concise and practical, enabling multiple exec actions that can be serialised cleanly to XML.schtasks/errors.go (1)
7-11: LGTM! Well-defined error variables.The error variables are well-defined with clear, concise messages that follow Go's error message style guide. They cover essential error cases for task management.
schtasks/task_test.go (1)
35-46: LGTM! Well-structured test implementation.The test follows best practices:
- Uses table-driven testing pattern
- Uses appropriate assertion functions
- Properly validates error cases
schtasks/principal.go (1)
1-34: LGTM! Well-structured principal management.The implementation:
- Follows Go's best practices for type and constant definitions
- Includes comprehensive documentation with links to Microsoft's Task Scheduler Schema
- Uses appropriate XML struct tags for serialisation
schtasks/schtasks_test.go (1)
23-44: Well-structured error handling test!The test comprehensively covers various error cases for task names, including empty names, invalid formats, and non-existent tasks. Good use of sub-tests and buffer verification.
schedule/handler_windows.go (2)
48-70:❓ Verification inconclusive
Verify permission handling with schtasks.exe.
The permission handling looks correct, but please verify that the permissions are properly applied when creating tasks with schtasks.exe.
🏁 Script executed:
#!/bin/bash # Create a test task and verify its permissions schtasks /query /v /fo csv | grep -i "Run As User"Length of output: 151
Manual Verification Required: Confirm schtasks.exe Execution Environment and Task Permissions
The current script did not produce any output because the
schtaskscommand was not found in this environment. Please manually verify that the permissions are applied correctly by running the command on a Windows system whereschtasks.exeis available, or use an alternative method to check the task permissions.
19-22:❓ Verification inconclusive
Verify error handling in schtasks.Init().
The method now uses schtasks.Init() instead of Connect(). Please ensure that all potential errors from schtasks.exe are properly handled in the Init() implementation.
🏁 Script executed:
#!/bin/bash # Search for error handling in schtasks.Init() ast-grep --pattern 'func Init() error { $$$ return $err }'Length of output: 64
Action Required: Verify Error Handling in schtasks.Init() Implementation
The update now uses
schtasks.Init()instead ofConnect(), but our automated search did not reveal explicit error handling patterns within theInit()function. This absence of output from the initial search means we must manually verify that:
- The implementation of
schtasks.Init()properly detects and returns errors from the underlyingschtasks.exeinvocation.- No potential error from the scheduler initialisation is being silently ignored.
Please review the
schtasks.Init()function to ensure comprehensive error handling is in place.go.mod (6)
3-3: Update Go Version
The Go version has been updated to 1.23.6, providing access to newer language features and improvements. Please ensure that all developer and CI environments are compatible with this version.
19-19: Replace Dependency for Date Handling
The dependency has been updated fromgithub.com/rickb777/datetogithub.com/rickb777/periodat version v1.0.8. Ensure that any references or usages in the codebase are updated accordingly.
27-27: Update x/sys Dependency
The version ofgolang.org/x/syshas been bumped from v0.29.0 to v0.30.0. This update likely includes bug fixes and performance improvements; please verify that all system-level interactions remain unaffected.
43-43: Upgrade go-ole Dependency
Thegithub.com/go-ole/go-olepackage is now at v1.3.0. Check for any breaking changes in the COM interfacing API that might affect modules using it.
47-47: Add New Indirect Dependency: govalues/decimal
A new indirect dependencygithub.com/govalues/decimalat v0.1.36 has been added. Confirm that this package is intentionally included for enhanced decimal number operations and that there are no duplicate dependencies.
60-60: Update Plural Dependency
Thegithub.com/rickb777/pluralpackage has been updated to v1.4.2. Please ensure that the API remains consistent and that any related functionality in the codebase is unaffected.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
schtasks/trigger_test.go (1)
121-126: Consider addressing the commented-out test case.There's a commented-out test case for "twice on fridays". Either implement the test or remove the commented code to maintain cleanliness.
schtasks/task.go (1)
359-368: Consider adding input validation for days of month.The
convertDaysOfMonthfunction should validate that the input days are within the valid range (1-31).func convertDaysOfMonth(input []int) DaysOfMonth { if len(input) == 0 { all := make([]int, 31) for i := 1; i <= 31; i++ { all[i-1] = i } return DaysOfMonth{all} } + // Validate input days + valid := make([]int, 0, len(input)) + for _, day := range input { + if day >= 1 && day <= 31 { + valid = append(valid, day) + } + } - return DaysOfMonth{input} + return DaysOfMonth{valid} }schtasks/schtasks.go (1)
145-159: Consider using regular expressions for more robust error matching.The current string matching could be fragile if the error messages change slightly. Regular expressions would provide more flexible pattern matching.
+import "regexp" func schTasksError(message string) error { message = strings.TrimSpace(message) message = strings.TrimPrefix(message, "ERROR: ") - if strings.Contains(message, "The system cannot find the") || - strings.Contains(message, "does not exist in the system") { + notFoundPattern := regexp.MustCompile(`(?i)(system cannot find|does not exist in the system)`) + if notFoundPattern.MatchString(message) { return ErrNotRegistered }schtasks/taskscheduler.go (1)
75-83: Improve temporary file cleanup.The deferred cleanup could fail silently. Consider logging cleanup errors and using a more robust temporary file pattern.
file, err := os.CreateTemp("", "*.xml") if err != nil { return fmt.Errorf("cannot create XML task file: %w", err) } filename := file.Name() defer func() { - file.Close() - os.Remove(filename) + if err := file.Close(); err != nil { + clog.Warningf("failed to close temporary file: %v", err) + } + if err := os.Remove(filename); err != nil { + clog.Warningf("failed to remove temporary file: %v", err) + } }()schtasks/taskscheduler_test.go (1)
82-253: Consider enabling parallel test execution.The test cases are independent and could benefit from parallel execution. Add
t.Parallel()in the test function to improve test performance.for _, fixture := range fixtures { t.Run(fixture.description, func(t *testing.T) { + t.Parallel() var err error count++
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
codecov.yml(1 hunks)schedule/handler.go(1 hunks)schedule/handler_windows.go(1 hunks)schtasks/actions.go(1 hunks)schtasks/config.go(1 hunks)schtasks/constants.go(1 hunks)schtasks/errors.go(1 hunks)schtasks/permission.go(1 hunks)schtasks/principal.go(1 hunks)schtasks/schtasks.go(1 hunks)schtasks/schtasks_test.go(1 hunks)schtasks/settings.go(1 hunks)schtasks/task.go(1 hunks)schtasks/task_test.go(1 hunks)schtasks/taskscheduler.go(1 hunks)schtasks/taskscheduler_test.go(3 hunks)schtasks/trigger.go(1 hunks)schtasks/trigger_test.go(1 hunks)sonar-project.properties(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- sonar-project.properties
- schtasks/config.go
🚧 Files skipped from review as they are similar to previous changes (8)
- schtasks/actions.go
- schtasks/errors.go
- schtasks/task_test.go
- schtasks/permission.go
- schtasks/schtasks_test.go
- schtasks/settings.go
- schtasks/trigger.go
- schtasks/principal.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build and test (1.23, macos-latest)
- GitHub Check: Build and test (1.23, windows-latest)
- GitHub Check: Analyze (go)
- GitHub Check: Build and test (1.23, ubuntu-latest)
🔇 Additional comments (12)
codecov.yml (1)
14-14: Approval of the new ignore entry for schtasks/permission.go.The addition of
- schtasks/permission.goto the ignore list is appropriate given its specific role in managing Windows scheduling permissions. This exclusion from coverage metrics is intentional and aligns with the current approach for similar utility files.schedule/handler.go (1)
56-56: Brilliant improvement to the error message!The new error message is more user-friendly and provides clearer guidance on what action is needed.
schedule/handler_windows.go (2)
19-22: Excellent simplification of the initialisation!The change to use
lookupBinaryaligns well with the PR's objective of using the schtasks.exe CLI. The implementation is more straightforward and robust.
24-26: Good cleanup of unused functionality.The empty
Closemethod with a clear comment is appropriate since the CLI-based approach doesn't require connection management.schtasks/trigger_test.go (1)
18-31: Excellent test setup handling edge cases!The test setup thoughtfully handles edge cases by adjusting dates when tests run on the first day of the month or Mondays. This prevents test flakiness and ensures consistent results.
schtasks/task.go (3)
16-22: Well-structured task registration information!The
RegistrationInfostruct is well-designed with proper XML tags and includes a helpful link to the security descriptor documentation.
84-106: Excellent schedule conversion implementation!The schedule conversion logic is well-structured and handles different types of schedules (one-time, daily, weekly, monthly) with proper error handling and warning messages.
274-298: Brilliant documentation of the difference compilation algorithm!The
compileDifferencesfunction includes clear documentation with a practical example that illustrates the algorithm's behaviour.schtasks/constants.go (2)
6-12: LGTM! Well-defined constants for task scheduling.The constants are properly defined with appropriate values for Windows Task Scheduler integration.
13-34: Well-documented security descriptor with appropriate permissions.The security descriptor follows Microsoft's best practices:
- Full access for administrators and system
- Limited access for local service
- Read-only access for authenticated users
schtasks/schtasks.go (1)
16-34: LGTM! Robust implementation of task listing.The function properly handles errors, safely parses CSV data, and ensures a clean output through sorting and deduplication.
schtasks/taskscheduler.go (1)
102-124: LGTM! Clean implementation of status display.The function properly handles errors and presents task information in a well-formatted table.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #459 +/- ##
==========================================
+ Coverage 78.79% 79.68% +0.89%
==========================================
Files 128 130 +2
Lines 12893 12914 +21
==========================================
+ Hits 10158 10290 +132
+ Misses 2344 2219 -125
- Partials 391 405 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
schtasks/trigger_test.go (4)
18-34: Consider using fixed test dates instead of runtime-dependent dates.While the current implementation handles edge cases for the first day of the month and Mondays, using fixed test dates would make the tests more deterministic and easier to understand.
Consider this approach:
- dayOfTheMonth := "1" - if time.Now().Day() == 1 { - dayOfTheMonth = "2" - } - fixedDay := "Monday" - if time.Now().Weekday() == time.Monday { - fixedDay = "Tuesday" - } + // Use fixed dates for deterministic testing + const ( + testDayOfMonth = "15" // Middle of month to avoid edge cases + testWeekday = "Wednesday" // Middle of week to avoid edge cases + )
35-175: Consider adding edge case test scenarios.The test suite is comprehensive but could benefit from additional scenarios:
- Invalid schedule strings
- Schedules with timezone specifications
- Schedules spanning DST changes
Example additional test case:
+ { + "schedule with timezone", + []string{"2020-01-02 03:04 Europe/London"}, + `<TimeTrigger>\s*<StartBoundary>2020-01-02T03:04:00\+00:00</StartBoundary>\s*</TimeTrigger>`, + 1, + },
177-212: Enhance error messages for better debugging.When tests fail, consider providing more context in the error messages.
- assert.Len(t, match, fixture.expectedMatchCount, fixture.expected) + assert.Len(t, match, fixture.expectedMatchCount, + "Schedule '%s' produced incorrect number of triggers.\nExpected pattern: %s\nActual XML:\n%s", + fixture.schedules[0], fixture.expected, buffer.String())
214-228: Optimise string concatenation using strings.Builder.The current implementation uses string concatenation in loops, which can be inefficient for large numbers of iterations.
func generateEveryDayString() string { - var everyDay string + var builder strings.Builder for day := 1; day <= 31; day++ { - everyDay += `<Day>` + strconv.Itoa(day) + `</Day>\s*` + builder.WriteString(`<Day>`) + builder.WriteString(strconv.Itoa(day)) + builder.WriteString(`</Day>\s*`) } - return everyDay + return builder.String() }schtasks/schtasks.go (1)
54-63: Simplify error handling.The error check on line 62 is redundant as err is already checked in the if block.
func createTaskFile(task Task, w io.Writer) error { - var err error encoder := xml.NewEncoder(w) encoder.Indent("", " ") - err = encoder.Encode(&task) - if err != nil { - return err - } - return err + return encoder.Encode(&task) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
schtasks/schtasks.go(1 hunks)schtasks/schtasks_test.go(1 hunks)schtasks/taskscheduler_test.go(3 hunks)schtasks/trigger_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- schtasks/schtasks_test.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and test (1.23, windows-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
schtasks/trigger_test.go (1)
1-16: LGTM! Well-structured imports and build constraints.The file is properly configured with Windows build constraints and includes all necessary imports for testing and time manipulation.
schtasks/schtasks.go (7)
1-14: LGTM! Well-structured imports and package declaration.The build tag correctly ensures Windows-only compilation, and imports are appropriate for the package's functionality.
36-52: LGTM! Well-structured task information retrieval.The functions demonstrate good separation of concerns and proper error handling.
92-114: LGTM! Well-implemented task export and listing.Both functions demonstrate proper error handling and clean command execution.
116-147: LGTM! Well-structured task management functions.The functions demonstrate proper error handling and clean command execution.
149-174: LGTM! Robust error handling and input validation.The functions demonstrate thorough error mapping and proper input sanitisation.
65-90:❓ Verification inconclusive
Verify the binaryPath constant.
The function implementation is solid with proper parameter validation and error handling. However, let's verify the binaryPath constant usage.
🏁 Script executed:
#!/bin/bash # Search for binaryPath constant definition ast-grep --pattern 'const binaryPath = $_' rg 'const.*binaryPath.*=' -A 1Length of output: 77
Verify the binaryPath constant definition
The
createTaskfunction is implemented well, with proper parameter validation and error handling. However, our search did not reveal any definition for thebinaryPathconstant. It appears that this constant might be defined externally or injected at runtime. Please manually verify whetherbinaryPathis declared elsewhere in the codebase or configured via build-time settings. If it is defined in a central configuration or shared module, consider adding a clarifying comment to avoid confusion for future maintainers.
16-34:❓ Verification inconclusive
Verify the tasksPathPrefix constant.
The function implementation looks solid with proper error handling and list processing. However, let's verify the tasksPathPrefix constant usage.
🏁 Script executed:
#!/bin/bash # Search for tasksPathPrefix constant definition ast-grep --pattern 'const tasksPathPrefix = $_' rg 'const.*tasksPathPrefix.*=' -A 1Length of output: 87
Manual Verification Required for tasksPathPrefix Constant Definition
The implementation of the
getRegisteredTasksfunction appears sound, with proper error handling, filtering, sorting, and deduplication. However, our searches for thetasksPathPrefixconstant did not return any results. Please verify manually whether this constant is defined elsewhere in the codebase or needs to be added, as its absence could lead to unintended behaviour.schtasks/taskscheduler_test.go (4)
20-82: LGTM! Well-structured test cases.The tests demonstrate good isolation, proper error handling, and clean cleanup.
84-108: LGTM! Well-implemented task recreation test.The test demonstrates proper setup, error handling, and cleanup.
110-224: LGTM! Comprehensive integration test setup.The test demonstrates thorough edge case handling and clear test descriptions.
225-280: LGTM! Well-structured test execution.The test demonstrates proper resource management, cleanup, and thorough validation.
Replacement of the unmaintained taskmaster library by using the
schtasks.exeCLI toolstatuscommand not able to display system tasksBreaking change
Minimum supported Windows is now 10.
You can still use resticprofile on Windows < 10 but the scheduling will no longer work.