Add infra commands #436
Add infra commands #436
Conversation
WalkthroughAdds CLI commands for cluster and machine management, a provider plugin framework (AWS/GCP) for cluster/machine provisioning, an execution DSL and TUI for running multi-step specs, API-layer infrastructure CRUD, new error codes, and related utilities and dependencies. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as cmd/{cluster,machine}.go
participant API as internal/infrastructure/infrastructure.go
participant Dispatcher as internal/infrastructure/cluster.go
participant Provider as internal/infrastructure/{aws,gcp}.go
participant TUI as internal/infrastructure/tui.go
User->>CLI: invoke create/list/remove/status
CLI->>API: call Create/List/Get/Delete endpoints
API-->>CLI: return response
CLI->>Dispatcher: request provider Setup/CreateMachine (when provisioning)
Dispatcher->>Provider: delegate to registered provider
Provider->>TUI: prompt/confirm/run steps
TUI->>User: show command, allow run/edit/skip
User->>TUI: respond
Provider->>Provider: run ExecutionSpec via util.runCommand
Provider-->>Dispatcher: success/failure
Dispatcher-->>API: finalize (e.g., return created resource or rollback)
CLI-->>User: formatted output (JSON/table) or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
* AWS initil implementation * more work on getting aws working from scratch * refactor awsSpecification commands * More work on gettting AWS working * cluster create and machine create commands fixes * checks if clustering is enabled for the authenticated user * remove debug logs * Small nits from Coder Rabbit * cleanup
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (13)
internal/infrastructure/gcp.go (2)
211-213: Hard-coded zone, machine type, and image family limit flexibility.Lines 211-213 hard-code the zone (
us-central1-a), machine type (e2-standard-4), and image family (hadron). These should be parameterized based on the cluster's region and size to match user selections.Consider:
- Mapping
cluster.Regionto appropriate zones- Mapping
cluster.Type(dev/small/medium/large) to machine types- Making image family/project configurable
88-90: Document why CreateMachine is a no-op for GCP.The CreateMachine method returns nil without implementation. If GCP machine provisioning is handled differently or deferred to a future iteration, add a comment explaining the rationale.
func (s *gcpSetup) CreateMachine(ctx context.Context, logger logger.Logger, region string, token string, clusterID string) error { + // TODO: Implement GCP machine provisioning or document if not applicable return nil }internal/infrastructure/util.go (2)
69-76: AWS-specific logic in generic command utility.Lines 69-76 contain AWS-specific error handling (
EntityAlreadyExists,AlreadyExists) in a generic command execution function. This creates coupling between the utility and AWS provider details.Consider moving provider-specific error handling to the provider implementations (aws.go, gcp.go) or making error handling pluggable via a callback.
45-83: Consider adding command timeout handling.The
runCommandfunction usesexec.CommandContextwhich respects the context, but infrastructure commands can be long-running. If the context doesn't have a timeout, commands could hang indefinitely.Consider documenting expected timeout behavior or adding explicit timeout recommendations for callers.
cmd/cluster.go (1)
28-54: Hard-coded provider regions should be externalized.The
providerRegionsmap hard-codes region lists for each provider. As cloud providers add/remove regions frequently, this data will become stale and require code changes to update.Consider:
- Loading regions from a configuration file
- Fetching regions dynamically from provider APIs (with caching)
- Or documenting the maintenance burden
internal/infrastructure/tui.go (3)
187-191: Duplicate stdin assignmentc.Stdin is set twice.
- c.Stdin = os.Stdin - c.Stdout = os.Stdout - c.Stderr = os.Stderr - c.Stdin = os.Stdin + c.Stdin = os.Stdin + c.Stdout = os.Stdout + c.Stderr = os.Stderr
143-156: Nil runner would panic; add guardexecAction assumes runner != nil when canExecute. Add a defensive check.
case run: var skip bool var err error if skipFunc != nil { skip, err = skipFunc(ctx) if err != nil { return err } } if !skip { - if err := runner(ctx, cmd, args); err != nil { + if runner == nil { + return fmt.Errorf("internal error: runner is nil") + } + if err := runner(ctx, cmd, args); err != nil { return err } }
162-165: Library helper calling os.Exit reduces composabilityReturning an error lets callers decide exit behavior; os.Exit(1) here hard-exits from deep inside.
- case cancelled: - os.Exit(1) + case cancelled: + return fmt.Errorf("cancelled")cmd/machine.go (2)
331-340: Interactive region selection cancellation not handledIf user cancels Select, region == "". Bail out cleanly.
} else if tui.HasTTY { // Interactive mode - prompt for missing values cluster := promptForClusterSelection(ctx, logger, apiUrl, apikey) provider = cluster.Provider region = promptForRegionSelection(ctx, logger, provider) clusterID = cluster.ID + if region == "" { + tui.ShowWarning("cancelled") + return + } } else {
299-305: Command group likely “management”, not “info”Create is a mutating operation; align with the “Machine Management” group.
- GroupID: "info", + GroupID: "management",internal/infrastructure/aws.go (1)
118-158: When AWS CLI is present but unauthenticated, region may remain emptycanExecute() sets skipFailedDetection and may not prompt for region; ensure a fallback from env or a prompt even when aws is found but not usable.
_, err = exec.LookPath("aws") if err == nil { _, err := runCommand(ctx, logger, "Checking AWS authentication...", "aws", "sts", "get-caller-identity") authenticated := err == nil if authenticated { val, err := runCommand(ctx, logger, "Checking AWS region...", "aws", "configure", "get", "region") if err == nil { canExecuteAWS = true region = strings.TrimSpace(val) - if region == "" { - region = "us-east-1" // default region - } + if region == "" { + region = "us-east-1" + } tui.ShowBanner("AWS Tools Detected", "I'll show you the command to run against the AWS account in region "+region+". You can choose to have me execute it for you, or run it yourself. If you prefer to run it on your own, the command will automatically be copied to your clipboard at each step.", false) } } - if !canExecuteAWS && region != "" { + if !canExecuteAWS { + if region == "" { + if v := os.Getenv("AWS_DEFAULT_REGION"); v != "" { + region = v + } else if v := os.Getenv("AWS_REGION"); v != "" { + region = v + } + if region == "" { + region = tui.Input(logger, "Please enter your AWS region:", "us-east-1") + } + } tui.ShowBanner("AWS Tools Detected but not Authenticated", "I'll show you the command to run against AWS region "+region+". You can choose to have me execute it for you, or run it yourself. If you prefer to run it on your own, the command will automatically be copied to your clipboard at each step.", false) } skipFailedDetection = true }internal/infrastructure/spec.go (1)
64-76: Double interpolation and runner ignoring argsargs are interpolated for display, but Execute.Run re-interpolates and ignores the args passed to execAction. This can drift and adds cost.
- args, err := ctx.Interpolate(s.Execute.Arguments...) + args, err := ctx.Interpolate(s.Execute.Arguments...) if err != nil { return err } return execAction( ctx.Context, ctx.Runnable, s.Title, s.Description, s.Execute.Command, args, - func(_ctx context.Context, cmd string, args []string) error { - return s.Execute.Run(ctx) - }, + func(_ctx context.Context, cmd string, args []string) error { + // execute with already-interpolated args and apply validation + out, err := runCommand(ctx.Context, ctx.Logger, s.Execute.Message, cmd, args...) + if err != nil { + return err + } + return s.Execute.Validate.Matches(ctx, strings.TrimSpace(string(out))) + }, s.Execute.Success,internal/infrastructure/infrastructure.go (1)
186-189: Message nit: “error checking cluster clustering enabled”Minor wording cleanup for clarity.
- return false, fmt.Errorf("error checking cluster clustering enabled: %w", err) + return false, fmt.Errorf("error checking clustering enabled: %w", err)
📜 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 (14)
cmd/cluster.go(1 hunks)cmd/machine.go(1 hunks)cmd/root.go(4 hunks)error_codes.yaml(1 hunks)go.mod(5 hunks)internal/errsystem/errorcodes.go(1 hunks)internal/infrastructure/aws.go(1 hunks)internal/infrastructure/cluster.go(1 hunks)internal/infrastructure/gcp.go(1 hunks)internal/infrastructure/infrastructure.go(1 hunks)internal/infrastructure/spec.go(1 hunks)internal/infrastructure/tui.go(1 hunks)internal/infrastructure/util.go(1 hunks)internal/util/api.go(0 hunks)
💤 Files with no reviewable changes (1)
- internal/util/api.go
🧰 Additional context used
📓 Path-based instructions (1)
cmd/*.go
📄 CodeRabbit inference engine (.cursor/rules/code-generation.mdc)
Ensure CLI commands build and test the pipeline (generation → SDK loading → agent usage) without writing into source SDK
Files:
cmd/root.gocmd/machine.gocmd/cluster.go
🧬 Code graph analysis (7)
internal/infrastructure/cluster.go (1)
internal/infrastructure/infrastructure.go (2)
Cluster(21-33)CreateMachine(242-271)
internal/infrastructure/gcp.go (3)
internal/infrastructure/cluster.go (2)
ClusterSetup(19-22)Setup(33-38)internal/infrastructure/infrastructure.go (2)
Cluster(21-33)CreateMachine(242-271)internal/infrastructure/spec.go (2)
ExecutionContext(98-103)ExecutionSpec(57-62)
cmd/root.go (4)
internal/bundler/bundler.go (1)
Version(31-31)internal/util/api.go (1)
Version(30-30)internal/project/project.go (1)
Version(32-32)internal/errsystem/console.go (1)
Version(24-24)
cmd/machine.go (4)
internal/util/api.go (2)
EnsureLoggedIn(389-406)GetURLs(268-300)internal/infrastructure/infrastructure.go (8)
EnsureMachineClusteringEnabled(226-234)Machine(36-54)ListMachines(134-148)DeleteMachine(167-180)GetMachine(151-164)CreateMachine(242-271)Cluster(21-33)ListClusters(90-99)internal/errsystem/errorcodes.go (3)
ErrInvalidArgumentProvided(129-132)ErrApiRequest(21-24)ErrMissingRequiredArgument(133-136)internal/errsystem/errsystem.go (2)
WithContextMessage(120-124)WithUserMessage(70-74)
internal/infrastructure/aws.go (3)
internal/infrastructure/cluster.go (2)
ClusterSetup(19-22)Setup(33-38)internal/infrastructure/infrastructure.go (2)
Cluster(21-33)CreateMachine(242-271)internal/infrastructure/spec.go (2)
ExecutionSpec(57-62)ExecutionContext(98-103)
cmd/cluster.go (6)
internal/organization/org.go (1)
ListOrganizations(26-39)internal/errsystem/errorcodes.go (4)
ErrApiRequest(21-24)ErrInvalidArgumentProvided(129-132)ErrMissingRequiredArgument(133-136)ErrCreateProject(9-12)internal/errsystem/errsystem.go (2)
WithContextMessage(120-124)WithUserMessage(70-74)internal/util/api.go (2)
EnsureLoggedIn(389-406)GetURLs(268-300)internal/infrastructure/infrastructure.go (7)
EnsureClusteringEnabled(215-223)Cluster(21-33)CreateCluster(66-87)CreateClusterArgs(57-63)ListClusters(90-99)DeleteCluster(118-131)GetCluster(102-115)internal/infrastructure/cluster.go (1)
Setup(33-38)
internal/infrastructure/infrastructure.go (3)
internal/util/api.go (1)
NewAPIClient(70-78)internal/errsystem/errorcodes.go (1)
ErrApiRequest(21-24)internal/errsystem/errsystem.go (2)
WithContextMessage(120-124)WithUserMessage(70-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
go.mod (1)
21-30: LGTM! Dependency additions align with CLI enhancements.The new direct dependencies support the infrastructure CLI features introduced in this PR:
go-shellwordsandgo-quotefor command parsing and shell executionreflowfor text formatting in terminal outputclipboardfor copying commands to clipboardgolang.org/x/termpromoted to direct for terminal interactionerror_codes.yaml (1)
101-105: LGTM! Error codes are well-defined.The new error codes CLI-0032 and CLI-0033 provide clear categorization for argument validation errors, which align with the cluster management CLI commands introduced in this PR.
internal/errsystem/errorcodes.go (1)
129-136: LGTM! Generated error codes follow the existing pattern.The new error variables are correctly generated and consistent with the rest of the error system.
cmd/root.go (1)
49-144: LGTM! Custom help provides clear command organization.The categorized help output groups commands logically (Core, Project, Infrastructure, Authentication, Tools) and includes a dynamic "Other Commands" section for extensibility. The implementation correctly handles both TTY and non-TTY scenarios.
internal/infrastructure/cluster.go (2)
19-38: LGTM! Clean provider registration pattern.The ClusterSetup interface and registry provide a clean extensibility point for infrastructure providers. The dispatcher correctly returns an error for unregistered providers, and the duplicate-registration check in
register()prevents initialization bugs.
44-66: LGTM! Secure key generation implementation.The
generateKey()function correctly generates ECDSA P-256 key pairs, marshals them to PEM format, and base64-encodes for transport. Error propagation is appropriate throughout.cmd/cluster.go (4)
103-139: Organization selection logic handles single-org case well.The
promptForClusterOrganizationfunction correctly handles both single and multi-organization scenarios, with appropriate fallbacks for non-TTY environments and preference persistence.
167-290: Cluster creation flow is well-structured.The
clusterNewCmdproperly validates inputs, provides interactive prompts when available, and gracefully handles both TTY and non-TTY scenarios. Error handling uses the centralized error system consistently.The confirmation step (
tui.Ask) before cluster creation is a good safeguard against accidental infrastructure provisioning.
306-375: List command provides clear output formatting.The
clusterListCmdhandles empty states well with helpful guidance, sorts output for consistency, and supports both JSON and table formats. The table columns are well-chosen for quick cluster overview.
394-423: Remove command includes appropriate safety checks.The
clusterRemoveCmdrequires explicit confirmation unless--forceis provided, which is a good safety pattern for destructive operations. The warning message clearly states the action is irreversible.
| ready := tui.Ask(logger, "Ready to create the cluster", true) | ||
| if !ready { | ||
| logger.Info("Cluster creation cancelled") | ||
| os.Exit(0) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Confirmation prompt can be bypassed in non-interactive mode.
Lines 259-263 show a confirmation prompt with tui.Ask, but in non-interactive mode (no TTY), this would need to default to true or require a --yes flag. Verify that tui.Ask handles non-TTY appropriately to prevent silent failures or forced confirmations.
🏁 Script executed:
#!/bin/bash
# Check how tui.Ask handles non-TTY environments
ast-grep --pattern $'func Ask($$$) $_ {
$$$
}'Length of output: 0
I'll search for the tui.Ask function and understand how it handles non-TTY environments:
I need to search the codebase directly to find how tui.Ask is implemented and how it handles non-TTY environments:
🏁 Script executed:
#!/bin/bash
# Find imports of tui package and Ask function usage
echo "=== Searching for tui package imports ==="
rg -n "tui\." cmd/cluster.go
echo "=== Searching for tui package import statement ==="
rg -n "import.*tui" --type=go
echo "=== Searching for Ask function definition ==="
rg -nP "func\s+Ask\s*\(" --type=go
echo "=== Looking for non-TTY or TTY detection ==="
rg -nP "(TTY|isatty|interactive)" --type=go -C 2Length of output: 20789
🏁 Script executed:
#!/bin/bash
# Search for tui.Ask usage and definition
echo "=== Searching for tui imports ==="
rg -n "import.*tui" --type=go
echo ""
echo "=== Searching for Ask function calls ==="
rg -nP "Ask\s*\(" --type=go
echo ""
echo "=== Look at the imports and context in cmd/cluster.go ==="
head -50 cmd/cluster.go
echo ""
echo "=== Look for tui package ==="
fd -t f "tui" --type=go | head -20Length of output: 7323
</script>
🏁 Script executed:
#!/bin/bash
# Get detailed context around the problematic lines
echo "=== Full context of cmd/cluster.go around line 259 ==="
sed -n '200,270p' cmd/cluster.go
echo ""
echo "=== Check go.mod for tui dependency ==="
grep -i "go-common" go.mod
echo ""
echo "=== Check how tui.Ask is called at line 408 (deletion) ==="
sed -n '400,420p' cmd/cluster.goLength of output: 3711
</script>
🏁 Script executed:
#!/bin/bash
# Look for external tui package source or documentation
echo "=== Check if go-common is vendored ==="
find . -type d -name "go-common" | head -5
echo ""
echo "=== Look for tui.Ask implementation ==="
find . -path "*/go-common/tui*" -type f | head -10
echo ""
echo "=== Check go.sum for go-common version ==="
grep "go-common" go.sum | head -5
echo ""
echo "=== Compare Ask usage patterns across codebase ==="
echo "Ask calls with force guard:"
rg -B2 "if !force.*Ask" --type=go
echo ""
echo "Ask calls without force guard:"
rg -B2 -A1 "^\s*ready :=" --type=goLength of output: 912
I found evidence of the issue. Let me verify the external package behavior to be certain:
</script>🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about tui.Ask in the codebase
echo "=== Look for comments around Ask calls ==="
rg -B5 -A2 "tui.Ask" --type=go | head -80
echo ""
echo "=== Check if there's a --force or --yes flag for cluster new ==="
grep -A20 "clusterNewCmd.*Flags" cmd/cluster.go | head -30Length of output: 5104
🌐 Web query:
agentuity go-common tui.Ask non-TTY behavior
💡 Result:
Do you want me to fetch and read the tui.Ask source (ask.go) from agentuity/go-common on GitHub and explain exactly how it behaves when stdout/stdin is not a TTY?
Add tui.HasTTY guard before confirmation prompt or implement --force flag for non-interactive bypass.
The confirmation prompt at line 259 runs unconditionally in both TTY and non-TTY modes. In non-interactive environments (CI/automated pipelines), tui.Ask will block indefinitely or fail since stdin is not available.
Pattern mismatch identified:
- Cluster deletion (line 408) uses
if !force && !tui.Ask(...)with--forceflag available - Agent deletion and project deletion follow the same guarded pattern
- Version check (internal/util/version.go:67) guards with
if tui.HasTTY { ... tui.Ask(...) } - Cluster creation has no such guard and no
--forceflag alternative
Recommended fix: Either wrap the Ask call with if tui.HasTTY { ... } or add a --force / --yes flag to clusterNewCmd (consistent with clusterRemoveCmd).
🤖 Prompt for AI Agents
In cmd/cluster.go around lines 259 to 263, the confirmation prompt is called
unconditionally which blocks in non-TTY environments; add the same guarded
pattern used by cluster deletion: introduce a boolean --force/--yes flag on
clusterNewCmd, read it into a local variable, and replace the current
unconditional Ask with a conditional check such as: if !force { if !tui.HasTTY {
logger.Error("non-interactive shell: use --force to bypass confirmation");
os.Exit(1) } if !tui.Ask(logger, "Ready to create the cluster", true) {
logger.Info("Cluster creation cancelled"); os.Exit(0) } } to ensure interactive
prompts only run in TTY and CI can bypass with --force.
| // Format started time or use created time | ||
| startedTime := "" | ||
| if machine.StartedAt != nil { | ||
| startedTime = (*machine.StartedAt)[:10] // show date only | ||
| } else { | ||
| startedTime = machine.CreatedAt[:10] | ||
| } | ||
|
|
There was a problem hiding this comment.
Unsafe substring on timestamps
Slicing [:10] can panic if the string is shorter. Guard length or parse time safely.
- if machine.StartedAt != nil {
- startedTime = (*machine.StartedAt)[:10] // show date only
- } else {
- startedTime = machine.CreatedAt[:10]
- }
+ if machine.StartedAt != nil && len(*machine.StartedAt) >= 10 {
+ startedTime = (*machine.StartedAt)[:10]
+ } else if len(machine.CreatedAt) >= 10 {
+ startedTime = machine.CreatedAt[:10]
+ } else {
+ startedTime = machine.CreatedAt
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Format started time or use created time | |
| startedTime := "" | |
| if machine.StartedAt != nil { | |
| startedTime = (*machine.StartedAt)[:10] // show date only | |
| } else { | |
| startedTime = machine.CreatedAt[:10] | |
| } | |
| // Format started time or use created time | |
| startedTime := "" | |
| if machine.StartedAt != nil && len(*machine.StartedAt) >= 10 { | |
| startedTime = (*machine.StartedAt)[:10] | |
| } else if len(machine.CreatedAt) >= 10 { | |
| startedTime = machine.CreatedAt[:10] | |
| } else { | |
| startedTime = machine.CreatedAt | |
| } |
🤖 Prompt for AI Agents
In cmd/machine.go around lines 130 to 137, the code slices timestamp strings
with [:10] which can panic if the string is shorter; instead, ensure you safely
obtain the date by either (a) checking the string length before slicing (e.g.,
if len(s) >= 10 { s[:10] } else { s }), or (b) preferably parse the timestamp
into a time.Time (using time.Parse with the known layout) and then format it
with t.Format("2006-01-02") for a robust date string; apply this to both
machine.StartedAt (after nil check) and machine.CreatedAt.
| resp, err := infrastructure.CreateMachine(ctx, logger, urls.API, apikey, clusterID, provider, region) | ||
| if err != nil { | ||
| logger.Fatal("error creating machine: %s", err) | ||
| } | ||
| fmt.Printf("Machine created successfully with ID: %s and Token: %s\n", resp.ID, resp.Token) | ||
| }, |
There was a problem hiding this comment.
Token printed to stdout (potential secret leakage)
Printing join/provisioning tokens can leak via logs/shell history. Avoid by default; show only ID or gate behind a flag.
- fmt.Printf("Machine created successfully with ID: %s and Token: %s\n", resp.ID, resp.Token)
+ fmt.Printf("Machine created successfully with ID: %s\n", resp.ID)
+ // If exposing the token is needed, add a --show-token flag and print explicitly.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resp, err := infrastructure.CreateMachine(ctx, logger, urls.API, apikey, clusterID, provider, region) | |
| if err != nil { | |
| logger.Fatal("error creating machine: %s", err) | |
| } | |
| fmt.Printf("Machine created successfully with ID: %s and Token: %s\n", resp.ID, resp.Token) | |
| }, | |
| resp, err := infrastructure.CreateMachine(ctx, logger, urls.API, apikey, clusterID, provider, region) | |
| if err != nil { | |
| logger.Fatal("error creating machine: %s", err) | |
| } | |
| fmt.Printf("Machine created successfully with ID: %s\n", resp.ID) | |
| // If exposing the token is needed, add a --show-token flag and print explicitly. | |
| }, |
🤖 Prompt for AI Agents
In cmd/machine.go around lines 342 to 347, the code prints the provisioning
token to stdout which risks secret leakage; change the output to only show the
Machine ID by default and do not print the token, and if the token must be
exposed provide an explicit boolean flag (e.g. --show-token) added to the
command that defaults to false; implement the flag handling, conditionally print
the token only when the flag is true, and ensure other logging paths do not
inadvertently log the token (use ID-only output or secure logger for any
sensitive data).
| func confirmAction(canExecute bool) actionType { | ||
| oldState, err := term.MakeRaw(int(os.Stdin.Fd())) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| defer term.Restore(int(os.Stdin.Fd()), oldState) | ||
| ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGINT, syscall.SIGTERM) |
There was a problem hiding this comment.
Raw-mode on non-TTY and panic-on-error
term.MakeRaw() will fail on non‑TTY; panic here kills the CLI. Detect TTY and degrade gracefully.
-func confirmAction(canExecute bool) actionType {
- oldState, err := term.MakeRaw(int(os.Stdin.Fd()))
- if err != nil {
- panic(err)
- }
+func confirmAction(canExecute bool) actionType {
+ fd := int(os.Stdin.Fd())
+ if !term.IsTerminal(fd) {
+ // no raw mode possible; prefer manual/skip flow without interaction
+ if canExecute {
+ return manual
+ }
+ return skip
+ }
+ oldState, err := term.MakeRaw(fd)
+ if err != nil {
+ return cancelled
+ }
defer term.Restore(int(os.Stdin.Fd()), oldState)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func confirmAction(canExecute bool) actionType { | |
| oldState, err := term.MakeRaw(int(os.Stdin.Fd())) | |
| if err != nil { | |
| panic(err) | |
| } | |
| defer term.Restore(int(os.Stdin.Fd()), oldState) | |
| ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGINT, syscall.SIGTERM) | |
| func confirmAction(canExecute bool) actionType { | |
| fd := int(os.Stdin.Fd()) | |
| if !term.IsTerminal(fd) { | |
| // no raw mode possible; prefer manual/skip flow without interaction | |
| if canExecute { | |
| return manual | |
| } | |
| return skip | |
| } | |
| oldState, err := term.MakeRaw(fd) | |
| if err != nil { | |
| return cancelled | |
| } | |
| defer term.Restore(int(os.Stdin.Fd()), oldState) | |
| ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGINT, syscall.SIGTERM) |
🤖 Prompt for AI Agents
internal/infrastructure/tui.go lines 50-56: term.MakeRaw is called
unconditionally and panics on non-TTY; change to detect whether stdin is a
terminal first (using term.IsTerminal or similar on os.Stdin.Fd()) and only call
term.MakeRaw when true, capturing the returned state and setting up defer
restore only when MakeRaw succeeded; if stdin is not a TTY or MakeRaw returns an
error, do not panic—handle it gracefully by falling back to non-raw input
handling (e.g., simple buffered read or default action) and ensure any
signal.NotifyContext usage and cancel are still set up correctly.
| buf := make([]byte, 1) | ||
| os.Stdin.Read(buf) | ||
| fmt.Print("\x1b[2K\x1b[2K\r") // erase current line and move cursor to beginning | ||
| ch <- buf[0] | ||
| }() |
There was a problem hiding this comment.
stdin read error unchecked
If Read fails or returns 0 bytes, buf[0] is invalid data. Check n/error.
- buf := make([]byte, 1)
- os.Stdin.Read(buf)
+ buf := make([]byte, 1)
+ n, _ := os.Stdin.Read(buf)
fmt.Print("\x1b[2K\x1b[2K\r") // erase current line and move cursor to beginning
- ch <- buf[0]
+ if n > 0 {
+ ch <- buf[0]
+ } else {
+ ch <- 0
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| buf := make([]byte, 1) | |
| os.Stdin.Read(buf) | |
| fmt.Print("\x1b[2K\x1b[2K\r") // erase current line and move cursor to beginning | |
| ch <- buf[0] | |
| }() | |
| buf := make([]byte, 1) | |
| n, _ := os.Stdin.Read(buf) | |
| fmt.Print("\x1b[2K\x1b[2K\r") // erase current line and move cursor to beginning | |
| if n > 0 { | |
| ch <- buf[0] | |
| } else { | |
| ch <- 0 | |
| } | |
| }() |
🤖 Prompt for AI Agents
In internal/infrastructure/tui.go around lines 60-64, the goroutine reads into
buf but ignores the returned n and err; change the Read call to capture n,err :=
os.Stdin.Read(buf) and only use buf[0] when n == 1 and err == nil. On err or n
== 0, handle it explicitly (e.g. log/return, close the channel or send a
sentinel) instead of dereferencing invalid data, and preserve the existing
cleanup (the erase cursor print) only when appropriate.
| clipboard.Write(clipboard.FmtText, []byte(clipboardCmd)) | ||
| fmt.Println(v) |
There was a problem hiding this comment.
Guard clipboard writes when init failed
Avoid panic if clipboard is unavailable.
- clipboard.Write(clipboard.FmtText, []byte(clipboardCmd))
+ if clipReady {
+ clipboard.Write(clipboard.FmtText, []byte(clipboardCmd))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| clipboard.Write(clipboard.FmtText, []byte(clipboardCmd)) | |
| fmt.Println(v) | |
| if clipReady { | |
| clipboard.Write(clipboard.FmtText, []byte(clipboardCmd)) | |
| } | |
| fmt.Println(v) |
🤖 Prompt for AI Agents
internal/infrastructure/tui.go around lines 139-140: the code unconditionally
calls clipboard.Write which can panic if the clipboard is uninitialized or
unavailable; change it to attempt the write and handle the returned error (or
check an init flag) so clipboard failures are guarded — e.g. call
clipboard.Write and if it returns an error, swallow or log it and avoid calling
fmt.Println only if needed, ensuring no panic occurs when the clipboard is
unavailable.
| args, err := shellwords.Parse(strings.TrimSpace(string(newbuf))) | ||
| if err != nil { | ||
| return fmt.Errorf("error parsing edited command: %w", err) | ||
| } | ||
| return execAction(ctx, canExecute, instruction, help, args[0], args[1:], runner, success, skipFunc) | ||
| } |
There was a problem hiding this comment.
Possible index out of range if edited command is empty
If the user clears the file or leaves only whitespace, args[0] panics.
- args, err := shellwords.Parse(strings.TrimSpace(string(newbuf)))
+ args, err := shellwords.Parse(strings.TrimSpace(string(newbuf)))
if err != nil {
return fmt.Errorf("error parsing edited command: %w", err)
}
- return execAction(ctx, canExecute, instruction, help, args[0], args[1:], runner, success, skipFunc)
+ if len(args) == 0 {
+ return fmt.Errorf("error parsing edited command: no command provided")
+ }
+ return execAction(ctx, canExecute, instruction, help, args[0], args[1:], runner, success, skipFunc)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| args, err := shellwords.Parse(strings.TrimSpace(string(newbuf))) | |
| if err != nil { | |
| return fmt.Errorf("error parsing edited command: %w", err) | |
| } | |
| return execAction(ctx, canExecute, instruction, help, args[0], args[1:], runner, success, skipFunc) | |
| } | |
| args, err := shellwords.Parse(strings.TrimSpace(string(newbuf))) | |
| if err != nil { | |
| return fmt.Errorf("error parsing edited command: %w", err) | |
| } | |
| if len(args) == 0 { | |
| return fmt.Errorf("error parsing edited command: no command provided") | |
| } | |
| return execAction(ctx, canExecute, instruction, help, args[0], args[1:], runner, success, skipFunc) | |
| } |
🤖 Prompt for AI Agents
internal/infrastructure/tui.go around lines 198 to 203: the code calls args[0]
without validating args, which will panic if the edited file is empty or only
whitespace; fix by checking len(args) before indexing and return a clear error
(or treat as user-cancel) when args is empty, e.g. if len(args) == 0 { return
fmt.Errorf("no command provided") } and only then call execAction with args[0]
and args[1:].
| func buildCommandSequences(command string, args []string) []sequenceCommand { | ||
| // If using sh -c, don't parse for pipes - let the shell handle it | ||
| if command == "sh" && len(args) > 0 && args[0] == "-c" { | ||
| return []sequenceCommand{{command: command, args: args}} | ||
| } | ||
|
|
||
| var sequences []sequenceCommand | ||
| current := sequenceCommand{ | ||
| command: command, | ||
| } | ||
| for _, arg := range args { | ||
| if arg == "|" { | ||
| sequences = append(sequences, current) | ||
| current = sequenceCommand{} | ||
| } else if current.command == "" { | ||
| current.command = arg | ||
| } else { | ||
| current.args = append(current.args, arg) | ||
| } | ||
| } | ||
| if current.command != "" { | ||
| sequences = append(sequences, current) | ||
| } | ||
| return sequences | ||
| } |
There was a problem hiding this comment.
Pipeline parsing may not handle quoted arguments correctly.
The buildCommandSequences function splits on | but doesn't account for quoted strings that might contain pipe characters (e.g., echo "foo | bar" | grep foo). This could lead to incorrect command splitting.
Consider using a proper shell parser or documenting this limitation if complex pipelines are not expected.
🤖 Prompt for AI Agents
internal/infrastructure/util.go lines 19-43: the current split-on-arg=="|" logic
can mis-split when the input is a single raw command string containing quoted
pipes; update buildCommandSequences to be quote-aware by parsing the raw command
string (when args represent a single string or when you need to handle embedded
pipes) with a simple state machine that tracks single quotes, double quotes and
backslash escapes and only treats unquoted '|' as a pipeline separator, then
split each pipeline segment into command + args with a tokenizer that preserves
quoted tokens (or use a reliable shell parsing library) so pipes inside quotes
are not treated as separators.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
cmd/machine.go (2)
130-137: Unsafe substring on timestampsSlicing
[:10]can panic if the string is shorter than 10 characters. Guard the length or parse the timestamp safely.Apply this diff to fix:
- // Format started time or use created time - startedTime := "" - if machine.StartedAt != nil { - startedTime = (*machine.StartedAt)[:10] // show date only - } else { - startedTime = machine.CreatedAt[:10] - } + // Format started time or use created time + startedTime := "" + if machine.StartedAt != nil && len(*machine.StartedAt) >= 10 { + startedTime = (*machine.StartedAt)[:10] + } else if len(machine.CreatedAt) >= 10 { + startedTime = machine.CreatedAt[:10] + } else { + startedTime = machine.CreatedAt + }
346-346: Token printed to stdout (potential secret leakage)Printing the provisioning token can leak via logs or shell history. Avoid printing it by default.
Apply this diff to remove token from output:
- fmt.Printf("Machine created successfully with ID: %s and Token: %s\n", resp.ID, resp.Token) + tui.ShowSuccess("Machine %s created successfully", resp.ID) + // Token is securely stored server-side and will be used during provisioning
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/machine.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
cmd/*.go
📄 CodeRabbit inference engine (.cursor/rules/code-generation.mdc)
Ensure CLI commands build and test the pipeline (generation → SDK loading → agent usage) without writing into source SDK
Files:
cmd/machine.go
🧬 Code graph analysis (1)
cmd/machine.go (4)
internal/util/api.go (2)
EnsureLoggedIn(389-406)GetURLs(268-300)internal/infrastructure/infrastructure.go (8)
EnsureMachineClusteringEnabled(226-234)Machine(36-54)ListMachines(134-148)DeleteMachine(167-180)GetMachine(151-164)CreateMachine(242-271)Cluster(21-33)ListClusters(90-99)internal/errsystem/errorcodes.go (3)
ErrInvalidArgumentProvided(129-132)ErrApiRequest(21-24)ErrMissingRequiredArgument(133-136)internal/errsystem/errsystem.go (2)
WithContextMessage(120-124)WithUserMessage(70-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and Test (macos-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
cmd/machine.go (6)
1-18: LGTM!Imports are well-organized and appropriate for the machine management CLI commands.
20-30: LGTM!Root command setup is appropriate with hidden flag and help fallback.
154-200: LGTM!Machine removal command properly gates access, prompts for confirmation, and provides good user feedback.
202-295: LGTM!Machine status command provides comprehensive information with proper format support and nil-safe field handling.
350-376: LGTM!Command registration and flag setup follow standard Cobra patterns correctly.
344-344: Inconsistent error handling patternLine 344 uses
logger.Fatalwhile the rest of the file consistently uses theerrsystem.New().ShowErrorAndExit()pattern for error handling.Apply this diff to align with the file's error handling pattern:
- resp, err := infrastructure.CreateMachine(ctx, logger, urls.API, apikey, clusterID, provider, region) - if err != nil { - logger.Fatal("error creating machine: %s", err) - } + var resp *infrastructure.CreateMachineResponse + tui.ShowSpinner("Creating machine...", func() { + var err error + resp, err = infrastructure.CreateMachine(ctx, logger, urls.API, apikey, clusterID, provider, region) + if err != nil { + errsystem.New(errsystem.ErrApiRequest, err, errsystem.WithContextMessage("Failed to create machine")).ShowErrorAndExit() + } + })Likely an incorrect or invalid review comment.
|
|
||
| // Handle user cancellation (empty string) | ||
| if id == "" { | ||
| errsystem.New(errsystem.ErrApiRequest, fmt.Errorf("no cluster selected"), errsystem.WithUserMessage("No cluster selected")).ShowErrorAndExit() |
There was a problem hiding this comment.
Use appropriate error type for user cancellation
Using ErrApiRequest for user cancellation is semantically incorrect. This is a user input issue, not an API request failure.
Apply this diff:
- errsystem.New(errsystem.ErrApiRequest, fmt.Errorf("no cluster selected"), errsystem.WithUserMessage("No cluster selected")).ShowErrorAndExit()
+ errsystem.New(errsystem.ErrInvalidArgumentProvided, fmt.Errorf("no cluster selected"), errsystem.WithUserMessage("No cluster selected")).ShowErrorAndExit()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| errsystem.New(errsystem.ErrApiRequest, fmt.Errorf("no cluster selected"), errsystem.WithUserMessage("No cluster selected")).ShowErrorAndExit() | |
| errsystem.New(errsystem.ErrInvalidArgumentProvided, fmt.Errorf("no cluster selected"), errsystem.WithUserMessage("No cluster selected")).ShowErrorAndExit() |
🤖 Prompt for AI Agents
In cmd/machine.go around line 414, the code uses errsystem.ErrApiRequest for a
user cancellation case; replace ErrApiRequest with the semantically correct
error type (e.g., errsystem.ErrUserCancelled or the project's
ErrUserInput/ErrUserCanceled constant) when calling errsystem.New(...), keep the
fmt.Errorf("no cluster selected") and WithUserMessage("No cluster selected")
as-is, and adjust imports or constant names if necessary to compile.
| } | ||
|
|
||
| // This should never happen, but handle it as an impossible path | ||
| errsystem.New(errsystem.ErrApiRequest, fmt.Errorf("selected cluster not found: %s", id), errsystem.WithUserMessage("Selected cluster not found")).ShowErrorAndExit() |
There was a problem hiding this comment.
Use appropriate error type for internal validation error
Using ErrApiRequest for an internal state error (selected cluster not found in list) is semantically incorrect.
Apply this diff:
- errsystem.New(errsystem.ErrApiRequest, fmt.Errorf("selected cluster not found: %s", id), errsystem.WithUserMessage("Selected cluster not found")).ShowErrorAndExit()
+ errsystem.New(errsystem.ErrInvalidArgumentProvided, fmt.Errorf("selected cluster not found: %s", id), errsystem.WithUserMessage("Selected cluster not found")).ShowErrorAndExit()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| errsystem.New(errsystem.ErrApiRequest, fmt.Errorf("selected cluster not found: %s", id), errsystem.WithUserMessage("Selected cluster not found")).ShowErrorAndExit() | |
| errsystem.New(errsystem.ErrInvalidArgumentProvided, fmt.Errorf("selected cluster not found: %s", id), errsystem.WithUserMessage("Selected cluster not found")).ShowErrorAndExit() |
🤖 Prompt for AI Agents
In cmd/machine.go around line 425, the error is created with
errsystem.ErrApiRequest which is wrong for an internal validation/state error;
replace the error type with the appropriate internal validation error constant
from the errsystem package (for example errsystem.ErrValidation or
errsystem.ErrInternal if that is the project's chosen constant), keeping the
fmt.Errorf detail and the user message ("Selected cluster not found"); ensure
the call uses the correct errsystem constant and retains the WithUserMessage and
ShowErrorAndExit usage.
Summary by CodeRabbit
New Features
Improvements