SANDBOX-1812: add typed HTTP client for sandbox agent API - #16
Conversation
- Introduce AgentClient with Execute, Assign, and HealthCheck - Add NetworkError, StatusError, and DecodeError for callers to branch on - Cap successful Execute bodies at 10 MB; truncate error bodies to 512 bytes - Cover options, error types, and concurrent use with httptest tests Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
WalkthroughAdds a configurable typed HTTP client for sandbox agent ChangesAgent client
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AgentClient
participant SandboxAgentAPI
Caller->>AgentClient: Execute, Assign, or HealthCheck
AgentClient->>SandboxAgentAPI: Send endpoint request
SandboxAgentAPI-->>AgentClient: Return status and body
AgentClient-->>Caller: Return response or structured error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (2)
pkg/agent/client.go (2)
106-113: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winError-path response bodies aren't drained before
Close(), unlike the success path.On success,
drainBody(Lines 179, 207) reads up to 4 KB so the connection can be reused. On theStatusErrorpath, only up to 512 bytes are read viareadBodyTruncated(Lines 111, 176, 204) before the deferredClose()runs — the rest of the body is left unread, which typically preventsnet/httpfrom reusing the underlying connection. GivenHealthCheck/Assignfailures may be frequent (e.g., during pod startup), this adds unnecessary connection churn. Consider draining the remainder after truncation.Also applies to: 171-178, 199-206
🤖 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/agent/client.go` around lines 106 - 113, Ensure all StatusError paths in the client response handling drain the remaining response body after readBodyTruncated and before the deferred Close, matching the success-path behavior in drainBody. Update the shared error-handling logic used by HealthCheck and Assign so the truncated body is preserved while the remainder is consumed for connection reuse.
84-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSignificant duplication across
Execute,Assign, andHealthCheck.Each method repeats request construction,
Do/classifyDoError, and theStatusCode != http.StatusOK→StatusErrorblock almost verbatim. Extracting a shareddoRequest(ctx, method, url, body, op string, headers map[string]string) (*http.Response, error)helper (returning the response for the caller to decode/drain) would reduce this to method-specific decode logic only.As per path instructions, focus on maintainability for
**.🤖 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/agent/client.go` around lines 84 - 209, Reduce duplication among AgentClient.Execute, AgentClient.Assign, and AgentClient.HealthCheck by introducing a shared doRequest helper that accepts the context, HTTP method, URL, optional body, operation name, and headers. Move request creation, header application, HTTP execution, classifyDoError handling, and non-OK StatusError construction into the helper, returning the successful response for each method’s existing decode or drain logic; then simplify the three methods to call it and retain only endpoint-specific behavior.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.
Nitpick comments:
In `@pkg/agent/client.go`:
- Around line 106-113: Ensure all StatusError paths in the client response
handling drain the remaining response body after readBodyTruncated and before
the deferred Close, matching the success-path behavior in drainBody. Update the
shared error-handling logic used by HealthCheck and Assign so the truncated body
is preserved while the remainder is consumed for connection reuse.
- Around line 84-209: Reduce duplication among AgentClient.Execute,
AgentClient.Assign, and AgentClient.HealthCheck by introducing a shared
doRequest helper that accepts the context, HTTP method, URL, optional body,
operation name, and headers. Move request creation, header application, HTTP
execution, classifyDoError handling, and non-OK StatusError construction into
the helper, returning the successful response for each method’s existing decode
or drain logic; then simplify the three methods to call it and retain only
endpoint-specific behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c834fa34-5f3d-4bb1-b51f-13ebd2e5137e
📒 Files selected for processing (3)
pkg/agent/client.gopkg/agent/client_test.gopkg/agent/errors.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)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: CodeRabbit / Review
- GitHub Check: build-test-coverage
🧰 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/agent/errors.gopkg/agent/client_test.gopkg/agent/client.go
🧬 Code graph analysis (2)
pkg/agent/client_test.go (1)
pkg/agent/types.go (3)
ExecResponse(11-16)ExecRequest(5-8)AssignRequest(19-21)
pkg/agent/client.go (1)
pkg/agent/types.go (3)
ExecResponse(11-16)ExecRequest(5-8)AssignRequest(19-21)
🔀 Multi-repo context codeready-toolchain/mcp-server-devsandbox
Linked repositories findings
codeready-toolchain/mcp-server-devsandbox
- No
/exec,/assign,AgentClient, or agent request/response type references were found. - The repository’s
/healthendpoint is the MCP server readiness probe inpkg/mcpinit/init.go:213, served on the MCP HTTP address (defaultlocalhost:8080), not the sandbox agent’s port8090. It should not conflict with this client’s/healthcalls. [::codeready-toolchain/mcp-server-devsandbox::]
Other linked repositories
- No consumers or shared contracts for
AgentClient,ExecRequest,ExecResponse,AssignRequest,/exec,/assign, or port8090were found inmcp-common,api,toolchain-common,host-operator, ortoolchain-e2e.
🔇 Additional comments (8)
pkg/agent/client.go (5)
1-34: LGTM!
40-81: LGTM!
106-113: 🎯 Functional CorrectnessOnly
http.StatusOK(200) is treated as success.Any other 2xx response (e.g., 201/202/204) from the agent would be classified as
StatusError. If the agent's actual contract could ever return a non-200 success code, this would misclassify successful calls as failures. Worth confirming the agent's documented status codes.Also applies to: 171-178, 199-206
211-242: LGTM!
106-146: 🎯 Functional CorrectnessBody-read failures may need separate classification
After a 200 response,io.ReadAll(limited)errors currently becomeDecodeError. If mid-stream cancellation or connection loss should be treated as retryable transport failure, branch onctx.Err()here before falling back toDecodeError; otherwise callers looking for*NetworkErrorwill miss it.pkg/agent/errors.go (1)
1-53: LGTM!pkg/agent/client_test.go (2)
22-33: LGTM!Also applies to: 35-219, 221-279, 281-335, 337-397, 399-461, 463-503
59-59: 🎯 Functional CorrectnessGo 1.24+ is already required —
go.moddeclaresgo 1.24.6, so botht.Context()andfor i := range nare supported here.> Likely an incorrect or invalid review comment.
2 participants
pkg/agent.AgentClientwithExecute,Assign, andHealthCheckfor the sandbox agent HTTP APINetworkError,StatusError,DecodeError) so callers can branch witherrors.As/errors.Ishttptestcovering success/failure paths, context cancel/deadline, options, and concurrent use (-race)Assisted by: Cursor
Summary by CodeRabbit
Release Notes
New Features
Tests