fix: GitHub integration not showing as connected after App install

[natifridman]

Fix multiple issues preventing GitHub App installations from being reflected as "Connected" in the integrations UI:

• Forward OAuth code param from GitHub callback to backend for verified ownership via token exchange (reuses existing helpers)
• Fix connectGitHub() return value: backend returns message, not username
• Fix GitHubConnectResponse type to match actual backend response
• Fix cache invalidation: mutations now also invalidate ['integrations', 'status'] (used by the Integrations page), not just ['github', 'status']
• Remove redirect_uri override from connect URL so GitHub uses the configured Callback URL in App settings
• Add debug logging to installation lookup and status queries

fix #602

[github-actions]

Claude Code Review

Summary

This PR fixes GitHub App installation integration issues by implementing verified ownership via OAuth code exchange and correcting type mismatches between frontend and backend. The changes are well-structured and follow established patterns, but there are several areas requiring attention before merge.

Issues by Severity

🚫 Blocker Issues

None identified - No issues that absolutely prevent merge.

🔴 Critical Issues

1. Missing Test Coverage for New OAuth Code Path (components/backend/handlers/github_auth.go:428-444)

• Issue: The new OAuth code verification logic (lines 428-444) lacks test coverage
• Risk: Untested security-critical code path for ownership verification
• Impact: The existing test file (github_auth_test.go) only tests the basic installation flow without the code parameter
• Fix Required: Add test cases covering:
  • Successful OAuth code exchange and ownership verification
  • Failed code exchange (invalid code)
  • Ownership verification failure (user doesn't own installation)
  • Graceful degradation when OAuth exchange fails

Example test structure needed:

It("Should verify ownership when OAuth code is provided", func() {
    requestBody := map[string]interface{}{
        "installationId": 12345,
        "code":           "valid-oauth-code",
    }
    // Mock HTTP responses for OAuth exchange
    // Assert ownership verification succeeds
})

It("Should reject installation when user doesn't own it", func() {
    requestBody := map[string]interface{}{
        "installationId": 12345,
        "code":           "code-for-different-user",
    }
    // Assert returns 403 Forbidden
})

🟡 Major Issues

1. Inconsistent Error Handling Pattern (components/backend/handlers/github_auth.go:430-432)

• Issue: OAuth exchange failure is logged but silently falls through to best-effort enrichment

if err != nil {
    log.Printf("LinkGitHubInstallationGlobal: OAuth code exchange failed for user=%s: %v", userIDStr, err)
    // Fall through to best-effort enrichment below
}

• Expected Pattern: Per error-handling.md, should either return error or clearly document why it's non-fatal
• Recommendation: Add comment explaining why this is intentional (backward compatibility with installations before OAuth verification)

if err != nil {
    // OAuth exchange is optional - installation can proceed without verification
    // for backward compatibility with GitHub App installations that don't use OAuth flow
    log.Printf("LinkGitHubInstallationGlobal: OAuth code exchange failed for user=%s: %v", userIDStr, err)
}

2. Potential Token Leak via Query Parameters (components/frontend/src/app/integrations/github/setup/page.tsx:16)

• Issue: OAuth code is extracted from URL query parameters and could appear in browser history/logs
• Current Code:

const code = url.searchParams.get('code')

• Security Standards Violation: Per security-standards.md, tokens should never be in logs
• Mitigation: While OAuth codes are single-use and short-lived, consider:
  1. Immediately clearing the URL after extraction (using window.history.replaceState)
  2. Adding a comment documenting the security consideration

const code = url.searchParams.get('code')
// Clear sensitive OAuth code from URL to prevent browser history exposure
if (code) {
  window.history.replaceState({}, document.title, url.pathname + '?installation_id=' + installationId)
}

3. Type Mismatch Not Fully Corrected (components/frontend/src/types/api/github.ts:92-93)

• Issue: Response type changed from username: string to installationId: number, but backend actually returns message field
• Backend Response (github_auth.go:475-478):

c.JSON(http.StatusOK, gin.H{
    "message":        "GitHub App installation linked successfully",
    "installationId": installation.InstallationID,
})

• Frontend Type:

export type GitHubConnectResponse = {
  message: string;
  installationId: number;  // ✅ Correct
};

• But Frontend Usage (services/api/github.ts:37):

return response.message;  // ✅ Correct

• Status: Actually correct on review - false alarm, but shows good attention to type safety

🔵 Minor Issues

1. Debug Logging Should Use Structured Logging (components/backend/handlers/integrations_status.go:48, 57, 62)

• Issue: New debug logs use log.Printf instead of structured logging
• Current:

log.Printf("getGitHubStatusForUser: querying status for user=%s", userID)

• Preferred Pattern (from backend-development.md): Use structured fields

log.Printf("getGitHubStatusForUser: user=%s action=query_status", userID)

• Impact: Low - logs are functional, just not optimally structured for parsing

2. Cache Invalidation Could Be More Granular (components/frontend/src/services/queries/use-github.ts:79-80)

• Issue: Invalidates all queries with key ['integrations', 'status']
• Current:

queryClient.invalidateQueries({ queryKey: ['integrations', 'status'] });

• Optimization: Could be more specific to avoid unnecessary refetches

queryClient.invalidateQueries({ 
  queryKey: ['integrations', 'status'],
  exact: true  // Only invalidate exact match, not partial matches
});

• Impact: Negligible performance impact in practice

3. Missing Comment on redirect_uri Removal (components/frontend/src/components/github-connection-card.tsx:44-46)

• Issue: Comment explains why redirect_uri was removed, which is good, but could be clearer
• Current:

// Let GitHub use the Callback URL configured in the App settings
// rather than overriding with redirect_uri (which GitHub ignores
// unless it matches a configured callback URL)

• Suggestion: Add reference to GitHub docs or explain the "why" more clearly:

// GitHub App installations redirect to the Callback URL configured in App settings.
// The redirect_uri parameter is ignored by GitHub unless it exactly matches
// a pre-configured callback URL. Since our callback URL is already configured
// in the GitHub App settings, we omit redirect_uri to use the default.
// See: https://docs.github.com/en/apps/creating-github-apps/writing-code-for-a-github-app/building-a-login-with-github-button-with-a-github-app

Positive Highlights

✅ Excellent Type Safety - Fixed type mismatches between frontend and backend systematically
✅ Security-Conscious - Added ownership verification via OAuth code exchange
✅ Good Logging - Added debug logging at key decision points for troubleshooting
✅ Follows React Query Patterns - Proper cache invalidation on mutations (per react-query-usage.md)
✅ Backward Compatible - Falls back gracefully when OAuth is not configured
✅ Code Reuse - Leverages existing exchangeOAuthCodeForUserToken and userOwnsInstallation helpers
✅ Clear PR Description - Well-documented changes with context

Recommendations

High Priority (Before Merge)

1. Add Test Coverage - Write tests for OAuth code verification path (see Critical Issue Outcome: Reduce Refinement Time with agent System #1)
2. Document Error Handling - Add comment explaining why OAuth exchange failure is non-fatal (see Major Issue Outcome: Reduce Refinement Time with agent System #1)
3. Security Hardening - Clear OAuth code from URL after extraction (see Major Issue Epic: RAT Architecture & Design #2)

Medium Priority (Can Address in Follow-up)

4. Structured Logging - Refactor debug logs to use structured fields (see Minor Issue Outcome: Reduce Refinement Time with agent System #1)
5. Documentation Enhancement - Improve redirect_uri removal comment with GitHub docs link (see Minor Issue Epic: Data Source Integration #3)

Low Priority (Nice-to-Have)

6. Query Invalidation - Use exact: true for more granular cache invalidation (see Minor Issue Epic: RAT Architecture & Design #2)

Code Quality Checklist

✅ Follows CLAUDE.md backend patterns
✅ Follows CLAUDE.md frontend patterns
✅ Uses user-scoped clients appropriately (N/A - no K8s client usage)
⚠️ Error handling follows patterns (needs documentation)
❌ Test coverage for new functionality (OAuth verification path missing)
✅ Security standards followed (with minor improvement needed)
✅ Type safety maintained
✅ No token leaks in logs

Final Verdict

Approval Status: ⚠️ Approve with Recommendations

This PR solves the core issue effectively and is architecturally sound. The critical missing piece is test coverage for the OAuth verification path, which is a security-critical code change. The other issues are minor and can be addressed quickly.

Suggested Actions:

1. Add tests for OAuth code verification (30-45 minutes)
2. Add error handling documentation comment (2 minutes)
3. Clear OAuth code from URL after extraction (5 minutes)
4. Merge after tests pass

Estimated Time to Address: ~1 hour

---

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