-
Notifications
You must be signed in to change notification settings - Fork 5
Refactor provider detection logic AST #52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…roach with enhanced evaluation methods and update schemas for improved clarity and maintainability
…proach, update schemas for clarity, and remove deprecated alias handling
…oved clarity and consistency
…eprecated doh-providers module
…arity and consistency
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request refactors the provider detection logic from a simple rule-based system to a more sophisticated AST (Abstract Syntax Tree) approach with composable logic operators. The change enables more complex detection rules while maintaining type safety and JSON serializability.
- Replaced individual
DetectionRuletypes with a unifiedLogicAST supporting AND/OR/NOT composition - Consolidated provider definitions to use a single
Providertype with categories instead of separate types per provider kind - Moved DoH (DNS over HTTPS) provider definitions from a separate file into the main DNS service file
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/services/registration.ts | Updated to use new detectRegistrar function and handle both name and domain updates |
| server/services/hosting.ts | Removed IP address field, reorganized email provider detection timing, and cleaned up formatting |
| server/services/hosting.test.ts | Removed test assertion for deprecated ipAddress field |
| server/services/doh-providers.ts | Deleted file - DoH provider definitions moved to dns.ts |
| server/services/dns.ts | Consolidated DoH provider definitions from separate file |
| lib/schemas/tls.ts | Removed CertificateAuthorityProvider schema in favor of unified provider system |
| lib/schemas/registration.ts | Removed RegistrarProvider schema in favor of unified provider system |
| lib/schemas/provider.ts | Complete rewrite to define Logic AST types and unified Provider interface |
| lib/schemas/hosting.ts | Removed HostingProvider schema and ipAddress field |
| lib/providers/detection.ts | Refactored detection logic to use new AST evaluation with evalRule function |
| lib/providers/detection.test.ts | Updated test to use new detectRegistrar function |
| lib/providers/catalog.ts | Converted all provider definitions to use new AST rule format with categories |
| lib/providers/README.md | Updated documentation to reflect new AST-based detection system |
| components/domain/sections/hosting-email-section.test.tsx | Removed test assertion for deprecated ipAddress field |
| components/domain/domain-report-view.test.tsx | Removed test assertion for deprecated ipAddress field |
| const ttl = record.isRegistered ? 24 * 60 * 60 : 60 * 60; | ||
| const registrarName = (record.registrar?.name || "").toString(); | ||
| const registrarUrl = (record.registrar?.url || "").toString(); | ||
| let registrarName = (record.registrar?.name || "").toString(); |
Copilot
AI
Sep 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The variable declaration has changed from const to let but the initial assignment logic remains the same. Consider keeping it as const registrarName and use a separate variable for the potentially updated name to maintain immutability where possible.
| const ctx: DetectionContext = { | ||
| headers: headersObj, | ||
| mx: (mxHosts ?? []).map((h) => h.toLowerCase().replace(/\.$/, "")), | ||
| ns: (nsHosts ?? []).map((h) => h.toLowerCase().replace(/\.$/, "")), |
Copilot
AI
Sep 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The regex /\.$$/ could be extracted as a constant for better readability and reusability, especially since FQDN trailing dot removal is a common DNS operation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
==========================================
+ Coverage 64.73% 66.52% +1.79%
==========================================
Files 94 93 -1
Lines 4942 5201 +259
Branches 410 405 -5
==========================================
+ Hits 3199 3460 +261
+ Misses 1727 1723 -4
- Partials 16 18 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.