merge dev - #1423
Merged
Merged
Conversation
… metrics
Everything in one commit because no smaller one builds. AgentTestCaseRunner.cs and
AgentTestingPlugin.cs each carry changes from several of the features below, and
splitting by file leaves an intermediate that fails to compile -- the runner would
reference an interface member that no longer exists, or the plugin would register a
judge whose implementation is not yet present. That also folds in the llmJudge work
that was already sitting uncommitted in this worktree.
Routing and agent chains
------------------------
Cases now declare what they verify (CaseType: Routing or Agent) and where the
conversation opens (EntryAgentId, defaulting to the suite own agent). That one value
decides whether the router is part of what a case measures: ConversationService
sends a routing-type agent through InstructLoop, which can hand off, and everything
else through InstructDirect, which cannot.
route_to_agent is on the mock allow list and so never reaches MockFunctionExecutor,
the only caller that records an observed tool call -- routing decisions were
therefore invisible. The chain is now reconstructed from the conversation own
assistant dialogs, each of which carries the agent that wrote it, and reported per
turn and per case with consecutive repeats collapsed by agent id.
A new agentChain assertion compares that chain in three modes: contains, ordered
(the hand-off assertion) and exact (which is how a case asserts nothing routed
away). An unrecognised mode fails rather than falling back to the loosest check.
Both agentChain and routedToAgent accept an agent id or its display name: the id is
what an author copies out of the agent list, and asserting an id against a name
could never pass no matter what the agent did.
routedToAgent now reads the chain last entry instead of a separate field, so the
two cannot disagree, and a turn-level assertion sees that turn own slice -- a turn
that produced no answer no longer inherits the previous turn agent.
Routing accuracy is tallied per model on the run, counting only Routing cases.
Errored cases count against it: "could not tell" is not "routed correctly".
E2E was dropped as a case type. A journey across several agents is an Agent case
whose agentChain assertion describes the hand-offs, and a third type bought only a
third branch in every validation and aggregation path.
Authored history
----------------
A case can carry prior turns, written into the conversation before it runs, so a
real exchange becomes a fixed starting context. Not driven through the model: no
token cost, and the preamble cannot itself become a source of flakiness.
AppendConversationDialogs is an UpdateOne with no upsert, so it silently writes
nothing when the conversation dialog document does not exist -- and PrepareAsync
deliberately does not create it. The conversation is therefore created first,
through the same call SendMessage uses, and the write is read back and counted. A
short count errors the case: running without the context it was written around would
otherwise report an ordinary pass or fail about a scenario that never existed.
Authored history is excluded from the agent chain. It is not something the agent did,
and letting it in would fail an exact chain assertion for a reason the author never
caused.
Copying a case
--------------
POST cases/{id}/copy duplicates a case inside its suite. Server-side because the
copy has to carry every field: a client that rebuilds the payload from its own form
drops what it does not know about, and a copy missing its mocks is indistinguishable
in the list until the run where it blocks every tool. Cloned by a BSON round trip
for the same reason -- a hand-written clone would silently omit the next field added.
The copy lands disabled whatever the source was. An exact duplicate joining the next
run measures the same thing twice, and for a routing case it double-weights one
routing decision.
Scope narrowing
---------------
Cases carry the registration a change-scoped evaluation needs: Priority, Severity,
Batch (derived from priority, with cross-cutting forced to batch 1), CrossCutting,
InvolvedAgents, BusinessDomain, ExpectedOutcome and LastReviewedDate. Existing cases
read back as P1/S1 -- mandatory but not stop-loss, which is the honest position for
a case nobody has triaged.
POST scope answers which cases a change needs to run. Every rule resolves towards
including, because the two failure directions are not symmetrical: a case wrongly
included costs tokens and is obvious, while one wrongly excluded produces no result
at all, and "not run" is indistinguishable from "passed" once the numbers are in a
report. Unknown involved agents therefore fail open, and both halves of the decision
are returned with the rule that produced them.
InvolvedAgents falls back to the case entry agent when unauthored, which is
definitionally involved and already known -- so an Agent case is picked up by a
change to its own agent without anyone maintaining a list.
Latency, tokens and cost
------------------------
Each turn is timed around the agent call alone, and the case reports that separately
from its wall clock, which also contains the canary and the conversation reads. The
run summarises P50 and P95 per model at completion, nearest-rank so every figure is
a duration some case actually took. Cases that never reached the model are excluded
from the percentiles -- otherwise a run that mostly crashed reports the best latency
on record -- but still counted in tokens and cost, which they really did spend.
Token usage is read as a delta across the case rather than as an absolute, so a
reused scope cannot bill one case for another tokens, and it is read in a finally
block so a timed-out case still reports what it cost. Total only: the input/output
split lives in TokenStatistics private fields and is not reachable through
ITokenStatistics.
The run also snapshots each model configured unit costs. A cost figure is not
comparable with another run without them, and recording only a version string
would leave nobody able to check whether two versions differ.
Rate limiting
-------------
BotSharp rate limiting counts human behaviour, and a regression suite does not
look like one: it opens a conversation per case per model, drives turns as fast as
the model answers, and runs in a BackgroundService whose user identity is empty --
which, because the Mongo filter drops an empty UserId rather than matching on it,
measured the harness against every conversation in the instance. Every case failed
with a message about conversation quotas that said nothing about the agent.
ISyntheticConversationProbe (new, in Abstraction) lets a harness declare which
conversations are its own, and RateLimitConversationHook stands aside for those on
its two volume guards. The plugin answers from the run registry rather than from the
conversation tag, because the tag is written only after the first message has
already passed the hook. The input-length guard still applies: that one is about a
single message being too large, which a test should surface rather than be excused
from. A probe that throws fails closed, so a bug there cannot lift the limits for
real traffic.
Tests
-----
336 unit tests, up from 194. The ones worth reviewing are CaseScopeTests, which pins
every narrowing rule in the direction of including, and
SyntheticConversationExemptionTests, which checks that real traffic is still limited
in every case where the harness is not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Runs accumulate forever -- one per case per model per run, and nothing ever removed
them. Adds a way to clear them, with the guards the operation needs.
POST runs/delete takes a list. Bulk-only, and the single-row button calls it with one
id: a separate DELETE runs/{id} would be a second path to the same destructive
operation, and the guard below is worth having in exactly one place.
Deleting a run deletes its case results too. Results are keyed by run id and reachable
no other way, so dropping the run alone would leave rows nothing can ever list, read or
clean up again -- and every later aggregate that scans results would keep counting them.
Results go first, so a process death between the two deletes leaves an orphaned RUN,
which is visible and deletable again, rather than orphaned RESULTS, which are not
reachable at all.
A run that is still executing is refused. Deleting it would not stop it: the queue keeps
driving cases, keeps spending tokens, and keeps writing results for a run id that no
longer exists. Cancel it first -- and on a live row the UI offers only Cancel, since the
other button would not do what it says.
One live run does not fail the batch. Selecting everything and clearing is the normal way
this gets used, and a running row in the list is common; refusing the whole call would
make the feature useless exactly when it is most wanted. Skipped runs come back with a
reason and the UI shows each one, because "deleted 2" while a third row silently stays is
how someone concludes the button is broken. The count on the button excludes running
runs, so it never promises a delete that will not happen.
Behind the same admin gate as triggering a run: runs are the record of whether an agent
change was evaluated at all, so removing them is at least as consequential as creating
them.
An empty list is a 400 rather than a successful no-op -- far more likely a select-all
that selected nothing than a deliberate request.
Not touched: the conversations these runs created. They live in BotSharp's own store and
are the only forensic record of what an agent actually said when an assertion failed, so
they are not something to remove as a side effect of tidying a list. The harness still
never cleans them up, which is a separate decision to make.
8 tests: the cascade, the running-run refusal, one live run not blocking a batch, an
already-deleted run being reported rather than erroring, duplicate ids deleting once, two
shapes of empty request, and the admin gate. 344 unit tests pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI failed to compile tests/UnitTest: TestHookA, TestHookB and TestHookC do not implement
ConversationHookBase.SelfId.
Not caused by this branch. master added `public abstract string SelfId { get; }` to
ConversationHookBase when it started implementing IHookBase, and updated every
implementation in the tree except those three -- the same break is present on
SciSharp/BotSharp master right now, so anything merged into it fails the same way. This
branch only surfaced it, because CI builds the merge.
SelfId is string.Empty for all three, and that is required rather than conventional:
IsMatch is IsNullOrEmpty(SelfId) || SelfId == agentId, and the test resolves hooks with
GetHooksOrderByPriority(string.Empty) then asserts all three come back. Any non-empty
value would match nothing and fail the count assertion.
The merge also brought two changes to RateLimitConversationHook, which this branch edits:
master added its own SelfId and moved the conversation id to
IConversationStateService.GetConversationId(), having previously resolved
IConversationStorage up front. Git merged the file cleanly, and both sides survived --
checked rather than assumed, since a clean merge is exactly how the scope panel ended up
half-restyled in the UI repo. The synthetic-conversation check now takes that same
conversation id as an argument instead of resolving IConversationService for itself, so
the hook reads from one source.
Verified: the whole BotSharp solution builds with zero compiler errors, tests/UnitTest
passes 4, and BotSharp.Core.UnitTests passes 344. Building only the projects this branch
touches is what let the original failure through, so the solution build is the check that
matters here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixed the issue of obtaining CurrentAgentId using SetState in a Hook.
Added BotSharp.Plugin.FloxiAI project with core classes for FloxiAI integration, including provider constants, DI registration, chat transport interface and implementation, and chat completion provider with streaming and tool call support. Registered the plugin in appsettings.json and WebStarter.csproj. Improved error handling in CompletionProvider.cs for unregistered providers.
…uthor) Lets a QA/PM write or edit a test case by chat instead of hand-writing turns, mocks and assertions. Stateless on the server -- the whole authoring conversation and the current draft travel with each request -- and it never saves: the draft still goes through the same create/update endpoints and the same CaseValidation those already ran, now shared instead of duplicated. Four guards on what a model may do to someone's draft: only fields it declares in changedFields are taken from its answer (an omitted field is kept, never deleted); a mock or toolCalled/toolNotCalled naming a function the agent cannot call is dropped rather than stored; a draft that fails validation gets one repair round against the real error text, and if that still fails the original draft comes back untouched; the change list shown to the user is diffed from the two drafts, never read off the model's own account. Also fixes a real failure: models routinely write argsMatchJson/resultContent/ a state value as a nested object instead of a JSON-string, which is valid JSON overall and so slipped past the existing "is this JSON" check and only broke at strong-typed deserialization. Normalises that shape deterministically before parsing, and separately gives a genuinely unparseable reply one retry (the validation-repair round already had one; this one didn't). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Agent test harness: routing cases, authored history, scope narrowing,…
GET /agent-test/mock-targets enumerated Functions, SecondaryFunctions and McpTools only, so an agent whose whole toolset comes from utilities -- Lessen Work Order Summary, Property Summary -- reported no callable functions at all. Utilities are expanded into SecondaryFunctions by BasicAgentHook.OnAgentUtilityLoaded at conversation time, and IAgentService.GetAgent, which is what every caller here holds, is a plain repository read that never runs that hook. Cost of the gap: a case authored against such an agent blocks its own tools on the first run, and the case editor's tool picker has nothing to offer. The filter mirrors the hook -- a disabled utility is off, only a `util-` prefixed name is ever loaded as a function, and the real FunctionDef is read off UtilityAssistant rather than the agent's own declaration. VisibilityExpression is deliberately not mirrored: it needs the conversation's render data and cannot be evaluated against a stored agent, so a conditionally-visible utility is offered and may turn out not to load. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The headers came straight from MCP:McpServerConfigs, so every call to a server went out as whatever fixed credential was configured for it. A host that wants to call a server as the user driving the conversation had no way in: GetMcpClientAsync is not virtual, and the transport is built inline. IMcpClientHeaderProvider is an optional hook, resolved with GetService and asked about every server. Nothing registers it by default, and an implementation is free to answer with what it was given, so a host without one -- or with one that does not recognise a server -- gets the configured headers back untouched. The headers passed to the provider belong to the McpSettings instance captured for the lifetime of the process, which is why the contract asks implementations to copy rather than write in place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r-provider Let the host adjust the headers an MCP connection is opened with
GetMcpClientAsync built a fresh transport and a fresh McpClient on every call, and Dispose did nothing, so a turn that listed a server's tools and then called three of them opened four connections and closed none of them. Pooling a connection means reusing whatever headers IMcpClientHeaderProvider answered with, and that is an identity. The pool is therefore an instance field of this class, which is registered per DI scope -- one HTTP request, one crontab run, one queued message -- so everything sharing a pool is already the same caller, and one user's connection cannot be handed to another. That is structural rather than a rule someone has to remember. The pool key also folds in a SHA-256 of the headers a connection opens with, so the guarantee survives this class later being registered with a longer lifetime: two credentials land on two entries even inside one pool. It is a hash of secrets, so it is never logged, and a test pins that down along with the three identities OneBrainMcpHeaderProvider can answer with never sharing an entry. Headers are now resolved once and handed to both the key and the transport. Resolving separately for each let the two disagree, and the key is the thing keeping one caller's connection away from another. Entries hold Lazy<Task<McpClient?>> so concurrent callers wanting the same server open one connection between them rather than one each. A failed connection is removed rather than cached, and McpToolExecutor now drops the pooled client when a call fails: keeping a dead one fails every remaining call in the scope, while discarding a live one costs a single reconnect. McpClient only implements IAsyncDisposable, so the manager implements both disposal interfaces. Async scopes get DisposeAsync; scopes created with CreateScope tear down synchronously and get a bounded wait instead, because a wedged transport must not hang the unit of work that is trying to finish. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pooling MCP clients, as the previous commit did, shares more than a socket. A client is a session: CreateAsync performs the initialize handshake, the server answers with a session id, and subscriptions and long-running tool tasks (ListTasksAsync, GetTaskResultAsync) live on it. Two callers on one session would see each other's tasks, and no per-request header can undo that, because it is server-side state rather than an authorization question. With IMcpClientHeaderProvider opening connections as the signed-in user, sharing a session would mean sharing an identity as well. So sessions are not shared at all now: every GetMcpClientAsync call opens its own and the caller owns it. The three call sites hold it in an await using, which closes the session on the server instead of leaving it to time out -- the leak the empty Dispose used to cause, and the reason the pool existed. What is shared instead is the layer that carries no identity. The HttpClient comes from IHttpClientFactory, named per server, so connections to one server reuse a pooled HttpMessageHandler. CreateClient hands back a fresh HttpClient each time, so one caller's headers are never seen by another. Building the transport with its own HttpClient, as this did before, gave every connection a private handler and therefore a private socket pool -- the usual way to exhaust sockets and to keep talking to an address DNS has already moved. AddBotSharpMCP now calls AddHttpClient so the factory it depends on is present. The call is idempotent, and a host that already registered one is unaffected. Timeout is left at the factory default. No configured tool is expected to run for 100 seconds, but that cap is one the SDK's own client may not have had, so a comment records the symptom and the one-line fix should a server keep a GET open for the length of its session. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Agent test: count utilities as mock targets
allow execute rules for each agent
feat: guard agents against prompt injection
Each triggered rule now runs in its own DI scope, so the scoped conversation, state and routing services start clean per run and no longer bleed between rules or into the caller's scope. The message hub observers are subscribed per run so rule-triggered conversations emit the same events as user-initiated ones. The nested agent/rule loops are flattened into a single list and dispatched via Parallel.ForEachAsync, throttled by MaxConcurrency (options -> RuleSettings -> built-in default of 5). Results are written into an indexed array so conversation ids keep rule order without a concurrent-add race. Triggered also takes a CancellationToken: it stops dispatching new rules, interrupts the inter-rule delay, and propagates to the caller. Per-rule failures are logged and isolated so one bad rule does not take down the others. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A cancelled run cannot both return its conversation ids and surface the cancellation, so the ids now ride along on the exception. Triggered throws RuleTriggerCanceledException, which derives from OperationCanceledException (existing handlers keep working) and exposes the conversations that were already started, so callers can still act on work that cannot be undone. Also drops the CancellationToken from the rule trigger endpoint, so a client disconnect no longer stops rules that are mid-dispatch. The token stays optional on IRuleEngine.Triggered for other callers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drops Parallel.ForEachAsync in favour of a plain loop over the rules. Conversation ids are appended as each rule finishes, so the indexed array and its collect helper are no longer needed to keep them in order. MaxConcurrency only existed to throttle the parallel run, so it goes too, along with the RuleSettings class and its "Rule" config binding that were added to configure it. The inter-rule delay falls back to the per-call option and then the built-in default. Per-rule service scopes, cancellation and per-rule error isolation are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A model routinely asks for several independent tools at once. Both providers kept FirstOrDefault of that set and dropped the rest, so the calls it did not get an answer for came back on the next turn -- the same lookups, run again, one per round trip. RoleDialogModel.ToolCalls now carries the whole set, in the order the model produced it. The single FunctionName, FunctionArgs and ToolCallId fields beside it are the first entry, computed from the same ordered list they were before, so a caller that can only run one call -- the routing engine, every agent on it -- sees exactly what it saw. From deliberately does not copy ToolCalls: it describes one model reply, and a message derived from that reply is not it. The streaming path had a second problem behind the first. Argument fragments arrive chunked and were concatenated into a single string across all calls, which is correct while there is one call and produces one malformed blob as soon as there are two. They are now accumulated per call, keyed by the tool call id that is present when a call opens, since the SDK update carries no index. The first call therefore has valid arguments where it used to have garbage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An agent that writes a sentence before calling a tool is saying something worth keeping -- it is the reasoning behind the call -- but it is not a message to the user, and rendering it would read as a half answer followed by a real one. MessageTypeName.Internal marks such a message. It is stored, read back into the model context like any other, and skipped when the dialog endpoint renders a conversation. Nothing in either repository produced this type before, so every message already in storage renders exactly as it did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…llel-execution Feature/rule engine parallel execution
Improve Twilio API error handling in outbound calls Added `Twilio.Exceptions` namespace and introduced a constant for geo-permission error code `21215`. Wrapped `CallResource.CreateAsync` in a `try-catch` block to handle `ApiException` errors. Specifically, added logging and messaging for geo-permission restrictions to enhance error handling and user feedback.
Fix "Twilio.Exceptions.ApiException: Account not allowed to call"
Listing tools costs a session of its own -- a handshake, a notification and a stream, three or four HTTP round trips -- and every agent load paid it again for a list that changes when a server is redeployed, not between two messages of one conversation. Measured against a remote server it was 0.3 to 0.5 seconds of every turn, before the model had been asked anything. The listing is now reused for McpSettings.ToolListCacheSeconds, sixty by default: short enough that a tool added upstream shows up while someone is still testing it, long enough that no conversation pays for the listing twice. Zero restores the old behaviour. The entry is keyed by the headers the connection would carry rather than by the server alone. IMcpClientHeaderProvider lets a host open the connection as the signed-in user, so a server that shows one caller a different set of tools than another must never be served one caller from the other one's entry. The headers are fingerprinted, so no credential ends up in a cache key. Two things are deliberately not cached. A failed or empty listing is not, because a server that is briefly unreachable would otherwise leave every agent that depends on it answering from its prompt alone -- with no tools and no error -- for the length of the window. And callers get their own FunctionDef instances over the shared parameter schemas, so an agent that rewrites a description on the way to the model cannot rewrite it for every other agent on the same server. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r-provider Feature/mcp client header provider
Handle multiple Twilio error codes in call handling Replaced single error code constant with a HashSet to handle multiple Twilio error codes (21215, 21216). Updated the `catch` block to check for error codes in the set. Added `message.StopCompletion` to stop further execution on error. Improved comments for clarity.
GTR-13432 (Handle multiple Twilio error codes in call handling)
Contributor
PR Summary by QodoExpand agent evaluation, add Floxi AI, and harden runtime integrations
AI Description
Diagram
High-Level Assessment
Files changed (72)
|
Contributor
Code Review by Qodo
1. MCP sessions timeout prematurely
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+232
to
+235
| // Timeout is left at the factory default (100s) deliberately: no configured tool is | ||
| // expected to run that long. Note this is a cap the SDK's own HttpClient may not have | ||
| // had, so it arrived with this change -- a server whose transport keeps a GET open for | ||
| // the session (SSE, or streamable HTTP with a standalone listening stream) would be cut |
Contributor
There was a problem hiding this comment.
1. Mcp sessions timeout prematurely 🐞 Bug ☼ Reliability
CreateHttpTransport uses a factory-created HttpClient without removing its default 100-second timeout, causing long-lived SSE and streamable-HTTP listening requests to be canceled after 100 seconds. This can terminate otherwise healthy MCP sessions and cause subsequent discovery and tool operations to fail regardless of the configured transport timeout.
Agent Prompt
## Issue description
MCP transports use an `IHttpClientFactory` client that retains the default 100-second timeout, causing long-lived SSE and streamable-HTTP listening requests to be canceled after 100 seconds. Update the HTTP client configuration so this lifetime timeout does not terminate otherwise healthy MCP sessions.
## Issue Context
Both HTTP and SSE server configurations use this transport. The transport already has its own configured connection timeout and caller cancellation behavior, and the existing comment identifies that a persistent session listening stream can exceed the inherited HTTP client timeout.
## Fix Focus Areas
- src/Infrastructure/BotSharp.Core/MCP/Managers/McpClientManager.cs[227-239]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+101
to
+105
| // The public network is the default, so a model that only carries an ApiKey still works; an | ||
| // Endpoint is only needed to point at a different deployment. | ||
| var endpoint = FloxiAiConstants.DefaultEndpoint; | ||
|
|
||
| var request = new HttpRequestMessage(HttpMethod.Post, BuildChatUrl(endpoint)) |
Contributor
There was a problem hiding this comment.
2. Floxi endpoint configuration ignored 🐞 Bug ⛨ Security
HttpFloxiChatTransport.BuildRequest loads the selected model’s settings but ignores its configured Endpoint and always posts to FloxiAiConstants.DefaultEndpoint. Private or custom OpenAI-compatible deployments therefore cannot be reached, and their bearer API keys may be sent to the unintended public Floxi service.
Agent Prompt
## Issue description
The Floxi HTTP transport loads the selected model settings but ignores `LlmModelSetting.Endpoint`, always selecting the public `FloxiAiConstants.DefaultEndpoint`. This prevents private or custom OpenAI-compatible deployments from being contacted and may disclose their bearer credentials to the wrong host.
## Issue Context
The transport contract documents a per-model configured endpoint with a default fallback. Use the configured endpoint when it is nonblank, retain `FloxiAiConstants.DefaultEndpoint` only as the fallback, and validate the resulting absolute HTTP(S) URI before attaching credentials.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.FloxiAI/Providers/HttpFloxiChatTransport.cs[96-115]
- src/Plugins/BotSharp.Plugin.FloxiAI/Providers/HttpFloxiChatTransport.cs[118-127]
- src/Infrastructure/BotSharp.Abstraction/MLTasks/Settings/LlmModelSetting.cs[31-33]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+41
to
+42
| ILogger<AgentTestRunExecutor> logger, | ||
| ILlmProviderService? llmProviders = null) |
Contributor
There was a problem hiding this comment.
3. Pricing snapshots always omitted 🐞 Bug ≡ Correctness
The production queue manually constructs AgentTestRunExecutor without the newly optional ILlmProviderService, so SnapshotPricing always returns an empty list. Completed queued runs therefore never persist the model pricing required to interpret their cost summaries.
Agent Prompt
## Issue description
Production queued runs omit `ILlmProviderService` when manually constructing `AgentTestRunExecutor`. The new pricing snapshot feature therefore silently records no prices.
## Issue Context
Resolve `ILlmProviderService` from the queue's execution scope and pass it into the executor, or resolve the executor through DI while retaining the scoped case-runner behavior.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.AgentTesting/Services/AgentTestRunExecutor.cs[38-48]
- src/Plugins/BotSharp.Plugin.AgentTesting/Services/AgentTestRunExecutor.cs[335-367]
- src/Plugins/BotSharp.Plugin.AgentTesting/Services/AgentTestRunQueue.cs[103-112]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+84
to
+87
| /// <see cref="FunctionName"/>, <see cref="FunctionArgs"/> and <see cref="ToolCallId"/> beside | ||
| /// this are the first entry, so a caller that can only run one call keeps working unchanged; | ||
| /// a caller that can run several reads this instead. The one difference is name repair: the | ||
| /// single field carries the normalized name it always has, while entries here keep the name |
Contributor
There was a problem hiding this comment.
4. Parallel tool calls ignored 🐞 Bug ≡ Correctness
OpenAI and Anthropic now populate RoleDialogModel.ToolCalls, but RoutingService.InvokeAgent copies and invokes only the legacy first-call fields. A response requesting multiple independent tools therefore executes only its first call and silently loses the remainder.
Agent Prompt
## Issue description
Providers now preserve every model-requested tool call in `RoleDialogModel.ToolCalls`, but the routing pipeline still invokes only `FunctionName`, `FunctionArgs`, and `ToolCallId` from the first call.
## Issue Context
Execute all calls and retain the assistant tool-call request plus one correctly identified result per call when constructing the follow-up model context. Preserve compatibility for providers that only populate the scalar fields.
## Fix Focus Areas
- src/Infrastructure/BotSharp.Core/Routing/RoutingService.InvokeAgent.cs[51-65]
- src/Infrastructure/BotSharp.Core/Routing/RoutingService.InvokeAgent.cs[89-130]
- src/Infrastructure/BotSharp.Abstraction/Conversations/Models/RoleDialogModel.cs[79-94]
- src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.Chat.cs[38-49]
- src/Plugins/BotSharp.Plugin.AnthropicAI/Providers/ChatCompletionProvider.cs[55-68]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+72
to
+79
| if (completion.ToolCallName != null) | ||
| { | ||
| responseMessage = new RoleDialogModel(AgentRole.Function, completion.Text) | ||
| { | ||
| CurrentAgentId = agent.Id, | ||
| MessageId = conversations.LastOrDefault()?.MessageId ?? string.Empty, | ||
| ToolCallId = completion.ToolCallId, | ||
| FunctionName = completion.ToolCallName.NormalizeFunctionName(), |
Contributor
There was a problem hiding this comment.
5. Floxi drops parallel calls 🐞 Bug ≡ Correctness
Floxi parses and accumulates only tool_calls[0] and never populates RoleDialogModel.ToolCalls. Additional calls are discarded before routing, so they remain unrecoverable even after the shared execution pipeline supports multiple calls.
Agent Prompt
## Issue description
The new Floxi provider reads only the first tool call in both complete and streamed responses. All additional calls are silently discarded.
## Issue Context
Parse every non-streaming call into `RoleDialogModel.ToolCalls`. For streams, maintain separate accumulators keyed by call index or ID, while continuing to populate the legacy scalar fields from the first call.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.FloxiAI/Providers/Chat/ChatCompletionProvider.cs[72-88]
- src/Plugins/BotSharp.Plugin.FloxiAI/Providers/Chat/ChatCompletionProvider.cs[623-644]
- src/Plugins/BotSharp.Plugin.FloxiAI/Providers/Chat/ChatCompletionProvider.cs[733-774]
- src/Plugins/BotSharp.Plugin.FloxiAI/Providers/Chat/ChatCompletionProvider.cs[279-296]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+120
to
+124
| // Pause before the next rule, so a large batch does not hammer the downstream provider. | ||
| var delay = options?.SendMessageDelayMs ?? RuleTriggerOptions.DefaultSendMessageDelayMs; | ||
| if (delay > 0) | ||
| { | ||
| await Task.Delay(delay, cancellationToken); |
Contributor
There was a problem hiding this comment.
6. Cancellation omits created conversation 🐞 Bug ≡ Correctness
RunRule creates and sends to a conversation before awaiting the new cancelable delay, while Triggered records its ID only after RunRule returns. Cancellation during that delay throws RuleTriggerCanceledException without the conversation that was already started, so callers cannot account for or clean up every created conversation.
Agent Prompt
## Issue description
A cancellation during the inter-rule delay prevents an already-created conversation ID from being included in `RuleTriggerCanceledException.ConversationIds`.
## Issue Context
`SendMessageToAgent` returns only after it has created and sent the conversation. Its ID is added to the outer list only after `RunRule` returns, but the new delay can throw first.
## Fix Focus Areas
- src/Infrastructure/BotSharp.Core.Rules/Engines/RuleEngine.cs[42-58]
- src/Infrastructure/BotSharp.Core.Rules/Engines/RuleEngine.cs[117-127]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
6 participants
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.
No description provided.