-
Notifications
You must be signed in to change notification settings - Fork 85
feat(public-api): complete session API surface for mcp-acp integration #807
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
adalton
wants to merge
54
commits into
main
Choose a base branch
from
andalton/mcp-update
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 22 commits
Commits
Show all changes
54 commits
Select commit
Hold shift + click to select a range
f9948f8
feat(public-api): complete session API surface for mcp-acp integration
adalton 2c1d28b
Merge remote-tracking branch 'origin/main' into andalton/mcp-update
adalton 080b205
style: apply gofmt formatting
adalton e29b679
fix: address review feedback — unstructured helpers, bind error, quer…
adalton 44f9dc9
fix: filter kubectl/meta K8s prefixes, align auth pattern, refine com…
adalton 2408a9b
Merge branch 'main' into andalton/mcp-update
jeremyeder b73fded
fix: sanitize sessionName param in logs/metrics handlers
adalton 5df0c94
Merge remote-tracking branch 'origin/main' into andalton/mcp-update
adalton 0f39434
Merge branch 'andalton/mcp-update' of github.com:ambient-code/platfor…
adalton 11813e0
fix: stream logs in public-api proxy, clarify SanitizeForLog usage
adalton 44a8a09
fix: handle K8s 403, stream transcript, align update status codes, ad…
adalton a4df45e
fix: add project/session context to error log messages
adalton ee74c91
fix: verify session CR before log retrieval, fix duration on bad time…
adalton ca25630
fix: validate query params before session CR check in GetSessionLogs
adalton 4e29939
Merge remote-tracking branch 'origin/main' into andalton/mcp-update
adalton e9c26ef
Merge branch 'main' into andalton/mcp-update
adalton b6ecde0
fix: address code review feedback on public-api session endpoints
adalton e912364
Merge branch 'main' into andalton/mcp-update
adalton dad993e
Merge branch 'main' into andalton/mcp-update
adalton 55f1123
Merge branch 'main' into andalton/mcp-update
adalton 47b5bc3
Merge branch 'main' into andalton/mcp-update
adalton 0fdcc01
Merge branch 'main' into andalton/mcp-update
adalton 77fe084
fix: validate annotation value types in PatchSession to prevent K8s 5…
adalton b1bacbb
Merge remote-tracking branch 'origin/main' into andalton/mcp-update
adalton 115b0c9
Merge remote-tracking branch 'origin/main' into andalton/mcp-update
adalton d3cdcf7
Merge remote-tracking branch 'origin/main' into andalton/mcp-update
adalton 9bd300d
Merge remote-tracking branch 'origin/main' into andalton/mcp-update
adalton 672676b
Merge remote-tracking branch 'origin/main' into andalton/mcp-update
adalton 153b11f
Merge remote-tracking branch 'origin/main' into andalton/mcp-update
adalton 818b330
Merge branch 'main' into andalton/mcp-update
adalton 7c545b1
Merge branch 'main' into andalton/mcp-update
adalton bff0f6d
Merge branch 'main' into andalton/mcp-update
adalton 05a3066
Merge branch 'main' into andalton/mcp-update
adalton 003a4bf
Merge branch 'main' into andalton/mcp-update
adalton f174841
Merge branch 'main' into andalton/mcp-update
adalton d8d249c
Merge remote-tracking branch 'origin/andalton/mcp-update' into andalt…
adalton 235c98b
Merge branch 'main' into andalton/mcp-update
adalton 8ed4ce3
Merge branch 'main' into andalton/mcp-update
adalton 67fc5af
Merge branch 'main' into andalton/mcp-update
adalton 00b2cf4
Merge branch 'main' into andalton/mcp-update
adalton aa998bf
Merge branch 'main' into andalton/mcp-update
adalton 58e50eb
Merge branch 'main' into andalton/mcp-update
adalton 3be43cd
Merge branch 'main' into andalton/mcp-update
adalton a407d68
Merge branch 'main' into andalton/mcp-update
adalton 9a300d9
Merge branch 'main' into andalton/mcp-update
adalton 4d6f152
Merge branch 'main' into andalton/mcp-update
adalton 6cefd7f
Merge branch 'main' into andalton/mcp-update
adalton 667f9b5
Merge branch 'main' into andalton/mcp-update
adalton 385732c
Merge branch 'main' into andalton/mcp-update
adalton 3f4170e
Merge branch 'main' into andalton/mcp-update
adalton f9723e9
Merge branch 'main' into andalton/mcp-update
adalton ebd1617
Merge branch 'main' into andalton/mcp-update
adalton 648526d
Merge branch 'main' into andalton/mcp-update
adalton 0a2dd86
Merge branch 'main' into andalton/mcp-update
adalton File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| package handlers | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "io" | ||
| "log" | ||
| "net/http" | ||
| "strconv" | ||
| "time" | ||
|
|
||
| "github.com/gin-gonic/gin" | ||
| corev1 "k8s.io/api/core/v1" | ||
| "k8s.io/apimachinery/pkg/api/errors" | ||
| v1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| const ( | ||
| defaultTailLines = int64(1000) | ||
| maxTailLines = int64(10000) | ||
| maxLogBytes = 10 * 1024 * 1024 // 10MB cap on log response size | ||
| ) | ||
|
|
||
| // GetSessionLogs returns container logs for the session's runner pod. | ||
| // GET /api/projects/:projectName/agentic-sessions/:sessionName/logs | ||
| // | ||
| // Query params: | ||
| // - tailLines: number of lines from the end (default 1000, max 10000) | ||
| // - container: specific container name (optional) | ||
| func GetSessionLogs(c *gin.Context) { | ||
| project := c.GetString("project") | ||
| if project == "" { | ||
| project = c.Param("projectName") | ||
| } | ||
| sessionName := c.Param("sessionName") | ||
| if !isValidKubernetesName(sessionName) { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid session name format"}) | ||
| return | ||
| } | ||
| safeSessionName := SanitizeForLog(sessionName) | ||
|
|
||
| // Validate query params before any K8s calls | ||
| tailLines := defaultTailLines | ||
| if tl := c.Query("tailLines"); tl != "" { | ||
| parsed, err := strconv.ParseInt(tl, 10, 64) | ||
| if err != nil || parsed < 1 { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "tailLines must be a positive integer"}) | ||
| return | ||
| } | ||
| if parsed > maxTailLines { | ||
| parsed = maxTailLines | ||
| } | ||
| tailLines = parsed | ||
| } | ||
|
|
||
| container := c.Query("container") | ||
|
|
||
| k8sClt, k8sDyn := GetK8sClientsForRequest(c) | ||
| if k8sClt == nil { | ||
| c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid or missing token"}) | ||
| c.Abort() | ||
| return | ||
| } | ||
|
|
||
| // Verify the session CR exists before attempting pod log retrieval | ||
| gvr := GetAgenticSessionV1Alpha1Resource() | ||
| ctx, cancel := context.WithTimeout(c.Request.Context(), 30*time.Second) | ||
| defer cancel() | ||
|
|
||
| _, err := k8sDyn.Resource(gvr).Namespace(project).Get(ctx, sessionName, v1.GetOptions{}) | ||
| if err != nil { | ||
| if errors.IsNotFound(err) { | ||
| c.JSON(http.StatusNotFound, gin.H{"error": "Session not found"}) | ||
| return | ||
| } | ||
| if errors.IsForbidden(err) { | ||
| log.Printf("GetSessionLogs: access denied for session %s/%s", project, safeSessionName) | ||
| c.JSON(http.StatusForbidden, gin.H{"error": "Access denied"}) | ||
| return | ||
| } | ||
| log.Printf("GetSessionLogs: failed to verify session %s/%s: %v", project, safeSessionName, err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to verify session"}) | ||
| return | ||
| } | ||
|
|
||
| // Pod naming convention: {sessionName}-runner | ||
| // Must match operator pod creation in internal/controller/reconcile_phases.go | ||
| podName := fmt.Sprintf("%s-runner", sessionName) | ||
|
|
||
| logOpts := &corev1.PodLogOptions{ | ||
| TailLines: &tailLines, | ||
| } | ||
| if container != "" { | ||
| logOpts.Container = container | ||
| } | ||
|
|
||
| logReq := k8sClt.CoreV1().Pods(project).GetLogs(podName, logOpts) | ||
| logStream, err := logReq.Stream(ctx) | ||
| if err != nil { | ||
| if errors.IsNotFound(err) { | ||
| // Pod doesn't exist (not yet created or already cleaned up) — return empty 200 | ||
| c.Data(http.StatusOK, "text/plain; charset=utf-8", []byte("")) | ||
| return | ||
| } | ||
| if errors.IsForbidden(err) { | ||
| log.Printf("GetSessionLogs: access denied for pod %s in project %s", safeSessionName, project) | ||
| c.JSON(http.StatusForbidden, gin.H{"error": "Access denied"}) | ||
| return | ||
| } | ||
| log.Printf("GetSessionLogs: failed to get logs for pod %s in project %s: %v", safeSessionName, project, err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to retrieve logs"}) | ||
| return | ||
| } | ||
| defer logStream.Close() | ||
|
|
||
| // Stream logs directly to the client with a size cap to prevent OOM | ||
| c.Header("Content-Type", "text/plain; charset=utf-8") | ||
| c.Status(http.StatusOK) | ||
| if _, err := io.Copy(c.Writer, io.LimitReader(logStream, maxLogBytes)); err != nil { | ||
| log.Printf("GetSessionLogs: error streaming logs for pod %s in project %s: %v", safeSessionName, project, err) | ||
| } | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| package handlers | ||
|
|
||
| import ( | ||
| "context" | ||
| "log" | ||
| "net/http" | ||
| "time" | ||
|
|
||
| "github.com/gin-gonic/gin" | ||
| "k8s.io/apimachinery/pkg/api/errors" | ||
| v1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| ) | ||
|
|
||
| // GetSessionMetrics returns usage metrics extracted from the session CR status. | ||
| // GET /api/projects/:projectName/agentic-sessions/:sessionName/metrics | ||
| func GetSessionMetrics(c *gin.Context) { | ||
| project := c.GetString("project") | ||
| if project == "" { | ||
| project = c.Param("projectName") | ||
| } | ||
| sessionName := c.Param("sessionName") | ||
| if !isValidKubernetesName(sessionName) { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid session name format"}) | ||
| return | ||
| } | ||
| safeSessionName := SanitizeForLog(sessionName) | ||
|
|
||
| k8sClt, k8sDyn := GetK8sClientsForRequest(c) | ||
| if k8sClt == nil { | ||
| c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid or missing token"}) | ||
| c.Abort() | ||
| return | ||
| } | ||
|
|
||
| gvr := GetAgenticSessionV1Alpha1Resource() | ||
|
|
||
| ctx, cancel := context.WithTimeout(c.Request.Context(), 30*time.Second) | ||
| defer cancel() | ||
|
|
||
| item, err := k8sDyn.Resource(gvr).Namespace(project).Get(ctx, sessionName, v1.GetOptions{}) | ||
| if err != nil { | ||
| if errors.IsNotFound(err) { | ||
| c.JSON(http.StatusNotFound, gin.H{"error": "Session not found"}) | ||
| return | ||
| } | ||
| if errors.IsForbidden(err) { | ||
| log.Printf("GetSessionMetrics: access denied for session %s/%s", project, safeSessionName) | ||
| c.JSON(http.StatusForbidden, gin.H{"error": "Access denied"}) | ||
| return | ||
| } | ||
| log.Printf("GetSessionMetrics: failed to get session %s/%s: %v", project, safeSessionName, err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to get session"}) | ||
| return | ||
| } | ||
|
|
||
| metrics := gin.H{ | ||
| "sessionId": sessionName, | ||
| } | ||
|
|
||
| // Extract timing info from status using unstructured helpers | ||
| if phase, ok, _ := unstructured.NestedString(item.Object, "status", "phase"); ok { | ||
| metrics["phase"] = phase | ||
| } | ||
| if startTime, ok, _ := unstructured.NestedString(item.Object, "status", "startTime"); ok { | ||
| metrics["startTime"] = startTime | ||
|
|
||
| // Calculate duration if possible | ||
| start, err := time.Parse(time.RFC3339, startTime) | ||
| if err == nil { | ||
| var end time.Time | ||
| if completionTime, ok, _ := unstructured.NestedString(item.Object, "status", "completionTime"); ok && completionTime != "" { | ||
| end, err = time.Parse(time.RFC3339, completionTime) | ||
| if err != nil { | ||
| // Malformed completionTime — skip both fields to avoid misleading data | ||
| end = time.Time{} | ||
| } else { | ||
| metrics["completionTime"] = completionTime | ||
| } | ||
| } else { | ||
| end = time.Now() | ||
| } | ||
| if !end.IsZero() { | ||
| metrics["durationSeconds"] = int(end.Sub(start).Seconds()) | ||
| } | ||
| } | ||
| } | ||
| if sdkRestartCount, ok, _ := unstructured.NestedInt64(item.Object, "status", "sdkRestartCount"); ok { | ||
| metrics["restartCount"] = int(sdkRestartCount) | ||
| } | ||
|
|
||
| // Extract timeout from spec | ||
| if timeout, ok, _ := unstructured.NestedInt64(item.Object, "spec", "timeout"); ok { | ||
| metrics["timeoutSeconds"] = int(timeout) | ||
| } | ||
|
|
||
| // Extract any usage annotations (token counts, tool calls, etc.) | ||
| annotations := item.GetAnnotations() | ||
| usage := gin.H{} | ||
| for k, v := range annotations { | ||
| // Look for usage-related annotations | ||
| switch k { | ||
| case "ambient-code.io/input-tokens": | ||
| usage["inputTokens"] = v | ||
| case "ambient-code.io/output-tokens": | ||
| usage["outputTokens"] = v | ||
| case "ambient-code.io/total-cost": | ||
| usage["totalCost"] = v | ||
| case "ambient-code.io/tool-calls": | ||
| usage["toolCalls"] = v | ||
| } | ||
| } | ||
| if len(usage) > 0 { | ||
| metrics["usage"] = usage | ||
| } | ||
|
|
||
| c.JSON(http.StatusOK, metrics) | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: ambient-code/platform
Length of output: 50378
🏁 Script executed:
Repository: ambient-code/platform
Length of output: 4168
🏁 Script executed:
Repository: ambient-code/platform
Length of output: 465
🏁 Script executed:
Repository: ambient-code/platform
Length of output: 1098
🏁 Script executed:
Repository: ambient-code/platform
Length of output: 3318
🏁 Script executed:
Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
Repository: ambient-code/platform
Length of output: 2077
🏁 Script executed:
Repository: ambient-code/platform
Length of output: 1655
🏁 Script executed:
Repository: ambient-code/platform
Length of output: 2638
🏁 Script executed:
Repository: ambient-code/platform
Length of output: 741
🏁 Script executed:
Repository: ambient-code/platform
Length of output: 47
Validate annotation values before assignment to avoid 500s on invalid PATCH payloads.
At line 1124,
anns[k] = vaccepts 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
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 77fe084. Added a type switch to validate annotation values before assignment:
nil→ deletes the annotation key (existing behavior)string→ assigns the valueThis 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 sendnull.