feat(manifests,runner): wire ambient-mcp sidecar in mpp-openshift overlay and reduce SSE log noise#1245
Open
markturansky wants to merge 9 commits intoalphafrom
Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR transitions terminology from "ignition" to "start" across API specifications, documentation, and CLI commands. It updates container image tags to Changes
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/ambient-api-server/pkg/rbac/middleware.go`:
- Around line 133-143: The pathToAction function is returning literal actions
("token", "stop", "ignite", "ignition") that don't match the canonical RBAC
action names, causing valid requests to be denied; update pathToAction (and its
use of parts and last) to translate those endpoint-specific literals into the
seeded RBAC action constants or canonical names used by the permissions system
(e.g., map "token" -> the token-management action constant,
"start"/"stop"/"ignite"/"ignition" -> the appropriate lifecycle/manage action),
or call the central RBAC helper/enum when returning the action, ensuring exact
matches with the existing RBAC action set.
In `@components/ambient-cli/cmd/acpctl/session/send.go`:
- Around line 139-140: The switch branch treating "RUN_FINISHED" and "RUN_ERROR"
together returns nil, causing RUN_ERROR to exit 0 and skip the trailing-newline
cleanup; separate the cases so "RUN_FINISHED" still returns nil but "RUN_ERROR"
performs the newline cleanup (the same cleanup currently at the end of the
function) and then returns a non-nil error (e.g., fmt.Errorf("runner reported
RUN_ERROR") or include the runner's error message if available) so the process
exits with a failure status.
- Around line 126-137: Large TOOL_CALL_RESULT events can exceed bufio.Scanner's
64KiB token limit causing "token too long" and truncating streams; locate where
the scanner is created (scanner := bufio.NewScanner(stream)) and call
scanner.Buffer(...) to raise the max token size (e.g., set initial buffer and a
larger max like 1MB) so long tool outputs don't break the stream; ensure this
buffer setup happens immediately after creating the scanner used to read event
streams in send.go.
In `@components/manifests/overlays/mpp-openshift/ambient-api-server.yaml`:
- Around line 55-57: The migration initContainer (named "migration" using image
quay.io/ambient_code/vteam_api_server:latest) lacks a resources block; add a
resources: section under that initContainer with both requests and limits (e.g.,
requests.cpu, requests.memory, limits.cpu, limits.memory) so the pod meets
cluster admission and scheduling requirements—update the migration initContainer
spec to include appropriate CPU/memory values consistent with project
conventions.
In `@components/manifests/overlays/mpp-openshift/kustomization.yaml`:
- Around line 42-43: The kustomize images entry for
quay.io/ambient_code/vteam_ambient_mcp is a no-op because the deployment reads
the image from the MCP_IMAGE env var; remove the images: override or replace it
with a kustomize patch that updates the MCP_IMAGE env var on the target workload
(e.g., a strategicMerge or JSON6902 patch against the Deployment/StatefulSet to
set spec.template.spec.containers[].env where name == MCP_IMAGE), ensuring the
patch references the correct workload name and container so the runtime image is
actually changed.
In `@docs/internal/design/frontend-backend-migration-plan.md`:
- Around line 45-46: The docs show conflicting HTTP methods for start/stop
(Lines mapping `POST /projects/{p}/agentic-sessions/{s}/start` and `.../stop` to
StartSession/StopSession) while elsewhere adding `GET
/api/ambient/v1/sessions/{id}/start` and `/stop`; fix by making the contract
consistent: choose the state-changing method POST for StartSession and
StopSession and update all references (including the `GET` entries at the other
location) to use POST, or if GET must be kept, move the GET entries to a safe,
idempotent read-only variant and document why; update the documented endpoints
(`StartSession`, `StopSession`, and their `/api/ambient/v1/sessions/{id}/...`
mappings) to the chosen method so clients/proxies and caches behave correctly.
- Around line 123-134: The proxy example constructs the search filter by
interpolating user-supplied projectName/sessionName directly into the search
expression (e.g., in ListSessions), which enables filter injection; fix by
validating and sanitizing the input and by building the query using proper
encoding or structured parameters instead of string interpolation—for example,
URL-encode the projectName (use url.QueryEscape or the client’s query-params
API), or switch to passing a structured filter object/parameter to
ambientAPIClient.Get rather than embedding into "search="; apply the same change
to the other examples that interpolate sessionName/projectName.
🪄 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: CHILL
Plan: Pro
Run ID: d1f2c41e-824d-4d95-8f03-b70c6659afce
📒 Files selected for processing (14)
components/ambient-api-server/pkg/rbac/middleware.gocomponents/ambient-cli/cmd/acpctl/session/send.gocomponents/manifests/overlays/local-dev/ambient-api-server-route.yamlcomponents/manifests/overlays/mpp-openshift/ambient-api-server-route.yamlcomponents/manifests/overlays/mpp-openshift/ambient-api-server.yamlcomponents/manifests/overlays/mpp-openshift/ambient-control-plane.yamlcomponents/manifests/overlays/mpp-openshift/kustomization.yamlcomponents/manifests/overlays/production/ambient-api-server-route.yamlcomponents/runners/ambient-runner/ambient_runner/bridges/claude/grpc_transport.pycomponents/runners/ambient-runner/ambient_runner/endpoints/events.pycomponents/runners/ambient-runner/ambient_runner/observability.pycomponents/runners/ambient-runner/ambient_runner/platform/auth.pycomponents/runners/ambient-runner/ambient_runner/platform/utils.pydocs/internal/design/frontend-backend-migration-plan.md
Comment on lines
+133
to
+143
| func pathToAction(method, path string) string { | ||
| parts := strings.Split(strings.TrimPrefix(path, "/"), "/") | ||
| for i, p := range parts { | ||
| if p == "v1" && i+2 < len(parts) { | ||
| last := parts[len(parts)-1] | ||
| switch last { | ||
| case "token": | ||
| return "token" | ||
| case "start", "stop", "ignite", "ignition": | ||
| return last | ||
| } |
Contributor
There was a problem hiding this comment.
Path-derived actions are not compatible with current RBAC permissions
Line 139-Line 143 introduces action literals ("token", "stop", "ignite", "ignition") that do not exist in the canonical RBAC action set/seeded built-in permissions, while matching is exact. This can cause valid non-admin traffic to be denied on those endpoints.
Proposed fix
func pathToAction(method, path string) string {
parts := strings.Split(strings.TrimPrefix(path, "/"), "/")
for i, p := range parts {
if p == "v1" && i+2 < len(parts) {
last := parts[len(parts)-1]
switch last {
case "token":
- return "token"
- case "start", "stop", "ignite", "ignition":
- return last
+ return "fetch_token"
+ case "start":
+ return "start"
+ case "stop", "ignite", "ignition":
+ // Keep method-based mapping until these actions are defined and seeded.
+ return httpMethodToAction(method)
}
}
}
return httpMethodToAction(method)
}As per coding guidelines, "Flag only errors, security risks, or functionality-breaking problems."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func pathToAction(method, path string) string { | |
| parts := strings.Split(strings.TrimPrefix(path, "/"), "/") | |
| for i, p := range parts { | |
| if p == "v1" && i+2 < len(parts) { | |
| last := parts[len(parts)-1] | |
| switch last { | |
| case "token": | |
| return "token" | |
| case "start", "stop", "ignite", "ignition": | |
| return last | |
| } | |
| func pathToAction(method, path string) string { | |
| parts := strings.Split(strings.TrimPrefix(path, "/"), "/") | |
| for i, p := range parts { | |
| if p == "v1" && i+2 < len(parts) { | |
| last := parts[len(parts)-1] | |
| switch last { | |
| case "token": | |
| return "fetch_token" | |
| case "start": | |
| return "start" | |
| case "stop", "ignite", "ignition": | |
| // Keep method-based mapping until these actions are defined and seeded. | |
| return httpMethodToAction(method) | |
| } | |
| } | |
| } | |
| return httpMethodToAction(method) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-api-server/pkg/rbac/middleware.go` around lines 133 - 143,
The pathToAction function is returning literal actions ("token", "stop",
"ignite", "ignition") that don't match the canonical RBAC action names, causing
valid requests to be denied; update pathToAction (and its use of parts and last)
to translate those endpoint-specific literals into the seeded RBAC action
constants or canonical names used by the permissions system (e.g., map "token"
-> the token-management action constant, "start"/"stop"/"ignite"/"ignition" ->
the appropriate lifecycle/manage action), or call the central RBAC helper/enum
when returning the action, ensuring exact matches with the existing RBAC action
set.
Comment on lines
+126
to
+137
| case "TOOL_CALL_RESULT": | ||
| if evt.Content != "" { | ||
| var content string | ||
| if err := json.Unmarshal([]byte(evt.Content), &content); err != nil { | ||
| content = evt.Content | ||
| } | ||
| lines := strings.SplitN(strings.TrimSpace(content), "\n", 4) | ||
| preview := strings.Join(lines, " | ") | ||
| if len(lines) >= 4 { | ||
| preview += " ..." | ||
| } | ||
| fmt.Fprintf(out, "→ %s\n", preview) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the send.go file
fd -t f "send.go" components/ambient-cli/cmd/acpctl/session/
# Get line count to understand file size
wc -l components/ambient-cli/cmd/acpctl/session/send.go
# Read around line 80 to find scanner initialization
sed -n '70,95p' components/ambient-cli/cmd/acpctl/session/send.goRepository: ambient-code/platform
Length of output: 751
🏁 Script executed:
# Check Go's bufio.Scanner default limits
# Also read more of the send.go file to understand event flow and payload sizes
sed -n '126,150p' components/ambient-cli/cmd/acpctl/session/send.goRepository: ambient-code/platform
Length of output: 647
🌐 Web query:
Go bufio.Scanner default MaxTokenSize token limit
💡 Result:
In Go, bufio.Scanner’s default maximum token size is bufio.MaxScanTokenSize = 64 * 1024 bytes (64 KiB). If a scanned token (e.g., a line when using the default ScanLines split function) exceeds that limit, Scan() stops and Err() reports bufio.Scanner: token too long. [1][2]
You can raise the limit by calling scanner.Buffer(initialBuf, maxTokenSize) before scanning. [1]
🏁 Script executed:
# Check what types of events/payloads flow through TOOL_CALL_RESULT
# Look for context about event sizes or tool output handling
grep -n "TOOL_CALL" components/ambient-cli/cmd/acpctl/session/send.go
# Also check if there are any tests or examples showing typical payload sizes
fd -t f "test" components/ambient-cli/cmd/acpctl/session/ | head -5Repository: ambient-code/platform
Length of output: 125
🏁 Script executed:
# Check if there are runner/event definitions showing typical content sizes
# Look for event definition or payload structure documentation
find . -type f -name "*.go" | xargs grep -l "TOOL_CALL_RESULT" | head -5
# Understand the event structure better
grep -B5 -A10 "type.*evt" components/ambient-cli/cmd/acpctl/session/send.go | head -30Repository: ambient-code/platform
Length of output: 233
🏁 Script executed:
# Check messages.go to understand event structure
cat -n components/ambient-cli/cmd/acpctl/session/messages.go | head -60
# Also check the full event handling in send.go to understand content sizes
sed -n '95,110p' components/ambient-cli/cmd/acpctl/session/send.goRepository: ambient-code/platform
Length of output: 2688
🏁 Script executed:
# Verify that large tool outputs are a realistic scenario
# Check runner/provider code to understand typical tool output sizes
fd -t f "runner" . --type f --path "**/cmd/**" | head -3
# Also check if there are any large example outputs in tests/fixtures
find . -type f -name "*fixture*" -o -name "*example*" | head -5Repository: ambient-code/platform
Length of output: 507
🏁 Script executed:
# Look for runner implementation that generates TOOL_CALL_RESULT events
find . -type f -name "*.go" | xargs grep -l "TOOL_CALL_RESULT" | xargs grep -l "runner" | head -3
# Check if there are any actual runner implementations
find . -type f -name "runner*.go" | head -5
# See if event content is documented anywhere with size considerations
grep -r "64" components/ambient-cli --include="*.go" | grep -i "buffer\|scanner\|token"Repository: ambient-code/platform
Length of output: 716
🏁 Script executed:
# Check runner_types.go to understand the event structure and content field
sed -n '1,100p' components/backend/handlers/runner_types.go
# Also verify that the scanner.Err() handling will properly catch token-too-long errors
sed -n '145,152p' components/ambient-cli/cmd/acpctl/session/send.goRepository: ambient-code/platform
Length of output: 3631
🏁 Script executed:
# Check if there's any documented max payload size or guidance on expected tool output sizes
grep -r "Content" components/backend/handlers --include="*.go" -A2 -B2 | grep -i "size\|limit\|max"
# Also check the actual implementation to see if tool outputs are known to be large
find . -type f -name "*.go" -path "*runner*" | xargs grep -l "Content" | head -3Repository: ambient-code/platform
Length of output: 195
🏁 Script executed:
# Final verification: check if the error handling actually stops the stream gracefully
# and understand what happens when scanner.Err() returns token-too-long
sed -n '82,92p' components/ambient-cli/cmd/acpctl/session/send.go
# Also verify the stream is being closed properly on error
sed -n '70,82p' components/ambient-cli/cmd/acpctl/session/send.goRepository: ambient-code/platform
Length of output: 640
Large TOOL_CALL_RESULT events exceed the default scanner token limit.
bufio.Scanner defaults to a 64 KiB token limit. Tool outputs exceeding this will cause scanner.Scan() to fail with token too long, stopping the stream mid-response. Add a buffer configuration near line 80:
Fix
scanner := bufio.NewScanner(stream)
scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/ambient-cli/cmd/acpctl/session/send.go` around lines 126 - 137,
Large TOOL_CALL_RESULT events can exceed bufio.Scanner's 64KiB token limit
causing "token too long" and truncating streams; locate where the scanner is
created (scanner := bufio.NewScanner(stream)) and call scanner.Buffer(...) to
raise the max token size (e.g., set initial buffer and a larger max like 1MB) so
long tool outputs don't break the stream; ensure this buffer setup happens
immediately after creating the scanner used to read event streams in send.go.
Comment on lines
+123
to
+134
| ```go | ||
| // Example: ListSessions proxy | ||
| func ListSessions(c *gin.Context) { | ||
| projectName := c.Param("projectName") | ||
| token := extractUserToken(c) | ||
| resp, err := ambientAPIClient.Get( | ||
| fmt.Sprintf("/api/ambient/v1/sessions?search=project_id='%s'", projectName), | ||
| token, | ||
| ) | ||
| // forward response body and status code as-is | ||
| c.DataFromReader(resp.StatusCode, resp.ContentLength, "application/json", resp.Body, nil) | ||
| } |
Contributor
There was a problem hiding this comment.
Unsafe query construction in proxy examples enables filter injection.
The examples interpolate projectName/sessionName directly into search= expressions. If implemented this way, crafted values can alter server-side filtering (query injection), leading to wrong session/project resolution or unauthorized data exposure. Require strict escaping/encoding or a structured query API before forwarding.
As per coding guidelines, "Flag only errors, security risks, or functionality-breaking problems."
Also applies to: 144-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/internal/design/frontend-backend-migration-plan.md` around lines 123 -
134, The proxy example constructs the search filter by interpolating
user-supplied projectName/sessionName directly into the search expression (e.g.,
in ListSessions), which enables filter injection; fix by validating and
sanitizing the input and by building the query using proper encoding or
structured parameters instead of string interpolation—for example, URL-encode
the projectName (use url.QueryEscape or the client’s query-params API), or
switch to passing a structured filter object/parameter to ambientAPIClient.Get
rather than embedding into "search="; apply the same change to the other
examples that interpolate sessionName/projectName.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
components/ambient-api-server/pkg/rbac/middleware.go (1)
133-143:⚠️ Potential issue | 🟠 MajorPath-derived action values will fail RBAC matching for
/tokenand/stopLine 139 returns
"token"and Line 141 can return"stop", but action matching is exact (Line 110) and seeded permissions use"fetch_token"/"start"(no"token"or"stop"action incomponents/ambient-api-server/pkg/rbac/permissions.go). This can deny valid requests.Proposed fix
func pathToAction(method, path string) string { parts := strings.Split(strings.TrimPrefix(path, "/"), "/") for i, p := range parts { if p == "v1" && i+2 < len(parts) { last := parts[len(parts)-1] switch last { case "token": - return "token" + return string(ActionFetchToken) case "start", "stop": - return last + return string(ActionStart) } } } return httpMethodToAction(method) }As per coding guidelines, "Flag only errors, security risks, or functionality-breaking problems."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ambient-api-server/pkg/rbac/middleware.go` around lines 133 - 143, pathToAction currently returns "token" (and "stop") which won't match the exact action strings used by the RBAC checks and seeded permissions; update pathToAction so it returns the canonical action names used in components/ambient-api-server/pkg/rbac/permissions.go (e.g., map "token" -> "fetch_token") and keep "start"/"stop" consistent with the seeded permission names so the action string produced by pathToAction matches the RBAC exact-matching logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/ambient-api-server/openapi/openapi.yaml`:
- Around line 187-189: The handler's JSON field name mismatches the OpenAPI
model: change the struct field named StartingPrompt in start_handler.go to
StartPrompt and update its struct tag to `json:"start_prompt"`, then update the
code that assigns to the old field (the assignment currently setting
StartingPrompt) to assign to StartPrompt instead so responses serialize with
`start_prompt` as defined in the OpenAPI schema.
---
Duplicate comments:
In `@components/ambient-api-server/pkg/rbac/middleware.go`:
- Around line 133-143: pathToAction currently returns "token" (and "stop") which
won't match the exact action strings used by the RBAC checks and seeded
permissions; update pathToAction so it returns the canonical action names used
in components/ambient-api-server/pkg/rbac/permissions.go (e.g., map "token" ->
"fetch_token") and keep "start"/"stop" consistent with the seeded permission
names so the action string produced by pathToAction matches the RBAC
exact-matching logic.
🪄 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: CHILL
Plan: Pro
Run ID: 711ccaa3-2065-4d2b-8737-e3ac58ed9f59
⛔ Files ignored due to path filters (9)
components/ambient-api-server/pkg/api/openapi/api/openapi.yamlis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/InboxMessage.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/Project.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/Session.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/StartResponse.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_inbox_message.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_project.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_session.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_start_response.gois excluded by!**/pkg/api/openapi/**
📒 Files selected for processing (13)
components/ambient-api-server/openapi/openapi.inbox.yamlcomponents/ambient-api-server/openapi/openapi.projects.yamlcomponents/ambient-api-server/openapi/openapi.sessions.yamlcomponents/ambient-api-server/openapi/openapi.yamlcomponents/ambient-api-server/pkg/rbac/middleware.gocomponents/ambient-api-server/plugins/agents/start_handler.gocomponents/ambient-cli/cmd/acpctl/agent/cmd.gocomponents/ambient-cli/cmd/acpctl/apply/cmd.godocs/internal/design/ambient-model.guide.mddocs/internal/design/ambient-model.spec.mddocs/internal/design/control-plane.spec.mddocs/internal/design/frontend-to-api-status.mddocs/internal/design/runner.spec.md
✅ Files skipped from review due to trivial changes (10)
- components/ambient-cli/cmd/acpctl/apply/cmd.go
- components/ambient-api-server/openapi/openapi.sessions.yaml
- components/ambient-api-server/openapi/openapi.inbox.yaml
- docs/internal/design/control-plane.spec.md
- components/ambient-api-server/openapi/openapi.projects.yaml
- components/ambient-cli/cmd/acpctl/agent/cmd.go
- docs/internal/design/frontend-to-api-status.md
- docs/internal/design/ambient-model.guide.md
- docs/internal/design/ambient-model.spec.md
- docs/internal/design/runner.spec.md
markturansky
force-pushed
the
feat/RHOAIENG-56712-mcp-sidecar-overlay-sse-logging
branch
from
353c8a7 to
48c9473
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/internal/design/ambient-model.spec.md`:
- Around line 299-301: The docs and OpenAPI disagree on the preview endpoint
path (docs show GET /projects/{id}/agents/{agent_id}/start while
components/ambient-api-server/openapi/openapi.agents.yaml exposes GET
.../ignition); pick one canonical path and make both sources match: either
change the OpenAPI path in openapi.agents.yaml (around the agents paths) from
/ignition to /start and update any operationId/description/servers accordingly,
or update docs docs/internal/design/ambient-model.spec.md (and any CLI mapping
references like acpctl agent start-preview) to use /ignition; ensure the CLI
mapping examples (acpctl agent start-preview) and any related path references
(start-preview, ignition) are updated consistently and run the OpenAPI validator
to confirm no broken refs.
🪄 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: CHILL
Plan: Pro
Run ID: dbd73118-33f9-42fe-b65f-650daa2abc74
⛔ Files ignored due to path filters (9)
components/ambient-api-server/pkg/api/openapi/api/openapi.yamlis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/InboxMessage.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/Project.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/Session.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/StartResponse.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_inbox_message.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_project.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_session.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_start_response.gois excluded by!**/pkg/api/openapi/**
📒 Files selected for processing (13)
components/ambient-api-server/openapi/openapi.inbox.yamlcomponents/ambient-api-server/openapi/openapi.projects.yamlcomponents/ambient-api-server/openapi/openapi.sessions.yamlcomponents/ambient-api-server/openapi/openapi.yamlcomponents/ambient-api-server/pkg/rbac/middleware.gocomponents/ambient-api-server/plugins/agents/start_handler.gocomponents/ambient-cli/cmd/acpctl/agent/cmd.gocomponents/ambient-cli/cmd/acpctl/apply/cmd.godocs/internal/design/ambient-model.guide.mddocs/internal/design/ambient-model.spec.mddocs/internal/design/control-plane.spec.mddocs/internal/design/frontend-to-api-status.mddocs/internal/design/runner.spec.md
✅ Files skipped from review due to trivial changes (6)
- components/ambient-api-server/openapi/openapi.inbox.yaml
- components/ambient-api-server/openapi/openapi.projects.yaml
- docs/internal/design/control-plane.spec.md
- components/ambient-cli/cmd/acpctl/apply/cmd.go
- components/ambient-cli/cmd/acpctl/agent/cmd.go
- components/ambient-api-server/openapi/openapi.sessions.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/internal/design/frontend-to-api-status.md
- components/ambient-api-server/openapi/openapi.yaml
- components/ambient-api-server/pkg/rbac/middleware.go
- docs/internal/design/ambient-model.guide.md
…ovements
- runner: retry credential fetch with fresh CP token on 401 instead of
aborting the turn; adds refresh_bot_token() to utils.py and wires it
into _fetch_credential() for both the direct BOT_TOKEN and caller-token
fallback paths
- rbac: fix pathToAction() so GET /credentials/{id}/token resolves to
action "token" (matching credential:token-reader role) instead of "read"
- cli: improve acpctl session send -f output with reasoning accumulation
([thinking] blocks), tool call annotations ([ToolName] → result), and
proper newline handling for streamed text
- routes: add haproxy.router.openshift.io/timeout: 10m to all
ambient-api-server Routes (production, mpp-openshift, local-dev) to
prevent 504s on long-running SSE streams
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <noreply@anthropic.com>
…rror message The _retry_with_fresh_bot_token helper was hardcoding HTTP 401 in the PermissionError message regardless of the actual response code. This broke test_raises_permission_error_on_403_without_caller_token which expects "authentication failed with HTTP 403". Now the original status code is passed through and the retry error message reflects the actual HTTP code from both the initial and retry attempts. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…_turn import - auth.py: don't retry with fresh CP token when caller-token BOT_TOKEN fallback fails; preserve that path's original error message - observability.py: guard claude_agent_sdk import in end_turn with try/except so Langfuse generation.update() is still called when the SDK is not installed 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…ging The stream was held open indefinitely because the server does not close the SSE connection after RUN_FINISHED. Now the CLI exits cleanly when either terminal event is received. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…P token When populate_runtime_credentials() raises PermissionError (expired BOT_TOKEN), the turn was silently dropped and the user message lost. Now _handle_user_message catches PermissionError, calls refresh_bot_token() to get a fresh CP token, and retries the full bridge.run() turn once — so the message is delivered to the runner after re-authentication instead of being discarded. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…rlay and reduce SSE log noise - Add MCP_IMAGE and MCP_API_SERVER_URL env vars to control-plane deployment, enabling the ambient-mcp sidecar for all new session pods - Add vteam_ambient_mcp:latest to kustomization image entries - Update all mgt-001 image tags to latest in mpp-openshift overlay - Downgrade SSE TAP yielding-event log from INFO to DEBUG to reduce log spam 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…Proxy route - Replace w.(http.Flusher) type assertion in StreamRunnerEvents with http.NewResponseController(w).Flush() to traverse rh-trex-ai's metricsResponseWrapper.Unwrap() chain (Go 1.20+) - Initialize SDK streamingClient with DisableCompression=true to prevent gzip reframing of SSE byte stream - Add haproxy.router.openshift.io/timeout-tunnel: 10m to api and grpc routes so HAProxy does not buffer long-lived SSE connections 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Rename ignite_handler.go → start_handler.go - Remove 'ignite', 'ignition' compat shim from RBAC middleware - Rename ignition_prompt → start_prompt in OpenAPI schemas (source + bundled) - Rename ignitionCmd/ignitionArgs → startPreviewCmd/startPreviewArgs in CLI - Update all comments, doc strings, and design docs to use 'start' terminology - No shim layer — no users to support 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- send.go: RUN_ERROR now returns an error (exit non-zero) instead of nil; RUN_FINISHED and RUN_ERROR both flush trailing newline before returning - ambient-api-server.yaml: add resource requests/limits to migration initContainer (cpu: 50m-500m, memory: 128Mi-512Mi) 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
markturansky
force-pushed
the
feat/RHOAIENG-56712-mcp-sidecar-overlay-sse-logging
branch
from
48c9473 to
7d33f1a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
docs/internal/design/frontend-backend-migration-plan.md (2)
45-46:⚠️ Potential issue | 🔴 CriticalStart/stop method contract is internally inconsistent (POST vs GET).
Lines 45-46 map start/stop to
POST, but Line 213 asks to addGETstart/stop endpoints. This conflict breaks client/server integration and cache/proxy behavior for state-changing operations.As per coding guidelines, "Flag only errors, security risks, or functionality-breaking problems."
Also applies to: 213-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/internal/design/frontend-backend-migration-plan.md` around lines 45 - 46, The Start/Stop endpoints are inconsistent: the table maps `POST /projects/{p}/agentic-sessions/{s}/start` and `.../stop` to StartSession/StopSession but elsewhere (line ~213) you proposed `GET` variants; change the GET entries to POST so all references use POST for StartSession and StopSession, update any client/docs that reference the GET routes, and ensure any server handlers (StartSession/StopSession) and cache/proxy expectations are aligned with POST (i.e., treat as state-changing, no caching).
123-134:⚠️ Potential issue | 🔴 CriticalUnsafe query construction enables filter injection.
Lines 129 and 146 interpolate
projectName/sessionNamedirectly intosearch=expressions. Crafted values can alter server-side filtering (query injection), leading to wrong session/project resolution or unauthorized data exposure. Require strict escaping/encoding or a structured query API before forwarding.As per coding guidelines, "Flag only errors, security risks, or functionality-breaking problems."
Also applies to: 144-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/internal/design/frontend-backend-migration-plan.md` around lines 123 - 134, The handler builds the `search=` filter by interpolating user-controlled `projectName`/`sessionName` (in ListSessions and the similar handler referenced) which allows query/filter injection; fix by constructing the request using proper URL encoding or structured query parameters instead of string interpolation—e.g., url-encode `projectName`/`sessionName` via net/url functions or build the query with url.Values and pass that to ambientAPIClient.Get; ensure you use the encoded value when calling ambientAPIClient.Get (same spot where `projectName` is used) and validate/normalize inputs from extractUserToken as needed before forwarding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/runners/ambient-runner/ambient_runner/bridges/claude/grpc_transport.py`:
- Around line 332-341: The retry path reuses the same GRPCMessageWriter instance
causing stale buffered payloads to be emitted on subsequent errors; in the
except block around _run_once(), recreate/reset the GRPCMessageWriter used by
_run_once (the local variable writer) before retrying or before calling
_synthesize_run_error so any previous snapshots are cleared—i.e., replace or
reinstantiate writer (or call its clear/reset API if available) prior to retry
and before invoking _synthesize_run_error to ensure no stale assistant text
persists.
- Around line 324-326: The call to the synchronous refresh_bot_token() inside
the async handler blocks the event loop; change it to run in a thread executor
by getting the running event loop (asyncio.get_running_loop()) and awaiting
loop.run_in_executor(None, refresh_bot_token) so the blocking retries/sleeps
don't stall other coroutines (keep the existing try/except and refresh_exc
handling), and add an asyncio import if missing.
---
Duplicate comments:
In `@docs/internal/design/frontend-backend-migration-plan.md`:
- Around line 45-46: The Start/Stop endpoints are inconsistent: the table maps
`POST /projects/{p}/agentic-sessions/{s}/start` and `.../stop` to
StartSession/StopSession but elsewhere (line ~213) you proposed `GET` variants;
change the GET entries to POST so all references use POST for StartSession and
StopSession, update any client/docs that reference the GET routes, and ensure
any server handlers (StartSession/StopSession) and cache/proxy expectations are
aligned with POST (i.e., treat as state-changing, no caching).
- Around line 123-134: The handler builds the `search=` filter by interpolating
user-controlled `projectName`/`sessionName` (in ListSessions and the similar
handler referenced) which allows query/filter injection; fix by constructing the
request using proper URL encoding or structured query parameters instead of
string interpolation—e.g., url-encode `projectName`/`sessionName` via net/url
functions or build the query with url.Values and pass that to
ambientAPIClient.Get; ensure you use the encoded value when calling
ambientAPIClient.Get (same spot where `projectName` is used) and
validate/normalize inputs from extractUserToken as needed before forwarding.
🪄 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: CHILL
Plan: Pro
Run ID: a7fc99cc-5f71-4a40-8d60-5a9d4770ec8e
⛔ Files ignored due to path filters (9)
components/ambient-api-server/pkg/api/openapi/api/openapi.yamlis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/InboxMessage.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/Project.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/Session.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/StartResponse.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_inbox_message.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_project.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_session.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_start_response.gois excluded by!**/pkg/api/openapi/**
📒 Files selected for processing (23)
components/ambient-api-server/openapi/openapi.inbox.yamlcomponents/ambient-api-server/openapi/openapi.projects.yamlcomponents/ambient-api-server/openapi/openapi.sessions.yamlcomponents/ambient-api-server/openapi/openapi.yamlcomponents/ambient-api-server/pkg/rbac/middleware.gocomponents/ambient-api-server/plugins/agents/start_handler.gocomponents/ambient-api-server/plugins/sessions/handler.gocomponents/ambient-cli/cmd/acpctl/agent/cmd.gocomponents/ambient-cli/cmd/acpctl/apply/cmd.gocomponents/ambient-cli/cmd/acpctl/session/send.gocomponents/ambient-sdk/go-sdk/client/client.gocomponents/manifests/overlays/mpp-openshift/ambient-api-server-route.yamlcomponents/manifests/overlays/mpp-openshift/ambient-api-server.yamlcomponents/manifests/overlays/mpp-openshift/ambient-control-plane.yamlcomponents/manifests/overlays/mpp-openshift/kustomization.yamlcomponents/runners/ambient-runner/ambient_runner/bridges/claude/grpc_transport.pycomponents/runners/ambient-runner/ambient_runner/endpoints/events.pydocs/internal/design/ambient-model.guide.mddocs/internal/design/ambient-model.spec.mddocs/internal/design/control-plane.spec.mddocs/internal/design/frontend-backend-migration-plan.mddocs/internal/design/frontend-to-api-status.mddocs/internal/design/runner.spec.md
✅ Files skipped from review due to trivial changes (10)
- components/ambient-api-server/openapi/openapi.inbox.yaml
- components/runners/ambient-runner/ambient_runner/endpoints/events.py
- components/ambient-api-server/openapi/openapi.projects.yaml
- docs/internal/design/frontend-to-api-status.md
- components/ambient-cli/cmd/acpctl/apply/cmd.go
- components/ambient-cli/cmd/acpctl/agent/cmd.go
- components/manifests/overlays/mpp-openshift/ambient-api-server-route.yaml
- components/ambient-api-server/plugins/sessions/handler.go
- docs/internal/design/ambient-model.spec.md
- components/manifests/overlays/mpp-openshift/kustomization.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- components/ambient-api-server/openapi/openapi.yaml
- components/ambient-api-server/openapi/openapi.sessions.yaml
- components/ambient-cli/cmd/acpctl/session/send.go
- docs/internal/design/ambient-model.guide.md
- components/manifests/overlays/mpp-openshift/ambient-control-plane.yaml
- components/ambient-api-server/pkg/rbac/middleware.go
- docs/internal/design/control-plane.spec.md
Comment on lines
+324
to
+326
| from ambient_runner.platform.utils import refresh_bot_token | ||
| refresh_bot_token() | ||
| except Exception as refresh_exc: |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify blocking call path in async handler.
rg -n -C3 'except PermissionError|refresh_bot_token\(' components/runners/ambient-runner/ambient_runner/bridges/claude/grpc_transport.py
rg -n -C4 'def refresh_bot_token|_fetch_token_from_cp' components/runners/ambient-runner/ambient_runner/platform/utils.py
rg -n -C5 'def _fetch_token_from_cp|time.sleep\(' components/runners/ambient-runner/ambient_runner/_grpc_client.pyRepository: ambient-code/platform
Length of output: 2335
🏁 Script executed:
rg -n -A2 'async def _handle_user_message' components/runners/ambient-runner/ambient_runner/bridges/claude/grpc_transport.pyRepository: ambient-code/platform
Length of output: 276
Avoid blocking the event loop during token refresh.
refresh_bot_token() is synchronous and performs retry/backoff sleeps via time.sleep(); calling it directly in this async handler blocks the event loop and stalls concurrent streams (SSE listeners, heartbeats).
Offload the call using run_in_executor():
Fix
try:
from ambient_runner.platform.utils import refresh_bot_token
- refresh_bot_token()
+ await asyncio.get_running_loop().run_in_executor(
+ None, refresh_bot_token
+ )
except Exception as refresh_exc:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| from ambient_runner.platform.utils import refresh_bot_token | |
| refresh_bot_token() | |
| except Exception as refresh_exc: | |
| from ambient_runner.platform.utils import refresh_bot_token | |
| await asyncio.get_running_loop().run_in_executor( | |
| None, refresh_bot_token | |
| ) | |
| except Exception as refresh_exc: |
🧰 Tools
🪛 Ruff (0.15.9)
[warning] 326-326: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/runners/ambient-runner/ambient_runner/bridges/claude/grpc_transport.py`
around lines 324 - 326, The call to the synchronous refresh_bot_token() inside
the async handler blocks the event loop; change it to run in a thread executor
by getting the running event loop (asyncio.get_running_loop()) and awaiting
loop.run_in_executor(None, refresh_bot_token) so the blocking retries/sleeps
don't stall other coroutines (keep the existing try/except and refresh_exc
handling), and add an asyncio import if missing.
Comment on lines
+332
to
+341
| try: | ||
| await _run_once() | ||
| except Exception as retry_exc: | ||
| logger.error( | ||
| "[GRPC LISTENER] bridge.run() failed after token refresh: session=%s error=%s", | ||
| self._session_id, | ||
| retry_exc, | ||
| exc_info=True, | ||
| ) | ||
| _synthesize_run_error(thread_id, str(retry_exc), active_streams, writer) |
Contributor
There was a problem hiding this comment.
Reset GRPCMessageWriter before retry to avoid stale payload persistence.
The retry path reuses the same writer; if attempt 1 buffered a snapshot and attempt 2 fails early, synthesized RUN_ERROR can push stale assistant text from attempt 1.
Proposed fix
try:
+ writer = GRPCMessageWriter(
+ session_id=self._session_id,
+ run_id=run_id,
+ grpc_client=self._grpc_client,
+ )
await _run_once()
except Exception as retry_exc:
logger.error(
"[GRPC LISTENER] bridge.run() failed after token refresh: session=%s error=%s",As per coding guidelines, **/*: "Flag only errors, security risks, or functionality-breaking problems."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| await _run_once() | |
| except Exception as retry_exc: | |
| logger.error( | |
| "[GRPC LISTENER] bridge.run() failed after token refresh: session=%s error=%s", | |
| self._session_id, | |
| retry_exc, | |
| exc_info=True, | |
| ) | |
| _synthesize_run_error(thread_id, str(retry_exc), active_streams, writer) | |
| try: | |
| writer = GRPCMessageWriter( | |
| session_id=self._session_id, | |
| run_id=run_id, | |
| grpc_client=self._grpc_client, | |
| ) | |
| await _run_once() | |
| except Exception as retry_exc: | |
| logger.error( | |
| "[GRPC LISTENER] bridge.run() failed after token refresh: session=%s error=%s", | |
| self._session_id, | |
| retry_exc, | |
| exc_info=True, | |
| ) | |
| _synthesize_run_error(thread_id, str(retry_exc), active_streams, writer) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/runners/ambient-runner/ambient_runner/bridges/claude/grpc_transport.py`
around lines 332 - 341, The retry path reuses the same GRPCMessageWriter
instance causing stale buffered payloads to be emitted on subsequent errors; in
the except block around _run_once(), recreate/reset the GRPCMessageWriter used
by _run_once (the local variable writer) before retrying or before calling
_synthesize_run_error so any previous snapshots are cleared—i.e., replace or
reinstantiate writer (or call its clear/reset API if available) prior to retry
and before invoking _synthesize_run_error to ensure no stale assistant text
persists.
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.
Summary
MCP_IMAGEandMCP_API_SERVER_URLenv vars to the control-plane deployment in thempp-openshiftoverlay, enabling theambient-mcpsidecar for all new session pods (the CP already has full sidecar wiring inbuildMCPSidecar()— this was the only missing config)vteam_ambient_mcp:latestto kustomization image entriesmgt-001image tags tolatestacross the overlay (vteam_api_server,vteam_control_plane,vteam_claude_runner)[SSE TAP] Yielding eventlog fromINFOtoDEBUGinambient_runner/endpoints/events.pyto eliminate per-event log spamTest plan
ambient-mcpsidecar containerAMBIENT_MCP_URL=http://localhost:8090andbuild_mcp_servers()activates the ambient SSE entryRelates to: RHOAIENG-56712
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
API Updates
ignition_prompttostart_prompt.Chores