feat: implement V2 repository format with per-repo autoPush support (RHOAIENG-37639)

[adalton]

Summary

Implements the V2 repository format that replaces flat url/branch fields with nested input/output configuration, enabling fork-based workflows and per-repository autoPush control.

Changes

Backend & Operator

• V2 Schema: Repositories now use input: {url, branch} (required) and output: {url, branch} (optional)
• Validation: Rejects V1 format, validates V2 format, prevents identical input/output
• Per-repo autoPush: Each repository has independent autoPush flag
• Backward compatibility: Existing sessions continue working, new sessions must use V2
• Tests: 44 backend unit tests, 16 runner autoPush tests

Runner

• Agentic autoPush: Runner respects per-repo autoPush flags when pushing changes
• System prompt: Claude receives autoPush instructions for each repository
• Push logic: Automatic push only for repos with autoPush: true

Frontend

• Extended create-session-dialog: Multi-repository configuration UI
• V2 form validation: Empty URL validation, output object cleanup, required field indicators
• Conditional UI: AutoPush checkbox appears only when output URL is configured
• Empty state: Helpful message when no repositories configured
• Field labels: All inputs have clear labels (Repository URL*, Branch, Output URL, Output Branch)

CRD

• Updated AgenticSession CRD to include V2 repository schema
• Added input, output, autoPush fields

This review replaces #457

[github-actions]

Claude Code Review

Summary

This PR implements the V2 repository format with per-repo autoPush support. The implementation is well-structured, comprehensive, and follows project patterns closely. The code demonstrates excellent test coverage (44 backend unit tests, 16 runner tests), proper validation, and thoughtful error handling.

Issues by Severity

🚫 Blocker Issues

None identified. The PR is ready for merge from a code quality perspective.

🔴 Critical Issues

1. Token Security - Backend Service Account Client Usage (SECURITY)

