feat: add send_video_file and send_document_file MCP tools#253
feat: add send_video_file and send_document_file MCP tools#253
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SendDocument and SendVideo result models, interfaces, and service implementations; updates reply-id handling to cast to int in photo sending and LLM iteration controller; numerous whitespace/formatting tweaks across migrations, tests, and helper files. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Service as SendDocumentToolService
participant SendMsg as _sendMessage
participant BotClient as ITelegramBotClient
Client->>Service: SendDocumentFile(filePath, toolContext, caption, replyToMessageId)
Service->>Service: Validate path, existence, size (env-dependent)
alt validation fails
Service-->>Client: SendDocumentResult(Success=false, Error)
else
Service->>Service: Read file into memory, build InputFile
Service->>Service: Resolve ReplyParameters (explicit or from ToolContext)
Service->>SendMsg: AddTaskWithResult(SendDocument(...), CancellationToken)
SendMsg->>BotClient: SendDocument(InputFile, caption, replyParameters)
BotClient-->>SendMsg: Message
SendMsg-->>Service: Message result
Service-->>Client: SendDocumentResult(Success=true, MessageId, ChatId)
end
sequenceDiagram
participant Client
participant Service as SendVideoToolService
participant SendMsg as _sendMessage
participant BotClient as ITelegramBotClient
Client->>Service: SendVideoFile(filePath, toolContext, caption, replyToMessageId)
Service->>Service: Validate path, existence, size (env-dependent)
alt validation fails
Service-->>Client: SendVideoResult(Success=false, Error)
else
Service->>Service: Read file into memory, build InputFile
Service->>Service: Resolve ReplyParameters (explicit or from ToolContext)
Service->>SendMsg: AddTaskWithResult(SendVideo(...), CancellationToken)
SendMsg->>BotClient: SendVideo(InputFile, caption, replyParameters)
BotClient-->>SendMsg: Message
SendMsg-->>Service: Message result
Service-->>Client: SendVideoResult(Success=true, MessageId, ChatId)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
🔍 PR检查报告📋 检查概览
🧪 测试结果
📊 代码质量
📁 测试产物
🔗 相关链接此报告由GitHub Actions自动生成 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/Model/Tools/SendDocumentResult.cs`:
- Line 6: The Error property on SendDocumentResult should be nullable because it
only exists for failures; change the property declaration of
SendDocumentResult.Error from string to string? and update any callers
(creators/readers of SendDocumentResult) to handle potential null (e.g., checks
for null before using Error or use null-coalescing where appropriate). Ensure
any serialization attributes or DTO mappings still allow null values for Error.
In `@TelegramSearchBot.Common/Model/Tools/SendVideoResult.cs`:
- Line 6: The Error property on the SendVideoResult model should be nullable to
reflect that successful results leave it unset; update the declaration of Error
in class SendVideoResult to a nullable string (e.g., string? Error { get; set;
}) and adjust any callers or consumers if they assume non-null (notably
SendVideoToolService.SendVideoFile where success paths do not set Error), so
nullable-reference warnings are eliminated and the contract accurately
represents optional error data.
In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs`:
- Around line 41-58: The SendDocument tool currently accepts any file path;
before calling File.Exists or FileInfo in the SendDocument method in
SendDocumentToolService (parameters: string filePath, ToolContext toolContext,
string caption = null, int? replyToMessageId = null), canonicalize the incoming
path with Path.GetFullPath, compare it against a configured safe root (e.g.
SafeUploadDirectory or AllowedUploadRoot) and reject the request if the
canonicalized path is not under that root (use a path-prefix check that accounts
for trailing separators). Return a SendDocumentResult with Success = false,
ChatId = toolContext.ChatId and an appropriate Error message when the path is
outside the allowlist, and only proceed to File.Exists/FileInfo and sending when
the path validation passes.
- Around line 62-85: The code currently buffers the entire file via
ReadAllBytesAsync and MemoryStream before queuing the rate-limited task, which
can OOM for large files; instead, in SendDocumentToolService move file opening
into the delegate passed to _sendMessage.AddTaskWithResult so streaming occurs
when the send actually runs: replace ReadAllBytesAsync/MemoryStream usage with
File.OpenRead(filePath) (or new FileStream(filePath, FileMode.Open,
FileAccess.Read, FileShare.Read, bufferSize: 81920, useAsync: true)) and create
the InputFile.FromStream from that FileStream inside the AddTaskWithResult async
lambda that calls _botClient.SendDocument, ensure the FileStream is disposed
after SendDocument completes (e.g., via using) and keep the existing size check,
caption truncation, replyParameters, CancellationTokenSource and SendTimeout
logic unchanged.
In `@TelegramSearchBot/Service/Tools/SendVideoToolService.cs`:
- Around line 41-58: The SendVideoToolService method currently checks
File.Exists(filePath) directly which allows arbitrary file access; before
touching the filesystem, resolve and normalize filePath with Path.GetFullPath
and a configured allowlist root (e.g., safeRoot from settings), then verify the
resolved path starts with the normalized safeRoot (case-insensitive on Windows)
and reject with a SendVideoResult (Success=false, ChatId=toolContext.ChatId,
Error="Path outside allowed media directory.") if it does not; only after that
perform File.Exists and proceed as before. Ensure you reference filePath,
toolContext.ChatId, SendVideoResult and File.Exists in the change and do not
call File.Exists until the allowlist check passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8b7bbd3-7fd7-4494-ac35-4c0e1bf9f8ab
📒 Files selected for processing (7)
TelegramSearchBot.Common/Model/Tools/SendDocumentResult.csTelegramSearchBot.Common/Model/Tools/SendVideoResult.csTelegramSearchBot/Interface/Tools/ISendDocumentToolService.csTelegramSearchBot/Interface/Tools/ISendVideoToolService.csTelegramSearchBot/Service/Tools/SendDocumentToolService.csTelegramSearchBot/Service/Tools/SendPhotoToolService.csTelegramSearchBot/Service/Tools/SendVideoToolService.cs
| 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.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*SendDocumentResult.cs" -o -name "*SendVideoResult.cs" | head -20Repository: ModerRAS/TelegramSearchBot
Length of output: 188
🏁 Script executed:
cat -n "./TelegramSearchBot.Common/Model/Tools/SendDocumentResult.cs"Repository: ModerRAS/TelegramSearchBot
Length of output: 382
🏁 Script executed:
cat -n "./TelegramSearchBot.Common/Model/Tools/SendVideoResult.cs"Repository: ModerRAS/TelegramSearchBot
Length of output: 379
🏁 Script executed:
find . -type f -name "*.cs" | xargs grep -l "SendDocumentResult" | head -10Repository: ModerRAS/TelegramSearchBot
Length of output: 255
🏁 Script executed:
find . -type f -name "*.cs" | xargs grep -l "SendVideoResult" | head -10Repository: ModerRAS/TelegramSearchBot
Length of output: 246
🏁 Script executed:
cat -n "./TelegramSearchBot/Service/Tools/SendDocumentToolService.cs"Repository: ModerRAS/TelegramSearchBot
Length of output: 5422
🏁 Script executed:
cat -n "./TelegramSearchBot/Service/Tools/SendVideoToolService.cs"Repository: ModerRAS/TelegramSearchBot
Length of output: 5375
Make Error nullable.
Error is only populated on failures, so keeping it non-nullable gives consumers the wrong contract and produces nullable noise.
🩹 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.
| public string Error { get; set; } | |
| public string? Error { get; set; } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TelegramSearchBot.Common/Model/Tools/SendDocumentResult.cs` at line 6, The
Error property on SendDocumentResult should be nullable because it only exists
for failures; change the property declaration of SendDocumentResult.Error from
string to string? and update any callers (creators/readers of
SendDocumentResult) to handle potential null (e.g., checks for null before using
Error or use null-coalescing where appropriate). Ensure any serialization
attributes or DTO mappings still allow null values for Error.
| 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.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the SendVideoResult.cs file
find . -name "SendVideoResult.cs" -type fRepository: ModerRAS/TelegramSearchBot
Length of output: 127
🏁 Script executed:
# Let's also search for SendVideoToolService to understand the usage context
find . -name "*SendVideoToolService*" -type fRepository: ModerRAS/TelegramSearchBot
Length of output: 188
🏁 Script executed:
# Search for how Error property is set/used
rg "SendVideoResult" -t cs -A 3 -B 3Repository: ModerRAS/TelegramSearchBot
Length of output: 5610
🏁 Script executed:
# Let's examine the complete SendVideoResult.cs file
cat -n ./TelegramSearchBot.Common/Model/Tools/SendVideoResult.csRepository: ModerRAS/TelegramSearchBot
Length of output: 379
🏁 Script executed:
# Let's also check the complete SendVideoToolService.SendVideoFile method to see all success/error paths
cat -n ./TelegramSearchBot/Service/Tools/SendVideoToolService.cs | sed -n '1,100p'Repository: ModerRAS/TelegramSearchBot
Length of output: 5366
🏁 Script executed:
# Check for any port-related code in SendVideoResult.cs (coding guideline check)
rg "Port|port|:|\d{4,5}" ./TelegramSearchBot.Common/Model/Tools/SendVideoResult.csRepository: ModerRAS/TelegramSearchBot
Length of output: 52
Make Error nullable.
Successful results don't set this property (lines 87-91 in SendVideoToolService.SendVideoFile), so the current contract is misleading and triggers nullable reference type warnings.
🩹 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.
| 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/SendVideoResult.cs` at line 6, The Error
property on the SendVideoResult model should be nullable to reflect that
successful results leave it unset; update the declaration of Error in class
SendVideoResult to a nullable string (e.g., string? Error { get; set; }) and
adjust any callers or consumers if they assume non-null (notably
SendVideoToolService.SendVideoFile where success paths do not set Error), so
nullable-reference warnings are eliminated and the contract accurately
represents optional error data.
| [BuiltInParameter("The file path to the document on the server.")] string filePath, | ||
| ToolContext toolContext, | ||
| [BuiltInParameter("Optional caption for the document (max 1024 characters).", IsRequired = false)] string caption = null, | ||
| [BuiltInParameter("Optional message ID to reply to.", IsRequired = false)] int? replyToMessageId = null) { | ||
| try { | ||
| if (string.IsNullOrWhiteSpace(filePath)) { | ||
| return new SendDocumentResult { | ||
| Success = false, | ||
| ChatId = toolContext.ChatId, | ||
| Error = "File path cannot be empty." | ||
| }; | ||
| } | ||
|
|
||
| if (!File.Exists(filePath)) { | ||
| return new SendDocumentResult { | ||
| Success = false, | ||
| ChatId = toolContext.ChatId, | ||
| Error = $"File not found: {filePath}" |
There was a problem hiding this comment.
Restrict filePath to an allowlisted upload directory.
As written, this tool can read and send any server file the process can access. That is an arbitrary file-read primitive, not just a document send tool. Canonicalize the path and reject anything outside a configured safe root before File.Exists or FileInfo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs` around lines 41 -
58, The SendDocument tool currently accepts any file path; before calling
File.Exists or FileInfo in the SendDocument method in SendDocumentToolService
(parameters: string filePath, ToolContext toolContext, string caption = null,
int? replyToMessageId = null), canonicalize the incoming path with
Path.GetFullPath, compare it against a configured safe root (e.g.
SafeUploadDirectory or AllowedUploadRoot) and reject the request if the
canonicalized path is not under that root (use a path-prefix check that accounts
for trailing separators). Return a SendDocumentResult with Success = false,
ChatId = toolContext.ChatId and an appropriate Error message when the path is
outside the allowlist, and only proceed to File.Exists/FileInfo and sending when
the path validation passes.
| var fileInfo = new FileInfo(filePath); | ||
| const long maxFileSizeBytes = 2L * 1024 * 1024 * 1024; | ||
| if (fileInfo.Length > maxFileSizeBytes) { | ||
| return new SendDocumentResult { | ||
| Success = false, | ||
| ChatId = toolContext.ChatId, | ||
| Error = $"File is too large ({fileInfo.Length / 1024 / 1024 / 1024}GB). Maximum allowed size is 2GB." | ||
| }; | ||
| } | ||
|
|
||
| var fileBytes = await File.ReadAllBytesAsync(filePath); | ||
| var document = InputFile.FromStream(new MemoryStream(fileBytes), fileInfo.Name); | ||
|
|
||
| var replyParameters = GetReplyParameters(replyToMessageId, toolContext); | ||
|
|
||
| using var cts = new CancellationTokenSource(SendTimeout); | ||
| var message = await _sendMessage.AddTaskWithResult(async () => await _botClient.SendDocument( | ||
| chatId: toolContext.ChatId, | ||
| document: document, | ||
| caption: string.IsNullOrEmpty(caption) ? null : caption.Length > 1024 ? caption.Substring(0, 1024) : caption, | ||
| parseMode: ParseMode.Html, | ||
| replyParameters: replyParameters, | ||
| cancellationToken: cts.Token | ||
| ), toolContext.ChatId); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the file
find . -name "SendDocumentToolService.cs" -type fRepository: ModerRAS/TelegramSearchBot
Length of output: 130
🏁 Script executed:
# Get the file size to determine read strategy
wc -l TelegramSearchBot/Service/Tools/SendDocumentToolService.cs 2>/dev/null || echo "File not found, searching more broadly"Repository: ModerRAS/TelegramSearchBot
Length of output: 132
🏁 Script executed:
# Search for SendDocumentToolService in the repository
rg "SendDocumentToolService" -lRepository: ModerRAS/TelegramSearchBot
Length of output: 190
🏁 Script executed:
cat -n TelegramSearchBot/Service/Tools/SendDocumentToolService.cs | sed -n '50,95p'Repository: ModerRAS/TelegramSearchBot
Length of output: 2511
🏁 Script executed:
# Also check the full file to understand the context and imports
cat -n TelegramSearchBot/Service/Tools/SendDocumentToolService.csRepository: ModerRAS/TelegramSearchBot
Length of output: 5422
🏁 Script executed:
# Find SendMessage class and AddTaskWithResult method
rg "class SendMessage" -A 20 | head -40Repository: ModerRAS/TelegramSearchBot
Length of output: 4463
🏁 Script executed:
# Also search for AddTaskWithResult definition
rg "AddTaskWithResult" -B 2 -A 10Repository: ModerRAS/TelegramSearchBot
Length of output: 13901
🏁 Script executed:
# Find and examine the SendMessage manager to understand the enqueue behavior
cat -n TelegramSearchBot/Manager/SendMessage.cs | head -60Repository: ModerRAS/TelegramSearchBot
Length of output: 2994
🏁 Script executed:
# Check if there are any other implementations using FileStream with InputFile
rg "InputFile.FromStream\|FileStream" -B 2 -A 2Repository: ModerRAS/TelegramSearchBot
Length of output: 52
🏁 Script executed:
# Search more broadly for InputFile usage
rg "InputFile\." -B 1 -A 1 | head -40Repository: ModerRAS/TelegramSearchBot
Length of output: 4377
🏁 Script executed:
# Check SendMessage.cs for the full AddTaskWithResult method
cat -n TelegramSearchBot/Manager/SendMessage.cs | tail -30Repository: ModerRAS/TelegramSearchBot
Length of output: 1293
Stream the document; this upload path can't support 2GB files in memory.
ReadAllBytesAsync allocates the entire file into a MemoryStream before the send task is queued, causing immediate memory pressure. For a 2GB file, this consumes 2GB of RAM upfront and will cause OOM on most systems. Instead, open a FileStream inside the AddTaskWithResult delegate so streaming occurs only when the rate-limited task executes, with the file read in chunks rather than all at once.
🩹 Proposed fix
- var fileBytes = await File.ReadAllBytesAsync(filePath);
- var document = InputFile.FromStream(new MemoryStream(fileBytes), fileInfo.Name);
-
var replyParameters = GetReplyParameters(replyToMessageId, toolContext);
using var cts = new CancellationTokenSource(SendTimeout);
- var message = await _sendMessage.AddTaskWithResult(async () => await _botClient.SendDocument(
- chatId: toolContext.ChatId,
- document: document,
- caption: string.IsNullOrEmpty(caption) ? null : caption.Length > 1024 ? caption.Substring(0, 1024) : caption,
- parseMode: ParseMode.Html,
- replyParameters: replyParameters,
- cancellationToken: cts.Token
- ), toolContext.ChatId);
+ var message = await _sendMessage.AddTaskWithResult(async () => {
+ await using var stream = new FileStream(
+ filePath,
+ FileMode.Open,
+ FileAccess.Read,
+ FileShare.Read,
+ bufferSize: 81920,
+ useAsync: true);
+ var document = InputFile.FromStream(stream, fileInfo.Name);
+
+ return await _botClient.SendDocument(
+ chatId: toolContext.ChatId,
+ document: document,
+ caption: string.IsNullOrEmpty(caption) ? null : caption.Length > 1024 ? caption.Substring(0, 1024) : caption,
+ parseMode: ParseMode.Html,
+ replyParameters: replyParameters,
+ cancellationToken: cts.Token
+ );
+ }, toolContext.ChatId);📝 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.
| var fileInfo = new FileInfo(filePath); | |
| const long maxFileSizeBytes = 2L * 1024 * 1024 * 1024; | |
| if (fileInfo.Length > maxFileSizeBytes) { | |
| return new SendDocumentResult { | |
| Success = false, | |
| ChatId = toolContext.ChatId, | |
| Error = $"File is too large ({fileInfo.Length / 1024 / 1024 / 1024}GB). Maximum allowed size is 2GB." | |
| }; | |
| } | |
| var fileBytes = await File.ReadAllBytesAsync(filePath); | |
| var document = InputFile.FromStream(new MemoryStream(fileBytes), fileInfo.Name); | |
| var replyParameters = GetReplyParameters(replyToMessageId, toolContext); | |
| using var cts = new CancellationTokenSource(SendTimeout); | |
| var message = await _sendMessage.AddTaskWithResult(async () => await _botClient.SendDocument( | |
| chatId: toolContext.ChatId, | |
| document: document, | |
| caption: string.IsNullOrEmpty(caption) ? null : caption.Length > 1024 ? caption.Substring(0, 1024) : caption, | |
| parseMode: ParseMode.Html, | |
| replyParameters: replyParameters, | |
| cancellationToken: cts.Token | |
| ), toolContext.ChatId); | |
| var fileInfo = new FileInfo(filePath); | |
| const long maxFileSizeBytes = 2L * 1024 * 1024 * 1024; | |
| if (fileInfo.Length > maxFileSizeBytes) { | |
| return new SendDocumentResult { | |
| Success = false, | |
| ChatId = toolContext.ChatId, | |
| Error = $"File is too large ({fileInfo.Length / 1024 / 1024 / 1024}GB). Maximum allowed size is 2GB." | |
| }; | |
| } | |
| var replyParameters = GetReplyParameters(replyToMessageId, toolContext); | |
| using var cts = new CancellationTokenSource(SendTimeout); | |
| var message = await _sendMessage.AddTaskWithResult(async () => { | |
| await using var stream = new FileStream( | |
| filePath, | |
| FileMode.Open, | |
| FileAccess.Read, | |
| FileShare.Read, | |
| bufferSize: 81920, | |
| useAsync: true); | |
| var document = InputFile.FromStream(stream, fileInfo.Name); | |
| return await _botClient.SendDocument( | |
| chatId: toolContext.ChatId, | |
| document: document, | |
| caption: string.IsNullOrEmpty(caption) ? null : caption.Length > 1024 ? caption.Substring(0, 1024) : caption, | |
| parseMode: ParseMode.Html, | |
| replyParameters: replyParameters, | |
| cancellationToken: cts.Token | |
| ); | |
| }, toolContext.ChatId); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs` around lines 62 -
85, The code currently buffers the entire file via ReadAllBytesAsync and
MemoryStream before queuing the rate-limited task, which can OOM for large
files; instead, in SendDocumentToolService move file opening into the delegate
passed to _sendMessage.AddTaskWithResult so streaming occurs when the send
actually runs: replace ReadAllBytesAsync/MemoryStream usage with
File.OpenRead(filePath) (or new FileStream(filePath, FileMode.Open,
FileAccess.Read, FileShare.Read, bufferSize: 81920, useAsync: true)) and create
the InputFile.FromStream from that FileStream inside the AddTaskWithResult async
lambda that calls _botClient.SendDocument, ensure the FileStream is disposed
after SendDocument completes (e.g., via using) and keep the existing size check,
caption truncation, replyParameters, CancellationTokenSource and SendTimeout
logic unchanged.
| [BuiltInParameter("The file path to the video on the server.")] string filePath, | ||
| ToolContext toolContext, | ||
| [BuiltInParameter("Optional caption for the video (max 1024 characters).", IsRequired = false)] string caption = null, | ||
| [BuiltInParameter("Optional message ID to reply to.", IsRequired = false)] int? replyToMessageId = null) { | ||
| try { | ||
| if (string.IsNullOrWhiteSpace(filePath)) { | ||
| return new SendVideoResult { | ||
| Success = false, | ||
| ChatId = toolContext.ChatId, | ||
| Error = "File path cannot be empty." | ||
| }; | ||
| } | ||
|
|
||
| if (!File.Exists(filePath)) { | ||
| return new SendVideoResult { | ||
| Success = false, | ||
| ChatId = toolContext.ChatId, | ||
| Error = $"File not found: {filePath}" |
There was a problem hiding this comment.
Restrict filePath to an allowlisted media directory.
This tool currently lets the caller probe and send any file the bot process can read. That turns prompt injection or a malicious caller into arbitrary file exfiltration. Normalize against a configured safe root and reject paths that escape it before touching the filesystem.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TelegramSearchBot/Service/Tools/SendVideoToolService.cs` around lines 41 -
58, The SendVideoToolService method currently checks File.Exists(filePath)
directly which allows arbitrary file access; before touching the filesystem,
resolve and normalize filePath with Path.GetFullPath and a configured allowlist
root (e.g., safeRoot from settings), then verify the resolved path starts with
the normalized safeRoot (case-insensitive on Windows) and reject with a
SendVideoResult (Success=false, ChatId=toolContext.ChatId, Error="Path outside
allowed media directory.") if it does not; only after that perform File.Exists
and proceed as before. Ensure you reference filePath, toolContext.ChatId,
SendVideoResult and File.Exists in the change and do not call File.Exists until
the allowlist check passes.
74b5499 to
8902f28
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
TelegramSearchBot.Common/Model/Tools/SendVideoResult.cs (1)
6-6:⚠️ Potential issue | 🟡 MinorMake
Errornullable to match the actual result contract.Line 6 currently declares
Erroras non-nullable, but successful results may not set it, which triggers CS8618 and misstates optional error semantics.Proposed fix
- public string Error { get; set; } + public string? Error { get; set; }#!/bin/bash set -euo pipefail # Verify model declaration fd 'SendVideoResult\.cs$' -x sed -n '1,80p' {} # Verify SendVideoResult construction sites and whether success paths omit Error rg -n -C3 --type cs 'new\s+SendVideoResult\s*\{' rg -n -C2 --type cs '\bError\s*='🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot.Common/Model/Tools/SendVideoResult.cs` at line 6, Property Error on SendVideoResult is declared non-nullable but can be unset for successful results; change its type to nullable (string?) on the SendVideoResult class and update any constructors or initialization sites of SendVideoResult (e.g., object initializers and any factory methods) so they either set Error explicitly or rely on the nullable property; ensure callers check for null instead of assuming a value to eliminate CS8618 warnings and reflect the optional error semantics.TelegramSearchBot/Service/Tools/SendDocumentToolService.cs (2)
46-60:⚠️ Potential issue | 🔴 CriticalRestrict
filePathto an allowlisted root before any file-system checks.Line 54 currently allows probing any readable server path. Canonicalize first (
Path.GetFullPath) and reject paths outside a configured safe directory.🔒 Suggested fix
+ private readonly string _safeUploadRoot; + - public SendDocumentToolService(ITelegramBotClient botClient, SendMessage sendMessage) { + public SendDocumentToolService(ITelegramBotClient botClient, SendMessage sendMessage /*, IConfiguration config */) { _botClient = botClient; _sendMessage = sendMessage; + _safeUploadRoot = Path.GetFullPath(/* config["SafeUploadRoot"] */ "/app/uploads"); } ... - if (!File.Exists(filePath)) { + var fullPath = Path.GetFullPath(filePath); + var safeRootWithSep = _safeUploadRoot.TrimEnd(Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar; + if (!fullPath.StartsWith(safeRootWithSep, StringComparison.Ordinal)) { + return new SendDocumentResult { + Success = false, + ChatId = toolContext.ChatId, + Error = "File path is outside allowed directory." + }; + } + + if (!File.Exists(fullPath)) { return new SendDocumentResult { Success = false, ChatId = toolContext.ChatId, - Error = $"File not found: {filePath}" + Error = "File not found." }; } - var fileInfo = new FileInfo(filePath); + var fileInfo = new FileInfo(fullPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs` around lines 46 - 60, The current checks in SendDocumentToolService that validate filePath (returning SendDocumentResult with toolContext.ChatId) must first canonicalize and restrict the path to an allowlisted root: call Path.GetFullPath(filePath) into a canonicalPath, obtain the configured safe root (e.g. a service-level _allowedRoot or configuration value), then ensure canonicalPath.StartsWith(Path.GetFullPath(_allowedRoot), StringComparison.OrdinalIgnoreCase) (or use a robust path-contains helper) and return a failed SendDocumentResult if it’s outside that root before performing File.Exists; update the code around the filePath checks to use canonicalPath and the configured allowed root check so arbitrary server paths cannot be probed.
72-85:⚠️ Potential issue | 🔴 CriticalStream the file inside the queued send task; don’t buffer up to 2GB in RAM.
Line 72 reads the entire file before enqueueing. With a 2GB limit, this can OOM and defeat rate-limited queueing behavior.
♻️ Suggested fix
- var fileBytes = await File.ReadAllBytesAsync(filePath); - var document = InputFile.FromStream(new MemoryStream(fileBytes), fileInfo.Name); - var replyParameters = GetReplyParameters(replyToMessageId, toolContext); using var cts = new CancellationTokenSource(SendTimeout); - var message = await _sendMessage.AddTaskWithResult(async () => await _botClient.SendDocument( - chatId: toolContext.ChatId, - document: document, - caption: string.IsNullOrEmpty(caption) ? null : caption.Length > 1024 ? caption.Substring(0, 1024) : caption, - parseMode: ParseMode.Html, - replyParameters: replyParameters, - cancellationToken: cts.Token - ), toolContext.ChatId); + var message = await _sendMessage.AddTaskWithResult(async () => { + await using var stream = new FileStream( + filePath, + FileMode.Open, + FileAccess.Read, + FileShare.Read, + bufferSize: 81920, + useAsync: true + ); + var document = InputFile.FromStream(stream, fileInfo.Name); + return await _botClient.SendDocument( + chatId: toolContext.ChatId, + document: document, + caption: string.IsNullOrEmpty(caption) ? null : caption.Length > 1024 ? caption.Substring(0, 1024) : caption, + parseMode: ParseMode.Html, + replyParameters: replyParameters, + cancellationToken: cts.Token + ); + }, toolContext.ChatId);#!/bin/bash # Verify this service does not pre-buffer document bytes anymore. rg -nP --type=cs 'ReadAllBytesAsync|new\s+MemoryStream\(' TelegramSearchBot/Service/Tools/SendDocumentToolService.cs rg -nP --type=cs 'AddTaskWithResult\s*\(' -A25 -B5 TelegramSearchBot/Service/Tools/SendDocumentToolService.cs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs` around lines 72 - 85, The current code reads the entire file into memory (File.ReadAllBytesAsync + new MemoryStream) before enqueuing the send via _sendMessage.AddTaskWithResult, which can OOM for large files; change to open a FileStream (File.OpenRead) and create the InputFile.FromStream inside the async lambda passed to _sendMessage.AddTaskWithResult so the file is streamed during SendDocument (TelegramBotClient.SendDocument) rather than buffered up-front; ensure the FileStream is disposed after SendDocument completes (use a using or try/finally inside the task), keep the same caption/truncation, parseMode, replyParameters, cancellation token (cts.Token) and target chat (toolContext.ChatId), and remove the ReadAllBytesAsync/new MemoryStream usage.
🤖 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/Controller/AI/LLM/LLMIterationCallbackController.cs`:
- Line 157: Extract snapshot.OriginalMessageId into a local variable (e.g.,
originalMessageIdLong), validate it fits into a 32-bit int (ensure value >=
int.MinValue and <= int.MaxValue), then assign to an int (e.g.,
originalMessageIdInt) and use that int for the API calls instead of casting
inline; apply this change to both places that currently use
(int)snapshot.OriginalMessageId in LLMIterationCallbackController so you avoid
duplicated casts and handle out-of-range values (throw or return a handled
error) before calling the API.
In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs`:
- Around line 54-59: The code in SendDocumentToolService is returning full
filesystem paths and likely raw exception text to users (see the File.Exists
branch and the similar block at lines 92-97); change these return Error messages
to a generic, non-sensitive message like "File not available" or "Unable to
retrieve document" and move detailed info (full path and exception.ToString())
into an internal log call using the service's logger (e.g., ILogger in
SendDocumentToolService or an existing logging instance) so the user-facing
SendDocumentResult contains no server-internal paths or exception details while
the logs retain the full diagnostics.
- Around line 35-37: The code casts a nullable long (messageId) to int without
range checking, risking truncation; update the ReplyParameters creation in
SendDocumentToolService (and mirror the same fix in SendVideoToolService and
SendPhotoToolService) to validate messageId.Value is within
int.MinValue..int.MaxValue before casting; if messageId is out of range, handle
it safely (e.g. log/warn and return null or otherwise avoid casting) and only
then construct new ReplyParameters { MessageId = (int)messageId.Value } so the
cast is guarded and cannot silently truncate.
---
Duplicate comments:
In `@TelegramSearchBot.Common/Model/Tools/SendVideoResult.cs`:
- Line 6: Property Error on SendVideoResult is declared non-nullable but can be
unset for successful results; change its type to nullable (string?) on the
SendVideoResult class and update any constructors or initialization sites of
SendVideoResult (e.g., object initializers and any factory methods) so they
either set Error explicitly or rely on the nullable property; ensure callers
check for null instead of assuming a value to eliminate CS8618 warnings and
reflect the optional error semantics.
In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs`:
- Around line 46-60: The current checks in SendDocumentToolService that validate
filePath (returning SendDocumentResult with toolContext.ChatId) must first
canonicalize and restrict the path to an allowlisted root: call
Path.GetFullPath(filePath) into a canonicalPath, obtain the configured safe root
(e.g. a service-level _allowedRoot or configuration value), then ensure
canonicalPath.StartsWith(Path.GetFullPath(_allowedRoot),
StringComparison.OrdinalIgnoreCase) (or use a robust path-contains helper) and
return a failed SendDocumentResult if it’s outside that root before performing
File.Exists; update the code around the filePath checks to use canonicalPath and
the configured allowed root check so arbitrary server paths cannot be probed.
- Around line 72-85: The current code reads the entire file into memory
(File.ReadAllBytesAsync + new MemoryStream) before enqueuing the send via
_sendMessage.AddTaskWithResult, which can OOM for large files; change to open a
FileStream (File.OpenRead) and create the InputFile.FromStream inside the async
lambda passed to _sendMessage.AddTaskWithResult so the file is streamed during
SendDocument (TelegramBotClient.SendDocument) rather than buffered up-front;
ensure the FileStream is disposed after SendDocument completes (use a using or
try/finally inside the task), keep the same caption/truncation, parseMode,
replyParameters, cancellation token (cts.Token) and target chat
(toolContext.ChatId), and remove the ReadAllBytesAsync/new MemoryStream usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53ec7248-0dd2-49fb-8323-980c28f33298
📒 Files selected for processing (8)
TelegramSearchBot.Common/Model/Tools/SendDocumentResult.csTelegramSearchBot.Common/Model/Tools/SendVideoResult.csTelegramSearchBot/Controller/AI/LLM/LLMIterationCallbackController.csTelegramSearchBot/Interface/Tools/ISendDocumentToolService.csTelegramSearchBot/Interface/Tools/ISendVideoToolService.csTelegramSearchBot/Service/Tools/SendDocumentToolService.csTelegramSearchBot/Service/Tools/SendPhotoToolService.csTelegramSearchBot/Service/Tools/SendVideoToolService.cs
✅ Files skipped from review due to trivial changes (3)
- TelegramSearchBot/Interface/Tools/ISendVideoToolService.cs
- TelegramSearchBot.Common/Model/Tools/SendDocumentResult.cs
- TelegramSearchBot/Interface/Tools/ISendDocumentToolService.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- TelegramSearchBot/Service/Tools/SendPhotoToolService.cs
- TelegramSearchBot/Service/Tools/SendVideoToolService.cs
| resumeStream, | ||
| snapshot.ChatId, | ||
| snapshot.OriginalMessageId, | ||
| (int)snapshot.OriginalMessageId, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify OriginalMessageId definition and data flow before keeping direct casts.
rg -nP --type=cs '\bOriginalMessageId\b' -C3
rg -nP --type=cs '\(int\)\s*snapshot\.OriginalMessageId' -C3Repository: ModerRAS/TelegramSearchBot
Length of output: 12823
🏁 Script executed:
# Check SendDraftStream method signature
rg -nP --type=cs 'SendDraftStream' -A 5 | head -40
# Check ReplyParameters MessageId type
rg -nP --type=cs 'ReplyParameters' -B 2 -A 5 | head -50
# Check if there's any validation of OriginalMessageId before use
rg -nP --type=cs 'OriginalMessageId' -B 2 -A 2 | grep -A 2 -B 2 'if\|check\|valid'Repository: ModerRAS/TelegramSearchBot
Length of output: 9023
🏁 Script executed:
# Look for SendDraftStream definition
fd --type f '*.cs' -x grep -l 'SendDraftStream' {} \; | head -5
# Get the actual method signature
rg -nP --type=cs 'void\s+SendDraftStream|Task.*SendDraftStream|async.*SendDraftStream' -A 3Repository: ModerRAS/TelegramSearchBot
Length of output: 1204
🏁 Script executed:
# Check what type Telegram.Bot API expects for MessageId in ReplyParameters
rg -nP --type=cs 'class ReplyParameters|MessageId.*=' -B 2 -A 2 | head -60Repository: ModerRAS/TelegramSearchBot
Length of output: 3783
Extract and validate snapshot.OriginalMessageId before using in API calls.
Both cast sites (lines 157, 201) convert long to int without validation. Assign to a variable to avoid duplication and add range check.
Suggested fix
+ if (snapshot.OriginalMessageId <= 0 || snapshot.OriginalMessageId > int.MaxValue) {
+ _logger.LogWarning("Invalid OriginalMessageId {OriginalMessageId} for snapshot {SnapshotId}.",
+ snapshot.OriginalMessageId, snapshotId);
+ await _botClient.AnswerCallbackQuery(e.CallbackQuery.Id, "⚠️ 原始消息ID无效,无法继续");
+ return;
+ }
+ var replyMessageId = (int)snapshot.OriginalMessageId;
+
List<Model.Data.Message> sentMessagesForDb = await _sendMessageService.SendDraftStream(
resumeStream,
snapshot.ChatId,
- (int)snapshot.OriginalMessageId,
+ replyMessageId,
initialContent,
CancellationToken.None
);
...
await _botClient.SendMessage(
snapshot.ChatId,
$"⚠️ AI 再次达到最大迭代次数限制({Env.MaxToolCycles} 次),已完成 {executionContext.SnapshotData.CyclesSoFar} 次循环,是否继续迭代?",
replyMarkup: keyboard,
- replyParameters: new ReplyParameters { MessageId = (int)snapshot.OriginalMessageId }
+ replyParameters: new ReplyParameters { MessageId = replyMessageId }
);Also applies to: 201-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TelegramSearchBot/Controller/AI/LLM/LLMIterationCallbackController.cs` at
line 157, Extract snapshot.OriginalMessageId into a local variable (e.g.,
originalMessageIdLong), validate it fits into a 32-bit int (ensure value >=
int.MinValue and <= int.MaxValue), then assign to an int (e.g.,
originalMessageIdInt) and use that int for the API calls instead of casting
inline; apply this change to both places that currently use
(int)snapshot.OriginalMessageId in LLMIterationCallbackController so you avoid
duplicated casts and handle out-of-range values (throw or return a handled
error) before calling the API.
| long? messageId = explicitReplyToMessageId ?? (toolContext.MessageId != 0 ? toolContext.MessageId : (long?)null); | ||
| return messageId.HasValue ? new ReplyParameters { MessageId = (int)messageId.Value } : null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify id type definitions and conversion sites.
rg -nP --type=cs '\bclass\s+ToolContext\b|\bMessageId\b' -C3
rg -nP --type=cs '\(int\)\s*messageId\.Value|\(int\)\s*toolContext\.MessageId' -C3Repository: ModerRAS/TelegramSearchBot
Length of output: 50383
🏁 Script executed:
#!/bin/bash
# Find ToolContext class definition
rg -nP --type=cs 'class\s+ToolContext\s*{' -A 20Repository: ModerRAS/TelegramSearchBot
Length of output: 823
🏁 Script executed:
#!/bin/bash
# Check ReplyParameters definition and MessageId property
rg -nP --type=cs 'class ReplyParameters|public.*MessageId' TelegramSearchBot/Service/Tools/ -A 2 -B 2Repository: ModerRAS/TelegramSearchBot
Length of output: 52
🏁 Script executed:
#!/bin/bash
# Find where ToolContext is defined and its MessageId property
find . -type f -name "*.cs" | xargs grep -l "class ToolContext" 2>/dev/null | head -5Repository: ModerRAS/TelegramSearchBot
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Get the exact method signature in SendDocumentToolService
sed -n '34,37p' TelegramSearchBot/Service/Tools/SendDocumentToolService.csRepository: ModerRAS/TelegramSearchBot
Length of output: 426
🏁 Script executed:
#!/bin/bash
# Check Telegram.Bot Types ReplyParameters definition or usage examples
rg -nP --type=cs 'ReplyParameters' TelegramSearchBot/Service/Tools/SendDocumentToolService.cs -B 5 -A 5 | head -30Repository: ModerRAS/TelegramSearchBot
Length of output: 1564
🏁 Script executed:
#!/bin/bash
# Find other similar conversion patterns in the codebase
rg -nP --type=cs '\(int\)\s*messageId|\(int\).*MessageId' -C 2Repository: ModerRAS/TelegramSearchBot
Length of output: 3324
🏁 Script executed:
#!/bin/bash
# Check all instances where ReplyParameters is created with MessageId cast from long
rg -nP --type=cs 'ReplyParameters.*MessageId\s*=\s*\(int\)' -B 2 -A 2Repository: ModerRAS/TelegramSearchBot
Length of output: 2748
🏁 Script executed:
#!/bin/bash
# Verify if there are constraints on int.MaxValue for message IDs in Telegram
rg -nP --type=cs 'int\.MaxValue|int\.MinValue' -C 2Repository: ModerRAS/TelegramSearchBot
Length of output: 1003
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or comments about message ID constraints
rg -nP --type=cs 'MessageId.*int|message.*ID.*range|int.*message' -i -C 1 | head -50Repository: ModerRAS/TelegramSearchBot
Length of output: 3074
Guard long→int conversion for reply message IDs without range validation.
Line 36 casts long to int without checking if the value exceeds int.MaxValue. Silent truncation can occur, targeting the wrong message ID. The same unsafe pattern exists in SendVideoToolService.cs and SendPhotoToolService.cs.
Add validation to ensure the message ID is within the valid int range before casting:
✅ Suggested fix
private static ReplyParameters GetReplyParameters(int? explicitReplyToMessageId, ToolContext toolContext) {
long? messageId = explicitReplyToMessageId ?? (toolContext.MessageId != 0 ? toolContext.MessageId : (long?)null);
- return messageId.HasValue ? new ReplyParameters { MessageId = (int)messageId.Value } : null;
+ if (!messageId.HasValue || messageId.Value <= 0 || messageId.Value > int.MaxValue) {
+ return null;
+ }
+ return new ReplyParameters { MessageId = (int)messageId.Value };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs` around lines 35 -
37, The code casts a nullable long (messageId) to int without range checking,
risking truncation; update the ReplyParameters creation in
SendDocumentToolService (and mirror the same fix in SendVideoToolService and
SendPhotoToolService) to validate messageId.Value is within
int.MinValue..int.MaxValue before casting; if messageId is out of range, handle
it safely (e.g. log/warn and return null or otherwise avoid casting) and only
then construct new ReplyParameters { MessageId = (int)messageId.Value } so the
cast is guarded and cannot silently truncate.
| if (!File.Exists(filePath)) { | ||
| return new SendDocumentResult { | ||
| Success = false, | ||
| ChatId = toolContext.ChatId, | ||
| Error = $"File not found: {filePath}" | ||
| }; |
There was a problem hiding this comment.
Avoid leaking internal filesystem/exception details to end users.
Returning full paths and raw exception text in chat responses exposes server internals.
🛡️ Suggested fix
- Error = $"File not found: {filePath}"
+ Error = "File not found."
...
} catch (Exception ex) {
+ // log ex server-side
return new SendDocumentResult {
Success = false,
ChatId = toolContext.ChatId,
- Error = $"Failed to send document: {ex.Message}"
+ Error = "Failed to send document."
};
}Also applies to: 92-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs` around lines 54 -
59, The code in SendDocumentToolService is returning full filesystem paths and
likely raw exception text to users (see the File.Exists branch and the similar
block at lines 92-97); change these return Error messages to a generic,
non-sensitive message like "File not available" or "Unable to retrieve document"
and move detailed info (full path and exception.ToString()) into an internal log
call using the service's logger (e.g., ILogger in SendDocumentToolService or an
existing logging instance) so the user-facing SendDocumentResult contains no
server-internal paths or exception details while the logs retain the full
diagnostics.
8902f28 to
ed68b1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
TelegramSearchBot.Common/Model/Tools/SendVideoResult.cs (1)
6-6:⚠️ Potential issue | 🟡 MinorMake
Errornullable to match actual usage.The
Errorproperty is only set on failure paths, but the current non-nullablestringdeclaration triggers nullable reference warnings and implies the property always has a value. This issue was previously flagged.🩹 Proposed fix
- public string Error { get; set; } + public string? Error { get; set; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot.Common/Model/Tools/SendVideoResult.cs` at line 6, The Error property on the SendVideoResult class is declared as a non-nullable string but is only populated on failure paths; change the Error property's type to nullable (string?) on SendVideoResult to match actual usage and silence nullable-reference warnings, and update any constructors or assignments that assume non-nullability (e.g., places that instantiate SendVideoResult or read/write Error) to handle null appropriately.
🤖 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/Service/Tools/SendDocumentToolService.cs`:
- Around line 62-70: The error message in SendDocumentToolService uses integer
division to show file size, causing truncation; change the GB calculation to use
floating-point division (e.g., divide by 1024.0*1024*1024) and format the result
with a suitable precision (like one or two decimal places) when building the
Error string so sizes like 1.9GB display correctly; you can also derive the max
size in GB from maxFileSizeBytes using the same floating-point calculation to
keep the message consistent.
---
Duplicate comments:
In `@TelegramSearchBot.Common/Model/Tools/SendVideoResult.cs`:
- Line 6: The Error property on the SendVideoResult class is declared as a
non-nullable string but is only populated on failure paths; change the Error
property's type to nullable (string?) on SendVideoResult to match actual usage
and silence nullable-reference warnings, and update any constructors or
assignments that assume non-nullability (e.g., places that instantiate
SendVideoResult or read/write Error) to handle null appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63e348c3-f746-4169-8945-a7b019e1ef0f
📒 Files selected for processing (8)
TelegramSearchBot.Common/Model/Tools/SendDocumentResult.csTelegramSearchBot.Common/Model/Tools/SendVideoResult.csTelegramSearchBot/Controller/AI/LLM/LLMIterationCallbackController.csTelegramSearchBot/Interface/Tools/ISendDocumentToolService.csTelegramSearchBot/Interface/Tools/ISendVideoToolService.csTelegramSearchBot/Service/Tools/SendDocumentToolService.csTelegramSearchBot/Service/Tools/SendPhotoToolService.csTelegramSearchBot/Service/Tools/SendVideoToolService.cs
✅ Files skipped from review due to trivial changes (6)
- TelegramSearchBot/Controller/AI/LLM/LLMIterationCallbackController.cs
- TelegramSearchBot/Interface/Tools/ISendDocumentToolService.cs
- TelegramSearchBot/Interface/Tools/ISendVideoToolService.cs
- TelegramSearchBot.Common/Model/Tools/SendDocumentResult.cs
- TelegramSearchBot/Service/Tools/SendPhotoToolService.cs
- TelegramSearchBot/Service/Tools/SendVideoToolService.cs
| var fileInfo = new FileInfo(filePath); | ||
| const long maxFileSizeBytes = 2L * 1024 * 1024 * 1024; | ||
| if (fileInfo.Length > maxFileSizeBytes) { | ||
| return new SendDocumentResult { | ||
| Success = false, | ||
| ChatId = toolContext.ChatId, | ||
| Error = $"File is too large ({fileInfo.Length / 1024 / 1024 / 1024}GB). Maximum allowed size is 2GB." | ||
| }; | ||
| } |
There was a problem hiding this comment.
File size in error message uses truncating integer division.
Line 68 calculates GB using integer division, which truncates. A 1.9GB file would display as "1GB" in the error, potentially confusing users about why their "1GB" file exceeds the 2GB limit.
🔧 Suggested fix using floating-point division
- Error = $"File is too large ({fileInfo.Length / 1024 / 1024 / 1024}GB). Maximum allowed size is 2GB."
+ Error = $"File is too large ({fileInfo.Length / 1024.0 / 1024.0 / 1024.0:F2}GB). Maximum allowed size is 2GB."📝 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.
| var fileInfo = new FileInfo(filePath); | |
| const long maxFileSizeBytes = 2L * 1024 * 1024 * 1024; | |
| if (fileInfo.Length > maxFileSizeBytes) { | |
| return new SendDocumentResult { | |
| Success = false, | |
| ChatId = toolContext.ChatId, | |
| Error = $"File is too large ({fileInfo.Length / 1024 / 1024 / 1024}GB). Maximum allowed size is 2GB." | |
| }; | |
| } | |
| var fileInfo = new FileInfo(filePath); | |
| const long maxFileSizeBytes = 2L * 1024 * 1024 * 1024; | |
| if (fileInfo.Length > maxFileSizeBytes) { | |
| return new SendDocumentResult { | |
| Success = false, | |
| ChatId = toolContext.ChatId, | |
| Error = $"File is too large ({fileInfo.Length / 1024.0 / 1024.0 / 1024.0:F2}GB). Maximum allowed size is 2GB." | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs` around lines 62 -
70, The error message in SendDocumentToolService uses integer division to show
file size, causing truncation; change the GB calculation to use floating-point
division (e.g., divide by 1024.0*1024*1024) and format the result with a
suitable precision (like one or two decimal places) when building the Error
string so sizes like 1.9GB display correctly; you can also derive the max size
in GB from maxFileSizeBytes using the same floating-point calculation to keep
the message consistent.
…olService interface
Also fix pre-existing long->int conversion bugs in SendPhotoToolService and LLMIterationCallbackController
ed68b1d to
60b4519
Compare
Also fix pre-existing long->int conversion bugs and apply formatting fixes
60b4519 to
67a8b44
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (9)
TelegramSearchBot/Service/Tools/SendVideoToolService.cs (4)
55-60:⚠️ Potential issue | 🟠 MajorDo not expose server paths or raw exception messages in tool responses.
Line 59 and Line 99 leak internal details to callers. Return generic errors and log full diagnostics server-side.
Suggested fix
- Error = $"File not found: {filePath}" + Error = "File not found." ... } catch (Exception ex) { + // _logger.LogError(ex, "Failed to send video from path {FilePath}", filePath); return new SendVideoResult { Success = false, ChatId = toolContext.ChatId, - Error = $"Failed to send video: {ex.Message}" + Error = "Failed to send video." }; }Also applies to: 95-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Tools/SendVideoToolService.cs` around lines 55 - 60, The response currently leaks internal file paths and raw exception details in SendVideoResult (see SendVideoToolService return that sets Error using filePath and the exception handling block around lines referencing SendVideoResult); change both places to return a generic, non-sensitive message (e.g., "File unavailable" or "Failed to prepare video") while moving full diagnostics (filePath, stack trace, exception) into a server-side log via the existing logger (e.g., _logger.LogError/LogDebug) so callers receive no internal paths or raw exceptions but admins can still debug.
47-56:⚠️ Potential issue | 🔴 CriticalRestrict
filePathto an allowlisted media root before filesystem access.Line 55 checks existence directly on user-provided paths, which enables arbitrary file reads.
Suggested fix
+ var safeRoot = Path.GetFullPath(/* configured safe media root */); + var fullPath = Path.GetFullPath(filePath); + var safePrefix = safeRoot.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) + + Path.DirectorySeparatorChar; + if (!fullPath.StartsWith(safePrefix, StringComparison.OrdinalIgnoreCase)) { + return new SendVideoResult { + Success = false, + ChatId = toolContext.ChatId, + Error = "Path outside allowed media directory." + }; + } - - if (!File.Exists(filePath)) { + if (!File.Exists(fullPath)) { return new SendVideoResult { Success = false, ChatId = toolContext.ChatId, - Error = $"File not found: {filePath}" + Error = "File not found." }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Tools/SendVideoToolService.cs` around lines 47 - 56, The code in SendVideoToolService uses the user-provided filePath directly with File.Exists which allows arbitrary file reads; before any filesystem access (including the existing string.IsNullOrWhiteSpace check and the File.Exists call) validate and restrict filePath to an allowlisted media root: normalize the path with Path.GetFullPath, resolve it against your configured media root (e.g., Path.Combine(mediaRoot, filePath) or resolve relative paths), and then ensure the resolved full path starts with the mediaRoot full path (case-insensitive on Windows) to prevent directory traversal; only then proceed to use File.Exists and return/send a SendVideoResult.
35-37:⚠️ Potential issue | 🟡 MinorGuard
long→intreply ID conversion before casting.Line 37 can silently truncate out-of-range values. Validate range (and positivity) before creating
ReplyParameters.Suggested fix
private static ReplyParameters GetReplyParameters(long? explicitReplyToMessageId, ToolContext toolContext) { long? messageId = explicitReplyToMessageId ?? ( toolContext.MessageId != 0 ? toolContext.MessageId : ( long? ) null ); - return messageId.HasValue ? new ReplyParameters { MessageId = ( int ) messageId.Value } : null; + if (!messageId.HasValue || messageId.Value <= 0 || messageId.Value > int.MaxValue) { + return null; + } + return new ReplyParameters { MessageId = ( int ) messageId.Value }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Tools/SendVideoToolService.cs` around lines 35 - 37, GetReplyParameters currently casts a long to int without range checks; before creating ReplyParameters in GetReplyParameters validate that messageId.HasValue and that messageId.Value is within int range and positive (e.g., messageId.Value > 0 && messageId.Value <= int.MaxValue); if the check fails, avoid the unsafe cast (return null or handle as an error) and only then construct new ReplyParameters { MessageId = (int)messageId.Value } to prevent silent truncation.
75-88:⚠️ Potential issue | 🔴 CriticalStream the video inside the queued send task instead of buffering the full file.
Line 75 loads the entire file into RAM before rate-limited execution; this can OOM under the 2GB local limit.
Suggested fix
- var fileBytes = await File.ReadAllBytesAsync(filePath); - var video = InputFile.FromStream(new MemoryStream(fileBytes), fileInfo.Name); - var replyParameters = GetReplyParameters(replyToMessageId, toolContext); using var cts = new CancellationTokenSource(SendTimeout); - var message = await _sendMessage.AddTaskWithResult(async () => await _botClient.SendVideo( - chatId: toolContext.ChatId, - video: video, - caption: string.IsNullOrEmpty(caption) ? null : caption.Length > 1024 ? caption.Substring(0, 1024) : caption, - parseMode: ParseMode.Html, - replyParameters: replyParameters, - cancellationToken: cts.Token - ), toolContext.ChatId); + var message = await _sendMessage.AddTaskWithResult(async () => { + await using var stream = new FileStream( + filePath, FileMode.Open, FileAccess.Read, FileShare.Read, 81920, useAsync: true); + var video = InputFile.FromStream(stream, fileInfo.Name); + + return await _botClient.SendVideo( + chatId: toolContext.ChatId, + video: video, + caption: string.IsNullOrEmpty(caption) ? null : caption.Length > 1024 ? caption.Substring(0, 1024) : caption, + parseMode: ParseMode.Html, + replyParameters: replyParameters, + cancellationToken: cts.Token + ); + }, toolContext.ChatId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Tools/SendVideoToolService.cs` around lines 75 - 88, The code currently buffers the entire video with File.ReadAllBytesAsync(filePath) before queuing, risking OOM; instead open a FileStream and create the InputFile.FromStream inside the queued task passed to _sendMessage.AddTaskWithResult so the file is streamed during the rate-limited send. Move the File.OpenRead(filePath) and InputFile.FromStream(new FileStream(...), fileInfo.Name) into the async delegate (the lambda that calls _botClient.SendVideo), wrap the stream in a using to dispose after SendVideo completes, preserve caption truncation, parseMode, replyParameters, SendTimeout/CancellationTokenSource usage, and keep toolContext.ChatId for AddTaskWithResult.TelegramSearchBot/Service/Tools/SendDocumentToolService.cs (5)
35-37:⚠️ Potential issue | 🟡 MinorGuard
long→intreply ID conversion before casting.Line 37 can truncate out-of-range values. Validate before casting.
Suggested fix
private static ReplyParameters GetReplyParameters(long? explicitReplyToMessageId, ToolContext toolContext) { long? messageId = explicitReplyToMessageId ?? ( toolContext.MessageId != 0 ? toolContext.MessageId : ( long? ) null ); - return messageId.HasValue ? new ReplyParameters { MessageId = ( int ) messageId.Value } : null; + if (!messageId.HasValue || messageId.Value <= 0 || messageId.Value > int.MaxValue) { + return null; + } + return new ReplyParameters { MessageId = ( int ) messageId.Value }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs` around lines 35 - 37, GetReplyParameters currently casts a long? messageId to int when creating a ReplyParameters instance, which may truncate or overflow; before casting in GetReplyParameters validate that messageId.Value fits within Int32.MinValue..Int32.MaxValue (or otherwise handle out-of-range cases) and only assign MessageId when within range; if out-of-range, return null or handle via an appropriate fallback (e.g., log/throw) so ReplyParameters.MessageId is never created from a truncated cast. Reference: GetReplyParameters, explicitReplyToMessageId, toolContext.MessageId, messageId, ReplyParameters.
47-56:⚠️ Potential issue | 🔴 CriticalRestrict
filePathto an allowlisted upload root beforeFile.Exists.Line 55 accepts arbitrary readable paths; this is an exfiltration risk.
Suggested fix
+ var safeRoot = Path.GetFullPath(/* configured safe upload root */); + var fullPath = Path.GetFullPath(filePath); + var safePrefix = safeRoot.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar) + + Path.DirectorySeparatorChar; + if (!fullPath.StartsWith(safePrefix, StringComparison.OrdinalIgnoreCase)) { + return new SendDocumentResult { + Success = false, + ChatId = toolContext.ChatId, + Error = "Path outside allowed upload directory." + }; + } - - if (!File.Exists(filePath)) { + if (!File.Exists(fullPath)) { return new SendDocumentResult { Success = false, ChatId = toolContext.ChatId, - Error = $"File not found: {filePath}" + Error = "File not found." }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs` around lines 47 - 56, Validate that the provided filePath is inside an allowed upload root before calling File.Exists to prevent arbitrary file access: in SendDocumentToolService (the method using the filePath variable that returns SendDocumentResult and uses toolContext.ChatId), resolve the absolute/canonical path (e.g., Path.GetFullPath or similar), normalize it, and ensure it starts with the configured allowlist root(s); if it does not, return a SendDocumentResult with Success=false and an explicit error (instead of checking File.Exists first); only after the allowlist check proceed to File.Exists and the existing logic.
55-60:⚠️ Potential issue | 🟠 MajorReturn generic user errors; log sensitive details internally.
Line 59 and Line 99 expose filesystem and exception internals.
Suggested fix
- Error = $"File not found: {filePath}" + Error = "File not found." ... } catch (Exception ex) { + // _logger.LogError(ex, "Failed to send document from path {FilePath}", filePath); return new SendDocumentResult { Success = false, ChatId = toolContext.ChatId, - Error = $"Failed to send document: {ex.Message}" + Error = "Failed to send document." }; }Also applies to: 95-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs` around lines 55 - 60, The SendDocumentToolService currently returns SendDocumentResult.Error containing sensitive filesystem paths and exception details (e.g., using filePath and exception messages); change both failure branches (the File.Exists check and the exception catch around SendDocument logic) to return a generic user-facing message like "Unable to send document" while logging the detailed info (filePath, exception) to the internal logger (e.g., _logger or ILogger used in SendDocumentToolService) along with toolContext.ChatId for traceability; ensure the SendDocumentResult still sets Success = false and ChatId = toolContext.ChatId but never includes raw paths or exception stacks in the Error property.
67-72:⚠️ Potential issue | 🟡 MinorFormat reported file size using floating-point math.
Line 71 uses integer division and truncates size display.
Suggested fix
+ var fileSizeMb = fileInfo.Length / (1024d * 1024d); return new SendDocumentResult { Success = false, ChatId = toolContext.ChatId, - Error = $"File is too large ({fileInfo.Length / 1024 / 1024}MB). Maximum allowed size is {( Env.IsLocalAPI ? "2GB" : "50MB" )}." + Error = $"File is too large ({fileSizeMb:F2}MB). Maximum allowed size is {( Env.IsLocalAPI ? "2GB" : "50MB" )}." };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs` around lines 67 - 72, The error message uses integer division (fileInfo.Length / 1024 / 1024) which truncates MB; update the SendDocumentResult Error formatting in the file size check inside SendDocumentToolService so it uses floating-point division and formats to a readable precision (e.g., fileInfo.Length / 1024.0 / 1024.0 with a format specifier like F2) when referencing fileInfo.Length and maxFileSizeBytes to produce a decimal MB value; keep the rest of the message (including references to Env.IsLocalAPI and toolContext.ChatId) unchanged.
75-88:⚠️ Potential issue | 🔴 CriticalStream document uploads inside the queued task; avoid
ReadAllBytesAsync.Line 75 eagerly buffers the whole file, which can OOM for large files.
Suggested fix
- var fileBytes = await File.ReadAllBytesAsync(filePath); - var document = InputFile.FromStream(new MemoryStream(fileBytes), fileInfo.Name); - var replyParameters = GetReplyParameters(replyToMessageId, toolContext); using var cts = new CancellationTokenSource(SendTimeout); - var message = await _sendMessage.AddTaskWithResult(async () => await _botClient.SendDocument( - chatId: toolContext.ChatId, - document: document, - caption: string.IsNullOrEmpty(caption) ? null : caption.Length > 1024 ? caption.Substring(0, 1024) : caption, - parseMode: ParseMode.Html, - replyParameters: replyParameters, - cancellationToken: cts.Token - ), toolContext.ChatId); + var message = await _sendMessage.AddTaskWithResult(async () => { + await using var stream = new FileStream( + filePath, FileMode.Open, FileAccess.Read, FileShare.Read, 81920, useAsync: true); + var document = InputFile.FromStream(stream, fileInfo.Name); + + return await _botClient.SendDocument( + chatId: toolContext.ChatId, + document: document, + caption: string.IsNullOrEmpty(caption) ? null : caption.Length > 1024 ? caption.Substring(0, 1024) : caption, + parseMode: ParseMode.Html, + replyParameters: replyParameters, + cancellationToken: cts.Token + ); + }, toolContext.ChatId);
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@TelegramSearchBot/Service/Tools/SendDocumentToolService.cs`:
- Around line 35-37: GetReplyParameters currently casts a long? messageId to int
when creating a ReplyParameters instance, which may truncate or overflow; before
casting in GetReplyParameters validate that messageId.Value fits within
Int32.MinValue..Int32.MaxValue (or otherwise handle out-of-range cases) and only
assign MessageId when within range; if out-of-range, return null or handle via
an appropriate fallback (e.g., log/throw) so ReplyParameters.MessageId is never
created from a truncated cast. Reference: GetReplyParameters,
explicitReplyToMessageId, toolContext.MessageId, messageId, ReplyParameters.
- Around line 47-56: Validate that the provided filePath is inside an allowed
upload root before calling File.Exists to prevent arbitrary file access: in
SendDocumentToolService (the method using the filePath variable that returns
SendDocumentResult and uses toolContext.ChatId), resolve the absolute/canonical
path (e.g., Path.GetFullPath or similar), normalize it, and ensure it starts
with the configured allowlist root(s); if it does not, return a
SendDocumentResult with Success=false and an explicit error (instead of checking
File.Exists first); only after the allowlist check proceed to File.Exists and
the existing logic.
- Around line 55-60: The SendDocumentToolService currently returns
SendDocumentResult.Error containing sensitive filesystem paths and exception
details (e.g., using filePath and exception messages); change both failure
branches (the File.Exists check and the exception catch around SendDocument
logic) to return a generic user-facing message like "Unable to send document"
while logging the detailed info (filePath, exception) to the internal logger
(e.g., _logger or ILogger used in SendDocumentToolService) along with
toolContext.ChatId for traceability; ensure the SendDocumentResult still sets
Success = false and ChatId = toolContext.ChatId but never includes raw paths or
exception stacks in the Error property.
- Around line 67-72: The error message uses integer division (fileInfo.Length /
1024 / 1024) which truncates MB; update the SendDocumentResult Error formatting
in the file size check inside SendDocumentToolService so it uses floating-point
division and formats to a readable precision (e.g., fileInfo.Length / 1024.0 /
1024.0 with a format specifier like F2) when referencing fileInfo.Length and
maxFileSizeBytes to produce a decimal MB value; keep the rest of the message
(including references to Env.IsLocalAPI and toolContext.ChatId) unchanged.
In `@TelegramSearchBot/Service/Tools/SendVideoToolService.cs`:
- Around line 55-60: The response currently leaks internal file paths and raw
exception details in SendVideoResult (see SendVideoToolService return that sets
Error using filePath and the exception handling block around lines referencing
SendVideoResult); change both places to return a generic, non-sensitive message
(e.g., "File unavailable" or "Failed to prepare video") while moving full
diagnostics (filePath, stack trace, exception) into a server-side log via the
existing logger (e.g., _logger.LogError/LogDebug) so callers receive no internal
paths or raw exceptions but admins can still debug.
- Around line 47-56: The code in SendVideoToolService uses the user-provided
filePath directly with File.Exists which allows arbitrary file reads; before any
filesystem access (including the existing string.IsNullOrWhiteSpace check and
the File.Exists call) validate and restrict filePath to an allowlisted media
root: normalize the path with Path.GetFullPath, resolve it against your
configured media root (e.g., Path.Combine(mediaRoot, filePath) or resolve
relative paths), and then ensure the resolved full path starts with the
mediaRoot full path (case-insensitive on Windows) to prevent directory
traversal; only then proceed to use File.Exists and return/send a
SendVideoResult.
- Around line 35-37: GetReplyParameters currently casts a long to int without
range checks; before creating ReplyParameters in GetReplyParameters validate
that messageId.HasValue and that messageId.Value is within int range and
positive (e.g., messageId.Value > 0 && messageId.Value <= int.MaxValue); if the
check fails, avoid the unsafe cast (return null or handle as an error) and only
then construct new ReplyParameters { MessageId = (int)messageId.Value } to
prevent silent truncation.
- Around line 75-88: The code currently buffers the entire video with
File.ReadAllBytesAsync(filePath) before queuing, risking OOM; instead open a
FileStream and create the InputFile.FromStream inside the queued task passed to
_sendMessage.AddTaskWithResult so the file is streamed during the rate-limited
send. Move the File.OpenRead(filePath) and InputFile.FromStream(new
FileStream(...), fileInfo.Name) into the async delegate (the lambda that calls
_botClient.SendVideo), wrap the stream in a using to dispose after SendVideo
completes, preserve caption truncation, parseMode, replyParameters,
SendTimeout/CancellationTokenSource usage, and keep toolContext.ChatId for
AddTaskWithResult.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57439da9-f236-4d08-9ff4-69e71684a573
📒 Files selected for processing (2)
TelegramSearchBot/Service/Tools/SendDocumentToolService.csTelegramSearchBot/Service/Tools/SendVideoToolService.cs
Summary
send_video_fileMCP tool for sending videos via file pathsend_document_fileMCP tool for sending documents/files via file pathChanges
SendVideoToolService.cs/ISendVideoToolService.cs— video sending toolSendDocumentToolService.cs/ISendDocumentToolService.cs— document sending toolSendVideoResult.cs/SendDocumentResult.cs— result modelsFeatures
Env.IsLocalAPI=true, 50MB otherwisesend_photo_filepattern with[BuiltInTool]attributes_sendMessage.AddTaskWithResult()Also includes
long→intconversion bugs inSendPhotoToolServiceandLLMIterationCallbackControllerSummary by CodeRabbit