refactor(llm): separate identity and provider resolution#350
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds IBotIdentityProvider and IGroupLlmSettingsService contracts and implementations, refactors LLMFactory to resolve providers via IServiceProvider, updates provider services to use injected bot identity and group settings, simplifies GeneralLLMService, wires services into controller and agent layers, and updates tests for DI-based wiring. ChangesBot Identity and Group Settings Refactoring
Sequence DiagramsequenceDiagram
participant Controller as GeneralLLMController
participant BotProvider as IBotIdentityProvider
participant GroupSettings as IGroupLlmSettingsService
participant Factory as ILLMFactory
participant Provider as OpenAIService
Controller->>BotProvider: GetIdentityAsync()
BotProvider-->>Controller: BotIdentity
Controller->>GroupSettings: GetModelAsync(chatId)
GroupSettings-->>Controller: modelName
Controller->>Factory: GetLLMService(provider)
Factory->>Provider: GetRequiredService<OpenAIService>()
Provider-->>Factory: instance
Factory-->>Controller: ILLMService
Controller->>Provider: ExecAsync(...)
Provider->>BotProvider: GetIdentityAsync()
BotProvider-->>Provider: BotIdentity
Provider-->>Controller: response
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
PR Check ReportSummary
Test Results
Code Quality
Test Artifacts
LinksThis report is auto-generated by GitHub Actions |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
TelegramSearchBot.LLM/Service/AI/LLM/BotIdentityProvider.cs (1)
16-20: 💤 Low valueAsync-over-sync pattern: consider synchronous GetIdentity method.
GetIdentityAsyncusesTask.FromResultinside a lock, which is synchronous work wrapped in an async signature. Since the operation is purely in-memory and doesn't perform I/O, consider either:
- Providing a synchronous
GetIdentity()method alongside the async version- Documenting why the async signature is needed (e.g., for future database-backed implementations)
The current implementation is correct but may cause unnecessary async state machine overhead for callers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TelegramSearchBot.LLM/Service/AI/LLM/BotIdentityProvider.cs` around lines 16 - 20, GetIdentityAsync currently wraps a synchronous, in-memory read (locking _lock and returning Task.FromResult(_identity)), which causes unnecessary async overhead; add a synchronous getter GetIdentity() that acquires the same _lock and returns BotIdentity directly (to be used by callers that don't need async), and either keep GetIdentityAsync as a simple wrapper that calls Task.FromResult(GetIdentity()) for compatibility or add a comment on why the async signature is required for future async implementations; reference the existing GetIdentityAsync method, the _lock field and the _identity field when making the change.TelegramSearchBot.LLM/Service/AI/LLM/OpenAIService.cs (2)
1614-1642: 💤 Low valueCode duplication: fallback logic mirrors GroupLlmSettingsService.
Lines 1620-1630 (SetModel fallback) and lines 1637-1641 (GetModel fallback) duplicate the logic implemented in
GroupLlmSettingsService. While this duplication is intentional for backward compatibility, it creates a maintenance burden where changes to the persistence logic need to be made in two places.Consider:
- Adding a comment documenting that this is fallback-only and should match GroupLlmSettingsService behavior
- In a future iteration, removing the fallback once all deployments have migrated to using the service
This is acceptable as-is for the transitional architecture but should be noted for future cleanup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TelegramSearchBot.LLM/Service/AI/LLM/OpenAIService.cs` around lines 1614 - 1642, The SetModel and GetModel methods contain fallback persistence logic that duplicates GroupLlmSettingsService behavior; add a clear comment inside both SetModel and GetModel indicating these blocks are fallback-only, must mirror GroupLlmSettingsService, and include a TODO to remove once all deployments use GroupLlmSettingsService, so future maintainers know why the duplication exists and to keep behavior in sync with GroupLlmSettingsService's SetModelAsync/GetModelAsync implementations.
108-117: ⚡ Quick winSync-over-async anti-pattern in BotName property getter.
Line 109 uses
GetAwaiter().GetResult()which is a sync-over-async anti-pattern that can cause deadlocks in certain contexts (though less likely in this worker service architecture). This appears necessary for backward compatibility whereBotNameis accessed as a property.Consider:
- Marking this property as
[Obsolete]and migrating callers toGetBotNameAsync()- Documenting that this property should only be used in non-async contexts
- Ensuring no callers use this property from an async context where deadlock could occur
The async methods at lines 1000 and 1183 correctly use
await GetBotNameAsync(), which is the preferred pattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TelegramSearchBot.LLM/Service/AI/LLM/OpenAIService.cs` around lines 108 - 117, The BotName property currently does sync-over-async via GetBotNameAsync().GetAwaiter().GetResult() which risks deadlocks; mark the BotName property as [Obsolete("Use GetBotNameAsync() instead")] and update any callers to call GetBotNameAsync() (await it) instead; if you must keep a synchronous fallback for backward compatibility, change the getter to call GetBotNameAsync().ConfigureAwait(false).GetAwaiter().GetResult() and keep the setter behavior that uses _botIdentityProvider.SetIdentity(Env.BotId, value) or _fallbackBotName to preserve existing semantics.TelegramSearchBot.LLMAgent/LLMAgentProgram.cs (1)
105-106: ⚡ Quick winService lifetime registration is consistent with other services.
Both registrations look appropriate for the LLMAgent's service lifetime needs. The singleton
IBotIdentityProviderensures shared identity state, and scopedIGroupLlmSettingsServicealigns with the scopedDataDbContextregistration (line 100-102).Note:
GroupLlmSettingsServicehas[Injectable(ServiceLifetime.Transient)]but is registered as Scoped here. While this works (manual registration takes precedence), consider updating the attribute toScopedfor consistency, or document why the lifetimes differ between the main app and the agent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TelegramSearchBot.LLMAgent/LLMAgentProgram.cs` around lines 105 - 106, The registration lifetime for GroupLlmSettingsService differs from its Injectable attribute: IGroupLlmSettingsService is registered as Scoped in the DI setup while GroupLlmSettingsService is annotated with [Injectable(ServiceLifetime.Transient)]; reconcile this by either changing the attribute on GroupLlmSettingsService to ServiceLifetime.Scoped, or changing the registration call for IGroupLlmSettingsService to AddTransient, and add a short comment explaining the intentional mismatch if you choose to keep differing lifetimes; reference the symbols IGroupLlmSettingsService, GroupLlmSettingsService, IBotIdentityProvider, BotIdentityProvider, and DataDbContext when making the change.TelegramSearchBot.LLM/Interface/AI/LLM/IGroupLlmSettingsService.cs (1)
6-6: ⚡ Quick winConsider nullable annotation for GetModelAsync return type.
The AI summary indicates that
GetModelAsyncreturnsnullwhen no record exists, but the return typeTask<string>doesn't indicate nullability. With nullable reference types enabled, this should beTask<string?>to accurately reflect the contract and prevent null-reference warnings at call sites.♻️ Proposed fix
- Task<string> GetModelAsync(long chatId, CancellationToken cancellationToken = default); + Task<string?> GetModelAsync(long chatId, CancellationToken cancellationToken = default);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TelegramSearchBot.LLM/Interface/AI/LLM/IGroupLlmSettingsService.cs` at line 6, The GetModelAsync method on IGroupLlmSettingsService currently returns Task<string> but can return null; change its signature to Task<string?> (nullable reference) and update all implementing classes/methods that implement GetModelAsync to return Task<string?> as well, then review and adjust callers to handle a possible null (add null checks, default values, or annotate with null-forgiving only where safe) so the contract matches actual behavior and nullable warnings are resolved.TelegramSearchBot.LLM/Service/AI/LLM/GroupLlmSettingsService.cs (1)
33-37: ⚖️ Poor tradeoffConsider potential race condition on concurrent inserts.
If multiple requests simultaneously call
SetModelAsyncfor a newchatId, both might seesettings == null(line 33) and attempt to insert, causing a database duplicate key exception. While this is a narrow window, consider either:
- Using
ExecuteUpdatefor atomic upsert (EF 7+)- Handling the DbUpdateException for duplicate key
- Documenting that the service should be called from a synchronized context
Given the Transient lifetime and likely single-threaded request handling per chat, this may be acceptable as-is.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TelegramSearchBot.LLM/Service/AI/LLM/GroupLlmSettingsService.cs` around lines 33 - 37, SetModelAsync currently checks if settings is null and then calls _dbContext.GroupSettings.AddAsync(new GroupSettings { GroupId = chatId, LLMModelName = modelName }, ...) which can race on concurrent inserts for the same chatId; modify SetModelAsync to perform an atomic upsert or to handle duplicate-key exceptions: either replace the insert path with an EF Core ExecuteUpdate/Upsert call (if on EF7+) targeting GroupSettings to set LLMModelName for the given GroupId, or wrap the AddAsync/SaveChangesAsync in a try/catch for DbUpdateException and on duplicate-key error re-query the GroupSettings row and update its LLMModelName then SaveChangesAsync; reference the settings variable, GroupSettings entity, _dbContext.GroupSettings.AddAsync, and SetModelAsync when making the change.TelegramSearchBot.LLMAgent/Service/LlmServiceProxy.cs (1)
66-73: 💤 Low valueConsider logging when IBotIdentityProvider is unavailable.
The fallback to
Env.BotId(line 71) ensures backward compatibility whenIBotIdentityProviderisn't registered. However, consider adding a debug or warning log when the fallback is used, to help diagnose configuration issues where the provider is unexpectedly missing.📝 Optional logging addition
private void ApplyBotIdentity(string botName, long botUserId) { var identityProvider = _serviceProvider.GetService<IBotIdentityProvider>(); if (identityProvider != null) { identityProvider.SetIdentity(botUserId, botName); } else { + _logger.LogDebug("IBotIdentityProvider not available, falling back to Env.BotId"); Env.BotId = botUserId; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TelegramSearchBot.LLMAgent/Service/LlmServiceProxy.cs` around lines 66 - 73, The ApplyBotIdentity method silently falls back to Env.BotId when IBotIdentityProvider is missing; add a diagnostic log in that branch: resolve an ILogger<LlmServiceProxy> (or the existing logger instance) from _serviceProvider inside ApplyBotIdentity and emit a warning or debug message indicating IBotIdentityProvider was not registered and the code is using Env.BotId (include botName and botUserId in the message for context). Ensure logging is only done when identityProvider == null and do not change the existing fallback assignment to Env.BotId.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@TelegramSearchBot.LLM/Service/AI/LLM/GroupLlmSettingsService.cs`:
- Line 28: SetModelAsync currently accepts modelName without validation which
can persist invalid settings; add a guard at the start of SetModelAsync (in
GroupLlmSettingsService) that checks if string.IsNullOrWhiteSpace(modelName) and
if so throws an ArgumentException/ArgumentNullException (including parameter
name modelName), or optionally return without persisting; also trim modelName
before use so you never store leading/trailing whitespace and perform this check
before any database calls that use chatId and modelName.
In `@TelegramSearchBot.LLM/Service/AI/LLM/LLMFactory.cs`:
- Line 11: Change the service lifetime on the LLMFactory registration: replace
the [Injectable(ServiceLifetime.Transient)] attribute on the LLMFactory class
with [Injectable(ServiceLifetime.Singleton)] so it follows the Scrutor DI
scanning rule for TelegramSearchBot namespace services; locate the Injectable
attribute applied to the LLMFactory class and update its ServiceLifetime
argument from Transient to Singleton.
In `@TelegramSearchBot/Controller/AI/LLM/GeneralLLMController.cs`:
- Around line 73-78: The code assumes botIdentity from
_botIdentityProvider.GetIdentityAsync() is non-null before checking UserName;
change the guard to first check for null (e.g., if (botIdentity == null ||
string.IsNullOrEmpty(botIdentity.UserName))) so the fallback to call
botClient.GetMe(), _botIdentityProvider.SetIdentity(me.Id, me.Username) and
create new BotIdentity(me.Id, me.Username ?? string.Empty) runs safely when
GetIdentityAsync() returns null or has an empty UserName; update references to
botIdentity accordingly so no dereference occurs before the null check.
- Around line 102-106: The code currently extracts the model name via
Message.Substring(5) without trimming or validating, so update the
Message.StartsWith("设置模型 ") handler to Trim() the substring (e.g. var model =
Message.Substring(5).Trim()), verify model is not null/empty/whitespace, and if
invalid send a user-friendly rejection via SendMessageService.SendMessage and
return without calling _groupLlmSettingsService.SetModelAsync; otherwise pass
the trimmed model into SetModelAsync, and use the trimmed value in
logger.LogInformation and the success SendMessageService.SendMessage call so no
blank model can be persisted.
---
Nitpick comments:
In `@TelegramSearchBot.LLM/Interface/AI/LLM/IGroupLlmSettingsService.cs`:
- Line 6: The GetModelAsync method on IGroupLlmSettingsService currently returns
Task<string> but can return null; change its signature to Task<string?>
(nullable reference) and update all implementing classes/methods that implement
GetModelAsync to return Task<string?> as well, then review and adjust callers to
handle a possible null (add null checks, default values, or annotate with
null-forgiving only where safe) so the contract matches actual behavior and
nullable warnings are resolved.
In `@TelegramSearchBot.LLM/Service/AI/LLM/BotIdentityProvider.cs`:
- Around line 16-20: GetIdentityAsync currently wraps a synchronous, in-memory
read (locking _lock and returning Task.FromResult(_identity)), which causes
unnecessary async overhead; add a synchronous getter GetIdentity() that acquires
the same _lock and returns BotIdentity directly (to be used by callers that
don't need async), and either keep GetIdentityAsync as a simple wrapper that
calls Task.FromResult(GetIdentity()) for compatibility or add a comment on why
the async signature is required for future async implementations; reference the
existing GetIdentityAsync method, the _lock field and the _identity field when
making the change.
In `@TelegramSearchBot.LLM/Service/AI/LLM/GroupLlmSettingsService.cs`:
- Around line 33-37: SetModelAsync currently checks if settings is null and then
calls _dbContext.GroupSettings.AddAsync(new GroupSettings { GroupId = chatId,
LLMModelName = modelName }, ...) which can race on concurrent inserts for the
same chatId; modify SetModelAsync to perform an atomic upsert or to handle
duplicate-key exceptions: either replace the insert path with an EF Core
ExecuteUpdate/Upsert call (if on EF7+) targeting GroupSettings to set
LLMModelName for the given GroupId, or wrap the AddAsync/SaveChangesAsync in a
try/catch for DbUpdateException and on duplicate-key error re-query the
GroupSettings row and update its LLMModelName then SaveChangesAsync; reference
the settings variable, GroupSettings entity, _dbContext.GroupSettings.AddAsync,
and SetModelAsync when making the change.
In `@TelegramSearchBot.LLM/Service/AI/LLM/OpenAIService.cs`:
- Around line 1614-1642: The SetModel and GetModel methods contain fallback
persistence logic that duplicates GroupLlmSettingsService behavior; add a clear
comment inside both SetModel and GetModel indicating these blocks are
fallback-only, must mirror GroupLlmSettingsService, and include a TODO to remove
once all deployments use GroupLlmSettingsService, so future maintainers know why
the duplication exists and to keep behavior in sync with
GroupLlmSettingsService's SetModelAsync/GetModelAsync implementations.
- Around line 108-117: The BotName property currently does sync-over-async via
GetBotNameAsync().GetAwaiter().GetResult() which risks deadlocks; mark the
BotName property as [Obsolete("Use GetBotNameAsync() instead")] and update any
callers to call GetBotNameAsync() (await it) instead; if you must keep a
synchronous fallback for backward compatibility, change the getter to call
GetBotNameAsync().ConfigureAwait(false).GetAwaiter().GetResult() and keep the
setter behavior that uses _botIdentityProvider.SetIdentity(Env.BotId, value) or
_fallbackBotName to preserve existing semantics.
In `@TelegramSearchBot.LLMAgent/LLMAgentProgram.cs`:
- Around line 105-106: The registration lifetime for GroupLlmSettingsService
differs from its Injectable attribute: IGroupLlmSettingsService is registered as
Scoped in the DI setup while GroupLlmSettingsService is annotated with
[Injectable(ServiceLifetime.Transient)]; reconcile this by either changing the
attribute on GroupLlmSettingsService to ServiceLifetime.Scoped, or changing the
registration call for IGroupLlmSettingsService to AddTransient, and add a short
comment explaining the intentional mismatch if you choose to keep differing
lifetimes; reference the symbols IGroupLlmSettingsService,
GroupLlmSettingsService, IBotIdentityProvider, BotIdentityProvider, and
DataDbContext when making the change.
In `@TelegramSearchBot.LLMAgent/Service/LlmServiceProxy.cs`:
- Around line 66-73: The ApplyBotIdentity method silently falls back to
Env.BotId when IBotIdentityProvider is missing; add a diagnostic log in that
branch: resolve an ILogger<LlmServiceProxy> (or the existing logger instance)
from _serviceProvider inside ApplyBotIdentity and emit a warning or debug
message indicating IBotIdentityProvider was not registered and the code is using
Env.BotId (include botName and botUserId in the message for context). Ensure
logging is only done when identityProvider == null and do not change the
existing fallback assignment to Env.BotId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 687c6757-da01-4254-8f44-de06cbba4bdc
📒 Files selected for processing (16)
TelegramSearchBot.LLM.Test/Service/AI/LLM/GeneralLLMServiceTests.csTelegramSearchBot.LLM.Test/Service/AI/LLM/LLMFactoryTests.csTelegramSearchBot.LLM/Interface/AI/LLM/IBotIdentityProvider.csTelegramSearchBot.LLM/Interface/AI/LLM/IGroupLlmSettingsService.csTelegramSearchBot.LLM/Service/AI/LLM/AnthropicService.csTelegramSearchBot.LLM/Service/AI/LLM/BotIdentityProvider.csTelegramSearchBot.LLM/Service/AI/LLM/GeminiService.csTelegramSearchBot.LLM/Service/AI/LLM/GeneralLLMService.csTelegramSearchBot.LLM/Service/AI/LLM/GroupLlmSettingsService.csTelegramSearchBot.LLM/Service/AI/LLM/LLMFactory.csTelegramSearchBot.LLM/Service/AI/LLM/OllamaService.csTelegramSearchBot.LLM/Service/AI/LLM/OpenAIResponsesService.csTelegramSearchBot.LLM/Service/AI/LLM/OpenAIService.csTelegramSearchBot.LLMAgent/LLMAgentProgram.csTelegramSearchBot.LLMAgent/Service/LlmServiceProxy.csTelegramSearchBot/Controller/AI/LLM/GeneralLLMController.cs
💤 Files with no reviewable changes (2)
- TelegramSearchBot.LLM.Test/Service/AI/LLM/GeneralLLMServiceTests.cs
- TelegramSearchBot.LLM/Service/AI/LLM/GeneralLLMService.cs
|
|
||
| namespace TelegramSearchBot.Service.AI.LLM { | ||
| [Injectable(Microsoft.Extensions.DependencyInjection.ServiceLifetime.Singleton)] | ||
| [Injectable(ServiceLifetime.Transient)] |
There was a problem hiding this comment.
Use singleton lifetime for Scrutor-scanned service registration.
Line 11 switches LLMFactory to transient, which conflicts with the project DI rule.
Proposed change
-[Injectable(ServiceLifetime.Transient)]
+[Injectable(ServiceLifetime.Singleton)]As per coding guidelines, "Use Scrutor DI scanning with [Injectable(ServiceLifetime.Singleton)] attribute for service registration in namespaces under TelegramSearchBot".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TelegramSearchBot.LLM/Service/AI/LLM/LLMFactory.cs` at line 11, Change the
service lifetime on the LLMFactory registration: replace the
[Injectable(ServiceLifetime.Transient)] attribute on the LLMFactory class with
[Injectable(ServiceLifetime.Singleton)] so it follows the Scrutor DI scanning
rule for TelegramSearchBot namespace services; locate the Injectable attribute
applied to the LLMFactory class and update its ServiceLifetime argument from
Transient to Singleton.
Comment on lines
+73
to
78
| var botIdentity = await _botIdentityProvider.GetIdentityAsync(); | ||
| if (string.IsNullOrEmpty(botIdentity.UserName)) { | ||
| var me = await botClient.GetMe(); | ||
| Env.BotId = me.Id; | ||
| service.BotName = me.Username; // service.BotName is the username, e.g., "MyBot" | ||
| _botIdentityProvider.SetIdentity(me.Id, me.Username); | ||
| botIdentity = new BotIdentity(me.Id, me.Username ?? string.Empty); | ||
| } |
There was a problem hiding this comment.
Guard against null identity before dereferencing.
Line 74 assumes GetIdentityAsync() never returns null. If it does, this path throws before fallback initialization.
Proposed change
- var botIdentity = await _botIdentityProvider.GetIdentityAsync();
- if (string.IsNullOrEmpty(botIdentity.UserName)) {
+ var botIdentity = await _botIdentityProvider.GetIdentityAsync() ?? new BotIdentity(0, string.Empty);
+ if (string.IsNullOrEmpty(botIdentity.UserName) || botIdentity.UserId == 0) {
var me = await botClient.GetMe();
_botIdentityProvider.SetIdentity(me.Id, me.Username);
botIdentity = new BotIdentity(me.Id, me.Username ?? string.Empty);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var botIdentity = await _botIdentityProvider.GetIdentityAsync(); | |
| if (string.IsNullOrEmpty(botIdentity.UserName)) { | |
| var me = await botClient.GetMe(); | |
| Env.BotId = me.Id; | |
| service.BotName = me.Username; // service.BotName is the username, e.g., "MyBot" | |
| _botIdentityProvider.SetIdentity(me.Id, me.Username); | |
| botIdentity = new BotIdentity(me.Id, me.Username ?? string.Empty); | |
| } | |
| var botIdentity = await _botIdentityProvider.GetIdentityAsync() ?? new BotIdentity(0, string.Empty); | |
| if (string.IsNullOrEmpty(botIdentity.UserName) || botIdentity.UserId == 0) { | |
| var me = await botClient.GetMe(); | |
| _botIdentityProvider.SetIdentity(me.Id, me.Username); | |
| botIdentity = new BotIdentity(me.Id, me.Username ?? string.Empty); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TelegramSearchBot/Controller/AI/LLM/GeneralLLMController.cs` around lines 73
- 78, The code assumes botIdentity from _botIdentityProvider.GetIdentityAsync()
is non-null before checking UserName; change the guard to first check for null
(e.g., if (botIdentity == null || string.IsNullOrEmpty(botIdentity.UserName)))
so the fallback to call botClient.GetMe(),
_botIdentityProvider.SetIdentity(me.Id, me.Username) and create new
BotIdentity(me.Id, me.Username ?? string.Empty) runs safely when
GetIdentityAsync() returns null or has an empty UserName; update references to
botIdentity accordingly so no dereference occurs before the null check.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
TelegramSearchBot.LLM/Service/AI/LLM/GroupLlmSettingsService.cs (1)
25-29: 💤 Low valueConsider using null-conditional operator for conciseness.
Lines 28 and 40 use ternary expressions that could be simplified using the null-conditional operator.
♻️ Suggested simplification
public async Task<string?> GetModelAsync(long chatId, CancellationToken cancellationToken = default) { var settings = await _dbContext.GroupSettings.AsNoTracking() .FirstOrDefaultAsync(s => s.GroupId == chatId, cancellationToken); - return settings == null ? null : settings.LLMModelName; + return settings?.LLMModelName; }- var previous = settings == null ? null : settings.LLMModelName; + var previous = settings?.LLMModelName;Also applies to: 38-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TelegramSearchBot.LLM/Service/AI/LLM/GroupLlmSettingsService.cs` around lines 25 - 29, Replace the ternary null checks with null-conditional access to simplify returns: in GetModelAsync, change "return settings == null ? null : settings.LLMModelName;" to "return settings?.LLMModelName;". Do the same for the other similar method(s) that fetch GroupSettings (e.g., the method that returns SystemMessage) by returning "settings?.SystemMessage" (or the appropriate property) instead of the ternary expression; keep the existing await _dbContext.GroupSettings.AsNoTracking().FirstOrDefaultAsync(...) calls unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@TelegramSearchBot.LLM/Service/AI/LLM/GroupLlmSettingsService.cs`:
- Around line 25-29: Replace the ternary null checks with null-conditional
access to simplify returns: in GetModelAsync, change "return settings == null ?
null : settings.LLMModelName;" to "return settings?.LLMModelName;". Do the same
for the other similar method(s) that fetch GroupSettings (e.g., the method that
returns SystemMessage) by returning "settings?.SystemMessage" (or the
appropriate property) instead of the ternary expression; keep the existing await
_dbContext.GroupSettings.AsNoTracking().FirstOrDefaultAsync(...) calls
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9d95602-2cb7-4299-b0db-df1adcebc631
📒 Files selected for processing (9)
TelegramSearchBot.LLM/Interface/AI/LLM/IGroupLlmSettingsService.csTelegramSearchBot.LLM/Service/AI/LLM/AnthropicService.csTelegramSearchBot.LLM/Service/AI/LLM/GeminiService.csTelegramSearchBot.LLM/Service/AI/LLM/GroupLlmSettingsService.csTelegramSearchBot.LLM/Service/AI/LLM/OllamaService.csTelegramSearchBot.LLM/Service/AI/LLM/OpenAIResponsesService.csTelegramSearchBot.LLM/Service/AI/LLM/OpenAIService.csTelegramSearchBot.LLMAgent/Service/LlmServiceProxy.csTelegramSearchBot/Controller/AI/LLM/GeneralLLMController.cs
🚧 Files skipped from review as they are similar to previous changes (8)
- TelegramSearchBot.LLM/Interface/AI/LLM/IGroupLlmSettingsService.cs
- TelegramSearchBot.LLMAgent/Service/LlmServiceProxy.cs
- TelegramSearchBot.LLM/Service/AI/LLM/GeminiService.cs
- TelegramSearchBot.LLM/Service/AI/LLM/OllamaService.cs
- TelegramSearchBot.LLM/Service/AI/LLM/OpenAIService.cs
- TelegramSearchBot.LLM/Service/AI/LLM/AnthropicService.cs
- TelegramSearchBot.LLM/Service/AI/LLM/OpenAIResponsesService.cs
- TelegramSearchBot/Controller/AI/LLM/GeneralLLMController.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR implements the first architecture slice from #293: removing hidden provider-coupled state from the LLM entry path and making provider resolution per-operation instead of cached inside a singleton factory.
Changes:
IBotIdentityProvider/BotIdentityProviderso bot identity is no longer stored throughOpenAIService.BotNameas shared provider state.IGroupLlmSettingsService/GroupLlmSettingsServiceso group model get/set no longer depends onOpenAIService.GeneralLLMControllerto depend on the identity/settings abstractions instead of injectingOpenAIServiceas a state holder.LLMFactoryto resolve provider services from DI for each call, instead of caching provider instances in the factory.GeneralLLMService.IBotIdentityProvider, while keeping an instance-levelBotNamecompatibility fallback for direct construction/tests.Notes
This intentionally keeps the PR bounded. It does not yet introduce a full
LlmExecutionRequestobject or split all channel/model selection rules out ofGeneralLLMService; those are better as follow-up slices after this state-boundary cleanup lands.Validation
dotnet build TelegramSearchBot.sln --configuration Release --no-restoredotnet test TelegramSearchBot.LLM.Test\TelegramSearchBot.LLM.Test.csproj --configuration Release --no-build— 202 passeddotnet test TelegramSearchBot.Test\TelegramSearchBot.Test.csproj --configuration Release --no-build --filter "FullyQualifiedName~EditLLMConf|FullyQualifiedName~Agent"— 30 passedPart of #293
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests