feat: add session-config repo support (spec.configRepo)

[jeremyeder]

No description provided.

[github-actions]

Claude Code Review

Summary

This PR adds support for session-level configuration repositories (spec.configRepo) and workspace-level default config repos (ProjectSettings.defaultConfigRepo). The implementation includes backend handlers, frontend UI, operator integration, runner hydration script, comprehensive tests (10 unit + 4 e2e), and removes dead code.

Overall Assessment: High-quality implementation with strong adherence to project standards. Clean code removal, comprehensive test coverage, and proper security patterns throughout.

---

Issues by Severity

🚫 Blocker Issues

None - No blocking issues found.

🔴 Critical Issues

1. Missing User Token Client Validation in Backend Handler

Location: components/backend/handlers/project_settings.go:23, 52

The handlers use user-scoped clients correctly (GetK8sClientsForRequest) but only check reqDyn for nil:

_, reqDyn := GetK8sClientsForRequest(c)
if reqDyn == nil {
    c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid or missing token"})
    c.Abort()
    return
}

Problem: Per CLAUDE.md line 530-535 and k8s-client-usage.md, you should check BOTH reqK8s and reqDyn:

reqK8s, reqDyn := GetK8sClientsForRequest(c)
if reqK8s == nil {  // Check reqK8s, not reqDyn
    c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid or missing token"})
    c.Abort()
    return
}

Why this matters: If GetK8sClientsForRequest fails, it returns nil, nil. Checking only reqDyn is correct in this case, but the pattern should match the rest of the codebase for consistency (see handlers/sessions.go:180, 227).

Fix: Change to check reqK8s instead of reqDyn for consistency with established patterns.

---

2. Type Assertion Without Safety Check

Location: components/backend/handlers/project_settings.go:84

spec, _, _ := unstructured.NestedMap(obj.Object, "spec")
if spec == nil {
    spec = map[string]interface{}{}
}

Problem: Per CLAUDE.md line 452-456 and error-handling.md, you must check the found return value:

// ❌ FORBIDDEN: Direct type assertions without checking
obj.Object["spec"].(map[string]interface{})

// ✅ REQUIRED: Use unstructured.Nested* helpers
spec, found, err := unstructured.NestedMap(obj.Object, "spec")
if \!found || err \!= nil {
    return fmt.Errorf("spec not found")
}

Current code: You're ignoring the found and err return values with _.

Fix:

spec, found, err := unstructured.NestedMap(obj.Object, "spec")
if err \!= nil {
    log.Printf("Failed to get spec: %v", err)
    c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to read spec"})
    return
}
if \!found {
    spec = map[string]interface{}{}
}

---

🟡 Major Issues

3. Direct Type Assertion in Sessions Handler

Location: components/backend/handlers/sessions.go:674

spec := session["spec"].(map[string]interface{})

Problem: This violates the type-safety rule (CLAUDE.md line 452-456). While this is existing code in the Create flow where spec was just created, it should still use safe access for defensive programming.

Recommendation:

spec, ok := session["spec"].(map[string]interface{})
if \!ok {
    log.Printf("Invalid spec type in session")
    c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to set config repo"})
    return
}

---

4. Frontend: Missing Type Definition for Config Repo

Location: components/frontend/src/types/project-settings.ts

The type definition is missing from the PR changes. The file was added but not shown in the diff.

Verify: Ensure this file exists and has proper types:

export type ProjectSettingsCR = {
  defaultConfigRepo?: {
    gitUrl: string;
    branch?: string;
  };
};

export type UpdateProjectSettingsRequest = {
  defaultConfigRepo?: {
    gitUrl: string;
    branch?: string;
  };
};

---

🔵 Minor Issues

5. Inconsistent Error Messages

Location: components/backend/handlers/project_settings.go:37

c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to read project settings"})

vs line 78:

c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to read project settings"})

Observation: The messages are identical for GET and UPDATE failures. Consider making them more specific:

• GET: "Failed to retrieve project settings"
• UPDATE: "Failed to update project settings"

---

6. Frontend: Potential Race Condition in useEffect

Location: components/frontend/src/components/create-session-dialog.tsx:98

