ci: add full golangci-lint, PR image builds, and action v6 - #27
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request expands CI with image-building and lint jobs, configures Go linting and coverage output, adds HTTP request and header safeguards, updates sandbox process handling, adjusts session warm-pool readiness behavior, and modernizes tests. ChangesCI and lint validation
HTTP safeguards and client handling
Sandbox process and tool maintenance
Session lifecycle
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
78-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the golangci-lint version.
version: latestmakes lint results non-reproducible and can introduce breaking linter or configuration changes without a repository change. Pin a known-compatible release and update it deliberately.As per path instructions, this is a maintainability/reproducibility concern, not a cosmetic nit.
🤖 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 @.github/workflows/ci.yml around lines 78 - 82, Update the golangci-lint-action configuration in the Lint step to replace version: latest with a specific known-compatible release number. Keep the existing action and args unchanged, and ensure future version changes are made deliberately by updating the pinned value.Source: Path instructions
🤖 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/ci.yml:
- Around line 15-16: Add persist-credentials: false to each of the three
actions/checkout steps in the workflow, ensuring PR-controlled jobs do not
retain the workflow token in local git configuration while preserving the
existing checkout behavior.
- Around line 35-64: Add a job-level permissions setting to build-image granting
only contents: read, while preserving the existing checkout and local
image-build steps.
In `@cmd/agent/main.go`:
- Around line 42-45: Update the agent http.Server initialization to set
ReadTimeout and IdleTimeout, then wrap request bodies with http.MaxBytesReader
in both the /exec and /assign handlers before decoding them. Apply consistent
body-size limits so slow, oversized POST requests cannot hold the pod open.
In `@pkg/session/manager.go`:
- Around line 137-140: Update tryClaimPod after m.pool.ClaimPod succeeds to
require the claimed pod to be ready before calling m.cache.Set. Use the claimed
pod’s PodReady status from the claim result, and preserve the existing return
path only for ready claims; otherwise continue without caching the pod.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 78-82: Update the golangci-lint-action configuration in the Lint
step to replace version: latest with a specific known-compatible release number.
Keep the existing action and args unchanged, and ensure future version changes
are made deliberately by updating the pinned 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 52f5bb2e-6762-44c3-8f72-30f22f765033
📒 Files selected for processing (20)
.github/workflows/cd.yml.github/workflows/ci.yml.gitignore.golangci.ymlMakefilecmd/agent/main.gopkg/agent/client.gopkg/agent/client_test.gopkg/sandbox/bash.gopkg/sandbox/bash_test.gopkg/sandbox/handler.gopkg/server/server_test.gopkg/session/cache_test.gopkg/session/config.gopkg/session/manager.gopkg/session/manager_test.gopkg/session/pool.gopkg/session/pool_test.gopkg/tools/bash.gopkg/tools/bash_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/mcp-common(manual)codeready-toolchain/mcp-server-devsandbox(manual)codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
💤 Files with no reviewable changes (2)
- pkg/session/pool.go
- pkg/tools/bash.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/session/config.goMakefilecmd/agent/main.gopkg/sandbox/bash_test.gopkg/session/cache_test.gopkg/server/server_test.gopkg/sandbox/handler.gopkg/session/pool_test.gopkg/agent/client.gopkg/session/manager_test.gopkg/sandbox/bash.gopkg/tools/bash_test.gopkg/session/manager.gopkg/agent/client_test.go
🧬 Code graph analysis (6)
pkg/session/config.go (2)
pkg/session/manager_test.go (1)
DefaultConfig(33-33)pkg/session/pool_test.go (1)
DefaultConfig(597-597)
pkg/sandbox/handler.go (1)
cmd/agent/main.go (2)
HandleHealth(41-41)HandleExec(39-39)
pkg/sandbox/bash.go (2)
pkg/sandbox/bash_test.go (1)
NewBashSession(14-14)cmd/agent/main.go (1)
NewBashSession(31-31)
pkg/tools/bash_test.go (2)
pkg/agent/types.go (1)
ExecResponse(11-16)pkg/sandbox/bash.go (1)
Close(121-129)
pkg/session/manager.go (2)
pkg/session/cache.go (1)
Set(51-60)pkg/session/pool.go (1)
ClaimPod(165-192)
pkg/agent/client_test.go (2)
pkg/agent/errors.go (5)
Error(33-35)Error(16-18)Error(47-49)Unwrap(20-22)Unwrap(51-53)pkg/agent/client.go (1)
DefaultMaxResponseSize(18-18)
🪛 ast-grep (0.44.1)
cmd/agent/main.go
[warning] 41-45: This http.Server is constructed without a ReadTimeout. Without a read timeout, a slow or malicious client can hold connections open indefinitely (e.g. a Slowloris attack), exhausting server resources and causing a denial of service. Set ReadTimeout (and ideally ReadHeaderTimeout, WriteTimeout, and IdleTimeout) on the http.Server to bound how long the server waits while reading a request.
Context: http.Server{
Addr: ":8090",
Handler: mux,
ReadHeaderTimeout: 10 * time.Second,
}
Note: [CWE-400] Uncontrolled Resource Consumption.
(http-server-missing-read-timeout-go)
pkg/session/manager.go
[warning] 38-38: A credential is hard-coded as a string literal. Secrets stored in source code, such as passwords, API keys, and tokens, can be leaked through version control or binaries and used by internal or external malicious actors. Rotate the exposed secret and load it at runtime from a secure secret vault, a Hardware Security Module (HSM), or an environment variable if permitted by your company policy (e.g. password := os.Getenv("APP_PASSWORD")).
Context: secretNamePrefix = "cli-mcp-sandbox-auth-"
Note: [CWE-798] Use of Hard-coded Credentials.
(hardcoded-credentials-string-literal-go)
🪛 zizmor (1.26.1)
.github/workflows/ci.yml
[warning] 15-16: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 51-52: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 70-71: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 35-64: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🔇 Additional comments (18)
.github/workflows/cd.yml (1)
29-29: LGTM!.github/workflows/ci.yml (1)
11-12: LGTM!Also applies to: 18-19
.golangci.yml (1)
1-67: LGTM!Makefile (1)
52-55: LGTM!.gitignore (1)
12-13: LGTM!pkg/agent/client.go (1)
30-31: LGTM!Also applies to: 106-106, 171-171, 199-199
pkg/agent/client_test.go (1)
51-58: LGTM!Also applies to: 129-129, 145-151, 170-177, 202-202, 236-239, 354-354, 428-428, 438-438, 447-447
pkg/sandbox/bash.go (2)
69-69: 🩺 Stability & AvailabilityVerify that
CommandContextis actually cancellable.
exec.CommandContext(context.Background(), ...)can never receive cancellation, so it behaves likeexec.Commandfor this session. If lifecycle cancellation is intended, pass a session-owned context and cancel it during shutdown; otherwise useexec.Commandto make the behavior explicit.
5-5: LGTM!Also applies to: 60-60, 106-106, 132-133
pkg/sandbox/bash_test.go (1)
252-254: LGTM!pkg/sandbox/handler.go (1)
83-85: LGTM!Also applies to: 94-102, 151-153
pkg/tools/bash_test.go (1)
36-39: LGTM!Also applies to: 53-56, 72-75, 91-91, 112-112, 122-129, 147-149, 165-168, 209-211, 223-234
pkg/session/config.go (1)
25-25: LGTM!pkg/session/manager.go (1)
37-38: LGTM!Also applies to: 49-50, 119-120, 271-271, 461-461, 480-481, 566-567
pkg/session/manager_test.go (1)
251-251: LGTM!Also applies to: 529-532
pkg/session/pool_test.go (1)
64-64: LGTM!Also applies to: 468-468, 570-570
pkg/session/cache_test.go (1)
152-152: LGTM!pkg/server/server_test.go (1)
74-74: LGTM!Also applies to: 113-113
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
81-86: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the golangci-lint version instead of using
latest.An unpinned linter can change behavior without a repository commit, causing unexpected CI failures or newly introduced findings. Use an explicit tested version and bump it deliberately.
🤖 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 @.github/workflows/ci.yml around lines 81 - 86, Update the golangci/golangci-lint-action configuration in the Lint step to replace version: latest with an explicit tested golangci-lint version. Keep the existing action and args unchanged, and ensure future version bumps are deliberate.
🤖 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/sandbox/handler_test.go`:
- Around line 173-188: The oversized request-body test currently uses malformed
JSON, so it does not verify the size limit. Update the “rejects oversized
request body” case in HandleExec to serialize a valid request with a command
longer than maxRequestBody, using strings as needed, and add the equivalent
oversized valid-JSON regression test for HandleAssign.
In `@pkg/sandbox/handler.go`:
- Around line 127-130: Update the request parsing in the handler around
agent.ExecRequest to validate the complete bounded body, not only the first JSON
value. After the initial decode, perform a second decode and require io.EOF (or
fully consume the MaxBytesReader body) before executing; reject trailing data or
limit errors with the existing bad-request response.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 81-86: Update the golangci/golangci-lint-action configuration in
the Lint step to replace version: latest with an explicit tested golangci-lint
version. Keep the existing action and args unchanged, and ensure future version
bumps are deliberate.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7753ace9-6a6c-4025-be6f-367772b3d735
📒 Files selected for processing (4)
.github/workflows/ci.ymlcmd/agent/main.gopkg/sandbox/handler.gopkg/sandbox/handler_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/mcp-common(manual)codeready-toolchain/mcp-server-devsandbox(manual)codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/agent/main.go
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Build Image (cli-mcp-server)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/sandbox/handler_test.gopkg/sandbox/handler.go
🧬 Code graph analysis (2)
pkg/sandbox/handler_test.go (1)
pkg/sandbox/handler.go (1)
ErrorResponse(80-82)
pkg/sandbox/handler.go (1)
pkg/sandbox/handler_test.go (2)
HandleAssign(258-258)HandleExec(77-77)
🔇 Additional comments (4)
.github/workflows/ci.yml (3)
9-27: LGTM!
28-32: 🎯 Functional CorrectnessVerify that
make teststill produces the required coverage artifact.This workflow no longer invokes
make test-coverage; confirm thatmake testnow generates the coverage HTML required by the PR objective, or retain a dedicated coverage step.
34-66: LGTM!pkg/sandbox/handler.go (1)
14-16: LGTM!Also applies to: 86-88, 97-105, 155-158
Match mcp-common lint config and fix existing findings so the lint job is clean. Also build both container images on PRs (no push), bump checkout/setup-go to v6, and write coverage HTML to a file for CI. Co-authored-by: Cursor <cursoragent@cursor.com>
Match mcp-common by running tests once in CI. Add workflow permissions and persist-credentials: false, and bound agent request reads with timeouts plus MaxBytesReader. Co-authored-by: Cursor <cursoragent@cursor.com>
Renaming the job broke the repo required status check, which still expects build-test-coverage even though coverage is no longer run. Co-authored-by: Cursor <cursoragent@cursor.com>
Fully consume MaxBytesReader-bounded request bodies before unmarshaling, strengthen oversized-body tests, and wait for PodReady after warm-pool claims before caching the session pod. Co-authored-by: Cursor <cursoragent@cursor.com>
Pass a WithoutCancel request context into updateLastActivity, and drop the unused httptest request parameter in the last-activity test. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/session/manager_test.go (1)
611-611: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winResolve unused-parameter lint finding in new test handler.
The revive linter flags parameter
ras unused at line 611. Since this PR's objective is to resolve lint findings and match themcp-commongolangci-lint configuration, this new code should not introduce a new violation. Renamerto_.♻️ Proposed fix
- ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ts := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {🤖 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/session/manager_test.go` at line 611, In the httptest server handler passed to httptest.NewServer, rename the unused request parameter r to _ while preserving the existing response behavior.Source: Linters/SAST tools
🤖 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.
Outside diff comments:
In `@pkg/session/manager_test.go`:
- Line 611: In the httptest server handler passed to httptest.NewServer, rename
the unused request parameter r to _ while preserving the existing response
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3ecf7cc5-2ac9-4026-831d-2e16c11c7937
📒 Files selected for processing (21)
.github/workflows/cd.yml.github/workflows/ci.yml.gitignore.golangci.ymlMakefilecmd/agent/main.gopkg/agent/client.gopkg/agent/client_test.gopkg/sandbox/bash.gopkg/sandbox/bash_test.gopkg/sandbox/handler.gopkg/sandbox/handler_test.gopkg/server/server_test.gopkg/session/cache_test.gopkg/session/config.gopkg/session/manager.gopkg/session/manager_test.gopkg/session/pool.gopkg/session/pool_test.gopkg/tools/bash.gopkg/tools/bash_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
codeready-toolchain/mcp-common(manual)codeready-toolchain/mcp-server-devsandbox(manual)codeready-toolchain/api(manual)codeready-toolchain/toolchain-common(manual)codeready-toolchain/host-operator(manual)codeready-toolchain/toolchain-e2e(manual)
💤 Files with no reviewable changes (1)
- pkg/tools/bash.go
🚧 Files skipped from review as they are similar to previous changes (14)
- Makefile
- .github/workflows/cd.yml
- .golangci.yml
- cmd/agent/main.go
- pkg/session/config.go
- .gitignore
- pkg/session/cache_test.go
- pkg/sandbox/bash_test.go
- .github/workflows/ci.yml
- pkg/server/server_test.go
- pkg/session/pool.go
- pkg/sandbox/handler.go
- pkg/session/pool_test.go
- pkg/agent/client_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Build Image (cli-mcp-server)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
pkg/sandbox/bash.gopkg/tools/bash_test.gopkg/sandbox/handler_test.gopkg/agent/client.gopkg/session/manager.gopkg/session/manager_test.go
🧬 Code graph analysis (5)
pkg/tools/bash_test.go (1)
pkg/agent/types.go (1)
ExecResponse(11-16)
pkg/sandbox/handler_test.go (2)
pkg/sandbox/handler.go (2)
ErrorResponse(93-95)IsAssigned(49-53)pkg/agent/types.go (2)
ExecRequest(5-8)AssignRequest(19-21)
pkg/agent/client.go (1)
pkg/agent/client_test.go (3)
HealthCheck(305-305)Execute(65-65)Assign(247-247)
pkg/session/manager.go (5)
pkg/agent/types.go (1)
ExecRequest(5-8)pkg/session/config.go (1)
SandboxConfig(7-21)pkg/session/manager_test.go (2)
NewSessionManager(53-53)NewSessionManager(172-172)pkg/agent/errors.go (1)
NetworkError(10-14)pkg/agent/client.go (5)
NewAgentClient(74-84)WithTimeout(43-49)WithHTTPClient(52-56)AgentClient(33-37)WithPort(59-63)
pkg/session/manager_test.go (1)
pkg/agent/types.go (1)
ExecResponse(11-16)
🪛 ast-grep (0.44.1)
pkg/session/manager.go
[warning] 35-35: A credential is hard-coded as a string literal. Secrets stored in source code, such as passwords, API keys, and tokens, can be leaked through version control or binaries and used by internal or external malicious actors. Rotate the exposed secret and load it at runtime from a secure secret vault, a Hardware Security Module (HSM), or an environment variable if permitted by your company policy (e.g. password := os.Getenv("APP_PASSWORD")).
Context: secretNamePrefix = "cli-mcp-sandbox-auth-"
Note: [CWE-798] Use of Hard-coded Credentials.
(hardcoded-credentials-string-literal-go)
🪛 GitHub Check: Lint
pkg/session/manager.go
[failure] 468-468:
G118: Goroutine uses context.Background/TODO while request-scoped context is available (gosec)
pkg/session/manager_test.go
[failure] 611-611:
unused-parameter: parameter 'r' seems to be unused, consider removing or renaming it as _ (revive)
🔇 Additional comments (9)
pkg/agent/client.go (1)
30-31: LGTM!Lint directive cleanup only — no functional changes. The removed
//nolint:errcheckcomments ondefer resp.Body.Close()align with the updated golangci-lint configuration.Also applies to: 106-106, 171-171, 199-199
pkg/sandbox/handler_test.go (1)
9-9: Trailing-data note doesn’t apply
decodeJSONBodyusesjson.Unmarshalon the full body, so concatenated trailing JSON is rejected already.> Likely an incorrect or invalid review comment.pkg/session/manager_test.go (3)
251-251: LGTM!
529-532: LGTM!
603-649: LGTM!pkg/sandbox/bash.go (1)
5-5: LGTM!Also applies to: 60-60, 69-69, 106-106, 132-133
pkg/tools/bash_test.go (1)
36-39: LGTM!Also applies to: 53-56, 72-75, 88-118, 122-144, 147-151, 162-168, 209-212, 223-234
pkg/session/manager.go (2)
142-153: LGTM!Also applies to: 374-403
468-468: 📐 Maintainability & Code QualityNo change needed.
updateLastActivityalready uses a 10s timeout, so the goroutine is bounded.> Likely an incorrect or invalid review comment.
1 participant
Match mcp-common lint config and fix existing findings so the lint job is clean. Also build both container images on PRs (no push), bump checkout/setup-go to v6, and write coverage HTML to a file for CI.
Assisted By: Cursor
Summary by CodeRabbit
Improvements
/execand/assignby enforcing a maximum request body size and tightening JSON decode/encode error handling.Developer Experience
coverage.html, which is excluded from version control.Maintenance