chore: modernize tooling (Go 1.26.4, deps, renovate, golangci-lint)#42
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 47 minutes and 22 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (41)
WalkthroughReplaces Dependabot with Renovate for dependency management, upgrades GitHub Actions and Go toolchain versions, significantly expands the golangci-lint linter and formatter configuration, adds Makefile ChangesLinter and CI Infrastructure
Error Wrapping, Safe Type Assertions, and Code Correctness
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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/devbox/shell.go (1)
121-154: 🩺 Stability & Availability | 🟠 MajorFix error propagation and goroutine leak in
containerExec.The function has two bugs:
Dropped error from
StdCopy: Thecase <-done:branch receives the error from the goroutine but discards it, always returningnilinstead of propagating failures from copying output streams.Goroutine leak on context cancellation: The unbuffered
donechannel causes the goroutine to block indefinitely whenctx.Done()fires first—the goroutine will attempt to send ondonewith no receiver, blocking until the process terminates.Both issues prevent proper error handling and can cause resource leaks. The fix makes
donebuffered, captures the copy error, and propagates it correctly:Proposed fix
func containerExec(ctx context.Context, containerID string, cmd []string) (stdoutBytes, stderrBytes []byte, err error) { @@ - done := make(chan error) + done := make(chan error, 1) go func() { - _, err = stdcopy.StdCopy(&stdout, &stderr, execAttachResp.Reader) - done <- err + _, copyErr := stdcopy.StdCopy(&stdout, &stderr, execAttachResp.Reader) + done <- copyErr }() select { - case <-done: - break + case copyErr := <-done: + if copyErr != nil { + return nil, nil, fmt.Errorf("failed to read exec output: %w", copyErr) + } case <-ctx.Done(): - return nil, nil, errors.New("context cancelled") + return nil, nil, fmt.Errorf("context cancelled: %w", ctx.Err()) } return stdout.Bytes(), stderr.Bytes(), 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/devbox/shell.go` around lines 121 - 154, The containerExec function has two bugs that need fixing. First, the unbuffered done channel causes a goroutine leak when ctx.Done() fires before the goroutine finishes—change the done channel declaration to be buffered with capacity 1. Second, the case <-done: branch in the select statement receives the error from the goroutine but discards it and always returns nil—capture the error value from the done channel and return it properly instead of returning nil for the error value.
🤖 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 @.github/workflows/build-and-release.yaml:
- Around line 20-29: Replace the mutable version tags with immutable full commit
SHAs for all three GitHub Actions in the release workflow. Change
actions/checkout@v7 to pin to its specific commit SHA, actions/setup-go@v6 to
its commit SHA, and goreleaser/goreleaser-action@v7 to its commit SHA. This
prevents supply-chain attacks where tag reassignment could inject malicious code
into your release pipeline that has elevated permissions and access to
production secrets. Obtain the exact commit SHAs from the respective GitHub
repositories and use them in the uses field instead of the version tags.
In @.github/workflows/ci.yaml:
- Line 18: The checkout action steps are not disabling credential persistence,
which keeps the GITHUB_TOKEN in git config across later steps unnecessarily. Add
the `persist-credentials: false` parameter to both checkout action steps (at
line 18 and line 44 where `actions/checkout@v7` is used) to prevent the GitHub
token from being persisted in the git configuration across subsequent workflow
steps.
- Around line 18-20: Replace all mutable version tags for GitHub Actions with
immutable commit SHAs in the CI workflow file. Update the uses statements for
actions/checkout, actions/setup-go, and golangci/golangci-lint-action to use
their specific commit hashes instead of version tags (like `@v7`, `@v6`, `@v9`). Each
action reference should be changed from the format "action@vX" to
"action@<commit-sha>" with a comment noting the version it corresponds to for
reference.
In @.golangci.yml:
- Around line 81-87: Add explanatory comments to the gosec rule exclusions in
the excludes section to document the rationale for disabling each security
check. For each excluded rule (G104, G204, G301, G302, G304, G306), include a
comment explaining why it is excluded, such as whether it's due to false
positive rate, intentional audit-only concerns, or project-specific permission
models. This preserves security oversight and clarifies the intent behind these
exclusions to future reviewers.
In `@internal/cert/cert.go`:
- Around line 136-137: Private key files are being written with overly
permissive file mode 0o644, which allows non-owner users to read sensitive key
material on multi-user systems. Change the file permission mode from 0o644 to
0o600 for both occurrences where private key files are written: the os.WriteFile
call for clientKeyPEM and the similar write operation at line 202 for the other
private key file. This restricts read and write access to the owner only, as
required for secure handling of cryptographic key material.
In `@internal/manager/manager.go`:
- Around line 388-399: The string prefix matching in the condition
`strings.HasPrefix(servicePath, sourcePrefix)` at line 388 lacks path-boundary
awareness and can incorrectly match sibling paths (for example, `/sources/api`
would match `/sources/api-gateway`). Replace this raw string prefix check with
path-boundary-aware matching by verifying that after the prefix match, the next
character is a path separator (or the strings are equal), ensuring only actual
sub-paths under the source directory are matched rather than paths that merely
start with the same string. Use the filepath package or add an explicit boundary
check after the prefix match.
In `@internal/project/project.go`:
- Around line 235-239: The code parses an IP address using net.ParseIP(item.IP)
but fails to check if the result is nil before calling ip.String() on line 238,
allowing invalid IPs to be converted to the string "<nil>" and propagated into
the ipToHosts map. Add a nil check after the net.ParseIP call and handle the
invalid IP case appropriately by either returning an error to fail fast or
skipping the invalid entry, ensuring that only valid parsed IPs are normalized
and assigned to item.IP.
---
Outside diff comments:
In `@cmd/devbox/shell.go`:
- Around line 121-154: The containerExec function has two bugs that need fixing.
First, the unbuffered done channel causes a goroutine leak when ctx.Done() fires
before the goroutine finishes—change the done channel declaration to be buffered
with capacity 1. Second, the case <-done: branch in the select statement
receives the error from the goroutine but discards it and always returns
nil—capture the error value from the done channel and return it properly instead
of returning nil for the error value.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4bd14500-9ac9-4aec-af03-8c458628d689
⛔ Files ignored due to path filters (5)
go.sumis excluded by!**/*.suminternal/git/git_test.gois excluded by!**/*_test.gointernal/hosts/hosts_test.gois excluded by!**/*_test.gointernal/manager/manager_test.gois excluded by!**/*_test.gotests/e2e/e2e_test.gois excluded by!**/*_test.go
📒 Files selected for processing (41)
.github/dependabot.yml.github/renovate.json.github/workflows/build-and-release.yaml.github/workflows/ci.yaml.golangci.ymlMakefileREADME.mdcmd/devbox/destroy.gocmd/devbox/devbox.gocmd/devbox/down.gocmd/devbox/env.gocmd/devbox/info.gocmd/devbox/init.gocmd/devbox/install_ca.gocmd/devbox/list.gocmd/devbox/logs.gocmd/devbox/mount.gocmd/devbox/ps.gocmd/devbox/restart.gocmd/devbox/run.gocmd/devbox/shell.gocmd/devbox/umount.gocmd/devbox/up.gocmd/devbox/update.gocmd/devbox/update_hosts.godocs/installation.mdgo.modinternal/app/const.gointernal/cert/cert.gointernal/cert/cert_darwin.gointernal/cert/cert_linux.gointernal/git/git.gointernal/git/normalizer.gointernal/hosts/hosts.gointernal/manager/manager.gointernal/pkg/fs/fs.gointernal/project/config.gointernal/project/export.gointernal/project/project.gointernal/table/export.gointernal/table/table.go
💤 Files with no reviewable changes (2)
- .github/dependabot.yml
- internal/table/table.go
pilat
force-pushed
the
chore/modernize-ci-lint-go-1.26
branch
from
c825532 to
d9604b0
Compare
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.
What
Modernizes the dev tooling, bringing it in line with our current standard:
moby/mobyis intentionally held at the versiondocker/compose v5.1.4supports — newer adds anAPIClientmethod the vendored compose client doesn't implement.gomodTidy).linuxanddarwin.go-version-file, goreleaser v7) plus a darwin lint pass so the platform-specific cert code gets linted too.Homebrew stays on the deprecated
brewsfor now (still functional); migrating tohomebrew_casksis a separate change.Testing
Unit (
-race), e2e, and lint all pass.Summary by CodeRabbit
Chores
Documentation
Refactor