feat: Enhance session detail UI and MCP integration #494
Conversation
…ation visibility - Changed the slash command handling in `content.go` to use the full command name. - Updated the `IntegrationsClient` to include the `GoogleDriveConnectionCard`. - Refactored `McpIntegrationsAccordion` to display MCP server statuses with improved UI elements and removed deprecated Google Drive integration code. - Enhanced `MessagesTab` to directly show command names instead of derived titles, improving clarity in command representation. - Adjusted the `ClaudeCodeAdapter` to ensure default MCP configurations are set correctly and improved workspace prompt generation for clarity. These changes aim to streamline command usage and enhance user experience across the application.
- Introduced a new endpoint for retrieving MCP server statuses in the backend, allowing the frontend to display real-time integration statuses. - Updated the `SelectWorkflow` function to handle MCP status checks during workflow selection. - Enhanced the `McpIntegrationsAccordion` component to fetch and display MCP server statuses dynamically. - Removed deprecated model configuration and session creation components to streamline the codebase. These changes improve the visibility of MCP integrations and enhance user experience by providing real-time feedback on server statuses.
- Updated the backend session handler to provide more context in error messages related to OOTB workflows, specifically addressing GitHub rate limits and repository not found errors. - Modified the frontend API route to forward query parameters and authorization headers, improving the integration with GitHub for better rate limits. - Refactored the session detail page to manage sidebar state based on session status, ensuring consistent UI behavior and adding state overlays for non-running sessions. These changes aim to enhance error visibility and streamline user interactions with session management.
- Updated the session detail page to improve the user interface, including adjustments to loading indicators and state overlays for non-running sessions. - Enhanced the `McpIntegrationsAccordion` component to reflect the new status options for MCP servers, including a 'configured' state. - Modified backend API responses to clarify MCP server statuses and added instructions for Google Drive and Jira integrations in the prompt. These changes aim to provide a clearer and more informative user experience regarding session states and MCP integrations.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
- Added new API routes for Google OAuth connection, status check, and disconnection. - Enhanced OAuth callback handling to support cluster-level user-scoped credentials. - Updated frontend components to manage Google Drive connection status and actions. - Implemented backend logic to sync Google OAuth credentials to session-specific secrets for access during session execution. These changes enable users to connect their Google Drive accounts at a cluster level, improving integration and accessibility across sessions.
- Change ConfigMap to Secret for Google OAuth credentials storage - Backend: storeGoogleCredentials, GetGoogleCredentials, DisconnectGoogleOAuthGlobal - Operator: syncGoogleCredentialsToSecret reads from Secret - Add GetK8sClientsForRequest to all Google OAuth endpoints for RBAC consistency - GetGoogleOAuthURLGlobal - GetGoogleOAuthStatusGlobal - DisconnectGoogleOAuthGlobal - Fix frontend polling memory leak with useRef and useEffect cleanup - Prevents interval from running after component unmount
- Fix test to create workflow at StateBaseDir/workflows/ instead of sessions/ - Fix expected slashCommand from /command to /test.command (matches filename)
Backend (oauth.go): - Add context and 10s timeout to getGoogleUserEmail HTTP call - Add isValidUserID validation to all Google OAuth endpoints - Prevent path traversal and invalid characters in userID Frontend (google-drive-connection-card.tsx): - Add popup blocker detection with user-friendly error message - Return early if popup is null/blocked instead of polling indefinitely
This comment has been minimized.
This comment has been minimized.
Response to Code Review Findings✅ Fixed Issues
⏭️ Not Fixed (By Design)
|
Show error indicator and message when useGoogleStatus query fails
Response to Updated Code Review✅ Fixed
❌ Invalid Findings
⏭️ Not Required for Merge
|
Claude Code ReviewSummaryThis PR introduces cluster-level Google OAuth integration, enhances session detail UI, and adds MCP integration. The implementation follows most established patterns but has several critical security issues that must be addressed before merge. Issues by Severity🚫 Blocker Issues1. Token Leakage in Logs (oauth.go:309, 315)
// ❌ BAD
c.Data(http.StatusOK, "text/html", []byte("Error: "+err.Error()))
// ✅ GOOD
log.Printf("Cluster-level OAuth failed: %v", err)
c.Data(http.StatusOK, "text/html", []byte("Authorization Error. Please try again."))2. Credentials Stored in Secret Without Encryption (oauth.go:950-970)
3. Missing RBAC Check for Cluster-Level OAuth (oauth.go:817-823)
// Add RBAC check
ssar := &authv1.SelfSubjectAccessReview{
Spec: authv1.SelfSubjectAccessReviewSpec{
ResourceAttributes: &authv1.ResourceAttributes{
Group: "vteam.ambient-code",
Resource: "clusteroauthconnections",
Verb: "create",
},
},
}
res, err := reqK8s.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, ssar, v1.CreateOptions{})
if err \!= nil || \!res.Status.Allowed {
c.JSON(http.StatusForbidden, gin.H{"error": "Unauthorized"})
return
}🔴 Critical Issues4. Race Condition in Secret Updates (oauth.go:957-970)
5. No Token Refresh Logic
6. userID Validation Insufficient (oauth.go:802-813)
// Add to isValidUserID
import "k8s.io/apimachinery/pkg/util/validation"
if errs := validation.IsDNS1123Subdomain(userID); len(errs) > 0 {
return false
}7. Operator Credential Sync Has No Error Recovery (sessions.go - new code)
🟡 Major Issues8. Frontend API Routes Missing Error Handling (google/*/route.ts)
9. Polling Interval Memory Leak (google-drive-connection-card.tsx:63-73)
10. No Audit Logging for Cluster-Level OAuth
🔵 Minor Issues11. HTTP Client Without Context Timeout (oauth.go:1144)
12. useGoogleStatus staleTime Too Short (use-google.ts:16)
13. Inconsistent Naming: "Global" vs "Cluster-Level"
14. Missing Type Definition for GoogleOAuthCredentials (frontend)
15. Workflow Path Correction Missing Tests (content_test.go:830-920)
Positive Highlights✅ User-Scoped Authentication: Correctly uses RecommendationsPriority 1 (Must Fix Before Merge):
Priority 2 (Should Fix Before Merge): Priority 3 (Nice-to-Have): Code Quality:
Overall Assessment: Strong implementation that follows most patterns, but CANNOT MERGE until blocker security issues are resolved. The credential storage model needs clarity and the missing RBAC check is a significant security gap. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Return generic error message to prevent information leakage. Detailed error is logged server-side for debugging.
Response to Third Code Review✅ Fixed
❌ Invalid / Already Addressed
SummaryAll legitimate security issues have been addressed. Remaining items are either:
|
Claude Code ReviewSummaryThis PR adds cluster-level Google OAuth integration, MCP status endpoints, session detail UI improvements, and workflow management enhancements. The implementation includes 1,541 additions across backend, frontend, and operator components. Overall Assessment: This PR demonstrates solid engineering with good security practices, but has several critical issues that must be addressed before merge. Issues by Severity🚫 Blocker Issues1. Security: Sensitive Credentials Stored in Plain Secret (oauth.go:954-1001) // storeGoogleCredentials stores Google OAuth credentials in cluster-level Secret
func storeGoogleCredentials(ctx context.Context, creds *GoogleOAuthCredentials) error {
// ...
secret.Data[creds.UserID] = b // ❌ Storing access/refresh tokens in plain Secret
}Issue: Access tokens and refresh tokens are stored in plain Kubernetes Secret without encryption. 2. Missing HMAC Signature Verification on OAuth Callback (oauth.go:304-328) stateBytes, err := base64.URLEncoding.DecodeString(strings.Split(state, ".")[0])
if err == nil {
if jsonErr := json.Unmarshal(stateBytes, &stateMap); jsonErr == nil {
// ❌ No HMAC signature verification for cluster-level OAuth state
if isCluster, ok := stateMap["cluster"].(bool); ok && isCluster {
HandleGoogleOAuthCallback(c.Request.Context(), code, stateMap)
}
}
}Issue: The new cluster-level OAuth path doesn't verify the HMAC signature from the state token before processing. 3. Frontend: Missing export type GoogleOAuthStatus = { // ✅ Correct
connected: boolean;
// ...
};
export type GoogleOAuthURLResponse = { // ✅ Correct
url: string;
state: string;
};Status: Actually this is CORRECT - using 🔴 Critical Issues4. Backend: Service Account Used Without User Token Validation (oauth.go:954-1001) func storeGoogleCredentials(ctx context.Context, creds *GoogleOAuthCredentials) error {
// Uses K8sClient (service account) without checking if user is authorized
secret, err := K8sClient.CoreV1().Secrets(Namespace).Get(ctx, secretName, v1.GetOptions{})
}Issue: 5. Operator: Missing Error Context in Credential Sync (sessions.go) Based on the diff stats (119 additions), the operator likely syncs credentials to session secrets. Without seeing the full code, potential issues:
Required: Review operator credential sync code for:
6. Frontend: No Error Boundary for OAuth Popup Flow (google-drive-connection-card.tsx:30-79) const handleConnect = async () => {
setConnecting(true)
try {
const response = await googleAuthApi.getGoogleOAuthURL()
const popup = window.open(authUrl, ...)
// ⚠️ No error handling if popup.close() fails or popup is blocked after opening
}
}Issue: Popup handling lacks comprehensive error states. 🟡 Major Issues7. Backend: Unbounded Retry Loop on Secret Conflicts (oauth.go:962-1007) for i := 0; i < 3; i++ { // retry on conflict
secret, err := K8sClient.CoreV1().Secrets(Namespace).Get(ctx, secretName, v1.GetOptions{})
// ...
if errors.IsConflict(uerr) {
continue // ⚠️ No backoff delay between retries
}
}Issue: Tight retry loop without exponential backoff. 8. Backend: Weak UserID Validation (oauth.go:803-814) func isValidUserID(userID string) bool {
if userID == "" || len(userID) > 253 {
return false
}
for _, ch := range userID {
if ch == '/' || ch == '\\' || ch == '\x00' {
return false
}
}
return true
}Issue: Doesn't validate K8s DNS label requirements (RFC 1123). 9. Frontend: Polling Interval Not Cleared on Success (google-drive-connection-card.tsx:63-73) pollTimerRef.current = setInterval(() => {
if (popup.closed) {
clearInterval(pollTimerRef.current!)
pollTimerRef.current = null
setConnecting(false)
refetch() // ⚠️ What if refetch() shows connected=true but interval keeps running?
}
}, 500)Issue: Interval clears only when popup closes, not when connection succeeds. 10. Missing OwnerReferences for Operator-Created Secrets Per CLAUDE.md Critical Rule #5, all child resources must have OwnerReferences. If the operator creates session-specific secrets for synced credentials, they should have OwnerReferences to the AgenticSession. Required: Verify operator code sets OwnerReferences when syncing credentials. 🔵 Minor Issues11. Backend: Inconsistent Error Messages (oauth.go:315-318) c.Data(http.StatusOK, "text/html; charset=utf-8", []byte(
"<html><body><h1>Authorization Error</h1><p>Failed to connect Google Drive. Please try again.</p>...",
))Issue: Returns HTTP 200 for error case (should be 400/500). 12. Frontend: Magic Numbers in UI Layout (google-drive-connection-card.tsx:39-42) const width = 600
const height = 700
const left = window.screen.width / 2 - width / 2Issue: Hardcoded popup dimensions. 13. Frontend: Type Safety - Loose Error Handling (google-drive-connection-card.tsx:76) errorToast(err instanceof Error ? err.message : 'Failed to connect Google Drive')Issue: Using 14. Backend: Missing Rate Limiting on OAuth Endpoints The new cluster-level OAuth endpoints ( Risk: Abuse of OAuth flow (e.g., state generation spam). Positive Highlights✅ Excellent Security Awareness
✅ Proper React Query Usage
✅ Good Code Organization
✅ Frontend Best Practices
✅ Comprehensive Logging
RecommendationsPriority 1 (Must Fix Before Merge)
Priority 2 (Should Fix Before Merge)
Priority 3 (Nice to Have)
Testing RecommendationsSecurity Testing:
Functional Testing:
Edge Cases:
Overall AssessmentCode Quality: 7/10 Recommendation: REQUEST CHANGES - Address blocker security issues (#1, #2) and critical RBAC validation (#4) before merge. Reviewed by Claude Code using:
🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
When multiple sessions exist in the same namespace, the operator would delete the ambient-vertex and ambient-admin-langfuse-secret when one session stopped, even if other sessions were still Running, Creating, or Pending and needed those secrets. This fix adds a check before deleting these shared secrets to count active sessions in the namespace. If any sessions are still active, the secret deletion is skipped. Fixes the MountVolume.SetUp failed error: 'secret "ambient-vertex" not found' Related to PR #494
When multiple sessions exist in the same namespace, the operator would delete the ambient-vertex and ambient-admin-langfuse-secret when one session stopped, even if other sessions were still Running, Creating, or Pending and needed those secrets. This fix adds a check before deleting these shared secrets to count active sessions in the namespace. If any sessions are still active, the secret deletion is skipped. Fixes the MountVolume.SetUp failed error: 'secret "ambient-vertex" not found' Related to PR #494
…495) ## Problem When multiple sessions exist in the same namespace (project), stopping one session would delete the `ambient-vertex` and `ambient-admin-langfuse-secret` secrets from the namespace, even if other sessions were still Running, Creating, or Pending and needed those secrets. This caused the error: ``` MountVolume.SetUp failed for volume "vertex" : secret "ambient-vertex" not found ``` ## Root Cause The `deleteAmbientVertexSecret` and `deleteAmbientLangfuseSecret` functions were called during session cleanup without checking if other sessions in the same namespace still needed these shared secrets. ## Solution Added a check before deleting these shared secrets to count active sessions (Running, Creating, or Pending) in the namespace. If any active sessions exist, the secret deletion is skipped with a log message. ## Testing - Deployed fix to stage cluster - Verified existing sessions can now start without the FailedMount error - Operator logs show proper skipping behavior when active sessions exist Related to PR #494
1 participant
Summary
This PR includes several improvements to the session detail UI, MCP integration, and adds cluster-level Google OAuth integration.
🆕 Google OAuth Integration (New)
Session Detail UI Improvements
MCP Integration
CI/CD Improvements
Maintenance
Testing
Files Changed
components/backend/handlers/oauth.go- Google OAuth routes and handlerscomponents/backend/routes.go- New route registrationscomponents/frontend/src/app/api/auth/google/*- Frontend API routescomponents/frontend/src/components/google-drive-connection-card.tsx- UI componentcomponents/frontend/src/services/api/google-auth.ts- API servicecomponents/frontend/src/services/queries/use-google.ts- React Query hookscomponents/operator/internal/handlers/sessions.go- Credential sync to sessions