feat: feature flags

[maskarb]

No description provided.

[bobbravo2]

@maskarb once we get the blockers and 🔴 fixed from the review, this is getting really close to landing. The testing path(s) might be deferrable for future PRs, too, however given this is a key platform functionality.. I'm inclined to ask for at least a critical path test or two 👍🏾

[github-actions]

Claude Code Review

Summary

This PR implements Unleash-based feature flag management with workspace-scoped ConfigMap overrides. The implementation adds comprehensive feature flag capabilities including:

• Backend Unleash client integration with optional initialization
• Workspace-scoped ConfigMap overrides (per-namespace feature flag control)
• Tag-based filtering for workspace-configurable flags
• Admin UI for workspace administrators
• Frontend Unleash SDK integration with proxy endpoints
• Comprehensive unit tests (1000+ lines)

Overall Assessment: ✅ Approve with Minor Suggestions

The code follows established patterns from CLAUDE.md and context files. Security, error handling, and authentication are implemented correctly. Excellent test coverage.

---

Issues by Severity

🚫 Blocker Issues

None - No blocking issues found.

---

🔴 Critical Issues

None - No critical issues found.

---

🟡 Major Issues

1. Backend Service Account Usage for ConfigMap Operations

Location: components/backend/handlers/featureflags_admin.go

Issue: All ConfigMap operations (lines 88, 400, 414, 432, 469, 489, 525, 539, 557, 594, 608, 626) use K8sClient (backend service account) instead of user-scoped clients.

Current Pattern:

cm, err := K8sClient.CoreV1().ConfigMaps(namespace).Get(ctx, FeatureFlagOverridesConfigMap, metav1.GetOptions{})

Expected Pattern (per CLAUDE.md:525-556):

reqK8s, _ := GetK8sClientsForRequest(c)
// Validate RBAC permissions for ConfigMap access
ssar := &authv1.SelfSubjectAccessReview{
    Spec: authv1.SelfSubjectAccessReviewSpec{
        ResourceAttributes: &authv1.ResourceAttributes{
            Resource:  "configmaps",
            Verb:      "get", // or "create", "update", "delete"
            Namespace: namespace,
            Name:      FeatureFlagOverridesConfigMap,
        },
    },
}
// Then use backend service account AFTER validation

Why this matters: Per ADR-0002 and k8s-client-usage.md, user operations should validate RBAC before using elevated permissions. The current implementation validates project access (line 140) but doesn't check ConfigMap-specific permissions.

Recommendation: Add RBAC checks for ConfigMap operations, or document why backend service account is appropriate here (similar to CR writes in sessions.go:417).

---

2. Potential Token Leak in Admin Proxy

Location: components/backend/handlers/featureflags_admin.go:649

Issue: Admin API token is set directly in header without redaction in logs.

req.Header.Set("Authorization", getUnleashAdminToken())

Risk: If HTTP client logs requests or errors include headers, the admin token could leak.

Recommendation: Ensure unleashAdminRequest errors don't expose the Authorization header (similar to server/server.go:22-34 token redaction pattern). Add test case verifying error messages don't contain tokens.

---

🔵 Minor Issues

1. Inconsistent Error Message Format

Location: components/backend/handlers/featureflags_admin.go:199

Issue: Log message includes response body which may contain sensitive data:

log.Printf("Unleash Admin API returned %d: %s", resp.StatusCode, string(body))

Recommendation: Don't log full response bodies. Use:

log.Printf("Unleash Admin API returned %d", resp.StatusCode)

Note: Already fixed on line 361 - just needs consistency.

---

2. Missing enabled Parameter Validation

Location: components/backend/handlers/featureflags_admin.go:280

Issue: No validation that flagName exists in Unleash before checking ConfigMap.

Current behavior: Returns 200 with enabled: false for non-existent flags.

Recommendation: Query Unleash first to verify flag exists, return 404 if not found (similar to GetFeatureFlag:362-367).

---

3. Frontend Type Safety

Location: components/frontend/src/components/providers/feature-flag-provider.tsx:18

Issue: Uses placeholder key when Unleash not configured:

const UNLEASH_CLIENT_KEY = process.env.NEXT_PUBLIC_UNLEASH_CLIENT_KEY || 'placeholder-not-configured';

Issue: This violates the "Zero any Types" rule (frontend-development.md:15-34) in spirit - we're passing invalid data to the SDK.

Recommendation: Conditionally render FlagProvider only when key exists, or use a more explicit fallback that won't accidentally authenticate.

---

4. React Query Stale Time Configuration

Location: components/frontend/src/services/queries/use-feature-flags-admin.ts:26-27

refetchInterval: 30000, // Refresh every 30s
staleTime: 10000, // Consider data stale after 10s

