Use-nango-instead-of-keytar

[omar-inkeep]

cloud deployments should ignore keytar (it is not an optional dependency).

to use keytar locally, pnpm will ask to do pnpm approve-builds.

oauth will use keytar if available, or will fall back to nango (self-hosted or cloud)

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
agents-manage-api	Ready	Preview	Comment	Sep 30, 2025 5:00pm
agents-manage-ui	Ready	Preview	Comment	Sep 30, 2025 5:00pm
agents-run-api	Ready	Preview	Comment	Sep 30, 2025 5:00pm

1 Skipped Deployment

Project	Deployment	Preview	Comments	Updated (UTC)
agents-docs	Skipped			Sep 30, 2025 5:00pm

[claude]

Claude finished @omar-inkeep's task —— View job

---

Code Review Completed

I've conducted a comprehensive review of the keytar → nango migration. Overall, this is a well-architected change that improves cloud deployment compatibility while maintaining backward compatibility. Here are my detailed findings:

✅ System Design & Architecture

Excellent fallback strategy: The OAuth route (oauth.ts:178-209) implements a clean keychain → nango fallback pattern that's both resilient and maintainable.

Proper abstraction: The CredentialStoreRegistry pattern allows seamless switching between storage backends without breaking existing code.

Smart optional dependency handling: Making keytar optional via optionalDependencies is the right approach for cloud deployments.

🔍 Code Quality & Implementation

Strong Points

1. Robust Error Handling: The nango-store implementation handles various edge cases gracefully (malformed JSON, API failures, network issues)

2. Comprehensive Input Validation: Uses Zod schemas for credential key validation (nango-store.ts:11-14)

3. Security-Conscious: Properly sanitizes metadata and limits key logging to prevent credential leakage

4. Race Condition Mitigation: The "optimistic creation" pattern in createNangoApiKeyConnection (nango-store.ts:226-238) correctly handles concurrent integration creation

Areas for Improvement

🔶 Medium Priority Issues

1. Inconsistent Environment Variable Patterns (nango-store.ts:264, 282)

   // Mix of process.env and config parameter access
   const importConnectionUrl = `${process.env.NANGO_SERVER_URL || 'https://api.nango.dev'}/connections`;
   Authorization: `Bearer ${process.env.NANGO_SECRET_KEY}`

   Recommendation: Use the constructor config consistently or document why direct env access is needed here.

2. String-based Error Handling (oauth.ts:190-192)

   } catch {
     // Fall through to Nango fallback
   }

   Recommendation: Log the specific error for debugging. Silent failures make troubleshooting difficult in production.

3. Mixed Credential Key Formats

   • Keychain uses simple strings (oauth_token_${toolId})
   • Nango requires JSON objects ({connectionId, providerConfigKey})

   Recommendation: Consider standardizing on a unified credential key interface to reduce complexity.

🔶 Package Management & Dependencies

4. Workspace Configuration (pnpm-workspace.yaml)

   • Removed 3 lines but only added 1, suggesting cleanup of unused workspace entries
   • ✅ Good: Maintains clean workspace structure

5. Bundle Configuration

   • ✅ Correctly externalizes keytar in build configs
   • ✅ Prevents bundling issues in cloud environments

🔍 Testing & Reliability

6. Test Coverage Gap: The keychain store has comprehensive tests, but I don't see equivalent test coverage for the nango-store implementation, particularly:
   • Token parsing edge cases in getAccessToken()
   • Integration creation race conditions
   • Credential key validation failures

🔶 Documentation & UX

7. Migration Strategy: While the fallback works, there's no clear guidance for users on:
   • When/why they'd need to run pnpm approve-builds for keytar
   • How to verify which credential store is being used
   • How to migrate existing keychain credentials to nango

⚠️ Potential Edge Cases

8. Token Parsing Robustness (nango-store.ts:77-92)

   • The extractAccessTokenForBearerType function attempts JSON parsing but falls back to raw string
   • Could fail silently on malformed token structures

   Recommendation: Add validation/logging for unexpected token formats.

9. Network Error Handling: Nango API calls don't implement retry logic or circuit breaker patterns, which could cause issues in unreliable network conditions.

🏗️ System-Level Considerations

10. Credential Store Selection Logic (defaults.ts:15-25)

    if (process.env.NANGO_SECRET_KEY) {
      stores.push(createNangoCredentialStore(...));
    }
    stores.push(createKeyChainStore('keychain-default'));

    Question: What's the precedence order when both stores are available? The CredentialStoreRegistry behavior isn't clear from this code.

💡 Future Extensibility

11. Auth Mode Support: The nango implementation supports many auth modes (SUPPORTED_AUTH_MODES) but the current OAuth flow seems focused on bearer tokens. Consider documenting which modes are actively tested.

🎯 Recommendations Summary

High Impact, Low Effort:

1. Add error logging to the keychain fallback in oauth.ts
2. Document the credential store precedence behavior
3. Add basic nango-store unit tests

Medium Impact:

1. Standardize environment variable access patterns
2. Add retry logic for Nango API calls
3. Improve token parsing validation

For Future Iterations:

1. Provide migration tooling for keychain → nango
2. Add observability for credential store selection
3. Consider credential key format standardization

Overall Assessment ⭐⭐⭐⭐⭐

This is solid engineering work that solves a real deployment problem. The fallback architecture is well thought out, error handling is generally robust, and the optional dependency approach is exactly right for this use case. The main gaps are in testing coverage and operational observability.

Ready to merge with the suggested improvements addressed in follow-up work.

---