refactor(pkg): reduce cyclomatic complexity below cyclop threshold#765
Merged
Conversation
✅ Deploy Preview for images-devsy-sh canceled.
|
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (76)
📝 WalkthroughWalkthroughThis PR refactors repository subsystems by extracting validation, orchestration, persistence, parsing, networking, Kubernetes, workspace, IDE, and utility logic into focused unexported helpers while preserving existing exported APIs. ChangesRepository-wide helper extraction
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 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 devsydev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
skevetter
force-pushed
the
refactor/cyclop-pkg
branch
4 times, most recently
from
July 26, 2026 17:25
b132f15 to
0471ec9
Compare
skevetter
marked this pull request as ready for review
July 26, 2026 17:50
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/devcontainer/setup/setup.go (1)
695-733: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winStale SSH key cleanup condition is inverted — old keys are never removed.
writePlatformSSHKeysoverwrites indices0..keyCount-1, so the genuinely stale files are the ones withindex >= keyCount. The current check skips exactly those (continue) and instead removes the files that are about to be overwritten anyway (index < keyCount). Result: when the number of configured platform SSH keys shrinks, old unused private key files at higher indices are left behind indefinitely.🔒 Proposed fix: invert the skip condition
- if index >= keyCount { + if index < keyCount { 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 `@pkg/devcontainer/setup/setup.go` around lines 695 - 733, Correct the stale-file filtering in removeStalePlatformSSHKeys: skip files whose parsed index is less than keyCount, and remove files with index greater than or equal to keyCount. Preserve the existing filename filtering, parsing behavior, and warning handling.pkg/ts/workspace_server.go (1)
239-268: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTwo goroutines serve the same
runnerProxyListenerwith different muxes — requests can be routed to the wrong handler.The git-credentials and docker-credentials goroutines both call
serveMux(runnerProxyListener, mux, ...)on the identical listener, each with a mux that only knows its own path.http.Servedoesn't exclusively own a listener; each incoming connection is accepted by whichever goroutine'sAccept()wins the race, so a/docker-credentialsrequest landing on the git-only server (or vice versa) gets a 404.🐛 Proposed fix: register both handlers on one mux, serve once
// Setup HTTP handler for git credentials go func() { mux := http.NewServeMux() transport := &http.Transport{DialContext: s.tsServer.Dial} mux.HandleFunc("/git-credentials", func(w http.ResponseWriter, r *http.Request) { s.gitCredentialsHandler(w, r, lc, transport, projectName, workspaceName) }) - serveMux(runnerProxyListener, mux, "HTTP runner proxy server error: %v") - }() - - // Setup HTTP handler for docker credentials. - go func() { - mux := http.NewServeMux() - transport := &http.Transport{DialContext: s.tsServer.Dial} mux.HandleFunc("/docker-credentials", func(w http.ResponseWriter, r *http.Request) { s.dockerCredentialsHandler(w, r, lc, transport, projectName, workspaceName) }) + serveMux(runnerProxyListener, mux, "HTTP runner proxy server error: %v") }()🤖 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 `@pkg/ts/workspace_server.go` around lines 239 - 268, Combine the git-credentials and docker-credentials routes into a single mux and serve it once on runnerProxyListener. Keep each route bound to its existing handler and transport setup, but remove the two competing serveMux goroutines so requests are dispatched by path reliably.
🧹 Nitpick comments (12)
pkg/driver/kubernetes/init_container.go (1)
116-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo identical container-merge helpers were extracted instead of one.
mergeInitContainerandmergeDevsyContainerapply the same rules (Env prepend, EnvFrom/Ports/ImagePullPolicy adoption, VolumeMounts prepend, SecurityContext fallback) to*corev1.Container; the shared root cause is duplicated merge logic that will now drift independently.
pkg/driver/kubernetes/init_container.go#L116-L131: replacemergeInitContainerwith a single sharedmergeContainer(dst, src *corev1.Container)helper.pkg/driver/kubernetes/run.go#L582-L598: dropmergeDevsyContainerand call the shared helper fromgetContainers.🤖 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 `@pkg/driver/kubernetes/init_container.go` around lines 116 - 131, Replace mergeInitContainer in pkg/driver/kubernetes/init_container.go:116-131 with the shared mergeContainer(dst, src *corev1.Container) helper, preserving the existing merge rules. In pkg/driver/kubernetes/run.go:582-598, remove mergeDevsyContainer and update getContainers to call mergeContainer instead.pkg/driver/kubernetes/run.go (1)
318-329: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
capshadows the predeclared builtin.Rename the loop variable (e.g.
c) to avoid shadowingcap; some linters (predeclared,revive) flag this.🤖 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 `@pkg/driver/kubernetes/run.go` around lines 318 - 329, Rename the loop variable in buildCapabilities from cap to a non-shadowing name such as c, and update its use when appending to capabilities.Add; preserve the existing capability conversion and return behavior.pkg/driver/kubernetes/driver.go (1)
241-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
secretExistsprobe; consider aligning the pull-secret flag check.
DeleteSecretalready short-circuits viasecretExists(pkg/driver/kubernetes/pullsecrets.go Line 103), so the guard on Line 243 adds a second API GET. Also,KubernetesPullSecretsEnabled != ""here differs from== pkgconfig.BoolTrueused inensurePullSecrets(pkg/driver/kubernetes/run.go Line 422); harmless today since delete is a no-op, but the divergence is easy to misread.♻️ Proposed simplification
func (k *KubernetesDriver) deleteWorkspaceSecrets(ctx context.Context, workspaceId string) error { // delete daemon config secret - if k.secretExists(ctx, getDaemonSecretName(workspaceId)) { - log.Infof("Delete daemon config secret %q", workspaceId) - if err := k.DeleteSecret(ctx, getDaemonSecretName(workspaceId)); err != nil { - return err - } - } + log.Infof("Delete daemon config secret %q", workspaceId) + if err := k.DeleteSecret(ctx, getDaemonSecretName(workspaceId)); 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 `@pkg/driver/kubernetes/driver.go` around lines 241 - 259, Update deleteWorkspaceSecrets to remove the redundant secretExists guard and call DeleteSecret directly for the daemon secret, relying on its existing no-op behavior. Align the pull-secret condition with ensurePullSecrets by checking KubernetesPullSecretsEnabled against pkgconfig.BoolTrue, while preserving the current error propagation.pkg/options/resolve_test.go (2)
664-665: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark the new test helpers with
t.Helper().Without it,
assert.*failures are attributed torunResolveTestCase/assertResolvedOptionsrather than the caller.♻️ Proposed change
func runResolveTestCase(t *testing.T, tc testCase) { + t.Helper() r := resolver.New(tc.UserValues, tc.ExtraValues, buildResolverOpts(tc)...)) { + t.Helper() strOptions := map[string]string{}Also applies to: 699-704
🤖 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 `@pkg/options/resolve_test.go` around lines 664 - 665, Mark the test helper functions runResolveTestCase and assertResolvedOptions with t.Helper() at their start so assertion failures are attributed to the calling test.
659-662: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
t.Runsubtests now that each case is a function call.Failures currently report only the helper's line;
tc.Nameis passed to some assertions but not all (assertResolvedOptionsdoesn't include it).♻️ Proposed change
for _, tc := range testCases { - runResolveTestCase(t, tc) + t.Run(tc.Name, func(t *testing.T) { + runResolveTestCase(t, tc) + }) }🤖 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 `@pkg/options/resolve_test.go` around lines 659 - 662, Update the test-case loop in the resolve tests to wrap each runResolveTestCase invocation in t.Run using tc.Name as the subtest name, so failures are attributed to the specific case even when assertions such as assertResolvedOptions omit the name.pkg/util/homedir.go (1)
52-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNamed results on
homeFromPlatformare never used.Every path returns explicitly, and the inner
case "windows"shadowshome/err. Either drop the names or documentdonein the signature only via the comment (already present).🤖 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 `@pkg/util/homedir.go` around lines 52 - 67, Remove the unused named return values from homeFromPlatform and give the function an unnamed return signature, preserving all existing explicit return paths and behavior.pkg/options/resolver/util.go (1)
126-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
variableDependencyParamsduplicatesvariableDependenciesParamsexcept fordeps/dep.Could collapse by passing
(p variableDependenciesParams, dep string)tovalidateVariableDependencyinstead of a second near-identical struct.🤖 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 `@pkg/options/resolver/util.go` around lines 126 - 162, Remove the duplicate variableDependencyParams type and change validateVariableDependency to accept variableDependenciesParams plus a dep string. Update addVariableDependencies to pass the existing p and current dep while preserving all validation behavior.pkg/options/resolve.go (1)
443-448: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNil
providerValuesnow takes a different branch.When
providerValuesis nil, this guard is skipped entirely and the option can be deleted purely onGlobal, whereas a plain lookup on a nil map would report!okand keep it. Current callers passdevConfig.ProviderOptions(...)(never nil), so this is latent only — dropping the nil check makes the semantics uniform.♻️ Simplify to a plain lookup
- if p.providerValues != nil { - if _, ok := p.providerValues[p.k]; !ok { - return false - } + if _, ok := p.providerValues[p.k]; !ok { + return false }🤖 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 `@pkg/options/resolve.go` around lines 443 - 448, Update the provider-value check in the option filtering logic to use a plain lookup on p.providerValues without guarding against nil, so nil maps produce !ok and preserve the option. Keep the existing p.k lookup and return behavior unchanged.pkg/platform/deploy.go (1)
177-183: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPlumb the caller's context into the log fetch instead of
context.Background().
loftPodFailureErroris reached from aPollUntilContextTimeoutcallback; usingcontext.Background()forGetLogsmeans this call ignores cancellation/timeout and can block the wait loop.🤖 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 `@pkg/platform/deploy.go` around lines 177 - 183, Update the GetLogs call in loftPodFailureError to use the caller-provided context instead of context.Background(), preserving cancellation and timeout propagation from the PollUntilContextTimeout callback.pkg/platform/kubeconfig.go (1)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLocal
kubeidentifier shadows the newly importedkubepackage.The file now imports
github.com/devsy-org/devsy/pkg/platform/kube(line 14) forkube.Interface. Renaming these tok8sOptsavoids future breakage if the package is referenced in these scopes.Also applies to: 83-87
🤖 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 `@pkg/platform/kubeconfig.go` at line 41, Rename the local Kubernetes options identifier currently declared as kube in the affected scopes to k8sOpts, including its references around the later usage, so it no longer shadows the imported kube package. Keep the existing behavior unchanged.pkg/credentials/server.go (1)
93-102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn 404 for unmapped paths.
Returning without writing yields an empty
200 OK, which is indistinguishable from a successful empty credential response for clients.♻️ Proposed change
handler, ok := routes[request.URL.Path] if !ok { + http.NotFound(writer, request) return }🤖 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 `@pkg/credentials/server.go` around lines 93 - 102, Update the route lookup in the HTTP handler returned by the server handler function so unmapped request paths write an HTTP 404 response before returning; preserve the existing handler invocation and 500 error behavior for mapped routes.pkg/client/clientimplementation/daemonclient/delete.go (1)
68-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog invalid grace-period values instead of silently dropping them.
A malformed
--grace-periodsilently degrades to "no timeout" with no signal to the user.♻️ Proposed change
duration, err := time.ParseDuration(raw) if err != nil { + log.Warnf("Invalid grace period %q, ignoring: %v", raw, err) 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 `@pkg/client/clientimplementation/daemonclient/delete.go` around lines 68 - 78, Update parseGracePeriod to log invalid non-empty grace-period values when time.ParseDuration fails, while preserving the nil return for malformed input and empty strings. Use the existing logging mechanism available in the surrounding delete-client code rather than silently discarding the parse error.
🤖 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 `@pkg/agent/tunnelserver/tunnelserver.go`:
- Around line 569-583: Update workspaceIgnoreExcludes to close the file opened
by os.Open after ignorefile.ReadAll completes, including when reading fails,
while preserving the existing excludes and warning behavior.
In `@pkg/client/clientimplementation/daemonclient/client.go`:
- Around line 327-349: Update workspaceUnreachableError to handle a nil instance
before accessing instance.Status, including the (nil, nil) result from
LocalClient.GetWorkspace in the unreachable-host path. Return an appropriate
workspace retrieval error for nil instances, while preserving the existing
getWorkspaceErr, status checks, and reachability error behavior.
In `@pkg/client/clientimplementation/daemonclient/form.go`:
- Around line 273-302: Update fieldParametersWithValues to avoid direct string
assertions on values returned by getDeepValue. Preserve string values as-is,
safely convert native persisted scalar types such as int and bool before
assigning StringValue, and ensure boolean parsing only operates on a valid
string representation so interactive updates cannot panic.
In `@pkg/devcontainer/config/config.go`:
- Around line 590-612: The applyMountField function indexes splitted2[1] without
validating that a value exists, causing malformed mount fields to panic. Check
for a missing value before handling recognized keys, and preserve or reject the
original malformed token consistently instead of assigning fields from it; valid
key-value parsing must remain unchanged.
- Around line 626-650: The unmarshalMountObject method currently panics when an
element of other is not a string. Change it to return an error, propagate
types.ErrUnsupportedType when any otherInterface element fails string
validation, and update its caller to return that error while preserving
successful string conversion.
In `@pkg/devcontainer/prebuild.go`:
- Around line 137-140: Update the image reference construction in the prebuild
image tagging flow to remove only the final tag suffix, preserving registry
ports and repository path components. Replace the current first-colon split used
for imageRepoName with logic that derives the repository from the last tag
separator before appending each tag.
In `@pkg/gitsshsigning/helper.go`:
- Around line 178-186: Update removeSignatureHelper to strip both carriage
return and newline characters from each line before passing it to
gitConfigFilter.process, preserving clean output for CRLF-terminated
configuration files while leaving other processing unchanged.
In `@pkg/platform/deploy.go`:
- Around line 205-212: Fix the format argument mapping in the error construction
around the return statement so the output uses pkgconfig.ProductNamePro, out,
message, and reason exactly once in their intended placeholders. Use explicit
argument indexes consistently for every format verb, preserving the existing
message text and support URL.
In `@pkg/platform/form/form.go`:
- Around line 288-301: Update assignFieldValue to replace both unchecked
value.(string) assertions with comma-ok type assertions. For boolean parameters,
parse only when the value is a string; for the general path, assign StringValue
only when the value is a string, otherwise preserve the existing default-value
behavior without panicking.
In `@pkg/platform/instance.go`:
- Around line 274-288: Guard waitForInstanceTimeoutError against a nil
updatedInstance before accessing readiness, template-sync, or Status.Conditions
fields. Update both affected call paths around PollUntilContextTimeout so an
already-cancelled context with no poll result returns a safe timeout error
without dereferencing nil.
In `@pkg/platform/kubeconfig.go`:
- Around line 329-331: Update the error wrapping in the GetKubeConfig branch to
use the message “get virtual cluster kube config: %w” instead of identifying it
as direct cluster endpoint token creation. Keep the existing error propagation
unchanged so the ingress and direct endpoint paths remain distinguishable.
In `@pkg/telemetry/collect.go`:
- Around line 185-207: Cache the result of d.client.WorkspaceConfig() in
buildEventProperties before checking it, then reuse that single value for
source_type and ide. Preserve the existing provider and event-property behavior
while ensuring the cached configuration is nil-checked before field access.
In `@pkg/util/homedir.go`:
- Around line 108-113: Update the passwd parsing in UserHomeDir so an empty
passwdParts[5] is treated as unavailable rather than authoritative: only return
success when the home field is non-empty, otherwise continue to the existing
shell-based fallback. Preserve the current behavior for valid home fields and
malformed entries.
---
Outside diff comments:
In `@pkg/devcontainer/setup/setup.go`:
- Around line 695-733: Correct the stale-file filtering in
removeStalePlatformSSHKeys: skip files whose parsed index is less than keyCount,
and remove files with index greater than or equal to keyCount. Preserve the
existing filename filtering, parsing behavior, and warning handling.
In `@pkg/ts/workspace_server.go`:
- Around line 239-268: Combine the git-credentials and docker-credentials routes
into a single mux and serve it once on runnerProxyListener. Keep each route
bound to its existing handler and transport setup, but remove the two competing
serveMux goroutines so requests are dispatched by path reliably.
---
Nitpick comments:
In `@pkg/client/clientimplementation/daemonclient/delete.go`:
- Around line 68-78: Update parseGracePeriod to log invalid non-empty
grace-period values when time.ParseDuration fails, while preserving the nil
return for malformed input and empty strings. Use the existing logging mechanism
available in the surrounding delete-client code rather than silently discarding
the parse error.
In `@pkg/credentials/server.go`:
- Around line 93-102: Update the route lookup in the HTTP handler returned by
the server handler function so unmapped request paths write an HTTP 404 response
before returning; preserve the existing handler invocation and 500 error
behavior for mapped routes.
In `@pkg/driver/kubernetes/driver.go`:
- Around line 241-259: Update deleteWorkspaceSecrets to remove the redundant
secretExists guard and call DeleteSecret directly for the daemon secret, relying
on its existing no-op behavior. Align the pull-secret condition with
ensurePullSecrets by checking KubernetesPullSecretsEnabled against
pkgconfig.BoolTrue, while preserving the current error propagation.
In `@pkg/driver/kubernetes/init_container.go`:
- Around line 116-131: Replace mergeInitContainer in
pkg/driver/kubernetes/init_container.go:116-131 with the shared
mergeContainer(dst, src *corev1.Container) helper, preserving the existing merge
rules. In pkg/driver/kubernetes/run.go:582-598, remove mergeDevsyContainer and
update getContainers to call mergeContainer instead.
In `@pkg/driver/kubernetes/run.go`:
- Around line 318-329: Rename the loop variable in buildCapabilities from cap to
a non-shadowing name such as c, and update its use when appending to
capabilities.Add; preserve the existing capability conversion and return
behavior.
In `@pkg/options/resolve_test.go`:
- Around line 664-665: Mark the test helper functions runResolveTestCase and
assertResolvedOptions with t.Helper() at their start so assertion failures are
attributed to the calling test.
- Around line 659-662: Update the test-case loop in the resolve tests to wrap
each runResolveTestCase invocation in t.Run using tc.Name as the subtest name,
so failures are attributed to the specific case even when assertions such as
assertResolvedOptions omit the name.
In `@pkg/options/resolve.go`:
- Around line 443-448: Update the provider-value check in the option filtering
logic to use a plain lookup on p.providerValues without guarding against nil, so
nil maps produce !ok and preserve the option. Keep the existing p.k lookup and
return behavior unchanged.
In `@pkg/options/resolver/util.go`:
- Around line 126-162: Remove the duplicate variableDependencyParams type and
change validateVariableDependency to accept variableDependenciesParams plus a
dep string. Update addVariableDependencies to pass the existing p and current
dep while preserving all validation behavior.
In `@pkg/platform/deploy.go`:
- Around line 177-183: Update the GetLogs call in loftPodFailureError to use the
caller-provided context instead of context.Background(), preserving cancellation
and timeout propagation from the PollUntilContextTimeout callback.
In `@pkg/platform/kubeconfig.go`:
- Line 41: Rename the local Kubernetes options identifier currently declared as
kube in the affected scopes to k8sOpts, including its references around the
later usage, so it no longer shadows the imported kube package. Keep the
existing behavior unchanged.
In `@pkg/util/homedir.go`:
- Around line 52-67: Remove the unused named return values from homeFromPlatform
and give the function an unnamed return signature, preserving all existing
explicit return paths and behavior.
🪄 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: 2a350102-cf90-40c9-b81f-c85703980a67
📒 Files selected for processing (76)
pkg/agent/agent.gopkg/agent/binary.gopkg/agent/delivery/factory.gopkg/agent/tunnelserver/tunnelserver.gopkg/client/clientimplementation/daemonclient/client.gopkg/client/clientimplementation/daemonclient/delete.gopkg/client/clientimplementation/daemonclient/form.gopkg/client/clientimplementation/daemonclient/up.gopkg/command/background.gopkg/config/config.gopkg/copy/copy.gopkg/credentials/server.gopkg/daemon/agent/daemon.gopkg/daemon/platform/workspace_watcher.gopkg/devcontainer/build/options.gopkg/devcontainer/buildkit/buildkit.gopkg/devcontainer/buildkit/cache.gopkg/devcontainer/buildkit/remote.gopkg/devcontainer/config/config.gopkg/devcontainer/config/merge.gopkg/devcontainer/config/parse_test.gopkg/devcontainer/config/result.gopkg/devcontainer/config/substitute.gopkg/devcontainer/feature/extend.gopkg/devcontainer/feature/features.gopkg/devcontainer/prebuild.gopkg/devcontainer/setup/setup.gopkg/devcontainer/single.gopkg/dockercredentials/dockercredentials.gopkg/dockerfile/parse.gopkg/driver/kubernetes/driver.gopkg/driver/kubernetes/init_container.gopkg/driver/kubernetes/pullsecrets.gopkg/driver/kubernetes/pvc.gopkg/driver/kubernetes/run.gopkg/driver/kubernetes/serviceaccount.gopkg/driver/kubernetes/wait.gopkg/extract/compress.gopkg/extract/extract.gopkg/gitsshsigning/helper.gopkg/ide/fleet/fleet.gopkg/ide/ideparse/parse.gopkg/ide/jetbrains/generic.gopkg/ide/rstudio/rstudio.gopkg/ide/vscode/vscode.gopkg/inject/inject.gopkg/netstat/netstat_util.gopkg/netstat/watcher.gopkg/options/resolve.gopkg/options/resolve_test.gopkg/options/resolver/parse.gopkg/options/resolver/resolve.gopkg/options/resolver/resolver.gopkg/options/resolver/util.gopkg/platform/client/client.gopkg/platform/deploy.gopkg/platform/form/form.gopkg/platform/instance.gopkg/platform/kubeconfig.gopkg/platform/owner.gopkg/platform/parameters/parameters.gopkg/platform/remotecommand/client.gopkg/provider/env.gopkg/provider/parse.gopkg/shell/shell.gopkg/ssh/config.gopkg/ssh/server/ssh.gopkg/telemetry/collect.gopkg/ts/workspace_server.gopkg/tunnel/browser.gopkg/tunnel/forwarder.gopkg/types/types.gopkg/util/homedir.gopkg/workspace/exec.gopkg/workspace/list.gopkg/workspace/workspace.go
skevetter
marked this pull request as draft
July 26, 2026 18:10
skevetter
force-pushed
the
refactor/cyclop-pkg
branch
5 times, most recently
from
July 26, 2026 20:51
9af634c to
1d341ab
Compare
skevetter
marked this pull request as ready for review
July 26, 2026 21:26
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/platform/form/form.go (1)
489-505: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRoute unknown AppParameter types to the string field in
parameterFields.Unlike the daemonclient path,
pkg/platform/form/parameterFieldsusesparam.Typedirectly and falls through when the value is unsupported, leavingfield niland appending it to the huh group. Add adefaultcase that callsstringParameterField(param, title)for unknown/empty types.🤖 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 `@pkg/platform/form/form.go` around lines 489 - 505, Update the type switch in parameterFields so its default branch routes unknown or empty param.Type values through stringParameterField(param, title), ensuring fields never remain nil while preserving the existing handling for supported types.
♻️ Duplicate comments (1)
pkg/platform/form/form.go (1)
254-310: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNon-string existing values now silently fall back to defaults.
The panic risk flagged previously is fixed, but for non-boolean parameters a YAML-decoded number (
float64) or nested value still discards the user's saved value and reverts toDefaultValue. Fortype: numberparameters stored as YAML scalars this is a visible regression in the update form.🛠️ Proposed fix
if s, ok := value.(string); ok { fieldParameter.StringValue = s + } else if value != nil { + fieldParameter.StringValue = fmt.Sprintf("%v", value) } else { fieldParameter.StringValue = fieldParameter.DefaultValue }🤖 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 `@pkg/platform/form/form.go` around lines 254 - 310, Update assignFieldValue so non-boolean existing values, including YAML-decoded numbers and nested scalar values, are preserved in the field parameter instead of falling back to DefaultValue. Convert supported scalar values to the string representation expected by StringValue, while retaining the existing default fallback only for values that cannot be represented.
🧹 Nitpick comments (4)
pkg/driver/kubernetes/driver.go (2)
210-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
ptr.To(int64(5))over the&[]int64{5}[0]idiom.
k8s.io/utils/ptris already used elsewhere in this package (e.g.finalizePodSpecin pkg/driver/kubernetes/run.go).♻️ Proposed tweak
Delete(ctx, workspaceId, metav1.DeleteOptions{ - GracePeriodSeconds: &[]int64{5}[0], + GracePeriodSeconds: ptr.To(int64(5)), })🤖 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 `@pkg/driver/kubernetes/driver.go` around lines 210 - 222, Replace the `&[]int64{5}[0]` expression in the PVC deletion flow with `ptr.To(int64(5))`, adding the existing `k8s.io/utils/ptr` import if needed. Keep the `GracePeriodSeconds` value and the surrounding `Delete` error handling unchanged.
241-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant existence check and inconsistent
KubernetesPullSecretsEnabledcomparison.
DeleteSecret(pkg/driver/kubernetes/pullsecrets.go:99-116) already short-circuits viasecretExists, so the guard on Line 243 doubles the API call. Also, creation uses== pkgconfig.BoolTrue(ensurePullSecretsin pkg/driver/kubernetes/run.go), while deletion uses!= "", so an explicit"false"still triggers a pull-secret lookup. Deletion is idempotent so behavior is safe, but the comparisons should match.♻️ Proposed simplification
func (k *KubernetesDriver) deleteWorkspaceSecrets(ctx context.Context, workspaceId string) error { // delete daemon config secret - if k.secretExists(ctx, getDaemonSecretName(workspaceId)) { - log.Infof("Delete daemon config secret %q", workspaceId) - if err := k.DeleteSecret(ctx, getDaemonSecretName(workspaceId)); err != nil { - return err - } - } + log.Infof("Delete daemon config secret %q", workspaceId) + if err := k.DeleteSecret(ctx, getDaemonSecretName(workspaceId)); err != nil { + return err + } // delete pull secret - if k.options.KubernetesPullSecretsEnabled != "" { + if k.options.KubernetesPullSecretsEnabled == pkgconfig.BoolTrue { log.Infof("Delete pull secret %q", workspaceId) if err := k.DeleteSecret(ctx, getPullSecretsName(workspaceId)); err != nil { return err } }Note: if pull secrets could have been created under an older option value, keeping the permissive delete check is intentional — confirm before tightening.
🤖 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 `@pkg/driver/kubernetes/driver.go` around lines 241 - 256, Update deleteWorkspaceSecrets to rely directly on DeleteSecret for the daemon secret instead of calling secretExists first, removing the redundant lookup while preserving error propagation. Align the KubernetesPullSecretsEnabled condition with ensurePullSecrets by using the same explicit enabled-value comparison, unless backward compatibility requires retaining the permissive deletion behavior.pkg/driver/kubernetes/run.go (1)
104-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck for an existing pod before building and ensuring Kubernetes resources.
runContainercallsbuildPodfirst, andbuildPodcreates/refreshes the service account bindings, daemon config secret, and pull secrets beforereconcileExistingPodcan returntrue. If a skipped run should not mutate cluster resources, move the reconcile check before theseensure*calls or split the pure pod spec build from the cluster-resource preparation.🤖 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 `@pkg/driver/kubernetes/run.go` around lines 104 - 189, Update runContainer to call reconcileExistingPod before buildPod, returning immediately when skip is true so skipped runs do not create or refresh Kubernetes resources through buildPod’s ensureServiceAccount, ensureDaemonConfig, and ensurePullSecrets calls. Preserve error propagation, and only build and run the pod when reconciliation does not skip.pkg/options/resolver/util.go (1)
98-124: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard against a nil
optionfor symmetry.
validateChildDependency/validateVariableDependencydefensively handlechildOption == nil/depOption == nil, butoption(fromg.GetNode(optionName)inaddDependency) is dereferenced without the same guard. Nodes can hold nil values (e.g.rootIDis added withnilat line 182), so an earlyif option == nil { return nil }inaddDependencywould keep the invariants consistent.🤖 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 `@pkg/options/resolver/util.go` around lines 98 - 124, Add an early nil check for the parent option in addDependency before it calls validateChildDependency or validateVariableDependency; return nil when option is nil. Preserve the existing dependency validation behavior for non-nil options and avoid dereferencing option before this guard.
🤖 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 `@pkg/driver/kubernetes/run.go`:
- Around line 450-457: Update the existingOptions parsing near
DevsyLastAppliedAnnotation to first read and validate the annotation value,
calling json.Unmarshal only when it is non-empty. Skip parsing and error logging
for missing annotations while preserving the existing error log for invalid
non-empty annotation data.
---
Outside diff comments:
In `@pkg/platform/form/form.go`:
- Around line 489-505: Update the type switch in parameterFields so its default
branch routes unknown or empty param.Type values through
stringParameterField(param, title), ensuring fields never remain nil while
preserving the existing handling for supported types.
---
Duplicate comments:
In `@pkg/platform/form/form.go`:
- Around line 254-310: Update assignFieldValue so non-boolean existing values,
including YAML-decoded numbers and nested scalar values, are preserved in the
field parameter instead of falling back to DefaultValue. Convert supported
scalar values to the string representation expected by StringValue, while
retaining the existing default fallback only for values that cannot be
represented.
---
Nitpick comments:
In `@pkg/driver/kubernetes/driver.go`:
- Around line 210-222: Replace the `&[]int64{5}[0]` expression in the PVC
deletion flow with `ptr.To(int64(5))`, adding the existing `k8s.io/utils/ptr`
import if needed. Keep the `GracePeriodSeconds` value and the surrounding
`Delete` error handling unchanged.
- Around line 241-256: Update deleteWorkspaceSecrets to rely directly on
DeleteSecret for the daemon secret instead of calling secretExists first,
removing the redundant lookup while preserving error propagation. Align the
KubernetesPullSecretsEnabled condition with ensurePullSecrets by using the same
explicit enabled-value comparison, unless backward compatibility requires
retaining the permissive deletion behavior.
In `@pkg/driver/kubernetes/run.go`:
- Around line 104-189: Update runContainer to call reconcileExistingPod before
buildPod, returning immediately when skip is true so skipped runs do not create
or refresh Kubernetes resources through buildPod’s ensureServiceAccount,
ensureDaemonConfig, and ensurePullSecrets calls. Preserve error propagation, and
only build and run the pod when reconciliation does not skip.
In `@pkg/options/resolver/util.go`:
- Around line 98-124: Add an early nil check for the parent option in
addDependency before it calls validateChildDependency or
validateVariableDependency; return nil when option is nil. Preserve the existing
dependency validation behavior for non-nil options and avoid dereferencing
option before this guard.
🪄 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: 9dd7e04d-8778-4bbc-9f21-f6c6022a2445
📒 Files selected for processing (76)
pkg/agent/agent.gopkg/agent/binary.gopkg/agent/delivery/factory.gopkg/agent/tunnelserver/tunnelserver.gopkg/client/clientimplementation/daemonclient/client.gopkg/client/clientimplementation/daemonclient/delete.gopkg/client/clientimplementation/daemonclient/form.gopkg/client/clientimplementation/daemonclient/up.gopkg/command/background.gopkg/config/config.gopkg/copy/copy.gopkg/credentials/server.gopkg/daemon/agent/daemon.gopkg/daemon/platform/workspace_watcher.gopkg/devcontainer/build/options.gopkg/devcontainer/buildkit/buildkit.gopkg/devcontainer/buildkit/cache.gopkg/devcontainer/buildkit/remote.gopkg/devcontainer/config/config.gopkg/devcontainer/config/merge.gopkg/devcontainer/config/parse_test.gopkg/devcontainer/config/result.gopkg/devcontainer/config/substitute.gopkg/devcontainer/feature/extend.gopkg/devcontainer/feature/features.gopkg/devcontainer/prebuild.gopkg/devcontainer/setup/setup.gopkg/devcontainer/single.gopkg/dockercredentials/dockercredentials.gopkg/dockerfile/parse.gopkg/driver/kubernetes/driver.gopkg/driver/kubernetes/init_container.gopkg/driver/kubernetes/pullsecrets.gopkg/driver/kubernetes/pvc.gopkg/driver/kubernetes/run.gopkg/driver/kubernetes/serviceaccount.gopkg/driver/kubernetes/wait.gopkg/extract/compress.gopkg/extract/extract.gopkg/gitsshsigning/helper.gopkg/ide/fleet/fleet.gopkg/ide/ideparse/parse.gopkg/ide/jetbrains/generic.gopkg/ide/rstudio/rstudio.gopkg/ide/vscode/vscode.gopkg/inject/inject.gopkg/netstat/netstat_util.gopkg/netstat/watcher.gopkg/options/resolve.gopkg/options/resolve_test.gopkg/options/resolver/parse.gopkg/options/resolver/resolve.gopkg/options/resolver/resolver.gopkg/options/resolver/util.gopkg/platform/client/client.gopkg/platform/deploy.gopkg/platform/form/form.gopkg/platform/instance.gopkg/platform/kubeconfig.gopkg/platform/owner.gopkg/platform/parameters/parameters.gopkg/platform/remotecommand/client.gopkg/provider/env.gopkg/provider/parse.gopkg/shell/shell.gopkg/ssh/config.gopkg/ssh/server/ssh.gopkg/telemetry/collect.gopkg/ts/workspace_server.gopkg/tunnel/browser.gopkg/tunnel/forwarder.gopkg/types/types.gopkg/util/homedir.gopkg/workspace/exec.gopkg/workspace/list.gopkg/workspace/workspace.go
🚧 Files skipped from review as they are similar to previous changes (56)
- pkg/devcontainer/config/result.go
- pkg/agent/binary.go
- pkg/command/background.go
- pkg/provider/env.go
- pkg/driver/kubernetes/serviceaccount.go
- pkg/driver/kubernetes/init_container.go
- pkg/devcontainer/buildkit/cache.go
- pkg/telemetry/collect.go
- pkg/extract/extract.go
- pkg/devcontainer/config/substitute.go
- pkg/dockercredentials/dockercredentials.go
- pkg/dockerfile/parse.go
- pkg/credentials/server.go
- pkg/devcontainer/single.go
- pkg/options/resolver/parse.go
- pkg/devcontainer/buildkit/remote.go
- pkg/devcontainer/setup/setup.go
- pkg/ide/jetbrains/generic.go
- pkg/devcontainer/build/options.go
- pkg/options/resolve_test.go
- pkg/netstat/watcher.go
- pkg/driver/kubernetes/pullsecrets.go
- pkg/platform/remotecommand/client.go
- pkg/client/clientimplementation/daemonclient/delete.go
- pkg/tunnel/browser.go
- pkg/devcontainer/feature/extend.go
- pkg/platform/instance.go
- pkg/gitsshsigning/helper.go
- pkg/devcontainer/config/merge.go
- pkg/agent/delivery/factory.go
- pkg/ide/fleet/fleet.go
- pkg/devcontainer/config/parse_test.go
- pkg/extract/compress.go
- pkg/ide/vscode/vscode.go
- pkg/types/types.go
- pkg/ide/rstudio/rstudio.go
- pkg/driver/kubernetes/wait.go
- pkg/client/clientimplementation/daemonclient/up.go
- pkg/platform/parameters/parameters.go
- pkg/devcontainer/feature/features.go
- pkg/workspace/exec.go
- pkg/daemon/agent/daemon.go
- pkg/ssh/config.go
- pkg/client/clientimplementation/daemonclient/client.go
- pkg/client/clientimplementation/daemonclient/form.go
- pkg/inject/inject.go
- pkg/options/resolver/resolver.go
- pkg/util/homedir.go
- pkg/agent/tunnelserver/tunnelserver.go
- pkg/workspace/workspace.go
- pkg/platform/client/client.go
- pkg/platform/deploy.go
- pkg/options/resolver/resolve.go
- pkg/ssh/server/ssh.go
- pkg/workspace/list.go
- pkg/shell/shell.go
skevetter
marked this pull request as draft
July 26, 2026 21:46
skevetter
force-pushed
the
refactor/cyclop-pkg
branch
from
July 26, 2026 21:51
1d341ab to
7c8e0b9
Compare
Decompose functions in pkg/ 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, and keep unexported methods ordered after exported ones (funcorder). Also fixes two pre-existing latent bugs surfaced during review: - types: LifecycleHook named array commands used an unchecked type assertion that panicked on non-string elements; now returns ErrUnsupportedType via the existing stringArray helper. - ssh/config: transformHostSection wrapped the (nil) file-open error instead of the scanner error on parse failure. Sets ReadHeaderTimeout on the credentials http.Server (gosec G112).
skevetter
force-pushed
the
refactor/cyclop-pkg
branch
from
July 26, 2026 21:58
7c8e0b9 to
9e40c85
Compare
skevetter
marked this pull request as ready for review
July 26, 2026 22:21
skevetter
enabled auto-merge (squash)
July 26, 2026 22:30
skevetter
added a commit
that referenced
this pull request
Jul 27, 2026
The #765 handler refactor switched unmatched paths from an implicit 200 (the old switch had no default) to http.NotFound. The readiness probe in waitForServer hits the root path, so startup detection began failing with 404 and git/docker credential forwarding was silently skipped for every driver ("credentials server did not start in time"). Add an explicit root route that returns 200 while keeping 404 for unknown credential endpoints.
skevetter
added a commit
that referenced
this pull request
Jul 27, 2026
#765's refactor widened the per-line trim from "\n" to "\r\n", so removing the SSH-signing section from a CRLF-authored gitconfig rewrote every line to LF. Trim only the LF to leave untouched lines' endings intact.
skevetter
added a commit
that referenced
this pull request
Jul 27, 2026
The #765 handler refactor switched unmatched paths from an implicit 200 (the old switch had no default) to http.NotFound. The readiness probe in waitForServer hits the root path, so startup detection began failing with 404 and git/docker credential forwarding was silently skipped for every driver ("credentials server did not start in time"). Add an explicit root route that returns 200 while keeping 404 for unknown credential endpoints.
skevetter
added a commit
that referenced
this pull request
Jul 27, 2026
#765's refactor widened the per-line trim from "\n" to "\r\n", so removing the SSH-signing section from a CRLF-authored gitconfig rewrote every line to LF. Trim only the LF to leave untouched lines' endings intact.
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
pkg/that exceeded the golangci-lintcyclopmax-complexity: 8into well-named helpers (guard clauses, extracted predicates, param/result structs, switch/lookup maps), preserving behavior.Part 2 of 2 of a repo-wide cyclop cleanup (companion PR covers
cmd/,e2e/,hack/).Latent bug fixes surfaced during review
While reducing complexity, review (CodeRabbit + independent adversarial passes) surfaced a few pre-existing bugs, fixed here as they touch the same code:
LifecycleHooknamed array-of-strings commands used an unchecked type assertion that panicked on non-string elements; now returnsErrUnsupportedType.transformHostSectionwrapped the (nil) file-open error instead of the scanner error on parse failure.loftPodFailureErrorused mixed implicit/explicitfmtverbs, droppingreasonand duplicatingout; now uses explicit indices.%q-quoted.Directoryusedos.Stat(follows links) for the type switch, so symlinks were dereferenced and theos.ModeSymlinkbranch was dead; now usesentry.Info()(Lstat) so symlinks are preserved.writePlatformSSHKeysstale-key removal condition was inverted (left oldplatform_git_ssh_<i>files on disk when the key count shrank); corrected, and stale files are now removed on write failure.removeSignatureHelpernow strips CRLF (\r\n) so CRLF-terminated git configs are handled cleanly.ReadHeaderTimeout.BoolValue) on interactive update instead of silently defaulting to false; uncheckedvalue.(string)assertions replaced with type-safe handling.parseGracePeriodnow rejects<= 0durations (a"0s"grace period previously produced an instant-cancel context).RemoveDaemonnow treatssystemctl disable's "does not exist" output as a no-op (previously only "not loaded" fromstopwas ignored)./proc/<pid>/statfield access against malformed input.Known follow-up (not addressed here)
exitWithError), matching current/origin/mainbehavior. Arguably it should log and continue with agent forwarding disabled (a non-essential convenience) rather than killing the session. Left as-is to keep this PR behavior-preserving on that path; tracked as a follow-up.go build ./...,go vet, and package tests pass;golangci-lint(only-new-issues vsmain) is clean.Summary by CodeRabbit
New Features
Bug Fixes
Refactor