Issue: Data refetches every 30s but is considered stale after 10s. This means 20s windows where data shows as stale but hasn't refetched yet.

Recommendation: Align timings or increase staleTime to match refetchInterval for consistent UX.

---

5. Missing Loading State in Feature Flag Provider

Location: components/frontend/src/components/providers/feature-flag-provider.tsx:38-42

Issue: Renders children immediately during SSR/before mount, meaning early renders see all flags as disabled.

Current:

if (!mounted || !baseUrl) {
    return <>{children}</>;
}

Recommendation: Add loading state or document that flags default to false during hydration.

---

6. Code Duplication in Enable/Disable Handlers

Location: components/backend/handlers/featureflags_admin.go:504-640

Issue: EnableFeatureFlag (lines 504-571) and DisableFeatureFlag (lines 573-640) are 95% identical.

Recommendation: Extract common logic:

func setFeatureFlagOverride(c *gin.Context, enabled bool) {
    // ... shared logic ...
    cm.Data[flagName] = strconv.FormatBool(enabled)
    // ...
}

func EnableFeatureFlag(c *gin.Context) {
    setFeatureFlagOverride(c, true)
}

func DisableFeatureFlag(c *gin.Context) {
    setFeatureFlagOverride(c, false)
}

---

Positive Highlights

✅ Excellent Security Practices

1. User token authentication enforced on all endpoints (lines 140-145, 272-277, 318-323, etc.)
2. No token logging - proper use of len(token) pattern
3. Type-safe ConfigMap access - proper nil checks (lines 427, 485, 552, 621)
4. Proxy pattern - Admin credentials never exposed to frontend

✅ Comprehensive Testing

• 1078 lines of unit tests covering authentication, ConfigMap operations, error handling, edge cases
• Proper test isolation using fake K8s clients
• Security fix during testing - Response body redaction (commit message notes)

✅ Error Handling

• Proper IsNotFound handling (lines 89, 401, 470, 526, 594)
• Graceful degradation when Unleash not configured (lines 154-176)
• Structured error responses with appropriate HTTP status codes

✅ Frontend Best Practices

• React Query hooks for all data operations (no manual fetch)
• Shadcn UI components exclusively (Switch, Table, Badge, etc.)
• Proper loading/error states in feature-flags-settings.tsx
• Batch save pattern - local state with Save/Discard (lines 54-80)

✅ Documentation Quality

• ADR-0006 provides excellent architectural context and decision rationale
• Tag-based governance for flag visibility well-documented
• Constitution integration - links to Principle XI
• Deployment scripts with platform detection (OpenShift vs Kubernetes)

---

Recommendations

Priority 1: Security & RBAC

1. Add ConfigMap RBAC validation or document why backend SA is appropriate
2. Verify no token leaks in unleashAdminRequest error messages
3. Remove response body from log line 199

Priority 2: Code Quality

4. Extract shared logic from Enable/Disable handlers
5. Add flag existence validation in EvaluateFeatureFlag
6. Align React Query timing configurations

Priority 3: Polish

7. Document frontend flag hydration behavior
8. Consider explicit fallback handling for missing Unleash key

---

Architecture Compliance

✅ Follows CLAUDE.md patterns:

• User token authentication (Backend Development Context)
• Error handling patterns (error-handling.md)
• React Query usage (react-query-usage.md)
• Shadcn UI components (frontend-development.md)

✅ Follows ADR-0002:

• User token extraction and validation
• Project context validation middleware

✅ Follows Project Structure:

• Proper package organization (handlers/, featureflags/)
• Service layer separation (frontend: api/ + queries/)
• Colocated components (workspace-sections/)

---

Test Coverage Assessment

✅ Excellent coverage:

• Authentication requirements (401)
• ConfigMap operations (create/update/delete)
• Unleash API error handling (503)
• Override precedence logic
• Edge cases (nil data, concurrent ops)

Suggested additions:

• Integration test for full flag evaluation flow
• Frontend unit tests for FeatureFlagProvider
• E2E test for workspace admin toggling flags

---

Final Verdict

Approve with suggestions ✅

This is high-quality code that follows established patterns. The major issues are about RBAC validation consistency and potential token leakage - both addressable with small changes. The comprehensive testing and documentation demonstrate excellent engineering practices.

Before merge:

