SANDBOX-1810: Implement pod cache and session manager - #12
Conversation
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds a Kubernetes-backed ChangesSession Package: PodCache, SandboxConfig, and SessionManager
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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.
Actionable comments posted: 4
🤖 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/session/config.go`:
- Around line 22-35: The production default in DefaultConfig() uses a mutable
:latest image tag, which can change behavior unexpectedly between deployments.
Update the SandboxConfig.Image value in DefaultConfig() to use a pinned
immutable version or digest instead, and keep the rest of the default config
unchanged.
In `@pkg/session/manager.go`:
- Around line 56-67: NewSessionManager currently allows SandboxConfig values
with an empty HMACKey, which can lead to token generation with an insecure
default. Add a validation check in NewSessionManager (and/or a helper it calls)
to reject empty config.HMACKey at construction time, returning or triggering a
fast failure before the SessionManager is created. Use NewSessionManager,
SandboxConfig, and computeToken as the key symbols to locate the fix.
- Around line 98-103: `ExecuteCommand()` is losing the resolved pod name from
`discoverPod()`, so `updateLastActivity()` later rebuilds
`podNamePrefix+sessionID` and may touch the wrong pod. Update the
`ExecuteCommand` flow to preserve and pass the actual `podName` returned by
`discoverPod()` through to `updateLastActivity()`, and adjust that helper so it
uses the resolved pod name instead of reconstructing one. Make the same change
anywhere else in the 405-430 range where last-activity refresh is triggered, so
labeled pods discovered by `discoverPod` get their annotations updated
correctly.
- Around line 379-380: Build the exec proxy URL using net.JoinHostPort instead
of formatting the host and port manually, so the request works for both IPv4 and
IPv6 pod IPs. Update the URL construction in the code that creates the HTTP
request in the session manager flow (the logic around the exec request in the
relevant manager method) to join podIP and m.config.AgentPort safely before
appending the /exec path.
🪄 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: 56dacb08-6c08-43d4-9294-228928eb102e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.modpkg/session/cache.gopkg/session/cache_test.gopkg/session/config.gopkg/session/manager.gopkg/session/manager_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)
📜 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.gopkg/session/cache.gogo.modpkg/session/cache_test.gopkg/session/manager_test.gopkg/session/manager.go
🪛 ast-grep (0.44.0)
pkg/session/manager.go
[warning] 37-37: 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)
[warning] 228-228: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: int32(m.config.AgentPort)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🪛 golangci-lint (2.12.2)
pkg/session/manager.go
[error] 394-394: Error return value of resp.Body.Close is not checked
(errcheck)
- Remove mutable :latest tag from DefaultConfig; require explicit Image - Validate HMACKey and Image are non-empty in NewSessionManager (fail-fast) - Use resolved pod name from cache in updateLastActivity instead of reconstructing it - Use net.JoinHostPort for IPv6-safe agent URL construction - Fix unchecked resp.Body.Close (errcheck lint) - Add TestNewSessionManagerValidation covering config rejection Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Feny Mehta <fbm3307@gmail.com>
1 participant
Implements
pkg/session— the pod cache and session manager for per-investigation sandbox pods (SANDBOX-1810).PodCachewith 30s TTL to avoid K8s API calls on every commandDELETE /sessions/{id}) + 30-minute idle TTL safety netAssisted by: Cursor
Summary by CodeRabbit