Skip to content

optimize AI Context#1328

Merged
iceljc merged 2 commits intoSciSharp:masterfrom
yileicn:master
Apr 21, 2026
Merged

optimize AI Context#1328
iceljc merged 2 commits intoSciSharp:masterfrom
yileicn:master

Conversation

@yileicn
Copy link
Copy Markdown
Member

@yileicn yileicn commented Apr 21, 2026

No description provided.

@qodo-code-review
Copy link
Copy Markdown
Contributor

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

Review Summary by Qodo

Add RecordOnly message type and exclude from LLM context

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add new RecordOnly message type for audit-only messages
• Exclude RecordOnly messages from LLM dialog history
• Filter conversation content to skip record-only messages
Diagram
flowchart LR
  A["MessageTypeName enum"] -->|"adds RecordOnly constant"| B["New message type"]
  C["GetConversationContent method"] -->|"filters using RecordOnly"| D["Optimized dialog history"]
  B -->|"excluded from"| D
Loading

Grey Divider

File Changes

1. src/Infrastructure/BotSharp.Abstraction/Conversations/Enums/MessageTypeName.cs ✨ Enhancement +4/-0

Add RecordOnly message type constant

• Add new RecordOnly constant to message type enum
• Include XML documentation explaining purpose for audit/record without LLM inclusion

src/Infrastructure/BotSharp.Abstraction/Conversations/Enums/MessageTypeName.cs


2. src/Infrastructure/BotSharp.Core/Routing/RoutingService.GetConversationContent.cs ✨ Enhancement +4/-2

Filter RecordOnly messages from conversation content

• Add using statement for MessageTypeName enum
• Filter dialogs to exclude RecordOnly messages before processing
• Optimize conversation content by removing record-only entries from LLM context

src/Infrastructure/BotSharp.Core/Routing/RoutingService.GetConversationContent.cs


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented Apr 21, 2026

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (1) 📎 Requirement gaps (0)

Grey Divider


Action required

1. RecordOnly leaks to LLM 🐞 Bug ⛨ Security
Description
MessageTypeName.RecordOnly is documented as “excluded from default LLM dialog history”, but
ConversationService.GetConversationSummary and EvaluatingService still build LLM inputs from
_storage.GetDialogs without filtering RecordOnly. This can send record/audit-only content to LLM
providers through summarization/evaluation flows.
Code

src/Infrastructure/BotSharp.Abstraction/Conversations/Enums/MessageTypeName.cs[R6-9]

+    /// <summary>
+    /// Persisted for record/audit but excluded from default LLM dialog history.
+    /// </summary>
+    public const string RecordOnly = "record_only";
Evidence
The new RecordOnly constant explicitly states it should be excluded from default LLM dialog history,
yet ConversationService.GetConversationSummary only removes Notification and then builds a prompt
that is sent via chatCompletion.GetChatCompletions; EvaluatingService similarly converts all dialogs
(except function-role) into strings passed into instructService.Instruct. Neither path filters
MessageType == record_only, so any persisted RecordOnly content will be included in those LLM
requests.

src/Infrastructure/BotSharp.Abstraction/Conversations/Enums/MessageTypeName.cs[3-14]
src/Infrastructure/BotSharp.Core/Conversations/Services/ConversationService.Summary.cs[23-29]
src/Infrastructure/BotSharp.Core/Conversations/Services/ConversationService.Summary.cs[87-97]
src/Infrastructure/BotSharp.Core/Evaluations/Services/EvaluatingService.Evaluate.cs[19-28]
src/Infrastructure/BotSharp.Core/Evaluations/Services/EvaluatingService.Evaluate.cs[78-92]
src/Infrastructure/BotSharp.Core/Evaluations/Services/EvaluatingService.Evaluate.cs[144-159]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`RecordOnly` dialogs are intended to be persisted for audit/record, but excluded from **default** LLM dialog history. Some LLM-facing flows (conversation summarization + evaluation) currently include dialogs fetched directly from storage without filtering out `MessageTypeName.RecordOnly`, so audit-only content can be sent to LLM providers.

