fix: derive git identity from GitHub/GitLab credentials at runtime

[natifridman]

Previously, git commits in sessions were attributed to "Ambient Code Bot" because git user.name and user.email were not configured from the user's GitHub/GitLab credentials.

This fix:

• Enhances GitHub/GitLab credential endpoints to fetch user identity from their respective APIs and return userName, email, and provider fields
• Updates the runner to configure git identity (both git config and environment variables) when populating runtime credentials
• GitHub identity takes precedence over GitLab when both are configured
• Falls back to bot identity when no credentials have user info

Fixes: GitHub credentials aren't mounted to session
Also: Adds provider field to distinguish GitHub vs GitLab credentials

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[github-actions]

Claude Code Review

Summary

This PR enhances GitHub and GitLab credential endpoints to fetch user identity (name, email) from provider APIs and configures git identity dynamically in the runner. This ensures commits are properly attributed to users rather than the default bot identity. The implementation follows established patterns with strong test coverage.

Issues by Severity

🚫 Blocker Issues

None identified - code is ready to merge.

🔴 Critical Issues

None identified.

🟡 Major Issues

1. GitHub API Token Header Uses Deprecated Format (runtime_credentials.go:414)
The GitHub API token is sent using Authorization: token <token> which is deprecated. GitHub now recommends Authorization: Bearer <token>.

Current:

req.Header.Set("Authorization", "token "+token)

Recommended:

req.Header.Set("Authorization", "Bearer "+token)

2. Error Response Bodies Not Read (runtime_credentials.go:425-430)
When GitHub/GitLab API calls fail, error response bodies are not read. This prevents detailed error logging and can cause connection leaks.

Suggested improvement:

if resp.StatusCode != http.StatusOK {
    body, _ := io.ReadAll(resp.Body)
    if resp.StatusCode == http.StatusForbidden {
        log.Printf("GitHub API /user returned 403 (token may lack 'read:user' scope). Response: %s", string(body))
    } else {
        log.Printf("GitHub API /user returned status %d: %s", resp.StatusCode, string(body))
    }
    return "", ""
}

3. HTTP Client Timeout Consistency
All HTTP clients use 10-second timeout (runtime_credentials.go:381, 407, 474), but this is not configurable. Consider making it a package-level constant for easier tuning:

const apiCallTimeout = 10 * time.Second

🔵 Minor Issues

1. Test Coverage Gap - Integration Tests (runtime_credentials_test.go:93-109)
The Go tests for API endpoints are skipped (Skip("Requires full integration test setup")). While unit tests exist for helpers, integration tests would verify the full flow.

Recommendation: Consider adding integration tests that use mock K8s clients (similar to existing backend integration tests).

2. Missing Context Cancellation Check (runtime_credentials.go:408, 475)
HTTP requests use http.NewRequestWithContext(ctx) but don't check if the context is already cancelled before making the call:

if ctx.Err() != nil {
    log.Printf("Context cancelled before fetching user identity")
    return "", ""
}

3. Potential Data Race in Environment Variables (auth.py:352-353)
configure_git_identity sets environment variables (GIT_USER_NAME, GIT_USER_EMAIL) without synchronization. If multiple sessions run concurrently (in different processes), this is fine. Document this assumption or add synchronization if needed.

4. Magic String "user@example.com" (auth.py:296)
The code checks user_email != "user@example.com" which appears to be a sentinel value. Consider using a constant:

EXAMPLE_EMAIL_SENTINEL = "user@example.com"
if user_email and user_email != EXAMPLE_EMAIL_SENTINEL:

Positive Highlights

✅ Excellent Security Practices

• RBAC validation ensures users can only access their own session credentials (runtime_credentials.go:57-67)
• No token logging (follows token redaction patterns)
• Type-safe unstructured access throughout

✅ Comprehensive Test Coverage

• Python tests cover all scenarios: GitHub-only, GitLab-only, both providers, precedence logic
• Tests validate fallback behavior when credentials are missing
• Error handling scenarios well-tested

✅ Follows Established Patterns

• Uses GetK8sClientsForRequest(c) for user authentication ✓
• Type-safe unstructured.NestedString for extracting fields ✓
• Proper error handling with errors.IsNotFound checks ✓
• Graceful degradation (falls back to bot identity if user identity unavailable)

✅ Provider Distinction

• provider field allows runner to distinguish GitHub vs GitLab credentials
• Clear precedence documented: GitHub > GitLab > bot defaults

✅ User Experience Improvement

• Commits will now be properly attributed to users
• Transparent fallback mechanism ensures sessions still work without identity

Recommendations

Priority 1 (Address Before Merge)

1. Update GitHub API token header format to Bearer (runtime_credentials.go:414)

Priority 2 (Follow-up PRs)

1. Read error response bodies for better debugging (runtime_credentials.go:425-430, 491-493)
2. Extract HTTP timeout to a constant
3. Add integration tests for credential endpoints

Priority 3 (Nice-to-Have)

1. Add context cancellation checks before API calls
2. Document environment variable thread-safety assumptions in Python runner
3. Replace magic string with constant (auth.py:296)

Architecture Alignment

✅ CLAUDE.md Compliance:

• Backend follows user token authentication pattern
• Type-safe unstructured access used correctly
• Error handling matches established patterns
• No panic() in production code
• Security standards followed (token redaction, RBAC)

✅ ADR Compliance:

• ADR-0002 (User Token Authentication): User-scoped clients used for session access ✓
• ADR-0004 (Go Backend, Python Runner): Clear separation maintained ✓

Conclusion

