Conversation
…e bundle support - Add EnableLocalBotAPI, TelegramBotApiId, TelegramBotApiHash, LocalBotApiPort to Config/Env - Auto-set BaseUrl and IsLocalAPI when EnableLocalBotAPI is true - Launch telegram-bot-api.exe as a ChildProcessManager-managed process in GeneralBootstrap - Add conditional Content entry for telegram-bot-api.exe in .csproj - Build telegram-bot-api from source via cmake+vcpkg in GitHub Actions push workflow Co-authored-by: ModerRAS <28183976+ModerRAS@users.noreply.github.com>
- 新增TodoItemResult模型 - 新增TodoToolService实现send_todo_to_group和send_quick_todo两个工具 - 支持结构化待办消息（标题、描述、优先级、截止日期） - 支持快速待办消息 - 使用BuiltInTool和BuiltInParameter属性集成LLM工具调用 Fixes #139
📝 Walkthrough🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected (Beta)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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检查报告📋 检查概览
🧪 测试结果
📊 代码质量
📁 测试产物
🔗 相关链接此报告由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: 5
🧹 Nitpick comments (6)
TelegramSearchBot/AppBootstrap/GeneralBootstrap.cs (1)
99-99: Windows-only implementation.The hardcoded
.exeextension limits this feature to Windows. If cross-platform support is desired in the future, consider detecting the OS and using the appropriate binary name. For now, the graceful fallback (Line 128-130) handles missing binaries appropriately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/AppBootstrap/GeneralBootstrap.cs` at line 99, The code hardcodes a Windows-only binary name into the botApiExePath variable; update GeneralBootstrap.cs so botApiExePath is selected based on the runtime OS (use RuntimeInformation.IsOSPlatform checks) and choose "telegram-bot-api.exe" for Windows and "telegram-bot-api" for Linux/macOS (or platform-appropriate names) before combining with AppContext.BaseDirectory, keeping the existing fallback logic intact..github/workflows/push.yml (2)
36-53: Add error handling to prevent silent failures.PowerShell does not stop on errors by default. If
cmakeor the build fails, the script continues and may produce incomplete artifacts or confusing downstream failures.🛡️ Suggested improvement
- name: Build telegram-bot-api from source if: steps.cache-tg-bot-api.outputs.cache-hit != 'true' shell: pwsh run: | + $ErrorActionPreference = "Stop" git clone --recursive https://github.com/tdlib/telegram-bot-api.git C:\vcpkg\vcpkg.exe install openssl:x64-windows-static zlib:x64-windows-static --no-print-usage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/push.yml around lines 36 - 53, The PowerShell step running the build (commands like git clone, cmake, cmake --build, Copy-Item) can silently continue on failure; make the step fail fast by enabling strict error handling (set $ErrorActionPreference = 'Stop' or equivalent) at the start of the run block and explicitly check/throw on non-zero exit codes (inspect $LASTEXITCODE) after critical commands such as cmake and cmake --build so any build or copy failure causes the job to stop and surface the error.
28-35: Consider a more stable cache key.The current cache key invalidates on any workflow file change, even unrelated ones. This could cause unnecessary rebuilds. Consider using a hash of only the telegram-bot-api build-related lines or a dedicated version file.
♻️ Suggested improvement
uses: actions/cache@v4 with: path: telegram-bot-api-bin - key: tg-bot-api-win-x64-${{ hashFiles('.github/workflows/push.yml') }} + key: tg-bot-api-win-x64-v1 restore-keys: | tg-bot-api-win-x64-Alternatively, create a separate file (e.g.,
.github/tg-bot-api-version.txt) containing a version number and hash that file instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/push.yml around lines 28 - 35, The cache key for the Telegram bot API action (id: cache-tg-bot-api / key: tg-bot-api-win-x64-...) is currently based on the entire workflow file and will invalidate on any workflow change; change the key to hash a dedicated version file or the specific build-related file(s) instead (for example create .github/tg-bot-api-version.txt and replace hashFiles('.github/workflows/push.yml') with hashFiles('.github/tg-bot-api-version.txt') or hash only the build script paths) so the cache only updates when the bot-api build/version actually changes.TelegramSearchBot/Service/Tools/TodoToolService.cs (3)
18-24: Remove unused_botClientfield.The
ITelegramBotClientis injected but never accessed. All Telegram API calls are made through_sendMessage.AddTaskWithResult(...), which handles the bot client internally. Consider removing the unused dependency.Proposed fix
- private readonly ITelegramBotClient _botClient; private readonly SendMessage _sendMessage; - public TodoToolService(ITelegramBotClient botClient, SendMessage sendMessage) { - _botClient = botClient; + public TodoToolService(SendMessage sendMessage) { _sendMessage = sendMessage; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Tools/TodoToolService.cs` around lines 18 - 24, Remove the unused ITelegramBotClient dependency: delete the private readonly ITelegramBotClient _botClient field and remove the ITelegramBotClient botClient parameter from the TodoToolService constructor (leaving SendMessage sendMessage and assignment to _sendMessage). Also update any references to the constructor (e.g., DI registrations) so they no longer supply an ITelegramBotClient, and remove any now-unused using/import related to the Telegram client in TodoToolService.
138-138: Consider using UTC for consistent timestamps.
DateTime.Nowuses the server's local timezone, which may vary across deployments. UsingDateTime.UtcNowensures consistency. If local time is intentional for user display, consider adding timezone information.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Tools/TodoToolService.cs` at line 138, The timestamp uses DateTime.Now in TodoToolService (the sb.AppendLine($"创建时间: {DateTime.Now:yyyy-MM-dd HH:mm}") call), which yields server-local time; change it to DateTime.UtcNow for consistent cross-deployment timestamps or, if local time is intentional for users, explicitly include the timezone/offset (e.g., display with offset or append "UTC" / local timezone info) so the created timestamp is unambiguous.
34-41: Consider validatingtoolContext.ChatIdfor defensive programming.While all current callers properly initialize
ChatId(verified across LLM services and tests),ToolContextuses a default constructor allowingChatIdto remain 0. Adding a simple check would provide defense-in-depth against future callers.Optional enhancement
try { + if (toolContext.ChatId == 0) { + return new TodoItemResult { + Success = false, + ChatId = 0, + Error = "Invalid chat context." + }; + } + if (string.IsNullOrWhiteSpace(title)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Tools/TodoToolService.cs` around lines 34 - 41, Add a defensive check for toolContext.ChatId in the TodoToolService method that creates todo items (the block starting with the try and title validation) by verifying toolContext.ChatId > 0 before proceeding; if invalid, return a TodoItemResult with Success = false, ChatId = toolContext.ChatId, and a clear Error like "ChatId is required" so callers get a consistent failure shape. Ensure this check runs before the title validation and uses the same return pattern/type as the existing title check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TelegramSearchBot.Common/Env.cs`:
- Around line 86-89: Add documentation in the Docs/ folder describing the new
Env class configuration properties EnableLocalBotAPI, TelegramBotApiId,
TelegramBotApiHash, and LocalBotApiPort; explain what each property does, show
example configuration entries, and include step-by-step instructions for
obtaining the API ID and API Hash from https://my.telegram.org and where to
place them (e.g., env file or config file consumed by the Env class). Also note
the default LocalBotApiPort (8081) and how to enable the local bot API by
setting EnableLocalBotAPI to true. Ensure the docs follow existing Docs/ style
and link back to the Env class members (EnableLocalBotAPI, TelegramBotApiId,
TelegramBotApiHash, LocalBotApiPort).
In `@TelegramSearchBot.Common/Model/Tools/TodoItemResult.cs`:
- Line 6: The Error property on the TodoItemResult class is non-nullable but
only populated when Success is false; change its declaration to a nullable
string (Error?) to reflect optionality and silence CS8618. Locate the
TodoItemResult class and update the public string Error { get; set; } to public
string? Error { get; set; }, and run the build/analysis to ensure no other
callers assume Error is always non-null (adjust any dereferences or assertions
accordingly).
In `@TelegramSearchBot/AppBootstrap/GeneralBootstrap.cs`:
- Around line 108-126: The ProcessStartInfo currently sets
RedirectStandardOutput and RedirectStandardError to true which can block if the
child process writes enough output; either set those flags to false (if you
don’t need the botApi output) or, preferably, keep them true but consume the
streams asynchronously by wiring OutputDataReceived and ErrorDataReceived
handlers and calling BeginOutputReadLine/BeginErrorReadLine on the Process
returned from Process.Start (referencing ProcessStartInfo, botApiExePath,
RedirectStandardOutput, RedirectStandardError, and the botApiProcess variable),
then continue adding the process to childProcessManager and awaiting
WaitForLocalBotApiReady as before.
In `@TelegramSearchBot/Service/Tools/TodoToolService.cs`:
- Line 84: The formattedMessage string in TodoToolService is interpolating raw
user input into HTML and must be HTML-escaped to prevent injection; update the
code that builds formattedMessage to HTML-encode the message parameter (e.g.,
use System.Net.WebUtility.HtmlEncode(message) or HttpUtility.HtmlEncode) and use
the encoded value in the interpolation (handle null/empty safely, e.g.,
string.IsNullOrEmpty fallback) so the variable formattedMessage uses the encoded
text instead of raw message.
- Around line 111-141: The FormatTodoMessage method interpolates user-provided
strings (title, description, dueDate, priority) directly into HTML; escape these
inputs using System.Net.WebUtility.HtmlEncode (or HttpUtility.HtmlEncode) before
any interpolation to prevent broken formatting or HTML injection. Update
FormatTodoMessage to call HtmlEncode on title, description, dueDate and priority
(use the encoded priority when rendering text, but keep the priorityIcon logic
unchanged), then build the StringBuilder output with the encoded values.
---
Nitpick comments:
In @.github/workflows/push.yml:
- Around line 36-53: The PowerShell step running the build (commands like git
clone, cmake, cmake --build, Copy-Item) can silently continue on failure; make
the step fail fast by enabling strict error handling (set $ErrorActionPreference
= 'Stop' or equivalent) at the start of the run block and explicitly check/throw
on non-zero exit codes (inspect $LASTEXITCODE) after critical commands such as
cmake and cmake --build so any build or copy failure causes the job to stop and
surface the error.
- Around line 28-35: The cache key for the Telegram bot API action (id:
cache-tg-bot-api / key: tg-bot-api-win-x64-...) is currently based on the entire
workflow file and will invalidate on any workflow change; change the key to hash
a dedicated version file or the specific build-related file(s) instead (for
example create .github/tg-bot-api-version.txt and replace
hashFiles('.github/workflows/push.yml') with
hashFiles('.github/tg-bot-api-version.txt') or hash only the build script paths)
so the cache only updates when the bot-api build/version actually changes.
In `@TelegramSearchBot/AppBootstrap/GeneralBootstrap.cs`:
- Line 99: The code hardcodes a Windows-only binary name into the botApiExePath
variable; update GeneralBootstrap.cs so botApiExePath is selected based on the
runtime OS (use RuntimeInformation.IsOSPlatform checks) and choose
"telegram-bot-api.exe" for Windows and "telegram-bot-api" for Linux/macOS (or
platform-appropriate names) before combining with AppContext.BaseDirectory,
keeping the existing fallback logic intact.
In `@TelegramSearchBot/Service/Tools/TodoToolService.cs`:
- Around line 18-24: Remove the unused ITelegramBotClient dependency: delete the
private readonly ITelegramBotClient _botClient field and remove the
ITelegramBotClient botClient parameter from the TodoToolService constructor
(leaving SendMessage sendMessage and assignment to _sendMessage). Also update
any references to the constructor (e.g., DI registrations) so they no longer
supply an ITelegramBotClient, and remove any now-unused using/import related to
the Telegram client in TodoToolService.
- Line 138: The timestamp uses DateTime.Now in TodoToolService (the
sb.AppendLine($"创建时间: {DateTime.Now:yyyy-MM-dd HH:mm}") call), which yields
server-local time; change it to DateTime.UtcNow for consistent cross-deployment
timestamps or, if local time is intentional for users, explicitly include the
timezone/offset (e.g., display with offset or append "UTC" / local timezone
info) so the created timestamp is unambiguous.
- Around line 34-41: Add a defensive check for toolContext.ChatId in the
TodoToolService method that creates todo items (the block starting with the try
and title validation) by verifying toolContext.ChatId > 0 before proceeding; if
invalid, return a TodoItemResult with Success = false, ChatId =
toolContext.ChatId, and a clear Error like "ChatId is required" so callers get a
consistent failure shape. Ensure this check runs before the title validation and
uses the same return pattern/type as the existing title check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2514ee2-c1dc-4258-af97-6291a8f47bf0
📒 Files selected for processing (6)
.github/workflows/push.ymlTelegramSearchBot.Common/Env.csTelegramSearchBot.Common/Model/Tools/TodoItemResult.csTelegramSearchBot/AppBootstrap/GeneralBootstrap.csTelegramSearchBot/Service/Tools/TodoToolService.csTelegramSearchBot/TelegramSearchBot.csproj
Comment on lines
+86
to
+89
| public bool EnableLocalBotAPI { get; set; } = false; | ||
| public string TelegramBotApiId { get; set; } | ||
| public string TelegramBotApiHash { get; set; } | ||
| public int LocalBotApiPort { get; set; } = 8081; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if documentation mentions the new configuration options
echo "=== Searching for documentation updates ==="
rg -l "EnableLocalBotAPI|TelegramBotApiId|LocalBotApiPort" Docs/ || echo "No documentation found for new config options in Docs/"
echo ""
echo "=== Checking README for local bot API mentions ==="
rg -i "local.*bot.*api|telegram-bot-api" README.md || echo "No local bot API documentation found in README"Repository: ModerRAS/TelegramSearchBot
Length of output: 267
Update Docs/ with the new Local Bot API configuration options.
Documentation for the new configuration options (EnableLocalBotAPI, TelegramBotApiId, TelegramBotApiHash, LocalBotApiPort) is missing. Add guidance to Docs/ on how users can obtain their API credentials from https://my.telegram.org and configure these settings. This is required per the coding guidelines for persistent configuration changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TelegramSearchBot.Common/Env.cs` around lines 86 - 89, Add documentation in
the Docs/ folder describing the new Env class configuration properties
EnableLocalBotAPI, TelegramBotApiId, TelegramBotApiHash, and LocalBotApiPort;
explain what each property does, show example configuration entries, and include
step-by-step instructions for obtaining the API ID and API Hash from
https://my.telegram.org and where to place them (e.g., env file or config file
consumed by the Env class). Also note the default LocalBotApiPort (8081) and how
to enable the local bot API by setting EnableLocalBotAPI to true. Ensure the
docs follow existing Docs/ style and link back to the Env class members
(EnableLocalBotAPI, TelegramBotApiId, TelegramBotApiHash, LocalBotApiPort).
| public bool Success { get; set; } | ||
| public int? MessageId { get; set; } | ||
| public long ChatId { get; set; } | ||
| public string Error { get; set; } |
There was a problem hiding this comment.
Make Error property nullable to reflect its optional nature.
The static analysis warning is valid. Error is only set when Success = false, so it should be declared as nullable to avoid CS8618 and correctly express intent.
Proposed fix
- public string Error { get; set; }
+ public string? Error { get; set; }📝 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
| public string Error { get; set; } | |
| public string? Error { get; set; } |
🧰 Tools
🪛 GitHub Check: build (ubuntu-latest)
[warning] 6-6:
Non-nullable property 'Error' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TelegramSearchBot.Common/Model/Tools/TodoItemResult.cs` at line 6, The Error
property on the TodoItemResult class is non-nullable but only populated when
Success is false; change its declaration to a nullable string (Error?) to
reflect optionality and silence CS8618. Locate the TodoItemResult class and
update the public string Error { get; set; } to public string? Error { get; set;
}, and run the build/analysis to ensure no other callers assume Error is always
non-null (adjust any dereferences or assertions accordingly).
Comment on lines
+108
to
+126
| var startInfo = new ProcessStartInfo { | ||
| FileName = botApiExePath, | ||
| UseShellExecute = false, | ||
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true | ||
| }; | ||
| startInfo.ArgumentList.Add("--local"); | ||
| startInfo.ArgumentList.Add($"--api-id={Env.TelegramBotApiId}"); | ||
| startInfo.ArgumentList.Add($"--api-hash={Env.TelegramBotApiHash}"); | ||
| startInfo.ArgumentList.Add($"--dir={botApiDataDir}"); | ||
| startInfo.ArgumentList.Add($"--http-port={Env.LocalBotApiPort}"); | ||
| var botApiProcess = Process.Start(startInfo); | ||
| if (botApiProcess == null) { | ||
| Log.Warning("telegram-bot-api 进程启动失败"); | ||
| } else { | ||
| childProcessManager.AddProcess(botApiProcess); | ||
| Log.Information("telegram-bot-api 已启动，等待端口 {Port} 就绪...", Env.LocalBotApiPort); | ||
| await WaitForLocalBotApiReady(Env.LocalBotApiPort); | ||
| } |
There was a problem hiding this comment.
Redirected output streams may cause process blocking.
When RedirectStandardOutput and RedirectStandardError are true, the output is buffered. If telegram-bot-api produces enough output without it being consumed, the process can block waiting for the buffer to be read. This is a known issue with long-running processes.
🐛 Option 1: Disable redirection if output isn't needed
var startInfo = new ProcessStartInfo {
FileName = botApiExePath,
UseShellExecute = false,
- RedirectStandardOutput = true,
- RedirectStandardError = true
+ RedirectStandardOutput = false,
+ RedirectStandardError = false
};🐛 Option 2: Consume output asynchronously
var botApiProcess = Process.Start(startInfo);
if (botApiProcess == null) {
Log.Warning("telegram-bot-api 进程启动失败");
} else {
+ // Consume output to prevent buffer blocking
+ botApiProcess.OutputDataReceived += (s, e) => { if (e.Data != null) Log.Debug("[bot-api] {Line}", e.Data); };
+ botApiProcess.ErrorDataReceived += (s, e) => { if (e.Data != null) Log.Warning("[bot-api] {Line}", e.Data); };
+ botApiProcess.BeginOutputReadLine();
+ botApiProcess.BeginErrorReadLine();
childProcessManager.AddProcess(botApiProcess);📝 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 startInfo = new ProcessStartInfo { | |
| FileName = botApiExePath, | |
| UseShellExecute = false, | |
| RedirectStandardOutput = true, | |
| RedirectStandardError = true | |
| }; | |
| startInfo.ArgumentList.Add("--local"); | |
| startInfo.ArgumentList.Add($"--api-id={Env.TelegramBotApiId}"); | |
| startInfo.ArgumentList.Add($"--api-hash={Env.TelegramBotApiHash}"); | |
| startInfo.ArgumentList.Add($"--dir={botApiDataDir}"); | |
| startInfo.ArgumentList.Add($"--http-port={Env.LocalBotApiPort}"); | |
| var botApiProcess = Process.Start(startInfo); | |
| if (botApiProcess == null) { | |
| Log.Warning("telegram-bot-api 进程启动失败"); | |
| } else { | |
| childProcessManager.AddProcess(botApiProcess); | |
| Log.Information("telegram-bot-api 已启动，等待端口 {Port} 就绪...", Env.LocalBotApiPort); | |
| await WaitForLocalBotApiReady(Env.LocalBotApiPort); | |
| } | |
| var startInfo = new ProcessStartInfo { | |
| FileName = botApiExePath, | |
| UseShellExecute = false, | |
| RedirectStandardOutput = true, | |
| RedirectStandardError = true | |
| }; | |
| startInfo.ArgumentList.Add("--local"); | |
| startInfo.ArgumentList.Add($"--api-id={Env.TelegramBotApiId}"); | |
| startInfo.ArgumentList.Add($"--api-hash={Env.TelegramBotApiHash}"); | |
| startInfo.ArgumentList.Add($"--dir={botApiDataDir}"); | |
| startInfo.ArgumentList.Add($"--http-port={Env.LocalBotApiPort}"); | |
| var botApiProcess = Process.Start(startInfo); | |
| if (botApiProcess == null) { | |
| Log.Warning("telegram-bot-api 进程启动失败"); | |
| } else { | |
| // Consume output to prevent buffer blocking | |
| botApiProcess.OutputDataReceived += (s, e) => { if (e.Data != null) Log.Debug("[bot-api] {Line}", e.Data); }; | |
| botApiProcess.ErrorDataReceived += (s, e) => { if (e.Data != null) Log.Warning("[bot-api] {Line}", e.Data); }; | |
| botApiProcess.BeginOutputReadLine(); | |
| botApiProcess.BeginErrorReadLine(); | |
| childProcessManager.AddProcess(botApiProcess); | |
| Log.Information("telegram-bot-api 已启动，等待端口 {Port} 就绪...", Env.LocalBotApiPort); | |
| await WaitForLocalBotApiReady(Env.LocalBotApiPort); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TelegramSearchBot/AppBootstrap/GeneralBootstrap.cs` around lines 108 - 126,
The ProcessStartInfo currently sets RedirectStandardOutput and
RedirectStandardError to true which can block if the child process writes enough
output; either set those flags to false (if you don’t need the botApi output)
or, preferably, keep them true but consume the streams asynchronously by wiring
OutputDataReceived and ErrorDataReceived handlers and calling
BeginOutputReadLine/BeginErrorReadLine on the Process returned from
Process.Start (referencing ProcessStartInfo, botApiExePath,
RedirectStandardOutput, RedirectStandardError, and the botApiProcess variable),
then continue adding the process to childProcessManager and awaiting
WaitForLocalBotApiReady as before.
| }; | ||
| } | ||
|
|
||
| var formattedMessage = $"📋 <b>待办</b>\n\n{message}"; |
There was a problem hiding this comment.
Escape HTML in user message content.
Same HTML injection concern as above—the message parameter should be HTML-encoded.
Proposed fix
- var formattedMessage = $"📋 <b>待办</b>\n\n{message}";
+ var formattedMessage = $"📋 <b>待办</b>\n\n{System.Net.WebUtility.HtmlEncode(message)}";📝 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 formattedMessage = $"📋 <b>待办</b>\n\n{message}"; | |
| var formattedMessage = $"📋 <b>待办</b>\n\n{System.Net.WebUtility.HtmlEncode(message)}"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TelegramSearchBot/Service/Tools/TodoToolService.cs` at line 84, The
formattedMessage string in TodoToolService is interpolating raw user input into
HTML and must be HTML-escaped to prevent injection; update the code that builds
formattedMessage to HTML-encode the message parameter (e.g., use
System.Net.WebUtility.HtmlEncode(message) or HttpUtility.HtmlEncode) and use the
encoded value in the interpolation (handle null/empty safely, e.g.,
string.IsNullOrEmpty fallback) so the variable formattedMessage uses the encoded
text instead of raw message.
Comment on lines
+111
to
+141
| private string FormatTodoMessage(string title, string description, string priority, string dueDate) { | ||
| var sb = new StringBuilder(); | ||
| sb.AppendLine("📋 <b>待办事项</b>"); | ||
| sb.AppendLine(); | ||
| sb.AppendLine($"📌 <b>标题:</b> {title}"); | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(description)) { | ||
| sb.AppendLine($"📝 <b>描述:</b> {description}"); | ||
| } | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(dueDate)) { | ||
| sb.AppendLine($"⏰ <b>截止日期:</b> {dueDate}"); | ||
| } | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(priority)) { | ||
| var priorityIcon = priority.ToLower() switch { | ||
| "low" => "🟢", | ||
| "medium" => "🟡", | ||
| "high" => "🟠", | ||
| "urgent" => "🔴", | ||
| _ => "⚪" | ||
| }; | ||
| sb.AppendLine($"🔥 <b>优先级:</b> {priorityIcon} {priority}"); | ||
| } | ||
|
|
||
| sb.AppendLine(); | ||
| sb.AppendLine("━━━━━━━━━━━━━━━━━━"); | ||
| sb.AppendLine($"创建时间: {DateTime.Now:yyyy-MM-dd HH:mm}"); | ||
|
|
||
| return sb.ToString(); | ||
| } |
There was a problem hiding this comment.
Escape HTML special characters in user-provided content.
User-provided strings (title, description, dueDate, priority) are interpolated directly into HTML without escaping. If input contains <, >, or &, it could break formatting or allow unintended HTML injection.
Use System.Net.WebUtility.HtmlEncode() or System.Web.HttpUtility.HtmlEncode() to sanitize user input before interpolation.
Proposed fix
+using System.Net;
+
private string FormatTodoMessage(string title, string description, string priority, string dueDate) {
var sb = new StringBuilder();
sb.AppendLine("📋 <b>待办事项</b>");
sb.AppendLine();
- sb.AppendLine($"📌 <b>标题:</b> {title}");
+ sb.AppendLine($"📌 <b>标题:</b> {WebUtility.HtmlEncode(title)}");
if (!string.IsNullOrWhiteSpace(description)) {
- sb.AppendLine($"📝 <b>描述:</b> {description}");
+ sb.AppendLine($"📝 <b>描述:</b> {WebUtility.HtmlEncode(description)}");
}
if (!string.IsNullOrWhiteSpace(dueDate)) {
- sb.AppendLine($"⏰ <b>截止日期:</b> {dueDate}");
+ sb.AppendLine($"⏰ <b>截止日期:</b> {WebUtility.HtmlEncode(dueDate)}");
}
if (!string.IsNullOrWhiteSpace(priority)) {
var priorityIcon = priority.ToLower() switch {
"low" => "🟢",
"medium" => "🟡",
"high" => "🟠",
"urgent" => "🔴",
_ => "⚪"
};
- sb.AppendLine($"🔥 <b>优先级:</b> {priorityIcon} {priority}");
+ sb.AppendLine($"🔥 <b>优先级:</b> {priorityIcon} {WebUtility.HtmlEncode(priority)}");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TelegramSearchBot/Service/Tools/TodoToolService.cs` around lines 111 - 141,
The FormatTodoMessage method interpolates user-provided strings (title,
description, dueDate, priority) directly into HTML; escape these inputs using
System.Net.WebUtility.HtmlEncode (or HttpUtility.HtmlEncode) before any
interpolation to prevent broken formatting or HTML injection. Update
FormatTodoMessage to call HtmlEncode on title, description, dueDate and priority
(use the encoded priority when rendering text, but keep the priorityIcon logic
unchanged), then build the StringBuilder output with the encoded values.
Fix whitespace formatting issues detected by CI on Windows
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
TelegramSearchBot.LLM/Service/AI/LLM/McpToolHelper.cs (1)
140-140:⚠️ Potential issue | 🟠 MajorInconsistent
paramIsRequiredlogic between methods that describe and validate tools.Line 140 in
RegisterToolsAndGetPromptString(XML generation) and line 678 inExecuteRegisteredToolAsync(runtime validation) both use complex logic that excludes non-nullable value types from the required check:&& !( param.ParameterType.IsValueType && Nullable.GetUnderlyingType(param.ParameterType) == null ). This forces non-nullable value types (e.g.,int,bool) to always be marked not required, even when they have no default value.In contrast, line 173 in
GetNativeToolDefinitions(JSON schema generation) uses simpler logic:!param.IsOptional && !param.HasDefaultValue, which would correctly mark non-nullable value types as required.This inconsistency is problematic because:
- Non-nullable value types cannot accept null, so they should be required when no default exists
- Tool descriptions (XML), runtime validation, and schema definitions now disagree on which parameters are required
- This could cause issues where a parameter is described as optional but validation requires it (or vice versa)
Align the logic across all three locations, or document why non-nullable value types should be treated as implicitly optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot.LLM/Service/AI/LLM/McpToolHelper.cs` at line 140, RegisterToolsAndGetPromptString, ExecuteRegisteredToolAsync and GetNativeToolDefinitions use inconsistent logic to determine paramIsRequired (one excludes non-nullable value types via "param.ParameterType.IsValueType && Nullable.GetUnderlyingType(...) == null"); align them by treating parameters as required when they have no IsOptional and no HasDefaultValue (i.e., use "!param.IsOptional && !param.HasDefaultValue") so non-nullable value types without defaults are marked required everywhere; update the paramIsRequired computation in RegisterToolsAndGetPromptString and ExecuteRegisteredToolAsync to match GetNativeToolDefinitions (or vice versa if you prefer that semantics) and ensure any XML/JSON schema or runtime validation code that reads paramIsRequired is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TelegramSearchBot.LLM/Service/AI/LLM/McpToolHelper.cs`:
- Line 173: The paramIsRequired computation at var paramIsRequired =
builtInParamAttr?.IsRequired ?? mcpParamAttr?.IsRequired ?? ( !param.IsOptional
&& !param.HasDefaultValue ); is inconsistent with the same logic used at other
sites (line 140 and 678); extract and use a single helper like
IsParameterRequired(ParameterInfo param, bool? attrIsRequired) and replace the
inline expressions in McpToolHelper.cs (references: paramIsRequired,
builtInParamAttr, mcpParamAttr, ParameterInfo) so all locations call the helper
to determine required-ness, passing the appropriate attribute value precedence
(built-in then MCP then fallback to !param.IsOptional &&
!param.HasDefaultValue).
---
Outside diff comments:
In `@TelegramSearchBot.LLM/Service/AI/LLM/McpToolHelper.cs`:
- Line 140: RegisterToolsAndGetPromptString, ExecuteRegisteredToolAsync and
GetNativeToolDefinitions use inconsistent logic to determine paramIsRequired
(one excludes non-nullable value types via "param.ParameterType.IsValueType &&
Nullable.GetUnderlyingType(...) == null"); align them by treating parameters as
required when they have no IsOptional and no HasDefaultValue (i.e., use
"!param.IsOptional && !param.HasDefaultValue") so non-nullable value types
without defaults are marked required everywhere; update the paramIsRequired
computation in RegisterToolsAndGetPromptString and ExecuteRegisteredToolAsync to
match GetNativeToolDefinitions (or vice versa if you prefer that semantics) and
ensure any XML/JSON schema or runtime validation code that reads paramIsRequired
is consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1c7cbf9-3106-4d98-ab67-a0896c36f618
📒 Files selected for processing (19)
TelegramSearchBot.Common/Attributes/McpAttributes.csTelegramSearchBot.Common/Model/Tools/IterationLimitReachedPayload.csTelegramSearchBot.Database/Migrations/20260303031828_AddUserWithGroupUniqueIndex.csTelegramSearchBot.Database/Migrations/20260313124507_AddChannelWithModelIsDeleted.csTelegramSearchBot.LLM.Test/Service/AI/LLM/GeneralLLMServiceTests.csTelegramSearchBot.LLM.Test/Service/AI/LLM/ModelCapabilityServiceTests.csTelegramSearchBot.LLM.Test/Service/AI/LLM/OpenAIProviderHistorySerializationTests.csTelegramSearchBot.LLM/Service/AI/LLM/GeminiService.csTelegramSearchBot.LLM/Service/AI/LLM/McpToolHelper.csTelegramSearchBot.LLM/Service/AI/LLM/OllamaService.csTelegramSearchBot.LLM/Service/AI/LLM/OpenAIService.csTelegramSearchBot.LLM/Service/Mcp/McpClient.csTelegramSearchBot.LLM/Service/Mcp/McpServerManager.csTelegramSearchBot.LLM/Service/Tools/FileToolService.csTelegramSearchBot.Test/Helper/WordCloudHelperTests.csTelegramSearchBot/Extension/ServiceCollectionExtension.csTelegramSearchBot/Service/BotAPI/SendMessageService.Streaming.csTelegramSearchBot/Service/Storage/MessageService.csTelegramSearchBot/Service/Vector/FaissVectorService.cs
✅ Files skipped from review due to trivial changes (18)
- TelegramSearchBot.Test/Helper/WordCloudHelperTests.cs
- TelegramSearchBot.LLM/Service/Mcp/McpClient.cs
- TelegramSearchBot.LLM/Service/AI/LLM/OllamaService.cs
- TelegramSearchBot/Service/Vector/FaissVectorService.cs
- TelegramSearchBot.LLM/Service/AI/LLM/GeminiService.cs
- TelegramSearchBot.LLM/Service/Tools/FileToolService.cs
- TelegramSearchBot.LLM.Test/Service/AI/LLM/OpenAIProviderHistorySerializationTests.cs
- TelegramSearchBot.Database/Migrations/20260313124507_AddChannelWithModelIsDeleted.cs
- TelegramSearchBot.LLM.Test/Service/AI/LLM/ModelCapabilityServiceTests.cs
- TelegramSearchBot.LLM.Test/Service/AI/LLM/GeneralLLMServiceTests.cs
- TelegramSearchBot.LLM/Service/Mcp/McpServerManager.cs
- TelegramSearchBot.Database/Migrations/20260303031828_AddUserWithGroupUniqueIndex.cs
- TelegramSearchBot.Common/Model/Tools/IterationLimitReachedPayload.cs
- TelegramSearchBot.LLM/Service/AI/LLM/OpenAIService.cs
- TelegramSearchBot.Common/Attributes/McpAttributes.cs
- TelegramSearchBot/Extension/ServiceCollectionExtension.cs
- TelegramSearchBot/Service/Storage/MessageService.cs
- TelegramSearchBot/Service/BotAPI/SendMessageService.Streaming.cs
| var mcpParamAttr = param.GetCustomAttribute<McpParameterAttribute>(); | ||
| var paramDescription = builtInParamAttr?.Description ?? mcpParamAttr?.Description ?? $"Parameter '{param.Name}'"; | ||
| var paramIsRequired = builtInParamAttr?.IsRequired ?? mcpParamAttr?.IsRequired ?? (!param.IsOptional && !param.HasDefaultValue); | ||
| var paramIsRequired = builtInParamAttr?.IsRequired ?? mcpParamAttr?.IsRequired ?? ( !param.IsOptional && !param.HasDefaultValue ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inconsistent paramIsRequired logic with line 140.
This line uses the simpler !param.IsOptional && !param.HasDefaultValue logic, which is correct but inconsistent with line 140 and line 678. The same parameter could be marked as required in native tool definitions but optional in XML prompts.
Consider unifying the logic across all three locations to ensure consistent behavior.
♻️ Consider extracting a shared helper method
private static bool IsParameterRequired(ParameterInfo param, bool? attrIsRequired)
{
return attrIsRequired ?? (!param.IsOptional && !param.HasDefaultValue);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TelegramSearchBot.LLM/Service/AI/LLM/McpToolHelper.cs` at line 173, The
paramIsRequired computation at var paramIsRequired =
builtInParamAttr?.IsRequired ?? mcpParamAttr?.IsRequired ?? ( !param.IsOptional
&& !param.HasDefaultValue ); is inconsistent with the same logic used at other
sites (line 140 and 678); extract and use a single helper like
IsParameterRequired(ParameterInfo param, bool? attrIsRequired) and replace the
inline expressions in McpToolHelper.cs (references: paramIsRequired,
builtInParamAttr, mcpParamAttr, ParameterInfo) so all locations call the helper
to determine required-ness, passing the appropriate attribute value precedence
(built-in then MCP then fallback to !param.IsOptional &&
!param.HasDefaultValue).
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
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.
实现Issue #139 - 添加LLM工具用于发送待办事项到群聊
新增功能
TodoToolService
优先级支持
消息格式
文件变更
Fixes #139
Summary by CodeRabbit
New Features
Chores