update architecture - #11
Merged
Merged
Conversation
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
WalkthroughAdds a new "Session & Pod Lifecycle" section to README.md documenting the stateless MCP server architecture, end-to-end request flow, three-tier pod resolution in ChangesSession & Pod Lifecycle Documentation
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
coderabbitai
Bot
added
the
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@README.md`:
- Around line 78-83: Update the Warm Pool documentation to state that warm pods
must be fully reset or scrubbed before being reused for a new session, including
shell state and emptyDir contents. In the README section describing
--warm-pool-size, revise the “assigned instantly” wording to mention the reset
boundary and clarify that reuse is only safe after reprovisioning/cleanup of
each warm pod between investigations.
- Around line 84-93: The Pod Spec currently exposes the bearer token through an
environment variable in the sandbox, which makes it visible to any command
running there. Update the auth flow in the Pod Spec and related sandbox
credential handling so the HMAC-SHA256-derived bearer token is not placed in a
bash-visible env var; instead deliver it through a less exposed mechanism
already used by the sandbox such as a mounted Secret file or other non-env
secret injection path, while keeping the token available to the component that
needs it.
- Around line 49-77: The Tier 1 cache in SessionManager.GetOrCreatePod currently
stores only a pod IP, which can become stale after restarts; update the cached
entry to include a stable pod identifier such as the pod name. On lookup, use
that cached name to re-resolve the current IP before returning the pod target,
and keep the PodCache behavior aligned with the existing label-discovery and
create flow.
🪄 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: f66c8e0e-d415-44bb-bd62-182b9754e2be
📒 Files selected for processing (1)
README.md
🔗 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:
README.md
🪛 markdownlint-cli2 (0.22.1)
README.md
[warning] 51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔀 Multi-repo context
Linked repositories findings
codeready-toolchain/mcp-server-devsandbox
deploy/README.mddocuments that all requests require authentication and useAuthorization: Bearer <token>.- The same README says the MCP server “uses an HTTP streaming transport with session management,” which matches the new README’s session-oriented architecture, but I found no code references to
SessionManager,GetOrCreatePod, pod cache, or warm pool behavior.
codeready-toolchain/host-operator
- I did not find any repo-level consumer of the new session/pod lifecycle architecture.
- The only nearby
/execreferences I found earlier were Kubernetes RBAC permissions (pods/exec), not an application API contract.
codeready-toolchain/toolchain-e2e
- No direct consumer of the new session/pod lifecycle behavior was found.
- The only nearby references were
pods/execRBAC checks and generic bearer-token usage in tests.
codeready-toolchain/toolchain-common
- Only generic bearer-token handling appears; no session/pod-cache coupling.
codeready-toolchain/api
- No relevant references found.
codeready-toolchain/mcp-common
- No relevant references found.
🔇 Additional comments (2)
README.md (2)
44-46: LGTM!
94-105: LGTM!
Comment on lines
+49
to
+77
| ### Request Flow | ||
|
|
||
| ``` | ||
| TARSy (LLM) | ||
| │ MCP tool call + X-Session-ID header | ||
| ▼ | ||
| MCP Server (stateless, multi-replica) | ||
| │ HTTP POST /exec + Bearer HMAC token | ||
| ▼ | ||
| Sandbox Pod (per-session, persistent bash) | ||
| │ stdout / stderr / exit_code | ||
| ▼ | ||
| MCP Server → TARSy | ||
| ``` | ||
|
|
||
| ### GetOrCreatePod — Three-Tier Lookup | ||
|
|
||
| When a command arrives for a session, `SessionManager.GetOrCreatePod` resolves the | ||
| target pod using a three-tier strategy: | ||
|
|
||
| | Tier | Method | Latency | Description | | ||
| |------|--------|---------|-------------| | ||
| | 1 | **PodCache** | ~0ms | In-memory map of session ID → pod IP with 30s TTL | | ||
| | 2 | **Label discovery** | ~50-100ms | `List` pods by `tarsy.redhat.com/session-id` label | | ||
| | 3 | **Create** | ~3-8s | Create auth Secret + Pod, wait for readiness probe | | ||
|
|
||
| Cache misses fall through to tier 2; if no pod exists, tier 3 creates one | ||
| idempotently (handles `AlreadyExists`). | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files | sed -n '1,120p'
printf '\n== README around cited lines ==\n'
cat -n README.md | sed -n '40,110p'
printf '\n== search for session manager / pod cache / pod identity ==\n'
rg -n "SessionManager|GetOrCreatePod|PodCache|X-Session-ID|session-id|pod IP|pod uid|pod name|Authorization: Bearer|HMAC" .Repository: codeready-toolchain/cli-mcp-server
Length of output: 39337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== outline likely relevant files ==\n'
fd -a '.*(session|pod|sandbox|server|transport).*' . -t f | sed -n '1,200p'
printf '\n== README lines 1-140 ==\n'
cat -n README.md | sed -n '1,140p'Repository: codeready-toolchain/cli-mcp-server
Length of output: 5495
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== relevant file candidates ==\n'
fd -a '^(README|.*session.*|.*pod.*|.*sandbox.*|.*server.*)\.(md|go|ts|js|yaml|yml)$' . -t f | sed -n '1,200p'
printf '\n== read any session/pod related code references ==\n'
rg -n "type SessionManager|func .*GetOrCreatePod|PodCache|session-id|X-Session-ID|pod IP|AlreadyExists|readiness probe|Authorization" \
--glob '!README.md' --glob '!**/vendor/**' .Repository: codeready-toolchain/cli-mcp-server
Length of output: 25449
Cache a stable pod identity, not just the IP. Tier 1 reads as session ID → pod IP, which can go stale after a restart and send the next request to the wrong address until TTL expiry; keep the pod name in the cache entry and re-resolve the current IP on lookup.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 51-51: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@README.md` around lines 49 - 77, The Tier 1 cache in
SessionManager.GetOrCreatePod currently stores only a pod IP, which can become
stale after restarts; update the cached entry to include a stable pod identifier
such as the pod name. On lookup, use that cached name to re-resolve the current
IP before returning the pod target, and keep the PodCache behavior aligned with
the existing label-discovery and create flow.
Source: Linked repositories
Comment on lines
+78
to
+83
| ### Warm Pool | ||
|
|
||
| When `--warm-pool-size N` is set, N pre-warmed pods are maintained in a ready state. | ||
| New sessions are assigned a warm pod instantly (~0ms) instead of waiting for a cold | ||
| start. Disabled by default (`N=0`). | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Document the reset boundary before reusing warm pods.
"Assigned to new sessions instantly" is only safe if every pod is fully reprovisioned or scrubbed between investigations. Without that, shell state and emptyDir contents can leak across sessions.
🤖 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 `@README.md` around lines 78 - 83, Update the Warm Pool documentation to state
that warm pods must be fully reset or scrubbed before being reused for a new
session, including shell state and emptyDir contents. In the README section
describing --warm-pool-size, revise the “assigned instantly” wording to mention
the reset boundary and clarify that reuse is only safe after
reprovisioning/cleanup of each warm pod between investigations.
Comment on lines
+84
to
+93
| ### Pod Spec | ||
|
|
||
| Each sandbox pod runs with a hardened security context: | ||
|
|
||
| - **Non-root**: UID/GID 1001, `allowPrivilegeEscalation: false`, all capabilities dropped | ||
| - **Resources**: 100m/500m CPU, 128Mi/512Mi memory | ||
| - **Readiness**: HTTP GET `/health` on port 8090 | ||
| - **Volumes**: `kubeconfig` (read-only Secret mount), `workspace` (emptyDir) | ||
| - **Auth**: HMAC-SHA256(key, sessionID) → bearer token delivered via Secret → env var | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Keep the bearer token out of the bash-visible environment.
Delivering the credential via env var inside the sandbox makes it readable by any command running there. That turns a shell compromise into control-plane credential exposure.
🤖 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 `@README.md` around lines 84 - 93, The Pod Spec currently exposes the bearer
token through an environment variable in the sandbox, which makes it visible to
any command running there. Update the auth flow in the Pod Spec and related
sandbox credential handling so the HMAC-SHA256-derived bearer token is not
placed in a bash-visible env var; instead deliver it through a less exposed
mechanism already used by the sandbox such as a mounted Secret file or other
non-env secret injection path, while keeping the token available to the
component that needs it.
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.
pod cache and session architecture
Summary by CodeRabbit