Skip to content

feat: Add CLI arguments support for LaunchAndKill action#6

Open
kjerk wants to merge 1 commit intomainfrom
claude/issue-3-20260206-0228
Open

feat: Add CLI arguments support for LaunchAndKill action#6
kjerk wants to merge 1 commit intomainfrom
claude/issue-3-20260206-0228

Conversation

@kjerk
Copy link
Copy Markdown
Owner

@kjerk kjerk commented Feb 6, 2026

Add support for CLI arguments when launching processes via the LaunchAndKill action.

Changes

  • Add optional arguments parameter to ProcessTools.LaunchProcess
  • Pass ActionArgs through TimerActionService.Launch
  • Wire up ActionArgs in TimerManager.Start for LaunchAndKill presets

This allows users to specify CLI arguments when launching processes.
Example: /path/to/app.exe with args --arg1 xyz --arg2 abc

Fixes #3

@kjerk
Copy link
Copy Markdown
Owner Author

kjerk commented Feb 6, 2026

@claude Can you code review this please? It's likely good as a simple change, but want to be sure the idea of the action args being used in this way isn't going to harm useful future use cases for that field by cornering us, or if it's simply fine as is.

@claude
Copy link
Copy Markdown

claude bot commented Feb 6, 2026

Claude finished @kjerk's task in 1m 17s —— View job


Code Review

Reviewing PR #6: CLI arguments support for LaunchAndKill action

Progress

  • Read repository guidelines (CLAUDE.md)
  • Read modified files and understand changes
  • Analyze design decision around ActionArgs usage
  • Provide comprehensive code review feedback

Review Summary

The implementation looks solid and the design decision is sound. Using ActionArgs for CLI arguments is appropriate and doesn't corner you into limiting future use cases.

Code Quality

The changes are clean, minimal, and well-implemented:

  1. Proper parameter threading: The arguments parameter flows correctly through the call chain:

    • TimerManager.StartTimerActionService.LaunchProcessTools.LaunchProcess
  2. Good defensive coding: The null/whitespace check in ProcessTools.cs:165-167 ensures empty strings don't get passed as arguments

  3. Consistent optional parameter handling: Using string arguments = null follows C# conventions and makes the API backward compatible

Design Analysis: ActionArgs Usage

Your concern about whether using ActionArgs for launch arguments might limit future use cases is valid to consider, but I believe this design is fine for these reasons:

Why This Works Well

  1. Semantic flexibility: The field name ActionArgs is generic enough to represent "arguments for the action," which works for:

    • Current use: CLI arguments when launching a process
    • Future uses: Could potentially be repurposed for other action types if needed (e.g., custom parameters for Suspend, Kill, etc.)
  2. Currently only used for LaunchAndKill: Looking at TimerActionService.cs:8-18, the actionArgs parameter is accepted by Execute() but currently not used by any action except indirectly through the launch path. This gives you flexibility to extend other actions later if needed.

  3. Action-specific interpretation: Each TimerActionType can interpret ActionArgs differently:

    • LaunchAndKill: CLI arguments for the launched process
    • Future actions could use it for action-specific configuration

Potential Future Considerations

If you later need more structured or action-specific parameters, you have several evolution paths:

  1. Keep ActionArgs as a simple string: Parse it differently based on action type (simplest)
  2. JSON/structured format: Store structured data in the string field (more flexible, no schema change)
  3. Add new fields: Add LaunchArgs, KillArgs, etc. alongside the generic ActionArgs (breaking change but clearest)

The current approach doesn't block any of these paths.

Minor Observations

  1. Unused parameter in Execute(): In TimerActionService.cs:8, the actionArgs parameter is accepted but currently unused since LaunchAndKill goes through the Launch() method at timer start, not through Execute(). This is fine, but worth noting that there's an asymmetry:

    • Launch happens in TimerManager.Start:98-99 (uses ActionArgs)
    • Kill happens in Execute:15 (doesn't use ActionArgs)

    This is actually correct behavior for your use case, just pointing it out for clarity.

  2. UseShellExecute = true: In ProcessTools.cs:162, you're using UseShellExecute = true. This is fine for launching applications, but be aware:

    • ✅ Allows launching without full paths (searches PATH)
    • ✅ Handles file associations (can "launch" documents)
    • ⚠️ Slightly higher security surface area (shell interpretation)
    • ℹ️ Cannot redirect standard output/error (not relevant here)

    Given your use case (user-configured process launching), this is appropriate.

Verdict

✅ Approve the approach. The implementation is clean, the design doesn't limit future flexibility, and ActionArgs is a reasonable field name that can accommodate future use cases. The code quality is good with proper null checking and consistent parameter handling.


@kjerk kjerk added the run-tests Maintainer toggled command to run tests on a PR label Feb 14, 2026
@github-actions github-actions bot removed the run-tests Maintainer toggled command to run tests on a PR label Feb 14, 2026
@kjerk kjerk added the run-tests Maintainer toggled command to run tests on a PR label Feb 14, 2026
@github-actions github-actions bot removed the run-tests Maintainer toggled command to run tests on a PR label Feb 14, 2026
@kjerk kjerk added the run-tests Maintainer toggled command to run tests on a PR label Feb 14, 2026
@github-actions github-actions bot removed the run-tests Maintainer toggled command to run tests on a PR label Feb 14, 2026
@kjerk kjerk force-pushed the claude/issue-3-20260206-0228 branch from 7c68dd0 to 2d3bf4e Compare February 14, 2026 10:24
@kjerk kjerk added the run-tests Maintainer toggled command to run tests on a PR label Feb 14, 2026
@github-actions github-actions bot removed the run-tests Maintainer toggled command to run tests on a PR label Feb 14, 2026
@kjerk kjerk force-pushed the claude/issue-3-20260206-0228 branch from 2d3bf4e to 328027a Compare February 14, 2026 11:15
Repository owner deleted a comment from github-actions bot Feb 14, 2026
Repository owner deleted a comment from github-actions bot Feb 14, 2026
@kjerk kjerk added the run-tests Maintainer toggled command to run tests on a PR label Feb 14, 2026
@github-actions github-actions bot removed the run-tests Maintainer toggled command to run tests on a PR label Feb 14, 2026
Repository owner deleted a comment from github-actions bot Feb 14, 2026
- Add optional arguments parameter to ProcessTools.LaunchProcess
- Pass ActionArgs through TimerActionService.Launch
- Wire up ActionArgs in TimerManager.Start for LaunchAndKill presets

This allows users to specify CLI arguments when launching processes.
Example: /path/to/app.exe with args "--arg1 xyz --arg2 abc"

Fixes #3

Co-authored-by: kjerk <kjerk@users.noreply.github.com>
@kjerk kjerk force-pushed the claude/issue-3-20260206-0228 branch from 328027a to feca109 Compare February 14, 2026 11:25
@kjerk kjerk added the run-tests Maintainer toggled command to run tests on a PR label Feb 14, 2026
@github-actions
Copy link
Copy Markdown

Screenshot Tests: 3 captured · feca109

01_empty-state.png 02_populated-state.png 03_edit-form-open.png
01_empty-state.png 02_populated-state.png 03_edit-form-open.png

workflow run

@github-actions github-actions bot removed the run-tests Maintainer toggled command to run tests on a PR label Feb 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FR: Allow command to include cli arguments

1 participant