Skip to content

refactor(client/clientimplementation): execution to use structured options#360

Merged
skevetter merged 1 commit intomainfrom
refactor-clientimplementation-to-structs
Jan 25, 2026
Merged

refactor(client/clientimplementation): execution to use structured options#360
skevetter merged 1 commit intomainfrom
refactor-clientimplementation-to-structs

Conversation

@skevetter
Copy link
Owner

@skevetter skevetter commented Jan 25, 2026 •
edited by coderabbitai bot
Loading

Signed-off-by: Samuel K skevetter@pm.me

Summary by CodeRabbit

  • Refactor
    • Restructured command execution and environment configuration systems by consolidating multi-parameter function calls into structured configuration objects throughout the codebase. These architectural improvements enhance code maintainability and extensibility without affecting functionality or user-facing behavior.

✏️ Tip: You can customize this high-level summary in your review settings.

@skevetter
refactor(client/clientimplementation): execution to use structured op…
aa2b49d 
…tions

Signed-off-by: Samuel K <skevetter@pm.me>
@coderabbitai
Copy link

coderabbitai bot commented Jan 25, 2026 •
edited
Loading

📝 Walkthrough

Walkthrough

This pull request refactors command execution across the codebase to use structured options objects instead of multiple positional parameters. Functions including RunCommand, RunCommandWithBinaries, and ToEnvironmentWithBinaries are updated to accept single options structs, consolidating their parameter lists into defined types.

Changes

Cohort / File(s) Summary
Agent command execution
cmd/agent/daemon.go		 Refactored RunCommand invocation to use RunCommandOptions struct (Ctx, Command, Environ, Stdout, Stderr)
Pro commands
cmd/pro/check_health.go, cmd/pro/create_workspace.go, cmd/pro/list_clusters.go, cmd/pro/list_projects.go, cmd/pro/list_templates.go, cmd/pro/list_workspaces.go		 Replaced RunCommandWithBinaries multi-argument calls with single CommandOptions struct across multiple list and utility commands
Pro self and workspace updates
cmd/pro/self.go, cmd/pro/update_workspace.go, cmd/pro/version.go, cmd/pro/watch_workspaces.go		 Consolidated RunCommandWithBinaries calls to use CommandOptions struct; added explicit output buffering in self.go and version.go
Provider utilities
cmd/provider/use.go		 Migrated RunCommandWithBinaries invocation to structured CommandOptions parameter
Binary management
pkg/binaries/download.go		 Introduced new EnvironmentOptions struct and refactored ToEnvironmentWithBinaries signature to accept single options parameter
Client implementation—machine operations
pkg/client/clientimplementation/machine_client.go		 Refactored all machine operation methods (Create, Start, Stop, Command, Status, Delete) to use CommandOptions; replaced per-call logging helpers with unified scheduleLogMessage mechanism (+105/-112 lines)
Client implementation—proxy operations
pkg/client/clientimplementation/proxy_client.go		 Consolidated RunCommandWithBinaries calls across proxy methods (Create, Up, SSH, Delete, Stop, Status, updateInstance) to use CommandOptions struct; updated logging in tryLock
Client implementation—workspace operations
pkg/client/clientimplementation/workspace_client.go		 Introduced CommandOptions and RunCommandOptions structs; refactored RunCommandWithBinaries and RunCommand signatures; updated environment construction and command execution paths (+122/-49 lines)
Custom driver
pkg/driver/custom/custom.go		 Updated RunCommand invocation to use new RunCommandOptions struct
Workspace utilities
pkg/workspace/list.go		 Refactored RunCommandWithBinaries call to use CommandOptions struct

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: refactoring client implementation to use structured options instead of multiple positional parameters for command execution.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai bot reviewed Jan 25, 2026
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pkg/client/clientimplementation/proxy_client.go (1)

394-415: Add interactive context guard to updateInstance in proxyClient.

The updateInstance method uses os.Stdin, os.Stdout, and os.Stderr directly, assuming interactive terminal access. However, unlike the daemonclient implementation which explicitly guards against non-interactive contexts with if !terminal.IsTerminalIn { return error }, the proxyClient has no such safeguard. While currently only called from the interactive cmd/up CLI, add a similar terminal check to prevent silent failures if future callers invoke RefreshOptions with reconfigure=true in non-interactive contexts.

🧹 Nitpick comments (3)
pkg/binaries/download.go (1)

33-44: Consider removing unused Log field or documenting its intended future use.

The opts.Log field from EnvironmentOptions is not used within ToEnvironmentWithBinaries. If it's intended for future logging within this function, consider adding a comment. Otherwise, it could be removed from the struct to avoid confusion.

pkg/client/clientimplementation/machine_client.go (1)

327-341: scheduleLogMessage could be extracted to a shared utility.

This helper function provides useful periodic logging during long operations. If used in multiple places, consider extracting it to a shared utilities package for reuse.

pkg/client/clientimplementation/workspace_client.go (1)

676-707: Clarify the relationship between RunCommand (public) and runCommand (private).

There are now two command execution functions:

  • RunCommand (public, lines 685-707) - lower-level, no logging, takes RunCommandOptions
  • runCommand (private, in machine_client.go lines 306-325) - adds debug logging, takes runCommandOptions

The naming similarity may cause confusion. Consider either:

  1. Renaming one for clarity (e.g., executeCommand for the private one)
  2. Adding documentation to clarify their distinct purposes

@skevetter skevetter merged commit bbb0664 into main Jan 25, 2026
38 checks passed
@skevetter skevetter deleted the refactor-clientimplementation-to-structs branch January 25, 2026 05:29
@coderabbitai coderabbitai bot mentioned this pull request Jan 27, 2026
Merged
@coderabbitai coderabbitai bot mentioned this pull request Feb 15, 2026
Closed
This was referenced Mar 10, 2026
Merged
Merged
@coderabbitai coderabbitai bot mentioned this pull request Mar 17, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@skevetter