Skip to content

feat!: Separate command execution options from tool-specific options#1828

Merged
thomhurst merged 13 commits into
mainfrom
fix-1811
Jan 3, 2026
Merged

feat!: Separate command execution options from tool-specific options#1828
thomhurst merged 13 commits into
mainfrom
fix-1811

Conversation

@thomhurst
Copy link
Copy Markdown
Owner

Summary

  • BREAKING CHANGE: All CLI tool service methods now use a 3-parameter signature: (options, executionOptions, cancellationToken)
  • Separates execution-related properties (WorkingDirectory, ThrowOnNonZeroExitCode, EnvironmentVariables, LogSettings) from tool-specific options into CommandExecutionOptions
  • Updates all tool integrations: DotNet, Git, Docker, Kubernetes, Helm, Terraform, AWS, Azure, and others
  • Slims down CommandLineToolOptions to only contain tool-specific properties

Motivation

Resolves #1811 - The previous design mixed tool-specific options with execution-related settings, leading to:

  • Naming conflicts when tool options had their own WorkingDirectory or similar properties
  • SRP (Single Responsibility Principle) violations
  • Confusion about which properties affect the tool vs. how commands are executed

New API Pattern

// Before (old API)
await context.DotNet().Build(new DotNetBuildOptions 
{
    Configuration = "Release",
    WorkingDirectory = "src"  // Execution option mixed with tool option
}, cancellationToken);

// After (new API)
await context.DotNet().Build(
    new DotNetBuildOptions { Configuration = "Release" },
    new CommandExecutionOptions { WorkingDirectory = "src" },
    cancellationToken);

Test plan

  • Core projects build successfully (0 errors)
  • Unit tests pass (TrxParsing, NUnit integration tests)
  • Full CI pipeline validation

🤖 Generated with Claude Code

thomhurst and others added 12 commits January 3, 2026 00:30
Documents the approved design for separating command execution options
from tool-specific options to eliminate naming conflicts and improve
Single Responsibility Principle compliance.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Detailed step-by-step plan covering:
- Core type changes (CommandExecutionOptions, CommandLineToolOptions)
- Interface and implementation updates
- Generator template updates
- Tool package regeneration
- Test and example updates

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add new CommandExecutionOptions record that will hold command execution
configuration separate from tool-specific arguments. This is the first
step in separating execution options from CommandLineOptions.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Make CommandLineToolOptions abstract with only tool-related properties
- Remove inheritance from CommandLineOptions
- Delete CommandLineOptions.cs (replaced by CommandExecutionOptions)

Part of #1811

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Updated ExecuteCommandLineTool method signature to accept
  CommandExecutionOptions? executionOptions parameter
- Added execOpts resolution at the start of the method
- Updated all references to use execOpts instead of options for:
  - WorkingDirectory
  - EnvironmentVariables
  - ExecutionTimeout
  - GracefulShutdownTimeout
  - ThrowOnNonZeroExitCode
  - Sudo
  - InputLoggingManipulator
  - OutputLoggingManipulator
  - InternalDryRun
- Updated Of method signature to accept CommandExecutionOptions
- Updated all _commandLogger.Log calls to pass execOpts parameter

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update the ICommandLogger interface and CommandLogger implementation
to accept CommandExecutionOptions instead of relying on properties
from CommandLineToolOptions. LogSettings and InternalDryRun are now
read from the execOpts parameter.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add GenericCommandLineToolOptions for creating generic CLI tool options
- Update CommandExtensions to use CommandExecutionOptions instead of deleted CommandLineOptions
- Fix all ExecuteCommandLineTool calls to use new 3-parameter signature
- Update CommandLogger to not reference removed CommandLogging property on tool options
- Update Command.GetOptionsObject to not reference removed OptionsObject property
- Remove Sudo assignments from Linux options (now handled by CommandExecutionOptions)

Part of #1811

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Updated the code generators to include the new executionOptions parameter:
- ServiceInterfaceGenerator: Added executionOptions parameter to method signatures
- GeneratorUtils.GenerateServiceMethod: Added executionOptions parameter and docs
- SubDomainClassGenerator: Updated GenerateExecuteMethod with executionOptions

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…1811)

BREAKING CHANGE: All CLI tool service methods now use a 3-parameter signature.

This major refactoring separates concerns by moving execution-related properties
(WorkingDirectory, ThrowOnNonZeroExitCode, EnvironmentVariables, LogSettings)
from CommandLineToolOptions to a dedicated CommandExecutionOptions parameter.

Changes:
- Update all CLI tool interfaces and implementations to use 3-parameter methods
- Update DotNet, Git, Docker, Kubernetes, Helm, Terraform, and other tool services
- Update build modules, tests, and examples to use the new API
- Slimmed-down CommandLineToolOptions to only contain tool-specific properties

New API pattern:
  await context.DotNet().Build(
      new DotNetBuildOptions { Configuration = "Release" },
      new CommandExecutionOptions { WorkingDirectory = "src" },
      cancellationToken);

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings January 3, 2026 11:45
@thomhurst
Copy link
Copy Markdown
Owner Author

Summary

This PR introduces a breaking change that separates execution-related properties from tool-specific options by introducing a new 3-parameter signature for all CLI tool service methods.

Critical Issues

None found ✅

Based on the PR description and codebase review, this change:

  • Addresses a legitimate architectural concern (issue Design: Separate command execution options from tool-specific options #1811) - mixing execution context with tool-specific options violates SRP
  • Resolves naming conflicts - properties like WorkingDirectory can now exist independently in both tool options and execution options
  • Follows a consistent pattern - 1720 files updated suggests systematic application across all tool integrations
  • Maintains type safety - The new CommandExecutionOptions class provides strong typing for execution context

Suggestions

1. Documentation & Migration Guide

Given the scope of this breaking change (1720 files), consider:

  • Adding a migration guide in the PR description or docs showing common patterns
  • Updating any getting-started documentation or examples
  • Consider adding analyzer warnings or compiler messages to guide users during migration

2. Backward Compatibility Helper (Optional)

For such a large breaking change, consider whether an [Obsolete] overload could ease migration:

[Obsolete("Use the 3-parameter overload")]
Task<CommandResult> Build(DotNetBuildOptions? options, CancellationToken token);

Though given this is a major version change, a clean break is also reasonable.

3. Test Coverage

The PR description shows:

  • ✅ Core projects build successfully
  • ✅ Unit tests pass (TrxParsing, NUnit integration tests)
  • ⬜ Full CI pipeline validation

Ensure the full CI pipeline completes successfully before merging to validate all 1720 file changes.

Previous Review Status

No previous comments found.

Verdict

APPROVE - No critical issues

This is a well-motivated breaking change that improves the API design by properly separating concerns. The systematic application across all tool integrations demonstrates thoroughness. The change aligns with SOLID principles (SRP) and resolves concrete issues (#1811) around naming conflicts.

…meter

Updated all Azure service files to use named parameter syntax
`cancellationToken: token` instead of positional `token` to match
the new 3-parameter API signature for ExecuteCommandLineTool.

This is part of Issue #1811 - separating command execution options
from tool-specific options.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@thomhurst thomhurst merged commit 61a1886 into main Jan 3, 2026
8 of 12 checks passed
@thomhurst thomhurst deleted the fix-1811 branch January 3, 2026 12:04
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.

Design: Separate command execution options from tool-specific options

2 participants