Skip to content

Conversation

iceljc
Copy link
Collaborator

@iceljc iceljc commented Aug 26, 2025

PR Type

Enhancement, Bug fix


Description

  • Add null check for agent to prevent runtime errors

  • Include ContentType property in file mapping

  • Remove unused BotSharp.Abstraction.Hooks import

  • Minor code formatting improvements


Diagram Walkthrough

flowchart LR
  A["Agent Loading"] --> B["Null Check Added"]
  B --> C["File Mapping Enhanced"]
  C --> D["ContentType Property"]
  E["Unused Import"] --> F["Removed"]
Loading

File Walkthrough

Relevant files
Enhancement
InstructService.Execute.cs
Enhanced error handling and file mapping                                 

src/Infrastructure/BotSharp.Core/Instructs/Services/InstructService.Execute.cs

  • Added null check for loaded agent with error response
  • Enhanced file mapping to include ContentType property
  • Removed unused BotSharp.Abstraction.Hooks import
  • Added minor formatting improvements
+11/-2   

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Null Check Logic

Verify that IAgentService.LoadAgent can actually return null; if it throws or guarantees a non-null Agent, this check is dead code. Also confirm downstream code paths handle both the new early return and the existing agent.Disabled branch consistently with expected API behavior.

if (agent == null)
{
    return new InstructResult
    {
        MessageId = message.MessageId,
        Text = $"Agent (id: {agentId}) does not exist!"
    };
}
ContentType Mapping

Ensure ContentType is non-empty and valid before passing to BotSharpFile. Consider normalizing or defaulting when missing to avoid provider-side errors.

    Files = files?.Select(x => new BotSharpFile { FileUrl = x.FileUrl, FileData = x.FileData, ContentType = x.ContentType }).ToList() ?? []
}

Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Default file content type

Ensure ContentType is always set to a safe default to avoid null/empty values
causing downstream errors (e.g., when handling uploads or HTTP headers).
Fallback to "application/octet-stream" if x.ContentType is null or whitespace.

src/Infrastructure/BotSharp.Core/Instructs/Services/InstructService.Execute.cs [98]

-Files = files?.Select(x => new BotSharpFile { FileUrl = x.FileUrl, FileData = x.FileData, ContentType = x.ContentType }).ToList() ?? []
+Files = files?.Select(x => new BotSharpFile { FileUrl = x.FileUrl, FileData = x.FileData, ContentType = string.IsNullOrWhiteSpace(x.ContentType) ? "application/octet-stream" : x.ContentType }).ToList() ?? []
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: This is a good suggestion for improving robustness by ensuring ContentType has a safe default value, which can prevent potential downstream errors if it's null or empty.

Low
  • More

@iceljc iceljc merged commit 5d875d0 into SciSharp:master Aug 26, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant