feat: Revamp E2E Testing for Ambient Code Platform #520
Conversation
This comment has been minimized.
This comment has been minimized.
Reverts the automatic copying of ambient-runner-secrets from operator namespace to session namespaces. This simplifies the operator logic and removes potential security concerns with cross-namespace secret copying. For E2E testing, secrets should be configured directly in the project namespace via the backend API or kubectl.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Claude Code ReviewSummaryThis PR introduces significant improvements to E2E testing infrastructure and documentation organization. The changes include 128 files with a net reduction of ~7,000 lines while improving clarity. The PR primarily focuses on infrastructure, documentation, and testing improvements with minimal production code changes. Overall Assessment: APPROVE with Minor Recommendations The PR demonstrates strong adherence to project standards with excellent documentation reorganization and testing infrastructure improvements. A few minor security and code quality considerations should be addressed. Issues by Severity🚫 Blocker IssuesNone - No blocking issues found. 🔴 Critical IssuesNone - No critical issues found. 🟡 Major Issues1. Frontend E2E Token Handling (Security Pattern Concern)Location: Issue: While the NEXT_PUBLIC_ prefix is appropriate for client-side environment variables, the comment "without localStorage hacks" suggests this may be replacing a problematic pattern. However, exposing tokens via client-side environment variables (even for E2E) could set a problematic precedent. Recommendation: Add explicit comment that this is ONLY for E2E testing environments, consider adding a runtime check to prevent this code from running in production, and document this pattern in the E2E testing guide with clear warnings. 2. Operator Secret Management SimplificationLocation: Change: Removed automatic secret copying from operator namespace to session namespaces (60+ lines removed). Positive: This simplification aligns with KISS principles and reduces complexity. Concern: Migration path unclear - existing deployments may rely on automatic secret propagation. Ensure migration guide exists for users upgrading. 🔵 Minor Issues
Positive Highlights✨ Excellent Work
RecommendationsHigh Priority
Medium Priority
Final VerdictAPPROVE ✅ This PR demonstrates:
The minor issues identified are non-blocking and can be addressed in follow-up PRs or immediately before merge. Recommended Actions Before Merge:
Review conducted by: Claude Code (AI Code Review Agent) 🔍 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 ANTHROPIC_API_KEY is set in e2e/.env, automatically create a
'test-project' namespace with the key pre-configured. This enables
immediate manual testing without needing to configure via UI/API.
Changes:
- deploy.sh: Create test-project namespace with ambient-runner-secrets
- refresh-env.sh: Update test-project secret when API key changes
- env.example: Document the automatic namespace configuration
For other project namespaces, users should configure API keys via:
- UI: Project Settings → API Keys
- API: PUT /api/projects/{project}/runner-secrets
- kubectl: Create ambient-runner-secrets in project namespace
Claude Code ReviewThis PR introduces a comprehensive overhaul of E2E testing infrastructure and documentation organization. Changes span ~128 files with 4,444 additions and 11,733 deletions. Critical Issues
Major Issues
Positive Highlights✅ Excellent E2E infrastructure modernization with Kind RecommendationApprove with required changes: Fix test RBAC, add production guard to E2E token, document resource limits. This is high-quality work that significantly improves testing and documentation. Minor issues can be addressed in follow-up PRs. Review based on CLAUDE.md and memory system standards 🔍 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. |
Adds a configurable KIND_HTTP_PORT variable to control which port the kind cluster listens on. This allows developers to choose between: - KIND_HTTP_PORT=auto (default): Uses 8080 for podman, 80 for docker - KIND_HTTP_PORT=80: Forces port 80 (good for docker on Linux) - KIND_HTTP_PORT=8080: Forces port 8080 (good for podman) - KIND_HTTP_PORT=<custom>: Any custom port (e.g., 9000) Usage: make kind-up KIND_HTTP_PORT=80 # Force port 80 make kind-up KIND_HTTP_PORT=9000 # Use port 9000 make kind-up # Auto-detect (default) Changes: - Makefile: Add KIND_HTTP_PORT variable, pass to scripts - setup-kind.sh: Accept KIND_HTTP_PORT override - extract-token.sh: Use KIND_HTTP_PORT for .env.test generation - Help text: Document new variable and examples
Enables running E2E tests without .env.test by accepting TEST_TOKEN and CYPRESS_BASE_URL from environment variables. This allows: 1. Testing directly against frontend on any port: CYPRESS_BASE_URL=http://localhost:3000 make test-e2e 2. Testing against external clusters without kind-up: TEST_TOKEN=$(oc whoami -t) CYPRESS_BASE_URL=https://... make test-e2e 3. Port forwarding scenarios: kubectl port-forward -n ambient-code svc/frontend-service 3000:3000 CYPRESS_BASE_URL=http://localhost:3000 make test-e2e Changes: - run-tests.sh: Load .env.test only if env vars not set - run-tests.sh: Allow running without .env.test file - Makefile: Pass CYPRESS_BASE_URL and TEST_TOKEN to script - Makefile: Update help text with environment variable examples Environment variables take precedence over .env.test values.
Claude Code ReviewSummaryThis PR introduces a comprehensive overhaul of E2E testing infrastructure and documentation reorganization. The changes span ~128 files with significant improvements to developer experience and testing capabilities. Overall, this is a well-structured refactoring with good practices, but there are some security and code quality concerns that should be addressed. Issues by SeverityCritical Issues1. Security: E2E test token exposed client-side (client.ts:96-100) Issue: The NEXT_PUBLIC_E2E_TOKEN is exposed to client-side JavaScript, making it visible in browser dev tools. This violates the security standard of never exposing tokens client-side and could be exploited if accidentally set in production. Recommendation: Use server-side only approach, add NODE_ENV checks, document in security-standards.md 2. Type Safety: Missing JSON validation (adapter.py:1303) Issue: Code doesn't validate JSON structure after parsing. If config is not a dict, config.get() will fail. Recommendation: Add isinstance check before accessing dict methods Major Issues1. Operator: Race condition in session restart (sessions.go:97-148) The operator sets phase=Pending and continues without waiting for status propagation, potentially causing duplicate job creation. 2. Frontend: Token injection ignores existing auth E2E token injection doesn't check if user is already authenticated, potentially overriding legitimate sessions. 3. Adapter: Empty string validation missing (adapter.py:913-930) Sanitization could result in empty strings causing downstream issues. Minor Issues
Positive Highlights✅ Excellent documentation reorganization with DOCUMENTATION_MAP.md RecommendationsPriority 1 (Before Merge):
Priority 2 (Follow-up):
Overall AssessmentHigh-quality refactoring with excellent E2E testing and documentation improvements. Main concern is E2E token security pattern. With Priority 1 changes, ready to merge. Recommendation: Request changes for security hardening, then approve. Review conducted with loaded context from memory system files 🔍 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. |
Claude Code ReviewSummaryThis PR introduces a comprehensive E2E testing infrastructure overhaul with kind overlay support and extensive documentation reorganization (~7,000 lines cleaned up). The changes significantly improve developer experience and testing capabilities while maintaining security standards. Issues by SeverityBlocker IssuesNone found - The PR follows established patterns and security guidelines. Critical IssuesNone found - Code quality, security, and architecture patterns are followed correctly. Major Issues
Minor Issues
Positive HighlightsSecurity Standards - Excellent
E2E Testing Infrastructure - Outstanding
Documentation Reorganization - Exceptional
Code Quality - Follows Standards
RecommendationsHigh Priority
Medium Priority
Low Priority
Final VerdictAPPROVED with minor recommendations This is an excellent PR that significantly improves the project's testing infrastructure and documentation organization. The code follows all established patterns, maintains security standards, and provides a solid foundation for E2E testing. Merge Recommendation: Approve and merge after addressing the E2E token injection security check (high priority item #1). Other recommendations can be addressed in follow-up PRs. Impact Summary:
Reviewed by: Claude Code (Sonnet 4.5) 🔍 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. |
This commit simplifies the build process by removing the embedding of Git metadata into Docker images and the Makefile. The following changes were made: - Removed Git-related build arguments and environment variables from Dockerfiles for backend, frontend, operator, and runner components. - Updated the Makefile to eliminate the capture of Git metadata during the build process. - Adjusted the build commands in the Makefile to focus solely on building the images without embedding versioning information. This streamlining enhances the build process and reduces complexity, while still allowing for effective version tracking through other means.
This commit deletes the SECURITY_DEV_MODE.md file, which analyzed the risks of accidentally deploying development mode to production and documented existing safeguards and recommendations. The removal is part of a broader effort to streamline documentation and focus on more relevant security practices.
Claude Code ReviewSummaryThis PR represents a significant infrastructure improvement introducing comprehensive E2E testing capabilities, documentation reorganization, and essential configuration updates. The changes span 128 files with a net reduction of ~7,000 lines while improving clarity and maintainability. Overall Assessment: APPROVED with Minor Recommendations The code quality is generally strong, following established patterns. However, there are several security considerations and architectural improvements to address. Issues by SeverityCritical Issues
The test user has cluster-admin privileges, which violates least-privilege principles and could mask RBAC issues in testing. Recommendation: Create a dedicated role with only required permissions. E2E tests should validate actual RBAC behavior.
Issues: NEXT_PUBLIC_* vars are exposed to the browser bundle, token visible in build artifacts and DevTools, bypasses established auth pattern. Recommendation: Add NODE_ENV check to only enable in test environment. Major Issues
Minor Issues
Positive Highlights
RecommendationsImmediate (Before Merge):
Short Term (Next PR): Final VerdictApproval Status: APPROVED with Recommendations Risk Level: Medium
Merge Recommendation: Merge after addressing Critical Issues 1 and 2 Great work on this comprehensive overhaul! Review completed by: Claude Code (Sonnet 4.5) 🔍 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. |
Claude Code ReviewSummaryThis PR introduces significant improvements to the E2E testing infrastructure and reorganizes documentation. The changes align well with established patterns, but there are security concerns and architectural inconsistencies that need attention before merging. Overall Assessment: 🟡 Major issues need addressing, particularly around security context and resource management. Critical Issues1. FSGroup Security Context May Violate OpenShift SCCLocation: Setting Impact: Pods will fail to start on OpenShift clusters (production environment). Fix: Use a non-root group ID like Reference: CLAUDE.md line 672-680 (Container Security pattern) 2. Missing Resource Requests/Limits ValidationLocation: Hardcoded resource limits (2Gi+512Mi per pod) without configurability impacts Kind/Minikube users and production workloads. Impact: Pods may fail to schedule in resource-constrained environments. Fix: Make resources configurable via ConfigMap or CR spec. 3. E2E Token Injection Security RiskLocation: Token injection via Impact: Production frontend could have hardcoded token, bypassing auth. Fix: Add NODE_ENV check: Major Issues
Positive Highlights✅ Excellent Documentation Reorganization Recommendations Before Merge
Final VerdictExcellent work overall, but FSGroup and E2E token issues need fixing to avoid production security risks and OpenShift compatibility issues. Estimated Fix Time: 15-30 minutes for code changes + 1 hour for OpenShift validation. 🔍 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. |
| # Check if you already have a podman machine | ||
| podman machine list | ||
| ``` | ||
| brew install kind kubectl docker |
There was a problem hiding this comment.
On the call today, we discussed this change from Podman to Docker. The conclusion was that this file is no longer needed and should be deleted, I think.
…lopment documentation This commit modifies the GitHub Actions workflow for building and deploying components by removing support for multiple platforms in Docker builds, now focusing solely on `linux/amd64`. Additionally, it deletes the `instrumentation.ts` file from the frontend, which was responsible for logging build information. The local development documentation has been significantly revamped to clarify scenarios for running components locally, including detailed setups and benefits for different development tasks. This streamlining aims to improve developer experience and efficiency.
Claude Code ReviewSummaryThis PR represents a major infrastructure and documentation overhaul for the Ambient Code Platform. The changes include E2E testing improvements, comprehensive documentation reorganization, and strategic cleanups. Overall, the PR is well-structured with 4,635 additions and 13,254 deletions (net -7,000 lines), demonstrating effective consolidation. Key Strengths:
Areas Requiring Attention:
Issues by SeverityBlocker IssuesNone - No blocking issues that prevent merge. Critical Issues1. Hardcoded E2E Token in Frontend API ClientFile: Issue:
Per Recommended Fix: Use proper auth context/provider for token management. For E2E tests, use Cypress commands to inject token via cy.setCookie() or localStorage, not environment variables. Major Issues2. Operator Status Updates Missing Error ContextFile: Failed status initialization is logged but doesn't update CR status with error message. User/operator won't know why phase initialization failed. Recommended Fix: Update status with error message and return error instead of just logging warning. Reference: 3. Inconsistent Documentation References in CLAUDE.mdMultiple documentation references in CLAUDE.md weren't updated after docs reorganization. Users following CLAUDE.md links will encounter 404s. Recommended Fix: Run comprehensive search and replace to update all doc references to new paths based on DOCUMENTATION_MAP.md Minor Issues4. Operator Code Comment ClarityThe TODO comment in 5. Python Adapter Import OrderFile: Minor style issue - imports should be grouped per PEP 8. Consider using isort to auto-format. 6. Missing Test Coverage for New E2E Session TestsConsider adding edge cases: session deletion while running, invalid workflow creation, session stuck in Starting state. Positive HighlightsExcellent Documentation StructureThe new docs/ organization is outstanding:
Kind Overlay ImplementationThe new Kind overlay is well-designed:
Effective CleanupRemoving 7,000+ lines of obsolete content while improving clarity demonstrates excellent maintenance. Operator Status Patch PatternThe new StatusPatch pattern is a great improvement - reduces API calls and improves performance. Frontend API Client Error HandlingThe error handling in client.ts is well-structured with type-safe error parsing and graceful fallbacks. RecommendationsPriority 1 (Before Merge)
Priority 2 (Follow-up PR)
Testing VerificationRequired Before Merge
ConclusionThis is a high-quality PR that significantly improves the platform's testing infrastructure and documentation. The 7,000-line reduction while improving clarity is commendable. Recommendation: APPROVE with Critical Issue Fixed The E2E token injection in the frontend API client must be addressed before merge (Priority 1, item 1). All other issues can be addressed in follow-up PRs. Great work on the comprehensive refactoring! Review completed by Claude Code (Sonnet 4.5) 🔍 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. |
2 participants
Overview
This PR introduces a comprehensive overhaul of the E2E testing infrastructure and documentation organization for the Ambient Code Platform.
Key Changes
E2E Testing Infrastructure
Documentation Reorganization
agents/- Active and archived agent definitionsarchitecture/- System diagrams and architecture docsdeployment/- Deployment guides including OpenShift, OAuth, S3, MinIOdeveloper/- Developer guides including security, build metadata, and local development options (kind, CRC, hybrid)integrations/- GitHub, GitLab, Google Workspace integrationsobservability/- Langfuse and metrics documentationtesting/- Testing strategy and guidestools/- Amber and other tooling docsDOCUMENTATION_MAP.md) for easy navigationCode Changes
Cleanup
Testing
The changes have been validated with:
Impact
Files Changed
Related Issues
Addresses ongoing E2E testing improvements and documentation reorganization efforts.