feat: add Guard utility class and null check policy - #842
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughSystematic refactor: inline null-coalescing validations ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Validation/Guard.cs`:
- Around line 33-34: Change the Guard class visibility from public to internal
by updating the class declaration for Guard; this reduces the public surface so
only facade types (AiModelBuilder and AiModelResult) remain public. If unit
tests require access to Guard, add an InternalsVisibleTo entry for the test
assembly (e.g., in AssemblyInfo.cs or the project file) so tests can access
internal members; otherwise update any external callers to use the public facade
APIs instead. Ensure the symbol name "Guard" is used exactly when changing the
modifier and verify no other public helper classes are left exposed
unintentionally.
- Around line 187-234: Add explicit validation of the bounds in both overloads:
in InRange(int value, int min, int max, ...) verify that min <= max and throw an
ArgumentException (or ArgumentOutOfRangeException) with a clear message if not;
in InRange(double value, double min, double max, ...) first ensure min and max
are finite (use double.IsNaN/IsInfinity checks or double.IsFinite if available)
and that min <= max, throwing an ArgumentException for invalid bounds, then
proceed with the existing checks for NaN/infinity/value outside the range;
reference the InRange(int...) and InRange(double...) methods when making the
changes.
There was a problem hiding this comment.
Pull request overview
This PR establishes a centralized argument-validation approach for AiDotNet by introducing a Guard utility (with cross-target polyfills) and migrating a few representative call sites away from ?? throw patterns, while adding focused unit tests to validate runtime null behavior and numeric validation edge cases.
Changes:
- Added
AiDotNet.Validation.Guardwith common validation helpers (NotNull, string checks, numeric checks). - Added/extended compiler attribute polyfills to support
CallerArgumentExpression/[NotNull]on net471. - Migrated several constructors to
Guard.NotNull(...)and removed one redundant/unreachable null-check inLoRAAdapterBase.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/AiDotNet.Tests/UnitTests/Validation/GuardTests.cs | New unit tests covering Guard behavior and numeric edge cases (NaN/Infinity, boundaries). |
| tests/AiDotNet.Tests/UnitTests/Documentation/NullableReferenceTypeTests.cs | New “educational” tests intended to demonstrate NRT runtime behavior. |
| src/Validation/Guard.cs | New Guard utility API with XML docs and core validation methods. |
| src/Polyfills/CompilerAttributePolyfills.cs | Adds polyfills for CallerArgumentExpressionAttribute and NotNullAttribute under non-.NET Core/Std TFMs. |
| src/LoRA/Adapters/LoRAAdapterBase.cs | Removes a redundant null-check in base constructor argument evaluation. |
| src/Agents/AgentBase.cs | Migrates constructor null-check to Guard.NotNull. |
| src/AdversarialRobustness/Safety/SafetyFilter.cs | Migrates constructor null-check to Guard.NotNull. |
| src/AdversarialRobustness/Defenses/AdversarialTraining.cs | Migrates constructor null-check to Guard.NotNull. |
| src/AdversarialRobustness/Alignment/RLHFAlignment.cs | Migrates constructor null-check to Guard.NotNull. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Establish a formal null handling policy with a Guard utility class as the standard way to validate arguments. Add NotNull, NotNullOrEmpty, NotNullOrWhiteSpace, Positive, NonNegative, and InRange methods with CallerArgumentExpression support. Fix LoRAAdapterBase redundant null check and migrate 4 representative files to demonstrate the pattern. Closes #258 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Make Guard class internal to keep facade api surface clean - Add bound validation (min <= max, finite checks) to InRange overloads - Fix CallerArgumentExpression docs to reflect compiler/language support rather than target framework - Update polyfill comment to clarify Roslyn populates the attribute regardless of target framework with LangVersion 10+ - Fix NullableReferenceTypeTests to use #nullable enable with #pragma warning disable at call sites instead of file-wide #nullable disable - Add tests for InRange bound validation edge cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace ?? throw new ArgumentNullException(nameof(param)) pattern with Guard.NotNull(param) throughout the codebase. This standardizes null argument validation on the Guard utility class introduced in this PR. Two patterns converted: - _field = param ?? throw ... -> Guard.NotNull(param); _field = param; - _ = param ?? throw ... -> Guard.NotNull(param); Remaining ~108 complex patterns (IOptions unwrapping, property access checks, chained calls) will be addressed in follow-up commits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…iles Handle additional patterns not covered by the bulk migration: - IOptions?.Value ?? throw -> Guard.NotNull + .Value access - param?.ToList()/ToArray() ?? throw -> Guard.NotNull + materialization - taskData.property ?? throw -> Guard.NotNull on property - Multi-line throw with message -> Guard.NotNull (message removed) - Dictionary/indexer assignment -> Guard.NotNull + assignment - options?.Property ?? throw with message -> Guard.NotNull chain Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Convert 30 additional patterns including: - IOptions?.Value unwrapping to Guard.NotNull + .Value access - Collection?.ToList()/ToArray() to Guard.NotNull + materialization - Chained (param ?? throw).Method() to Guard.NotNull + method call - Dictionary/collection assignment patterns - Array element validation with custom parameter names - MetaLearning Options one-liner constructors 27 patterns remain in base() constructor chains where Guard.NotNull cannot be called before base() - this is a C# language limitation and the ?? throw pattern is correct and idiomatic there. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a793e2c to
7fd29bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (24)
src/Augmentation/Compose.cs (1)
162-173: 🛠️ Refactor suggestion | 🟠 MajorInconsistent null-check pattern in
Withmethod.This method still uses the old inline null-check pattern (
if (augmentation is null) throw ...) while line 77 in the constructor now usesGuard.NotNull(). For consistency across the codebase—which is the stated goal of this PR—this should also use the Guard utility.♻️ Proposed fix for consistency
public Compose<T, TData> With(IAugmentation<T, TData> augmentation) { - if (augmentation is null) - throw new ArgumentNullException(nameof(augmentation)); + Guard.NotNull(augmentation); var newList = new List<IAugmentation<T, TData>>(_augmentations) { augmentation }; return new Compose<T, TData>(newList, Probability)src/AiDotNet.Serving/Security/ApiKeys/ApiKeyService.cs (1)
83-86: 🛠️ Refactor suggestion | 🟠 MajorInconsistent null-check pattern — should use Guard.NotNull for consistency.
The constructor was migrated to
Guard.NotNull(), butCreateAsyncstill uses the old inline throw pattern. This contradicts the PR's stated goal of establishing a consistent null-check policy across the codebase.♻️ Proposed fix to use Guard.NotNull
public async Task<ApiKeyCreateResponse> CreateAsync(ApiKeyCreateRequest request, CancellationToken cancellationToken = default) { - if (request == null) - { - throw new ArgumentNullException(nameof(request)); - } + Guard.NotNull(request); if (string.IsNullOrWhiteSpace(request.Name))src/Data/Loaders/StatefulDataLoader.cs (1)
100-105: 🧹 Nitpick | 🔵 TrivialInconsistent null-check pattern within the same file.
The constructor now uses
Guard.NotNull(inner)butLoadStatestill uses the oldif (state is null) throw new ArgumentNullException(...)pattern. For consistency with the PR's null-check policy and internal uniformity, this should also useGuard.NotNull.♻️ Proposed fix for consistency
public void LoadState(DataLoaderCheckpoint state) { - if (state is null) - { - throw new ArgumentNullException(nameof(state)); - } + Guard.NotNull(state); if (state.TotalCount != TotalCount)src/AiDotNet.Serving/Services/ProtectedModelArtifact.cs (1)
10-19:⚠️ Potential issue | 🟠 MajorInconsistent null validation within the constructor.
The PR's objective is to establish a consistent null-checking policy, yet only
keyandnonceare validated here whilemodelName,encryptedPath,keyId, andalgorithmare assigned without any null checks. This inconsistency within the same constructor undermines the policy being established.For a class representing protected model artifacts, having a null
modelName,keyId, oralgorithmwould likely cause issues downstream.🛡️ Proposed fix for consistent validation
public ProtectedModelArtifact(string modelName, string encryptedPath, string keyId, byte[] key, byte[] nonce, string algorithm) { - ModelName = modelName; - EncryptedPath = encryptedPath; - KeyId = keyId; + Guard.NotNullOrWhiteSpace(modelName); + Guard.NotNullOrWhiteSpace(encryptedPath); + Guard.NotNullOrWhiteSpace(keyId); Guard.NotNull(key); Guard.NotNull(nonce); + Guard.NotNullOrWhiteSpace(algorithm); + ModelName = modelName; + EncryptedPath = encryptedPath; + KeyId = keyId; Key = key.ToArray(); Nonce = nonce.ToArray(); - Algorithm = algorithm; + Algorithm = algorithm; }src/AiDotNet.Serving/Security/ServingRequestContextMiddleware.cs (1)
16-36: 🧹 Nitpick | 🔵 TrivialImplementation looks solid.
The
InvokeAsyncmethod is production-ready with proper error handling and consistent use ofConfigureAwait(false). The DI-injected parameters (resolver,accessor) don't have explicitGuard.NotNull()calls, but ASP.NET Core's method injection throws a clearInvalidOperationExceptionduring service resolution if dependencies aren't registered—so explicit checks are defensive rather than necessary.For consistency with the new Guard pattern, you could add defensive checks, but this is entirely optional since the DI container's failure mode is already clear and actionable:
Guard.NotNull(resolver); Guard.NotNull(accessor);,
src/AiDotNet.Serving/Sandboxing/Sql/SqlSandboxExecutor.cs (1)
52-53: 🧹 Nitpick | 🔵 TrivialInconsistent null-checking pattern — consider migrating to Guard.NotNull().
The constructor was migrated to use
Guard.NotNull(), butExecuteAsyncstill uses the inlineis nullthrow pattern. For consistency with this PR's objective of establishing a uniform null-check policy, these should also use the new Guard utility.♻️ Suggested fix for consistency
- if (request is null) throw new ArgumentNullException(nameof(request)); - if (requestContext is null) throw new ArgumentNullException(nameof(requestContext)); + Guard.NotNull(request); + Guard.NotNull(requestContext);src/Augmentation/LabelMixingEventArgs.cs (1)
161-171: 🧹 Nitpick | 🔵 TrivialInconsistent null validation:
AugmentationAppliedEventArgsconstructor lacks Guard checks.While
LabelMixingEventArgswas migrated to useGuard.NotNull(), this constructor in the same file still acceptsaugmentationNameandparameterswithout validation. This undermines the PR's objective of establishing a consistent null-check policy.Both
augmentationName(string) andparameters(IDictionary) are nullable reference types that should be validated for consistency with the established pattern.♻️ Proposed fix for consistency
public AugmentationAppliedEventArgs( string augmentationName, IDictionary<string, object> parameters, int sampleIndex, bool wasApplied) { - AugmentationName = augmentationName; - Parameters = parameters; + Guard.NotNull(augmentationName); + AugmentationName = augmentationName; + Guard.NotNull(parameters); + Parameters = parameters; SampleIndex = sampleIndex; WasApplied = wasApplied; }src/AiDotNet.Serving/ProgramSynthesis/ServingCodeTaskExecutor.cs (1)
26-27: 🧹 Nitpick | 🔵 TrivialInconsistent null-check patterns within the same file.
The constructor uses the new
Guard.NotNull()pattern, butExecuteAsyncstill uses the legacyif (x is null) throwpattern. For maintainability and consistency, consider migrating these checks toGuard.NotNull()as well.♻️ Suggested refactor for consistency
public Task<CodeTaskResultBase> ExecuteAsync( CodeTaskRequestBase request, ServingRequestContext requestContext, CancellationToken cancellationToken) { - if (request is null) throw new ArgumentNullException(nameof(request)); - if (requestContext is null) throw new ArgumentNullException(nameof(requestContext)); + Guard.NotNull(request); + Guard.NotNull(requestContext); // The current task implementations are synchronous and deterministic; keep the Serving surface async for evolution.src/AnomalyDetection/OutlierRemovalAdapter.cs (1)
125-133: 🧹 Nitpick | 🔵 TrivialConsider migrating remaining null checks to Guard.NotNull for consistency.
The
ConvertToMatrixVectormethod still uses manualthrow new ArgumentNullExceptionpatterns. While the PR notes bulk migration is deferred (~482 sites), since this file is already being touched, migrating these two checks would maintain consistency within the same file.♻️ Suggested refactor for consistency
private static (Matrix<T> inputs, Vector<T> outputs) ConvertToMatrixVector(TInput inputs, TOutput outputs) { - if (inputs == null) - { - throw new ArgumentNullException(nameof(inputs), "Inputs cannot be null."); - } - - if (outputs == null) - { - throw new ArgumentNullException(nameof(outputs), "Outputs cannot be null."); - } + Guard.NotNull(inputs); + Guard.NotNull(outputs); Matrix<T> inputMatrix; Vector<T> outputVector;Note: The custom messages ("Inputs cannot be null." / "Outputs cannot be null.") would be lost with
Guard.NotNull, but the parameter names are self-documenting. If custom messages are required, this refactor can be skipped.src/AiDotNet.Serving/ProgramSynthesis/ServingProgramEvaluator.cs (2)
40-41: 🛠️ Refactor suggestion | 🟠 MajorInconsistent null-check pattern within the same file.
The constructor now uses
Guard.NotNull()but these method parameters still use the oldif ... throwpattern. For consistency with the PR's null-check policy migration, these should also useGuard.NotNull().♻️ Proposed fix for consistency
- if (request is null) throw new ArgumentNullException(nameof(request)); - if (requestContext is null) throw new ArgumentNullException(nameof(requestContext)); + Guard.NotNull(request); + Guard.NotNull(requestContext);
88-88: 🧹 Nitpick | 🔵 TrivialSilent null replacement could mask caller bugs.
If
testCases[i]is null, this silently creates an empty example instead of failing fast. An emptyProgramInputOutputExamplewould pass validation but produce confusing test results with empty input/output. Consider whether failing explicitly would be more appropriate here.💡 Alternative: fail fast on null elements
- var example = testCases[i] ?? new ProgramInputOutputExample(); + var example = testCases[i]; + Guard.NotNull(example);src/AiDotNet.Serving/Models/ServableModelWrapper.cs (1)
67-77: 🧹 Nitpick | 🔵 TrivialUnify null‑checks with Guard in this constructor.
This constructor still uses a manual
ArgumentNullExceptionwhile the rest of the PR standardizes onGuard.NotNull. Aligning here improves consistency and preservesCallerArgumentExpressionbehavior.♻️ Suggested refactor
- if (regressionModel == null) - { - throw new ArgumentNullException(nameof(regressionModel)); - } + Guard.NotNull(regressionModel);src/ContinualLearning/MemoryAwareSynapses.cs (1)
87-94:⚠️ Potential issue | 🔴 CriticalBlocking: validate non-empty inputs to prevent divide-by-zero.
ComputeOutputSensitivitydivides byinputs.Shape[0]; if the batch is empty this will blow up or produce invalid values. Add an explicit guard right after the null check.✅ Suggested fix
Guard.NotNull(network); Guard.NotNull(taskData.inputs); + if (taskData.inputs.Shape[0] == 0) + { + throw new ArgumentException("inputs must contain at least one sample", nameof(taskData.inputs)); + }As per coding guidelines: “Production Readiness (CRITICAL - Flag as BLOCKING)… missing validation of external inputs.”
src/ContinualLearning/GenerativeReplay.cs (1)
202-213:⚠️ Potential issue | 🔴 CriticalBlocking: validate external inputs before computing replay counts.
batchSizeand_replayRatioare external/constructor-configured inputs; without validation, negative or >1 ratios can yield negative counts or oversampling and can propagate intoGenerateSamples. Add explicit validation before use.✅ Suggested fix
public (Tensor<T> inputs, Tensor<T> targets) CreateMixedBatch( Tensor<T> currentInputs, Tensor<T> currentTargets, int batchSize) { Guard.NotNull(currentInputs); Guard.NotNull(currentTargets); + Guard.Positive(batchSize); + Guard.InRange(_replayRatio, 0d, 1d); if (_generator == null || _taskCount == 0) { return (currentInputs, currentTargets); }As per coding guidelines, "missing validation of external inputs" is a blocking production‑readiness issue and must be fixed.
src/ContinualLearning/LearningWithoutForgetting.cs (2)
80-84:⚠️ Potential issue | 🟡 MinorGuard against NaN/Infinity before clamping.
Math.Max(0.1, value)leaves NaN unchanged and accepts Infinity, which can poison downstream softmax math.✅ Suggested fix
public double Temperature { get => _temperature; - set => _temperature = Math.Max(0.1, value); + set + { + if (double.IsNaN(value) || double.IsInfinity(value)) + { + throw new ArgumentOutOfRangeException(nameof(value), "Temperature must be finite."); + } + _temperature = Math.Max(0.1, value); + } }
110-121:⚠️ Potential issue | 🟠 MajorEnsure training mode is restored on exceptions.
IfPredictthrows, the network can be left in evaluation mode, which is a reliability bug.🔧 Suggested fix (ensure restore)
// Record the network's current predictions (before training on new task) network.SetTrainingMode(false); -var predictions = network.Predict(newTaskInputs); -_oldPredictions[taskId] = predictions.Clone(); -network.SetTrainingMode(true); +try +{ + var predictions = network.Predict(newTaskInputs); + _oldPredictions[taskId] = predictions.Clone(); +} +finally +{ + network.SetTrainingMode(true); +}src/DistributedTraining/ShardedModelBase.cs (1)
323-326: 🧹 Nitpick | 🔵 TrivialInconsistent null-check pattern in
SetParameters.This method uses the old explicit
throw new ArgumentNullExceptionpattern while the constructor now usesGuard.NotNull. For consistency with the PR's null-check policy, consider migrating this as well.♻️ Suggested refactor for consistency
public virtual void SetParameters(Vector<T> parameters) { - if (parameters == null) - { - throw new ArgumentNullException(nameof(parameters)); - } + Guard.NotNull(parameters); if (parameters.Length != ParameterCount)src/ContinualLearning/ProgressiveNeuralNetworks.cs (1)
149-154:⚠️ Potential issue | 🟡 MinorMissing Guard.NotNull in
ComputeLoss.The
ComputeLossmethod does not validate itsnetworkparameter, whileBeforeTask,AfterTask, andModifyGradientsall do. This inconsistency could lead to aNullReferenceExceptionif the method is called with a null network.🛡️ Suggested fix for consistency
public T ComputeLoss(INeuralNetwork<T> network) { + Guard.NotNull(network); + // Progressive networks use parameter freezing, not loss-based regularizationsrc/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs (1)
85-93: 🧹 Nitpick | 🔵 TrivialConsider migrating remaining null checks for consistency.
While the PR notes that bulk migration is deferred, this file has mixed patterns: the constructor uses
Guard.NotNullbutCertifyPrediction,CertifyBatch,ComputeCertifiedRadius, and other public methods still use explicit null checks. For intra-file consistency, consider migrating these in a follow-up.src/AdversarialRobustness/Alignment/RLHFAlignment.cs (3)
241-243:⚠️ Potential issue | 🔴 CriticalBlocking:
Reset()is an empty stub.This is a blocking production-readiness issue; the method currently does nothing.
🛠️ Suggested fix
- public void Reset() { } + public void Reset() + { + rewardModel = null; + }As per coding guidelines: "Every PR must contain production-ready code. Flag ALL of the following as blocking issues... empty method bodies."
313-340:⚠️ Potential issue | 🔴 CriticalBlocking: reward-model training and RL fine-tuning are placeholders.
These methods explicitly note simplified/placeholder behavior, which is not production-ready and can mislead users.
🛠️ Suggested fix (wire to real trainers)
+ private readonly IRewardModelTrainer<T> _rewardModelTrainer; + private readonly IRlFineTuner<T> _fineTuner; @@ - public RLHFAlignment(AlignmentMethodOptions<T> options) + public RLHFAlignment( + AlignmentMethodOptions<T> options, + IRewardModelTrainer<T> rewardModelTrainer, + IRlFineTuner<T> fineTuner) { Guard.NotNull(options); this.options = options; + Guard.NotNull(rewardModelTrainer); + _rewardModelTrainer = rewardModelTrainer; + Guard.NotNull(fineTuner); + _fineTuner = fineTuner; } @@ - private Func<Vector<T>, Vector<T>, double> TrainRewardModel(AlignmentFeedbackData<T> feedbackData) - { - // Train a reward model from human preference comparisons - // This is a simplified placeholder - real implementation would use neural networks - return (input, output) => - { - double sum = 0.0; - for (int i = 0; i < output.Length; i++) - { - sum += NumOps.ToDouble(output[i]); - } - var outputMean = output.Length > 0 ? (sum / output.Length) : 0.0; - var reward = 1.0 - Math.Abs(outputMean - 0.5); - return MathHelper.Clamp(reward, 0.0, 1.0); - }; - } + private Func<Vector<T>, Vector<T>, double> TrainRewardModel(AlignmentFeedbackData<T> feedbackData) + => _rewardModelTrainer.Train(feedbackData); @@ - private IPredictiveModel<T, Vector<T>, Vector<T>> FinetuneWithRL( - IPredictiveModel<T, Vector<T>, Vector<T>> baseModel, - AlignmentFeedbackData<T> feedbackData, - Func<Vector<T>, Vector<T>, double> rewardModelFunc) - { - _ = feedbackData; - return new RlhfFineTunedPredictiveModel(baseModel, rewardModelFunc, options.KLCoefficient); - } + private IPredictiveModel<T, Vector<T>, Vector<T>> FinetuneWithRL( + IPredictiveModel<T, Vector<T>, Vector<T>> baseModel, + AlignmentFeedbackData<T> feedbackData, + Func<Vector<T>, Vector<T>, double> rewardModelFunc) + => _fineTuner.FineTune(baseModel, feedbackData, rewardModelFunc, options);As per coding guidelines: "Every PR must contain production-ready code. Flag ALL of the following as blocking issues... Simplified implementations."
342-387:⚠️ Potential issue | 🔴 CriticalBlocking: critique/revision and honesty checks are placeholders.
These methods are explicitly simplified and return default behavior, which is not production-ready.
🛠️ Suggested fix (delegate to constitutional evaluator)
+ private readonly IConstitutionalEvaluator<T> _constitutionalEvaluator; @@ - public RLHFAlignment( - AlignmentMethodOptions<T> options, - IRewardModelTrainer<T> rewardModelTrainer, - IRlFineTuner<T> fineTuner) + public RLHFAlignment( + AlignmentMethodOptions<T> options, + IRewardModelTrainer<T> rewardModelTrainer, + IRlFineTuner<T> fineTuner, + IConstitutionalEvaluator<T> constitutionalEvaluator) { @@ + Guard.NotNull(constitutionalEvaluator); + _constitutionalEvaluator = constitutionalEvaluator; } @@ - private static string GenerateCritique(Vector<T> response, string[] principles) - { - _ = response; - return $"Response evaluated against {principles.Length} constitutional principles"; - } + private string GenerateCritique(Vector<T> response, string[] principles) + => _constitutionalEvaluator.GenerateCritique(response, principles); @@ - private static Vector<T> ReviseBasedOnCritique( - IPredictiveModel<T, Vector<T>, Vector<T>> model, - Vector<T> input, - Vector<T> response, - string critique) - { - _ = model; - _ = input; - _ = critique; - return response; - } + private Vector<T> ReviseBasedOnCritique( + IPredictiveModel<T, Vector<T>, Vector<T>> model, + Vector<T> input, + Vector<T> response, + string critique) + => _constitutionalEvaluator.Revise(model, input, response, critique); @@ - private bool IsHonest(Vector<T> output, Vector<T> input) - { - _ = output; - _ = input; - return true; // Placeholder - } + private bool IsHonest(Vector<T> output, Vector<T> input) + => _constitutionalEvaluator.IsHonest(output, input);As per coding guidelines: "Every PR must contain production-ready code. Flag ALL of the following as blocking issues... Simplified implementations."
src/Deployment/Mobile/Android/NNAPIBackend.cs (1)
109-171:⚠️ Potential issue | 🔴 CriticalBlocking: Placeholder NNAPI methods return hardcoded/identity results.
Lines 109–171 contain multiple placeholders (
IsNNAPIAvailablehardcodedtrue,InitializeRuntime/CompileForNNAPIno-ops,ExecuteOnNNAPIidentity copy,GetSupportedOperationshardcoded list). This makes the backend appear functional while doing nothing, which is unsafe in production. Production-ready behavior must integrate real NNAPI calls or fail fast to avoid silent incorrect results.🛑 Minimum safe fallback (fail fast instead of returning fake results)
public static bool IsNNAPIAvailable() { - // Check Android API level - // NNAPI was introduced in Android 8.1 (API level 27) - // This would check the actual Android API level at runtime - return true; // Placeholder + throw new PlatformNotSupportedException( + "NNAPI availability check is not implemented. Wire to Android NNAPI APIs."); } private void InitializeRuntime() { - // Initialize NNAPI runtime with configuration - // This would call Android NNAPI initialization functions + throw new PlatformNotSupportedException( + "NNAPI runtime initialization is not implemented. Wire to Android NNAPI APIs."); } private void CompileForNNAPI(byte[] modelData) { - // Compile the model for NNAPI execution - // This involves: - // 1. Parsing the model format (TFLite or ONNX) - // 2. Creating NNAPI model - // 3. Adding operations - // 4. Compiling for target device + throw new PlatformNotSupportedException( + "NNAPI model compilation is not implemented. Wire to Android NNAPI APIs."); } private T[] ExecuteOnNNAPI(T[] input) { - // Execute inference using NNAPI - // This would: - // 1. Set input tensors - // 2. Execute computation - // 3. Get output tensors - // 4. Return results - - // Placeholder implementation - var output = new T[input.Length]; - Array.Copy(input, output, input.Length); - return output; + throw new PlatformNotSupportedException( + "NNAPI execution is not implemented. Wire to Android NNAPI APIs."); } private List<string> GetSupportedOperations() { - // Query NNAPI for supported operations on this device - return new List<string> - { - "CONV_2D", "DEPTHWISE_CONV_2D", "FULLY_CONNECTED", - "MAX_POOL_2D", "AVERAGE_POOL_2D", "SOFTMAX", - "RELU", "RELU6", "ADD", "MUL" - }; + throw new PlatformNotSupportedException( + "NNAPI supported-operations query is not implemented. Wire to Android NNAPI APIs."); }As per coding guidelines: "Production Readiness (CRITICAL - Flag as BLOCKING)" and "Stubs/Placeholders... or placeholder return values (e.g.,
return default;when real logic is needed)" must be fixed before release.src/ActiveLearning/VariationRatios.cs (1)
224-224:⚠️ Potential issue | 🔴 CriticalBLOCKING: C# 12 collection expression breaks net471 compatibility.
The code uses C# 12 collection expression syntax
[.. selected], but AiDotNet targets net471 (which defaults to C# 7.3). This will fail to compile on the .NET Framework 4.7.1 target. Revert toToArray()immediately.Required fix
- return [.. selected]; + return selected.ToArray();
🤖 Fix all issues with AI agents
In `@src/AiDotNet.Serving/Models/ServableModelWrapper.cs`:
- Around line 41-46: The constructor currently uses Guard.NotNull(modelName)
which allows empty or whitespace-only names; update the validation to reject
those by calling a NotNullOrWhiteSpace-style guard (e.g.,
Guard.NotNullOrWhiteSpace(modelName)) or add an explicit whitespace check before
assigning to _modelName in the ServableModelWrapper constructor so _modelName
cannot be "" or " " (keep the rest of the assignments to _inputDimension,
_outputDimension, and _predictFunc unchanged).
In `@src/AiDotNet.Serving/ProgramSynthesis/ServingCodeTaskResultRedactor.cs`:
- Around line 14-17: The constructor of ServingCodeTaskResultRedactor currently
only guards the IOptions parameter itself, but not options.Value; add a null
check for options.Value and throw/guard accordingly before assigning to _options
so that ServingCodeTaskResultRedactor's constructor validates both the options
object and its Value (i.e., ensure options.Value is non-null prior to _options =
options.Value).
In `@src/AiDotNet.Serving/Security/HeaderTierResolver.cs`:
- Around line 15-19: Constructor of HeaderTierResolver currently only guards the
IOptions<TierEnforcementOptions> wrapper; also validate the contained value:
after retrieving options.Value in the HeaderTierResolver constructor (or before
assigning to _options), check that the value is non-null and throw a clear
exception if it is null (e.g., call Guard.NotNull on the extracted
TierEnforcementOptions instance or explicit null-check), so
HeaderTierResolver._options is guaranteed non-null when used later.
In `@src/AiDotNet.Serving/Services/ModelRegistryLoader.cs`:
- Around line 42-45: The file mixes Guard.NotNull() and old null-check throws;
update the remaining checks in LoadWithServableModel and RefreshModel to use
Guard.NotNull(...) like the constructor (e.g., replace patterns that do "if (x
== null) throw new ArgumentNullException(...)" with "Guard.NotNull(x);"),
ensuring you reference the same parameter names used in those methods and
preserve any custom parameter names/messages by passing them into Guard.NotNull;
keep assignments and logic unchanged after validation.
In `@src/AiDotNet.Serving/Services/ModelStartupService.cs`:
- Around line 58-63: The null-check currently only verifies the
IOptions<ModelOptions> parameter (options) but not its Value, so _options can
end up null; add an explicit guard for the options.Value before assignment
(e.g., call Guard.NotNull(options.Value) or Guard.NotNull(options?.Value)
immediately after validating options) and then set _options = options.Value to
ensure ModelStartupService._options is never null.
In `@src/AutoML/RL/RandomSearchRLAutoML.cs`:
- Around line 34-37: Replace the inline null-check in RecordFailedTrial with the
Guard helper for consistency: remove the if (summary is null) { throw new
ArgumentNullException(nameof(summary)); } block and call Guard.NotNull(summary)
at the start of the RecordFailedTrial method so it matches the rest of the
file’s null-check pattern (refer to the RecordFailedTrial method and the summary
parameter).
In `@src/Data/Loaders/StreamingDataLoader.cs`:
- Around line 239-242: Replace the loose null check on filePath with a
whitespace-aware check: call Guard.NotNullOrWhiteSpace(filePath) before
assigning to _filePath so empty or whitespace-only paths are rejected early;
leave the Guard.NotNull(lineParser) and assignment to _lineParser as-is to
preserve the existing parser validation.
In `@src/Data/Sampling/DataSamplerBase.cs`:
- Around line 177-183: The Weights property setter uses Guard.NotNull but lacks
the same non-empty validation and error message as the constructor; update the
setter for consistency by checking both null and empty (e.g., use
Guard.NotNull(value) then if (!value.Any()) throw new ArgumentException("Weights
cannot be empty", nameof(value))) before assigning WeightsArray and calling
ComputeCumulativeProbabilities(), mirroring the constructor's behavior and
ensuring ComputeCumulativeProbabilitiesCore still receives a valid non-empty
array.
In `@src/Deployment/Edge/EdgeOptimizer.cs`:
- Around line 21-22: The file mixes null-check styles; replace the explicit null
checks in OptimizeForEdge and PartitionModel with the Guard helper to match the
constructor: remove the if (model == null) throw new
ArgumentNullException(nameof(model)); blocks and call Guard.NotNull(model) at
the start of both OptimizeForEdge and PartitionModel so the file consistently
uses Guard.NotNull for model validation.
…eployment - ServableModelWrapper: use NotNullOrWhiteSpace for modelName - ServingCodeTaskResultRedactor: guard options.Value - HeaderTierResolver: guard options.Value - ModelRegistryLoader: replace manual null checks with Guard in LoadWithServableModel/RefreshModel - ModelStartupService: guard options.Value - RandomSearchRLAutoML: replace manual null check with Guard.NotNull in RecordFailedTrial - CsvStreamingDataLoader: use NotNullOrWhiteSpace for filePath - WeightedSamplerBase: add empty-array check in Weights setter - EdgeOptimizer: replace manual null checks with Guard.NotNull in OptimizeForEdge/PartitionModel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 (3)
src/AiDotNet.Serving/Models/ServableModelWrapper.cs (1)
67-76: 🧹 Nitpick | 🔵 TrivialConsider using Guard.NotNull for regressionModel to stay consistent.
Keeps validation style uniform and leverages CallerArgumentExpression.♻️ Suggested change
- if (regressionModel == null) - { - throw new ArgumentNullException(nameof(regressionModel)); - } + Guard.NotNull(regressionModel);src/Deployment/Edge/EdgeOptimizer.cs (1)
71-89:⚠️ Potential issue | 🔴 CriticalBLOCKING: PartitionModel is non-functional due to placeholder Extract methods.*
PartitionModelalways throws becauseExtractEdgeLayers/ExtractCloudLayersare stubbed. Production-ready code should either implement real partitioning for supported models or avoid exposing this API until it’s implemented. A concrete path is to require a partitionable model interface and only throw when the model doesn’t implement it.✅ Concrete fix (delegate to a partitionable model)
public PartitionedModel<T, TInput, TOutput> PartitionModel(IFullModel<T, TInput, TOutput> model) { Guard.NotNull(model); + if (model is not IPartitionableModel<T, TInput, TOutput> partitionable) + { + throw new NotSupportedException( + "Model partitioning requires IPartitionableModel. Implement the interface on your model " + + "or remove partitioning from the public API until supported."); + } + var partitioned = new PartitionedModel<T, TInput, TOutput> { OriginalModel = model, PartitionStrategy = _config.PartitionStrategy }; // Analyze model and determine optimal partition point var partitionPoint = DeterminePartitionPoint(model); // Split model into edge and cloud parts - partitioned.EdgeModel = ExtractEdgeLayers(model, 0, partitionPoint); - partitioned.CloudModel = ExtractCloudLayers(model, partitionPoint); + partitioned.EdgeModel = partitionable.ExtractEdgeLayers(0, partitionPoint); + partitioned.CloudModel = partitionable.ExtractCloudLayers(partitionPoint); return partitioned; }As per coding guidelines: “Production Readiness (CRITICAL - Flag as BLOCKING) … Stubs/Placeholders … Incomplete features.”
Also applies to: 261-319
src/AiDotNet.Serving/Services/ModelRegistryLoader.cs (1)
62-110:⚠️ Potential issue | 🔴 CriticalBlocking: LoadFromRegistry constructs a placeholder servable model that throws at inference time.*
This is a non-production placeholder (explicitly called out as “future implementation”), and it registers a model that will always fail at prediction. This must be fixed before release.
Production-ready behavior should either (a) fully deserialize and build a functional servable model or (b) refuse to load and clearly instruct callers to use
LoadWithServableModel.✅ Concrete fix (fail fast + require a real predict delegate)
- // Create a placeholder predict function that throws if the model isn't properly loaded - // In a full implementation, you would load the serialized model from StoragePath - var servableModel = new ServableModelWrapper<T>( - name, - inputDimension, - outputDimension, - input => throw new InvalidOperationException( - $"Model '{name}' requires proper deserialization from storage path: {registeredModel.StoragePath}"), - null, - enableBatching: true, - enableSpeculativeDecoding: false); + if (predictFunc is null) + { + throw new InvalidOperationException( + "predictFunc must be provided until model deserialization is implemented. " + + "Use LoadWithServableModel for pre-loaded models."); + } + + var servableModel = new ServableModelWrapper<T>( + name, + inputDimension, + outputDimension, + predictFunc, + null, + enableBatching: true, + enableSpeculativeDecoding: false);Apply the same pattern to
LoadFromRegistryByStage.As per coding guidelines: “Production Readiness (CRITICAL - Flag as BLOCKING): Stubs/Placeholders… incomplete features.”
Also applies to: 135-173
Summary
src/Validation/Guard.cs) withNotNull,NotNullOrEmpty,NotNullOrWhiteSpace,Positive,NonNegative, andInRangemethods — the standard way to validate arguments across AiDotNet[NotNull],[CallerArgumentExpression]on net471 so Guard works on all target frameworks?? throwonLoRAAdapterBaseline 169 (second check was unreachable since line 168 already validated)?? throwtoGuard.NotNull():AgentBase,RLHFAlignment,AdversarialTraining,SafetyFilterGuardTests(all methods, edge cases, NaN/Infinity) andNullableReferenceTypeTests(educational: proves NRT is compile-time only)Design decisions
Guard.NotNull(x); _field = x;[CallerArgumentExpression]auto-fills parameter name on net6+; on net471, callers passnameof()explicitly?? throwsites is a separate future effort.Test plan
dotnet build --no-restorepasses with 0 errors on both net10.0 and net471dotnet test --filter "GuardTests|NullableReferenceTypeTests"— 59 tests pass on both TFMsCloses #258
🤖 Generated with Claude Code
Summary by CodeRabbit