feat(agentic): Phase 2 — multi-agent orchestration (epic #1544)#1551
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Contributor
|
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:
WalkthroughAdds agent contracts and run results, a tool-calling AgentExecutor, AgentAsTool adapter, Supervisor and Swarm coordinators, long-term memory and conversation stores (in-memory/JSON/SQLite), wrappers (memory-augmented/threaded), connector/tool validation hardening, generator/API adjustments, and comprehensive unit tests. ChangesAgentic agent orchestration and memory runtime
Sequence Diagram(s)sequenceDiagram
participant User as User/Caller
participant Supervisor as SupervisorAgent
participant Coordinator as Coordinator IChatClient
participant ToolLayer as ToolCollection (AgentAsTool)
participant Worker as Worker IAgent
User->>Supervisor: RunAsync(request)
Supervisor->>Supervisor: Wrap workers as AgentAsTool
Supervisor->>Coordinator: GetResponseAsync(transcript, options)
Coordinator-->>Supervisor: assistant response (maybe tool call)
alt Coordinator requests handoff tool
Supervisor->>ToolLayer: InvokeToToolMessageAsync(handoff)
ToolLayer->>Worker: InvokeAsync(task)
Worker-->>ToolLayer: ToolInvocationResult (FinalText)
ToolLayer-->>Supervisor: ChatMessage.Tool appended
Supervisor->>Coordinator: GetResponseAsync(updated transcript, options)
else No handoff
Supervisor-->>User: AgentRunResult.Finished
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
Deployment failed with the following error: Learn More: https://vercel.com/franklins-projects-02a0b5a0?upgradeToPro=build-rate-limit |
This was referenced Jun 9, 2026
ooples
changed the title
Base automatically changed from
feature/agentic-phase0-model-abstractions
to
master
June 11, 2026 12:13
ooples
force-pushed
the
feature/agentic-phase2-multi-agent
branch
from
072ed9e to
978191c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/Models/Results/AiModelResult.cs (1)
1441-1460:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the public XML docs to match the new chat-client contract.
These methods now take
IChatClient<T>, but the docs still omit that requirement in places and still tell users to set up agent assistance / agent configuration. Generated API docs will point callers at removed setup and document the wrong failure mode. Update theparam,exception, and beginner guidance so they describe supplying a chat client instead.Also applies to: 5920-5945, 6562-6736
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Models/Results/AiModelResult.cs` around lines 1441 - 1460, Update the XML doc for EvaluateBenchmarksAsync to reflect the new chat-client contract: state that callers must supply an IChatClient<T> (or null to use defaults), update the <param name="chatClient"> description to explain how to provide a chat client instance instead of agent configuration, remove any guidance about agent assistance/agent setup, and adjust <exception> tags to describe failures originating from missing/invalid chat client or BenchmarkRunner.RunAsync rather than agent misconfiguration; apply the same doc changes to the other affected methods referenced (lines 5920-5945 and 6562-6736).src/Reasoning/Training/ReinforcementLearner.cs (1)
247-247:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBLOCKING:
Console.WriteLineused for logging throughout training pipeline.Per project guidelines, using
Console.WriteLinefor logging instead of proper logging abstractions is a non-production pattern requiring immediate fix. This class already hasOnEpochCompleteandOnBatchCompleteevents - all progress output should flow through those or through an injectedILogger.Hardcoded console output:
- Line 247:
Console.WriteLine($"\n=== Epoch {epoch + 1}/{_config.Epochs} ===")- Line 260:
Console.WriteLine($"Validation Accuracy: ...")- Line 285-286: Early stopping message
- Lines 317, 331-332, 346: STaR training progress
- Line 424: Batch progress
Production-ready code should:
- Use the existing event infrastructure (
OnEpochComplete,OnBatchComplete) for all progress reporting, or- Accept an
ILogger<ReinforcementLearner<T>>dependency for structured logging, or- Remove console output entirely since events already exist
🐛 Suggested fix: Replace Console.WriteLine with event-based reporting
- Console.WriteLine($"\n=== Epoch {epoch + 1}/{_config.Epochs} ==="); + // Progress reported via OnEpochComplete event - Console.WriteLine($"Validation Accuracy: {evalMetrics.Accuracy:P2}"); + // Validation metrics included in epochMetrics, reported via OnEpochComplete - Console.WriteLine($"Early stopping at epoch {epoch + 1}"); + // Early stopping condition - caller can detect via epoch count in resultsAlso applies to: 260-260, 285-286, 317-317, 324-324, 331-332, 346-346, 424-424
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Reasoning/Training/ReinforcementLearner.cs` at line 247, Replace hardcoded Console.WriteLine calls in ReinforcementLearner<T> with event-based or injected-logger reporting: locate the epoch/batch loop where Console.WriteLine($"\n=== Epoch {epoch + 1}/{_config.Epochs} ==="), validation/early-stopping, STaR progress, and batch progress are printed and emit those messages via the existing OnEpochComplete and OnBatchComplete events (populate event args with epoch index, metrics, and progress text) or send them to an injected ILogger<ReinforcementLearner<T>> instance; remove all Console.WriteLine usages and ensure subscribers can consume the same information through the events or logger so behavior remains functionally equivalent without direct console output.Source: Coding guidelines
src/Reasoning/Strategies/ChainOfThoughtStrategy.cs (1)
148-158:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBLOCKING: TODO comment and placeholder verification implementation.
This is a blocking issue per project guidelines. The code:
- Contains a
// TODO: Implement verificationcomment- Unconditionally marks all steps as
IsVerified = truewithout performing actual verificationThis is non-production-ready code that actively lies about the verification status of reasoning steps. Users relying on
IsVerifiedwill get false confidence in unverified reasoning.Production-ready implementation should either:
- Actually integrate with
CriticModel<T>(which exists in this PR) to perform real verification, or- Remove the verification block entirely and leave
IsVerifiedasfalse, or- Throw
NotSupportedExceptionwhenEnableVerificationis true until the feature is implemented🐛 Suggested fix: Integrate with CriticModel or disable feature
// Step 3: Optionally verify steps if (config.EnableVerification && parsedSteps.Count > 0) { AppendTrace("Verification enabled - checking reasoning quality..."); - // TODO: Implement verification when ICriticModel concrete implementation is available - // For now, mark all steps as verified with high confidence - foreach (var step in chain.Steps) - { - step.IsVerified = true; - } + // Verification requires ICriticModel injection - not yet supported in this strategy + AppendTrace("WARNING: Verification requested but not implemented - steps remain unverified"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Reasoning/Strategies/ChainOfThoughtStrategy.cs` around lines 148 - 158, The verification branch in ChainOfThoughtStrategy currently contains a TODO and falsely marks every step IsVerified = true; replace this placeholder by either invoking the CriticModel<T> / ICriticModel implementation to perform real checks on parsedSteps and set step.IsVerified and confidence based on the model's result, or if the critic integration isn't ready, remove the blanket assignment and throw a NotSupportedException (or set EnableVerification to false) when config.EnableVerification is true so callers are not misled; update the logic around config.EnableVerification, parsedSteps, and chain.Steps to reflect the chosen behavior and ensure any thrown exception mentions verification is not implemented yet.Source: Coding guidelines
src/Reasoning/Verification/OutcomeRewardModel.cs (1)
358-361:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBlocking: Replace the empty catch-all in semantic-similarity scoring.
Line 358 currently swallows all exceptions and silently falls back to
0.0. This hides external-call failures and makes cancellation/error behavior non-deterministic in production.
Production-ready behavior should rethrow cancellation and only fallback on explicitly handled transient failures.Proposed fix
- catch - { - // Fallback to simple comparison - } + catch (OperationCanceledException) + { + throw; + } + catch (Exception ex) when ( + ex is System.Net.Http.HttpRequestException || + ex is TimeoutException) + { + // Fallback to simple comparison for transient provider failures. + return 0.0; + }As per coding guidelines, catch blocks that swallow exceptions without logging are a blocking non-production pattern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Reasoning/Verification/OutcomeRewardModel.cs` around lines 358 - 361, The empty catch-all in the semantic-similarity scoring within the OutcomeRewardModel class must be replaced: remove the bare catch and instead rethrow cancellation (catch and rethrow OperationCanceledException/TaskCanceledException), then catch only explicit transient failures (e.g., HttpRequestException, TimeoutException or other known transient exceptions from the semantic similarity call), log the exception details, and return the 0.0 fallback only for those handled transient cases; any other exceptions should be rethrown. Ensure logging uses the existing logger in OutcomeRewardModel and update the semantic-similarity scoring method's catch logic accordingly.Source: Coding guidelines
src/Reasoning/Verification/SelfRefinementEngine.cs (1)
75-75:⚠️ Potential issue | 🟠 MajorPropagate cancellation into SelfRefinementEngine’s LLM call (src/Reasoning/Verification/SelfRefinementEngine.cs ~75).
RefineStepAsyncaccepts acancellationToken, but_chatModel.GenerateResponseAsync(prompt)doesn’t forward it. AiDotNet already provides a cancellation-token overload (ChatClientExtensions.GenerateResponseAsync(..., CancellationToken)) that ultimately passes the token toGetResponseAsync, so this is the outlier.Proposed fix
- string refinedContent = await _chatModel.GenerateResponseAsync(prompt); + cancellationToken.ThrowIfCancellationRequested(); + string refinedContent = await _chatModel.GenerateResponseAsync(prompt, cancellationToken);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Reasoning/Verification/SelfRefinementEngine.cs` at line 75, RefineStepAsync currently calls _chatModel.GenerateResponseAsync(prompt) without the cancellation token; change that call to the cancellation-aware overload (use ChatClientExtensions.GenerateResponseAsync or the GenerateResponseAsync overload that accepts a CancellationToken) so RefineStepAsync forwards its cancellationToken into the LLM call, ensuring the token flows through to GetResponseAsync; update the invocation in SelfRefinementEngine.RefineStepAsync to pass the cancellationToken parameter.Source: Coding guidelines
src/Agentic/Tools/DelegateAgentTool.cs (1)
1-157: 🧹 Nitpick | 🔵 TrivialMake
DelegateAgentToolnon-public and route callers through the facade
DelegateAgentToolispublic sealedwith public constructors, but the library already exposesAgentToolFactory.FromDelegate/ScanInstancereturningIAgentTool; keeping this type public unnecessarily expands the API surface.- Unit tests currently use
new DelegateAgentTool(...), so switching tointernal sealedwill require updatingtests/AiDotNet.Tests/UnitTests/Agentic/Tools/AgentToolTests.csto create tools viaAgentToolFactory.FromDelegate(name, description, delegate)instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Tools/DelegateAgentTool.cs` around lines 1 - 157, Change DelegateAgentTool from public sealed to internal sealed and make its constructors internal so the concrete type is not part of the public API; update callers (notably unit tests) to obtain tools via the public factory methods AgentToolFactory.FromDelegate or AgentToolFactory.ScanInstance instead of using new DelegateAgentTool(...). Ensure all references to the public constructors/members are replaced with the factory calls (search for symbol DelegateAgentTool and its constructors) and run tests to fix any compile errors from the accessibility change.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Agentic/Agents/SupervisorAgent.cs`:
- Around line 62-69: Replace the verbose null/empty checks on settings.Name,
settings.Description and settings.SystemPrompt with string.IsNullOrWhiteSpace
for clarity: update the ternaries that currently use patterns like
"settings.Name is { } name && name.Trim().Length > 0" to use
"!string.IsNullOrWhiteSpace(settings.Name)" (and similarly for Description and
SystemPrompt) while preserving the fallback values (e.g., "supervisor" and
BuildDefaultRoutingPrompt(workers)); ensure you reference the same properties
(settings.Name, settings.Description, settings.SystemPrompt) and keep the
existing assignment targets (Name, Description, systemPrompt).
In `@src/Agentic/Memory/EmbeddingAgentMemoryStore.cs`:
- Around line 103-110: In GetAllAsync add a cancellation check before doing
work: call cancellationToken.ThrowIfCancellationRequested() at the start of the
method (before acquiring/inside the lock) so the synchronous in-memory path
respects cancellation; keep the existing lock(_gate) and return
Task.FromResult(all) behavior after the check.
- Line 86: The call to _embeddingModel.EmbedAsync(query) in
EmbeddingAgentMemoryStore is missing the cancellationToken — update the call
that assigns queryVector to pass the existing cancellationToken (matching the
pattern used earlier at line 56) so the embedding operation can be cancelled;
specifically modify the _embeddingModel.EmbedAsync invocation to include the
cancellationToken parameter so the method respects cancellation.
- Around line 113-123: The RemoveAsync method doesn't honor the
CancellationToken on the synchronous path; before acquiring _gate and calling
_entries.RemoveAll in RemoveAsync(string id, CancellationToken cancellationToken
= default) you should check the token (e.g., call
cancellationToken.ThrowIfCancellationRequested()) so cancellation is observed
prior to performing the removal operation, mirroring the pattern used in
GetAllAsync.
- Line 56: EmbeddingAgentMemoryStore<T> currently ignores the CancellationToken
passed into AddAsync, SearchAsync, GetAllAsync and RemoveAsync and cannot
forward it to _embeddingModel.EmbedAsync (which lacks a token); add
cancellationToken.ThrowIfCancellationRequested() at the start of each of those
methods to honor early cancellation, and inside SearchAsync also check
cancellationToken periodically (e.g., inside the scoring/loop that computes
similarity scores) to abort long-running processing promptly; keep using
_embeddingModel.EmbedAsync(content) as-is since it has no token parameter but
ensure you check ThrowIfCancellationRequested() immediately before and after
calls that may be lengthy.
In `@src/Agentic/Memory/InMemoryAgentMemoryStore.cs`:
- Around line 22-37: Add cooperative cancellation checks to the in-memory async
methods by calling cancellationToken.ThrowIfCancellationRequested() at the start
of AddAsync, SearchAsync, GetAllAsync, and RemoveAsync (before any work or
locking on _gate) so the methods honor the provided CancellationToken; ensure
the call is the very first statement in each method so cancellation is observed
consistently.
In `@src/Agentic/Memory/InMemoryConversationStore.cs`:
- Around line 24-45: Add cancellation checks at the start of each public async
method (AppendAsync, GetAsync, ListThreadsAsync, ClearAsync) by calling
cancellationToken.ThrowIfCancellationRequested() before acquiring the _gate lock
or touching _threads; this ensures the method honors the provided
CancellationToken and avoids doing work when cancellation has been requested.
In `@src/Agentic/Memory/JsonFileConversationStore.cs`:
- Around line 43-47: The JsonFileConversationStore constructor currently trusts
the incoming filePath, enabling path traversal; modify
JsonFileConversationStore(string filePath) to canonicalize and validate the
path: define a fixed base directory (e.g., a configured conversations
directory), resolve the candidate full path via Path.GetFullPath(filePath) and
ensure it starts with the base directory, or alternatively accept only a
filename by using Path.GetFileName(filePath) and combine it with the base
directory via Path.Combine; if validation fails, throw an ArgumentException;
ensure the base directory exists (Directory.CreateDirectory) and store the
validated/canonicalized path in _filePath.
- Around line 118-133: In Load(), wrap the JsonConvert.DeserializeObject call in
a try/catch that catches JsonException (and optionally IOException) so malformed
or unreadable JSON won’t crash the process or leave callers with an unhandled
exception; inside the catch, log the error (including _filePath and exception
details) via the existing logging mechanism or throw a new descriptive
exception, and return new Dictionary<string,
List<StoredMessage>>(StringComparer.Ordinal) as a safe default; ensure you
reference Load(), JsonConvert.DeserializeObject(..., SerializerSettings) and
_filePath when locating the code to modify.
- Around line 135-139: The Save method currently writes directly to _filePath
which can corrupt the file if interrupted; change Save(Dictionary<string,
List<StoredMessage>> store) to write the serialized JSON to a temp file in the
same directory (e.g., _filePath + ".tmp" or use a unique temp name), flush and
close the temp file, then atomically replace the original using File.Replace (or
File.Move/Move with overwrite fallback on platforms without Replace), and ensure
you catch exceptions to delete the temp file on failure; keep using
SerializerSettings for serialization and preserve the existing method signature
(Save) and _filePath field.
In `@src/Agentic/Models/ChatMessage.cs`:
- Around line 28-41: The ChatMessage constructor currently allows an empty
contents list; update ChatMessage.ChatMessage(ChatRole,
IReadOnlyList<AiContent>, string?) to validate that contents contains at least
one element: after Guard.NotNull(contents) and before copying, check
contents.Count (or use contents.Any()) and throw an ArgumentException (or
ArgumentOutOfRangeException) with a clear message if empty; keep the existing
per-item Guard.NotNull(part) and then proceed to copy and assign Role, Contents,
and AuthorName so downstream code can rely on the "one or more content parts"
contract.
In `@src/Agentic/Models/Connectors/AnthropicChatClient.cs`:
- Around line 139-142: The final streaming frame currently emits ChatUsage(0,0)
when usage was never reported, which conflates "unknown" with "zero"; change
inputTokens and outputTokens from int to nullable (int?) and/or use the
roleEmitted flag so that you only construct/send a ChatUsage object when real
usage values were reported (i.e. tokens != null or a usage-reported flag is
true). Update the declarations for ChatFinishReason? finishReason, inputTokens,
outputTokens and uses of roleEmitted, and replace any unconditional
ChatUsage(0,0) emission sites (including the other occurrences flagged) with
logic that omits usage or sends null/absent usage when tokens are unknown.
Ensure all places that previously defaulted to zero now check the
nullable/token-reported state before including ChatUsage in the final frame.
- Line 61: Validate the provided endpoint in the AnthropicChatClient constructor
before assigning to _endpoint: ensure the string is non-empty, trim it, and
verify it parses as an absolute URI with http or https scheme (use Uri.TryCreate
and check IsAbsoluteUri and Scheme). If validation fails, throw an
ArgumentException (or ArgumentNullException for null/empty) with a clear message
so invalid endpoints are rejected at construction time instead of later during
requests.
- Around line 396-405: The ParseArguments method currently swallows
JsonException and returns an empty JObject; change this to surface the error
instead: in ParseArguments(string argumentsJson) catch the JsonException as ex
(catch (JsonException ex)), log or include ex.Message/details, and then rethrow
a descriptive exception (e.g. throw new ArgumentException("Failed to parse tool
arguments JSON", ex)) so callers don't proceed with an empty/default JObject;
ensure callers that call ParseArguments (and any invoking methods in this class)
handle or propagate the exception appropriately.
In `@src/Agentic/Models/Connectors/MeaiChatClient.cs`:
- Around line 101-113: The ToMeaiMessages method lacks validation of individual
elements in the messages list, allowing a null entry to cause a
NullReferenceException; update ToMeaiMessages to iterate with index/foreach and
validate each message != null before dereferencing (throw
ArgumentException/ArgumentNullException with a clear message that identifies the
offending index or that the messages collection contains null entries), and
ensure you still check message.Contents and use MapRole(message.Role) only after
that validation so MapRole and message.Text are never called on a null element.
In `@src/Agentic/Models/ImageContent.cs`:
- Around line 69-72: The FromUri factory currently only checks for
null/whitespace via Guard.NotNullOrWhiteSpace but does not validate the URI
format; update ImageContent.FromUri to validate the URI string (e.g., using
Uri.TryCreate or Uri.IsWellFormedUriString) before constructing ImageContent so
malformed URIs fail fast at the API boundary, and throw or use Guard to surface
a clear error; ensure the validation happens prior to calling the ImageContent
constructor (referencing FromUri and the ImageContent constructor).
- Around line 55-58: The FromBytes factory (ImageContent.FromBytes) currently
only checks for null and allows zero-length payloads; update the validation to
reject empty byte arrays as well (e.g., throw ArgumentException or use a Guard
helper to validate data.Length > 0) before constructing the ImageContent
instance so degenerate images cannot be created.
In `@src/Agentic/Tools/DelegateAgentTool.cs`:
- Around line 140-155: In UnwrapResultAsync, replace the silent null-return when
extracting Task<T>.Result with explicit defensive checks and errors: locate the
PropertyInfo via task.GetType().GetProperty("Result", BindingFlags.Instance |
BindingFlags.Public), throw an InvalidOperationException (including
task.GetType().FullName) if the property is null, and wrap GetValue in a
try/catch to rethrow a clear exception on reflection failures; this ensures
UnwrapResultAsync surfaces unexpected/malformed Task<T> types or reflection
errors instead of returning null.
In `@src/AiDotNet.Storage.Sqlite/SqliteConversationStore.cs`:
- Around line 148-159: EnsureSchemaAsync currently runs DDL on every call which
can race under concurrent threads and incur overhead; add a static SemaphoreSlim
(e.g., _schemaLock) to serialize schema creation and a static volatile bool flag
(e.g., _schemaInitialized) to short-circuit subsequent calls after the first
successful creation. Modify EnsureSchemaAsync to first check _schemaInitialized
and return if true, otherwise await _schemaLock.WaitAsync, re-check
_schemaInitialized inside the lock, run the existing CREATE TABLE/INDEX
commands, set _schemaInitialized=true on success, and finally release the
semaphore in a try/finally.
- Line 161: The ParseRole helper currently uses Enum.Parse which will throw on
unknown stored strings; update ParseRole(string role) to defensively handle
invalid values by using Enum.TryParse<ChatRole>(role, true, out var parsed) and
returning a safe default or throwing a more descriptive DataIntegrityException
(or logging and returning ChatRole.System/Unknown) so GetAsync callers don't
crash with ArgumentException; ensure the change is made in the ParseRole method
and callers (e.g., GetAsync) still receive a valid ChatRole or a clear
data-integrity error.
- Line 84: In SqliteConversationStore.cs inside the async method where the local
IDbTransaction/SqliteTransaction variable transaction is committed, replace the
synchronous transaction.Commit() call with an awaited asynchronous call: await
transaction.CommitAsync(cancellationToken).ConfigureAwait(false) so the method
remains fully async and avoids blocking the thread pool; ensure you pass the
existing CancellationToken variable used in the method and keep
ConfigureAwait(false) for consistency with the surrounding async calls.
In `@src/Models/Results/AiModelResult.cs`:
- Around line 1453-1456: The public method EvaluateBenchmarksAsync currently
changed parameter order which breaks existing call sites; restore the original
call shape by adding a compatibility-preserving overload: keep the original
signature Task<BenchmarkReport> EvaluateBenchmarksAsync(BenchmarkingOptions?
options = null, CancellationToken cancellationToken = default) and implement it
as a forwarder that calls the new EvaluateBenchmarksAsync(BenchmarkingOptions?
options = null, IChatClient<T>? chatClient = null, CancellationToken
cancellationToken = default) (or alternatively move the new IChatClient<T>?
chatClient parameter to after CancellationToken), ensuring the method name
EvaluateBenchmarksAsync remains available with the original parameter ordering
so existing callers continue to compile.
- Around line 1453-1460: The XML docs for EvaluateBenchmarksAsync are missing
the conditional requirement that IChatClient<T> must be provided when any suite
of type BenchmarkSuiteKind.Reasoning is selected; update the method's
documentation to state this contract or add an upfront guard in
EvaluateBenchmarksAsync that checks options?.Suites for any
BenchmarkSuiteKind.Reasoning and throws an InvalidOperationException (matching
BenchmarkRunner.RunAsync's message) if chatClient is null; reference
EvaluateBenchmarksAsync, BenchmarkRunner.RunAsync, BenchmarkingOptions.Suites,
BenchmarkSuiteKind.Reasoning and IChatClient<T> when making the change.
In `@src/Reasoning/DomainSpecific/CodeReasoner.cs`:
- Line 74: Public constructors for CodeReasoner<T>, LogicalReasoner<T>,
MathematicalReasoner<T>, and ScientificReasoner<T> changed signatures
(IChatModel<T> → IChatClient<T> and where applicable IEnumerable<ITool>? →
IEnumerable<IAgentTool>?), so update the release/migration notes: explicitly
list each type (CodeReasoner<T>, LogicalReasoner<T>, MathematicalReasoner<T>,
ScientificReasoner<T>) with the old constructor signature and the new one, state
the breaking-change rationale, provide a short migration snippet showing how to
replace IChatModel<T> with IChatClient<T> and ITool → IAgentTool, and add this
note to the CHANGELOG and PR description so downstream consumers can update
their subclasses and usages accordingly.
In `@src/Reasoning/ReasoningStrategyBase.cs`:
- Around line 317-319: ReasoningStrategyBase.ExecuteToolAsync builds a hardcoded
JObject { "input": input } which breaks tools whose ParametersSchema use
different parameter names (e.g., AgentAsTool<T> expects "task",
DelegateAgentTool binds by real parameter names); change ExecuteToolAsync to
accept a JObject arguments (preferred) or else inspect
tool.ParametersSchema["properties"] to obtain the single expected parameter name
and place the string value under that key, and throw a clear exception if the
schema does not define exactly one string property; update all callers of
ExecuteToolAsync accordingly and reference IAgentTool,
ReasoningStrategyBase.ExecuteToolAsync, AgentAsTool<T>, DelegateAgentTool and
ParametersSchema when locating code to modify.
---
Outside diff comments:
In `@src/Agentic/Tools/DelegateAgentTool.cs`:
- Around line 1-157: Change DelegateAgentTool from public sealed to internal
sealed and make its constructors internal so the concrete type is not part of
the public API; update callers (notably unit tests) to obtain tools via the
public factory methods AgentToolFactory.FromDelegate or
AgentToolFactory.ScanInstance instead of using new DelegateAgentTool(...).
Ensure all references to the public constructors/members are replaced with the
factory calls (search for symbol DelegateAgentTool and its constructors) and run
tests to fix any compile errors from the accessibility change.
In `@src/Models/Results/AiModelResult.cs`:
- Around line 1441-1460: Update the XML doc for EvaluateBenchmarksAsync to
reflect the new chat-client contract: state that callers must supply an
IChatClient<T> (or null to use defaults), update the <param name="chatClient">
description to explain how to provide a chat client instance instead of agent
configuration, remove any guidance about agent assistance/agent setup, and
adjust <exception> tags to describe failures originating from missing/invalid
chat client or BenchmarkRunner.RunAsync rather than agent misconfiguration;
apply the same doc changes to the other affected methods referenced (lines
5920-5945 and 6562-6736).
In `@src/Reasoning/Strategies/ChainOfThoughtStrategy.cs`:
- Around line 148-158: The verification branch in ChainOfThoughtStrategy
currently contains a TODO and falsely marks every step IsVerified = true;
replace this placeholder by either invoking the CriticModel<T> / ICriticModel
implementation to perform real checks on parsedSteps and set step.IsVerified and
confidence based on the model's result, or if the critic integration isn't
ready, remove the blanket assignment and throw a NotSupportedException (or set
EnableVerification to false) when config.EnableVerification is true so callers
are not misled; update the logic around config.EnableVerification, parsedSteps,
and chain.Steps to reflect the chosen behavior and ensure any thrown exception
mentions verification is not implemented yet.
In `@src/Reasoning/Training/ReinforcementLearner.cs`:
- Line 247: Replace hardcoded Console.WriteLine calls in ReinforcementLearner<T>
with event-based or injected-logger reporting: locate the epoch/batch loop where
Console.WriteLine($"\n=== Epoch {epoch + 1}/{_config.Epochs} ==="),
validation/early-stopping, STaR progress, and batch progress are printed and
emit those messages via the existing OnEpochComplete and OnBatchComplete events
(populate event args with epoch index, metrics, and progress text) or send them
to an injected ILogger<ReinforcementLearner<T>> instance; remove all
Console.WriteLine usages and ensure subscribers can consume the same information
through the events or logger so behavior remains functionally equivalent without
direct console output.
In `@src/Reasoning/Verification/OutcomeRewardModel.cs`:
- Around line 358-361: The empty catch-all in the semantic-similarity scoring
within the OutcomeRewardModel class must be replaced: remove the bare catch and
instead rethrow cancellation (catch and rethrow
OperationCanceledException/TaskCanceledException), then catch only explicit
transient failures (e.g., HttpRequestException, TimeoutException or other known
transient exceptions from the semantic similarity call), log the exception
details, and return the 0.0 fallback only for those handled transient cases; any
other exceptions should be rethrown. Ensure logging uses the existing logger in
OutcomeRewardModel and update the semantic-similarity scoring method's catch
logic accordingly.
In `@src/Reasoning/Verification/SelfRefinementEngine.cs`:
- Line 75: RefineStepAsync currently calls
_chatModel.GenerateResponseAsync(prompt) without the cancellation token; change
that call to the cancellation-aware overload (use
ChatClientExtensions.GenerateResponseAsync or the GenerateResponseAsync overload
that accepts a CancellationToken) so RefineStepAsync forwards its
cancellationToken into the LLM call, ensuring the token flows through to
GetResponseAsync; update the invocation in SelfRefinementEngine.RefineStepAsync
to pass the cancellationToken parameter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83580c60-d95c-480a-b556-32e2f518b65d
📒 Files selected for processing (217)
Directory.Packages.propssrc/Agentic/Agents/AgentAsTool.cssrc/Agentic/Agents/AgentExecutor.cssrc/Agentic/Agents/AgentExecutorOptions.cssrc/Agentic/Agents/AgentRunResult.cssrc/Agentic/Agents/IAgent.cssrc/Agentic/Agents/SupervisorAgent.cssrc/Agentic/Agents/SupervisorOptions.cssrc/Agentic/Agents/Swarm.cssrc/Agentic/Agents/SwarmMember.cssrc/Agentic/Agents/SwarmOptions.cssrc/Agentic/Agents/ToolNaming.cssrc/Agentic/Memory/AgentMemory.cssrc/Agentic/Memory/EmbeddingAgentMemoryStore.cssrc/Agentic/Memory/IAgentMemoryStore.cssrc/Agentic/Memory/IConversationStore.cssrc/Agentic/Memory/InMemoryAgentMemoryStore.cssrc/Agentic/Memory/InMemoryConversationStore.cssrc/Agentic/Memory/JsonFileConversationStore.cssrc/Agentic/Memory/MemoryAugmentationOptions.cssrc/Agentic/Memory/MemoryAugmentedAgent.cssrc/Agentic/Memory/ScoredMemory.cssrc/Agentic/Memory/ThreadedAgent.cssrc/Agentic/Models/AiContent.cssrc/Agentic/Models/AiToolDefinition.cssrc/Agentic/Models/ChatClientExtensions.cssrc/Agentic/Models/ChatFinishReason.cssrc/Agentic/Models/ChatMessage.cssrc/Agentic/Models/ChatOptions.cssrc/Agentic/Models/ChatResponse.cssrc/Agentic/Models/ChatResponseFormatKind.cssrc/Agentic/Models/ChatResponseUpdate.cssrc/Agentic/Models/ChatRole.cssrc/Agentic/Models/ChatUsage.cssrc/Agentic/Models/Connectors/AnthropicChatClient.cssrc/Agentic/Models/Connectors/AzureOpenAIChatClient.cssrc/Agentic/Models/Connectors/ChatClientBase.cssrc/Agentic/Models/Connectors/MeaiChatClient.cssrc/Agentic/Models/Connectors/OpenAIChatClient.cssrc/Agentic/Models/IChatClient.cssrc/Agentic/Models/ImageContent.cssrc/Agentic/Models/ImageMediaType.cssrc/Agentic/Models/ImageMediaTypeExtensions.cssrc/Agentic/Models/StreamingToolCallUpdate.cssrc/Agentic/Models/TextContent.cssrc/Agentic/Models/ToolCallContent.cssrc/Agentic/Models/ToolChoiceMode.cssrc/Agentic/Models/ToolResultContent.cssrc/Agentic/Tools/AgentToolAttribute.cssrc/Agentic/Tools/AgentToolBase.cssrc/Agentic/Tools/AgentToolFactory.cssrc/Agentic/Tools/DelegateAgentTool.cssrc/Agentic/Tools/FunctionAgentTool.cssrc/Agentic/Tools/IAgentTool.cssrc/Agentic/Tools/JsonSchemaGenerator.cssrc/Agentic/Tools/StructuredOutputExtensions.cssrc/Agentic/Tools/ToolCollection.cssrc/Agentic/Tools/ToolInvocationResult.cssrc/Agentic/Tools/ToolParameterAttribute.cssrc/Agents/Agent.cssrc/Agents/AgentBase.cssrc/Agents/AgentGlobalConfiguration.cssrc/Agents/AgentHyperparameterApplicator.cssrc/Agents/AgentKeyResolver.cssrc/Agents/ChainOfThoughtAgent.cssrc/Agents/HyperparameterDefinition.cssrc/Agents/HyperparameterRegistry.cssrc/Agents/HyperparameterResponseParser.cssrc/Agents/HyperparameterValidationResult.cssrc/Agents/PlanAndExecuteAgent.cssrc/Agents/RAGAgent.cssrc/Agents/README.mdsrc/AiDotNet.Generators/AgentToolSchemaGenerator.cssrc/AiDotNet.Storage.Sqlite/SqliteConversationStore.cssrc/AiDotNet.csprojsrc/AiModelBuilder.BuildPipeline.cssrc/AiModelBuilder.Configure.cssrc/AiModelBuilder.Coverage.cssrc/AiModelBuilder.DomainML.cssrc/AiModelBuilder.Internals.cssrc/AiModelBuilder.Workflows.cssrc/AiModelBuilder.cssrc/Benchmarking/BenchmarkRunner.cssrc/Configuration/IConfiguredView.cssrc/Interfaces/IAgent.cssrc/Interfaces/IAiModelBuilder.cssrc/Interfaces/IChain.cssrc/Interfaces/IChatModel.cssrc/Interfaces/IFunctionTool.cssrc/Interfaces/ILanguageModel.cssrc/Interfaces/IPromptChain.cssrc/Interfaces/ITool.cssrc/LanguageModels/AnthropicChatModel.cssrc/LanguageModels/AzureOpenAIChatModel.cssrc/LanguageModels/ChatModelBase.cssrc/LanguageModels/Models/OpenAIChoice.cssrc/LanguageModels/Models/OpenAIMessage.cssrc/LanguageModels/Models/OpenAIRequest.cssrc/LanguageModels/Models/OpenAIResponse.cssrc/LanguageModels/Models/OpenAIUsage.cssrc/LanguageModels/OpenAIChatModel.cssrc/LanguageModels/README.mdsrc/Models/AgentAssistanceOptions.cssrc/Models/AgentAssistanceOptionsBuilder.cssrc/Models/AgentConfiguration.cssrc/Models/AgentGlobalConfigurationBuilder.cssrc/Models/AgentRecommendation.cssrc/Models/Options/AiModelResultOptions.cssrc/Models/Results/AiModelResult.cssrc/PromptEngineering/Chains/ChainBase.cssrc/PromptEngineering/Chains/ConditionalChain.cssrc/PromptEngineering/Chains/LoopChain.cssrc/PromptEngineering/Chains/MapReduceChain.cssrc/PromptEngineering/Chains/ParallelChain.cssrc/PromptEngineering/Chains/RouterChain.cssrc/PromptEngineering/Chains/SequentialChain.cssrc/PromptEngineering/Tools/FunctionToolBase.cssrc/PromptEngineering/Tools/ToolRegistry.cssrc/Reasoning/Components/ContradictionDetector.cssrc/Reasoning/Components/ThoughtEvaluator.cssrc/Reasoning/Components/ThoughtGenerator.cssrc/Reasoning/DomainSpecific/CodeReasoner.cssrc/Reasoning/DomainSpecific/LogicalReasoner.cssrc/Reasoning/DomainSpecific/MathematicalReasoner.cssrc/Reasoning/DomainSpecific/ScientificReasoner.cssrc/Reasoning/Reasoner.cssrc/Reasoning/ReasoningStrategyBase.cssrc/Reasoning/Search/MonteCarloTreeSearch.cssrc/Reasoning/Strategies/ChainOfThoughtStrategy.cssrc/Reasoning/Strategies/SelfConsistencyStrategy.cssrc/Reasoning/Strategies/TreeOfThoughtsStrategy.cssrc/Reasoning/Training/PolicyGradientTrainer.cssrc/Reasoning/Training/ReinforcementLearner.cssrc/Reasoning/Verification/CriticModel.cssrc/Reasoning/Verification/OutcomeRewardModel.cssrc/Reasoning/Verification/ProcessRewardModel.cssrc/Reasoning/Verification/SelfRefinementEngine.cssrc/Tools/AiModelTool.cssrc/Tools/BingSearchResponse.cssrc/Tools/BingWebPage.cssrc/Tools/BingWebPages.cssrc/Tools/CalculatorTool.cssrc/Tools/CrossValidationTool.cssrc/Tools/DataAnalysisTool.cssrc/Tools/DiffusionTools.cssrc/Tools/FeatureImportanceTool.cssrc/Tools/HyperparameterTool.cssrc/Tools/ModelSelectionTool.cssrc/Tools/RAGTool.cssrc/Tools/RegularizationTool.cssrc/Tools/SearchProvider.cssrc/Tools/SearchTool.cssrc/Tools/SerpAPIResponse.cssrc/Tools/SerpAPIResult.cssrc/Tools/ThreeDHyperparameterTool.cssrc/Tools/ThreeDModelSelectionTool.cssrc/Tools/ToolBase.cssrc/Tools/VectorSearchTool.cssrc/Tools/WebSearchTool.cstests/AiDotNet.Tests/IntegrationTests/Agents/AgentsDeepMathIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Agents/AgentsIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Agents/HyperparameterAutoApplyIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/ConfigureMethodCoverage/Bucket11_HijackPathTests.cstests/AiDotNet.Tests/IntegrationTests/LanguageModels/LanguageModelsDeepMathIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/LanguageModels/LanguageModelsIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/PromptEngineering/PromptEngineeringIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Reasoning/ReasoningExtendedIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Tools/ToolsDeepMathIntegrationTests.cstests/AiDotNet.Tests/IntegrationTests/Tools/ToolsIntegrationTests.cstests/AiDotNet.Tests/MergedPRBugFixTests.cstests/AiDotNet.Tests/PromptEngineering/ToolRegistryTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Agents/AgentExecutorTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Agents/AgentMemoryStoreTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Agents/AgentTestInfrastructure.cstests/AiDotNet.Tests/UnitTests/Agentic/Agents/ConversationMemoryTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Agents/SqliteConversationStoreTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Agents/SupervisorAgentTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Agents/SwarmTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Connectors/AnthropicChatClientTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Connectors/AzureOpenAIChatClientTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Connectors/MeaiChatClientTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Connectors/OpenAIChatClientTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Models/ChatModelAbstractionTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Tools/AgentToolTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Tools/GeneratedAgentToolTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Tools/StructuredOutputTests.cstests/AiDotNet.Tests/UnitTests/Agents/AgentHyperparameterApplicatorTests.cstests/AiDotNet.Tests/UnitTests/Agents/HyperparameterRegistryTests.cstests/AiDotNet.Tests/UnitTests/Agents/HyperparameterResponseParserTests.cstests/AiDotNet.Tests/UnitTests/Tools/RAGToolTests.cstests/AiDotNet.Tests/UnitTests/Tools/ToolBaseTests.cstests/AiDotNet.Tests/UnitTests/Tools/VectorSearchToolTests.cstests/Reasoning/Benchmarks/AiModelResultBenchmarkFacadeTests.cstests/Reasoning/Benchmarks/BenchmarkTests.cstests/Reasoning/Benchmarks/ProblemAnswerLookupChain.cstests/Reasoning/IntegrationTests.cstests/Reasoning/Search/SearchAlgorithmTests.cstests/Reasoning/Strategies/ChainOfThoughtStrategyTests.cstests/Reasoning/Verification/CalculatorVerifierTests.cstests/UnitTests/Agents/AgentTests.cstests/UnitTests/Agents/ChainOfThoughtAgentTests.cstests/UnitTests/Agents/MockChatModel.cstests/UnitTests/Agents/PlanAndExecuteAgentTests.cstests/UnitTests/DistributedTraining/DistributedTrainingTests.cstests/UnitTests/LanguageModels/AnthropicChatModelTests.cstests/UnitTests/LanguageModels/AzureOpenAIChatModelTests.cstests/UnitTests/LanguageModels/OpenAIChatModelTests.cstests/UnitTests/MatrixDecomposition/IcaDecompositionTests.cstests/UnitTests/MatrixDecomposition/NmfDecompositionTests.cstests/UnitTests/Models/Generative/Diffusion/DDPMModelTests.cstests/UnitTests/NeuralNetworks/Layers/ExpertTests.cstests/UnitTests/NeuralNetworks/Layers/MixtureOfExpertsLayerTests.cstests/UnitTests/NeuralNetworks/MixtureOfExpertsNeuralNetworkTests.cstests/UnitTests/Normalizers/MaxAbsScalerTests.cstests/UnitTests/Normalizers/QuantileTransformerTests.cstests/UnitTests/Tools/CalculatorToolTests.cstests/UnitTests/Tools/SearchToolTests.cs
💤 Files with no reviewable changes (57)
- src/Agents/README.md
- src/Tools/BingWebPages.cs
- src/Interfaces/ITool.cs
- src/Agents/AgentKeyResolver.cs
- src/Tools/BingSearchResponse.cs
- src/Tools/CalculatorTool.cs
- src/LanguageModels/Models/OpenAIRequest.cs
- src/Interfaces/IChain.cs
- src/Agents/ChainOfThoughtAgent.cs
- src/Models/AgentAssistanceOptions.cs
- src/Agents/AgentBase.cs
- src/Interfaces/IChatModel.cs
- src/LanguageModels/Models/OpenAIChoice.cs
- src/LanguageModels/Models/OpenAIResponse.cs
- src/Interfaces/IAgent.cs
- src/Agents/PlanAndExecuteAgent.cs
- src/Models/AgentGlobalConfigurationBuilder.cs
- src/LanguageModels/AzureOpenAIChatModel.cs
- src/PromptEngineering/Chains/LoopChain.cs
- src/Agents/RAGAgent.cs
- src/Interfaces/ILanguageModel.cs
- src/Tools/BingWebPage.cs
- src/Interfaces/IPromptChain.cs
- src/Tools/CrossValidationTool.cs
- src/Agents/HyperparameterResponseParser.cs
- src/LanguageModels/ChatModelBase.cs
- src/PromptEngineering/Chains/SequentialChain.cs
- src/Models/AgentConfiguration.cs
- src/LanguageModels/Models/OpenAIUsage.cs
- src/PromptEngineering/Chains/ChainBase.cs
- src/Agents/HyperparameterRegistry.cs
- src/Agents/AgentGlobalConfiguration.cs
- src/Agents/HyperparameterValidationResult.cs
- src/Tools/AiModelTool.cs
- src/PromptEngineering/Tools/FunctionToolBase.cs
- src/Interfaces/IFunctionTool.cs
- src/PromptEngineering/Chains/ParallelChain.cs
- src/LanguageModels/README.md
- src/Models/AgentRecommendation.cs
- src/PromptEngineering/Chains/ConditionalChain.cs
- src/Agents/HyperparameterDefinition.cs
- src/LanguageModels/Models/OpenAIMessage.cs
- src/Configuration/IConfiguredView.cs
- src/PromptEngineering/Tools/ToolRegistry.cs
- src/Agents/Agent.cs
- src/PromptEngineering/Chains/MapReduceChain.cs
- src/PromptEngineering/Chains/RouterChain.cs
- src/Agents/AgentHyperparameterApplicator.cs
- src/Models/Options/AiModelResultOptions.cs
- src/LanguageModels/AnthropicChatModel.cs
- src/LanguageModels/OpenAIChatModel.cs
- src/Models/AgentAssistanceOptionsBuilder.cs
- src/AiModelBuilder.BuildPipeline.cs
- src/AiModelBuilder.Internals.cs
- src/AiModelBuilder.Workflows.cs
- src/AiModelBuilder.cs
- src/AiModelBuilder.Configure.cs
ooples
pushed a commit
that referenced
this pull request
Jun 11, 2026
The flagship local-first differentiator: an IChatClient<T> that runs entirely in-process over AiDotNet's own model — no network, no API key. Same agent code drives it as drives OpenAI/Anthropic. - ICausalLanguageModel<T>: minimal next-token-logits seam the real Transformer (or any model) plugs into; implementations may keep an internal KV-cache. - IGenerationTokenizer (+TokenizerGenerationAdapter over the repo's ITokenizer): encode/decode/EOS seam. - TokenSampler<T>: greedy (temp 0 = argmax) + temperature + top-k + top-p (nucleus), seedable for reproducibility; reads logits via Convert.ToDouble so float/double share one path. - IChatPromptTemplate + ChatMlPromptTemplate (role-tagged, opens the assistant turn). - LocalEngineChatClient<T>: renders prompt -> encodes -> autoregressively samples until EOS or token limit -> decodes; non-streaming + streaming (incremental decode deltas); usage = prompt/generated token counts. Drop-in for agents/supervisor/swarm/memory. (Native tool-calling + structured-output constraints via constrained decoding are a follow-up; tools are ignored this slice.) - 9 tests (green net10.0 + net471) on a deterministic ScriptedCausalModel + WordTokenizer: greedy decode + EOS stop, token-limit -> Length, streaming deltas reconstruct full text, AgentExecutor drop-in, sampler argmax/seed-reproducibility/top-k/top-p, ChatML template. No null-forgiving. Stacked on Phase 2 (#1551) — the integration test shows the local engine driving an AgentExecutor. Part of epic #1544 (Phase 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ooples
pushed a commit
that referenced
this pull request
Jun 11, 2026
The flagship local-first differentiator: an IChatClient<T> that runs entirely in-process over AiDotNet's own model — no network, no API key. Same agent code drives it as drives OpenAI/Anthropic. - ICausalLanguageModel<T>: minimal next-token-logits seam the real Transformer (or any model) plugs into; implementations may keep an internal KV-cache. - IGenerationTokenizer (+TokenizerGenerationAdapter over the repo's ITokenizer): encode/decode/EOS seam. - TokenSampler<T>: greedy (temp 0 = argmax) + temperature + top-k + top-p (nucleus), seedable for reproducibility; reads logits via Convert.ToDouble so float/double share one path. - IChatPromptTemplate + ChatMlPromptTemplate (role-tagged, opens the assistant turn). - LocalEngineChatClient<T>: renders prompt -> encodes -> autoregressively samples until EOS or token limit -> decodes; non-streaming + streaming (incremental decode deltas); usage = prompt/generated token counts. Drop-in for agents/supervisor/swarm/memory. (Native tool-calling + structured-output constraints via constrained decoding are a follow-up; tools are ignored this slice.) - 9 tests (green net10.0 + net471) on a deterministic ScriptedCausalModel + WordTokenizer: greedy decode + EOS stop, token-limit -> Length, streaming deltas reconstruct full text, AgentExecutor drop-in, sampler argmax/seed-reproducibility/top-k/top-p, ChatML template. No null-forgiving. Stacked on Phase 2 (#1551) — the integration test shows the local engine driving an AgentExecutor. Part of epic #1544 (Phase 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ooples
pushed a commit
that referenced
this pull request
Jun 11, 2026
The flagship local-first differentiator: an IChatClient<T> that runs entirely in-process over AiDotNet's own model — no network, no API key. Same agent code drives it as drives OpenAI/Anthropic. - ICausalLanguageModel<T>: minimal next-token-logits seam the real Transformer (or any model) plugs into; implementations may keep an internal KV-cache. - IGenerationTokenizer (+TokenizerGenerationAdapter over the repo's ITokenizer): encode/decode/EOS seam. - TokenSampler<T>: greedy (temp 0 = argmax) + temperature + top-k + top-p (nucleus), seedable for reproducibility; reads logits via Convert.ToDouble so float/double share one path. - IChatPromptTemplate + ChatMlPromptTemplate (role-tagged, opens the assistant turn). - LocalEngineChatClient<T>: renders prompt -> encodes -> autoregressively samples until EOS or token limit -> decodes; non-streaming + streaming (incremental decode deltas); usage = prompt/generated token counts. Drop-in for agents/supervisor/swarm/memory. (Native tool-calling + structured-output constraints via constrained decoding are a follow-up; tools are ignored this slice.) - 9 tests (green net10.0 + net471) on a deterministic ScriptedCausalModel + WordTokenizer: greedy decode + EOS stop, token-limit -> Length, streaming deltas reconstruct full text, AgentExecutor drop-in, sampler argmax/seed-reproducibility/top-k/top-p, ChatML template. No null-forgiving. Stacked on Phase 2 (#1551) — the integration test shows the local engine driving an AgentExecutor. Part of epic #1544 (Phase 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (15)
src/Agentic/Memory/EmbeddingAgentMemoryStore.cs (4)
103-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
GetAllAsyncignores thecancellationTokenparameter.While this is a fast synchronous path, the method signature promises cancellation support. Add
cancellationToken.ThrowIfCancellationRequested()at method entry for consistency.🛡️ Proposed fix
public Task<IReadOnlyList<AgentMemory>> GetAllAsync(CancellationToken cancellationToken = default) { + cancellationToken.ThrowIfCancellationRequested(); lock (_gate) { IReadOnlyList<AgentMemory> all = _entries.Select(e => e.Memory).ToList(); return Task.FromResult(all); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Memory/EmbeddingAgentMemoryStore.cs` around lines 103 - 110, GetAllAsync currently ignores the cancellationToken parameter; at the start of the GetAllAsync method add cancellationToken.ThrowIfCancellationRequested() before acquiring the lock (preserving the existing lock(_gate) and the _entries.Select(...) logic) so the synchronous fast path honors cancellation requests while leaving the rest of the implementation unchanged.
113-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
RemoveAsyncignores thecancellationTokenparameter.The method accepts a
cancellationTokenbut never checks it. AddcancellationToken.ThrowIfCancellationRequested()after validation and before the lock for consistency.🛡️ Proposed fix
public Task RemoveAsync(string id, CancellationToken cancellationToken = default) { Guard.NotNullOrWhiteSpace(id); + cancellationToken.ThrowIfCancellationRequested(); lock (_gate) { _entries.RemoveAll(e => string.Equals(e.Memory.Id, id, StringComparison.Ordinal)); } return Task.CompletedTask; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Memory/EmbeddingAgentMemoryStore.cs` around lines 113 - 123, In RemoveAsync add cancellation support by calling cancellationToken.ThrowIfCancellationRequested() immediately after Guard.NotNullOrWhiteSpace(id) and before entering the lock so the method respects the provided CancellationToken; update the method body around the Guard and lock in EmbeddingAgentMemoryStore.RemoveAsync to perform the cancellation check prior to modifying _entries.
48-64:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
AddAsyncignores thecancellationTokenparameter.The method accepts a
cancellationTokenbut never checks it. WhileIEmbeddingModel<T>.EmbedAsyncdoesn't accept a token (based on past review findings), you must callcancellationToken.ThrowIfCancellationRequested()at the method entry to honor early cancellation and avoid misleading callers who expect cancellation support.🛡️ Proposed fix
public async Task<string> AddAsync( string content, IReadOnlyDictionary<string, string>? metadata = null, CancellationToken cancellationToken = default) { Guard.NotNullOrWhiteSpace(content); + cancellationToken.ThrowIfCancellationRequested(); var id = Guid.NewGuid().ToString("N"); var memory = new AgentMemory(id, content, metadata); var vector = await _embeddingModel.EmbedAsync(content).ConfigureAwait(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Memory/EmbeddingAgentMemoryStore.cs` around lines 48 - 64, AddAsync currently ignores the cancellationToken; at the start of the method (in AddAsync) call cancellationToken.ThrowIfCancellationRequested() to honor early cancellation before any work (e.g., Guard.NotNullOrWhiteSpace, generating id, calling _embeddingModel.EmbedAsync). This ensures callers can cancel immediately even though _embeddingModel.EmbedAsync doesn't accept a token.
67-100:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
SearchAsyncignores thecancellationTokenparameter.The method accepts a
cancellationTokenbut never checks it, even during the potentially lengthy scoring loop. AddcancellationToken.ThrowIfCancellationRequested()at method entry and periodically inside the scoring loop (lines 89-93) to abort long-running searches promptly.🛡️ Proposed fix
public async Task<IReadOnlyList<ScoredMemory>> SearchAsync( string query, int topK = 5, CancellationToken cancellationToken = default) { Guard.NotNullOrWhiteSpace(query); Guard.Positive(topK); + cancellationToken.ThrowIfCancellationRequested(); List<Entry> snapshot; lock (_gate) { snapshot = new List<Entry>(_entries); } if (snapshot.Count == 0) { return new List<ScoredMemory>(); } var queryVector = await _embeddingModel.EmbedAsync(query).ConfigureAwait(false); + cancellationToken.ThrowIfCancellationRequested(); var scored = new List<ScoredMemory>(snapshot.Count); foreach (var entry in snapshot) { + cancellationToken.ThrowIfCancellationRequested(); var score = Convert.ToDouble(_similarity.Calculate(queryVector, entry.Vector)); scored.Add(new ScoredMemory(entry.Memory, score)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Memory/EmbeddingAgentMemoryStore.cs` around lines 67 - 100, SearchAsync currently ignores the cancellationToken; add cancellationToken.ThrowIfCancellationRequested() at the start of SearchAsync (after Guard checks and before calling _embeddingModel.EmbedAsync) and also inside the scoring loop in the foreach over snapshot (before calling _similarity.Calculate and before adding to scored) so long-running searches can be aborted promptly; keep existing logic (embedding await and ordering) but sprinkle these token checks to promptly throw OperationCanceledException when requested.src/Agentic/Agents/SupervisorAgent.cs (1)
62-69: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider using
string.IsNullOrWhiteSpacefor cleaner null/empty checks.This pattern was already identified in a previous review. The verbose
is { } x && x.Trim().Length > 0checks can be simplified for better readability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Agents/SupervisorAgent.cs` around lines 62 - 69, Replace the verbose null/empty checks in the SupervisorAgent constructor (the assignments to Name, Description and systemPrompt) with string.IsNullOrWhiteSpace-based conditions: use string.IsNullOrWhiteSpace(settings.Name) ? "supervisor" : settings.Name.Trim() for Name, similarly for Description with its default, and for systemPrompt use string.IsNullOrWhiteSpace(settings.SystemPrompt) ? BuildDefaultRoutingPrompt(workers) : settings.SystemPrompt.Trim(); update the symbols Name, Description, and systemPrompt accordingly to keep trimming and defaults.src/AiDotNet.Storage.Sqlite/SqliteConversationStore.cs (3)
84-84: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse CommitAsync for consistency.
Line 84 uses the synchronous
transaction.Commit()while the entire method is async. For consistency and to avoid blocking the thread pool, useawait transaction.CommitAsync(cancellationToken).ConfigureAwait(false).⚡ Async consistency fix
- transaction.Commit(); + await transaction.CommitAsync(cancellationToken).ConfigureAwait(false);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/AiDotNet.Storage.Sqlite/SqliteConversationStore.cs` at line 84, Replace the synchronous transaction.Commit() call with an asynchronous commit to avoid blocking: in the method where transaction.Commit() is called (the transaction variable in SqliteConversationStore), change it to await transaction.CommitAsync(cancellationToken).ConfigureAwait(false); ensure the surrounding method remains async and that a CancellationToken named cancellationToken (or the method's token) is passed through to CommitAsync.
148-159: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffSchema creation overhead on every operation.
EnsureSchemaAsyncruns DDL (CREATE TABLE IF NOT EXISTS,CREATE INDEX IF NOT EXISTS) on every single operation. While SQLite's locking makes this safe from races, it adds unnecessary overhead.Consider caching a flag after first successful schema creation to skip repeated DDL checks.
💡 Optional caching pattern
+ private static readonly SemaphoreSlim _schemaLock = new(1, 1); + private static volatile bool _schemaInitialized; + private static async Task EnsureSchemaAsync(SqliteConnection connection, CancellationToken cancellationToken) { + if (_schemaInitialized) return; + + await _schemaLock.WaitAsync(cancellationToken).ConfigureAwait(false); + try + { + if (_schemaInitialized) return; + using var command = connection.CreateCommand(); command.CommandText = "CREATE TABLE IF NOT EXISTS Conversations (" + "Seq INTEGER PRIMARY KEY AUTOINCREMENT, " + "ThreadId TEXT NOT NULL, " + "Role TEXT NOT NULL, " + "Text TEXT NOT NULL);" + "CREATE INDEX IF NOT EXISTS IX_Conversations_ThreadId ON Conversations (ThreadId, Seq);"; await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false); + + _schemaInitialized = true; + } + finally + { + _schemaLock.Release(); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/AiDotNet.Storage.Sqlite/SqliteConversationStore.cs` around lines 148 - 159, The EnsureSchemaAsync method currently runs the CREATE TABLE/INDEX DDL on every call which adds unnecessary overhead; modify SqliteConversationStore to run EnsureSchemaAsync only once per connection lifecycle by adding a private static or instance-level volatile/atomic boolean (e.g., _schemaEnsured or _schemaEnsuredForConnection) that is checked at the start of EnsureSchemaAsync (or before calling it from methods that open the connection) and set to true after a successful schema creation; keep the existing SQL and error handling but skip executing the command when the flag is true to avoid repeated DDL calls while preserving safety.
161-161:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnsafe enum parsing will crash on invalid data.
Line 161 uses
Enum.Parsewithout error handling. If the database contains an invalid role string (from data corruption or schema evolution), this throwsArgumentException, crashingGetAsync.Production code should handle invalid persisted data gracefully.
🛡️ Defensive parsing
- private static ChatRole ParseRole(string role) => (ChatRole)Enum.Parse(typeof(ChatRole), role); + private static ChatRole ParseRole(string role) + { + if (Enum.TryParse<ChatRole>(role, out var result)) + { + return result; + } + throw new InvalidOperationException($"Invalid role value in database: '{role}'"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/AiDotNet.Storage.Sqlite/SqliteConversationStore.cs` at line 161, Replace the unsafe Enum.Parse call in the ParseRole helper with defensive parsing: update ParseRole(string role) to use Enum.TryParse<ChatRole>(...) (optionally ignore case), and if parsing fails return a safe default (e.g., ChatRole.User) and emit a warning/log so callers like GetAsync can continue instead of throwing; ensure all call sites (ParseRole and GetAsync) rely on the new safe behavior rather than propagating exceptions.src/Agentic/Memory/JsonFileConversationStore.cs (3)
135-139: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftNon-atomic writes risk data loss.
The current implementation writes directly to
_filePath(line 138). If the process crashes or loses power mid-write, the file is left corrupted, losing all conversation history.Production persistence should use atomic write-to-temp-then-rename patterns.
💾 Atomic write pattern
private void Save(Dictionary<string, List<StoredMessage>> store) { var json = JsonConvert.SerializeObject(store, SerializerSettings); - File.WriteAllText(_filePath, json); + var tempPath = _filePath + ".tmp"; + File.WriteAllText(tempPath, json); + File.Move(tempPath, _filePath, overwrite: true); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Memory/JsonFileConversationStore.cs` around lines 135 - 139, The Save method currently writes JSON directly to _filePath which can corrupt the file on crash; change Save(Dictionary<string, List<StoredMessage>> store) to perform an atomic write by serializing using SerializerSettings to a temporary file in the same directory (e.g., _filePath + ".tmp" or use Path.GetTempFileName in the same folder), flush and close the temp file, then replace/rename the temp file to _filePath using an atomic operation (File.Replace or File.Move with overwrite semantics) and ensure exceptions clean up the temp file; update Save to handle IO exceptions and preserve original file on failure.
118-133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing error handling for JSON deserialization.
Line 131 calls
JsonConvert.DeserializeObjectwithout a try/catch. Malformed JSON from file corruption or manual editing will throwJsonException, crashing the operation.Production code must handle deserialization failures gracefully with proper error handling and logging.
🛡️ Required error handling
private Dictionary<string, List<StoredMessage>> Load() { if (!File.Exists(_filePath)) { return new Dictionary<string, List<StoredMessage>>(StringComparer.Ordinal); } var json = File.ReadAllText(_filePath); if (json.Trim().Length == 0) { return new Dictionary<string, List<StoredMessage>>(StringComparer.Ordinal); } + try + { - var loaded = JsonConvert.DeserializeObject<Dictionary<string, List<StoredMessage>>>(json, SerializerSettings); - return loaded ?? new Dictionary<string, List<StoredMessage>>(StringComparer.Ordinal); + var loaded = JsonConvert.DeserializeObject<Dictionary<string, List<StoredMessage>>>(json, SerializerSettings); + return loaded ?? new Dictionary<string, List<StoredMessage>>(StringComparer.Ordinal); + } + catch (JsonException ex) + { + throw new InvalidOperationException($"Failed to load conversation store from '{_filePath}'. The file may be corrupted.", ex); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Memory/JsonFileConversationStore.cs` around lines 118 - 133, In Load(), wrap the JsonConvert.DeserializeObject call in a try/catch that catches JsonException (and optionally Exception) when reading/parsing the contents of _filePath; on error log the exception and file path via your logger and return a new empty Dictionary<string, List<StoredMessage>>(StringComparer.Ordinal) to avoid crashing; keep using SerializerSettings and ensure the method still returns loaded when deserialization succeeds.
43-47:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBLOCKING: Path traversal vulnerability still present.
The constructor accepts an arbitrary
filePathwithout validation, canonicalization, or sandboxing. An attacker-controlled path enables reading/writing anywhere the process has permissions, exposing a critical security vulnerability.Production-ready code must validate paths against a safe base directory or enforce strict path policies.
🔒 Required mitigation
public JsonFileConversationStore(string filePath) { Guard.NotNullOrWhiteSpace(filePath); + + // Canonicalize and validate against base directory + var fullPath = Path.GetFullPath(filePath); + var baseDirectory = Path.GetFullPath(AppDomain.CurrentDomain.BaseDirectory); + if (!fullPath.StartsWith(baseDirectory, StringComparison.OrdinalIgnoreCase)) + { + throw new ArgumentException($"File path must be within application directory: {baseDirectory}", nameof(filePath)); + } + - _filePath = filePath; + _filePath = fullPath; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Memory/JsonFileConversationStore.cs` around lines 43 - 47, The constructor for JsonFileConversationStore currently assigns an attacker-controlled filePath to _filePath after only checking NotNullOrWhiteSpace; to fix, canonicalize and validate the path against a safe base directory (e.g., compute Path.GetFullPath(filePath) and ensure it starts with the canonical base directory or is a child of a configured allowed directory) before assigning to _filePath, and throw an exception if the check fails; consider injecting or defining a constant for the allowed base directory, reject relative/parent-traversal segments, and preserve the Guard.NotNullOrWhiteSpace check in the constructor.src/Agentic/Models/Connectors/MeaiChatClient.cs (1)
101-116:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBLOCKING: Validate each message element before dereferencing in ToMeaiMessages.
The method validates the
messagescollection but not individual elements. A null entry will crash withNullReferenceExceptionat Line 106 (message.Contents) or Line 112 (message.Role,message.Text) instead of failing fast with clear argument validation.As per coding guidelines, missing validation of external inputs at system boundaries is a blocking production-readiness issue.
🛡️ Proposed fix
private static List<Meai.ChatMessage> ToMeaiMessages(IReadOnlyList<ChatMessage> messages) { var result = new List<Meai.ChatMessage>(messages.Count); foreach (var message in messages) { + Guard.NotNull(message); + if (message.Contents.Any(c => c is ToolCallContent || c is ToolResultContent || c is ImageContent)) { throw new NotSupportedException(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Models/Connectors/MeaiChatClient.cs` around lines 101 - 116, The ToMeaiMessages method must validate inputs and each message element before dereferencing: add a null-check for the messages parameter and throw ArgumentNullException if null, then inside the loop validate that each ChatMessage (the loop variable message) is not null and that message.Contents is not null (throw ArgumentNullException/ArgumentException with clear param names), and also ensure message.Role and message.Text are present (or non-null) before using them; after these checks continue with the existing validation for ToolCallContent/ToolResultContent/ImageContent and constructing new Meai.ChatMessage. This ensures ToMeaiMessages fails fast with clear exceptions instead of raising NullReferenceException.Source: Coding guidelines
src/Agentic/Models/Connectors/AnthropicChatClient.cs (3)
49-64:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThe past review comment on line 61 regarding endpoint validation remains valid.
The constructor accepts a custom
endpointparameter but does not validate it before storing. As noted in the prior review, malformed endpoints will fail during request execution rather than at construction time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Models/Connectors/AnthropicChatClient.cs` around lines 49 - 64, The AnthropicChatClient constructor stores a user-supplied endpoint without validation; add validation in the AnthropicChatClient(string apiKey, string modelName = ..., string? endpoint = null, ...) constructor by checking endpoint when non-null/whitespace (use Guard.NotNullOrWhiteSpace or equivalent) and verifying it parses as an absolute URI (e.g., Uri.TryCreate with UriKind.Absolute); if invalid, throw an ArgumentException (or use existing Guard helpers) with a clear message so malformed endpoints fail fast during construction rather than at request time.
139-142:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe past review comment regarding streaming usage reporting remains valid.
The streaming flow initializes
inputTokensandoutputTokensto0and always constructs aChatUsagein the final frame (lines 219-221), even when the provider never reported usage. This conflates "unknown usage" with "zero tokens consumed," which is semantically incorrect and misleads consumers about actual token consumption.Also applies to: 168-168, 211-211, 219-221
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Models/Connectors/AnthropicChatClient.cs` around lines 139 - 142, The streaming path currently initializes inputTokens/outputTokens to 0 and always attaches a ChatUsage at the end, conflating "unknown" with "zero"; change inputTokens and outputTokens to nullable ints (int?) or add a bool usageReported flag, update the streaming handlers that parse provider usage to set these nullable values or the flag when real usage arrives, and only construct/attach the ChatUsage object in the final frame when usageReported is true (or both nullable ints are non-null); update any references to inputTokens/outputTokens (and roleEmitted/ChatFinishReason handling) in the streaming method(s) in AnthropicChatClient.cs so consumers receive null/absent usage when the provider never reported it instead of 0.
396-406:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winThe past review comment regarding
ParseArgumentserror handling remains valid.Lines 402-405 catch
JsonExceptionand silently return an emptyJObject. As noted in the prior review, this can execute tools with incorrect/default arguments and masks provider or model defects. Per coding guidelines, catch blocks that swallow exceptions without logging are blocking production-readiness issues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Models/Connectors/AnthropicChatClient.cs` around lines 396 - 406, ParseArguments currently swallows JsonException and returns an empty JObject; instead either remove the try/catch so the JsonException can bubble up or catch JsonException, log the parsing error with contextual info and rethrow (or wrap and throw a descriptive exception) so callers don't proceed with default/incorrect args; update the ParseArguments method to stop silently swallowing exceptions and use the project's logging mechanism or rethrow the exception to surface provider/model parsing failures.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Agentic/Agents/AgentAsTool.cs`:
- Around line 59-71: Change the per-instance ParametersSchema property on
AgentAsTool<T> to a single shared immutable field: make ParametersSchema static
readonly (or static get-only) on the AgentAsTool<T> class so the JObject schema
is allocated once and reused across all instances; update any usages that
reference the instance property to use the static member name ParametersSchema
on AgentAsTool<T> and ensure initialization remains the same but is performed in
the static declaration.
In `@src/Agentic/Agents/AgentExecutor.cs`:
- Around line 50-51: Replace the verbose null/whitespace check in the
AgentExecutor.Name property with a consistent string helper: use
!string.IsNullOrWhiteSpace(_options.Name) to decide whether to return the
configured name or the fallback "agent"; keep referencing the same property
(Name) and field (_options.Name) when making the change for readability and
consistency with SupervisorAgent.
In `@src/Agentic/Agents/Swarm.cs`:
- Around line 94-97: Replace the verbose null/whitespace checks used to set Name
and Description with the more concise string.IsNullOrWhiteSpace pattern: instead
of `_options.Name is { } name && name.Trim().Length > 0` and
`_options.Description is { } description && description.Trim().Length > 0`, use
`!string.IsNullOrWhiteSpace(_options.Name)` and
`!string.IsNullOrWhiteSpace(_options.Description)` respectively in the
constructor (where Name and Description are assigned) so the logic is clearer
and consistent across Swarm.cs and similar locations.
- Around line 234-249: BuildHandoffMap currently constructs the dictionary with
StringComparer.OrdinalIgnoreCase but compares names using
StringComparison.Ordinal and also forces a ToList() allocation when
active.Handoffs is null; change the equality check inside the loop to use
StringComparison.OrdinalIgnoreCase to match the dictionary comparer, and avoid
the extra allocation by computing allowed as an IEnumerable (e.g. var allowed =
active.Handoffs ?? _members.Keys.Where(n => !string.Equals(n, active.Name,
StringComparison.OrdinalIgnoreCase))); keep ToolNaming.HandoffToolName(peer) and
SwarmMember<T>.Handoffs/_members references intact so the rest of the logic is
unchanged.
In `@src/Agentic/Models/Connectors/ChatClientBase.cs`:
- Around line 46-62: The constructor currently mutates a caller-owned
HttpClient's Timeout using the protected settable property TimeoutMs; change
this so we do not overwrite a provided HttpClient: make TimeoutMs immutable for
the instance (e.g., readonly/private set) and only apply HttpClient.Timeout when
the constructor creates a new HttpClient, or alternatively implement the
TimeoutMs setter to update the live HttpClient.Timeout (synchronizing state).
Locate the ChatClientBase constructor and the TimeoutMs property to either
remove the public setter or add synchronized logic that safely updates
HttpClient.Timeout on the owned instance without mutating caller-owned clients.
- Line 29: Change the public abstract ChatClientBase<T> class to internal to
narrow the connector surface: update the declaration from public abstract class
ChatClientBase<T> : IChatClient<T> to internal abstract class ChatClientBase<T>
: IChatClient<T>, and then update any dependent internal helpers/members
(constructors, protected/internal methods or properties) as needed so they
remain accessible to implementations in the same assembly; ensure any external
code that previously referenced ChatClientBase<T> instead uses the public facade
(IChatClient<T> or the concrete client types) and adjust unit tests or internal
factory code to reference the now-internal type within the assembly.
In `@src/Agentic/Tools/DelegateAgentTool.cs`:
- Around line 37-42: The DelegateAgentTool constructor must validate the
MethodInfo/target pairing upfront: ensure method is not null, then if
method.IsStatic accept a null or non-null target; if !method.IsStatic require a
non-null target; and if target is non-null verify method.DeclaringType (or
method.ReflectedType) is assignable from target.GetType() (use
Type.IsAssignableFrom) and throw an ArgumentException (or ArgumentNullException)
on failure so invalid combinations are rejected early instead of failing later
in _method.Invoke; update the constructor (which calls BuildSchema(method) and
assigns _method and _target) to perform these checks before setting fields or
proceeding.
- Around line 86-96: The code currently treats all reference-type parameters as
optional by assigning null when omitted; update the missing-argument check in
DelegateAgentTool to use nullability metadata instead: use
System.Reflection.NullabilityInfoContext (e.g. new
NullabilityInfoContext().Create(parameter).ReadState) to detect whether a
reference parameter is annotated as nullable before allowing callArgs[i] = null,
and only permit null when the parameter is explicitly nullable or has a default
value or is a nullable value type; otherwise return
ToolInvocationResult.Error($"Missing required parameter '{name}'.") as before.
- Around line 140-155: UnwrapResultAsync currently only handles Task and
therefore boxes ValueTask/ValueTask<T> results; update UnwrapResultAsync to
detect ValueTask and ValueTask<T> (e.g., check "returnValue is ValueTask vt" and
await vt for non-generic, and for generic use
AsTask()/GetType().GetProperty("Result") on the resulting Task) so awaited
results are returned, or alternatively add an explicit rejection in
AgentToolFactory.FromDelegate / ScanInstance for ValueTask return types with a
clear construction-time error; reference UnwrapResultAsync,
AgentToolSchemaGenerator, and AgentToolFactory.FromDelegate/ScanInstance when
making the change.
In `@src/Agentic/Tools/JsonSchemaGenerator.cs`:
- Line 24: Change the accessibility of the helper class JsonSchemaGenerator from
public to internal so it is not part of the public API surface; update the class
declaration for JsonSchemaGenerator accordingly and ensure no other public types
depend on it (adjust any internal callers if needed), keeping the schema
generation helper accessible only within the assembly.
In `@src/AiDotNet.Generators/AgentToolSchemaGenerator.cs`:
- Around line 291-350: TypeSchemaJson currently returns a generic
{"type":"object"} for complex CLR types which causes CreateAgentTools() to emit
less-detailed schemas than AgentToolFactory.ScanInstance() (which uses
JsonSchemaGenerator.ForType). Update TypeSchemaJson to recurse into complex
object types: when the type is not a primitive/enum/array/dictionary/enumerable,
enumerate its public readable properties (respecting Unwrap,
attributes/annotations, and nullable/required semantics), build a "properties"
object and "required" list, and produce a full JSON Schema for the object rather
than the bare fallback. Prefer delegating to or mirroring the logic in
JsonSchemaGenerator.ForType where possible so CreateAgentTools() and
ScanInstance() produce consistent schemas; keep existing handling for
arrays/dictionaries/enumerables via
Unwrap/TryGetDictionaryValueType/TryGetEnumerableElementType.
- Around line 167-169: The generated binder currently maps any missing arg to
default(T) which drops declared defaults and silently converts missing required
value types to 0/false; in AgentToolSchemaGenerator.cs update the code that
emits the three-line assignment (the block using local, Verbatim(p.Name), token
and pType) so it: 1) if the parameter has an explicit default value (use the
parameter metadata you already have for p), emit the default literal instead of
default(pType); 2) if the parameter is required and is a non-nullable value
type, emit code that treats a missing token as an error (e.g., throw an
ArgumentException or set a binding-failed path consistent with
DelegateAgentTool) rather than assigning default(pType); and 3) preserve the
existing nullable/reference type behavior. Adjust the emission logic to branch
on p.HasDefaultValue / p.IsOptional and on whether pType is a non-nullable value
type to implement these cases.
In `@src/Interfaces/IAiModelBuilder.cs`:
- Line 2338: Update the public docs and examples to reflect the new
IAiModelBuilder<T, TInput, TOutput> API by replacing usages of the removed
AgentConfiguration<T> and ConfigureAgentAssistance(...) with the new
agentic/chat-client entry point and the ConfigureRLAgent(IRLAgent<T> agent)
method; update the ConfigureReasoning example (previously referencing
AgentConfiguration<T> at lines ~1469-1478) to show constructing or obtaining an
IRLAgent<T> and passing it into IAiModelBuilder.ConfigureRLAgent, and add a
concise migration/release-note in the PR description documenting the breaking
change, the replaced symbols (AgentConfiguration<T>, ConfigureAgentAssistance)
and the new recommended pattern (IRLAgent<T> + ConfigureRLAgent) so downstream
consumers can migrate.
---
Duplicate comments:
In `@src/Agentic/Agents/SupervisorAgent.cs`:
- Around line 62-69: Replace the verbose null/empty checks in the
SupervisorAgent constructor (the assignments to Name, Description and
systemPrompt) with string.IsNullOrWhiteSpace-based conditions: use
string.IsNullOrWhiteSpace(settings.Name) ? "supervisor" : settings.Name.Trim()
for Name, similarly for Description with its default, and for systemPrompt use
string.IsNullOrWhiteSpace(settings.SystemPrompt) ?
BuildDefaultRoutingPrompt(workers) : settings.SystemPrompt.Trim(); update the
symbols Name, Description, and systemPrompt accordingly to keep trimming and
defaults.
In `@src/Agentic/Memory/EmbeddingAgentMemoryStore.cs`:
- Around line 103-110: GetAllAsync currently ignores the cancellationToken
parameter; at the start of the GetAllAsync method add
cancellationToken.ThrowIfCancellationRequested() before acquiring the lock
(preserving the existing lock(_gate) and the _entries.Select(...) logic) so the
synchronous fast path honors cancellation requests while leaving the rest of the
implementation unchanged.
- Around line 113-123: In RemoveAsync add cancellation support by calling
cancellationToken.ThrowIfCancellationRequested() immediately after
Guard.NotNullOrWhiteSpace(id) and before entering the lock so the method
respects the provided CancellationToken; update the method body around the Guard
and lock in EmbeddingAgentMemoryStore.RemoveAsync to perform the cancellation
check prior to modifying _entries.
- Around line 48-64: AddAsync currently ignores the cancellationToken; at the
start of the method (in AddAsync) call
cancellationToken.ThrowIfCancellationRequested() to honor early cancellation
before any work (e.g., Guard.NotNullOrWhiteSpace, generating id, calling
_embeddingModel.EmbedAsync). This ensures callers can cancel immediately even
though _embeddingModel.EmbedAsync doesn't accept a token.
- Around line 67-100: SearchAsync currently ignores the cancellationToken; add
cancellationToken.ThrowIfCancellationRequested() at the start of SearchAsync
(after Guard checks and before calling _embeddingModel.EmbedAsync) and also
inside the scoring loop in the foreach over snapshot (before calling
_similarity.Calculate and before adding to scored) so long-running searches can
be aborted promptly; keep existing logic (embedding await and ordering) but
sprinkle these token checks to promptly throw OperationCanceledException when
requested.
In `@src/Agentic/Memory/JsonFileConversationStore.cs`:
- Around line 135-139: The Save method currently writes JSON directly to
_filePath which can corrupt the file on crash; change Save(Dictionary<string,
List<StoredMessage>> store) to perform an atomic write by serializing using
SerializerSettings to a temporary file in the same directory (e.g., _filePath +
".tmp" or use Path.GetTempFileName in the same folder), flush and close the temp
file, then replace/rename the temp file to _filePath using an atomic operation
(File.Replace or File.Move with overwrite semantics) and ensure exceptions clean
up the temp file; update Save to handle IO exceptions and preserve original file
on failure.
- Around line 118-133: In Load(), wrap the JsonConvert.DeserializeObject call in
a try/catch that catches JsonException (and optionally Exception) when
reading/parsing the contents of _filePath; on error log the exception and file
path via your logger and return a new empty Dictionary<string,
List<StoredMessage>>(StringComparer.Ordinal) to avoid crashing; keep using
SerializerSettings and ensure the method still returns loaded when
deserialization succeeds.
- Around line 43-47: The constructor for JsonFileConversationStore currently
assigns an attacker-controlled filePath to _filePath after only checking
NotNullOrWhiteSpace; to fix, canonicalize and validate the path against a safe
base directory (e.g., compute Path.GetFullPath(filePath) and ensure it starts
with the canonical base directory or is a child of a configured allowed
directory) before assigning to _filePath, and throw an exception if the check
fails; consider injecting or defining a constant for the allowed base directory,
reject relative/parent-traversal segments, and preserve the
Guard.NotNullOrWhiteSpace check in the constructor.
In `@src/Agentic/Models/Connectors/AnthropicChatClient.cs`:
- Around line 49-64: The AnthropicChatClient constructor stores a user-supplied
endpoint without validation; add validation in the AnthropicChatClient(string
apiKey, string modelName = ..., string? endpoint = null, ...) constructor by
checking endpoint when non-null/whitespace (use Guard.NotNullOrWhiteSpace or
equivalent) and verifying it parses as an absolute URI (e.g., Uri.TryCreate with
UriKind.Absolute); if invalid, throw an ArgumentException (or use existing Guard
helpers) with a clear message so malformed endpoints fail fast during
construction rather than at request time.
- Around line 139-142: The streaming path currently initializes
inputTokens/outputTokens to 0 and always attaches a ChatUsage at the end,
conflating "unknown" with "zero"; change inputTokens and outputTokens to
nullable ints (int?) or add a bool usageReported flag, update the streaming
handlers that parse provider usage to set these nullable values or the flag when
real usage arrives, and only construct/attach the ChatUsage object in the final
frame when usageReported is true (or both nullable ints are non-null); update
any references to inputTokens/outputTokens (and roleEmitted/ChatFinishReason
handling) in the streaming method(s) in AnthropicChatClient.cs so consumers
receive null/absent usage when the provider never reported it instead of 0.
- Around line 396-406: ParseArguments currently swallows JsonException and
returns an empty JObject; instead either remove the try/catch so the
JsonException can bubble up or catch JsonException, log the parsing error with
contextual info and rethrow (or wrap and throw a descriptive exception) so
callers don't proceed with default/incorrect args; update the ParseArguments
method to stop silently swallowing exceptions and use the project's logging
mechanism or rethrow the exception to surface provider/model parsing failures.
In `@src/Agentic/Models/Connectors/MeaiChatClient.cs`:
- Around line 101-116: The ToMeaiMessages method must validate inputs and each
message element before dereferencing: add a null-check for the messages
parameter and throw ArgumentNullException if null, then inside the loop validate
that each ChatMessage (the loop variable message) is not null and that
message.Contents is not null (throw ArgumentNullException/ArgumentException with
clear param names), and also ensure message.Role and message.Text are present
(or non-null) before using them; after these checks continue with the existing
validation for ToolCallContent/ToolResultContent/ImageContent and constructing
new Meai.ChatMessage. This ensures ToMeaiMessages fails fast with clear
exceptions instead of raising NullReferenceException.
In `@src/AiDotNet.Storage.Sqlite/SqliteConversationStore.cs`:
- Line 84: Replace the synchronous transaction.Commit() call with an
asynchronous commit to avoid blocking: in the method where transaction.Commit()
is called (the transaction variable in SqliteConversationStore), change it to
await transaction.CommitAsync(cancellationToken).ConfigureAwait(false); ensure
the surrounding method remains async and that a CancellationToken named
cancellationToken (or the method's token) is passed through to CommitAsync.
- Around line 148-159: The EnsureSchemaAsync method currently runs the CREATE
TABLE/INDEX DDL on every call which adds unnecessary overhead; modify
SqliteConversationStore to run EnsureSchemaAsync only once per connection
lifecycle by adding a private static or instance-level volatile/atomic boolean
(e.g., _schemaEnsured or _schemaEnsuredForConnection) that is checked at the
start of EnsureSchemaAsync (or before calling it from methods that open the
connection) and set to true after a successful schema creation; keep the
existing SQL and error handling but skip executing the command when the flag is
true to avoid repeated DDL calls while preserving safety.
- Line 161: Replace the unsafe Enum.Parse call in the ParseRole helper with
defensive parsing: update ParseRole(string role) to use
Enum.TryParse<ChatRole>(...) (optionally ignore case), and if parsing fails
return a safe default (e.g., ChatRole.User) and emit a warning/log so callers
like GetAsync can continue instead of throwing; ensure all call sites (ParseRole
and GetAsync) rely on the new safe behavior rather than propagating exceptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 709a4278-3eb4-4d80-99a3-6b882f3986d3
📒 Files selected for processing (158)
Directory.Packages.propssrc/Agentic/Agents/AgentAsTool.cssrc/Agentic/Agents/AgentExecutor.cssrc/Agentic/Agents/AgentExecutorOptions.cssrc/Agentic/Agents/AgentRunResult.cssrc/Agentic/Agents/IAgent.cssrc/Agentic/Agents/SupervisorAgent.cssrc/Agentic/Agents/SupervisorOptions.cssrc/Agentic/Agents/Swarm.cssrc/Agentic/Agents/SwarmMember.cssrc/Agentic/Agents/SwarmOptions.cssrc/Agentic/Agents/ToolNaming.cssrc/Agentic/Memory/AgentMemory.cssrc/Agentic/Memory/EmbeddingAgentMemoryStore.cssrc/Agentic/Memory/IAgentMemoryStore.cssrc/Agentic/Memory/IConversationStore.cssrc/Agentic/Memory/InMemoryAgentMemoryStore.cssrc/Agentic/Memory/InMemoryConversationStore.cssrc/Agentic/Memory/JsonFileConversationStore.cssrc/Agentic/Memory/MemoryAugmentationOptions.cssrc/Agentic/Memory/MemoryAugmentedAgent.cssrc/Agentic/Memory/ScoredMemory.cssrc/Agentic/Memory/ThreadedAgent.cssrc/Agentic/Models/AiContent.cssrc/Agentic/Models/AiToolDefinition.cssrc/Agentic/Models/ChatClientExtensions.cssrc/Agentic/Models/ChatFinishReason.cssrc/Agentic/Models/ChatMessage.cssrc/Agentic/Models/ChatOptions.cssrc/Agentic/Models/ChatResponse.cssrc/Agentic/Models/ChatResponseFormatKind.cssrc/Agentic/Models/ChatResponseUpdate.cssrc/Agentic/Models/ChatRole.cssrc/Agentic/Models/ChatUsage.cssrc/Agentic/Models/Connectors/AnthropicChatClient.cssrc/Agentic/Models/Connectors/AzureOpenAIChatClient.cssrc/Agentic/Models/Connectors/ChatClientBase.cssrc/Agentic/Models/Connectors/MeaiChatClient.cssrc/Agentic/Models/Connectors/OpenAIChatClient.cssrc/Agentic/Models/IChatClient.cssrc/Agentic/Models/ImageContent.cssrc/Agentic/Models/ImageMediaType.cssrc/Agentic/Models/ImageMediaTypeExtensions.cssrc/Agentic/Models/StreamingToolCallUpdate.cssrc/Agentic/Models/TextContent.cssrc/Agentic/Models/ToolCallContent.cssrc/Agentic/Models/ToolChoiceMode.cssrc/Agentic/Models/ToolResultContent.cssrc/Agentic/Tools/AgentToolAttribute.cssrc/Agentic/Tools/AgentToolBase.cssrc/Agentic/Tools/AgentToolFactory.cssrc/Agentic/Tools/DelegateAgentTool.cssrc/Agentic/Tools/FunctionAgentTool.cssrc/Agentic/Tools/IAgentTool.cssrc/Agentic/Tools/JsonSchemaGenerator.cssrc/Agentic/Tools/StructuredOutputExtensions.cssrc/Agentic/Tools/ToolCollection.cssrc/Agentic/Tools/ToolInvocationResult.cssrc/Agentic/Tools/ToolParameterAttribute.cssrc/Agents/Agent.cssrc/Agents/AgentBase.cssrc/Agents/AgentGlobalConfiguration.cssrc/Agents/AgentHyperparameterApplicator.cssrc/Agents/AgentKeyResolver.cssrc/Agents/ChainOfThoughtAgent.cssrc/Agents/HyperparameterDefinition.cssrc/Agents/HyperparameterRegistry.cssrc/Agents/HyperparameterResponseParser.cssrc/Agents/HyperparameterValidationResult.cssrc/Agents/PlanAndExecuteAgent.cssrc/Agents/RAGAgent.cssrc/Agents/README.mdsrc/AiDotNet.Generators/AgentToolSchemaGenerator.cssrc/AiDotNet.Storage.Sqlite/SqliteConversationStore.cssrc/AiDotNet.csprojsrc/AiModelBuilder.BuildPipeline.cssrc/AiModelBuilder.Configure.cssrc/AiModelBuilder.Coverage.cssrc/AiModelBuilder.DomainML.cssrc/AiModelBuilder.Internals.cssrc/AiModelBuilder.Workflows.cssrc/AiModelBuilder.cssrc/Benchmarking/BenchmarkRunner.cssrc/Configuration/IConfiguredView.cssrc/Interfaces/IAgent.cssrc/Interfaces/IAiModelBuilder.cssrc/Interfaces/IChain.cssrc/Interfaces/IChatModel.cssrc/Interfaces/IFunctionTool.cssrc/Interfaces/ILanguageModel.cssrc/Interfaces/IPromptChain.cssrc/Interfaces/ITool.cssrc/LanguageModels/AnthropicChatModel.cssrc/LanguageModels/AzureOpenAIChatModel.cssrc/LanguageModels/ChatModelBase.cssrc/LanguageModels/Models/OpenAIChoice.cssrc/LanguageModels/Models/OpenAIMessage.cssrc/LanguageModels/Models/OpenAIRequest.cssrc/LanguageModels/Models/OpenAIResponse.cssrc/LanguageModels/Models/OpenAIUsage.cssrc/LanguageModels/OpenAIChatModel.cssrc/LanguageModels/README.mdsrc/Models/AgentAssistanceOptions.cssrc/Models/AgentAssistanceOptionsBuilder.cssrc/Models/AgentConfiguration.cssrc/Models/AgentGlobalConfigurationBuilder.cssrc/Models/AgentRecommendation.cssrc/Models/Options/AiModelResultOptions.cssrc/Models/Results/AiModelResult.cssrc/PromptEngineering/Chains/ChainBase.cssrc/PromptEngineering/Chains/ConditionalChain.cssrc/PromptEngineering/Chains/LoopChain.cssrc/PromptEngineering/Chains/MapReduceChain.cssrc/PromptEngineering/Chains/ParallelChain.cssrc/PromptEngineering/Chains/RouterChain.cssrc/PromptEngineering/Chains/SequentialChain.cssrc/PromptEngineering/Tools/FunctionToolBase.cssrc/PromptEngineering/Tools/ToolRegistry.cssrc/Reasoning/Components/ContradictionDetector.cssrc/Reasoning/Components/ThoughtEvaluator.cssrc/Reasoning/Components/ThoughtGenerator.cssrc/Reasoning/DomainSpecific/CodeReasoner.cssrc/Reasoning/DomainSpecific/LogicalReasoner.cssrc/Reasoning/DomainSpecific/MathematicalReasoner.cssrc/Reasoning/DomainSpecific/ScientificReasoner.cssrc/Reasoning/Reasoner.cssrc/Reasoning/ReasoningStrategyBase.cssrc/Reasoning/Strategies/ChainOfThoughtStrategy.cssrc/Reasoning/Strategies/SelfConsistencyStrategy.cssrc/Reasoning/Strategies/TreeOfThoughtsStrategy.cssrc/Reasoning/Training/PolicyGradientTrainer.cssrc/Reasoning/Training/ReinforcementLearner.cssrc/Reasoning/Verification/CriticModel.cssrc/Reasoning/Verification/OutcomeRewardModel.cssrc/Reasoning/Verification/ProcessRewardModel.cssrc/Reasoning/Verification/SelfRefinementEngine.cssrc/Tools/AiModelTool.cssrc/Tools/BingSearchResponse.cssrc/Tools/BingWebPage.cssrc/Tools/BingWebPages.cssrc/Tools/CalculatorTool.cssrc/Tools/CrossValidationTool.cssrc/Tools/DataAnalysisTool.cssrc/Tools/DiffusionTools.cssrc/Tools/FeatureImportanceTool.cssrc/Tools/HyperparameterTool.cssrc/Tools/ModelSelectionTool.cssrc/Tools/RAGTool.cssrc/Tools/RegularizationTool.cssrc/Tools/SearchProvider.cssrc/Tools/SearchTool.cssrc/Tools/SerpAPIResponse.cssrc/Tools/SerpAPIResult.cssrc/Tools/ThreeDHyperparameterTool.cssrc/Tools/ThreeDModelSelectionTool.cssrc/Tools/ToolBase.cssrc/Tools/VectorSearchTool.cssrc/Tools/WebSearchTool.cs
💤 Files with no reviewable changes (76)
- src/LanguageModels/Models/OpenAIUsage.cs
- src/Reasoning/Verification/ProcessRewardModel.cs
- src/Interfaces/ITool.cs
- src/Interfaces/IPromptChain.cs
- src/PromptEngineering/Tools/FunctionToolBase.cs
- src/Agents/HyperparameterDefinition.cs
- src/Reasoning/DomainSpecific/ScientificReasoner.cs
- src/PromptEngineering/Tools/ToolRegistry.cs
- src/Reasoning/Components/ContradictionDetector.cs
- src/Reasoning/DomainSpecific/CodeReasoner.cs
- src/Configuration/IConfiguredView.cs
- src/Agents/HyperparameterValidationResult.cs
- src/Reasoning/Strategies/ChainOfThoughtStrategy.cs
- src/LanguageModels/Models/OpenAIChoice.cs
- src/Reasoning/Verification/SelfRefinementEngine.cs
- src/Reasoning/Training/PolicyGradientTrainer.cs
- src/Reasoning/Verification/OutcomeRewardModel.cs
- src/Tools/CrossValidationTool.cs
- src/PromptEngineering/Chains/ConditionalChain.cs
- src/Agents/RAGAgent.cs
- src/Interfaces/IChain.cs
- src/Agents/HyperparameterRegistry.cs
- src/Models/AgentConfiguration.cs
- src/Agents/AgentKeyResolver.cs
- src/Interfaces/IAgent.cs
- src/Agents/AgentGlobalConfiguration.cs
- src/LanguageModels/Models/OpenAIMessage.cs
- src/Agents/AgentBase.cs
- src/PromptEngineering/Chains/SequentialChain.cs
- src/Reasoning/Verification/CriticModel.cs
- src/Agents/ChainOfThoughtAgent.cs
- src/Agents/Agent.cs
- src/Reasoning/DomainSpecific/MathematicalReasoner.cs
- src/Interfaces/IFunctionTool.cs
- src/LanguageModels/Models/OpenAIResponse.cs
- src/Agents/AgentHyperparameterApplicator.cs
- src/Models/AgentRecommendation.cs
- src/Tools/BingSearchResponse.cs
- src/Tools/CalculatorTool.cs
- src/Tools/BingWebPage.cs
- src/LanguageModels/Models/OpenAIRequest.cs
- src/PromptEngineering/Chains/LoopChain.cs
- src/Reasoning/DomainSpecific/LogicalReasoner.cs
- src/Models/AgentAssistanceOptions.cs
- src/Reasoning/Strategies/TreeOfThoughtsStrategy.cs
- src/Tools/BingWebPages.cs
- src/Models/AgentGlobalConfigurationBuilder.cs
- src/PromptEngineering/Chains/ChainBase.cs
- src/Tools/AiModelTool.cs
- src/PromptEngineering/Chains/MapReduceChain.cs
- src/LanguageModels/ChatModelBase.cs
- src/LanguageModels/README.md
- src/Models/AgentAssistanceOptionsBuilder.cs
- src/LanguageModels/AnthropicChatModel.cs
- src/Agents/HyperparameterResponseParser.cs
- src/Interfaces/ILanguageModel.cs
- src/Models/Options/AiModelResultOptions.cs
- src/Interfaces/IChatModel.cs
- src/Reasoning/Reasoner.cs
- src/PromptEngineering/Chains/RouterChain.cs
- src/Agents/README.md
- src/Reasoning/Strategies/SelfConsistencyStrategy.cs
- src/AiModelBuilder.BuildPipeline.cs
- src/Agents/PlanAndExecuteAgent.cs
- src/AiModelBuilder.Workflows.cs
- src/LanguageModels/OpenAIChatModel.cs
- src/PromptEngineering/Chains/ParallelChain.cs
- src/LanguageModels/AzureOpenAIChatModel.cs
- src/Reasoning/Training/ReinforcementLearner.cs
- src/Reasoning/ReasoningStrategyBase.cs
- src/Reasoning/Components/ThoughtGenerator.cs
- src/Reasoning/Components/ThoughtEvaluator.cs
- src/AiModelBuilder.cs
- src/AiModelBuilder.Internals.cs
- src/AiModelBuilder.Configure.cs
- src/Models/Results/AiModelResult.cs
…1544) Foundational single-model agent for the multi-agent layer: - IAgent<T>: uniform named/described/RunAsync contract that both leaf agents and coordinators (supervisor/swarm) implement, so agents nest and any agent can be surfaced as a tool to another. - AgentExecutor<T>: drives an IChatClient<T> in a provider-native function-calling loop (call model -> run requested tools -> feed ToolResult messages back -> repeat) until a final answer or the iteration cap. Replaces the legacy prompt-parsed ReAct loop. Aggregates token usage; returns the full transcript. - AgentExecutorOptions (nullable defaults: name/description/system-prompt/max-iterations/sampling/tool-choice) + AgentRunResult (FinalText/Messages/Iterations/Completed/Usage). - Tests (10, green net10.0 + net471) via a deterministic ScriptedChatClient + RecordingTool: plain answer, tool round-trip with arg + result-passthrough assertions, system-prompt prepend, tool advertisement on/off, iteration-cap -> Completed=false, usage aggregation, empty/null guards. No null-forgiving. Stacked on Phase 0 (#1545); part of epic #1544 (Phase 2). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…doffs (#1544) - AgentAsTool<T>: adapts any IAgent<T> into an IAgentTool (transfer_to_<worker>, single required 'task' arg). Worker name is sanitized to a valid tool name; the worker's final answer becomes the tool result. Surfaces partial output as an error when a worker hits its step budget. - SupervisorAgent<T> (itself an IAgent<T>, so supervisors nest into hierarchical teams): wraps each worker as a handoff tool and delegates routing to AgentExecutor's native tool-calling loop — no bespoke control flow. Auto-generates a routing system prompt listing workers + specialties when none is supplied. - SupervisorOptions (nullable defaults: name/description/system-prompt/max-iterations/temperature). - 6 tests (green net10.0 + net471): single-worker route + result-passthrough, correct-worker-among-many, one-handoff-tool-per-worker advertisement, default-prompt lists workers, nested supervisors compose, empty-team guard. No null-forgiving. Part of epic #1544 (Phase 2). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Swarm<T> (an IAgent<T>): peer-to-peer team with no central coordinator. The active member answers directly or transfers the whole shared conversation to a peer via a native transfer_to_<peer> tool, which the swarm intercepts at the loop level (switch active member + re-run the turn with that member's instructions/tools) rather than executing. Members' own tools run normally. - SwarmMember<T>: name + client + per-member system prompt + own tools + allowed handoff peers (null = any peer, empty = terminal). SwarmOptions: name/description/shared MaxIterations budget (guarantees termination on ping-pong) /temperature. - Each turn prepends the *active* member's system prompt to the shared history; handoff tools are advertised only for allowed peers. - AgentRunResult gains AgentName (which agent produced the final answer); AgentExecutor now reports its own name. - ToolNaming helper centralizes handoff tool-name sanitization (shared with AgentAsTool). - 8 tests (green net10.0 + net471): direct answer, handoff-over-shared-history (+active system prompt), own-tool execution, allowed-peers-only advertisement, ping-pong terminates at cap, unknown-entry/unknown-peer/duplicate-name guards. No null-forgiving. Part of epic #1544 (Phase 2). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
) - IConversationStore: per-thread dialogue persistence (Append/Get/ListThreads/Clear). - InMemoryConversationStore (zero-config, process-local) + JsonFileConversationStore (durable across restarts; persists role+text dialogue via Newtonsoft, in-process lock, no native dependency so it runs on both TFMs). - ThreadedAgent<T>: wraps any IAgent<T> with conversation memory — loads a thread's history, prepends it to new input, runs the inner agent, then persists the new user+assistant turn. Turns a stateless agent (executor/supervisor/swarm) into a stateful chat. Persists clean user/assistant dialogue; intermediate tool/system turns stay on the per-turn AgentRunResult. - 8 tests (green net10.0 + net471): cross-turn memory within a thread, thread isolation, empty-id guard, in-memory round-trip + unknown-thread-empty, JSON-file persists-across-instances + threaded-agent-survives-new-instance + clear. No null-forgiving. Part of epic #1544 (Phase 2). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ion store (#1544) Long-term cross-thread memory (the durable counterpart to per-thread IConversationStore): - IAgentMemoryStore (AddAsync/SearchAsync/GetAllAsync/RemoveAsync) + AgentMemory/ScoredMemory. - InMemoryAgentMemoryStore: lexical-overlap ranking (zero-config, no model). - EmbeddingAgentMemoryStore<T>: TRUE semantic recall — embeds memories+query via IEmbeddingModel<T> and ranks by the RAG stack's ISimilarityMetric<T> (cosine by default), reusing existing RAG infrastructure. Drop-in over the lexical default (same interface). - MemoryAugmentedAgent<T> (IAgent<T>): retrieves memories relevant to the latest user message and injects them as system context before delegating to the inner agent (executor/supervisor/swarm). Read-only by design — what gets remembered stays under app control. Composes with ThreadedAgent (long-term recall + short-term thread memory). Durable conversation backend: - SqliteConversationStore (AiDotNet.Storage.Sqlite): DB-backed IConversationStore (auto-incrementing Seq preserves order, schema created on demand), keeping the native SQLite dep out of core. Mirrors the SQLite graph checkpointer. - 12 tests (9 memory green net10.0+net471; 3 SQLite conversation net10-gated for the e_sqlite3 native-lib reason — pre-existing repo-wide). No null-forgiving. Part of epic #1544 (Phase 2). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ooples
force-pushed
the
feature/agentic-phase2-multi-agent
branch
from
978191c to
dcb57b9
Compare
ooples
pushed a commit
that referenced
this pull request
Jun 11, 2026
The flagship local-first differentiator: an IChatClient<T> that runs entirely in-process over AiDotNet's own model — no network, no API key. Same agent code drives it as drives OpenAI/Anthropic. - ICausalLanguageModel<T>: minimal next-token-logits seam the real Transformer (or any model) plugs into; implementations may keep an internal KV-cache. - IGenerationTokenizer (+TokenizerGenerationAdapter over the repo's ITokenizer): encode/decode/EOS seam. - TokenSampler<T>: greedy (temp 0 = argmax) + temperature + top-k + top-p (nucleus), seedable for reproducibility; reads logits via Convert.ToDouble so float/double share one path. - IChatPromptTemplate + ChatMlPromptTemplate (role-tagged, opens the assistant turn). - LocalEngineChatClient<T>: renders prompt -> encodes -> autoregressively samples until EOS or token limit -> decodes; non-streaming + streaming (incremental decode deltas); usage = prompt/generated token counts. Drop-in for agents/supervisor/swarm/memory. (Native tool-calling + structured-output constraints via constrained decoding are a follow-up; tools are ignored this slice.) - 9 tests (green net10.0 + net471) on a deterministic ScriptedCausalModel + WordTokenizer: greedy decode + EOS stop, token-limit -> Length, streaming deltas reconstruct full text, AgentExecutor drop-in, sampler argmax/seed-reproducibility/top-k/top-p, ChatML template. No null-forgiving. Stacked on Phase 2 (#1551) — the integration test shows the local engine driving an AgentExecutor. Part of epic #1544 (Phase 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ooples
pushed a commit
that referenced
this pull request
Jun 11, 2026
The flagship local-first differentiator: an IChatClient<T> that runs entirely in-process over AiDotNet's own model — no network, no API key. Same agent code drives it as drives OpenAI/Anthropic. - ICausalLanguageModel<T>: minimal next-token-logits seam the real Transformer (or any model) plugs into; implementations may keep an internal KV-cache. - IGenerationTokenizer (+TokenizerGenerationAdapter over the repo's ITokenizer): encode/decode/EOS seam. - TokenSampler<T>: greedy (temp 0 = argmax) + temperature + top-k + top-p (nucleus), seedable for reproducibility; reads logits via Convert.ToDouble so float/double share one path. - IChatPromptTemplate + ChatMlPromptTemplate (role-tagged, opens the assistant turn). - LocalEngineChatClient<T>: renders prompt -> encodes -> autoregressively samples until EOS or token limit -> decodes; non-streaming + streaming (incremental decode deltas); usage = prompt/generated token counts. Drop-in for agents/supervisor/swarm/memory. (Native tool-calling + structured-output constraints via constrained decoding are a follow-up; tools are ignored this slice.) - 9 tests (green net10.0 + net471) on a deterministic ScriptedCausalModel + WordTokenizer: greedy decode + EOS stop, token-limit -> Length, streaming deltas reconstruct full text, AgentExecutor drop-in, sampler argmax/seed-reproducibility/top-k/top-p, ChatML template. No null-forgiving. Stacked on Phase 2 (#1551) — the integration test shows the local engine driving an AgentExecutor. Part of epic #1544 (Phase 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ooples
pushed a commit
that referenced
this pull request
Jun 11, 2026
The flagship local-first differentiator: an IChatClient<T> that runs entirely in-process over AiDotNet's own model — no network, no API key. Same agent code drives it as drives OpenAI/Anthropic. - ICausalLanguageModel<T>: minimal next-token-logits seam the real Transformer (or any model) plugs into; implementations may keep an internal KV-cache. - IGenerationTokenizer (+TokenizerGenerationAdapter over the repo's ITokenizer): encode/decode/EOS seam. - TokenSampler<T>: greedy (temp 0 = argmax) + temperature + top-k + top-p (nucleus), seedable for reproducibility; reads logits via Convert.ToDouble so float/double share one path. - IChatPromptTemplate + ChatMlPromptTemplate (role-tagged, opens the assistant turn). - LocalEngineChatClient<T>: renders prompt -> encodes -> autoregressively samples until EOS or token limit -> decodes; non-streaming + streaming (incremental decode deltas); usage = prompt/generated token counts. Drop-in for agents/supervisor/swarm/memory. (Native tool-calling + structured-output constraints via constrained decoding are a follow-up; tools are ignored this slice.) - 9 tests (green net10.0 + net471) on a deterministic ScriptedCausalModel + WordTokenizer: greedy decode + EOS stop, token-limit -> Length, streaming deltas reconstruct full text, AgentExecutor drop-in, sampler argmax/seed-reproducibility/top-k/top-p, ChatML template. No null-forgiving. Stacked on Phase 2 (#1551) — the integration test shows the local engine driving an AgentExecutor. Part of epic #1544 (Phase 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
src/Agentic/Agents/AgentExecutor.cs (1)
50-51: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winInconsistent null/whitespace checking pattern across agent implementations.
Both
AgentExecutorandSupervisorAgentuse the verbose patternis { } x && x.Trim().Length > 0for string validation. For consistency and readability across the agentic subsystem, prefer!string.IsNullOrWhiteSpace(...)in all locations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Agents/AgentExecutor.cs` around lines 50 - 51, Replace the verbose null/whitespace check in the AgentExecutor Name property with the standard helper: change the expression that reads "_options.Name is { } name && name.Trim().Length > 0 ? name : \"agent\"" to use "!string.IsNullOrWhiteSpace(_options.Name) ? _options.Name : \"agent\"" (also update the equivalent pattern in SupervisorAgent to the same !string.IsNullOrWhiteSpace(...) form) so both AgentExecutor.Name and SupervisorAgent use consistent, readable string validation.src/Agentic/Memory/JsonFileConversationStore.cs (3)
135-139:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBLOCKING: Data loss risk from non-atomic writes.
Line 138 writes directly to
_filePath. If the process crashes during write, the file may be corrupted, losing all conversation history. Production-ready persistence requires atomic write-to-temp-then-rename.💾 Atomic write implementation
private void Save(Dictionary<string, List<StoredMessage>> store) { var json = JsonConvert.SerializeObject(store, SerializerSettings); - File.WriteAllText(_filePath, json); + + var tempPath = _filePath + ".tmp"; + File.WriteAllText(tempPath, json); + File.Move(tempPath, _filePath, overwrite: true); }Note:
File.Movewith overwrite is atomic on most file systems.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Memory/JsonFileConversationStore.cs` around lines 135 - 139, The Save method currently writes JSON directly to _filePath which risks corruption if interrupted; change Save(Dictionary<string, List<StoredMessage>> store) to perform an atomic write by serializing to a temp file in the same directory (use a unique temp name based on _filePath), write the JSON to that temp file, flush/close it, then replace or move the temp file over _filePath using an atomic operation (File.Replace or File.Move with overwrite) so the switch is atomic; keep using SerializerSettings and preserve exception handling as needed.
118-133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBLOCKING: Missing error handling for JSON deserialization.
Line 131's
JsonConvert.DeserializeObjecthas no try/catch. Malformed JSON (from file corruption or manual editing) will throwJsonException, crashing the operation. Production code must handle deserialization failures.🛡️ Defensive deserialization
private Dictionary<string, List<StoredMessage>> Load() { if (!File.Exists(_filePath)) { return new Dictionary<string, List<StoredMessage>>(StringComparer.Ordinal); } var json = File.ReadAllText(_filePath); if (json.Trim().Length == 0) { return new Dictionary<string, List<StoredMessage>>(StringComparer.Ordinal); } + try + { var loaded = JsonConvert.DeserializeObject<Dictionary<string, List<StoredMessage>>>(json, SerializerSettings); return loaded ?? new Dictionary<string, List<StoredMessage>>(StringComparer.Ordinal); + } + catch (JsonException ex) + { + throw new InvalidOperationException( + $"Failed to load conversation store from '{_filePath}'. The file may be corrupted.", ex); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Memory/JsonFileConversationStore.cs` around lines 118 - 133, The Load() method currently calls JsonConvert.DeserializeObject without error handling; wrap the deserialization in a try/catch that catches JsonException (and optionally Exception) around the JsonConvert.DeserializeObject(...) call in JsonFileConversationStore.Load(), and on failure return a new Dictionary<string, List<StoredMessage>>(StringComparer.Ordinal) instead of letting the exception propagate; optionally move or rename the corrupted _filePath (e.g., append ".corrupt" + timestamp) and/or log the error so callers can recover safely while preserving SerializerSettings usage.
43-47:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winBLOCKING: Path traversal vulnerability requires immediate fix.
The constructor accepts an arbitrary
filePathwithout validation or canonicalization, enabling an attacker-controlled path to read/write anywhere the process has access. Production-ready code must validate paths against a safe base directory.🔒 Mitigation approach
Define a fixed base directory (e.g., via configuration), canonicalize the incoming path with
Path.GetFullPath, and verify it starts with the base directory. Alternatively, accept only a filename and enforce a fixed base directory.+private static readonly string BaseDirectory = + Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "Conversations"); + public JsonFileConversationStore(string filePath) { Guard.NotNullOrWhiteSpace(filePath); - _filePath = filePath; + + var fullPath = Path.GetFullPath(filePath); + var baseDir = Path.GetFullPath(BaseDirectory); + if (!fullPath.StartsWith(baseDir, StringComparison.OrdinalIgnoreCase)) + { + throw new ArgumentException( + $"File path must be within conversation directory: {baseDir}", + nameof(filePath)); + } + + Directory.CreateDirectory(baseDir); + _filePath = fullPath; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Memory/JsonFileConversationStore.cs` around lines 43 - 47, The JsonFileConversationStore constructor currently assigns an unvalidated filePath to _filePath, introducing path traversal risk; change it to require/obtain a configured base directory (e.g., from options), canonicalize the incoming path with Path.GetFullPath, and verify the canonicalized path starts with the canonicalized base directory before assigning to _filePath (or alternatively accept only a filename and combine it with the fixed base directory using Path.Combine then canonicalize/validate); on validation failure throw an ArgumentException or SecurityException. Ensure the checks are performed in the JsonFileConversationStore(string filePath) constructor (or a factory) and reuse the same canonicalization logic for any subsequent file accesses.src/AiDotNet.Storage.Sqlite/SqliteConversationStore.cs (1)
27-175:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winProduction-ready implementation with one data integrity concern.
The core implementation is complete with proper validation, parameterized queries, and transaction handling. However, line 161's
Enum.Parsewill crash on invalid stored role values (e.g., from data corruption).🛡️ Defensive enum parsing
-private static ChatRole ParseRole(string role) => (ChatRole)Enum.Parse(typeof(ChatRole), role); +private static ChatRole ParseRole(string role) +{ + if (Enum.TryParse<ChatRole>(role, out var result)) + { + return result; + } + throw new InvalidOperationException($"Invalid role value in database: '{role}'"); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/AiDotNet.Storage.Sqlite/SqliteConversationStore.cs` around lines 27 - 175, The ParseRole helper currently calls Enum.Parse which will throw on bad data; change ParseRole(string role) to use Enum.TryParse<ChatRole>(role, out var parsed) and if TryParse returns true return parsed, otherwise throw a clear exception (e.g., InvalidDataException or FormatException) with context including the invalid role value and the thread id/sequence where available (ParseRole is used from GetAsync), ensuring callers receive a descriptive error instead of a generic crash; update ParseRole implementation and any unit tests that assume strict parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Agentic/Agents/AgentAsTool.cs`:
- Around line 30-33: Make the adapter implementation AgentAsTool<T> internal
instead of public so it is hidden from the public API surface; update the class
declaration for AgentAsTool<T> (and keep its constant DefaultNamePrefix and
IAgentTool implementation) to internal sealed to restrict visibility, and ensure
any internal callers or factory methods in the same assembly still reference it
(adjust their access if needed) so external consumers only see the intended
facade types.
- Around line 77-85: Validate that the incoming "task" is a JSON string token
before casting or using it: in InvokeAsync, fetch the JToken via
arguments["task"] and check token != null and token.Type == JTokenType.String,
then get its string via token.Value<string>() (or similar) and ensure it's not
empty/whitespace; if any check fails return ToolInvocationResult.Error($"The
'{Name}' handoff requires a non-empty 'task' string argument."); do not use a
direct cast like (string?)arguments["task"] which can throw on non-string JSON.
In `@src/Agentic/Agents/Swarm.cs`:
- Around line 132-141: The code currently filters out all ChatRole.System
messages in RunAsync and similar block (var shared = new List<ChatMessage>...
foreach (var message in messages) if (message.Role != ChatRole.System)
shared.Add(message);) which discards any caller-supplied system prompts; instead
preserve incoming system messages by copying all messages into shared (do not
skip ChatRole.System) and when preparing the per-turn active member prompt (in
BuildRequestMessages or wherever the active member system message is prepended),
insert the active member system message in addition to—but not
replacing—existing system messages so seeded context remains; apply the same
change to the other occurrence around lines 272-281.
- Around line 56-66: The constructor currently only checks raw member.Name
uniqueness but must also reject members whose transfer tool keys collide when
normalized by ToolNaming.HandoffToolName (used by BuildHandoffMap); update the
loop that builds _members to compute the normalized handoff key for each member
via ToolNaming.HandoffToolName(member) (or the correct overload that accepts the
member/peer) and maintain a HashSet<string> of these normalized names (using the
same comparer/normalization ToolNaming uses); if the normalized key already
exists (or collides with an existing member), throw an ArgumentException
indicating a duplicate handoff tool name so BuildHandoffMap cannot silently
overwrite peers.
In `@src/Agentic/Agents/SwarmMember.cs`:
- Around line 39-55: The constructor SwarmMember(string name, IChatClient<T>
client, ..., IReadOnlyList<string>? handoffs) currently stores the caller-owned
handoffs list by reference and doesn't validate elements; update the SwarmMember
constructor to validate that every entry in handoffs is non-null/non-whitespace
(use Guard or equivalent) and reject or throw on invalid entries, then copy the
list into an immutable snapshot (e.g., a read-only/immutable collection) before
assigning to the Handoffs property so downstream code cannot observe external
mutations.
In `@src/Agentic/Memory/MemoryAugmentedAgent.cs`:
- Around line 91-109: The BuildMemoryBlock method currently injects untrusted
scored.Memory.Content directly into a System chat message (used by RunAsync),
which risks prompt-injection; update BuildMemoryBlock to isolate and sanitize
memory entries before adding them by wrapping each memory in explicit delimiters
(e.g., fenced code block or quoted block), escaping/control-character stripping
and enforcing a max length per scored.Memory.Content, and ensure the returned
string trims and preserves those fences so the System message created at the top
of RunAsync contains guarded, non-executable memory text rather than raw
user-influenced content.
In `@src/Agentic/Memory/ThreadedAgent.cs`:
- Around line 77-103: In RunAsync (ThreadedAgent.RunAsync) add validation to
ensure no element in the newMessages collection is null before proceeding: after
checking newMessages is non-null and non-empty, iterate the collection (or use
Any/Contains) to detect null items and throw an
ArgumentException/ArgumentNullException with a clear message (matching the style
used in InMemoryConversationStore.AppendAsync) so the API fails fast with a
descriptive error; keep the variable name newMessages and preserve existing
behavior if all elements are non-null.
---
Duplicate comments:
In `@src/Agentic/Agents/AgentExecutor.cs`:
- Around line 50-51: Replace the verbose null/whitespace check in the
AgentExecutor Name property with the standard helper: change the expression that
reads "_options.Name is { } name && name.Trim().Length > 0 ? name : \"agent\""
to use "!string.IsNullOrWhiteSpace(_options.Name) ? _options.Name : \"agent\""
(also update the equivalent pattern in SupervisorAgent to the same
!string.IsNullOrWhiteSpace(...) form) so both AgentExecutor.Name and
SupervisorAgent use consistent, readable string validation.
In `@src/Agentic/Memory/JsonFileConversationStore.cs`:
- Around line 135-139: The Save method currently writes JSON directly to
_filePath which risks corruption if interrupted; change Save(Dictionary<string,
List<StoredMessage>> store) to perform an atomic write by serializing to a temp
file in the same directory (use a unique temp name based on _filePath), write
the JSON to that temp file, flush/close it, then replace or move the temp file
over _filePath using an atomic operation (File.Replace or File.Move with
overwrite) so the switch is atomic; keep using SerializerSettings and preserve
exception handling as needed.
- Around line 118-133: The Load() method currently calls
JsonConvert.DeserializeObject without error handling; wrap the deserialization
in a try/catch that catches JsonException (and optionally Exception) around the
JsonConvert.DeserializeObject(...) call in JsonFileConversationStore.Load(), and
on failure return a new Dictionary<string,
List<StoredMessage>>(StringComparer.Ordinal) instead of letting the exception
propagate; optionally move or rename the corrupted _filePath (e.g., append
".corrupt" + timestamp) and/or log the error so callers can recover safely while
preserving SerializerSettings usage.
- Around line 43-47: The JsonFileConversationStore constructor currently assigns
an unvalidated filePath to _filePath, introducing path traversal risk; change it
to require/obtain a configured base directory (e.g., from options), canonicalize
the incoming path with Path.GetFullPath, and verify the canonicalized path
starts with the canonicalized base directory before assigning to _filePath (or
alternatively accept only a filename and combine it with the fixed base
directory using Path.Combine then canonicalize/validate); on validation failure
throw an ArgumentException or SecurityException. Ensure the checks are performed
in the JsonFileConversationStore(string filePath) constructor (or a factory) and
reuse the same canonicalization logic for any subsequent file accesses.
In `@src/AiDotNet.Storage.Sqlite/SqliteConversationStore.cs`:
- Around line 27-175: The ParseRole helper currently calls Enum.Parse which will
throw on bad data; change ParseRole(string role) to use
Enum.TryParse<ChatRole>(role, out var parsed) and if TryParse returns true
return parsed, otherwise throw a clear exception (e.g., InvalidDataException or
FormatException) with context including the invalid role value and the thread
id/sequence where available (ParseRole is used from GetAsync), ensuring callers
receive a descriptive error instead of a generic crash; update ParseRole
implementation and any unit tests that assume strict parsing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cec5c001-8908-4b40-8c2c-20d8656ca08d
📒 Files selected for processing (30)
src/Agentic/Agents/AgentAsTool.cssrc/Agentic/Agents/AgentExecutor.cssrc/Agentic/Agents/AgentExecutorOptions.cssrc/Agentic/Agents/AgentRunResult.cssrc/Agentic/Agents/IAgent.cssrc/Agentic/Agents/SupervisorAgent.cssrc/Agentic/Agents/SupervisorOptions.cssrc/Agentic/Agents/Swarm.cssrc/Agentic/Agents/SwarmMember.cssrc/Agentic/Agents/SwarmOptions.cssrc/Agentic/Agents/ToolNaming.cssrc/Agentic/Memory/AgentMemory.cssrc/Agentic/Memory/EmbeddingAgentMemoryStore.cssrc/Agentic/Memory/IAgentMemoryStore.cssrc/Agentic/Memory/IConversationStore.cssrc/Agentic/Memory/InMemoryAgentMemoryStore.cssrc/Agentic/Memory/InMemoryConversationStore.cssrc/Agentic/Memory/JsonFileConversationStore.cssrc/Agentic/Memory/MemoryAugmentationOptions.cssrc/Agentic/Memory/MemoryAugmentedAgent.cssrc/Agentic/Memory/ScoredMemory.cssrc/Agentic/Memory/ThreadedAgent.cssrc/AiDotNet.Storage.Sqlite/SqliteConversationStore.cstests/AiDotNet.Tests/UnitTests/Agentic/Agents/AgentExecutorTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Agents/AgentMemoryStoreTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Agents/AgentTestInfrastructure.cstests/AiDotNet.Tests/UnitTests/Agentic/Agents/ConversationMemoryTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Agents/SqliteConversationStoreTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Agents/SupervisorAgentTests.cstests/AiDotNet.Tests/UnitTests/Agentic/Agents/SwarmTests.cs
💤 Files with no reviewable changes (7)
- tests/AiDotNet.Tests/UnitTests/Agentic/Agents/ConversationMemoryTests.cs
- tests/AiDotNet.Tests/UnitTests/Agentic/Agents/SqliteConversationStoreTests.cs
- tests/AiDotNet.Tests/UnitTests/Agentic/Agents/AgentMemoryStoreTests.cs
- tests/AiDotNet.Tests/UnitTests/Agentic/Agents/AgentTestInfrastructure.cs
- tests/AiDotNet.Tests/UnitTests/Agentic/Agents/SupervisorAgentTests.cs
- tests/AiDotNet.Tests/UnitTests/Agentic/Agents/SwarmTests.cs
- tests/AiDotNet.Tests/UnitTests/Agentic/Agents/AgentExecutorTests.cs
…1-11) CodeRabbit PR #1551 — first 11 unresolved threads: - **SupervisorAgent.cs** (1): swap the `is { } x && x.Trim().Length > 0` style for `string.IsNullOrWhiteSpace` while still using a pattern-match to narrow nullability (net471's IsNullOrWhiteSpace lacks NotNullWhen attribution). - **EmbeddingAgentMemoryStore** (2-5): honour CancellationToken at the entry to every async method (IEmbeddingModel<T>.EmbedAsync has no token overload, so we ThrowIfCancellationRequested BEFORE the embed call; GetAllAsync / RemoveAsync get the same check up-front). - **InMemoryAgentMemoryStore** (6): same ThrowIfCancellationRequested pattern on AddAsync / SearchAsync / GetAllAsync / RemoveAsync. - **InMemoryConversationStore** (7): same on AppendAsync / GetAsync / ListThreadsAsync / ClearAsync. - **JsonFileConversationStore** (8-10): - canonicalise filePath via Path.GetFullPath in the constructor (defends against relative-path surprises without restricting to a base directory); - wrap Load()'s JsonConvert.DeserializeObject in try/catch so a corrupted file surfaces as InvalidOperationException with the file path instead of a raw JsonException; - Save() now writes-then-renames via File.Move(overwrite=true) (or File.Replace on older targets) so a crash mid-write can't corrupt the backing file. - **ChatMessage.cs** (11): reject empty content lists at the constructor with ArgumentException — the type contract says "one or more content parts".
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Agentic/Memory/JsonFileConversationStore.cs (1)
55-121:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCancellation tokens accepted but never checked in file-backed methods.
All four async methods (
AppendAsync,GetAsync,ListThreadsAsync,ClearAsync) accept aCancellationTokenbut never callThrowIfCancellationRequested(). Unlike the in-memory stores where operations are instantaneous, file I/O can be slow (especially on network drives or under heavy load), making cancellation support more valuable here.For consistency with
InMemoryAgentMemoryStore,EmbeddingAgentMemoryStore, andInMemoryConversationStorein this same PR—all of which now honor cancellation—add the check before entering the lock.♻️ Proposed fix for all four methods
public Task AppendAsync(string threadId, IReadOnlyList<ChatMessage> messages, CancellationToken cancellationToken = default) { Guard.NotNullOrWhiteSpace(threadId); Guard.NotNull(messages); + cancellationToken.ThrowIfCancellationRequested(); lock (_gate) {public Task<IReadOnlyList<ChatMessage>> GetAsync(string threadId, CancellationToken cancellationToken = default) { Guard.NotNullOrWhiteSpace(threadId); + cancellationToken.ThrowIfCancellationRequested(); lock (_gate) {public Task<IReadOnlyList<string>> ListThreadsAsync(CancellationToken cancellationToken = default) { + cancellationToken.ThrowIfCancellationRequested(); lock (_gate) {public Task ClearAsync(string threadId, CancellationToken cancellationToken = default) { Guard.NotNullOrWhiteSpace(threadId); + cancellationToken.ThrowIfCancellationRequested(); lock (_gate) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Memory/JsonFileConversationStore.cs` around lines 55 - 121, Add cancellation checks at the start of the file-backed methods by calling cancellationToken.ThrowIfCancellationRequested() before acquiring the _gate lock in AppendAsync, GetAsync, ListThreadsAsync, and ClearAsync so file I/O can be cancelled; update those four methods (AppendAsync, GetAsync, ListThreadsAsync, ClearAsync) to call cancellationToken.ThrowIfCancellationRequested() immediately after parameter validation and before lock(_gate).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Agentic/Memory/JsonFileConversationStore.cs`:
- Around line 55-121: Add cancellation checks at the start of the file-backed
methods by calling cancellationToken.ThrowIfCancellationRequested() before
acquiring the _gate lock in AppendAsync, GetAsync, ListThreadsAsync, and
ClearAsync so file I/O can be cancelled; update those four methods (AppendAsync,
GetAsync, ListThreadsAsync, ClearAsync) to call
cancellationToken.ThrowIfCancellationRequested() immediately after parameter
validation and before lock(_gate).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f174c140-23f9-4b63-a173-330f1e0000c9
📒 Files selected for processing (6)
src/Agentic/Agents/SupervisorAgent.cssrc/Agentic/Memory/EmbeddingAgentMemoryStore.cssrc/Agentic/Memory/InMemoryAgentMemoryStore.cssrc/Agentic/Memory/InMemoryConversationStore.cssrc/Agentic/Memory/JsonFileConversationStore.cssrc/Agentic/Models/ChatMessage.cs
- **AnthropicChatClient** (12, 13): validate the optional custom endpoint as
an absolute URI in the ctor; ParseArguments now throws ArgumentException
on malformed tool-call JSON instead of silently substituting {}.
- **MeaiChatClient.ToMeaiMessages** (14): null-check each message element so
a null entry fails fast at the boundary, not deep inside .Contents.
- **ImageContent** (15, 16): reject empty byte[] in FromBytes; require an
absolute URI in FromUri (fully-qualified System.Uri to avoid clash with
the class's own Uri member).
- **SqliteConversationStore** (17, 18, 19): switch transaction.Commit() to
CommitAsync; cache per-connection-string schema-init flag so DDL only runs
once; ParseRole uses TryParse with a descriptive InvalidOperationException
naming the bad value.
- **AiModelResult.EvaluateBenchmarksAsync** (20, 21): add a forwarding
overload that preserves the old (options, cancellationToken) call shape;
XML-doc the chatClient null contract for reasoning suites.
- **CodeReasoner** (22): document the IChatModel->IChatClient +
ITool->IAgentTool breaking change in the ctor XML remarks.
- **AgentAsTool** (23): make ParametersSchema static so identical-per-tool
JObject isn't reallocated per instance.
- **AgentExecutor / Swarm** (24, 25): use a pattern-match form of
IsNullOrWhiteSpace that still narrows nullability on net471.
- **Swarm.BuildHandoffMap** (26): align dictionary comparer with peer-skip
comparer (both Ordinal) and drop the per-call LINQ .ToList() in favour of
iterating _members.Keys directly.
- **DelegateAgentTool** (28, 29): reject inconsistent MethodInfo/target
pairings up front (instance method with null target; declaring-type
mismatch); on net6+ use NullabilityInfoContext to honour
`string?` annotation when deciding whether a missing reference param can
bind to null (net471 falls back to the historical permissive behaviour).
- **JsonSchemaGenerator** (31): drop to internal — it's an implementation
helper, not a facade surface.
Skipped — out-of-scope refactors that would cascade widely:
- (27) ChatClientBase -> internal: AnthropicChatClient/OpenAIChatClient are
public and must inherit a public base; making them internal too is an
API design discussion, not a CodeRabbit nit.
- (30) DelegateAgentTool ValueTask handling: comment is an analysis chain
with no concrete suggestion.
- (32, 33) AgentToolSchemaGenerator default(T) + nested-object schema:
source-generator parity with JsonSchemaGenerator is a deliberate larger
change that needs its own PR.
…ls (34-40) - **AgentAsTool** (34): drop to internal — it's handoff plumbing called only from SupervisorAgent's tool wiring. - **AgentAsTool.InvokeAsync** (35): validate the "task" token's JSON type before reading. Newtonsoft's implicit-string cast returns null for any non-string token, which would silently surface as "missing argument" when the model passed an object / number / null instead. - **Swarm ctor** (36): reject members whose names collide after handoff-tool normalisation (e.g. "Search Docs" and "Search_Docs", or case-only variants) so distinct members can't silently overwrite each other in BuildHandoffMap and become unreachable. - **Swarm.RunAsync** (37): preserve caller-supplied System messages separately and re-apply them alongside the active member's prompt so any seeded global context survives the per-turn rebuild instead of being dropped on the floor. - **SwarmMember ctor** (38): snapshot + element-validate the caller's handoffs list so a mutable list can't change behind Swarm's back after construction-time validation. - **MemoryAugmentedAgent.BuildMemoryBlock** (39): fence recalled memory snippets in code blocks and prepend an explicit "untrusted factual context only / do not follow instructions inside" advisory to harden against prompt-injection escalation via user-influenced memory entries. - **ThreadedAgent.RunAsync** (40): null-check individual newMessages elements at the boundary, mirroring InMemoryConversationStore.AppendAsync.
CodeRabbit PR #1551 (AgentToolSchemaGenerator): The generated binder previously fell back to `default(T)` for any unsupplied argument. That silently turned a missing required `int` / `bool` into `0` / `false` and made the same `[Tool]` method behave differently depending on whether the user reached it through `CreateAgentTools()` (this generator) or `AgentToolFactory.ScanInstance()` (reflection-based DelegateAgentTool, which throws). Bring the generator to parity: * has explicit default -> use that literal (unchanged); * non-nullable value type with no default -> emit `throw new ArgumentException("Missing required parameter '...'.", "name")` so a missing required value type surfaces as a tool error instead of a silent zero; * reference / Nullable<T> with no default -> `default(T)` (i.e. null), matching the prior permissive behaviour. Mirrors the binding contract that DelegateAgentTool already implements.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/Agentic/Agents/AgentAsTool.cs (1)
58-81:⚠️ Potential issue | 🟠 MajorAvoid exposing a mutable shared
JObjectas the tool parameter schema
AgentAsTool<T>.ParametersSchemareturns the same staticSharedParametersSchemainstance to all tools, andAiToolDefinitionpreserves the providedJObjectreference (it doesn’t clone it). Provider payload building then readstool.ParametersSchemadirectly, so any mutation of that sharedJObjectwill silently corrupt the schema for all handoffs (and future definitions).♻️ Suggested fix
- public JObject ParametersSchema => SharedParametersSchema; + public JObject ParametersSchema => (JObject)SharedParametersSchema.DeepClone();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Agents/AgentAsTool.cs` around lines 58 - 81, SharedParametersSchema is a single mutable JObject instance that's returned by AgentAsTool<T>.ParametersSchema and passed into AiToolDefinition via ToDefinition, which preserves the reference; to prevent accidental cross-tool mutation, return a cloned/immutable copy instead of the shared instance and ensure ToDefinition passes a clone (or a read-only deep copy) to AiToolDefinition; update the ParametersSchema getter and/or ToDefinition to call a deep clone (or create an immutable JObject) of SharedParametersSchema before returning/passing it so each consumer gets its own independent object.src/Agentic/Agents/Swarm.cs (1)
149-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn the full caller transcript from
AgentRunResult.
callerSystemMessagesare re-applied to each model request, but they never get merged back into the message list returned fromAgentRunResult.Finished/Stopped. A run that starts withRunAsync(IReadOnlyList<ChatMessage>)therefore loses part of its original input history, which makes the result non-replayable and hands any conversation-store wrapper an incomplete transcript.🛠️ Concrete fix
- var callerSystemMessages = new List<ChatMessage>(); - var shared = new List<ChatMessage>(messages.Count + 8); + var transcript = new List<ChatMessage>(messages.Count + 8); + var callerSystemMessages = new List<ChatMessage>(); + var shared = new List<ChatMessage>(messages.Count + 8); foreach (var message in messages) { + transcript.Add(message); if (message.Role == ChatRole.System) { callerSystemMessages.Add(message); } else { shared.Add(message); } } @@ var response = await active.Client.GetResponseAsync(requestMessages, requestOptions, cancellationToken) .ConfigureAwait(false); shared.Add(response.Message); + transcript.Add(response.Message); @@ - shared.Add(ChatMessage.Tool(call.CallId, $"Control transferred to '{peer}'.")); + var toolMessage = ChatMessage.Tool(call.CallId, $"Control transferred to '{peer}'."); + shared.Add(toolMessage); + transcript.Add(toolMessage); @@ - shared.Add(ChatMessage.Tool( + var toolMessage = ChatMessage.Tool( call.CallId, $"Ignored: already transferring control to '{nextMemberName}' this turn.", - isError: true)); + isError: true); + shared.Add(toolMessage); + transcript.Add(toolMessage); @@ var toolMessage = await active.Tools.InvokeToToolMessageAsync(call, cancellationToken) .ConfigureAwait(false); shared.Add(toolMessage); + transcript.Add(toolMessage); @@ - shared.Add(ChatMessage.Tool( - call.CallId, $"Unknown tool '{call.ToolName}'.", isError: true)); + var toolMessage = ChatMessage.Tool( + call.CallId, $"Unknown tool '{call.ToolName}'.", isError: true); + shared.Add(toolMessage); + transcript.Add(toolMessage); @@ return AgentRunResult.Finished( response.Message.Text, - shared, + transcript, iteration, sawUsage ? new ChatUsage(inputTokens, outputTokens) : null, active.Name); } - var lastText = shared.Count > 0 ? shared[shared.Count - 1].Text : string.Empty; + var lastText = transcript.Count > 0 ? transcript[transcript.Count - 1].Text : string.Empty; return AgentRunResult.Stopped( lastText, - shared, + transcript, maxIterations, sawUsage ? new ChatUsage(inputTokens, outputTokens) : null, active.Name);As per coding guidelines, production-ready code should preserve validated external input state instead of dropping it at a runtime boundary.
Also applies to: 182-182, 304-320
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Agents/Swarm.cs` around lines 149 - 165, The code strips out System-role messages into callerSystemMessages and never merges them back into the returned transcript, causing AgentRunResult.Finished/Stopped to omit the original caller-provided system messages; update the logic where the final message list for AgentRunResult (used by Finished/Stopped) is constructed (after RunAsync(IReadOnlyList<ChatMessage>) processing and any per-turn system re-application) to reinsert callerSystemMessages into the final returned list (preserving their original positions or at minimum prepending/appending them consistently) so the AgentRunResult contains the full replayable transcript; ensure the same fix is applied to the other similar blocks mentioned (around the uses of callerSystemMessages/shared at the other locations).Source: Coding guidelines
src/Agentic/Tools/DelegateAgentTool.cs (2)
111-119: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftNullable-reference requiredness has drifted across
DelegateAgentTool,JsonSchemaGenerator, andAgentToolSchemaGenerator.DelegateAgentToolstill allows missing reference arguments on older TFMs,JsonSchemaGeneratorstill marks nullable reference parameters as required, andAgentToolSchemaGeneratorstill binds missing non-nullable reference parameters asnull. The shared root cause is three independent implementations of the same required/nullability decision. Centralizing that rule and reusing it across the reflection binder, reflection schema builder, and source generator would remove the current cross-path contract mismatch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Tools/DelegateAgentTool.cs` around lines 111 - 119, DelegateAgentTool, JsonSchemaGenerator, and AgentToolSchemaGenerator have duplicated, drifting logic for required/nullability decisions; extract a single shared helper (e.g., ParameterNullabilityHelper or RequirednessResolver) that exposes a clear API (e.g., IsParameterRequired(ParameterInfo) and AllowsNull(ParameterInfo)) and replace ad-hoc checks (including the current IsNullableAnnotated usage) in DelegateAgentTool (callArgs null/required decision), JsonSchemaGenerator (marking parameters as required in schemas), and AgentToolSchemaGenerator (binding missing non-nullable params) to call this helper so all three paths use the identical rule.
111-119:⚠️ Potential issue | 🟠 MajorMajor: net471 fallback makes missing required reference parameters silently null (breaks required-parameter contract).
DelegateAgentTooltreats a missing reference parameter as nullable whenIsNullableAnnotated(parameter)is true:callArgs[i] = null;(src/Agentic/Tools/DelegateAgentTool.cs around 111-116). On net471,IsNullableAnnotatedreturnstruefor every reference type (#else // net471 ... return true;at ~203-206), so missing requiredstringargs becomenullinstead of returningToolInvocationResult.Error($"Missing required parameter '{name}'.");(~119).This also contradicts schema “required” generation:
JsonSchemaGenerator.IsRequired(...)marks parameters as required when they are not optional/default and are notNullable<T>—it does not use reference nullability metadata (src/Agentic/Tools/JsonSchemaGenerator.cs around 95-109), so requiredstringparameters stay required across TFMs while invocation behavior diverges on net471.Current unit tests only cover missing required value-type params (e.g.,
bin AgentToolTests.cs ~118-125); there’s no test for missing required reference params (e.g., acity: stringcase), so add one and fix the behavior.Fix: remove the net471 “permissive for all reference types” fallback. Instead, parse compiler-emitted nullable annotations on net471 (
System.Runtime.CompilerServices.NullableAttribute/NullableContextAttribute) and return nullable-state per parameter; only fall back to permissive behavior when the nullable metadata is genuinely unavailable for that specific parameter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Tools/DelegateAgentTool.cs` around lines 111 - 119, DelegateAgentTool currently treats all reference types as nullable on net471 because IsNullableAnnotated returns true unconditionally; remove that permissive fallback and implement proper nullable-metadata parsing for net471 by reading System.Runtime.CompilerServices.NullableAttribute and NullableContextAttribute on the parameter, declaring member, declaring type and assembly to determine the actual nullable state for the specific parameter (only treat as nullable when the metadata explicitly indicates it); change DelegateAgentTool.IsNullableAnnotated to return nullability based on those attributes and only fall back to permissive behavior when no nullable metadata exists for that parameter, and add a unit test in AgentToolTests that asserts missing required reference parameters (e.g., a required string parameter) produce ToolInvocationResult.Error to match JsonSchemaGenerator.IsRequired behavior.src/Agentic/Tools/JsonSchemaGenerator.cs (1)
95-109:⚠️ Potential issue | 🟠 MajorFix required-parameter generation for nullable reference types (
string?).
JsonSchemaGenerator.IsRequiredonly exempts optional/defaulted parameters andNullable<T>value types, so a nullable reference parameter likestring? citygets added to the schema’s"required"list. This contradictsDelegateAgentTool.InvokeCoreAsync, which explicitly allows missing/null values for reference parameters annotated as nullable (string?) viaIsNullableAnnotated. The mismatch over-constrains schema-enforcing providers.Update
IsRequiredto mirror the same nullability annotation check used inDelegateAgentTool(and treat reference parameters as non-required on TFMs withoutNullabilityInfoContext), e.g.:private static bool IsRequired(ParameterInfo parameter, ToolParameterAttribute? attribute) { if (attribute?.Required is { } explicitRequired) return explicitRequired; if (parameter.IsOptional || parameter.HasDefaultValue) return false; // Nullable<T> value types are not required. if (Nullable.GetUnderlyingType(parameter.ParameterType) is not null) return false; // Non-nullable value types are required. if (parameter.ParameterType.IsValueType) return true; `#if` NET6_0_OR_GREATER try { var ctx = new System.Reflection.NullabilityInfoContext(); var info = ctx.Create(parameter); var allowsNull = info.ReadState == System.Reflection.NullabilityState.Nullable || info.WriteState == System.Reflection.NullabilityState.Nullable; return !allowsNull; } catch { // Be conservative: don't mark reference params as required if we can't read nullability metadata. return false; } `#else` // Reference-type nullability annotations are unavailable; treat as optional. return false; `#endif` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Tools/JsonSchemaGenerator.cs` around lines 95 - 109, Update IsRequired in JsonSchemaGenerator to mirror DelegateAgentTool.InvokeCoreAsync nullability logic: first respect attribute?.Required and optional/default checks, then return false for Nullable<T> value types and true for non-nullable value types; for reference types use System.Reflection.NullabilityInfoContext (when available) to detect nullable annotations (treat Nullable read/write as not required) and on older TFMs or if reading fails conservatively return false (do not mark reference params like string? as required). Ensure you reference the IsRequired method and align behavior with DelegateAgentTool.InvokeCoreAsync's IsNullableAnnotated semantics.src/Agentic/Memory/MemoryAugmentedAgent.cs (1)
91-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve caller
Systemmessages ahead of recalled memory.Lines 91-95 always prepend the memory block at index 0, so any caller-supplied
Systemprompt is moved behind it. That changes instruction precedence and can weaken the application’s policy prompt. Insert the recalled-memory message after the existing leadingSystemmessages instead of before the entire transcript.Suggested fix
- var contextMessage = ChatMessage.System(BuildMemoryBlock(relevant)); - var augmented = new List<ChatMessage>(messages.Count + 1) { contextMessage }; - augmented.AddRange(messages); + var contextMessage = ChatMessage.System(BuildMemoryBlock(relevant)); + var insertAt = 0; + while (insertAt < messages.Count && messages[insertAt].Role == ChatRole.System) + { + insertAt++; + } + + var augmented = new List<ChatMessage>(messages.Count + 1); + for (var i = 0; i < insertAt; i++) + { + augmented.Add(messages[i]); + } + + augmented.Add(contextMessage); + + for (var i = insertAt; i < messages.Count; i++) + { + augmented.Add(messages[i]); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Memory/MemoryAugmentedAgent.cs` around lines 91 - 95, The current code always prepends the recalled memory System message (created via BuildMemoryBlock) which pushes any caller-supplied System prompts behind it; change the insertion so the memory System message is inserted after existing leading System messages instead of at index 0. Concretely, compute the index of the first non-System message in the incoming messages list (or count leading messages where msg.Role == ChatMessageRole.System), create the contextMessage via BuildMemoryBlock(relevant), insert it at that computed index (instead of always prepending), and then call _inner.RunAsync(augmented, cancellationToken) as before.src/Agentic/Memory/ThreadedAgent.cs (1)
98-109:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSame-thread runs are not atomic.
Lines 98-109 split a turn into
GetAsync→_inner.RunAsync→AppendAsyncwith no concurrency control. Two callers hitting the samethreadIdcan both answer against the same stale history and then append in the wrong causal order. BecauseSqliteConversationStoreis designed to be shared across processes, an in-memory lock here would only mask part of the problem; the store contract needs a versioned/conditional append path.Design sketch
+public sealed record ConversationSnapshot( + IReadOnlyList<ChatMessage> Messages, + long Version); + public interface IConversationStore { - Task<IReadOnlyList<ChatMessage>> GetAsync(string threadId, CancellationToken cancellationToken = default); - Task AppendAsync(string threadId, IReadOnlyList<ChatMessage> messages, CancellationToken cancellationToken = default); + Task<ConversationSnapshot> GetSnapshotAsync(string threadId, CancellationToken cancellationToken = default); + Task AppendIfVersionAsync( + string threadId, + IReadOnlyList<ChatMessage> messages, + long expectedVersion, + CancellationToken cancellationToken = default); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Memory/ThreadedAgent.cs` around lines 98 - 109, The current RunAsync implementation in ThreadedAgent.cs performs _store.GetAsync(threadId) → _inner.RunAsync(...) → _store.AppendAsync(threadId, turn) with no concurrency control, allowing race conditions; change the contract and call pattern to use optimistic concurrency: modify the conversation store to return a version/token from GetAsync (e.g., GetAsync returns (messages, version)) and add a conditional AppendAsync overload that accepts the expected version and fails if it mismatches; then update ThreadedAgent.RunAsync to read the version from _store.GetAsync, call _inner.RunAsync, and attempt _store.AppendAsync(threadId, turn, expectedVersion) inside a short retry loop that re-reads the latest history and reruns _inner.RunAsync only on append conflict, so concurrent callers never silently overwrite each other.src/AiDotNet.Storage.Sqlite/SqliteConversationStore.cs (1)
154-171:⚠️ Potential issue | 🟠 MajorFix schema initialization caching for
Data Source=:memory:SQLite connections (data integrity bug).
SqliteConversationStore.EnsureSchemaAsynccaches schema creation in_schemaInitialisedkeyed only by_connectionStringand then skips DDL on subsequent calls. WithData Source=:memory:, SQLite creates a separate private in-memory database per connection, so later connections won’t have theConversationstable (leading tono such table: Conversations).Suggested fix
- if (_schemaInitialised.ContainsKey(_connectionString)) + if (ShouldCacheSchema(_connectionString) && + _schemaInitialised.ContainsKey(_connectionString)) { return; } @@ - _schemaInitialised[_connectionString] = true; + if (ShouldCacheSchema(_connectionString)) + { + _schemaInitialised[_connectionString] = true; + } } + + private static bool ShouldCacheSchema(string connectionString) + { + var builder = new SqliteConnectionStringBuilder(connectionString); + return !string.Equals(builder.DataSource, ":memory:", StringComparison.Ordinal) + && builder.Mode != SqliteOpenMode.Memory; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/AiDotNet.Storage.Sqlite/SqliteConversationStore.cs` around lines 154 - 171, EnsureSchemaAsync is incorrectly skipping DDL for in-memory SQLite because _schemaInitialised is keyed only by _connectionString; when using "Data Source=:memory:" each SqliteConnection gets its own private DB so the Conversations table may be missing. Fix by changing the caching logic in EnsureSchemaAsync to not cache (or to bypass the cache) for in-memory connections: detect when _connectionString contains "Mode=Memory" or "Data Source=:memory:" (or check connection.FileName for ":memory:"), and in that case always run the CREATE TABLE/INDEX commands (do not read/write _schemaInitialised), otherwise keep the existing _schemaInitialised behavior. Target symbols: EnsureSchemaAsync, _schemaInitialised, _connectionString, and the command execution block.
♻️ Duplicate comments (3)
src/Agentic/Models/Connectors/AnthropicChatClient.cs (1)
150-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve “usage unknown” in streaming responses.
When Anthropic omits usage in the stream, Lines 150-152 initialize both counters to
0and Lines 230-232 always emitnew ChatUsage(inputTokens, outputTokens). That changes “provider did not report usage” into “provider reported zero tokens,” which breaks downstream accounting. Track whether usage was actually seen and passnullwhen it was not.Also applies to: 179-179, 222-232
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Models/Connectors/AnthropicChatClient.cs` around lines 150 - 152, The code initializes inputTokens and outputTokens to 0 and always constructs new ChatUsage(inputTokens, outputTokens) which turns “usage unknown” into zero; modify the streaming/response handling in AnthropicChatClient (look for ChatFinishReason, inputTokens, outputTokens and where ChatUsage is constructed) to track whether usage was actually reported (e.g., a boolean usageSeen flag updated when any usage field is parsed), and only create/pass a ChatUsage instance when usageSeen is true; otherwise pass null for usage. Apply the same change at the other occurrences referenced (around the blocks that currently set tokens to 0 and emit ChatUsage) so that absent usage remains null rather than zeroed.src/Agentic/Tools/DelegateAgentTool.cs (1)
163-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait
ValueTaskresults in the reflection path too.
src/AiDotNet.Generators/AgentToolSchemaGenerator.csalready treatsValueTaskandValueTask<T>as async return types, butUnwrapResultAsynconly unwrapsTask. A reflected tool returningValueTask<T>will serialize the boxed struct instead of the awaited result. HandleValueTask/ValueTask<T>here, or reject them at registration time so the reflection and generated paths stay consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Tools/DelegateAgentTool.cs` around lines 163 - 177, UnwrapResultAsync currently only handles Task; update it to also detect boxed ValueTask and ValueTask<T> from the incoming returnValue (in the UnwrapResultAsync method) and await them the same way as Task: if returnValue is a non-generic ValueTask, call its AsTask() via reflection and await; if it's a ValueTask<T> (generic), call AsTask() via reflection, await that Task and return its Result (or use the same Result-property reflection used for Task); alternatively, if you prefer to reject unsupported types, add a registration-time check that disallows ValueTask/ValueTask<T> so reflection and generator behavior remain consistent. Ensure you reference UnwrapResultAsync, returnValue, taskType, and the existing Result-property reflection logic when making the change.src/Agentic/Memory/MemoryAugmentedAgent.cs (1)
114-117:⚠️ Potential issue | 🔴 CriticalFenced recall is still escapable.
Lines 114-117 still write
scored.Memory.Contentverbatim inside a fenced block. A stored memory containing ````` can terminate that fence and regain fullSystem-message influence. Escape or re-encode the snippet before appending it.Suggested fix
foreach (var scored in memories) { builder.AppendLine("- Memory snippet (verbatim):"); builder.AppendLine("```text"); - builder.AppendLine(scored.Memory.Content); + builder.AppendLine(EscapeForFencedBlock(scored.Memory.Content)); builder.AppendLine("```"); } return builder.ToString().TrimEnd(); } + + private static string EscapeForFencedBlock(string content) + { + return content + .Replace("\0", string.Empty) + .Replace("```", "`\u200B``"); + }As per coding guidelines, missing validation/guarding of external inputs at system boundaries is a blocking production-readiness issue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Agentic/Memory/MemoryAugmentedAgent.cs` around lines 114 - 117, The code appends scored.Memory.Content verbatim into a fenced code block (builder.AppendLine(scored.Memory.Content)), allowing a stored "```" to break the fence; fix by escaping or re-encoding the memory before appending: create/Use a helper like EscapeForFencedBlock(string) and call builder.AppendLine(EscapeForFencedBlock(scored.Memory.Content)) instead of writing the raw content, ensuring the helper removes nulls and replaces occurrences of "```" (e.g., inject a zero-width space or other safe sequence) so the fence cannot be terminated.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Agentic/Agents/SwarmMember.cs`:
- Around line 50-69: The handoffSnapshot currently uses a plain string[] which
still allows callers to downcast Handoffs back to string[] and mutate it; update
the SwarmMember constructor so Handoffs is assigned an immutable/read-only
collection instead of a raw array (e.g., create an ImmutableArray<string> via
ImmutableArray.Create(copy) or wrap the array with Array.AsReadOnly /
ReadOnlyCollection<string>) after the existing validation loop, and assign that
immutable/read-only instance to the Handoffs property (preserving the
IReadOnlyList<string> type) so callers cannot mutate the snapshot at runtime;
keep the Guard.NotNullOrWhiteSpace checks and the same copy semantics.
In `@src/Agentic/Models/Connectors/AnthropicChatClient.cs`:
- Around line 60-68: The constructor currently uses Uri.TryCreate(endpoint,
UriKind.Absolute, out _) which allows non-HTTP schemes; change the validation
to: call Guard.NotNullOrWhiteSpace(endpoint), parse into a Uri instance via
Uri.TryCreate(endpoint, UriKind.Absolute, out var uri), then verify uri.Scheme
is either Uri.UriSchemeHttp or Uri.UriSchemeHttps and throw an ArgumentException
if not, and finally store the normalized URI (e.g., uri.AbsoluteUri) into the
backing field (_endpoint) instead of the raw string so only http/https endpoints
are accepted at the boundary.
In `@src/Agentic/Models/ImageContent.cs`:
- Around line 77-82: The constructor/validation in ImageContent currently
accepts any absolute URI via System.Uri.TryCreate(uri, UriKind.Absolute, out _);
update it to capture the created System.Uri (e.g., out var createdUri) and
enforce createdUri.Scheme is either Uri.UriSchemeHttp or Uri.UriSchemeHttps,
throwing the same ArgumentException (or a slightly updated message) if not; keep
the initial TryCreate check but add the scheme check so only http/https URIs are
allowed for the uri parameter.
In `@src/AiDotNet.Generators/AgentToolSchemaGenerator.cs`:
- Around line 168-192: The generated binder currently only throws for missing
non-nullable value types; update AgentToolSchemaGenerator so missing
non-nullable reference parameters also throw to match DelegateAgentTool and
IsRequired(...). Change the missingHandler logic around
p.HasExplicitDefaultValue/isValueType: treat a parameter as required (throw)
when it has no explicit default and either isValueType OR (is a reference type
and p.NullableAnnotation != NullableAnnotation.Annotated). Use the same throw
expression format you already build (throw new
global::System.ArgumentException(..., {Verbatim(p.Name)})) and keep
GetParameterDefaultLiteral for explicit defaults; keep the existing
TryGetValue/ToObject generation otherwise.
In `@src/Models/Results/AiModelResult.cs`:
- Around line 1454-1457: Add a compatibility-preserving overload that accepts
only chatClient and forwards to the existing full overload with options set to
null: implement public Task<BenchmarkReport> EvaluateBenchmarksAsync(ChatClient?
chatClient, CancellationToken cancellationToken = default) =>
EvaluateBenchmarksAsync(options: null, chatClient: chatClient,
cancellationToken: cancellationToken); this restores the old
AiModelResult.EvaluateBenchmarksAsync(chatClient: ...) facade while leaving the
existing EvaluateBenchmarksAsync(BenchmarkingOptions? options, ...) forwarder
and the full overload untouched.
---
Outside diff comments:
In `@src/Agentic/Agents/AgentAsTool.cs`:
- Around line 58-81: SharedParametersSchema is a single mutable JObject instance
that's returned by AgentAsTool<T>.ParametersSchema and passed into
AiToolDefinition via ToDefinition, which preserves the reference; to prevent
accidental cross-tool mutation, return a cloned/immutable copy instead of the
shared instance and ensure ToDefinition passes a clone (or a read-only deep
copy) to AiToolDefinition; update the ParametersSchema getter and/or
ToDefinition to call a deep clone (or create an immutable JObject) of
SharedParametersSchema before returning/passing it so each consumer gets its own
independent object.
In `@src/Agentic/Agents/Swarm.cs`:
- Around line 149-165: The code strips out System-role messages into
callerSystemMessages and never merges them back into the returned transcript,
causing AgentRunResult.Finished/Stopped to omit the original caller-provided
system messages; update the logic where the final message list for
AgentRunResult (used by Finished/Stopped) is constructed (after
RunAsync(IReadOnlyList<ChatMessage>) processing and any per-turn system
re-application) to reinsert callerSystemMessages into the final returned list
(preserving their original positions or at minimum prepending/appending them
consistently) so the AgentRunResult contains the full replayable transcript;
ensure the same fix is applied to the other similar blocks mentioned (around the
uses of callerSystemMessages/shared at the other locations).
In `@src/Agentic/Memory/MemoryAugmentedAgent.cs`:
- Around line 91-95: The current code always prepends the recalled memory System
message (created via BuildMemoryBlock) which pushes any caller-supplied System
prompts behind it; change the insertion so the memory System message is inserted
after existing leading System messages instead of at index 0. Concretely,
compute the index of the first non-System message in the incoming messages list
(or count leading messages where msg.Role == ChatMessageRole.System), create the
contextMessage via BuildMemoryBlock(relevant), insert it at that computed index
(instead of always prepending), and then call _inner.RunAsync(augmented,
cancellationToken) as before.
In `@src/Agentic/Memory/ThreadedAgent.cs`:
- Around line 98-109: The current RunAsync implementation in ThreadedAgent.cs
performs _store.GetAsync(threadId) → _inner.RunAsync(...) →
_store.AppendAsync(threadId, turn) with no concurrency control, allowing race
conditions; change the contract and call pattern to use optimistic concurrency:
modify the conversation store to return a version/token from GetAsync (e.g.,
GetAsync returns (messages, version)) and add a conditional AppendAsync overload
that accepts the expected version and fails if it mismatches; then update
ThreadedAgent.RunAsync to read the version from _store.GetAsync, call
_inner.RunAsync, and attempt _store.AppendAsync(threadId, turn, expectedVersion)
inside a short retry loop that re-reads the latest history and reruns
_inner.RunAsync only on append conflict, so concurrent callers never silently
overwrite each other.
In `@src/Agentic/Tools/DelegateAgentTool.cs`:
- Around line 111-119: DelegateAgentTool, JsonSchemaGenerator, and
AgentToolSchemaGenerator have duplicated, drifting logic for
required/nullability decisions; extract a single shared helper (e.g.,
ParameterNullabilityHelper or RequirednessResolver) that exposes a clear API
(e.g., IsParameterRequired(ParameterInfo) and AllowsNull(ParameterInfo)) and
replace ad-hoc checks (including the current IsNullableAnnotated usage) in
DelegateAgentTool (callArgs null/required decision), JsonSchemaGenerator
(marking parameters as required in schemas), and AgentToolSchemaGenerator
(binding missing non-nullable params) to call this helper so all three paths use
the identical rule.
- Around line 111-119: DelegateAgentTool currently treats all reference types as
nullable on net471 because IsNullableAnnotated returns true unconditionally;
remove that permissive fallback and implement proper nullable-metadata parsing
for net471 by reading System.Runtime.CompilerServices.NullableAttribute and
NullableContextAttribute on the parameter, declaring member, declaring type and
assembly to determine the actual nullable state for the specific parameter (only
treat as nullable when the metadata explicitly indicates it); change
DelegateAgentTool.IsNullableAnnotated to return nullability based on those
attributes and only fall back to permissive behavior when no nullable metadata
exists for that parameter, and add a unit test in AgentToolTests that asserts
missing required reference parameters (e.g., a required string parameter)
produce ToolInvocationResult.Error to match JsonSchemaGenerator.IsRequired
behavior.
In `@src/Agentic/Tools/JsonSchemaGenerator.cs`:
- Around line 95-109: Update IsRequired in JsonSchemaGenerator to mirror
DelegateAgentTool.InvokeCoreAsync nullability logic: first respect
attribute?.Required and optional/default checks, then return false for
Nullable<T> value types and true for non-nullable value types; for reference
types use System.Reflection.NullabilityInfoContext (when available) to detect
nullable annotations (treat Nullable read/write as not required) and on older
TFMs or if reading fails conservatively return false (do not mark reference
params like string? as required). Ensure you reference the IsRequired method and
align behavior with DelegateAgentTool.InvokeCoreAsync's IsNullableAnnotated
semantics.
In `@src/AiDotNet.Storage.Sqlite/SqliteConversationStore.cs`:
- Around line 154-171: EnsureSchemaAsync is incorrectly skipping DDL for
in-memory SQLite because _schemaInitialised is keyed only by _connectionString;
when using "Data Source=:memory:" each SqliteConnection gets its own private DB
so the Conversations table may be missing. Fix by changing the caching logic in
EnsureSchemaAsync to not cache (or to bypass the cache) for in-memory
connections: detect when _connectionString contains "Mode=Memory" or "Data
Source=:memory:" (or check connection.FileName for ":memory:"), and in that case
always run the CREATE TABLE/INDEX commands (do not read/write
_schemaInitialised), otherwise keep the existing _schemaInitialised behavior.
Target symbols: EnsureSchemaAsync, _schemaInitialised, _connectionString, and
the command execution block.
---
Duplicate comments:
In `@src/Agentic/Memory/MemoryAugmentedAgent.cs`:
- Around line 114-117: The code appends scored.Memory.Content verbatim into a
fenced code block (builder.AppendLine(scored.Memory.Content)), allowing a stored
"```" to break the fence; fix by escaping or re-encoding the memory before
appending: create/Use a helper like EscapeForFencedBlock(string) and call
builder.AppendLine(EscapeForFencedBlock(scored.Memory.Content)) instead of
writing the raw content, ensuring the helper removes nulls and replaces
occurrences of "```" (e.g., inject a zero-width space or other safe sequence) so
the fence cannot be terminated.
In `@src/Agentic/Models/Connectors/AnthropicChatClient.cs`:
- Around line 150-152: The code initializes inputTokens and outputTokens to 0
and always constructs new ChatUsage(inputTokens, outputTokens) which turns
“usage unknown” into zero; modify the streaming/response handling in
AnthropicChatClient (look for ChatFinishReason, inputTokens, outputTokens and
where ChatUsage is constructed) to track whether usage was actually reported
(e.g., a boolean usageSeen flag updated when any usage field is parsed), and
only create/pass a ChatUsage instance when usageSeen is true; otherwise pass
null for usage. Apply the same change at the other occurrences referenced
(around the blocks that currently set tokens to 0 and emit ChatUsage) so that
absent usage remains null rather than zeroed.
In `@src/Agentic/Tools/DelegateAgentTool.cs`:
- Around line 163-177: UnwrapResultAsync currently only handles Task; update it
to also detect boxed ValueTask and ValueTask<T> from the incoming returnValue
(in the UnwrapResultAsync method) and await them the same way as Task: if
returnValue is a non-generic ValueTask, call its AsTask() via reflection and
await; if it's a ValueTask<T> (generic), call AsTask() via reflection, await
that Task and return its Result (or use the same Result-property reflection used
for Task); alternatively, if you prefer to reject unsupported types, add a
registration-time check that disallows ValueTask/ValueTask<T> so reflection and
generator behavior remain consistent. Ensure you reference UnwrapResultAsync,
returnValue, taskType, and the existing Result-property reflection logic when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf19616a-4430-42bb-a042-c130d1eb6566
📒 Files selected for processing (15)
src/Agentic/Agents/AgentAsTool.cssrc/Agentic/Agents/AgentExecutor.cssrc/Agentic/Agents/Swarm.cssrc/Agentic/Agents/SwarmMember.cssrc/Agentic/Memory/MemoryAugmentedAgent.cssrc/Agentic/Memory/ThreadedAgent.cssrc/Agentic/Models/Connectors/AnthropicChatClient.cssrc/Agentic/Models/Connectors/MeaiChatClient.cssrc/Agentic/Models/ImageContent.cssrc/Agentic/Tools/DelegateAgentTool.cssrc/Agentic/Tools/JsonSchemaGenerator.cssrc/AiDotNet.Generators/AgentToolSchemaGenerator.cssrc/AiDotNet.Storage.Sqlite/SqliteConversationStore.cssrc/Models/Results/AiModelResult.cssrc/Reasoning/DomainSpecific/CodeReasoner.cs
CodeRabbit (#31 / #34) flagged these as "implementation helpers" and I narrowed them to internal in 7bcbed5 / ddf1c1a. On reflection both are user composition surfaces, not internal plumbing: - AgentAsTool<T> — turning any IAgent<T> into an IAgentTool is the building block users want when assembling their own multi-agent topologies outside the bundled SupervisorAgent / Swarm. - JsonSchemaGenerator — users implementing custom IAgentTool / structured-output flows reach for it directly to derive a JSON Schema from a C# type. Keeping it internal forces them to hand-roll the same walk over PropertyInfo / nullability metadata. Only internal plumbing should be internal; user-configurable composition primitives stay public.
…nder/schema parity) - SwarmMember: wrap handoffs snapshot in Array.AsReadOnly so callers can't downcast Handoffs to string[] and mutate routes after validation. - AnthropicChatClient: reject non-http(s) custom endpoints (UriKind.Absolute alone accepts file:/ftp:); store the normalized URI. - ImageContent.FromUri: restrict to http(s) web schemes (rejected file:/mailto:) and store the normalized URI — it is later serialized as a provider-fetched URL. - AgentToolSchemaGenerator: the generated binder now reuses IsRequired so a missing required reference parameter throws instead of binding default(T)/null, matching the schema's required set (previously only value types threw). - AiModelResult: restore the EvaluateBenchmarksAsync(chatClient, ct) facade overload (forwards with options: null) for back-compat.
… error
- AgentToolSchemaGenerator.TypeSchemaJson now recurses into complex object types
(public readable properties -> properties + required), cycle/depth-guarded
(MaxSchemaDepth=6), mirroring the runtime JsonSchemaGenerator.ForType so the
compile-time CreateAgentTools() path emits the same nested-object detail as
AgentToolFactory.ScanInstance() instead of a bare {"type":"object"} that
stripped DTO structure from the model's view. Required-property semantics match
ForType (Nullable<T> optional; value type required; reference type required only
when explicitly non-nullable).
- SqliteConversationStore: DbTransaction.CommitAsync is netstandard2.1/.NET Core
3.0+, absent on net471 -> CS1061 broke the solution build. Commit synchronously
under NETFRAMEWORK, async elsewhere.
Solution builds 0 errors on net10.0 + net471; 32 agentic schema/tool tests green.
ooples
pushed a commit
that referenced
this pull request
Jun 12, 2026
The flagship local-first differentiator: an IChatClient<T> that runs entirely in-process over AiDotNet's own model — no network, no API key. Same agent code drives it as drives OpenAI/Anthropic. - ICausalLanguageModel<T>: minimal next-token-logits seam the real Transformer (or any model) plugs into; implementations may keep an internal KV-cache. - IGenerationTokenizer (+TokenizerGenerationAdapter over the repo's ITokenizer): encode/decode/EOS seam. - TokenSampler<T>: greedy (temp 0 = argmax) + temperature + top-k + top-p (nucleus), seedable for reproducibility; reads logits via Convert.ToDouble so float/double share one path. - IChatPromptTemplate + ChatMlPromptTemplate (role-tagged, opens the assistant turn). - LocalEngineChatClient<T>: renders prompt -> encodes -> autoregressively samples until EOS or token limit -> decodes; non-streaming + streaming (incremental decode deltas); usage = prompt/generated token counts. Drop-in for agents/supervisor/swarm/memory. (Native tool-calling + structured-output constraints via constrained decoding are a follow-up; tools are ignored this slice.) - 9 tests (green net10.0 + net471) on a deterministic ScriptedCausalModel + WordTokenizer: greedy decode + EOS stop, token-limit -> Length, streaming deltas reconstruct full text, AgentExecutor drop-in, sampler argmax/seed-reproducibility/top-k/top-p, ChatML template. No null-forgiving. Stacked on Phase 2 (#1551) — the integration test shows the local engine driving an AgentExecutor. Part of epic #1544 (Phase 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ooples
added a commit
that referenced
this pull request
Jun 12, 2026
* feat(agentic): Phase 3 — local in-process inference engine (#1544) The flagship local-first differentiator: an IChatClient<T> that runs entirely in-process over AiDotNet's own model — no network, no API key. Same agent code drives it as drives OpenAI/Anthropic. - ICausalLanguageModel<T>: minimal next-token-logits seam the real Transformer (or any model) plugs into; implementations may keep an internal KV-cache. - IGenerationTokenizer (+TokenizerGenerationAdapter over the repo's ITokenizer): encode/decode/EOS seam. - TokenSampler<T>: greedy (temp 0 = argmax) + temperature + top-k + top-p (nucleus), seedable for reproducibility; reads logits via Convert.ToDouble so float/double share one path. - IChatPromptTemplate + ChatMlPromptTemplate (role-tagged, opens the assistant turn). - LocalEngineChatClient<T>: renders prompt -> encodes -> autoregressively samples until EOS or token limit -> decodes; non-streaming + streaming (incremental decode deltas); usage = prompt/generated token counts. Drop-in for agents/supervisor/swarm/memory. (Native tool-calling + structured-output constraints via constrained decoding are a follow-up; tools are ignored this slice.) - 9 tests (green net10.0 + net471) on a deterministic ScriptedCausalModel + WordTokenizer: greedy decode + EOS stop, token-limit -> Length, streaming deltas reconstruct full text, AgentExecutor drop-in, sampler argmax/seed-reproducibility/top-k/top-p, ChatML template. No null-forgiving. Stacked on Phase 2 (#1551) — the integration test shows the local engine driving an AgentExecutor. Part of epic #1544 (Phase 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(agentic): Phase 3 — wire real AiDotNet networks into local inference (#1544) - NeuralNetworkCausalLanguageModel<T>: adapts a trained NeuralNetworkBase<T> LM (Mamba/GLA/Transformer-LM-head) to ICausalLanguageModel<T>. Encodes context as the one-hot [1,seq,vocab] tensor these models expect, runs a forward pass (ResetState first so recurrent models start fresh), and extracts the final position's logits (handles rank-2 and rank-3 outputs). Full context re-fed each step; KV-cached fast path is a follow-up. - 3 integration tests over a real tiny untrained MambaLanguageModel<double> (greedy, deterministic): adapter logit width + no-NaN, end-to-end generation honoring the token budget, streaming termination. Pinned to CpuEngine + non-parallel collection (documented GPU-autodetect mitigation). - Fixed Engine_IsDropInForAgentExecutor: it relied on the engine's stochastic default sampling while asserting a fixed output; now configures greedy (Temperature 0) so the assertion is deterministic. Root-caused the flake to default-temperature sampling, not a library/GPU issue. - Local suite green + stable (verified repeated runs) on net10.0 + net471. No null-forgiving. Part of epic #1544 (Phase 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(agentic): Phase 3 — constrained decoding + stop sequences (#1544) Native LOCAL guaranteed-structure generation — enforced at the logits, not merely prompted (a capability cloud models can't guarantee): - ITokenConstraint seam: AllowedNextTokens(generated) returns null (free) / a set (restrict) / empty (terminal -> stop). TokenSampler.Sample gains an allowed-token mask honored by both greedy (argmax within allowed) and stochastic (candidates restricted before top-k/top-p). - AllowedTokenSetConstraint (fixed allow-list) + FiniteStateTokenConstraint (finite-state grammar over token ids: start set + per-token transitions; terminal states stop) — the general mechanism a JSON-schema/regular grammar compiles to. - LocalEngineOptions.Constraint wires it into LocalEngineChatClient generation (non-streaming + streaming). - Stop sequences: ChatOptions.StopSequences now halt generation and trim the output at the earliest match (both paths). - 5 tests (green, stable, net10.0 + net471): allowed-set greedy/stochastic masking, FSA forces an exact JSON-shaped sequence despite a model biased to 'garbage', allow-list restricts whole output, stop-sequence halts+trims. No null-forgiving. Part of epic #1544 (Phase 3). JSON-schema->token-grammar compiler (to drive structured output / tool-calling off this framework) is the next follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(agentic): Phase 3 — beam-search decoding (#1544) - LocalEngineOptions.BeamWidth (>1) switches non-streaming GetResponseAsync to beam search: explores N hypotheses in parallel, expands each by its top tokens (honoring the ITokenConstraint and stop sequences per beam), prunes to the top-N by length-normalized log-probability, and returns the best completion. Deterministic; streaming stays token-by-token. - LogSoftmax (with allowed-token masking → -inf) + length-normalized scoring + Beam bookkeeping. - 3 tests (green net10.0 + net471): greedy takes the locally-best token ("A"); beam width 2 discovers the globally better "BC" path the same model would miss greedily; beam respects a finite-state constraint ("AB"). No null-forgiving (removed an introduced allowed! via proper narrowing). Part of epic #1544 (Phase 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(agentic): Phase 3 — incremental (KV-cache) decoding seam + engine fast path (#1544) - IIncrementalCausalLanguageModel<T> : ICausalLanguageModel<T> adds ResetCache / StartSequence(prompt) / AppendToken(id) — the contract a KV-cached model implements to advance one token at a time instead of re-feeding the full O(n^2) context. - LocalEngineChatClient auto-detects the interface and takes the incremental fast path (prime once, then feed single tokens), falling back to full re-feed otherwise. Constraint + stop-sequence + sampler logic shared via a single PickToken helper across both paths. - 2 tests (green net10.0 + net471): the engine drives the incremental path correctly (ResetCache once, StartSequence with the prompt, AppendToken per generated token) and produces output IDENTICAL to the full-refeed path. The remaining half — a real K/V cache inside Mamba/GLA/attention forward — is a model-layer change (the network Predict API has no incremental entry point yet); this lands the engine-side support + verification it plugs into. Part of epic #1544 (Phase 3). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(agentic): Phase 3 — document quantization composition via ModelCompression (#1544) Quantization for the local engine is achieved by quantizing the NeuralNetworkBase<T> with the existing ModelCompression stack before wrapping it — the adapter accepts any such network unchanged. No engine-side code (no duplication of ModelCompression). Documented on NeuralNetworkCausalLanguageModel. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: franklinic <franklin@ivorycloud.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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
2 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.
Draft — do not review yet. Part of the agentic-orchestration epic #1544 (Phase 2). Stacked on Phase 0 (#1545) because the agent layer is built on Phase 0's
IChatClient<T>/ tools. Will be rebased once #1545 merges.Phase 2 goal
Multi-agent orchestration (supervisor / swarm / handoff) + conversation threads & long-term memory, built on the Phase 0 model+tools layer and (where useful) the Phase 1 graph runtime.
Landed so far
IAgent<T>— uniform agent contract (Name/Description/RunAsync) that both leaf agents and coordinators implement, so agents compose/nest.AgentExecutor<T>— native function-calling agent loop (call model → run requested tools → feed results back → repeat → final answer / iteration cap), replacing the legacy prompt-parsed ReAct loop. Aggregates usage; returns the full transcript.AgentExecutorOptions(nullable defaults) +AgentRunResult.ScriptedChatClient+RecordingTool.Planned next in this PR
No null-forgiving operators; enums for closed sets; "For Beginners" docs throughout.
🤖 Generated with Claude Code
Summary by CodeRabbit