### Issue Context
- `MessageTypeName.RecordOnly`’s XML doc sets an explicit contract.
- `ConversationService.GetConversationSummary` builds a text prompt from stored dialogs and sends it via `GetChatCompletions`.
- `EvaluatingService` converts stored dialogs into `ref_conversation` strings passed into `instructService.Instruct`.

### Fix Focus Areas
- src/Infrastructure/BotSharp.Core/Conversations/Services/ConversationService.Summary.cs[23-29]
- src/Infrastructure/BotSharp.Core/Conversations/Services/ConversationService.Summary.cs[101-119]
- src/Infrastructure/BotSharp.Core/Evaluations/Services/EvaluatingService.Evaluate.cs[19-28]
- src/Infrastructure/BotSharp.Core/Evaluations/Services/EvaluatingService.Evaluate.cs[144-159]

### Suggested fix
Apply a consistent “LLM history” filter (at least excluding `RecordOnly`, and ideally using the same comparer conventions as elsewhere) before building prompt content in these flows. Consider centralizing the filter into a shared helper to avoid future drift.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. dialogs missing null guard 📘 Rule violation ☼ Reliability
Description
GetConversationContent dereferences dialogs via dialogs.Where(...) without checking for null,
which can throw at this boundary and fail to return a safe fallback. This violates the requirement
to add null/empty guards and return safe fallbacks when data is missing.
Code

src/Infrastructure/BotSharp.Core/Routing/RoutingService.GetConversationContent.cs[11]

+        var contentDialogs = dialogs.Where(x => x.MessageType != MessageTypeName.RecordOnly).TakeLast(maxDialogCount).ToList();
Evidence
The checklist requires null/empty validation at boundaries with safe fallbacks. The modified line
uses dialogs.Where(...) directly, which will throw NullReferenceException if dialogs is null
instead of returning a safe fallback (e.g., empty string).

src/Infrastructure/BotSharp.Core/Routing/RoutingService.GetConversationContent.cs[11-11]
Best Practice: Learned patterns

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`GetConversationContent` calls `dialogs.Where(...)` without validating `dialogs` for null/empty, risking a `NullReferenceException` instead of returning a safe fallback.

## Issue Context
This method is `public` and accepts a caller-provided list; per compliance, boundary inputs must be validated and safe fallbacks returned.

## Fix Focus Areas
- src/Infrastructure/BotSharp.Core/Routing/RoutingService.GetConversationContent.cs[7-12]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Case-sensitive RecordOnly filter 🐞 Bug ≡ Correctness
Description
RoutingService.GetConversationContent excludes RecordOnly via a case-sensitive string inequality
check. If persisted MessageType casing differs (the codebase elsewhere uses case-insensitive
IsEqualTo), RecordOnly dialogs can still be included in routing context.
Code

src/Infrastructure/BotSharp.Core/Routing/RoutingService.GetConversationContent.cs[11]

+        var contentDialogs = dialogs.Where(x => x.MessageType != MessageTypeName.RecordOnly).TakeLast(maxDialogCount).ToList();
Evidence
The new filter uses x.MessageType != MessageTypeName.RecordOnly (case-sensitive). The repository
already provides StringExtensions.IsEqualTo(..., OrdinalIgnoreCase) and uses it for message type
comparisons (e.g., filtering for Plain), indicating comparisons are intended to be case-insensitive.

src/Infrastructure/BotSharp.Core/Routing/RoutingService.GetConversationContent.cs[7-13]
src/Infrastructure/BotSharp.Abstraction/Utilities/StringExtensions.cs[48-56]
src/Infrastructure/BotSharp.Core/Conversations/Services/ConversationService.cs[166-173]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`RoutingService.GetConversationContent` filters out `RecordOnly` dialogs using a case-sensitive string comparison, which is inconsistent with the codebase’s case-insensitive `IsEqualTo` convention.

### Issue Context
If `MessageType` values come from persistence or external inputs with inconsistent casing, `record_only` entries may not be filtered and will leak into routing context.

### Fix Focus Areas
- src/Infrastructure/BotSharp.Core/Routing/RoutingService.GetConversationContent.cs[11-11]