1. Address ConfigMap RBAC validation (Major Issue Outcome: Reduce Refinement Time with agent System #1)
2. Verify no token leaks in admin proxy errors (Major Issue Epic: RAT Architecture & Design #2)
3. Remove response body logging (Minor Issue Outcome: Reduce Refinement Time with agent System #1)

Post-merge improvements:

• Refactor Enable/Disable handlers to reduce duplication
• Add integration tests for full evaluation flow
• Document flag hydration behavior

---

🤖 Generated with Claude Code

---

🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

1. CLAUDE.md - Master project instructions, development standards
2. backend-development.md - Go backend, K8s integration patterns
3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
4. security-standards.md - Auth, RBAC, token handling
5. k8s-client-usage.md - User token vs service account patterns
6. error-handling.md - Consistent error patterns
7. react-query-usage.md - Data fetching patterns

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.

[github-actions]

Claude Code Review

Summary

This PR introduces comprehensive feature flag management using Unleash with workspace-scoped overrides. The implementation follows established patterns with good separation between platform-wide and workspace-specific controls. Code quality is generally high with proper use of user token authentication, React Query, and Shadcn UI components.

Issues by Severity

🚫 Blocker Issues

None - No blocking issues found.

🔴 Critical Issues

1. Potential Token Leakage in Admin API Calls (components/backend/handlers/featureflags_admin.go:649)

   • Line 649: req.Header.Set("Authorization", getUnleashAdminToken())
   • Issue: Admin token is being set directly without redaction in logs
   • Security Risk: If HTTP client logging is enabled, tokens could be exposed
   • Fix: Add explicit log redaction for Unleash admin API calls
   • Pattern Violation: Violates security-standards.md token handling rules

2. Using Backend Service Account Without User Permission Validation (components/backend/handlers/featureflags_admin.go:400-433)

   • Lines 400-433 in SetFeatureFlagOverride, 525-558 in EnableFeatureFlag, 594-627 in DisableFeatureFlag
   • Issue: Uses K8sClient (service account) to create/update ConfigMaps without RBAC check
   • Security Risk: User token is retrieved (line 140-145) but not used for ConfigMap operations
   • Fix Pattern: Should use user-scoped client for ConfigMap operations OR perform explicit RBAC check before using service account
   • Pattern Violation: Violates k8s-client-usage.md - "User operations must use GetK8sClientsForRequest"

🟡 Major Issues

3. Missing Type Safety in Frontend API Responses (components/frontend/src/services/api/feature-flags-admin.ts)

   • Lines 52-55: API client assumes response structure without validation
   • Issue: response.features || [] assumes features exists without type guard
   • Impact: Could break if API returns unexpected structure
   • Fix: Add response validation or proper error handling
   • Pattern Note: Frontend standards require proper type checking

4. Environment Variable Caching May Miss Runtime Updates (components/backend/handlers/featureflags_admin.go:654-686)

   • Package-level variables cache env vars (lines 27-33)
   • Getters lazy-load once (lines 654-686)
   • Issue: If env vars change at runtime (K8s ConfigMap updates), changes won't be detected
   • Impact: Requires pod restart to pick up Unleash config changes
   • Recommendation: Document this behavior or add runtime refresh capability

5. Frontend Route Handlers Missing Error Response Types (components/frontend/src/app/api/feature-flags/*/route.ts)

   • All Next.js route handlers (6 files)
   • Issue: Error responses are generic, not following typed error pattern
   • Impact: Frontend can't distinguish between error types
   • Fix: Add proper error response types and status code handling

6. Potential Race Condition in Feature Flag State (components/frontend/src/components/workspace-sections/feature-flags-settings.tsx:63-75)

   • Lines 63-75: useEffect resets local state on flagsKey change
   • Issue: If flags refetch while user is editing, unsaved changes could be lost
   • Impact: Poor UX - user loses changes during auto-refresh
   • Fix: Add confirmation dialog or skip refresh when hasChanges=true

🔵 Minor Issues

7. Inconsistent Error Logging Patterns (components/backend/handlers/featureflags_admin.go)

   • Lines 150, 184, 198: Some errors log, some don't before returning
   • Issue: Inconsistent with error-handling.md pattern ("always log before returning")
   • Fix: Ensure all error paths have contextual log statements

8. Missing Context Cancellation (components/backend/handlers/featureflags_admin.go:136, 267, etc.)

   • Multiple functions use context.Background() instead of request context
   • Issue: Can't cancel operations if HTTP request is cancelled
   • Fix: Use c.Request.Context() instead of context.Background()

9. Hardcoded HTTP Timeout (components/backend/handlers/featureflags_admin.go:644)

   • Line 644: Timeout: 10 * time.Second is hardcoded
   • Issue: Not configurable for different network environments
   • Fix: Make configurable via environment variable

10. Frontend Component Too Large (components/frontend/src/components/workspace-sections/feature-flags-settings.tsx)

    • 477 lines exceeds recommended 200 line limit
    • Issue: Violates frontend standards for component size
    • Fix: Extract sub-components (FeatureFlagRow, SaveControls, etc.)

11. Missing PropTypes Documentation (components/frontend/src/components/workspace-sections/feature-flags-settings.tsx)

    • Component props lack JSDoc comments
    • Impact: Harder to understand component API
    • Fix: Add JSDoc comments for props

Positive Highlights

✅ Excellent RBAC Implementation

• All endpoints properly call GetK8sClientsForRequest to verify user tokens (lines 140-145, 272-277, etc.)
• Proper 401 responses for missing tokens

✅ Proper React Query Usage

• Clean separation: api/ layer (pure functions) + queries/ layer (hooks)
• Query keys properly namespaced: featureFlagKeys.list(projectName)
• Mutations invalidate related queries correctly

✅ Shadcn UI Component Usage

• 100% Shadcn components (Switch, Table, Card, Badge, Alert, etc.)
• No custom UI built from scratch
• Follows frontend-development.md standards

✅ Type-Safe API Layer

• Good TypeScript types for all API functions
• Uses type over interface consistently
• Proper type exports from API layer

✅ Three-Tier Override Logic

• Clean architecture: ConfigMap override → Unleash default → disabled
• Well-documented in code and ADR-0006

✅ Comprehensive Tests

• 1080 lines of backend tests (featureflags_admin_test.go)
• Good coverage of edge cases

✅ Tag-Based Filtering

• Smart design: Only workspace-configurable flags shown in UI
• Platform-only flags hidden from workspace admins

Recommendations

High Priority

1. Fix RBAC Pattern Violation (Critical Epic: RAT Architecture & Design #2)

   • Add explicit RBAC check before ConfigMap operations OR
   • Use user-scoped client: reqK8s, _ := GetK8sClientsForRequest(c) for ConfigMap ops
   • Pattern: See handlers/sessions.go:250-260 for RBAC check example

2. Add Token Redaction (Critical Outcome: Reduce Refinement Time with agent System #1)

   • Implement custom HTTP client with redacting logger for Unleash API calls
   • Pattern: See server/server.go:22-34 for token redaction formatter

3. Fix Frontend State Race Condition (Major Epic: Testing & Validation #6)

   • Add enabled: \!hasChanges to useQuery refetchInterval
   • Or add confirmation: "Refresh will discard unsaved changes"

Medium Priority

4. Extract Frontend Sub-Components (Minor Bump actions/checkout from 4 to 5 #10)

   • Create FeatureFlagRow.tsx (table row logic)
   • Create FeatureFlagSaveControls.tsx (save/discard buttons)
   • Keeps main component under 200 lines

5. Use Request Context (Minor Test: Updated Workflow Validation #8)

   • Replace context.Background() with c.Request.Context()
   • Enables proper cancellation

6. Add Response Validation (Major Epic: Data Source Integration #3)

   • Validate API responses match expected types
   • Use zod or similar for runtime type checking

Documentation Improvements

7. Document Environment Variable Caching (Major Epic: AI Agent Development #4)

   • Add comment in code that pod restart is required for config changes
   • Or implement hot-reload capability

8. Add JSDoc Comments

   • Document component props and API function parameters

Code Quality Metrics

• ✅ Backend: Uses user token authentication correctly
• ✅ Backend: Proper error handling (mostly)
• ✅ Frontend: Zero any types
• ✅ Frontend: All React Query patterns followed
• ✅ Frontend: All Shadcn UI components
• ⚠️ Backend: Missing RBAC check for ConfigMap operations
• ⚠️ Frontend: Component exceeds size limit (477 lines)

Security Checklist

• ✅ User token extracted and validated
• ⚠️ ConfigMap operations use service account without RBAC check
• ⚠️ Admin token might leak in HTTP logs
• ✅ No tokens in application logs (except Unleash admin token handling)
• ✅ Input validation present (flag names, request bodies)
• ✅ Proper HTTP status codes

Overall Assessment

Quality Score: 7.5/10

This is a well-designed feature with good architectural patterns. The main concerns are:

1. RBAC gap in ConfigMap operations (violates established backend patterns)
2. Potential token leakage in Unleash admin API calls
3. Frontend state management race condition

Once these critical issues are addressed, this PR will be production-ready. The three-tier override system (workspace → platform → disabled) is elegant and the tag-based filtering is a smart design choice.

Recommendation: Request changes to address Critical #1 and #2 before merging.

---

🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

1. CLAUDE.md - Master project instructions, development standards
2. backend-development.md - Go backend, K8s integration patterns
3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
4. security-standards.md - Auth, RBAC, token handling
5. k8s-client-usage.md - User token vs service account patterns
6. error-handling.md - Consistent error patterns
7. react-query-usage.md - Data fetching patterns

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.