useEffect(() => {
  if (projectSettings?.defaultConfigRepo?.gitUrl) {
    const current = form.getValues();

The code is cut off at line 100, but if this is setting form values in useEffect without proper dependencies, it could cause render loops.

Recommendation: Ensure the dependency array includes projectSettings and uses a guard to prevent infinite loops:

useEffect(() => {
  if (projectSettings?.defaultConfigRepo?.gitUrl) {
    form.setValue('configRepoUrl', projectSettings.defaultConfigRepo.gitUrl);
    if (projectSettings.defaultConfigRepo.branch) {
      form.setValue('configRepoBranch', projectSettings.defaultConfigRepo.branch);
    }
  }
}, [projectSettings, form]); // Ensure proper dependencies

---

7. Missing Context in Operator Log Messages

Location: components/operator/internal/handlers/sessions.go:794

base = append(base, corev1.EnvVar{Name: "CONFIG_REPO_GIT_URL", Value: gitURL})

Observation: No log message when config repo env vars are injected. For debugging, consider adding:

if gitURL, ok := configRepo["gitUrl"].(string); ok && strings.TrimSpace(gitURL) \!= "" {
    log.Printf("Injecting config repo for session %s/%s: %s", namespace, sessionName, gitURL)
    base = append(base, corev1.EnvVar{Name: "CONFIG_REPO_GIT_URL", Value: gitURL})
}

---

8. Dead Code Removal Not Reflected in Routes

Location: Check components/backend/routes.go

You removed repository.go and repository_test.go (196 lines deleted). Verify that:

• No routes still reference the deleted handlers
• No imports are left dangling

Action: Confirm with:

cd components/backend && grep -r "EnrichProjectSettingsWithProviders" .
grep -r "repository" routes.go

---

Positive Highlights

✅ Excellent Test Coverage: 10 unit tests (Ginkgo) + 4 e2e tests (Cypress) covering happy path, edge cases, and error conditions.

✅ Proper Security Patterns: User token authentication enforced, RBAC checks, proper auth tests with WithAuthCheckEnabled().

✅ Clean Code Removal: Removed 196 lines of dead code (repository.go, repository_test.go) - excellent housekeeping!

✅ Consistent Naming: defaultConfigRepo in ProjectSettings, configRepo in AgenticSession spec - clear and consistent.

✅ React Query Best Practices: Proper query hooks with invalidation, cache management, and mutation patterns per react-query-usage.md.

✅ Shadcn UI Compliance: All UI components use Shadcn (Input, Button, Card, etc.) - no custom components.

✅ Type Safety (Frontend): Zero any types, proper TypeScript usage throughout.

✅ CRD Updates: Proper OpenAPI schema in agenticsessions-crd.yaml and projectsettings-crd.yaml.

✅ Error Handling: Non-fatal handling in hydrate.sh - session continues without config overlay if clone fails, with error reporting via /workspace/.config-repo-error.

✅ Idempotent Operations: Retry logic in hydrate.sh (2 attempts), overlay pattern uses cp -rn (workspace files take precedence).

---

Recommendations

Prioritized Action Items

Before Merge:

1. Fix Critical Issue Outcome: Reduce Refinement Time with agent System #1: Change reqDyn to reqK8s check in project_settings.go:23, 52
2. Fix Critical Issue Epic: RAT Architecture & Design #2: Properly handle found and err from NestedMap at line 84
3. Verify Frontend Types: Confirm components/frontend/src/types/project-settings.ts exists with proper exports

Post-Merge (Nice-to-Have):

4. Add logging when config repo env vars are injected (operator)
5. Make error messages more specific (GET vs UPDATE)
6. Add safe type assertions in sessions.go:674

---

Architecture & Design

Design Pattern: The separation of concerns is excellent:

• spec.configRepo: Session-level override
• ProjectSettings.defaultConfigRepo: Workspace-level default
• UI pre-fills from defaults, allows per-session override

Hydration Strategy: The cp -rn overlay pattern (workspace files override config files) is the correct design choice - preserves user customizations.

API Design: RESTful endpoints (GET /PUT /api/projects/:project/project-settings) follow project conventions.

---

Testing Assessment

Unit Tests (project_settings_test.go):

• ✅ GET: empty response, with config, without config, auth required
• ✅ PUT: set config, clear config, preserve other fields, 404 on missing CR, reject invalid JSON, auth required

E2E Tests (project-settings.cy.ts):

• ✅ Empty settings for new workspace
• ✅ Save and retrieve config repo
• ✅ Clear config repo with empty gitUrl
• ✅ Pre-fill in Create Session dialog

Coverage: Excellent. All major flows covered.

---

Security Review

✅ Authentication: User token required for all operations
✅ Authorization: RBAC enforced via user-scoped clients
✅ Input Validation: URLs validated, empty strings handled
✅ Token Security: No tokens in logs
✅ Container Security: Not applicable (no new Job specs)

---

Performance Considerations

⚠️ Config Repo Clone: Blocking operation in hydrate.sh with 2 retries + 5s sleep. For large repos, this could delay session startup by 10+ seconds.

Mitigation: Already implemented - non-fatal error handling, depth=1 clone, single-branch fetch.

Future Optimization: Consider caching config repos at the cluster level (shared PVC), but out of scope for this PR.

---

Final Verdict

Approve with Minor Fixes ⭐⭐⭐⭐☆

This is a well-architected feature with strong adherence to project standards. The two critical issues are straightforward to fix and don't affect functionality - they're about consistency and defensive programming.

Estimated Fix Time: 5 minutes

Strengths: Clean code removal, comprehensive tests, proper React Query patterns, security-conscious, excellent UX (pre-fill from workspace defaults).

Weaknesses: Minor type-safety oversights in error handling paths (easily fixed).

---

🔍 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.