refactor(cmd): reduce cyclomatic complexity below cyclop threshold#764
Merged
Conversation
✅ Deploy Preview for devsydev canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (42)
🚧 Files skipped from review as they are similar to previous changes (37)
📝 WalkthroughWalkthroughThis PR refactors CLI, Pro, workspace, agent-container, lifecycle, installation, rendering, and tooling flows by extracting validation, orchestration, state updates, output formatting, and cleanup into internal helper functions without changing exported APIs. ChangesCommand flow refactor
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
✅ Deploy Preview for images-devsy-sh canceled.
|
skevetter
force-pushed
the
refactor/cyclop-cmd
branch
2 times, most recently
from
July 26, 2026 18:46
aba3316 to
0a144f2
Compare
skevetter
marked this pull request as ready for review
July 26, 2026 22:48
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/pro/workspace/sleep.go (1)
86-91: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUnhandled
(nil, nil)result fromplatform.FindInstancein the sleep and wakeup flows. Both commands pass the lookup result straight into the new helper, which dereferences it, so a non-existent workspace panics instead of erroring out;cmd/pro/workspace/rebuild.goline 73 already guards this.
cmd/pro/workspace/sleep.go#L86-L91: return a "workspace not found in project" error whenworkspaceInstance == nilbefore callingcmd.sleep.cmd/pro/workspace/wakeup.go#L74-L79: add the same nil guard before callingcmd.wakeup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/pro/workspace/sleep.go` around lines 86 - 91, Guard the results of platform.FindInstance in the sleep flow at cmd/pro/workspace/sleep.go lines 86-91 and the wakeup flow at cmd/pro/workspace/wakeup.go lines 74-79: when workspaceInstance is nil, return the existing “workspace not found in project” error before calling cmd.sleep or cmd.wakeup; preserve the current error handling for lookup failures.
🧹 Nitpick comments (7)
cmd/machine/list.go (1)
72-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate machine-loading loop across plain/JSON renderers.
printMachinesPlainandprintMachinesJSONboth re-implement the same "iterate entries →LoadMachineConfig→ wrap error → sort by ID" logic, just diverging on the final shape. Extracting a shared loader avoids the two paths drifting apart later.♻️ Proposed refactor
+func loadMachines(devsyConfig *config.Config, entries []os.DirEntry) ([]*provider.Machine, error) { + machines := []*provider.Machine{} + for _, entry := range entries { + machineConfig, err := provider.LoadMachineConfig(devsyConfig.DefaultContext, entry.Name()) + if err != nil { + return nil, fmt.Errorf("load machine config: %w", err) + } + machines = append(machines, machineConfig) + } + sort.SliceStable(machines, func(i, j int) bool { + return machines[i].ID < machines[j].ID + }) + return machines, nil +} + func printMachinesPlain(devsyConfig *config.Config, entries []os.DirEntry) error { - tableEntries := [][]string{} - for _, entry := range entries { - machineConfig, err := provider.LoadMachineConfig( - devsyConfig.DefaultContext, - entry.Name(), - ) - if err != nil { - return fmt.Errorf("load machine config: %w", err) - } - - tableEntries = append(tableEntries, []string{ - machineConfig.ID, - machineConfig.Provider.Name, - time.Since(machineConfig.CreationTimestamp.Time).Round(1 * time.Second).String(), - }) + machines, err := loadMachines(devsyConfig, entries) + if err != nil { + return err } - sort.SliceStable(tableEntries, func(i, j int) bool { - return tableEntries[i][0] < tableEntries[j][0] - }) + tableEntries := [][]string{} + for _, m := range machines { + tableEntries = append(tableEntries, []string{ + m.ID, + m.Provider.Name, + time.Since(m.CreationTimestamp.Time).Round(1 * time.Second).String(), + }) + } table.Print([]string{"Name", "Provider", "Age"}, tableEntries) return nil } func printMachinesJSON(devsyConfig *config.Config, entries []os.DirEntry) error { - tableEntries := []*provider.Machine{} - for _, entry := range entries { - machineConfig, err := provider.LoadMachineConfig( - devsyConfig.DefaultContext, - entry.Name(), - ) - if err != nil { - return fmt.Errorf("load machine config: %w", err) - } - - tableEntries = append(tableEntries, machineConfig) + machines, err := loadMachines(devsyConfig, entries) + if err != nil { + return err } - sort.SliceStable(tableEntries, func(i, j int) bool { - return tableEntries[i].ID < tableEntries[j].ID - }) - out, err := json.Marshal(tableEntries) + out, err := json.Marshal(machines) if err != nil { return err } fmt.Print(string(out)) //nolint:forbidigo // CLI stdout output return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/machine/list.go` around lines 72 - 121, Extract the shared entry iteration, provider.LoadMachineConfig calls, error wrapping, and ID-based sorting from printMachinesPlain and printMachinesJSON into a helper returning []*provider.Machine. Update both renderers to use this helper, with printMachinesPlain only transforming the loaded machines into table rows and printMachinesJSON marshaling the returned machines.cmd/pro/list.go (1)
83-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate instance-iteration/login-check loop between
printPlainandprintJSON.Both helpers repeat "iterate
proInstances→ optionally callcheckLogin→ sort by host" before diverging into table rows vsproTableEntry. Consider extracting a shared builder (e.g., returning enriched instance + authenticated flag) that both renderers consume, so the login-check/sorting logic isn't duplicated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/pro/list.go` around lines 83 - 146, Extract the shared proInstances iteration, optional checkLogin handling, and host sorting from printPlain and printJSON into a common builder that returns each instance with its authentication state. Update both renderers to consume this shared result while preserving their existing plain-table and JSON-specific output behavior.cmd/pro/cluster/add.go (2)
296-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
--versioncan be appended twice.If both the server-reported
chartVersionandcmd.HelmChartVersionare set, two--versionflags are passed; helm silently takes the last one. Make the explicit flag win explicitly.♻️ Proposed refactor
- if chartVersion != "" { - helmArgs = append(helmArgs, "--version", chartVersion) - } - - if cmd.HelmChartVersion != "" { - helmArgs = append(helmArgs, "--version", cmd.HelmChartVersion) - } + if cmd.HelmChartVersion != "" { + helmArgs = append(helmArgs, "--version", cmd.HelmChartVersion) + } else if chartVersion != "" { + helmArgs = append(helmArgs, "--version", chartVersion) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/pro/cluster/add.go` around lines 296 - 302, Update the Helm argument construction around chartVersion and cmd.HelmChartVersion so only one --version flag is appended: use cmd.HelmChartVersion when provided, otherwise fall back to the server-reported chartVersion. Preserve the existing behavior when either value is unset, while ensuring the explicit command value wins without duplicate flags.
407-410: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMisleading error prefix when the helm install is what failed.
errors.Joinmerges the pod-readiness and helm errors, but everything surfaces aswait for pod: .... Wrap them separately so users can tell which step broke.♻️ Proposed refactor
- _, err := platform.WaitForPodReady(ctx, clientset, namespace) - if err = errors.Join(err, <-errChan); err != nil { - return fmt.Errorf("wait for pod: %w", err) - } + _, podErr := platform.WaitForPodReady(ctx, clientset, namespace) + if podErr != nil { + podErr = fmt.Errorf("wait for pod: %w", podErr) + } + if err := errors.Join(podErr, <-errChan); err != nil { + return err + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/pro/cluster/add.go` around lines 407 - 410, Update the error handling around platform.WaitForPodReady and the helm error received from errChan so each failure is wrapped with its own step-specific context before being combined or returned. Ensure helm installation failures are reported with a helm-install prefix rather than the misleading “wait for pod” prefix, while preserving both errors when they occur.cmd/internal/agentcontainer/daemon.go (1)
100-111: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider tightening the activity-file mode.
0o666makes the file world-writable (SASTworld-writable-chmod-go, CWE-276). If only the workspace user needs to touch it,0o664with the right group ownership would be safer. Ignore if multiple unrelated UIDs must update activity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/internal/agentcontainer/daemon.go` around lines 100 - 111, In the timeout setup within the daemon flow, tighten the activity file permissions by replacing the world-writable 0o666 mode used by os.WriteFile and os.Chmod with 0o664, provided the existing group ownership supports all required writers. Preserve the current activity-file creation and error handling behavior.Source: Linters/SAST tools
cmd/workspace/import.go (1)
274-289: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider tightening permissions on extracted import data.
extractExportDirnow centralizes the directory creation for both machine and provider imports with0o755(world-readable/executable), unlikeconfig.SaveConfig, which uses0o700/0o600for similarly sensitive data. The existing#nosec G301TODO is still open; now that the logic is consolidated in one helper, it's a good, low-cost opportunity to tighten this.🔒 Proposed tightened permissions
func extractExportDir(dir, data, label string) error { - // `#nosec` G301 -- TODO Consider using a more secure permission setting and ownership if needed. - if err := os.MkdirAll(dir, 0o755); err != nil { + if err := os.MkdirAll(dir, 0o700); err != nil { return fmt.Errorf("create %s dir: %w", label, err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/workspace/import.go` around lines 274 - 289, Update extractExportDir to create the export directory with restrictive owner-only permissions (0o700) instead of 0o755, and ensure extracted files are also not world-readable by applying 0o600 permissions after extraction. Remove or update the stale G301 TODO to reflect the tightened directory permission.cmd/internal/ssh_git_clone.go (1)
79-96: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a dial timeout to
runSSHSession.
ssh.DialusessshConfigfromgetConfig, which sets noTimeout. As aGIT_SSH_COMMANDdrop-in, this can hang indefinitely against an unreachable host with no way for callers (e.g., git clone during workspace setup) to recover.⏱️ Proposed fix
return &ssh.ClientConfig{ User: userName, Auth: []ssh.AuthMethod{ssh.PublicKeys(signers...)}, HostKeyCallback: ssh.InsecureIgnoreHostKey(), + Timeout: 30 * time.Second, }, nil🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/internal/ssh_git_clone.go` around lines 79 - 96, Update runSSHSession to enforce a finite connection timeout when dialing the SSH server, rather than relying on the timeout-less sshConfig from getConfig. Use the appropriate timeout-aware SSH dialing API or apply a defined timeout while preserving the existing error handling and session setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/pro/provider/create/workspace.go`:
- Line 74: Correct the typo in the error message returned by the workspace
unmarshalling path, changing “workpace” to “workspace” while preserving the
existing instanceEnv context and wrapped error in the surrounding function.
In `@cmd/pro/provider/watch/workspaces.go`:
- Around line 315-321: Update the locking flow around the duplicate check in the
relevant workspace watch method so every return path unlocks s.m, including when
the instance already exists in s.instances. Use deferred unlocking for the
critical section, then release the mutex before calling buildProInstance to
avoid holding it during construction.
In `@cmd/pro/start.go`:
- Around line 520-537: The NoOption path in the prompt flow does not perform the
re-check its label promises. Update the surrounding reachability/prompt logic
associated with YesOption and NoOption to repeat the DNS check when NoOption is
selected, preserving the existing port-forwarding behavior for YesOption;
alternatively, change NoOption’s text to accurately describe the current
false-return behavior.
- Around line 1196-1200: Update promptEnableIngress so omitting --host does not
immediately return false; preserve the interactive ingress prompt for existing
installations after cluster-type detection, and only use cmd.Host as the default
when it is provided. Ensure the prompt remains able to select disabling ingress
before returning the final value.
In `@cmd/pro/update_provider.go`:
- Around line 92-110: The len(splitted) == 0 guard in resolveNewProviderSource
cannot detect a provider source missing the @ separator. Validate that splitting
providerSource produces both repository and version components before
constructing the updated source, and return the existing error for invalid input
while preserving the normal version replacement behavior.
In `@cmd/pro/workspace/import.go`:
- Around line 377-382: Update parameterValueString to handle float64 values
produced by YAML unmarshalling, converting decimals to strings with
strconv.FormatFloat before parameters.VerifyValue is called in the import flow.
Preserve existing conversions and error behavior for all other types.
In `@hack/merge_mac_metadata/main.go`:
- Around line 70-75: Update the entry handling around the type assertion in the
metadata merge flow so non-map values are not silently discarded. Preserve each
unsupported entry in the output using the prior append behavior, or explicitly
reject it before writing output; keep the existing map-specific merge handling
unchanged.
---
Outside diff comments:
In `@cmd/pro/workspace/sleep.go`:
- Around line 86-91: Guard the results of platform.FindInstance in the sleep
flow at cmd/pro/workspace/sleep.go lines 86-91 and the wakeup flow at
cmd/pro/workspace/wakeup.go lines 74-79: when workspaceInstance is nil, return
the existing “workspace not found in project” error before calling cmd.sleep or
cmd.wakeup; preserve the current error handling for lookup failures.
---
Nitpick comments:
In `@cmd/internal/agentcontainer/daemon.go`:
- Around line 100-111: In the timeout setup within the daemon flow, tighten the
activity file permissions by replacing the world-writable 0o666 mode used by
os.WriteFile and os.Chmod with 0o664, provided the existing group ownership
supports all required writers. Preserve the current activity-file creation and
error handling behavior.
In `@cmd/internal/ssh_git_clone.go`:
- Around line 79-96: Update runSSHSession to enforce a finite connection timeout
when dialing the SSH server, rather than relying on the timeout-less sshConfig
from getConfig. Use the appropriate timeout-aware SSH dialing API or apply a
defined timeout while preserving the existing error handling and session setup.
In `@cmd/machine/list.go`:
- Around line 72-121: Extract the shared entry iteration,
provider.LoadMachineConfig calls, error wrapping, and ID-based sorting from
printMachinesPlain and printMachinesJSON into a helper returning
[]*provider.Machine. Update both renderers to use this helper, with
printMachinesPlain only transforming the loaded machines into table rows and
printMachinesJSON marshaling the returned machines.
In `@cmd/pro/cluster/add.go`:
- Around line 296-302: Update the Helm argument construction around chartVersion
and cmd.HelmChartVersion so only one --version flag is appended: use
cmd.HelmChartVersion when provided, otherwise fall back to the server-reported
chartVersion. Preserve the existing behavior when either value is unset, while
ensuring the explicit command value wins without duplicate flags.
- Around line 407-410: Update the error handling around platform.WaitForPodReady
and the helm error received from errChan so each failure is wrapped with its own
step-specific context before being combined or returned. Ensure helm
installation failures are reported with a helm-install prefix rather than the
misleading “wait for pod” prefix, while preserving both errors when they occur.
In `@cmd/pro/list.go`:
- Around line 83-146: Extract the shared proInstances iteration, optional
checkLogin handling, and host sorting from printPlain and printJSON into a
common builder that returns each instance with its authentication state. Update
both renderers to consume this shared result while preserving their existing
plain-table and JSON-specific output behavior.
In `@cmd/workspace/import.go`:
- Around line 274-289: Update extractExportDir to create the export directory
with restrictive owner-only permissions (0o700) instead of 0o755, and ensure
extracted files are also not world-readable by applying 0o600 permissions after
extraction. Remove or update the stale G301 TODO to reflect the tightened
directory permission.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ecfb39c-0336-4d95-8179-c4be0d30d41d
📒 Files selected for processing (42)
cmd/context/delete.gocmd/context/options.gocmd/ide/list.gocmd/ide/options.gocmd/internal/agentcontainer/credentials_server.gocmd/internal/agentcontainer/daemon.gocmd/internal/agentcontainer/setup.gocmd/internal/agentworkspace/build.gocmd/internal/check_provider_update.gocmd/internal/runusercommands.gocmd/internal/ssh_git_clone.gocmd/machine/list.gocmd/pro/cluster/add.gocmd/pro/daemon/netcheck.gocmd/pro/list.gocmd/pro/login.gocmd/pro/provider/create/workspace.gocmd/pro/provider/list/templates.gocmd/pro/provider/list/workspaces.gocmd/pro/provider/update/workspace.gocmd/pro/provider/watch/workspaces.gocmd/pro/start.gocmd/pro/update_provider.gocmd/pro/workspace/import.gocmd/pro/workspace/rebuild.gocmd/pro/workspace/sleep.gocmd/pro/workspace/wakeup.gocmd/provider/add.gocmd/workspace/build.gocmd/workspace/exec.gocmd/workspace/import.gocmd/workspace/list.gocmd/workspace/logs.gocmd/workspace/ssh.gocmd/workspace/stop.gocmd/workspace/troubleshoot.gocmd/workspace/up/up.gocmd/workspace/up/up_validate.goe2e/framework/server_utils.goe2e/tests/up/helper.gohack/merge_mac_metadata/main.gohack/pro/main.go
Comment on lines
+520
to
+537
| const ( | ||
| YesOption = "Yes" | ||
| NoOption = "No, re-run the DNS check" | ||
| ) | ||
|
|
||
| answer, err := log.QuestionDefault(&survey.QuestionOptions{ | ||
| Question: "Unable to reach Devsy at https://" + host + ". Do you want to start port-forwarding instead?", | ||
| DefaultValue: YesOption, | ||
| Options: []string{ | ||
| YesOption, | ||
| NoOption, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| return answer == YesOption, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
"No, re-run the DNS check" does not re-run anything.
Answering NoOption returns false, which falls straight through to successRemote — the reachability check is never repeated, so the option label misleads. Either loop on the check or reword the option.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/pro/start.go` around lines 520 - 537, The NoOption path in the prompt
flow does not perform the re-check its label promises. Update the surrounding
reachability/prompt logic associated with YesOption and NoOption to repeat the
DNS check when NoOption is selected, preserving the existing port-forwarding
behavior for YesOption; alternatively, change NoOption’s text to accurately
describe the current false-return behavior.
Comment on lines
+1196
to
+1200
| // Skip question if --host flag is provided | ||
| enableIngress := cmd.Host != "" | ||
| if !enableIngress { | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare with the pre-refactor inline logic
git log --oneline -3 -- cmd/pro/start.go
git diff HEAD~1 -- cmd/pro/start.go | sed -n '1,400p'Repository: devsy-org/devsy
Length of output: 10811
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cmd/pro/start.go promptEnableIngress section =="
sed -n '1180,1235p' cmd/pro/start.go | cat -n
echo
echo "== command line flag definitions for host =="
rg -n "Host|host" cmd -g '*.go' | sed -n '1,200p'
echo
echo "== deterministic call-flow check for handleAlreadyExistingInstallation =="
python3 - <<'PY'
from pathlib import Path
p = Path('cmd/pro/start.go')
s = p.read_text()
func = 'func (cmd *StartCmd) handleAlreadyExistingInstallation(ctx context.Context) error'
start = s.index(func)
end = s.index('\nfunc (cmd *StartCmd) promptEnableIngress', start)
body = s[start:end]
print(body)
for needle in ['!cmd.Upgrade && term.IsTerminal(os.Stdin)', 'if enableIngress', '!enableIngress', 'ensureHostAndIngress']:
print(f'{needle!r}:', needle in body)
# Extract the exact promptEnableIngress body for source-repro
func2 = 'func (cmd *StartCmd) promptEnableIngress(ctx context.Context) (bool, error)'
start2 = s.index(func2)
end2 = s.index('\nfunc (cmd *StartCmd) ', start2 + 1)
prompt = s[start2:end2]
print("== promptEnableIngress source ==")
print(prompt)
print("== promptEnableIngress predicates mentioning host/terminal/prompt ==")
for token in ['cmd.Host != ""', 'isLocal', 'isInstalledLocally', 'QuestionDefault', 'log.Info("Existing instance found."', 'confirmLocalCluster', 'confirmIngressOnLocal', 'ensureIngressController']:
print(f'{token!r}:', token in prompt)
PYRepository: devsy-org/devsy
Length of output: 18894
Keep the interactive ingress prompt enabled when --host is omitted
promptEnableIngress returns false immediately for existing installations unless cmd.Host != "", so the terminal path can no longer choose to disable ingress. The original flow still asked after detecting the cluster type; keep that prompt before returning based on an existing --host default.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/pro/start.go` around lines 1196 - 1200, Update promptEnableIngress so
omitting --host does not immediately return false; preserve the interactive
ingress prompt for existing installations after cluster-type detection, and only
use cmd.Host as the default when it is provided. Ensure the prompt remains able
to select disabling ingress before returning the final value.
Comment on lines
+92
to
+110
|
|
||
| func resolveNewProviderSource( | ||
| devsyConfig *config.Config, | ||
| providerName, newVersion string, | ||
| ) (string, error) { | ||
| providerSource, err := workspace.ResolveProviderSource( | ||
| devsyConfig, | ||
| providerName, | ||
| ) | ||
| if err != nil { | ||
| return "", fmt.Errorf("resolve provider source %s: %w", providerName, err) | ||
| } | ||
| splitted := strings.Split(providerSource, "@") | ||
| if len(splitted) == 0 { | ||
| return "", fmt.Errorf("no provider source found %s", providerSource) | ||
| } | ||
|
|
||
| return splitted[0] + "@" + newVersion, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
len(splitted) == 0 check is dead code — never validates missing @.
strings.Split never returns an empty slice; for a providerSource without @ it returns a 1-element slice, so this guard can never trigger. The check should ensure the split actually produced a repo and version part.
🐛 Proposed fix
splitted := strings.Split(providerSource, "@")
- if len(splitted) == 0 {
+ if len(splitted) < 2 {
return "", fmt.Errorf("no provider source found %s", providerSource)
}📝 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.
Suggested change
| func resolveNewProviderSource( | |
| devsyConfig *config.Config, | |
| providerName, newVersion string, | |
| ) (string, error) { | |
| providerSource, err := workspace.ResolveProviderSource( | |
| devsyConfig, | |
| providerName, | |
| ) | |
| if err != nil { | |
| return "", fmt.Errorf("resolve provider source %s: %w", providerName, err) | |
| } | |
| splitted := strings.Split(providerSource, "@") | |
| if len(splitted) == 0 { | |
| return "", fmt.Errorf("no provider source found %s", providerSource) | |
| } | |
| return splitted[0] + "@" + newVersion, nil | |
| } | |
| func resolveNewProviderSource( | |
| devsyConfig *config.Config, | |
| providerName, newVersion string, | |
| ) (string, error) { | |
| providerSource, err := workspace.ResolveProviderSource( | |
| devsyConfig, | |
| providerName, | |
| ) | |
| if err != nil { | |
| return "", fmt.Errorf("resolve provider source %s: %w", providerName, err) | |
| } | |
| splitted := strings.Split(providerSource, "@") | |
| if len(splitted) < 2 { | |
| return "", fmt.Errorf("no provider source found %s", providerSource) | |
| } | |
| return splitted[0] + "@" + newVersion, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/pro/update_provider.go` around lines 92 - 110, The len(splitted) == 0
guard in resolveNewProviderSource cannot detect a provider source missing the @
separator. Validate that splitting providerSource produces both repository and
version components before constructing the updated source, and return the
existing error for invalid input while preserving the normal version replacement
behavior.
Comment on lines
+70
to
+75
| // Defensive: electron-builder always emits maps; skip non-map entries | ||
| // (prior behavior appended raw entries via variadic spread). | ||
| entry, ok := e.(map[string]any) | ||
| if !ok { | ||
| continue | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Do not silently drop existing entry types.
Lines 70-75 change the documented prior behavior from appending non-map entries to discarding them. That can remove metadata during a merge. Preserve e for this branch, or reject it explicitly before writing output; silent deletion conflicts with this PR’s behavior-preservation objective.
Proposed compatibility fix
entry, ok := e.(map[string]any)
if !ok {
- continue
+ files = append(files, e)
+ continue
}📝 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.
Suggested change
| // Defensive: electron-builder always emits maps; skip non-map entries | |
| // (prior behavior appended raw entries via variadic spread). | |
| entry, ok := e.(map[string]any) | |
| if !ok { | |
| continue | |
| } | |
| // Defensive: electron-builder always emits maps; skip non-map entries | |
| // (prior behavior appended raw entries via variadic spread). | |
| entry, ok := e.(map[string]any) | |
| if !ok { | |
| files = append(files, e) | |
| continue | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hack/merge_mac_metadata/main.go` around lines 70 - 75, Update the entry
handling around the type assertion in the metadata merge flow so non-map values
are not silently discarded. Preserve each unsupported entry in the output using
the prior append behavior, or explicitly reject it before writing output; keep
the existing map-specific merge handling unchanged.
skevetter
marked this pull request as draft
July 26, 2026 23:00
skevetter
force-pushed
the
refactor/cyclop-cmd
branch
3 times, most recently
from
July 26, 2026 23:30
dc09d7a to
db51c02
Compare
Decompose functions in cmd/, e2e/, and hack/ that exceeded the golangci-lint cyclop max-complexity of 8 into well-named helpers (guard clauses, extracted predicates, param/result structs, switch/lookup maps), preserving behavior. Extracted helpers bundle args into structs to respect the repo's revive argument-limit/function-result-limit rules, keep unexported methods ordered after exported ones (funcorder), and annotate legitimate CLI stdout prints with //nolint:forbidigo.
skevetter
force-pushed
the
refactor/cyclop-cmd
branch
from
July 26, 2026 23:35
db51c02 to
18fee19
Compare
skevetter
marked this pull request as ready for review
July 27, 2026 00:14
skevetter
enabled auto-merge (squash)
July 27, 2026 00:21
skevetter
disabled auto-merge
July 27, 2026 00:23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Decomposes functions in
cmd/,e2e/, andhack/that exceeded the golangci-lintcyclopmax-complexity: 8into well-named helpers. Strictly behavior-preserving — only guard clauses, extracted predicates, and switch/lookup maps; no functional changes.Part 1 of 2 splitting a repo-wide cyclop cleanup (this PR: CLI + tooling layer; companion PR covers
pkg/).go build ./...passes andgolangci-lint --enable-only=cyclopreports zero issues across these paths.Summary by CodeRabbit