feat(public-api): complete session API surface for mcp-acp integration#807
feat(public-api): complete session API surface for mcp-acp integration#807
Conversation
Add PATCH, logs, transcript, and metrics endpoints to the public API so
mcp-acp can operate end-to-end with API keys. Enrich SessionResponse DTO
with displayName, repos, labels, and timeout fields.
Backend changes:
- Add GET /api/.../logs (streaming with 10MB cap, pod log via K8s API)
- Add GET /api/.../metrics (session CR status + usage annotations)
- Fix PatchSession to delete annotations set to nil instead of storing
empty strings
Public API changes:
- Add PATCH /v1/sessions/:id with smart routing (start/stop/update/labels)
- Add GET /v1/sessions/:id/{logs,transcript,metrics} proxy handlers
- Forward labelSelector query params in ListSessions
- Forward displayName in CreateSession
- Validate label keys against reserved internal prefixes on write
- Fix labels-to-annotations mapping on create path for write/read symmetry
- Label PATCH returns full session DTO (follow-up GET for consistency)
Other:
- Fix setup-vertex-kind.sh to restart backend-api after ConfigMap patch
- Add IMAGE_PUBLIC_API substitution in e2e deploy script
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MCP Public API Update — E2E Test PlanResult: 24/24 tests passed Bugs found and fixed during testing:
Initial Setup1. Verify port-forward to public API
2. Create service account with RBAC for testing
3. Generate bearer token
4. Ensure backend-api has Vertex AI configThe
5. Verify token works with public API
Session CRUD — Enriched Fields6. Create a session with displayName, model, and task
7. Get the created session — verify enriched DTO
8. List sessions — verify enriched fields in list
PATCH — Smart Routing9. PATCH stop session (
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…y comments - Use unstructured.NestedString/NestedFloat64 in sessions_metrics.go instead of direct type assertions - Replace raw JSON bind error with generic "Invalid request body" in PatchSession - Add safety comments explaining why raw query forwarding is safe in ListSessions, GetSessionTranscript, and GetSessionLogs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review ResponseThanks for the thorough review. Here's what I addressed and what I'm pushing back on: Fixed (e29b679)Item 1 — Unstructured helpers in Item 5 — Raw JSON bind error: Replaced Item 6 — Query string forwarding comments: Added safety comments on all three Not changingItem 2 — Log injection sanitization: No existing handler in Item 3 — Item 4 — |
This comment has been minimized.
This comment has been minimized.
…ments - Add kubectl.kubernetes.io/ and meta.kubernetes.io/ to internalLabelPrefixes to prevent leaking last-applied-configuration - Align sessions_metrics.go auth check to use k8sClt for consistency with sessions_logs.go - Refine query forwarding comments to note backend validates params Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Response (iteration 2)Fixed (44f9dc9)Item 1 — Auth check consistency: Changed Item 5 — Item 6 — Comment wording: Updated to "Forwarded verbatim to backend; backend enforces its own validation and RBAC via the user's token." Not changingItem 2 — Labels silently dropped on CreateSession: This was already fixed and E2E validated. The backend Item 3 — Hardcoded Item 4 — Fallback response in Item 7 — Usage annotations as strings: By design. K8s annotations are always |
This comment has been minimized.
This comment has been minimized.
|
still needs review to be addressed |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…m into andalton/mcp-update
Review Response (iteration 3)Fixed (b73fded): Sanitized |
|
Claude Code Review SummaryPR #807 completes the public API session surface (PATCH, logs, transcript, metrics endpoints), enriches the Issues by SeverityBlocker IssuesNone. Critical IssuesNone. Major Issues1.
c.Header("Content-Type", "text/plain; charset=utf-8")
c.Status(http.StatusOK)
if _, err := io.Copy(c.Writer, resp.Body); err != nil {
log.Printf("GetSessionLogs: error streaming backend response for %s: %v", sessionID, err)
}Minor Issues1. Backend
2.
3.
Positive Highlights
Recommendations
Generated with Claude Code |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Response (iteration 3)Fixed (11813e0)Major — Public-API Minor 3 — Not changingMinor 1 — 200 vs 204 for missing pod: Changing the HTTP status code is a behavioral change that could break existing callers (MCP clients, CLI). The current 200-with-empty-body is documented and semantically valid — "here are the logs, there are none yet." A 204 would also require callers to handle an additional status code for a non-error case. Minor 2 — Two round-trips for label PATCH: The reviewer acknowledges this is intentional. The PATCH endpoint doesn't return a full session DTO, so the follow-up GET is necessary to give callers a consistent |
This comment has been minimized.
This comment has been minimized.
…d session ID to label fallback Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude Code Review Summary PR 807 adds PATCH, logs, transcript, and metrics endpoints to the public-API, enriching SessionResponse DTO and fixing label write/read symmetry. User-token auth is correctly enforced, log-injection prevention is applied consistently, reserved-prefix validation guards annotation writes, and test coverage is thorough. No blocker or critical issues were found. Issues by Severity BLOCKER: None. CRITICAL: None. MAJOR ISSUES
MINOR ISSUES
POSITIVE HIGHLIGHTS
RECOMMENDATIONS
|
…stamp, add audit logging - GetSessionLogs now checks session CR exists before pod log access (returns 404 for unknown sessions) - Skip durationSeconds/completionTime when completionTime fails RFC3339 parse - Log IsForbidden attempts in both backend handlers for security auditing - Add sessionID context to all patch error log messages - Document follow-up GET race window in patchSessionLabels Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@components/backend/handlers/sessions.go`:
- Around line 1121-1125: The current annotation assignment (anns[k] = v) accepts
arbitrary JSON types and empty strings which cause Kubernetes API errors; update
the session PATCH handler to validate each annotation value before assignment:
ensure v is a string type, treat empty-string ("") as nil/delete (call
delete(anns, k)), and if v is a non-string return a 400 Bad Request with a clear
validation error; keep using the same variables (anns, k, v) and ensure the
validated string value is assigned to anns[k] only after these checks so the
subsequent Kubernetes Update call never receives non-string annotation values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4ce53b79-ede4-4d98-ad13-5899bf591a2d
📒 Files selected for processing (1)
components/backend/handlers/sessions.go
| if v == nil { | ||
| delete(anns, k) | ||
| } else { | ||
| anns[k] = v | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current PatchSession behavior writes arbitrary annotation value types and lacks type validation.
rg -n -C4 'if annsPatch, ok := metaPatch\["annotations"\]\.\(map\[string\]interface\{\}\); ok \{|for k, v := range annsPatch|anns\[k\] = v|v == nil' components/backend/handlers/sessions.go
# Check whether tests cover invalid annotation value types for PatchSession.
rg -n -C3 --type go 'PatchSession|annotations|null|Invalid request body|must be string'Repository: ambient-code/platform
Length of output: 50378
🏁 Script executed:
# Check what happens after annotations are set - look for error handling
rg -A10 'SetNestedMap.*annotations' components/backend/handlers/sessions.go
# Search for any validation of annotation values in PatchSession or related code
rg -B5 -A5 'annotation.*string|string.*annotation' components/backend/handlers/sessions.go | head -50
# Check the types to confirm Annotations is defined as map[string]string
rg -A3 'type.*Session.*struct' components/backend/types/session.goRepository: ambient-code/platform
Length of output: 4168
🏁 Script executed:
# Look for tests that might be patching annotations with non-string values
rg -n 'annotations.*:.*[0-9]|annotations.*:.*true|annotations.*:.*false|annotations.*:.*{' components/public-api/handlers/sessions_patch_test.go components/backend/handlers/sessions_test.go
# Check if there's any type assertion or validation before annotation assignment
rg -B3 -A3 'for k, v := range annsPatch' components/backend/handlers/sessions.goRepository: ambient-code/platform
Length of output: 465
🏁 Script executed:
# Verify Kubernetes annotation constraints - check if there's validation elsewhere
rg -n 'annotation.*string|string.*annotation|map\[string\]string' components/backend/handlers/sessions.go | head -20
# Check what unstructured.SetNestedMap does with non-string values in comments or tests
rg -i 'SetNestedMap.*annotation|annotation.*SetNestedMap' --type goRepository: ambient-code/platform
Length of output: 1098
🏁 Script executed:
# Check if errors from SetNestedMap are actually ignored
rg -B5 'unstructured\.SetNestedMap.*annotations' components/backend/handlers/sessions.go | grep -A5 'SetNestedMap'
# Look at error handling around the Update call to see if annotation errors propagate
rg -B2 -A8 'k8sDyn\.Resource.*Update.*item' components/backend/handlers/sessions.go | head -40
# Search for how the backend type Annotations (map[string]string) is used vs actual annotation storage
rg -n 'Annotations.*map\[string\]string' components/backend/types/session.go
# Check if there are any tests for patching with invalid annotation types
rg -A20 'TestE2E_PatchSession|func.*Patch.*Session' components/public-api/handlers/sessions_patch_test.go | grep -i 'annotation\|invalid\|string\|number\|bool'Repository: ambient-code/platform
Length of output: 3318
🏁 Script executed:
# Look at the session DTO in public-api to see type consistency
rg -B2 -A2 'Annotations' components/public-api/types/dto.go
# Check if validation happens when binding the PATCH request
rg -B10 -A5 'ShouldBindJSON.*patch\|patch.*ShouldBindJSON' components/backend/handlers/sessions.go
# See if empty string annotations are tested anywhere
rg -n '"".*annotation|annotation.*""' components/public-api/handlers/sessions_patch_test.go components/backend/handlers/sessions_test.goRepository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Check Kubernetes unstructured documentation and usage to understand what happens with non-string values
rg -C3 'unstructured\.' components/backend/handlers/sessions.go | head -60Repository: ambient-code/platform
Length of output: 2077
🏁 Script executed:
# Check if there are any integration tests or actual test data with non-string annotation values
rg -n 'annotations.*:' components/public-api/handlers/sessions_patch_test.go -A2 -B2
# Specifically look for what annotation values are being tested
rg '"annotations"' components/public-api/handlers/sessions_patch_test.go -A5 -B2Repository: ambient-code/platform
Length of output: 1655
🏁 Script executed:
# Check what the backend returns in PatchSession response - verify if it successfully reflects non-string values
rg -A3 'PatchSession.*200\|StatusOK.*annotation' components/public-api/handlers/sessions_patch_test.go
# Look at the full PatchSession test to understand what values are tested
sed -n '150,222p' components/public-api/handlers/sessions_patch_test.goRepository: ambient-code/platform
Length of output: 2638
🏁 Script executed:
# Check if there's any documentation or handling of what types annotations should accept
rg -i 'kubernetes.*annotation|annotation.*kubernetes|string.*only|must be string' --type go | head -20Repository: ambient-code/platform
Length of output: 741
🏁 Script executed:
# See if the backend handler has any imports that might indicate type validation
rg -n 'import\|^import' components/backend/handlers/sessions.go | head -10Repository: ambient-code/platform
Length of output: 47
Validate annotation values before assignment to avoid 500s on invalid PATCH payloads.
At line 1124, anns[k] = v accepts arbitrary JSON types without validation. Kubernetes annotations must be strings; non-string values (number, boolean, object, array) are rejected by the Kubernetes API during the subsequent Update call and currently surface as 500 errors instead of 400. Additionally, empty-string annotation values are not handled and persist rather than being deleted, contrary to Kubernetes semantics.
💡 Suggested fix
for k, v := range annsPatch {
- if v == nil {
- delete(anns, k)
- } else {
- anns[k] = v
- }
+ switch vv := v.(type) {
+ case nil:
+ delete(anns, k)
+ case string:
+ // Treat empty string as delete to avoid retaining "removed" annotations.
+ if strings.TrimSpace(vv) == "" {
+ delete(anns, k)
+ continue
+ }
+ anns[k] = vv
+ default:
+ c.JSON(http.StatusBadRequest, gin.H{
+ "error": fmt.Sprintf("Invalid annotation value for key %q: must be string or null", k),
+ })
+ return
+ }
}📝 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.
| if v == nil { | |
| delete(anns, k) | |
| } else { | |
| anns[k] = v | |
| } | |
| for k, v := range annsPatch { | |
| switch vv := v.(type) { | |
| case nil: | |
| delete(anns, k) | |
| case string: | |
| // Treat empty string as delete to avoid retaining "removed" annotations. | |
| if strings.TrimSpace(vv) == "" { | |
| delete(anns, k) | |
| continue | |
| } | |
| anns[k] = vv | |
| default: | |
| c.JSON(http.StatusBadRequest, gin.H{ | |
| "error": fmt.Sprintf("Invalid annotation value for key %q: must be string or null", k), | |
| }) | |
| return | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/sessions.go` around lines 1121 - 1125, The
current annotation assignment (anns[k] = v) accepts arbitrary JSON types and
empty strings which cause Kubernetes API errors; update the session PATCH
handler to validate each annotation value before assignment: ensure v is a
string type, treat empty-string ("") as nil/delete (call delete(anns, k)), and
if v is a non-string return a 400 Bad Request with a clear validation error;
keep using the same variables (anns, k, v) and ensure the validated string value
is assigned to anns[k] only after these checks so the subsequent Kubernetes
Update call never receives non-string annotation values.
There was a problem hiding this comment.
Fixed in 77fe084. Added a type switch to validate annotation values before assignment:
nil→ deletes the annotation key (existing behavior)string→ assigns the value- anything else → returns 400 Bad Request with a descriptive error
This prevents non-string JSON types (numbers, booleans, objects, arrays) from reaching the Kubernetes API and causing 500 errors.
Not adopted: the suggestion to treat empty strings as deletes. Empty-string annotations are valid in Kubernetes (kubectl annotate ... key="" is allowed), and silently converting them to deletes could break callers who intentionally set empty values. If a caller wants to remove an annotation, they should send null.
…00 errors Kubernetes annotations must be map[string]string, but the PATCH handler accepted arbitrary JSON types (numbers, booleans, objects, arrays) and passed them through to the K8s API, which rejected them with a 500. Use a type switch to validate each value: allow string and nil (delete), reject all other types with a 400 Bad Request. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Queue Status
Action needed: Fix failing CI checks
|
2 participants
Summary
SessionResponseDTO withdisplayName,repos,labels, andtimeoutfieldssetup-vertex-kind.shto restartbackend-apiafter ConfigMap patchTest plan
🤖 Generated with Claude Code