This is a well-designed and thoroughly tested enhancement that solves a real UX problem (proper commit attribution). The code follows project standards and includes comprehensive test coverage. The identified issues are minor and most can be addressed in follow-up PRs.

Recommendation: Approve with Priority 1 fix applied (GitHub token header format).

---

🔍 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 successfully addresses the git identity attribution issue by fetching user identity from GitHub/GitLab APIs at runtime and configuring git credentials in the runner. The implementation follows established patterns with comprehensive test coverage and good error handling.

Issues by Severity

🚫 Blocker Issues

None - PR is ready to merge after addressing critical issues below.

🔴 Critical Issues

1. Potential Token Leakage in Logs (backend) - runtime_credentials.go:465, 532

• Location: runtime_credentials.go:465, runtime_credentials.go:532
• Issue: Logging user identity with name could indirectly leak sensitive information if names contain identifiable patterns
• Pattern Violation: Security standards require minimal logging of user-related data
• Recommendation: Consider using nameLength instead of full name, or ensure this is acceptable for your security posture

2. Missing io.ReadAll Response Size Limits - runtime_credentials.go:442, 509

• Location: runtime_credentials.go:442, runtime_credentials.go:509
• Issue: Unbounded io.ReadAll could cause memory exhaustion with large/malicious responses
• Pattern: Error responses are limited but success responses are not
• Recommendation: Add size limit to prevent DoS with io.LimitReader

🟡 Major Issues

3. Race Condition in Git Config - auth.py:360-369

• Location: auth.py:360-369 (configure_git_identity)
• Issue: Sequential subprocess.run calls without checking return codes could leave git in inconsistent state
• Scenario: If first command succeeds but second fails, user.name is set but user.email is not
• Recommendation: Check return codes or log failures

4. Context Cancellation Not Propagated to HTTP Client - runtime_credentials.go:414, 486

• Location: runtime_credentials.go:414, runtime_credentials.go:486
• Issue: http.Client created without context-aware timeout could continue running after context cancellation
• Note: This is partially mitigated by NewRequestWithContext, but verify client.Do propagates cancellation

🔵 Minor Issues

5. Test Coverage Gap - runtime_credentials_test.go

• Location: runtime_credentials_test.go:44-51
• Issue: Tests document that actual API integration testing is difficult without dependency injection
• Recommendation: Consider adding httptest-based integration tests or refactor functions to accept custom HTTP clients

6. Python Import Inside Function - auth.py:348

• Location: auth.py:348
• Issue: import subprocess inside configure_git_identity function
• Pattern: Imports should be at module level unless there is a specific reason
• Recommendation: Move to top of file with other imports

7. Inconsistent Error Logging - auth.py:371-374

• Location: auth.py:371-374
• Issue: Specific exception types logged as warnings, generic Exception logged as error with traceback
• Recommendation: Consider logging all failures consistently or add comments explaining rationale

Positive Highlights

✅ Excellent Test Coverage: 403 lines of comprehensive Python tests covering all edge cases

✅ Security Conscious: Token validation, RBAC checks, context cancellation handling

✅ Graceful Degradation: Falls back to Ambient Code Bot identity when credentials unavailable

✅ Clear Precedence Logic: GitHub takes precedence over GitLab, well-documented

✅ Type-Safe Backend Code: Proper use of unstructured.NestedString

✅ Follows Error Handling Patterns: Logs errors with context, returns empty strings on failure

✅ No Token Leakage: Tokens are never logged, only metadata like hasEmail

✅ Proper Context Usage: http.NewRequestWithContext used throughout

Recommendations

Priority 1 (Before Merge):

1. Add size limits to io.ReadAll for GitHub/GitLab API response bodies (issue Epic: RAT Architecture & Design #2)
2. Check subprocess return codes or log failures in git config (issue Epic: Data Source Integration #3)
3. Move subprocess import to top of auth.py (issue Epic: Testing & Validation #6)

Priority 2 (Follow-up PR):

4. Consider reducing user identity logging if security team deems it sensitive (issue Outcome: Reduce Refinement Time with agent System #1)
5. Add httptest-based integration tests for GitHub/GitLab API fetching (issue Epic: Jira Integration & Workflow #5)
6. Refactor for dependency injection to improve testability (issue Epic: Jira Integration & Workflow #5)

Priority 3 (Nice to Have):

7. Document rate limiting for GitHub/GitLab API calls
8. Add metrics/observability for identity fetch success/failure rates

Architectural Review

✅ Separation of Concerns: Backend fetches identity, runner configures git

✅ Backward Compatible: Falls back to bot identity when no credentials available

✅ Multi-Provider Support: Provider field distinguishes GitHub from GitLab

✅ Stateless Design: Identity fetched at runtime, not stored in session CR

Compliance Checklist

• Authentication: Uses user-scoped clients
• Authorization: RBAC checks performed
• Error Handling: All errors logged with context
• Token Security: No tokens in logs
• Type Safety: Uses unstructured.Nested* helpers
• Tests: Comprehensive coverage (525 lines total)
• Logging: Structured logs with context
• Code Quality: Properly formatted

Summary Verdict

APPROVE with minor fixes recommended before merge

This is a well-implemented fix that follows the platforms security and error handling patterns. The critical issues should be addressed before merge to prevent potential DoS and ensure consistent git configuration state. The code is production-ready after these fixes.

Great work on comprehensive test coverage and graceful degradation!

---

Review by Claude Code with loaded context from CLAUDE.md, backend-development.md, security-standards.md, k8s-client-usage.md, and error-handling.md

---

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