• Location: components/backend/handlers/sessions.go:723-728
• Issue: provisionRunnerTokenForSession() uses backend service account clients (K8sClient, DynamicClient) instead of user-scoped clients
• Violation: CLAUDE.md Critical Rule Outcome: Reduce Refinement Time with agent System #1: "FORBIDDEN: Using backend service account for user-initiated API operations"
• Why Critical: Bypasses RBAC authorization checks. User could create tokens in projects they don't have access to.
• Fix Required:

  // CURRENT (WRONG - uses backend SA):
  if err := provisionRunnerTokenForSession(c, K8sClient, DynamicClient, project, name); err \!= nil {
  
  // SHOULD BE (use user-scoped clients):
  reqK8s, reqDyn := GetK8sClientsForRequest(c)
  if reqK8s == nil {
      log.Printf("Cannot provision token: invalid user token")
  } else if err := provisionRunnerTokenForSession(c, reqK8s, reqDyn, project, name); err \!= nil {
      log.Printf("Warning: failed to provision runner token: %v", err)
  }

• Context: The function creates ServiceAccounts, Roles, RoleBindings, and Secrets. Per CLAUDE.md, backend SA should ONLY be used for "Writing CRs after validation" and "Minting tokens/secrets for runners". However, the RBAC checks to verify user has permission to create these resources are missing. See .claude/patterns/k8s-client-usage.md Pattern 2 for the correct "Validate Then Escalate" pattern.

🟡 Major Issues

1. Inconsistent Empty Branch Handling

• Locations:
  • components/backend/handlers/helpers.go:96 (ParseRepoMap)
  • components/backend/types/session.go:ValidateRepo()
• Issue: ParseRepoMap() normalizes empty/whitespace-only branches to nil, but test case on line 166 expects whitespace to be preserved as-is
• Impact: Test "branch with leading/trailing whitespace is preserved as-is" expects Branch: StringPtr(" main ") but ParseRepoMap sets it to nil due to strings.TrimSpace(branch) \!= "" check
• Fix: Decide on canonical behavior:
  • Option A (recommended): Always normalize empty/whitespace-only to nil
  • Option B: Preserve whitespace (but validate Git won't accept it)
• Test Impact: Lines 156-194 in helpers_test.go may fail

2. Missing RBAC Permission Check Before Token Provisioning

• Location: components/backend/handlers/sessions.go:738-803
• Issue: No RBAC check to verify user can create ServiceAccounts/Roles/RoleBindings in the project namespace
• Pattern Violation: See CLAUDE.md "Pattern 2: Create Resource (Validate Then Escalate)" - missing Step 3
• Fix Required:

  // Before creating SA/Role/RoleBinding, check user has permission
  ssar := &authv1.SelfSubjectAccessReview{
      Spec: authv1.SelfSubjectAccessReviewSpec{
          ResourceAttributes: &authv1.ResourceAttributes{
              Resource:  "serviceaccounts",
              Verb:      "create",
              Namespace: project,
          },
      },
  }
  res, err := reqK8s.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, ssar, v1.CreateOptions{})
  if err \!= nil || \!res.Status.Allowed {
      return fmt.Errorf("unauthorized to provision runner tokens in project %s", project)
  }

3. Role Update Pattern Could Fail Silently

• Location: components/backend/handlers/sessions.go:756-767
• Issue: Role update succeeds even if user lacks update permission on Roles
• Risk: Failed update won't be logged/returned, leaving stale permissions
• Fix: Log update errors and consider returning them:

  if _, err := reqK8s.RbacV1().Roles(project).Update(c.Request.Context(), role, v1.UpdateOptions{}); err \!= nil {
      log.Printf("Failed to update Role %s: %v", roleName, err)
      return fmt.Errorf("update Role: %w", err)
  }

🔵 Minor Issues

1. Missing Error Context in ParseRepoMap

• Location: components/backend/handlers/sessions.go:179-183
• Issue: Error from ParseRepoMap() is logged but loses context about which repo index failed
• Fix: Include repo index in error message:

  r, err := ParseRepoMap(m)
  if err \!= nil {
      log.Printf("Skipping invalid repo at index %d: %v", len(repos), err)
      continue
  }

2. Token Refreshed-At Annotation Not Used

• Location: components/backend/handlers/sessions.go:45 (constant defined but no readers)
• Issue: runnerTokenRefreshedAtAnnotation is written but never read by backend/operator
• Impact: Low (annotation is harmless, but unused code)
• Fix: Either implement token refresh logic or remove annotation

3. Frontend: Missing Required Field Validation on Form Submission

• Location: components/frontend/src/components/create-session-dialog.tsx:94-102
• Issue: Form allows submission with empty input.url (only filtered during cleanup)
• Risk: Backend returns 400 error instead of client-side validation
• Fix: Add Zod validation for required input.url:

  const sessionRepoSchema = z.object({
    input: z.object({
      url: z.string().min(1, "Repository URL is required"),
      branch: z.string().optional(),
    }),
    // ...
  })

4. Frontend: Inconsistent Type Usage (Type vs Interface)

• Location: Multiple files in components/frontend/src/types/
• Issue: Mix of type and interface definitions
• Pattern Violation: CLAUDE.md Frontend Rule Epic: AI Agent Development #4: "Use type over interface"
• Fix: Convert all interfaces to types (low priority - doesn't affect functionality)

Positive Highlights

✅ Exceptional Test Coverage: 44 backend unit tests + 16 runner tests demonstrate thorough validation
✅ Type Safety: Proper use of unstructured.Nested* helpers throughout
✅ Validation Synchronization: Cross-reference comments between ValidateRepo() and ParseRepoMap() prevent divergence
✅ Error Handling: Consistent patterns with proper logging and context
✅ OwnerReferences: Correctly set on all child resources (SA, Role, RoleBinding, Secret)
✅ Round-Trip Testing: session_test.go verifies serialization/deserialization integrity
✅ Documentation: Comprehensive commit message and PR description
✅ No Panic Usage: Follows "Never Panic in Production Code" pattern
✅ Frontend UX: Conditional autoPush checkbox, empty states, required field indicators

Recommendations

Immediate (Before Merge)

1. Fix Critical Security Issue: Add RBAC check before provisionRunnerTokenForSession() or use user-scoped clients
2. Fix Branch Normalization: Align ParseRepoMap behavior with test expectations
3. Add Permission Check: Validate user can create ServiceAccounts before provisioning tokens

Short-Term (Next PR)

1. Implement token refresh logic using runnerTokenRefreshedAtAnnotation
2. Add frontend Zod validation for required repository URL
3. Add integration tests for V2 repo format end-to-end flow

Long-Term

1. Consider extracting token provisioning to operator for consistency with existing patterns
2. Add metrics for autoPush success/failure rates
3. Document V2 migration guide for existing users

---

Overall Assessment: This is high-quality work with comprehensive testing and proper patterns. The critical security issue with token provisioning must be addressed before merge, but the rest is ready.

Recommendation: APPROVE WITH CONDITIONS - Fix critical security issue, then merge.

Confidence Level: HIGH (based on thorough code review against CLAUDE.md standards)

---

🔍 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 implements the V2 repository format with per-repo autoPush support (RHOAIENG-37639). The implementation is comprehensive and follows established patterns across backend, frontend, operator, and runner components. The code demonstrates strong adherence to project standards with excellent test coverage (44 backend unit tests, 16 runner tests).

Issues by Severity

🚫 Blocker Issues

None identified. The PR is ready for merge from a code quality perspective.

🔴 Critical Issues

None identified. Security, authentication, and error handling patterns are correctly implemented.

🟡 Major Issues

1. Validation Logic Duplication Between ParseRepoMap() and ValidateRepo()

Location: components/backend/handlers/helpers.go:84-141 and components/backend/types/session.go:131-161

Issue: While both functions have cross-reference comments acknowledging the duplication, having validation logic in two places creates maintenance risk. The comments help but don't eliminate the risk of divergence.

Impact: If validation rules change (e.g., new constraints on URL format, branch naming), developers must remember to update both locations.

Recommendation: Consider one of these approaches:

1. Make ParseRepoMap() call ValidateRepo() after parsing
2. Extract shared validation logic to a common function
3. If keeping separate for performance, add integration tests that verify both functions reject the same invalid inputs

Example refactor:

// In handlers/helpers.go
func ParseRepoMap(m map[string]interface{}) (types.SimpleRepo, error) {
    r := types.SimpleRepo{}
    
    // ... parsing logic ...
    
    // Use the canonical validation
    if err := r.ValidateRepo(); err != nil {
        return r, err
    }
    
    return r, nil
}

2. Frontend: Potential Type Safety Issue with Optional Fields

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

Issue: The cleanup logic for repos uses runtime checks instead of TypeScript types:

const cleanedRepos = values.repos
  ?.filter(repo => repo.input.url && repo.input.url.trim() !== "")
  .map(repo => ({
    input: repo.input,
    output: repo.output?.url && repo.output.url.trim() !== ""
      ? repo.output
      : undefined,

Impact: Minor - works correctly but could be more type-safe.

Recommendation: Consider using Zod's .transform() or .refine() in the schema to handle cleanup declaratively rather than in the submit handler.

🔵 Minor Issues

1. Inconsistent Error Messages

Location: Various validation error messages

Observation: Some errors say "input is required" while others say "input.url is required". Consider standardizing the format for consistency (e.g., always use dot notation for nested fields).

2. Magic String for Default Branch

Location: components/backend/handlers/content.go:273

Before:

branch := "main"

After:

branch := types.DefaultBranch

Status: ✅ Already fixed in this PR. Good consistency improvement.

3. Test Coverage for Edge Cases

Location: Test files

Observation: Excellent test coverage overall (44 backend unit tests). Consider adding a few more edge cases:

• Input/output with same URL but one has nil branch, other has empty string
• Very long URLs (test length limits if any)
• URLs with special characters (spaces, unicode)

Priority: Low - current coverage is already excellent.

Positive Highlights

✅ Excellent Adherence to Backend Standards

1. Type-Safe Unstructured Access - Consistently uses unstructured.Nested* helpers with proper error checking:

   inputMap, hasInput := m["input"].(map[string]interface{})
   if !hasInput {
       return r, fmt.Errorf("input is required in repository configuration")
   }

2. Proper Error Handling - All errors are logged with context and appropriate HTTP status codes are returned.

3. No Token Leaks - No tokens or sensitive data exposed in logs or error messages.

4. Security Context - Although not shown in this PR's changes, the existing job creation patterns correctly set SecurityContext.

✅ Comprehensive Test Coverage

• 44 backend unit tests covering validation, parsing, serialization, and round-trip scenarios
• 16 runner autoPush tests (709 lines) covering single-repo, multi-repo, and edge cases
• Table-driven tests following Go best practices
• Clear test names describing what is being tested

Example of excellent test structure:

{
    name: "valid V2 format with autoPush",
    input: map[string]interface{}{
        "input": map[string]interface{}{
            "url":    "https://github.com/user/repo",
            "branch": "main",
        },
        "output": map[string]interface{}{
            "url":    "https://github.com/user/fork",
            "branch": "feature",
        },
        "autoPush": true,
    },
    want: types.SimpleRepo{
        // ...
    },
    wantErr: false,
},

✅ Frontend Follows Design Guidelines

1. React Hook Form with Zod validation - proper form state management
2. Shadcn UI components - Button, Dialog, Form, Input, Checkbox all from @/components/ui/*
3. React Query integration - Uses useCreateSession mutation hook
4. Type safety - No any types detected
5. Field arrays - Proper use of useFieldArray for dynamic repository list

✅ Backward Compatibility Maintained

• CRD changes use optional fields (input, output, autoPush)
• V2 format is enforced for new sessions but doesn't break existing CRs
• Clear error messages guide users to V2 format

✅ Clear Separation of Concerns

• Types (types/session.go) - Pure data structures with validation methods
• Handlers (handlers/sessions.go) - HTTP logic only
• Helpers (handlers/helpers.go) - Reusable parsing/validation utilities
• Tests - Separate test files for each component

✅ Operator Integration

The operator correctly:

• Parses V2 format using ParseRepoMap()
• Handles drift detection by comparing input URLs
• Passes full V2 structure (input/output/autoPush) to runner API

Recommendations

High Priority

1. Address Validation Duplication - Refactor to eliminate the duplicated validation logic between ParseRepoMap() and ValidateRepo() as described in Major Issue Outcome: Reduce Refinement Time with agent System #1.

Medium Priority

2. Add Integration Test - Create an integration test that verifies ParseRepoMap() and ValidateRepo() reject the same invalid inputs. This would catch divergence even if they remain separate.

3. Document V2 Migration Path - Consider adding a migration guide or ADR documenting:

   • Why V2 was needed
   • How to convert V1 configs to V2
   • What happens to existing V1 sessions

Low Priority

4. Frontend Type Safety - Consider moving repo cleanup logic to Zod schema transforms.

5. Add Edge Case Tests - Add tests for URLs with special characters, very long URLs, and branch name edge cases.

Code Quality Metrics

• ✅ Go formatting: gofmt compliant
• ✅ Error handling: All errors logged with context
• ✅ Type safety: No unsafe type assertions
• ✅ Security: Token handling correct, no leaks
• ✅ Tests: 60 total tests (44 backend + 16 runner)
• ✅ Documentation: Clear comments and cross-references

Final Verdict

✅ APPROVED FOR MERGE with recommendation to address validation duplication post-merge (non-blocking).

This is a well-implemented feature that demonstrates:

• Strong understanding of project patterns
• Comprehensive testing
• Excellent code organization
• Proper security practices
• Good backward compatibility

The validation duplication issue is the only notable concern, and it's documented with cross-reference comments to prevent divergence. This can be refactored in a follow-up PR without blocking this feature.

---

Great work on this implementation! The V2 repository format is a significant enhancement that enables fork-based workflows and per-repo automation control.

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com

---

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