fix oauth set credential id

[omar-inkeep]

should use generateIdFromName

[vercel]

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

Project	Deployment	Preview	Comments	Updated (UTC)
agents-docs	Ready	Preview	Comment	Oct 2, 2025 8:45pm
agents-manage-api	Ready	Preview	Comment	Oct 2, 2025 8:45pm
agents-manage-ui	Ready	Preview	Comment	Oct 2, 2025 8:45pm
agents-run-api	Ready	Preview	Comment	Oct 2, 2025 8:45pm

[claude]

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

---

PR Review: Fix OAuth Set Credential ID

Todo List:

• Read the modified OAuth route file to understand the changes
• Research generateIdFromName function usage and purpose in codebase
• Analyze OAuth-related patterns for consistency
• Review security implications of credential ID generation
• Check for potential testing impact
• Provide comprehensive review feedback

---

Review Summary

This PR correctly addresses an inconsistency where credential IDs were using raw tool names instead of the standardized generateIdFromName function. This is a critical fix for data integrity and system consistency.

✅ Positive Changes

1. Data Integrity & Consistency

• Fixes inconsistency where credential IDs weren't being normalized properly
• Aligns with established patterns across the codebase (used in 8+ locations)
• Both keychain and Nango credential creation paths now use the same ID generation logic

2. Security Improvement

• Prevents potential issues with invalid characters in tool names being used as IDs
• The generateIdFromName function properly sanitizes input by:
  • Converting to lowercase
  • Replacing invalid characters with hyphens
  • Removing leading/trailing hyphens
  • Replacing consecutive hyphens with single hyphens
  • Validating against resource ID schema constraints

3. Database Constraint Compliance

• Ensures generated IDs comply with the primary key constraint on the credential_references table
• Follows URL-safe character restrictions (a-z, 0-9, _, -)
• Prevents database errors from malformed IDs

🚨 Critical Concerns

1. Potential Breaking Change for Existing Data

• High Impact: If any existing credentials were created with raw tool names containing spaces, special characters, or uppercase letters, this change could break the lookup mechanism in findOrCreateCredential (line 40-43)
• Example scenario: Tool named "My OAuth Tool" would have previously created credential with ID "My OAuth Tool", but now would generate ID "my-oauth-tool", causing the lookup to fail and potentially creating duplicate credentials
• Recommendation: Consider adding a migration to update existing credential IDs, or implement backward compatibility in the lookup logic

2. Missing Database Migration
According to the CLAUDE.md guidelines, schema-related changes require:

pnpm db:generate  # Generate migration from schema changes
pnpm db:migrate   # Apply migration to database

However, this appears to be a code change rather than schema change, but existing data compatibility should still be considered.

🔍 Implementation Analysis

Locations Changed (agents-manage-api/src/routes/oauth.ts):

• Line 281: id: generateIdFromName(mcpTool.name) (keychain storage)
• Line 297: id: generateIdFromName(mcpTool.name) (Nango storage)

Function Implementation (packages/agents-core/src/validation/id-validation.ts:27-49):

export function generateIdFromName(name: string): string {
  const id = name
    .toLowerCase()
    .replace(/[^a-z0-9_-]+/g, '-') // Replace invalid chars with hyphens
    .replace(/^-+|-+$/g, '') // Remove leading/trailing hyphens
    .replace(/-{2,}/g, '-'); // Replace multiple consecutive hyphens with single hyphen
  // ... validation and truncation logic
}

📊 Risk Assessment & Recommendations

Immediate Actions Required:

1. Data Impact Assessment: Query existing credential_references table to identify records where id contains characters that would be normalized by generateIdFromName
2. Backward Compatibility: Consider modifying findOrCreateCredential to try both normalized and original IDs during lookup
3. Testing: Add integration tests covering the OAuth flow with various tool name formats

Suggested Migration Strategy:

// In findOrCreateCredential, try both IDs:
try {
  const existingCredential = await getCredentialReferenceWithTools(dbClient)({
    scopes: { tenantId, projectId },
    id: generateIdFromName(originalToolName), // New normalized ID
  });
  if (existingCredential) return existingCredential;
  
  // Fallback to original ID for backward compatibility
  return await getCredentialReferenceWithTools(dbClient)({
    scopes: { tenantId, projectId },
    id: originalToolName,
  });
} catch {
  // Continue with creation...
}

🧪 Testing Gaps

Missing Test Coverage:

• OAuth flow with tool names containing special characters, spaces, uppercase letters
• Credential lookup behavior with existing vs. new ID format
• Edge cases like empty tool names or names with only special characters

Recommended Test Cases:

describe('OAuth credential ID generation', () => {
  it('should handle tool names with spaces and special characters', async () => {
    // Test with "My OAuth Tool!" -> "my-oauth-tool"
  });
  
  it('should maintain backward compatibility with existing credentials', async () => {
    // Test lookup of credentials created before this change
  });
});

🎯 Final Recommendation

Approve with Required Changes: This fix is necessary and follows established patterns, but needs additional safeguards for existing data.

Priority Actions:

1. Add backward compatibility in credential lookup
2. Assess impact on existing credentials
3. Add comprehensive test coverage
4. Consider gradual rollout or feature flag

The core change is correct and improves system consistency, but the implementation needs to account for existing data to prevent service disruption.