### Suggested fix
Replace `x.MessageType != MessageTypeName.RecordOnly` with a case-insensitive check, e.g. `!x.MessageType.IsEqualTo(MessageTypeName.RecordOnly)` (handling null/empty safely).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

4. Dialog cap semantics shifted 🐞 Bug ⚙ Maintainability
Description
GetConversationContent now applies TakeLast(maxDialogCount) after filtering RecordOnly, so
maxDialogCount limits the number of non-RecordOnly dialogs rather than the last N dialogs overall.
This can change which historical messages appear in routing context when RecordOnly dialogs exist
near the tail.
Code

src/Infrastructure/BotSharp.Core/Routing/RoutingService.GetConversationContent.cs[R11-12]

+        var contentDialogs = dialogs.Where(x => x.MessageType != MessageTypeName.RecordOnly).TakeLast(maxDialogCount).ToList();
+        foreach (var dialog in contentDialogs)
Evidence
The PR changed iteration from dialogs.TakeLast(maxDialogCount) to building `contentDialogs =
dialogs.Where(...).TakeLast(maxDialogCount)`. That is a real behavior change: when RecordOnly
dialogs are present near the end, older non-RecordOnly dialogs may be pulled in to fill the cap.

src/Infrastructure/BotSharp.Core/Routing/RoutingService.GetConversationContent.cs[10-13]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`maxDialogCount` is now applied after filtering out `RecordOnly` dialogs, which changes the set of messages included in the routing prompt compared to the previous behavior.

### Issue Context
If callers treat `maxDialogCount` as a recency window (“last N events”), filtering first can expand that window backward in time when many `RecordOnly` events exist at the end.

### Fix Focus Areas
- src/Infrastructure/BotSharp.Core/Routing/RoutingService.GetConversationContent.cs[11-12]

### Suggested fix
Decide and document the intended semantics:
- If `maxDialogCount` should cap by recency regardless of type: use `dialogs.TakeLast(maxDialogCount).Where(...)`.
- If it should cap the number of *content* dialogs: keep current logic but consider documenting this behavior (e.g., in method XML docs or parameter name).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Comment on lines +6 to +9
/// <summary>
/// Persisted for record/audit but excluded from default LLM dialog history.
/// </summary>
public const string RecordOnly = "record_only";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Recordonly leaks to llm 🐞 Bug ⛨ Security

MessageTypeName.RecordOnly is documented as “excluded from default LLM dialog history”, but
ConversationService.GetConversationSummary and EvaluatingService still build LLM inputs from
_storage.GetDialogs without filtering RecordOnly. This can send record/audit-only content to LLM
providers through summarization/evaluation flows.
Agent Prompt
### Issue description
`RecordOnly` dialogs are intended to be persisted for audit/record, but excluded from **default** LLM dialog history. Some LLM-facing flows (conversation summarization + evaluation) currently include dialogs fetched directly from storage without filtering out `MessageTypeName.RecordOnly`, so audit-only content can be sent to LLM providers.

### Issue Context
- `MessageTypeName.RecordOnly`’s XML doc sets an explicit contract.
- `ConversationService.GetConversationSummary` builds a text prompt from stored dialogs and sends it via `GetChatCompletions`.
- `EvaluatingService` converts stored dialogs into `ref_conversation` strings passed into `instructService.Instruct`.

### Fix Focus Areas
- src/Infrastructure/BotSharp.Core/Conversations/Services/ConversationService.Summary.cs[23-29]
- src/Infrastructure/BotSharp.Core/Conversations/Services/ConversationService.Summary.cs[101-119]
- src/Infrastructure/BotSharp.Core/Evaluations/Services/EvaluatingService.Evaluate.cs[19-28]
- src/Infrastructure/BotSharp.Core/Evaluations/Services/EvaluatingService.Evaluate.cs[144-159]

### Suggested fix
Apply a consistent “LLM history” filter (at least excluding `RecordOnly`, and ideally using the same comparer conventions as elsewhere) before building prompt content in these flows. Consider centralizing the filter into a shared helper to avoid future drift.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@iceljc iceljc merged commit 902018a into SciSharp:master Apr 21, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants