fix: add comprehensive commitlint support for dependabot and proactive fixes - #436
Conversation
|
Warning Rate limit exceeded@ooples has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a large Adversarial Robustness & AI Safety subsystem: new interfaces and implementations for attacks, defenses, certified verifiers, many fine‑tuning strategies (incl. RLHF/PPO), safety/content classifiers and prediction‑path wiring, model‑card tooling, numeric/statistics helpers, Newtonsoft JSON migration for prompt tooling, registry/model‑card integration, and extensive tests. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a comprehensive Adversarial Robustness and AI Safety module to the AiDotNet library. The module provides state-of-the-art techniques for securing AI systems against adversarial attacks, ensuring alignment with human values, and implementing safety filters.
Key Changes:
- Implementation of adversarial attack algorithms (FGSM, PGD, C&W, AutoAttack)
- Defense mechanisms including adversarial training and certified robustness
- AI alignment methods with RLHF support
- Comprehensive safety filtering infrastructure
- Model documentation tools (Model Cards)
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 30 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Models/SafetyValidationResult.cs | Data model for input safety validation results |
| src/Models/SafetyFilterResult.cs | Data model for output filtering results |
| src/Models/RobustnessMetrics.cs | Metrics for evaluating adversarial robustness |
| src/Models/RedTeamingResults.cs | Results from red teaming testing |
| src/Models/JailbreakDetectionResult.cs | Data model for jailbreak attempt detection |
| src/Models/HarmfulContentResult.cs | Results from harmful content identification |
| src/Models/CertifiedPrediction.cs | Certified predictions with robustness guarantees |
| src/Models/CertifiedAccuracyMetrics.cs | Metrics for certified accuracy evaluation |
| src/Models/AlignmentMetrics.cs | Metrics for AI alignment evaluation |
| src/Models/AlignmentFeedbackData.cs | Human feedback data for alignment |
| src/Models/AlignmentEvaluationData.cs | Test cases for alignment evaluation |
| src/Models/Options/*.cs | Configuration options classes for various features |
| src/Interfaces/*.cs | Interface definitions for attacks, defenses, alignment, and safety |
| src/AdversarialRobustness/Safety/SafetyFilter.cs | Implementation of comprehensive safety filtering |
| src/AdversarialRobustness/Defenses/AdversarialTraining.cs | Adversarial training defense implementation |
| src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs | Randomized smoothing for certified robustness |
| src/AdversarialRobustness/Attacks/*.cs | Attack algorithm implementations |
| src/AdversarialRobustness/Alignment/RLHFAlignment.cs | RLHF alignment implementation |
| src/AdversarialRobustness/Documentation/ModelCard.cs | Model card generation for documentation |
| src/AdversarialRobustness/README.md | Comprehensive module documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
da6a6ee to
cd0fc3a
Compare
github-actions
Bot
changed the title
|
🤖 PR Title Auto-Fixed Your PR title was automatically updated to follow Conventional Commits format. Original title: New title: Detected type: Valid types and their effects:
If the detected type is incorrect, you can manually edit the PR title. |
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (2)
src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (1)
126-141: Explicit loop retained for performance.The explicit
foreachloop (rather than LINQ.Select()) was intentionally kept per prior feedback to avoid allocations in hot paths.src/AdversarialRobustness/Alignment/RLHFAlignment.cs (1)
240-248: Reward computation is loop-invariant and KL coefficient unused.The
rewardis computed once perinputbut the loop applies the same adjustment to every element. Additionally,options.KLCoefficientis discarded without being used for KL penalty computation.Consider computing reward once outside the loop and adding a TODO to implement proper KL penalty:
return (input) => { var output = baseModel(input); - // Apply KL penalty to stay close to base model - _ = options.KLCoefficient; + // TODO: Implement KL penalty to stay close to base model using options.KLCoefficient - // Adjust output based on reward (simplified) + // Adjust output based on reward (simplified, loop-invariant) + var reward = rewardModelFunc(input, output); for (int i = 0; i < output.Length; i++) { - var reward = rewardModelFunc(input, output); var adjustment = NumOps.FromDouble(reward * 0.1); // Small adjustment output[i] = Clip01(NumOps.Add(output[i], adjustment)); } return output; };
🧹 Nitpick comments (25)
src/Models/RedTeamingResults.cs (1)
70-78: Consider privacy implications when persisting or logging vulnerability reports.The
ExamplePromptandProblematicResponsefields store actual adversarial inputs and model outputs for analysis. When serializing or loggingVulnerabilityReportinstances, be mindful that these fields may contain sensitive content—consider redaction or access controls in production environments.src/Models/CertifiedPrediction.cs (1)
20-20: Review the use ofdefault!for generic property initialization.The null-forgiving operator (
!) ondefaultsuppresses nullable warnings but doesn't prevent potential null reference issues ifTis a reference type. While the context suggestsTis intended for numeric types (likelyINumber<T>), the lack of explicit constraints makes this risky.Consider one of these approaches:
Option 1 (Preferred): Remove the
!operator ifTis constrained to value types:-public T CertifiedRadius { get; set; } = default!; +public T CertifiedRadius { get; set; }Option 2: Add explicit type constraints (if applicable):
public class CertifiedPrediction<T> where T : struct, INumber<T>Option 3: Make it nullable if null is a valid state:
-public T CertifiedRadius { get; set; } = default!; +public T? CertifiedRadius { get; set; }src/Models/SafetyValidationResult.cs (1)
48-69: Consider using an enum for Severity.The
Severityproperty currently uses a string with a default value of "Medium". This approach is less type-safe and could lead to inconsistent values.Consider defining a severity enum:
public enum ValidationSeverity { Low, Medium, High, Critical }Then update the property:
-public string Severity { get; set; } = "Medium"; +public ValidationSeverity Severity { get; set; } = ValidationSeverity.Medium;This provides compile-time type safety and prevents typos or inconsistent severity values.
src/Models/Options/AdversarialDefenseOptions.cs (2)
15-15: Unused generic type parameterT.The class declares a generic type parameter
Tbut never uses it. This adds unnecessary complexity and may confuse consumers of the API.Consider removing the generic parameter:
-public class AdversarialDefenseOptions<T> +public class AdversarialDefenseOptionsAlternatively, if
Tis intended for future use or consistency with other options classes, document the reason for keeping it.
26-46: Consider adding validation for numeric properties.Properties like
AdversarialRatio(should be 0-1),Epsilon(should be positive),TrainingEpochs(should be positive), andEnsembleSize(should be ≥1) have no validation. Invalid values could cause unexpected behavior at runtime.You could add validation in property setters or provide a
Validate()method:private double _adversarialRatio = 0.5; public double AdversarialRatio { get => _adversarialRatio; set => _adversarialRatio = value is >= 0 and <= 1 ? value : throw new ArgumentOutOfRangeException(nameof(value), "Must be between 0 and 1"); }src/AdversarialRobustness/Attacks/AutoAttack.cs (1)
127-156: Consider returning a failure indicator when all attacks fail.If all three attacks throw exceptions,
candidatesis empty and the method returns the originalinputunchanged (line 129 default). This silent fallback may mask issues from callers who expect an adversarial example.Consider either:
- Throwing an exception when no candidates are generated
- Returning a result type that indicates success/failure
- Logging a warning when falling back to the original input
+ if (candidates.Count == 0) + { + // All attacks failed - consider logging or throwing + throw new InvalidOperationException("All component attacks failed to generate adversarial examples."); + } + // Select the best adversarial examplesrc/AdversarialRobustness/Attacks/FGSMAttack.cs (1)
65-66: Hard-coded clip range assumes normalized image data.The clipping to
[0, 1]assumes inputs are normalized images. For other data types or unnormalized inputs, this range may be inappropriate.Consider making clip bounds configurable via
AdversarialAttackOptions<T>:adversarial[i] = Clip(adversarial[i], NumOps.FromDouble(Options.ClipMin), NumOps.FromDouble(Options.ClipMax));src/Models/Options/CertifiedDefenseOptions.cs (2)
16-16: Unused generic type parameterT.Similar to
AdversarialDefenseOptions<T>, this class declares a generic type parameter that is never used.Consider removing the unused generic parameter for consistency:
-public class CertifiedDefenseOptions<T> +public class CertifiedDefenseOptions
38-46:ConfidenceLevelshould be validated.The documentation states the value should be between 0 and 1, but there's no enforcement. Values outside this range (e.g., 1.5 or -0.1) would be invalid but accepted.
Add validation:
private double _confidenceLevel = 0.99; public double ConfidenceLevel { get => _confidenceLevel; set => _confidenceLevel = value is > 0 and < 1 ? value : throw new ArgumentOutOfRangeException(nameof(value), "Must be between 0 and 1 exclusive"); }src/Models/HarmfulContentResult.cs (1)
7-7: Unused generic type parameterT.The generic type parameter
Tis not used anywhere inHarmfulContentResult<T>. This is consistent with the unused generics in the options classes.Consider removing the unused parameter:
-public class HarmfulContentResult<T> +public class HarmfulContentResultNote: This would require updating
ISafetyFilter<T>.IdentifyHarmfulContentandSafetyFilter<T>.IdentifyHarmfulContentto returnHarmfulContentResultinstead.src/Models/Options/AdversarialAttackOptions.cs (2)
16-16: Unused generic type parameterT.The class declares
Tbut never uses it—all properties aredouble,int,string, orbool. This adds unnecessary complexity and can confuse consumers.Consider removing the generic parameter:
-public class AdversarialAttackOptions<T> +public class AdversarialAttackOptionsThis would require updating references in
AdversarialAttackBase<T>,CWAttack<T>, etc., to use the non-generic version.
61-61: Consider using an enum forNormTypeinstead of a magic string.Using a string for norm type is error-prone (typos like
"Linfinity"or"l-infinity"won't be caught at compile time).+public enum NormType +{ + LInfinity, + L2, + L1 +} + public class AdversarialAttackOptions<T> { // ... - public string NormType { get; set; } = "L-infinity"; + public NormType NormType { get; set; } = NormType.LInfinity;src/AdversarialRobustness/Attacks/CWAttack.cs (1)
52-53: Hard-coded hyperparameters should be configurable.
c(confidence) andlearningRateare fixed at1.0and0.01respectively. In the original C&W attack,cis typically found via binary search, and learning rate may need tuning per model.Consider adding these to
AdversarialAttackOptions<T>or usingOptions.StepSizefor the learning rate:-var c = 1.0; // Confidence parameter -var learningRate = 0.01; +var c = 1.0; // Consider making configurable or using binary search +var learningRate = Options.StepSize;src/Models/JailbreakDetectionResult.cs (1)
7-7: Unused generic type parameterT.Similar to
AdversarialAttackOptions<T>, this class declaresTbut none of its properties use it. Consider removing the generic parameter for simplicity.src/Models/SafetyFilterResult.cs (1)
65-68: ClarifyOriginalContentusage or rename.The XML doc says "what was removed or replaced," but in
SafetyFilter.cs(lines 195, 207), it's set to"Output blocked"or"Content sanitized"rather than actual content. Consider renaming toActionDescriptionor updating the doc to clarify it may contain action metadata.src/Models/Options/AlignmentMethodOptions.cs (2)
16-16: Unused generic type parameterT.Same issue as
AdversarialAttackOptions<T>: the class declaresTbut all properties are primitive types. Consider removing the generic parameter.
100-101: Consider an enum forRewardModelArchitecture.Using a string for architecture type has the same drawbacks as
NormType—no compile-time validation. If the set of supported architectures is known, an enum would improve type safety.src/Models/Options/SafetyFilterOptions.cs (1)
15-16: Unused generic type parameterT.The type parameter
Tis declared but never referenced by any member of this class. If it exists solely for API consistency withISafetyFilter<T>, consider documenting this in the remarks. Otherwise, removing it would simplify the API.src/AdversarialRobustness/Defenses/AdversarialTraining.cs (2)
29-29: Unused fieldattackMethod.The
attackMethodis initialized in the constructor but never used. IfApplyDefenseis intended as a runtime defense wrapper (not actual adversarial training), consider removing this field to avoid confusion. Alternatively, document the intended future use.
206-211:ApplyDenoisingis a stub.The method returns a clone of the input without any actual denoising. Consider adding a TODO comment or implementing a basic denoising algorithm (e.g., Gaussian smoothing) for consistency with the other preprocessing methods.
src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (1)
173-188:ProjectL2may divide by zero.If
normequals zero (e.g., zero perturbation vector), the checkLessThanOrEquals(norm, epsilon)passes and returns early. However, ifnormis exactly zero and somehow greater than epsilon (shouldn't happen, but defensive coding), division by zero could occur. The current logic is safe in practice, but consider adding an explicit zero check for robustness.protected T[] ProjectL2(T[] perturbation, T epsilon) { var norm = ComputeL2Norm(perturbation); - if (NumOps.LessThanOrEquals(norm, epsilon)) + if (NumOps.LessThanOrEquals(norm, epsilon) || NumOps.Equals(norm, NumOps.Zero)) { return perturbation; }src/AdversarialRobustness/Alignment/RLHFAlignment.cs (1)
290-294: PlaceholderIsHonestalways returnstrue.This method currently provides no real honesty evaluation, which may give misleading metrics.
Consider adding a TODO or logging when placeholder evaluations are used:
private bool IsHonest(T[] output, T[] input) { - // Simplified honesty check - return true; // Placeholder + // TODO: Implement proper honesty evaluation + // Currently returns true as placeholder - metrics may be misleading + return true; }src/AdversarialRobustness/Safety/SafetyFilter.cs (1)
252-265: Regex patterns are compiled on each match, which is inefficient.The jailbreak patterns are matched using
Regex.IsMatchin a loop without caching compiled regexes. For high-throughput scenarios, this is costly.Consider pre-compiling the regexes in the constructor:
private readonly List<string> jailbreakPatterns; + private readonly List<Regex> compiledJailbreakPatterns; private readonly Dictionary<string, List<string>> harmfulContentPatterns; public SafetyFilter(SafetyFilterOptions<T> options) { this.options = options; // Initialize jailbreak detection patterns jailbreakPatterns = new List<string> { /* ... */ }; + compiledJailbreakPatterns = jailbreakPatterns + .Select(p => new Regex(p, RegexOptions.IgnoreCase | RegexOptions.Compiled)) + .ToList();Then use
compiledJailbreakPatterns[i].IsMatch(text)in the detection loop.src/AdversarialRobustness/Documentation/ModelCard.cs (1)
264-268: Consider documenting potential exceptions.
SaveToFilecan throwIOException,UnauthorizedAccessException, etc. While propagating exceptions is acceptable, adding XML documentation would help callers./// <summary> /// Saves the Model Card to a markdown file. /// </summary> /// <param name="filePath">The path where the Model Card should be saved.</param> + /// <exception cref="IOException">Thrown when the file cannot be written.</exception> + /// <exception cref="UnauthorizedAccessException">Thrown when access to the path is denied.</exception> public void SaveToFile(string filePath)src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (1)
157-162: Median calculation is simplified.For even-length lists, the median should be the average of the two middle elements. The current implementation returns
certifiedRadii[Count / 2], which is the upper-middle element.For more accurate statistics:
if (certifiedRadii.Count > 0) { metrics.AverageCertifiedRadius = NumOps.FromDouble(certifiedRadii.Average()); certifiedRadii.Sort(); - metrics.MedianCertifiedRadius = NumOps.FromDouble(certifiedRadii[certifiedRadii.Count / 2]); + int mid = certifiedRadii.Count / 2; + var median = certifiedRadii.Count % 2 == 0 + ? (certifiedRadii[mid - 1] + certifiedRadii[mid]) / 2.0 + : certifiedRadii[mid]; + metrics.MedianCertifiedRadius = NumOps.FromDouble(median); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
src/AdversarialRobustness/Alignment/RLHFAlignment.cs(1 hunks)src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs(1 hunks)src/AdversarialRobustness/Attacks/AutoAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/CWAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/FGSMAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/PGDAttack.cs(1 hunks)src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs(1 hunks)src/AdversarialRobustness/Defenses/AdversarialTraining.cs(1 hunks)src/AdversarialRobustness/Documentation/ModelCard.cs(1 hunks)src/AdversarialRobustness/README.md(1 hunks)src/AdversarialRobustness/Safety/SafetyFilter.cs(1 hunks)src/Interfaces/IAdversarialAttack.cs(1 hunks)src/Interfaces/IAdversarialDefense.cs(1 hunks)src/Interfaces/IAlignmentMethod.cs(1 hunks)src/Interfaces/ICertifiedDefense.cs(1 hunks)src/Interfaces/ISafetyFilter.cs(1 hunks)src/Models/AlignmentEvaluationData.cs(1 hunks)src/Models/AlignmentFeedbackData.cs(1 hunks)src/Models/AlignmentMetrics.cs(1 hunks)src/Models/CertifiedAccuracyMetrics.cs(1 hunks)src/Models/CertifiedPrediction.cs(1 hunks)src/Models/HarmfulContentResult.cs(1 hunks)src/Models/JailbreakDetectionResult.cs(1 hunks)src/Models/Options/AdversarialAttackOptions.cs(1 hunks)src/Models/Options/AdversarialDefenseOptions.cs(1 hunks)src/Models/Options/AlignmentMethodOptions.cs(1 hunks)src/Models/Options/CertifiedDefenseOptions.cs(1 hunks)src/Models/Options/SafetyFilterOptions.cs(1 hunks)src/Models/RedTeamingResults.cs(1 hunks)src/Models/RobustnessMetrics.cs(1 hunks)src/Models/SafetyFilterResult.cs(1 hunks)src/Models/SafetyValidationResult.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (19)
src/AdversarialRobustness/Attacks/FGSMAttack.cs (3)
src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (8)
T(38-38)T(41-49)T(52-65)T(109-114)T(119-124)AdversarialAttackBase(13-189)AdversarialAttackBase(31-35)AdversarialAttackOptions(68-71)src/Interfaces/IAdversarialAttack.cs (1)
AdversarialAttackOptions(88-88)src/Models/Options/AdversarialAttackOptions.cs (1)
AdversarialAttackOptions(16-99)
src/Models/CertifiedAccuracyMetrics.cs (2)
src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (1)
CertifiedAccuracyMetrics(117-165)src/Interfaces/ICertifiedDefense.cs (1)
CertifiedAccuracyMetrics(91-95)
src/AdversarialRobustness/Attacks/AutoAttack.cs (2)
src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (9)
T(38-38)T(41-49)T(52-65)T(109-114)T(119-124)T(129-141)AdversarialAttackBase(13-189)AdversarialAttackBase(31-35)AdversarialAttackOptions(68-71)src/Models/Options/AdversarialAttackOptions.cs (1)
AdversarialAttackOptions(16-99)
src/Models/HarmfulContentResult.cs (2)
src/AdversarialRobustness/Safety/SafetyFilter.cs (3)
HarmfulContentResult(280-352)T(355-366)T(407-412)src/Interfaces/ISafetyFilter.cs (1)
HarmfulContentResult(101-101)
src/AdversarialRobustness/Attacks/PGDAttack.cs (2)
src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (8)
T(38-38)T(41-49)T(52-65)T(109-114)T(119-124)AdversarialAttackBase(13-189)AdversarialAttackBase(31-35)AdversarialAttackOptions(68-71)src/Models/Options/AdversarialAttackOptions.cs (1)
AdversarialAttackOptions(16-99)
src/Models/Options/SafetyFilterOptions.cs (2)
src/AdversarialRobustness/Safety/SafetyFilter.cs (3)
SafetyFilterOptions(369-369)T(355-366)T(407-412)src/Interfaces/ISafetyFilter.cs (1)
SafetyFilterOptions(123-123)
src/AdversarialRobustness/Alignment/RLHFAlignment.cs (7)
src/AdversarialRobustness/Attacks/AutoAttack.cs (1)
T(72-157)src/AdversarialRobustness/Defenses/AdversarialTraining.cs (6)
T(65-80)T(174-188)T(190-204)T(206-211)T(231-240)Func(53-62)src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (7)
T(38-38)T(41-49)T(52-65)T(109-114)T(119-124)T(129-141)T(146-155)src/Interfaces/IAlignmentMethod.cs (4)
AlignmentMethodOptions(107-107)Func(48-48)Func(78-78)AlignmentMetrics(60-60)src/Models/AlignmentFeedbackData.cs (1)
AlignmentFeedbackData(7-41)src/Models/AlignmentMetrics.cs (1)
AlignmentMetrics(7-61)src/Models/AlignmentEvaluationData.cs (1)
AlignmentEvaluationData(7-33)
src/Models/JailbreakDetectionResult.cs (2)
src/AdversarialRobustness/Safety/SafetyFilter.cs (3)
JailbreakDetectionResult(232-277)T(355-366)T(407-412)src/Interfaces/ISafetyFilter.cs (1)
JailbreakDetectionResult(82-82)
src/Models/AlignmentMetrics.cs (2)
src/AdversarialRobustness/Alignment/RLHFAlignment.cs (3)
AlignmentMetrics(56-104)T(262-267)T(355-360)src/Interfaces/IAlignmentMethod.cs (1)
AlignmentMetrics(60-60)
src/AdversarialRobustness/Documentation/ModelCard.cs (3)
src/AdversarialRobustness/Defenses/AdversarialTraining.cs (1)
RobustnessMetrics(83-144)src/Interfaces/IAdversarialDefense.cs (1)
RobustnessMetrics(74-78)src/Models/RobustnessMetrics.cs (1)
RobustnessMetrics(7-38)
src/Models/SafetyFilterResult.cs (2)
src/AdversarialRobustness/Safety/SafetyFilter.cs (3)
SafetyFilterResult(165-229)T(355-366)T(407-412)src/Interfaces/ISafetyFilter.cs (1)
SafetyFilterResult(65-65)
src/Models/RedTeamingResults.cs (2)
src/AdversarialRobustness/Alignment/RLHFAlignment.cs (3)
RedTeamingResults(127-180)T(262-267)T(355-360)src/Interfaces/IAlignmentMethod.cs (1)
RedTeamingResults(97-97)
src/AdversarialRobustness/Safety/SafetyFilter.cs (5)
src/Models/Options/SafetyFilterOptions.cs (1)
SafetyFilterOptions(16-113)src/Models/SafetyValidationResult.cs (2)
SafetyValidationResult(7-43)ValidationIssue(48-69)src/Models/SafetyFilterResult.cs (2)
SafetyFilterResult(7-43)FilterAction(48-74)src/Models/JailbreakDetectionResult.cs (2)
JailbreakDetectionResult(7-43)JailbreakIndicator(48-69)src/Models/HarmfulContentResult.cs (2)
HarmfulContentResult(7-48)HarmfulContentFinding(53-79)
src/Models/SafetyValidationResult.cs (2)
src/AdversarialRobustness/Safety/SafetyFilter.cs (3)
SafetyValidationResult(73-162)T(355-366)T(407-412)src/Interfaces/ISafetyFilter.cs (1)
SafetyValidationResult(48-48)
src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (4)
src/Interfaces/ICertifiedDefense.cs (5)
CertifiedDefenseOptions(105-105)CertifiedPrediction(48-48)CertifiedPrediction(60-60)CertifiedAccuracyMetrics(91-95)Reset(110-110)src/Models/Options/CertifiedDefenseOptions.cs (1)
CertifiedDefenseOptions(16-89)src/Models/CertifiedPrediction.cs (1)
CertifiedPrediction(7-46)src/Models/CertifiedAccuracyMetrics.cs (1)
CertifiedAccuracyMetrics(7-43)
src/Models/Options/AdversarialAttackOptions.cs (4)
src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (6)
AdversarialAttackOptions(68-71)T(38-38)T(41-49)T(52-65)T(109-114)T(119-124)src/Interfaces/IAdversarialAttack.cs (1)
AdversarialAttackOptions(88-88)src/AdversarialRobustness/Attacks/CWAttack.cs (1)
T(50-98)src/AdversarialRobustness/Defenses/AdversarialTraining.cs (5)
T(65-80)T(174-188)T(190-204)T(206-211)T(231-240)
src/AdversarialRobustness/Attacks/CWAttack.cs (2)
src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (9)
T(38-38)T(41-49)T(52-65)T(109-114)T(119-124)T(129-141)AdversarialAttackBase(13-189)AdversarialAttackBase(31-35)AdversarialAttackOptions(68-71)src/Models/Options/AdversarialAttackOptions.cs (1)
AdversarialAttackOptions(16-99)
src/Interfaces/IAlignmentMethod.cs (2)
src/Models/AlignmentFeedbackData.cs (1)
AlignmentFeedbackData(7-41)src/Models/AlignmentEvaluationData.cs (1)
AlignmentEvaluationData(7-33)
src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (2)
src/Interfaces/IAdversarialAttack.cs (2)
AdversarialAttackOptions(88-88)Reset(97-97)src/Models/Options/AdversarialAttackOptions.cs (1)
AdversarialAttackOptions(16-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: SonarCloud Analysis
- GitHub Check: CodeQL Analysis
- GitHub Check: Codacy Security Scan
🔇 Additional comments (35)
src/Models/CertifiedAccuracyMetrics.cs (1)
1-43: LGTM! Well-structured metrics container.The class provides a clean data container for certified accuracy evaluation results with appropriate XML documentation. The property defaults using
default!for generic typeTandnew()for the dictionary are idiomatic for this pattern.One minor consideration: since
Trepresents numeric types and properties likeCertificationRadiusare assigned conditionally in the consuming code (as seen inRandomizedSmoothing.EvaluateCertifiedAccuracy), thedefault!initialization is appropriate here.src/Interfaces/IAdversarialDefense.cs (1)
1-97: LGTM! Well-designed interface for adversarial defenses.The interface provides a clear contract for adversarial defense implementations with:
ApplyDefensereturning a function allowing defense compositionPreprocessInputfor input-level transformationsEvaluateRobustnessfor measuring defense effectiveness- Consistent pattern with
GetOptions()andReset()mirroring the attack interfaceThe extensive documentation with beginner-friendly explanations adds value for library users.
src/Interfaces/IAdversarialAttack.cs (1)
1-98: LGTM! Clean interface design for adversarial attacks.The interface establishes a well-structured contract for adversarial attack implementations:
- Single-sample and batch generation methods for flexibility
- Perturbation calculation for attack analysis
- Consistent
GetOptions()/Reset()pattern matchingIAdversarialDefense<T>The XML documentation correctly escapes the ampersand in "C&W" (Carlini & Wagner) and provides helpful context for library users.
src/Interfaces/IAlignmentMethod.cs (1)
1-113: LGTM! Comprehensive interface for AI alignment methods.The interface covers the key AI alignment techniques:
AlignModelfor RLHF-style training with human feedbackApplyConstitutionalPrinciplesfor rule-based behavioral guidancePerformRedTeamingfor systematic vulnerability testingEvaluateAlignmentfor measuring alignment metricsThe documentation effectively explains complex AI safety concepts (RLHF, Constitutional AI, red teaming) in an accessible manner for library users.
src/Models/RedTeamingResults.cs (1)
1-84: LGTM! Well-structured data models for red teaming results.The class design effectively captures:
- Per-prompt attack results (parallel arrays for prompts, responses, success flags, severity, vulnerability types)
- Aggregate metrics (SuccessRate, AverageSeverity)
- Detailed vulnerability reports with actionable recommendations
The separation of
VulnerabilityReportas a non-generic class is appropriate since it stores string representations of prompts/responses.src/AdversarialRobustness/README.md (1)
1-351: Excellent comprehensive documentation!The README provides thorough coverage of all module features with clear examples, academic references, best practices, and beginner-friendly explanations. The structure is logical and the code examples demonstrate proper API usage patterns.
src/Models/RobustnessMetrics.cs (1)
1-38: LGTM - Clean data model implementation.The class provides a clear structure for robustness metrics with appropriate XML documentation. The
AdditionalMetricsdictionary allows for extensibility. The double properties will default to 0.0, which is appropriate for metric values.src/Models/AlignmentMetrics.cs (1)
1-61: Excellent documentation and structure.The class is well-designed with comprehensive XML documentation including helpful remarks that explain what each metric measures. The pattern is consistent with other metrics classes in the module.
src/Models/CertifiedPrediction.cs (1)
1-46: Well-structured data model for certified predictions.The class provides comprehensive information about certified predictions with appropriate documentation. The structure effectively captures both the prediction result and the certification guarantees.
src/Models/AlignmentEvaluationData.cs (1)
1-33: Excellent defensive initialization pattern.The class properly initializes all array properties with
Array.Empty<>(), which prevents null reference exceptions and is more efficient than allocating empty arrays. The XML documentation clearly describes each property's purpose.src/Interfaces/ISafetyFilter.cs (1)
1-129: Outstanding interface design and documentation!The interface provides a clear and comprehensive contract for safety filtering with exceptional documentation. The beginner-friendly explanations and real-world examples make this accessible while maintaining professional standards. The method signatures are well-designed and consistent.
src/Models/SafetyValidationResult.cs (1)
7-43: Well-designed validation result model.The class properly initializes all collections and correctly uses nullable
T[]?forSanitizedInputsince sanitization may not always be applicable. The structure effectively captures validation outcomes with appropriate extensibility throughValidationDetails.src/Models/AlignmentFeedbackData.cs (1)
1-41: Excellent use of named tuples and defensive initialization.The class effectively uses named tuples for the
Preferencesproperty, making the code self-documenting. All arrays are properly initialized withArray.Empty<>(), and the XML documentation clearly explains the purpose of each property, particularly the Preferences structure.src/AdversarialRobustness/Attacks/AutoAttack.cs (1)
162-180: LGTM!The
IsSuccessfulAttackmethod correctly uses a ternary expression and properly determines attack success based on targeted vs. untargeted mode.src/AdversarialRobustness/Attacks/FGSMAttack.cs (2)
49-70: LGTM!The FGSM perturbation logic correctly applies the sign of the gradient scaled by epsilon, handles targeted vs. untargeted modes appropriately, and clips results to valid bounds.
133-164: LGTM!The
Softmaximplementation correctly handles numerical stability by subtracting the max logit before exponentiation. The defensive check forsum <= 0is harmless even though it's unreachable in practice.src/Models/HarmfulContentResult.cs (2)
44-47:Detailsdictionary withobjectvalues may have serialization concerns.Using
Dictionary<string, object>provides flexibility but can cause issues with JSON serialization when values contain complex types. Ensure that values stored here are serializable primitives or provide custom serialization handling.
53-79: LGTM!
HarmfulContentFindingis a well-structured data class with appropriate defaults for string properties. The design cleanly encapsulates individual findings.src/AdversarialRobustness/Attacks/CWAttack.cs (2)
185-204: Consider usingMath.Clampif targeting .NET Standard 2.1+.The custom
ClampandClampInthelpers could be replaced withMath.Clampfor modern frameworks. If .NET Framework 4.7.1 compatibility is required (as suggested by theAtanhcomment), keeping these is appropriate.
174-183: The targeted attack loss formula is correct.The canonical C&W loss for targeted attacks is max{ Z(x')_i : i≠t } — Z(x')_t, which represents the difference between the highest non-target logit and the target logit. When the model correctly classifies as the target, this value becomes negative. The current implementation using
maxOtherLogit - targetLogitmatches this standard formulation and will work correctly during optimization to push the target class above other classes.src/Models/JailbreakDetectionResult.cs (1)
1-69: LGTM on overall structure.The data model is well-documented with clear property purposes. The use of
List<JailbreakIndicator>for extensible indicator tracking andDictionary<string, object>for flexible detection details are appropriate patterns for this domain.src/Models/SafetyFilterResult.cs (1)
1-74: Well-structured result model.The generic parameter
Tis correctly used forFilteredOutput. The separation ofFilterActionfor individual actions and the extensibleFilteringDetailsdictionary provide good flexibility.src/Models/Options/AlignmentMethodOptions.cs (1)
26-96: LGTM on configuration properties.The defaults are sensible, and the documentation is thorough with beginner-friendly explanations. The properties cover the key aspects of RLHF, Constitutional AI, and red teaming configuration.
src/Interfaces/ICertifiedDefense.cs (1)
1-111: Well-designed interface with comprehensive documentation.The interface defines a clear contract for certified defense mechanisms. The method signatures are appropriate for the domain, and the documentation is thorough with beginner-friendly explanations that will help users understand the concepts.
src/Models/Options/SafetyFilterOptions.cs (1)
18-113: Configuration properties look good.The default values are sensible for a safety filtering system. The documentation clearly explains each property's purpose and impact.
src/AdversarialRobustness/Attacks/PGDAttack.cs (1)
56-90: PGD attack implementation looks correct.The iterative attack loop correctly implements the PGD algorithm: gradient computation, signed gradient step with optional targeting, projection to epsilon-ball, and clipping. The ternary operators for initialization and projection (as suggested in past reviews) are already in place.
src/AdversarialRobustness/Defenses/AdversarialTraining.cs (2)
53-62:ApplyDefensedoesn't perform adversarial training.The method name and class documentation suggest this should augment training data with adversarial examples, but it currently returns a preprocessing wrapper. Consider either:
- Renaming to clarify the runtime defense behavior, or
- Documenting that actual training-time augmentation requires external trainer integration (as the comment suggests)
125-134: Specific exception handling addresses prior feedback.The catch blocks now use specific exception types (
ArgumentException,InvalidOperationException) instead of a generic catch clause, addressing the prior review feedback.src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (1)
13-35: Base class design is solid.The abstract base class provides a clean foundation with shared utilities (clipping, norm computation, projection) and consistent serialization patterns. The protected
NumOpsfield enables generic numeric operations across derived attack implementations.src/AdversarialRobustness/Alignment/RLHFAlignment.cs (1)
276-288: LGTM!The single-pass iteration for finding both max and min values is efficient and addresses the prior review feedback.
src/AdversarialRobustness/Safety/SafetyFilter.cs (2)
421-433: LGTM!The logging implementation addresses prior feedback: the log path is now configurable via
options.LogFilePath, and exception handling uses specific types (IOException,UnauthorizedAccessException) instead of a generic catch.
335-348: LGTM!The manual iteration to find the primary harm category is O(n) and addresses the prior feedback about inefficient LINQ usage.
src/AdversarialRobustness/Documentation/ModelCard.cs (2)
23-26: LGTM!Setting
Datein the constructor rather than as a property initializer addresses the prior feedback about unexpected instantiation-time behavior.
273-299: LGTM!The factory method provides a convenient way to create model cards from evaluation results with sensible defaults.
src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (1)
66-75: LGTM!The manual iteration to find the most frequently predicted class is O(n) and addresses prior feedback about inefficient LINQ usage.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
src/Models/Results/PredictionModelResult.cs (1)
994-1004: Same type-specific filtering issue applies to outputs.Similar concerns as input validation:
- Output filtering only applies when
TOutputisVector<T>(line 996)- Unsafe casting at line 1001:
(TOutput)(object)filtered.FilteredOutput- No logging or telemetry when outputs are modified - users won't know their predictions were sanitized
This is a duplicate of the input validation architectural issue. The safety filtering should either:
- Work generically with all TInput/TOutput types
- Be clearly documented as Vector-only
- Include type constraints when enabled
- Emit warnings/logs when filtering modifies outputs
src/AdversarialRobustness/Attacks/FGSMAttack.cs (1)
94-123: Gradient computation inconsistency for targeted attacks.The original loss (line 101) always uses
trueLabel, while the perturbed loss (line 116) usesOptions.TargetClassfor targeted attacks. This asymmetry produces incorrect gradients for targeted attacks because you're computing the gradient of different loss functions.src/AdversarialRobustness/Attacks/AutoAttack.cs (1)
20-29: Documentation does not match implementation.The documentation describes AutoAttack as including APGD-CE, APGD-DLR, FAB, and Square Attack, but the implementation uses PGD, CW, and FGSM. Update the documentation to reflect the actual implementation.
src/AdversarialRobustness/Attacks/PGDAttack.cs (2)
118-133: Random starting point may violate L2 norm constraint.When
NormTypeis "L2", the random initialization applies per-component clipping suitable for L-infinity but can exceed the L2 budget for high-dimensional inputs. Consider projecting the random perturbation to the L2 ball when using L2 norm.
226-235: Softmax edge case: unnormalized values when sum ≤ 0.If
sum <= 0(line 226), the method returns unnormalized exponentiated values. Consider returning uniform probabilities or throwing an exception for this edge case.src/AdversarialRobustness/Attacks/CWAttack.cs (1)
136-155: Finite-difference gradient is computationally expensive.The gradient computation requires O(Iterations × input.Length) model calls, which becomes prohibitively slow for high-dimensional inputs. This is a known limitation of finite-difference approximation in the absence of automatic differentiation.
src/AdversarialRobustness/Safety/SafetyFilter.cs (1)
219-230: SanitizeOutput is still a no-op placeholder.The
FilterActionat line 227 claims "Moderate harmful content detected and sanitized", butSanitizeOutput(lines 440-446) only clones the vector without modification. This misleads consumers about the actual filtering behavior.Consider either:
- Implementing actual sanitization logic in
SanitizeOutput, or- Updating the action message to reflect that content is flagged but not automatically modified:
result.Actions.Add(new FilterAction { - ActionType = "Sanitize", - Reason = "Moderate harmful content detected and sanitized", + ActionType = "Flag", + Reason = "Moderate harmful content detected (manual review recommended)", Location = 0, - OriginalContent = "Content sanitized" + OriginalContent = "Content flagged but not modified" });src/AdversarialRobustness/Alignment/RLHFAlignment.cs (1)
110-115: Potential division by zero if test inputs are empty.If
evaluationData.TestInputs.Rowsis 0, the divisions at lines 111-114 will produce NaN values. Consider adding a guard to return default metrics or throw an exception for empty test data.int total = evaluationData.TestInputs.Rows; + if (total == 0) + { + return metrics; // or throw ArgumentException + } + metrics.HelpfulnessScore = (double)helpfulCount / total; metrics.HarmlessnessScore = (double)harmlessCount / total; metrics.HonestyScore = (double)honestCount / total; metrics.PreferenceMatchRate = totalPreferenceMatch / total;
🧹 Nitpick comments (12)
src/Interfaces/ICertifiedDefense.cs (1)
29-29: Consider adding a numeric type constraint to T.The generic parameter
Tis used for numeric operations (radius calculations, noise sigma, etc.), but lacks a constraint. Without a constraint likewhere T : struct, INumber<T>(or the project's equivalent numeric constraint), users could instantiate with non-numeric types and encounter unclear runtime errors.Apply this diff to add the constraint:
-public interface ICertifiedDefense<T> : IModelSerializer +public interface ICertifiedDefense<T> : IModelSerializer + where T : struct, INumber<T>If
INumber<T>is not available in your target framework, use the project's numeric constraint convention (e.g., check how other generic numeric interfaces in the codebase constrainT).src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (1)
111-125: Consider parallelizing batch certification for better performance.The current implementation processes inputs sequentially. For large batches, parallel processing could significantly improve throughput.
Apply this diff to add parallel processing:
public CertifiedPrediction<T>[] CertifyBatch(Matrix<T> inputs, IPredictiveModel<T, Vector<T>, Vector<T>> model) { if (inputs == null) { throw new ArgumentNullException(nameof(inputs)); } var results = new CertifiedPrediction<T>[inputs.Rows]; - for (int i = 0; i < inputs.Rows; i++) - { - results[i] = CertifyPrediction(inputs.GetRow(i), model); - } + Parallel.For(0, inputs.Rows, i => + { + results[i] = CertifyPrediction(inputs.GetRow(i), model); + }); return results; }Note: Ensure thread safety if the
Randominstance or model state is shared across threads. Consider usingThreadLocal<Random>if parallelizing.src/Models/SafetyValidationResult.cs (1)
50-71: Consider using an enum forSeverityto improve type safety.Using a string for
Severityallows arbitrary values. An enum would provide compile-time validation and better IntelliSense support.+public enum ValidationSeverity +{ + Low, + Medium, + High, + Critical +} + public class ValidationIssue { /// <summary> /// Gets or sets the severity level of the issue. /// </summary> - public string Severity { get; set; } = "Medium"; + public ValidationSeverity Severity { get; set; } = ValidationSeverity.Medium;src/AiDotNet.Tensors/Helpers/MathHelper.cs (1)
227-248: Silent clamping may mask issues for boundary inputs.The clamping at lines 243-244 prevents infinity/NaN but silently returns finite values when the mathematically correct result would be ±∞ (at x = ±1). This could hide bugs where callers inadvertently pass boundary values.
Consider documenting this behavior explicitly or throwing for out-of-range inputs to match
Math.Atanhsemantics on .NET Core (which returns ±∞ at boundaries).public static double Atanh(double x) { - // Clamp to open interval (-1, 1) to avoid infinities from the identity near boundaries. - const double eps = 1e-12; - x = Clamp(x, -1.0 + eps, 1.0 - eps); + // Return infinity at boundaries to match Math.Atanh behavior + if (x <= -1.0) return double.NegativeInfinity; + if (x >= 1.0) return double.PositiveInfinity; // atanh(x) = 0.5 * ln((1+x)/(1-x)) return 0.5 * Math.Log((1.0 + x) / (1.0 - x)); }Alternatively, if the clamping is intentional for numerical stability in the C&W attack context, add a comment explaining the design rationale.
src/Models/Options/PredictionModelResultOptions.cs (1)
534-548: Clarify or reconsider the null-means-enabled semantics.The documentation states that when
SafetyFilterConfigurationis null, "safety filtering defaults to enabled." This is inverse to the typical pattern where null means "not configured" or "feature disabled."This could cause confusion:
- Other nullable properties in this class (e.g.,
BiasDetector,FairnessEvaluator) follow the pattern where null = not configured/disabled- Users expecting consistency might be surprised that null enables safety filtering
Consider either:
- Changing to null = disabled (consistent with other properties), requiring explicit opt-in
- Using a non-nullable property with a default-constructed instance that has
Enabled = trueIf the intent is opt-out safety filtering, making this explicit would be clearer:
- public SafetyFilterConfiguration<T>? SafetyFilterConfiguration { get; set; } + public SafetyFilterConfiguration<T> SafetyFilterConfiguration { get; set; } = new() { Enabled = true };This makes the default behavior visible and follows the principle of least surprise.
src/Models/AlignmentEvaluationData.cs (1)
9-35: Consider adding validation for dimensional consistency.The class is a clean data container, but properties can become inconsistent:
TestInputs.RowCountmight not matchLabels.LengthorReferenceScores.LengthTestInputs.RowCountmight not matchExpectedOutputs.RowCountEvaluationCriteria.Lengthmight not match the number of test casesWhile acceptable as a simple DTO, consider adding:
- A validation method:
public bool Validate(out List<string> errors)- Or factory methods that enforce consistency:
public static AlignmentEvaluationData<T> Create(...)- Or property setters that validate dimensions
This would prevent downstream errors when consuming code assumes dimensional consistency.
Example validation method:
public bool Validate(out List<string> errors) { errors = new List<string>(); int rowCount = TestInputs.RowCount; if (ExpectedOutputs.RowCount != rowCount) errors.Add($"ExpectedOutputs row count ({ExpectedOutputs.RowCount}) doesn't match TestInputs ({rowCount})"); if (Labels.Length > 0 && Labels.Length != rowCount) errors.Add($"Labels length ({Labels.Length}) doesn't match TestInputs row count ({rowCount})"); if (ReferenceScores.Length > 0 && ReferenceScores.Length != rowCount) errors.Add($"ReferenceScores length ({ReferenceScores.Length}) doesn't match TestInputs row count ({rowCount})"); return errors.Count == 0; }src/Models/AlignmentFeedbackData.cs (1)
9-43: Consider validation for dimensional consistency across properties.Similar to AlignmentEvaluationData, this class could benefit from validation ensuring:
Ratings.LengthmatchesOutputs.RowCount(when non-empty)TextualFeedback.LengthmatchesOutputs.RowCount(when non-empty)Rewards.LengthmatchesOutputs.RowCount(when non-empty)- Relationship between
Inputs.RowCountandOutputs.RowCountis documentedAdding a validation method would prevent downstream confusion and errors.
tests/AiDotNet.Tests/AdversarialRobustness/SafetyFilterTests.cs (2)
10-25: Test is correct, consider simplifying options.The test correctly validates NaN detection. Minor suggestion: line 16 sets
MaxInputLength = 10, which isn't relevant to testing NaN validation. Removing it would make the test clearer about what's being tested.var options = new SafetyFilterOptions<double> { EnableInputValidation = true, - MaxInputLength = 10 };
44-61: Consider expanding safety score test coverage.The test correctly validates score bounds. However, it only checks that the score is between 0.0 and 1.0. Consider adding tests for:
- What inputs produce low safety scores (e.g., with NaN, infinity, out-of-range values)
- What inputs produce high safety scores (normal values)
- How the score changes with different validation settings
This would better document the expected behavior of
ComputeSafetyScore.Example additional tests:
[Fact] public void ComputeSafetyScore_WithNaN_ReturnsLowScore() { var options = new SafetyFilterOptions<double> { EnableInputValidation = true }; var filter = new SafetyFilter<double>(options); var input = new Vector<double>(new[] { 0.0, double.NaN, 1.0 }); var score = filter.ComputeSafetyScore(input); Assert.True(score < 0.5); // Expect low score for invalid input } [Fact] public void ComputeSafetyScore_WithValidInput_ReturnsHighScore() { var options = new SafetyFilterOptions<double> { EnableInputValidation = true }; var filter = new SafetyFilter<double>(options); var input = new Vector<double>(new[] { 0.0, 0.5, 1.0 }); var score = filter.ComputeSafetyScore(input); Assert.True(score > 0.8); // Expect high score for valid input }src/AdversarialRobustness/Attacks/FGSMAttack.cs (1)
107-111: Consider extracting a CloneInput helper for consistency.The manual vector cloning here is correct but verbose. For consistency with
PGDAttackandCWAttack, consider extracting a sharedCloneInputhelper method.Apply this diff to add the helper:
+ private static Vector<T> CloneInput(Vector<T> input) + { + var clone = new Vector<T>(input.Length); + for (int i = 0; i < input.Length; i++) + { + clone[i] = input[i]; + } + return clone; + } private Vector<T> ComputeGradient(Vector<T> input, int trueLabel, IPredictiveModel<T, Vector<T>, Vector<T>> targetModel) { var gradient = new Vector<T>(input.Length); var delta = NumOps.FromDouble(0.001); var originalOutput = targetModel.Predict(input); var originalLoss = ComputeLoss(originalOutput, trueLabel); for (int i = 0; i < input.Length; i++) { - var perturbedInput = new Vector<T>(input.Length); - for (int j = 0; j < input.Length; j++) - { - perturbedInput[j] = input[j]; - } + var perturbedInput = CloneInput(input); perturbedInput[i] = NumOps.Add(perturbedInput[i], delta);src/AdversarialRobustness/Attacks/AutoAttack.cs (1)
89-137: Consider refactoring duplicate exception handling.The three attack invocations (lines 89-137) use identical exception handling patterns. For maintainability, consider extracting a helper method to reduce duplication.
Example refactor:
private (Vector<T>? adversarial, double perturbation, bool success) TryAttack( Func<Vector<T>> attackFunc, Vector<T> input, IPredictiveModel<T, Vector<T>, Vector<T>> targetModel, int trueLabel) { try { var adversarial = attackFunc(); var perturbation = NumOps.ToDouble(ComputeL2Norm(CalculatePerturbation(input, adversarial))); var success = IsSuccessfulAttack(targetModel.Predict(adversarial), trueLabel); return (adversarial, perturbation, success); } catch (ArgumentException) { return (null, double.PositiveInfinity, false); } catch (InvalidOperationException) { return (null, double.PositiveInfinity, false); } } // Usage: var pgdResult = TryAttack(() => pgdAttack.GenerateAdversarialExample(input, trueLabel, targetModel), input, targetModel, trueLabel); if (pgdResult.adversarial != null) candidates.Add((pgdResult.adversarial, pgdResult.perturbation, pgdResult.success));src/AdversarialRobustness/Defenses/AdversarialTraining.cs (1)
220-263: Preprocessing methods implemented with reasonable approximations.The JPEG compression and bit-depth reduction methods provide reasonable approximations of these defense techniques. The denoising method is currently a placeholder (just clones input) but is clearly marked as simplified. For production use, consider implementing actual denoising (e.g., Gaussian blur, median filter) if this method is enabled.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
src/AdversarialRobustness/Alignment/RLHFAlignment.cs(1 hunks)src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs(1 hunks)src/AdversarialRobustness/Attacks/AutoAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/CWAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/FGSMAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/PGDAttack.cs(1 hunks)src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs(1 hunks)src/AdversarialRobustness/Defenses/AdversarialTraining.cs(1 hunks)src/AdversarialRobustness/README.md(1 hunks)src/AdversarialRobustness/Safety/SafetyFilter.cs(1 hunks)src/AiDotNet.Tensors/Helpers/MathHelper.cs(1 hunks)src/Diagnostics/ProfileReport.cs(2 hunks)src/Interfaces/IAdversarialAttack.cs(1 hunks)src/Interfaces/IAdversarialDefense.cs(1 hunks)src/Interfaces/IAlignmentMethod.cs(1 hunks)src/Interfaces/ICertifiedDefense.cs(1 hunks)src/Interfaces/IFunctionTool.cs(5 hunks)src/Interfaces/IPredictionModelBuilder.cs(2 hunks)src/Interfaces/ISafetyFilter.cs(1 hunks)src/Models/AlignmentEvaluationData.cs(1 hunks)src/Models/AlignmentFeedbackData.cs(1 hunks)src/Models/Options/PredictionModelResultOptions.cs(1 hunks)src/Models/Options/SafetyFilterConfiguration.cs(1 hunks)src/Models/RedTeamingResults.cs(1 hunks)src/Models/Results/PredictionModelResult.cs(5 hunks)src/Models/SafetyFilterResult.cs(1 hunks)src/Models/SafetyValidationResult.cs(1 hunks)src/PredictionModelBuilder.cs(4 hunks)src/PromptEngineering/Tools/FunctionToolBase.cs(6 hunks)src/PromptEngineering/Tools/ToolRegistry.cs(4 hunks)tests/AiDotNet.Tests/AdversarialRobustness/SafetyFilterTests.cs(1 hunks)tests/AiDotNet.Tests/PromptEngineering/ToolRegistryTests.cs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/AdversarialRobustness/README.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Security Scan
- GitHub Check: SonarCloud Analysis
- GitHub Check: CodeQL Analysis
🔇 Additional comments (52)
src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (2)
222-231: Good fix - Deserialize now properly restores state.The
Deserializemethod now correctly parses the serialized options and restores the instance state, addressing the previous review feedback.
278-291: Verify the inverse normal CDF approximation against a reference implementation.The implementation structure appears consistent with published approximation algorithms, but the accuracy of the polynomial coefficients should be validated by testing against known quantile values from scipy.stats.norm.ppf or NumPy's equivalent function. Measure the error across the domain [0.5, 0.99] to ensure it meets acceptable tolerance for certified robustness calculations.
src/Diagnostics/ProfileReport.cs (2)
2-2: Verify Newtonsoft.Json dependency is added to project file.The migration from System.Text.Json to Newtonsoft.Json requires that the Newtonsoft.Json NuGet package be added as a project dependency. Check the .csproj project file to confirm the package reference exists (format:
<PackageReference Include="Newtonsoft.Json" Version="..." />), or verify it's included in a centralized Directory.Packages.props file if using Central Package Management.
137-165: Verify JSON output format compatibility between libraries.The change from
System.Text.JsontoNewtonsoft.Jsonintroduces format differences that could impact consumers. System.Text.Json supports ISO 8601-1:2019 format, while Newtonsoft.Json uses a different default format. Since the_startTimeDateTime field is included in the serialized output, consumers expecting the ISO 8601 format may fail to parse the new output.Verify that:
- Any tests validating JSON output format are updated to expect Newtonsoft.Json format
- Existing API consumers or dependent systems can handle the format change
- Custom DateTime converters are applied if format compatibility is required
src/PromptEngineering/Tools/ToolRegistry.cs (2)
160-160: LGTM!Using
ToString(Formatting.None)is the appropriate way to serialize aJObjectto a compact JSON string for the parameter schema output.
3-4: Verify that this breaking API change includes explicit documentation of the rationale.This change replaces the modern System.Text.Json (JsonDocument) with Newtonsoft.Json (JObject), which is generally considered a regression for performance and maintainability in new .NET projects. The switch is only appropriate if ExecuteTool requires dynamic JSON manipulation or advanced serialization features that JsonDocument cannot provide (note: .NET 6+ offers JsonNode as a mutable alternative within System.Text.Json).
Before merging:
- Confirm a documented reason exists explaining why JObject's capabilities are necessary
- Verify all ExecuteTool call sites have been updated to use JObject
- Consider whether System.Text.Json's JsonNode (.NET 6+) or JsonObject could satisfy requirements instead
tests/AiDotNet.Tests/PromptEngineering/ToolRegistryTests.cs (2)
10-40: LGTM!The
MockToolimplementation correctly reflects the updatedIFunctionToolinterface withJObject-based parameters. The use of raw string literals for the parameter schema andTryGetValuefor validation is appropriate.
149-149: LGTM!Test code correctly updated to use
JObject.Parsefor constructing arguments, consistent with the new API.Also applies to: 159-159
src/Interfaces/IFunctionTool.cs (1)
158-168: LGTM!The example code is properly updated to demonstrate
JObjectusage withValue<string>()accessor methods. The documentation remains clear and beginner-friendly.src/PromptEngineering/Tools/FunctionToolBase.cs (1)
106-120: [Your rewritten review comment text here]
[Exactly ONE classification tag]src/Models/SafetyValidationResult.cs (1)
1-45: LGTM! Well-structured data model for safety validation.The class design is clean with sensible defaults for collection properties. The nullable
SanitizedInputappropriately handles cases where sanitization isn't applicable.src/Interfaces/IAlignmentMethod.cs (1)
1-116: LGTM! Comprehensive interface for AI alignment methods.The interface is well-designed with clear method contracts. The extensive documentation with beginner explanations follows the project's documentation standards and will help new users understand RLHF, Constitutional AI, and red teaming concepts.
src/Interfaces/IPredictionModelBuilder.cs (1)
320-326: LGTM! Clean fluent API addition for safety filter configuration.The method follows the established pattern of other
Configure*methods in the interface. The non-nullable parameter is appropriate since explicitly calling this method implies intent to configure safety filtering.src/Models/Results/PredictionModelResult.cs (3)
4-4: LGTM!The using directive correctly imports the safety namespace required for SafetyFilter integration.
461-461: LGTM!The SafetyFilter property follows the established pattern for internal properties in this class.
825-834: Verify whether "enabled by default" SafetyFilter behavior is properly documented.The code creates a
SafetyFilterby default whenSafetyFilterConfigurationis null. Users must explicitly setEnabled = falseto opt out. Confirm that:
- This default-enabled behavior is documented in
PredictionModelResultOptions- The opt-out mechanism (
SafetyFilterConfiguration.Enabled = false) is clearly communicated- Any performance implications are addressed in documentation
tests/AiDotNet.Tests/AdversarialRobustness/SafetyFilterTests.cs (1)
27-42: LGTM!Test correctly validates length limit enforcement.
src/PredictionModelBuilder.cs (3)
64-64: LGTM!The safety filter configuration field follows the established pattern for optional builder configurations and is correctly typed.
1506-1515: LGTM!The
ConfigureSafetyFiltermethod follows the builder pattern consistently, includes proper null validation, and provides clear documentation.
1031-1031: Verify RL path includes safety configuration.The safety filter configuration is correctly propagated in supervised (line 1031) and meta-learning (line 1089) paths. Ensure it's also included in the RL path (
BuildRLInternalAsync) for consistency, though it's not visible in the annotated code snippet.Also applies to: 1089-1089
src/AdversarialRobustness/Attacks/FGSMAttack.cs (2)
70-83: LGTM!The FGSM perturbation application correctly handles both targeted and untargeted attacks with proper sign adjustment and clipping.
150-181: LGTM!The Softmax implementation uses proper numerical stability techniques (max subtraction) and handles edge cases appropriately.
src/Models/Options/SafetyFilterConfiguration.cs (1)
17-39: LGTM!The configuration class is well-designed with sensible defaults, clear documentation, and follows established patterns for optional configurations.
src/Interfaces/IAdversarialAttack.cs (1)
29-101: LGTM!The
IAdversarialAttack<T>interface is well-designed with clear method contracts and excellent documentation that balances technical accuracy with beginner accessibility.src/Models/SafetyFilterResult.cs (1)
9-76: LGTM!Both
SafetyFilterResult<T>andFilterActionare well-designed data containers with clear property names, appropriate defaults, and good documentation.src/AdversarialRobustness/Attacks/AutoAttack.cs (1)
139-168: LGTM!The best candidate selection correctly prioritizes successful attacks and minimizes perturbation size. The logic handles all cases appropriately.
src/AdversarialRobustness/Attacks/PGDAttack.cs (1)
79-103: LGTM!The iterative PGD attack loop correctly implements the algorithm with proper gradient computation, step sizing, epsilon-ball projection, and input clipping.
src/AdversarialRobustness/Attacks/CWAttack.cs (2)
70-89: LGTM!The tanh-space transformation correctly implements the C&W technique for enforcing box constraints, with appropriate numerical stability handling (clamping to avoid atanh singularities).
183-206: LGTM!The attack loss computation correctly implements the C&W objective for both targeted and untargeted attacks, with proper handling of logit differences.
src/Interfaces/ISafetyFilter.cs (1)
1-131: LGTM! Excellent interface design and documentation.The interface is well-structured with clear method contracts and exceptionally thorough XML documentation. The beginner-friendly explanations are particularly helpful for understanding the purpose and usage of each safety filter function.
src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (4)
14-36: LGTM! Proper initialization with defensive checks.The constructor properly validates the options parameter and initializes the random number generator with a seed for reproducibility, which is important for testing and debugging adversarial attacks.
42-67: LGTM! Solid batch processing with proper validation.The batch generation method includes appropriate null checks and dimension validation. The per-sample iteration approach is clean and allows derived classes to override for optimized batch processing if needed.
141-180: LGTM! Correct implementations of norm computations.The helper methods correctly implement sign, L-infinity, and L2 norm calculations. Using double precision for intermediate L2 computations is a good approach to avoid numerical issues.
182-213: LGTM! Correct projection implementations.Both projection methods correctly enforce L-infinity and L2 constraints. The L2 projection appropriately checks if the norm is already within bounds before scaling, and both methods create new vectors to avoid side effects.
src/Interfaces/IAdversarialDefense.cs (1)
1-100: LGTM! Well-designed defense interface with excellent documentation.The interface provides a comprehensive contract for adversarial defense mechanisms. The documentation is particularly strong, with clear explanations of each method's purpose and beginner-friendly analogies that make complex concepts accessible.
src/AdversarialRobustness/Safety/SafetyFilter.cs (6)
26-72: LGTM! Constructor properly initializes pattern dictionaries.The constructor correctly sets up jailbreak and harmful content patterns with reasonable defaults. The pattern-matching approach provides a solid baseline for safety filtering while allowing for future customization through options.
74-169: LGTM! Comprehensive input validation with multiple safety checks.The method implements a layered validation approach, checking for length violations, invalid numeric values, jailbreak attempts, and harmful content. The progressive reduction of the safety score based on findings provides useful signal about input risk.
243-289: LGTM! Pattern-based jailbreak detection with reasonable scoring.The method implements a straightforward pattern-matching approach for detecting jailbreak attempts. The confidence scoring based on the number of matched patterns is reasonable, and the severity classification provides actionable information.
291-364: LGTM! Efficient harmful content detection with category scoring.The method correctly identifies harmful content across multiple categories using pattern matching. The use of
TryGetValueis efficient, and the manual loop for finding the primary category is straightforward and performant.
366-415: LGTM! Proper scoring and serialization implementation.The
ComputeSafetyScoremethod effectively combines multiple safety signals with reasonable weights. The serialization methods are now properly implemented with symmetric encode/decode logic, addressing previous review concerns.
448-481: LGTM! Helper methods with appropriate error handling.The logging method now properly uses the configurable
LogFilePathfrom options with a reasonable fallback, addressing previous feedback. Exception handling for I/O operations is appropriate for a logging context where failures should not disrupt the main flow.src/Models/RedTeamingResults.cs (1)
1-86: LGTM! Clean data model classes with safe defaults.Both
RedTeamingResults<T>andVulnerabilityReportare well-designed data containers with clear property names, comprehensive documentation, and safe default initializations that prevent null reference issues.src/AdversarialRobustness/Defenses/AdversarialTraining.cs (4)
26-70: LGTM! Reasonable initialization with PGD attack for adversarial training.The constructor properly configures a PGD attack with sensible defaults (step size = epsilon/4, 10 iterations, random start). The
ApplyDefensemethod appropriately acknowledges the limitation that full training-time augmentation requires deeper trainer integration and provides a runtime preprocessing defense as an interim solution.
91-181: LGTM! Comprehensive robustness evaluation with proper error handling.The method correctly evaluates model robustness by comparing clean and adversarial accuracy. The exception handling approach (treating attack failures as successful defenses) is reasonable and prevents crashes from malformed attacks. The metrics provide valuable insights into model robustness.
265-292: LGTM! Correct implementations of utility methods.Both
ArgMaxandComputeL2Normare correctly implemented. The L2 norm calculation uses double precision for intermediate computations to avoid numerical issues, which is a good practice.
294-336: LGTM! Clean decorator pattern for preprocessing.The
PreprocessingPredictiveModelwrapper correctly implements the decorator pattern, applying input preprocessing while delegating all other operations to the inner model. The sealed modifier is appropriate for this internal implementation detail.src/AdversarialRobustness/Alignment/RLHFAlignment.cs (6)
29-55: LGTM! Clear RLHF alignment workflow.The
AlignModelmethod follows the standard RLHF approach: first training a reward model from human feedback, then using it to fine-tune the policy model via reinforcement learning. The implementation structure is sound.
120-234: LGTM! Well-structured constitutional AI and red teaming implementations.Both methods are properly implemented.
ApplyConstitutionalPrinciplesuses a clean wrapper pattern, andPerformRedTeaminghandles edge cases gracefully (empty prompts) while validating response length consistency across prompts.
236-271: LGTM! Serialization properly implemented.The
Deserializemethod now correctly restores state from JSON, addressing previous review feedback. The serialization and deserialization methods are symmetric and handle null cases appropriately.
273-398: LGTM! Helper methods with reasonable heuristics for demonstration.The helper methods provide simplified heuristics appropriate for a demonstration implementation. The
IsHarmlessmethod efficiently computes both max and min in a single pass, andComputeCosineSimilarityincludes appropriate epsilon to prevent division by zero.
400-481: LGTM! Nested model with simplified reward adjustment.The
RlhfFineTunedPredictiveModelimplements a simplified version of RLHF fine-tuning. The KL penalty is acknowledged as a placeholder for future integration, and the reward-based adjustment provides a reasonable demonstration of the concept.
483-534: LGTM! Constitutional AI wrapper with iterative refinement.The
ConstitutionalPredictiveModelcorrectly implements the constitutional AI pattern, applying iterative critique and revision to model outputs based on defined principles. The decorator pattern is cleanly implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/AiDotNet.Tensors/Compatibility/HalfCompat.cs (1)
86-112: Consolidate redundant conditional compilation branches.Both the
#if !NET5_0_OR_GREATERand#elsebranches now delegate toMathHelper.Clampforintandlongoverloads, making the conditional compilation unnecessary. The genericClamp<T>methods (lines 79-84, 97-102) are also identical across both branches.Apply this diff to remove the redundant conditional compilation:
-#if !NET5_0_OR_GREATER - public static T Clamp<T>(T value, T min, T max) where T : IComparable<T> - { - if (value.CompareTo(min) < 0) return min; - if (value.CompareTo(max) > 0) return max; - return value; - } - - public static int Clamp(int value, int min, int max) - { - return MathHelper.Clamp(value, min, max); - } - - public static long Clamp(long value, long min, long max) - { - return MathHelper.Clamp(value, min, max); - } -#else - // For NET5+, delegate int/long to Math.Clamp; generic T uses custom implementation public static T Clamp<T>(T value, T min, T max) where T : IComparable<T> { if (value.CompareTo(min) < 0) return min; if (value.CompareTo(max) > 0) return max; return value; } public static int Clamp(int value, int min, int max) { return MathHelper.Clamp(value, min, max); } public static long Clamp(long value, long min, long max) { return MathHelper.Clamp(value, min, max); } -#endif
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/AiDotNet.Tensors/Compatibility/HalfCompat.cs(3 hunks)src/AiDotNet.Tensors/Engines/CpuEngine.cs(1 hunks)src/AiDotNet.Tensors/Operators/AtanhOperator.cs(3 hunks)src/Polyfills/NetFrameworkPolyfills.cs(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/AiDotNet.Tensors/Engines/CpuEngine.cs
🧰 Additional context used
🧬 Code graph analysis (3)
src/AiDotNet.Tensors/Operators/AtanhOperator.cs (2)
src/AiDotNet.Tensors/Helpers/MathHelper.cs (1)
MathHelper(21-1272)src/AiDotNet.Tensors/Engines/CpuEngine.cs (2)
Atanh(1109-1112)Atanh(1115-1118)
src/Polyfills/NetFrameworkPolyfills.cs (1)
src/AiDotNet.Tensors/Helpers/MathHelper.cs (1)
MathHelper(21-1272)
src/AiDotNet.Tensors/Compatibility/HalfCompat.cs (1)
src/AiDotNet.Tensors/Helpers/MathHelper.cs (1)
MathHelper(21-1272)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: SonarCloud Analysis
- GitHub Check: CodeQL Analysis
- GitHub Check: Codacy Security Scan
🔇 Additional comments (5)
src/AiDotNet.Tensors/Compatibility/HalfCompat.cs (1)
2-2: LGTM!The using directive for
AiDotNet.Tensors.Helpersis correctly added to support the MathHelper delegation pattern.src/AiDotNet.Tensors/Operators/AtanhOperator.cs (2)
3-3: LGTM!The using directive for
AiDotNet.Tensors.Helpersis correctly added to support MathHelper delegation.
19-19: LGTM!The delegation to
MathHelper.Atanhcentralizes the Atanh implementation and removes platform-specific conditional branches. Note thatMathHelper.Atanhincludes input clamping to avoid infinities at domain boundaries, which is a good defensive practice for numerical stability.Also applies to: 85-85
src/Polyfills/NetFrameworkPolyfills.cs (2)
3-3: LGTM!The using directive for
AiDotNet.Tensors.Helpersis correctly added to support MathHelper delegation.
171-171: LGTM!The delegation to
MathHelperforLog2andClampmethods successfully eliminates conditional compilation branches and centralizes the numeric operations. This improves maintainability while preserving the public API.Also applies to: 184-184, 197-197, 210-210
d76ada0 to
f054f86
Compare
ooples
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (11)
src/AdversarialRobustness/Documentation/ModelCard.cs (1)
23-26: Constructor approach for Date initialization is appropriate.Setting
Datein the constructor correctly captures the ModelCard creation time. This addresses the concern about property initializer timing that was raised previously.src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (4)
40-44: Fixed seed still undermines statistical validity.The constructor now uses
RandomHelper.CreateSeededRandom(42)(line 43), but this still produces identical noise sequences across runs, which weakens the probabilistic guarantees of randomized smoothing as previously noted. Consider accepting an optional seed viaCertifiedDefenseOptions<T>or using a non-deterministic seed by default.
198-203: Median calculation still incorrect for even-sized lists.Line 202 uses
certifiedRadii[certifiedRadii.Count / 2], which gives the upper middle element for even-length lists. The standard median calculation should average the two middle values when the count is even. This issue was previously flagged and remains unresolved.
251-255: Box-Muller transform can produce NaN when u1 is zero.Line 252's
random.NextDouble()can return exactly 0, causingMath.Log(0)on line 254 to produce-Infinityand resulting inNaNnoise values. This critical bug was previously identified and remains unfixed. Use1.0 - random.NextDouble()to ensure u1 is in (0, 1].
293-302: Confidence parameter is ignored in bound calculations.Both methods hardcode a z-score of 1.96 (lines 296, 301), which corresponds to 95% confidence, ignoring the
confidenceparameter. With the defaultConfidenceLevel = 0.99, this produces incorrect bounds. The z-score should be computed from theconfidenceargument using an inverse normal CDF. This issue was previously flagged and remains unresolved.src/AdversarialRobustness/Attacks/CWAttack.cs (1)
126-158: C&W gradient is finite-difference overwand will not scale to large inputs
ComputeObjectiveAndGradientperturbs eachw[i]and re-evaluates the full objective, leading toO(w.Length)model calls per iteration. For typical C&W use cases (e.g., image inputs), this becomes very slow.If you intend this as a reference implementation, consider:
- Documenting the complexity/limitations in the XML remarks, and/or
- Adding a configuration flag to:
- Use a coarser approximation (e.g., subsample dimensions), or
- Switch to true gradients if
targetModelexposes them.src/AdversarialRobustness/Attacks/AutoAttack.cs (1)
7-29: XML documentation does not match the implemented ensembleThe summary/remarks still describe the canonical AutoAttack ensemble (APGD‑CE, APGD‑DLR, FAB, Square), but this implementation actually runs PGD, C&W, and FGSM. That’s likely to confuse users and misrepresent robustness claims.
Recommend updating the remarks to reflect the concrete attacks used here and optionally noting that this is an AutoAttack‑style ensemble rather than the exact Croce & Hein configuration:
- /// AutoAttack typically includes: - /// - APGD-CE (Auto PGD with Cross-Entropy loss) - /// - APGD-DLR (Auto PGD with DLR loss) - /// - FAB (Fast Adaptive Boundary) - /// - Square Attack + /// This implementation combines: + /// - PGD (Projected Gradient Descent) + /// - C&W (Carlini & Wagner, L2-focused) + /// - FGSM (Fast Gradient Sign Method) + /// It is an AutoAttack-style ensemble, not the exact set of attacks from the + /// original Croce & Hein paper.src/AdversarialRobustness/Attacks/PGDAttack.cs (2)
204-235: Softmax should return a valid distribution whensum <= 0
Softmaxcurrently returns the raw exponentiated logits whensum <= 0, which yields an unnormalized and potentially misleading “probability” vector. Even thoughsum <= 0should be rare, the fallback should still respect the contract of returning probabilities.A simple, safe alternative is to return a uniform distribution in this edge case:
- if (sum <= 0.0) - return probabilities; + if (sum <= 0.0) + { + // Fallback to a uniform distribution + var uniform = NumOps.FromDouble(1.0 / logits.Length); + for (int i = 0; i < probabilities.Length; i++) + { + probabilities[i] = uniform; + } + return probabilities; + }
115-133: RandomStartingPoint can violate the L2 constraint and contradicts its own summary
RandomStartingPointis documented as sampling “within the epsilon‑ball”, but it currently draws each component independently in[-epsilon, epsilon]and never projects for the L2 case. For high‑dimensional inputs this can substantially exceed the L2 budget, whileProjectToEpsilonBalllater strictly enforces it.Either:
- Restrict the random init to L∞ only and update the XML to say so, or
- Adjust the implementation to respect
Options.NormTypeand project the sampled perturbation into the appropriate norm ball (especially for"L2"), e.g.:private Vector<T> RandomStartingPoint(Vector<T> input, T epsilon) { - var randomStart = new Vector<T>(input.Length); - - for (int i = 0; i < input.Length; i++) - { - var randomValue = NumOps.FromDouble(Random.NextDouble() * 2.0 - 1.0); - var perturbation = NumOps.Multiply(epsilon, randomValue); - - randomStart[i] = NumOps.Add(input[i], perturbation); - randomStart[i] = MathHelper.Clamp(randomStart[i], NumOps.Zero, NumOps.One); - } - - return randomStart; + var perturbation = new Vector<T>(input.Length); + for (int i = 0; i < input.Length; i++) + { + var r = NumOps.FromDouble(Random.NextDouble() * 2.0 - 1.0); + perturbation[i] = NumOps.Multiply(epsilon, r); + } + + perturbation = Options.NormType == "L2" + ? ProjectL2(perturbation, epsilon) + : ProjectLInfinity(perturbation, epsilon); + + var randomStart = new Vector<T>(input.Length); + for (int i = 0; i < input.Length; i++) + { + randomStart[i] = MathHelper.Clamp( + NumOps.Add(input[i], perturbation[i]), NumOps.Zero, NumOps.One); + } + return randomStart; }src/AdversarialRobustness/Safety/SafetyFilter.cs (1)
220-231: FilterAction reports sanitization even thoughSanitizeOutputis a no-op
SanitizeOutputcurrently just clones the output vector and ignoresharmfulResult, butFilterOutputrecords an action:
ActionType = "Sanitize"Reason = "Moderate harmful content detected and sanitized"OriginalContent = "Content sanitized"WasModified = trueThis is misleading for consumers and tests; no actual sanitization occurs.
Either implement real sanitization based on
harmfulResult, or make the metadata accurately reflect the behavior, e.g.:- // Sanitize for moderate violations - result.FilteredOutput = SanitizeOutput(output, harmfulResult); - result.WasModified = true; - result.Actions.Add(new FilterAction - { - ActionType = "Sanitize", - Reason = "Moderate harmful content detected and sanitized", - Location = 0, - OriginalContent = "Content sanitized" - }); + // Flag moderate violations without automatic editing (placeholder) + result.FilteredOutput = SanitizeOutput(output, harmfulResult); + result.WasModified = false; + result.Actions.Add(new FilterAction + { + ActionType = "Flag", + Reason = "Moderate harmful content detected (no automatic sanitization applied)", + Location = 0, + OriginalContent = "Content flagged for review" + });or alternatively change
SanitizeOutputto actually mask/replace risky spans and keep the current action description.Also applies to: 440-446
src/AdversarialRobustness/Alignment/RLHFAlignment.cs (1)
110-115: Guard against division by zero when test set is empty.If
evaluationData.TestInputs.Rowsis 0, lines 111-114 will divide by zero, producingNaNmetrics. Add a guard to return zero scores for an empty test set.Apply this diff:
int total = evaluationData.TestInputs.Rows; + if (total == 0) + { + metrics.HelpfulnessScore = 0.0; + metrics.HarmlessnessScore = 0.0; + metrics.HonestyScore = 0.0; + metrics.PreferenceMatchRate = 0.0; + metrics.OverallAlignmentScore = 0.0; + return metrics; + } + metrics.HelpfulnessScore = (double)helpfulCount / total; metrics.HarmlessnessScore = (double)harmlessCount / total; metrics.HonestyScore = (double)honestCount / total; metrics.PreferenceMatchRate = totalPreferenceMatch / total; metrics.OverallAlignmentScore = (metrics.HelpfulnessScore + metrics.HarmlessnessScore + metrics.HonestyScore) / 3.0;
🧹 Nitpick comments (9)
src/AdversarialRobustness/Documentation/ModelCard.cs (1)
176-199: Consider consistent section rendering behavior.Sections like
RobustnessMetrics,FairnessMetrics, andCaveatsare only rendered when non-empty, whileIntendedUses,Limitations, etc. always appear with "Not specified" placeholder. This asymmetry may be intentional (required vs optional sections), but documenting this distinction or making it consistent would improve clarity.src/Models/Options/AdversarialDefenseOptions.cs (1)
1-93: Well-designed options class with sensible defaults.The AdversarialDefenseOptions class provides:
- Reasonable default values for adversarial training (0.5 ratio, 0.1 epsilon, 100 epochs)
- Clear, beginner-friendly documentation
- Comprehensive coverage of defense configuration needs
Consider using enums instead of strings for PreprocessingMethod (line 66) and AttackMethod (line 92). This would provide:
- Type safety and compile-time validation
- IntelliSense support for valid values
- Prevention of typos in configuration
Example:
public enum PreprocessingMethod { JPEG, BitDepthReduction, Denoising, None } public enum AttackMethod { FGSM, PGD, CW, AutoAttack }Then update the properties:
public PreprocessingMethod PreprocessingMethod { get; set; } = PreprocessingMethod.JPEG; public AttackMethod AttackMethod { get; set; } = AttackMethod.PGD;tests/AiDotNet.Tests/AdversarialRobustness/SafetyFilterTests.cs (1)
44-60: LGTM! Consider expanding test coverage for output filtering.The test correctly validates that safety scores are bounded between 0 and 1.
For more comprehensive coverage, consider adding tests for:
- Output filtering behavior (FilterOutput method)
- Harmful content detection (HarmfulContentCategories option)
- Jailbreak detection patterns
- Edge cases (empty vectors, all-NaN inputs, infinity values)
src/Models/Options/AdversarialAttackOptions.cs (1)
1-99: LGTM! Consider using an enum for NormType.The options class provides clear defaults and good documentation for adversarial attack configuration.
Minor suggestion:
NormType(line 61) uses a string value, which means typos like"L-infinty"won't be caught at compile time. Consider using an enum:public enum NormType { LInfinity, L2, L1 }This would provide compile-time type safety and better IDE support.
src/AdversarialRobustness/Attacks/PGDAttack.cs (1)
166-186: Finite-difference gradient is extremely expensive for high-dimensional inputs
ComputeGradientuses forward finite differences with one model evaluation per input dimension. WithIterationsPGD steps this isO(Iterations × input.Length)calls totargetModel.Predict, which will be prohibitive for image-scale inputs or large feature vectors.If automatic differentiation or analytical gradients aren’t available, consider:
- Documenting this cost clearly in the XML remarks (PGD is “slow but strong” here).
- Optionally adding a configuration to:
- Subsample dimensions for gradient estimation, or
- Use a single backward pass when
targetModelexposes gradients.src/AdversarialRobustness/Defenses/AdversarialTraining.cs (1)
30-52: AdversarialTraining state and serialization are inconsistent with the unusedattackMethodThe constructor derives a PGD
attackMethodfromoptions, but:
attackMethodis never used in this class (current behavior only wraps the model for preprocessing and uses an externally suppliedattackinEvaluateRobustness).Deserializemutatesoptionsbut cannot updateattackMethod(it’sreadonly), so any future logic that relies on the internal attack would see stale parameters after loading.Given the current behavior:
- Either remove
attackMethodentirely until you actually integrate training‑time adversarial augmentation, or- Refactor so that:
attackMethodis derived lazily fromoptionson demand, orDeserializereconstructs a new attack instance from the loaded options.This will keep the serialization contract honest and avoid subtle state bugs when you later start using
attackMethod.Also applies to: 190-206
src/AdversarialRobustness/Alignment/RLHFAlignment.cs (3)
429-443: Consider removing unused_klCoefficientfield until implemented.The
_klCoefficientfield is stored (line 429) but explicitly discarded at line 443 with a comment about "future integration." This adds unnecessary state and maintenance burden.Consider either:
- Remove the field entirely until KL regularization is implemented
- Keep it but add a TODO/FIXME comment referencing a tracking issue
private readonly IPredictiveModel<T, Vector<T>, Vector<T>> _baseModel; private readonly Func<Vector<T>, Vector<T>, double> _rewardModel; - private readonly double _klCoefficient; - public RlhfFineTunedPredictiveModel(IPredictiveModel<T, Vector<T>, Vector<T>> baseModel, Func<Vector<T>, Vector<T>, double> rewardModel, double klCoefficient) + public RlhfFineTunedPredictiveModel(IPredictiveModel<T, Vector<T>, Vector<T>> baseModel, Func<Vector<T>, Vector<T>, double> rewardModel, double klCoefficient = 0.0) { _baseModel = baseModel ?? throw new ArgumentNullException(nameof(baseModel)); _rewardModel = rewardModel ?? throw new ArgumentNullException(nameof(rewardModel)); - _klCoefficient = klCoefficient; + // TODO: Implement KL penalty using klCoefficient for PPO-style regularization }
483-506: Consider decoupling from parent class internals.
ConstitutionalPredictiveModelaccesses the parent class's privateoptionsfield via_alignment.options.CritiqueIterations(line 499). This creates tight coupling between the nested class and parent implementation details.Consider passing only the required value:
- private readonly RLHFAlignment<T> _alignment; + private readonly int _critiqueIterations; - public ConstitutionalPredictiveModel(IPredictiveModel<T, Vector<T>, Vector<T>> inner, RLHFAlignment<T> alignment, string[] principles) + public ConstitutionalPredictiveModel(IPredictiveModel<T, Vector<T>, Vector<T>> inner, int critiqueIterations, string[] principles) { _inner = inner ?? throw new ArgumentNullException(nameof(inner)); - _alignment = alignment ?? throw new ArgumentNullException(nameof(alignment)); + _critiqueIterations = critiqueIterations; _principles = principles ?? throw new ArgumentNullException(nameof(principles)); } public Vector<T> Predict(Vector<T> input) { var response = _inner.Predict(input); - for (int i = 0; i < _alignment.options.CritiqueIterations; i++) + for (int i = 0; i < _critiqueIterations; i++) { var critique = GenerateCritique(response, _principles); response = ReviseBasedOnCritique(_inner, input, response, critique); } return response; }And update the call site:
- return new ConstitutionalPredictiveModel(model, this, principles); + return new ConstitutionalPredictiveModel(model, options.CritiqueIterations, principles);
302-318: Document placeholder implementation limitations prominently.Several critical methods are placeholders with simplified logic:
GenerateCritique(line 302) just formats a message without real critiqueReviseBasedOnCritique(line 310) returns the original response unchangedIsHonest(line 341) always returnstrueWhile the comments note these are "simplified," users may not realize the alignment features are non-functional. Consider adding prominent documentation warnings.
Add clear warnings in the class documentation:
/// <summary> /// Implements Reinforcement Learning from Human Feedback (RLHF) for AI alignment. /// </summary> /// <remarks> +/// <para><b>⚠️ Important:</b> This is a prototype implementation with simplified placeholders. +/// Constitutional AI critique/revision and honesty evaluation are not yet functional. +/// Do not use in production without implementing the full logic.</para> /// <para>Also applies to: 341-347
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
src/AdversarialRobustness/Alignment/RLHFAlignment.cs(1 hunks)src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs(1 hunks)src/AdversarialRobustness/Attacks/AutoAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/CWAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/FGSMAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/PGDAttack.cs(1 hunks)src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs(1 hunks)src/AdversarialRobustness/Defenses/AdversarialTraining.cs(1 hunks)src/AdversarialRobustness/Documentation/ModelCard.cs(1 hunks)src/AdversarialRobustness/README.md(1 hunks)src/AdversarialRobustness/Safety/SafetyFilter.cs(1 hunks)src/AiDotNet.Tensors/Compatibility/HalfCompat.cs(3 hunks)src/AiDotNet.Tensors/Engines/CpuEngine.cs(1 hunks)src/AiDotNet.Tensors/Helpers/MathHelper.cs(1 hunks)src/AiDotNet.Tensors/Operators/AtanhOperator.cs(3 hunks)src/Diagnostics/ProfileReport.cs(2 hunks)src/Interfaces/IAdversarialAttack.cs(1 hunks)src/Interfaces/IAdversarialDefense.cs(1 hunks)src/Interfaces/IAlignmentMethod.cs(1 hunks)src/Interfaces/ICertifiedDefense.cs(1 hunks)src/Interfaces/IFunctionTool.cs(5 hunks)src/Interfaces/IPredictionModelBuilder.cs(2 hunks)src/Interfaces/ISafetyFilter.cs(1 hunks)src/Models/AlignmentEvaluationData.cs(1 hunks)src/Models/AlignmentFeedbackData.cs(1 hunks)src/Models/AlignmentMetrics.cs(1 hunks)src/Models/CertifiedAccuracyMetrics.cs(1 hunks)src/Models/CertifiedPrediction.cs(1 hunks)src/Models/HarmfulContentResult.cs(1 hunks)src/Models/JailbreakDetectionResult.cs(1 hunks)src/Models/Options/AdversarialAttackOptions.cs(1 hunks)src/Models/Options/AdversarialDefenseOptions.cs(1 hunks)src/Models/Options/AlignmentMethodOptions.cs(1 hunks)src/Models/Options/CertifiedDefenseOptions.cs(1 hunks)src/Models/Options/PredictionModelResultOptions.cs(1 hunks)src/Models/Options/SafetyFilterConfiguration.cs(1 hunks)src/Models/Options/SafetyFilterOptions.cs(1 hunks)src/Models/RedTeamingResults.cs(1 hunks)src/Models/Results/PredictionModelResult.cs(6 hunks)src/Models/RobustnessMetrics.cs(1 hunks)src/Models/SafetyFilterResult.cs(1 hunks)src/Models/SafetyValidationResult.cs(1 hunks)src/Polyfills/NetFrameworkPolyfills.cs(5 hunks)src/PredictionModelBuilder.cs(4 hunks)src/PromptEngineering/Tools/FunctionToolBase.cs(6 hunks)src/PromptEngineering/Tools/ToolRegistry.cs(4 hunks)tests/AiDotNet.Tests/AdversarialRobustness/SafetyFilterTests.cs(1 hunks)tests/AiDotNet.Tests/PromptEngineering/ToolRegistryTests.cs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- src/Models/Options/AlignmentMethodOptions.cs
- src/Interfaces/IAdversarialAttack.cs
- src/Models/Options/SafetyFilterConfiguration.cs
- src/Interfaces/IAdversarialDefense.cs
- src/Models/AlignmentFeedbackData.cs
- src/PromptEngineering/Tools/ToolRegistry.cs
- src/Models/JailbreakDetectionResult.cs
- src/Models/AlignmentMetrics.cs
- src/Models/Options/CertifiedDefenseOptions.cs
- src/AiDotNet.Tensors/Engines/CpuEngine.cs
- src/Models/SafetyValidationResult.cs
- src/AiDotNet.Tensors/Compatibility/HalfCompat.cs
- src/PromptEngineering/Tools/FunctionToolBase.cs
- src/AiDotNet.Tensors/Operators/AtanhOperator.cs
- src/Models/Options/PredictionModelResultOptions.cs
- src/Models/RobustnessMetrics.cs
- src/Models/Options/SafetyFilterOptions.cs
- src/AdversarialRobustness/Attacks/FGSMAttack.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Security Scan
- GitHub Check: CodeQL Analysis
- GitHub Check: SonarCloud Analysis
🔇 Additional comments (37)
tests/AiDotNet.Tests/PromptEngineering/ToolRegistryTests.cs (3)
10-40: LGTM! MockTool properly updated for Newtonsoft.Json migration.The
MockToolimplementation correctly adapts to the newJObject-based API:
ParameterSchemaproperty type updated toJObjectExecuteandValidateArgumentsmethod signatures useJObjectargumentsTryGetValueis the idiomatic way to check for key existence inJObject
149-150: LGTM!Argument construction correctly updated to use
JObject.Parsewith raw string literal syntax.
159-161: LGTM!Consistent with the JSON library migration. Test correctly verifies exception handling for non-existent tools.
src/Diagnostics/ProfileReport.cs (2)
164-164: Verify JSON output format compatibility with downstream consumers.The serialization method change to
JsonConvert.SerializeObjectis a shift from System.Text.Json to Newtonsoft.Json, which have different default behaviors for DateTime serialization, property casing, and other formatting details. Given the diagnostic nature of this output and the potential for format-sensitive consumers, verify that downstream tooling remains compatible with the new JSON format, particularly for theStartTimeDateTime field.
2-2: Verify Newtonsoft.Json package dependency is added.The migration from System.Text.Json to Newtonsoft.Json requires that the Newtonsoft.Json NuGet package be added to the project file. Confirm the package reference exists in the .csproj before merging.
src/AdversarialRobustness/Documentation/ModelCard.cs (2)
28-101: Property declarations look good.The DTO-style properties with sensible defaults are appropriate for a data model class. Collection initializers ensure non-null defaults.
264-268: File I/O implementation is acceptable.The synchronous file write is straightforward. Consider adding an async variant (
SaveToFileAsync) in a future iteration if large model cards or async contexts become common use cases.src/Models/CertifiedPrediction.cs (1)
1-46: LGTM! Clean data model with proper initialization.The class is well-structured with clear documentation. The use of
default!forCertifiedRadius(line 20) is acceptable since T is constrained to numeric types in practice, wheredefaultyields a sensible zero value. TheCertificationDetailsdictionary is properly initialized to avoid null references.src/Models/CertifiedAccuracyMetrics.cs (1)
1-43: LGTM! Consistent metrics model.The class follows the same clean pattern as
CertifiedPrediction<T>. Multiple properties usedefault!for type T (lines 22, 27, 37), which is acceptable for numeric types. Documentation is thorough and initialization is correct.src/Interfaces/ICertifiedDefense.cs (1)
1-114: Excellent interface design with outstanding documentation.The interface is well-architected with clear method contracts. The extensive documentation (lines 6-27) provides valuable context about certified defenses, including concrete examples (Randomized Smoothing, IBP, CROWN) and practical motivations. The beginner-friendly explanations throughout strike a good balance between accessibility and technical accuracy.
src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (4)
47-108: Certification logic is correct and well-structured.The main certification flow is sound: sample predictions under Gaussian noise, tally class counts, compute certified radius using concentration inequalities, and package results with diagnostic details. The manual loop (lines 80-87) for finding the top class is efficient (O(n)) and more readable than LINQ alternatives.
221-231: Deserialization now correctly restores state.The
Deserializemethod (line 230) now properly parses the JSON and restores theoptionsfield, addressing the previous concern that it was a no-op. The null-coalescing fallback to a new instance is a sensible safeguard.
304-319: ArgMax implementation is correct.The method efficiently finds the index of the maximum element using a simple linear scan. The implementation is straightforward and correct.
321-324: Clipping implementation is correct.The method properly delegates to
MathHelper.Clampto constrain values to the [0, 1] range.src/Models/AlignmentEvaluationData.cs (1)
1-35: LGTM!The AlignmentEvaluationData class is well-designed with:
- Appropriate property types for alignment evaluation workflows
- Safe defaults that prevent null reference exceptions
- Clear, beginner-friendly documentation
- Consistent with other data model patterns in the codebase
src/Models/RedTeamingResults.cs (1)
1-86: LGTM!Both RedTeamingResults and VulnerabilityReport are well-designed data containers:
- Comprehensive property coverage for red teaming workflows
- Appropriate property types and safe defaults
- Clear documentation with beginner-friendly explanations
- VulnerabilityReport provides actionable detail with recommendations
src/Interfaces/IPredictionModelBuilder.cs (2)
320-325: LGTM!The ConfigureSafetyFilter method follows the established builder pattern:
- Consistent signature with other Configure methods
- Returns builder instance for method chaining
- Appropriate generic type parameter usage
- Clear, concise documentation
12-12: Verify the new using directive.The addition of
using AiDotNet.Models.Options;supports the new ConfigureSafetyFilter method. Ensure this namespace exists and contains SafetyFilterConfiguration.src/Polyfills/NetFrameworkPolyfills.cs (3)
169-172: Verify MathHelper.Log2 implementation.The polyfill now delegates to MathHelper.Log2. Confirm that MathHelper.Log2 provides equivalent functionality across all target frameworks, including .NET Framework 4.7.1.
3-3: Check for circular dependencies with AiDotNet.Tensors.Helpers.The new using directive on line 3 introduces a dependency from the Polyfills layer to Tensors.Helpers. Verify that:
- No files in src/AiDotNet.Tensors/Helpers/ reference the Polyfills namespace
- This dependency aligns with the project's layering architecture
182-185: Verify MathHelper.Clamp implementations.The polyfill delegates all Clamp overloads (int, float, double) to MathHelper. Confirm that MathHelper provides these overloads and that they handle edge cases correctly (e.g., when min > max).
src/Interfaces/IFunctionTool.cs (1)
1-1: [Rewritten review comment]
[Classification tag]src/AdversarialRobustness/README.md (1)
1-370: Excellent comprehensive documentation!This README provides clear, beginner-friendly documentation for the adversarial robustness module. The structure is logical, examples are practical, and the best practices section adds valuable guidance.
Verify that the code examples compile and match the actual API signatures:
#!/bin/bash # Verify that key classes referenced in README exist echo "Checking for FGSMAttack..." fd -e cs FGSMAttack.cs echo "Checking for PGDAttack..." fd -e cs PGDAttack.cs echo "Checking for SafetyFilter..." fd -e cs SafetyFilter.cs echo "Checking for RandomizedSmoothing..." fd -e cs RandomizedSmoothing.cs echo "Checking for RLHFAlignment..." fd -e cs RLHFAlignment.cs echo "Checking for ModelCard..." fd -e cs ModelCard.cstests/AiDotNet.Tests/AdversarialRobustness/SafetyFilterTests.cs (2)
10-25: LGTM! Good NaN validation test.This test correctly validates that NaN values in input vectors are detected and marked as invalid by the safety filter.
27-42: LGTM! Good length validation test.This test correctly validates that inputs exceeding MaxInputLength are detected and marked as invalid.
src/Models/Results/PredictionModelResult.cs (4)
826-830: LGTM! Clean safety filter initialization.The initialization logic correctly:
- Respects the explicit opt-out via
Enabled == false- Falls back to a default
SafetyFilter<T>instance with default options when not explicitly configured- Provides flexibility while maintaining safety by default
1016-1065: LGTM! Solid matrix validation with good defensive checks.This helper method correctly:
- Validates each row of the matrix through the safety filter
- Provides clear error messages with row indices on validation failures
- Performs defensive length checks to catch incompatible sanitized outputs (lines 1056-1059)
- Optimizes by only reconstructing the matrix when sanitization actually occurred
The row-by-row validation is appropriate for the safety-critical nature of this feature.
1067-1094: LGTM! Consistent output filtering for matrices.This method correctly applies safety filtering to each row of the output matrix with appropriate defensive checks (lines 1085-1088) to ensure filtered rows maintain the expected dimensions.
948-1012: Consider safer alternatives toUnsafe.Asfor type conversions.The safety filter integration uses
Unsafe.As<Vector<T>, TInput>and similar conversions at lines 963, 969, 1004, and 1010. While thetypeof(TInput) == typeof(Vector<T>)checks provide runtime validation before the conversion,Unsafe.Assuppresses the runtime's normal type safety checks and places responsibility on the caller to ensure the cast is legal.Safer alternatives to consider:
- Use pattern matching:
if (sanitized is TInput typedInput) { dataForPrediction = typedInput; }to leverage type safety checks- Add generic constraints:
where TInput : Vector<T>to enforce type relationships at compile time- Introduce overloaded methods for
Vector<T>andMatrix<T>to avoid generic conversions entirelyVerify that the
typeofchecks remain consistently applied across all call sites where these methods are invoked.src/Models/HarmfulContentResult.cs (1)
1-79: LGTM! Clean data models with safe defaults.Both
HarmfulContentResult<T>andHarmfulContentFindingare well-structured data containers with appropriate default values:
- Collections initialize to empty (preventing null reference exceptions)
HarmfulContentDetecteddefaults tofalse(safe default)RecommendedActiondefaults to"Allow"(permissive default, appropriate for opt-in safety)- String properties initialize to
string.Empty(preventing null issues)src/Models/SafetyFilterResult.cs (1)
1-76: LGTM! Well-designed safety result models.Both
SafetyFilterResult<T>andFilterActionare cleanly implemented with thoughtful defaults:
IsSafedefaults tofalse(fail-closed, security-conscious)FilteredOutputinitializes toVector<T>.Empty()(prevents null access)- Collections initialize to empty (safe and predictable)
ReplacementContentis nullable where appropriate (line 75)The initialization strategy supports safe default construction while allowing explicit configuration.
src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (3)
42-67: LGTM!The batch generation logic correctly validates inputs, checks dimensions, and delegates to
GenerateAdversarialExamplefor each sample. The explicit loop is appropriate for performance-critical code.
70-94: LGTM!The perturbation calculation is correct with proper null checks and dimension validation.
144-213: LGTM!The utility methods (
Sign,ComputeLInfinityNorm,ComputeL2Norm,ProjectLInfinity,ProjectL2) are correctly implemented with appropriate numeric operations and bounds checking.src/AdversarialRobustness/Alignment/RLHFAlignment.cs (3)
138-234: LGTM!The red teaming implementation properly validates inputs, handles edge cases (empty prompts), and ensures consistent output dimensions across all adversarial prompts.
327-339: LGTM!The
IsHarmlesscheck now computes both max and min values in a single pass, addressing the previous optimization suggestion.
380-398: LGTM!The cosine similarity computation correctly handles edge cases (mismatched lengths, zero vectors) with appropriate safeguards.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
commitlint.config.js (1)
32-34: Outdated comment: missingdepsandreverttypes.The comment on line 34 lists valid types but doesn't include
depsorrevert, which are both now allowed per thetype-enumrule above.🔎 Proposed fix
// All new commits MUST follow conventional commits format: // type(scope)?: description -// Valid types: feat, fix, docs, refactor, perf, test, chore, ci, style +// Valid types: feat, fix, docs, refactor, perf, test, chore, ci, style, revert, deps
🧹 Nitpick comments (5)
.github/workflows/commitlint-fix.yml (1)
60-64: SkippingRevertcommits may hide intentional reverts that need validation.The regex skips commits starting with "Revert", but
revertis a valid conventional commit type. Genuine revert commits (e.g.,revert: undo feature X) would be skipped from validation, but malformed reverts would also pass unchecked.Consider only skipping auto-generated revert commits (e.g.,
Revert "..."pattern from GitHub) rather than all commits starting with "Revert":- if [[ "$FIRST_LINE" =~ ^(Merge|Revert) ]]; then + if [[ "$FIRST_LINE" =~ ^Merge || "$FIRST_LINE" =~ ^Revert\ \" ]]; thensrc/Models/Options/CertifiedDefenseOptions.cs (1)
1-105: LGTM! Well-documented options class with sensible defaults.The options class provides a clean configuration surface for certified defense mechanisms with comprehensive XML documentation and reasonable default values. The
RandomSeedproperty correctly handles both reproducible testing (when set) and proper statistical validity (when null).Optional: Consider adding property validation
For robustness, you could add validation to ensure properties stay within valid ranges:
private int _numSamples = 1000; public int NumSamples { get => _numSamples; set => _numSamples = value > 0 ? value : throw new ArgumentOutOfRangeException(nameof(NumSamples), "Must be positive"); } private double _confidenceLevel = 0.99; public double ConfidenceLevel { get => _confidenceLevel; set => _confidenceLevel = value is >= 0 and <= 1 ? value : throw new ArgumentOutOfRangeException(nameof(ConfidenceLevel), "Must be in [0, 1]"); } // Similar for NoiseSigma, BatchSize, etc.This prevents invalid configurations but adds ceremony to a simple options class, so it's optional.
src/Helpers/StatisticsHelper.cs (2)
1114-1224: Beta CDF / inverse CDF: implementation looks sound; consider a couple of robustness tweaks and verificationThe overall structure here is good: parameter checks are clear, endpoints for
xare handled explicitly inCalculateBetaCDF, andCalculateInverseBetaCDFuses a sensible Newton–Raphson refinement with clamping and derivative guard.Two things worth tightening:
- For the inverse, you currently reject
probability <= 0or>= 1. That’s fine for Clopper–Pearson (since all call sites pass strictly interior probabilities), but it makes the API a bit surprising compared to other CDF inverses in this class which usually special‑case the extremes. You may want to:
- Treat
probability == 0→ return0andprobability == 1→ return1, or- Document explicitly that the valid domain is
(0, 1)and that callers are responsible for clipping.- Both
CalculateInverseBetaCDFand existing callers ofRegularizedIncompleteBetaFunctionrely on that helper being numerically correct. Given its non‑trivial custom implementation, I’d strongly recommend adding a small regression test set comparing:
CalculateBetaCDFandCalculateInverseBetaCDF(round‑trip) against a trusted library (e.g., SciPy) for a few(α, β, p)combinations,- and verifying that the Newton loop converges in a reasonable number of iterations across typical parameter ranges used in your randomized smoothing pipeline.
No blockers here, but some targeted tests would de‑risk this quite a bit.
1226-1327: Clopper–Pearson helpers: formulas look correct; small consistency/refactor suggestionsThe two Clopper–Pearson helpers appear mathematically correct:
- Two‑sided interval:
lower: Beta⁻¹(α/2; successes, trials − successes + 1) withlower=0whensuccesses==0upper: Beta⁻¹(1 − α/2; successes + 1, trials − successes) withupper=1whensuccesses==trials- One‑sided lower bound:
alpha = 1 − confidence,lower= Beta⁻¹(α; successes, trials − successes + 1) with0whensuccesses==0.A couple of minor improvements you might consider:
- The argument validation (
successes/trialsbounds andconfidencein (0,1)) is duplicated between the two methods; factoring that into a small private helper would reduce drift if you ever tweak the preconditions.- Behavior for edge cases like
trials == 0is currently “throw”; if these APIs are ever called on empty evaluation batches, you may prefer returning a degenerate interval like(0, 1)instead, but that’s a product decision rather than a correctness bug.From a correctness perspective this block looks good as‑is.
src/AdversarialRobustness/Attacks/AutoAttack.cs (1)
138-168: Document fallback behavior when all attacks fail.If all three attacks throw exceptions (empty candidates list), the method returns the original
inputunmodified. While this is a safe fail-safe behavior, it might be unexpected. Consider adding a comment or debug log to clarify this fallback:🔎 Suggested documentation enhancement
// Select the best adversarial example // Prioritize: 1) Successful attacks, 2) Smallest perturbation + // If all attacks fail (empty candidates), returns the original input as a fallback Vector<T> bestAdversarial = input; double bestPerturbation = double.PositiveInfinity; bool foundSuccessful = false;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/commitlint-fix.yml(1 hunks).github/workflows/pr-title-lint.yml(3 hunks)commitlint.config.js(1 hunks)src/AdversarialRobustness/Attacks/AutoAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/CWAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/FGSMAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/PGDAttack.cs(1 hunks)src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs(1 hunks)src/Helpers/StatisticsHelper.cs(1 hunks)src/Interfaces/IInputGradientComputable.cs(1 hunks)src/Models/Options/CertifiedDefenseOptions.cs(1 hunks)src/NeuralNetworks/NeuralNetworkBase.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/AdversarialRobustness/Attacks/CWAttack.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/Interfaces/IInputGradientComputable.cssrc/Models/Options/CertifiedDefenseOptions.cssrc/AdversarialRobustness/Attacks/AutoAttack.cssrc/AdversarialRobustness/Attacks/PGDAttack.cssrc/Helpers/StatisticsHelper.cssrc/AdversarialRobustness/Attacks/FGSMAttack.cssrc/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cssrc/NeuralNetworks/NeuralNetworkBase.cs
📚 Learning: 2025-12-19T19:05:13.598Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 445
File: src/Interfaces/IPredictionModelBuilder.cs:7-8
Timestamp: 2025-12-19T19:05:13.598Z
Learning: In ooples/AiDotNet, IAggregationStrategy<TModel> is declared in AiDotNet.Interfaces (src/Interfaces/IAggregationStrategy.cs). For src/Interfaces/IPredictionModelBuilder.cs (also in AiDotNet.Interfaces), referencing IAggregationStrategy does not require an additional using. Avoid flagging missing using directives for this case in future reviews.
Applied to files:
src/Interfaces/IInputGradientComputable.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/Interfaces/IInputGradientComputable.cssrc/Models/Options/CertifiedDefenseOptions.cssrc/AdversarialRobustness/Attacks/AutoAttack.cssrc/AdversarialRobustness/Attacks/PGDAttack.cssrc/Helpers/StatisticsHelper.cssrc/AdversarialRobustness/Attacks/FGSMAttack.cssrc/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cssrc/NeuralNetworks/NeuralNetworkBase.cs
🔇 Additional comments (25)
commitlint.config.js (1)
6-10: LGTM! Thedepstype addition aligns with the PR objectives.This properly extends the conventional commits type list to allow dependabot-style commits.
.github/workflows/pr-title-lint.yml (2)
30-30: LGTM! Comprehensivedepstype support added.The changes correctly:
- Add
depsto the validation pattern (line 30)- Auto-detect dependency updates from title keywords (lines 77-79)
- Document the
depstype with its no-release impact (line 150)Also applies to: 77-79, 150-150
20-21: Remove concern aboutactions/checkout@v6— the version exists and is documented.The official actions/checkout repository documents the use of
actions/checkout@v6, so the workflow will not fail due to the version not existing. The original concern about v4 being the current stable version is outdated..github/workflows/commitlint-fix.yml (2)
19-23: Verifyactions/checkout@v6availability.Same concern as in
pr-title-lint.yml- verify that v6 exists.
197-298: LGTM on dependabot handling job.The
handle-dependabotjob correctly:
- Runs only for dependabot PRs (line 201)
- Checks for uppercase descriptions and fixes to lowercase
- Preserves the commit body
- Uses
--force-with-leasefor safer force-pushsrc/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (7)
48-57: Excellent fix to the random seed issue!The constructor now properly uses the configurable
RandomSeedfrom options when provided (for reproducible testing) and falls back toEnvironment.TickCountfor non-deterministic random generation (proper statistical validity). This directly addresses the previous concern while maintaining good documentation.
60-121: LGTM! Solid implementation of the certification logic.The method correctly implements randomized smoothing with proper null checks, noise sampling, majority voting, and certified radius computation. The explicit loop for finding the top class (lines 93-100) is appropriate given the developer's prior decision to prioritize clarity and performance over LINQ brevity.
211-229: Perfect fix to the median calculation!The median computation now correctly handles both even-sized lists (averaging the two middle values) and odd-sized lists (taking the middle element). This addresses the previous review feedback and ensures statistically accurate metrics.
247-269: Deserialization properly implemented!The
Deserializemethod now correctly restores state from the serialized JSON bytes, addressing the previous review concern. The method includes appropriate null handling and matches theSerializeformat. Combined withLoadModel, state persistence is now fully functional.
271-289: Excellent fix to the Box-Muller edge case!The change to
var u1 = 1.0 - random.NextDouble()(line 280) correctly prevents theMath.Log(0)edge case that would produceNaNnoise values. The detailed comment explains the reasoning clearly. This directly addresses the previous review concern about potentialInfinityandNaNvalues.
313-329: Outstanding fix with an even better approach!The confidence bound methods now correctly use the
confidenceparameter viaStatisticsHelper.CalculateClopperPearsonInterval, addressing the previous concern about the hardcoded 1.96 z-score. Moreover, using the exact Clopper-Pearson method (based on the Beta distribution) provides guaranteed coverage and is superior to the normal approximation that was previously suggested. Excellent improvement!
1-352: Exceptional work addressing all previous review feedback!This implementation demonstrates thorough attention to previous review comments:
- ✅ Random seed now configurable for testing vs. statistical validity
- ✅ Deserialization fully implemented
- ✅ Box-Muller edge case fixed with clear documentation
- ✅ Confidence intervals use the confidence parameter with exact Clopper-Pearson method
- ✅ Median calculation handles both even and odd counts correctly
The code is well-structured, properly documented, and integrates cleanly with the broader
StatisticsHelperandMathHelperutilities. The implementation correctly follows the randomized smoothing paper by Cohen et al. and provides solid certified robustness guarantees.src/Interfaces/IInputGradientComputable.cs (1)
1-63: LGTM! Well-designed interface for input gradient computation.The interface design is clean and provides both Vector and Tensor overloads for flexibility. The comprehensive documentation clearly explains the distinction between parameter gradients and input gradients, along with practical use cases for adversarial robustness and interpretability.
src/AdversarialRobustness/Attacks/FGSMAttack.cs (4)
24-83: LGTM! Correct FGSM implementation with proper validations.The implementation correctly applies the FGSM formula (x_adv = x + epsilon * sign(gradient)) with appropriate handling for targeted attacks via negation. Input validation and clipping to valid range [0, 1] are properly implemented.
85-156: LGTM! Correct analytic gradient computation.The analytic gradient path correctly implements the cross-entropy loss gradient (∂L/∂z = p - one_hot(target)) and backpropagates through the model. The consistent use of
targetClassfor both targeted and untargeted attacks addresses the previously flagged issue.
158-193: LGTM! Consistent finite-difference gradient approximation.The finite-difference implementation correctly uses the same
targetClassfor both original and perturbed loss computations, ensuring a consistent gradient estimate. This addresses the previously flagged asymmetry issue.
195-251: LGTM! Numerically stable loss and softmax computation.The cross-entropy loss computation includes appropriate safeguards (max(prob, 1e-10)) to avoid log(0), and the softmax implementation uses the standard numerical stability trick of subtracting the maximum logit.
src/NeuralNetworks/NeuralNetworkBase.cs (2)
19-19: LGTM! Appropriate interface addition.Adding
IInputGradientComputable<T>to the class signature enables neural networks to expose input gradient computation capabilities, which is essential for adversarial robustness techniques.
1195-1219: Verify ifPredict()properly caches intermediate activations for gradient computation.The
ComputeInputGradientmethods callPredict()to perform the forward pass before computing input gradients. Standard backpropagation implementations require cached intermediate activations (layer inputs and outputs) to compute gradients correctly. Verify that the concrete implementations ofPredict()cache the necessary activations (_layerInputs and _layerOutputs) that layerBackward()methods depend on. IfPredict()does not cache these activations, consider usingForwardWithMemory()instead (see lines 429-451), or confirm that the cached activations are handled through an alternative mechanism withinBackwardWithInputGradient().src/AdversarialRobustness/Attacks/AutoAttack.cs (1)
37-59: LGTM! Proper initialization of component attacks.The constructor correctly initializes the three component attacks (PGD, CW, FGSM) with appropriate options. The explicit
UseRandomStart = truefor PGD is a good practice to ensure stronger attacks in the ensemble.src/AdversarialRobustness/Attacks/PGDAttack.cs (5)
29-103: LGTM! Correct PGD algorithm implementation.The iterative PGD attack correctly implements the standard algorithm: gradient computation, step along gradient sign, projection to epsilon-ball, and clipping. The conditional random start initialization enhances attack strength.
125-151: LGTM! Random start correctly projects to norm ball.The random starting point implementation now correctly projects the entire perturbation vector to the appropriate norm ball (L2 or L-infinity) before applying it to the input. This addresses the previously flagged issue where the L2 constraint could have been violated.
153-179: LGTM! Correct epsilon-ball projection.The projection correctly computes the perturbation relative to the original input, projects it to the specified norm ball, and applies it back to the original. This ensures the adversarial example stays within the epsilon budget.
181-280: LGTM! Correct gradient computation with consistent target class.Both analytic and finite-difference gradient implementations correctly use the same
targetClassfor all loss computations, ensuring accurate gradient estimates. The analytic path properly implements the cross-entropy + softmax gradient formula.
282-345: LGTM! Softmax edge case properly handled.The softmax implementation now includes proper handling for the edge case where sum <= 0, returning a uniform distribution as a fallback. This addresses the previously flagged issue and prevents returning unnormalized values. The numerical stability trick and loss computation are both correct.
e5037e3 to
1898c54
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/AdversarialRobustness/Documentation/ModelCard.cs (1)
303-308: RedundantDateassignment (previously flagged).Line 307 sets
Date = DateTime.UtcNow, but the constructor already initializes this. The previous review suggested removing this redundancy.Suggested fix
var card = new ModelCard { ModelName = modelName, - ModelType = modelType ?? string.Empty, - Date = DateTime.UtcNow + ModelType = modelType ?? string.Empty };src/AdversarialRobustness/Alignment/RLHFAlignment.cs (1)
110-115: Guard against division by zero when TestInputs is empty.If
evaluationData.TestInputs.Rowsis 0,totalwill be 0, causing divisions on lines 111–114 to produceNaN. Consider adding a guard to handle this edge case.🔎 Proposed fix
int total = evaluationData.TestInputs.Rows; + if (total == 0) + { + metrics.HelpfulnessScore = 0.0; + metrics.HarmlessnessScore = 0.0; + metrics.HonestyScore = 0.0; + metrics.PreferenceMatchRate = 0.0; + metrics.OverallAlignmentScore = 0.0; + return metrics; + } + metrics.HelpfulnessScore = (double)helpfulCount / total; metrics.HarmlessnessScore = (double)harmlessCount / total; metrics.HonestyScore = (double)honestCount / total; metrics.PreferenceMatchRate = totalPreferenceMatch / total;
🧹 Nitpick comments (8)
src/AdversarialRobustness/Documentation/ModelCard.cs (1)
56-101: Consider protecting collection properties from null assignment.Collection properties with public setters can be set to
null, which would causeNullReferenceExceptioninGenerate()when accessing.Count. While typical usage won't null these, you could add defensive measures.Options:
- Use
initsetters if mutation after construction isn't needed.- Add null-coalescing in
Generate()(e.g.,(IntendedUses?.Count ?? 0) > 0).- Use property setters with null guards.
src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (3)
54-56: Consider using unseededRandom()for true non-determinism.When
RandomSeedis not provided, line 56 usesEnvironment.TickCountas the seed, which is deterministic within a process execution window. MultipleRandomizedSmoothinginstances created in rapid succession will receive identical seeds, producing correlated noise sequences that weaken statistical validity.For proper statistical independence, use an unseeded
Random()when no explicit seed is configured:🔎 Suggested improvement
// Use the configured seed if provided, otherwise use non-deterministic random // for proper statistical validity of the certification this.random = options.RandomSeed.HasValue ? RandomHelper.CreateSeededRandom(options.RandomSeed.Value) - : RandomHelper.CreateSeededRandom(Environment.TickCount); + : new Random();
72-72: Consider validatingNoiseSigmais positive.The noise standard deviation is converted from
options.NoiseSigmawithout validation. IfNoiseSigmais zero or negative, the certification becomes meaningless. While this may be validated during options construction, an explicit guard here would prevent subtle bugs.🔎 Suggested validation
var sigma = NumOps.FromDouble(options.NoiseSigma); + if (NumOps.LessThanOrEquals(sigma, NumOps.Zero)) + { + throw new ArgumentException("NoiseSigma must be positive for meaningful certification.", nameof(options)); + }
131-135: Consider parallelizing batch certification for performance.The current implementation processes each input sequentially. For large batches, parallel processing could significantly reduce wall-clock time since each certification is independent.
🔎 Suggested parallelization
- var results = new CertifiedPrediction<T>[inputs.Rows]; - for (int i = 0; i < inputs.Rows; i++) - { - results[i] = CertifyPrediction(inputs.GetRow(i), model); - } + var results = new CertifiedPrediction<T>[inputs.Rows]; + Parallel.For(0, inputs.Rows, i => + { + results[i] = CertifyPrediction(inputs.GetRow(i), model); + });Note: Ensure thread-safety of the
randomfield if parallelizing. Consider usingThreadLocal<Random>or synchronization.src/AdversarialRobustness/Defenses/AdversarialTraining.cs (2)
55-70: Consider documenting the unusedattackMethodfield or adding a TODO.The
attackMethodfield (line 31) is initialized but never used because training-time adversarial augmentation requires trainer integration. Consider adding a// TODO:comment near the field declaration to clarify the intent and prevent accidental removal.
252-263: Denoising is a no-op placeholder.The
ApplyDenoisingmethod currently just clones the input without applying any actual denoising. Consider either implementing a basic denoising algorithm (e.g., Gaussian smoothing) or documenting this as a placeholder.🔎 Option: Add a TODO comment
private Vector<T> ApplyDenoising(Vector<T> input) { - // Simple moving average denoising (for demonstration) - // In practice, would use more sophisticated methods + // TODO: Implement actual denoising (e.g., moving average, Gaussian blur) + // Currently returns input unchanged as a placeholder var clone = new Vector<T>(input.Length); for (int i = 0; i < input.Length; i++) { clone[i] = input[i]; } - return clone; // Simplified + return clone; }.github/workflows/commitlint-fix.yml (1)
102-102: Body whitespace normalization may alter formatting.The
awk '{$1=$1};1'command normalizes whitespace within each line of the commit body, which could unintentionally alter formatting in code blocks or intentionally indented content. Lines 182 and 298 preserve the body exactly using justtail -n +3.Consider using the same approach here for consistency:
🔎 Proposed fix
- BODY=$(echo "$COMMIT_MSG" | tail -n +3 | awk '{$1=$1};1') + BODY=$(echo "$COMMIT_MSG" | tail -n +3)src/AdversarialRobustness/Safety/SafetyFilter.cs (1)
383-392: Consider using LINQ'sMaxByfor cleaner max-finding.The manual loop to find the category with the highest score is correct but verbose. LINQ's
MaxBywould be more concise and idiomatic.🔎 Suggested refactor
- var primaryCategory = string.Empty; - var maxScore = double.NegativeInfinity; - foreach (var kv in result.CategoryScores) - { - if (kv.Value > maxScore) - { - maxScore = kv.Value; - primaryCategory = kv.Key; - } - } - - result.PrimaryHarmCategory = primaryCategory; + result.PrimaryHarmCategory = result.CategoryScores.MaxBy(kv => kv.Value).Key;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/commitlint-fix.yml(5 hunks)src/AdversarialRobustness/Alignment/RLHFAlignment.cs(1 hunks)src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs(1 hunks)src/AdversarialRobustness/Attacks/AutoAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/CWAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/FGSMAttack.cs(1 hunks)src/AdversarialRobustness/Attacks/PGDAttack.cs(1 hunks)src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs(1 hunks)src/AdversarialRobustness/Defenses/AdversarialTraining.cs(1 hunks)src/AdversarialRobustness/Documentation/ModelCard.cs(1 hunks)src/AdversarialRobustness/README.md(1 hunks)src/AdversarialRobustness/Safety/SafetyFilter.cs(1 hunks)src/AiDotNet.Tensors/Compatibility/HalfCompat.cs(3 hunks)src/AiDotNet.Tensors/Engines/CpuEngine.cs(1 hunks)src/AiDotNet.Tensors/Helpers/MathHelper.cs(1 hunks)src/AiDotNet.Tensors/Operators/AtanhOperator.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/AiDotNet.Tensors/Compatibility/HalfCompat.cs
- src/AiDotNet.Tensors/Helpers/MathHelper.cs
- src/AdversarialRobustness/Attacks/AutoAttack.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/AiDotNet.Tensors/Operators/AtanhOperator.cssrc/AiDotNet.Tensors/Engines/CpuEngine.cssrc/AdversarialRobustness/Documentation/ModelCard.cssrc/AdversarialRobustness/Safety/SafetyFilter.cssrc/AdversarialRobustness/Attacks/PGDAttack.cssrc/AdversarialRobustness/Attacks/AdversarialAttackBase.cssrc/AdversarialRobustness/Attacks/FGSMAttack.cssrc/AdversarialRobustness/Defenses/AdversarialTraining.cssrc/AdversarialRobustness/Attacks/CWAttack.cssrc/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cssrc/AdversarialRobustness/Alignment/RLHFAlignment.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/AiDotNet.Tensors/Operators/AtanhOperator.cssrc/AiDotNet.Tensors/Engines/CpuEngine.cssrc/AdversarialRobustness/Documentation/ModelCard.cssrc/AdversarialRobustness/Safety/SafetyFilter.cssrc/AdversarialRobustness/Attacks/PGDAttack.cssrc/AdversarialRobustness/Attacks/AdversarialAttackBase.cssrc/AdversarialRobustness/Attacks/FGSMAttack.cssrc/AdversarialRobustness/Defenses/AdversarialTraining.cssrc/AdversarialRobustness/Attacks/CWAttack.cssrc/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cssrc/AdversarialRobustness/Alignment/RLHFAlignment.cs
📚 Learning: 2025-12-19T19:05:13.598Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 445
File: src/Interfaces/IPredictionModelBuilder.cs:7-8
Timestamp: 2025-12-19T19:05:13.598Z
Learning: In ooples/AiDotNet, IAggregationStrategy<TModel> is declared in AiDotNet.Interfaces (src/Interfaces/IAggregationStrategy.cs). For src/Interfaces/IPredictionModelBuilder.cs (also in AiDotNet.Interfaces), referencing IAggregationStrategy does not require an additional using. Avoid flagging missing using directives for this case in future reviews.
Applied to files:
src/AdversarialRobustness/Attacks/CWAttack.cs
🔇 Additional comments (37)
src/AdversarialRobustness/Documentation/ModelCard.cs (4)
5-26: Well-documented class with appropriate constructor initialization.The XML documentation is thorough, explaining the Model Card concept with a beginner-friendly analogy and proper academic citation. Setting
Datein the constructor rather than as a property initializer is the correct approach for capturing instantiation time.
107-258: Clean markdown generation with consistent formatting.The
Generate()method handles empty states gracefully with "Not specified" placeholders, uses consistent date formatting (ISO 8601), and appropriately conditionalizes optional sections like Robustness and Fairness metrics. The StringBuilder usage is efficient for this use case.
265-281: Robust file handling with proper validation.Good defensive checks: validates the file path, normalizes with
Path.GetFullPath, and creates parent directories as needed. The synchronous I/O is appropriate for small documentation files like model cards.
310-320: Good defensive copying for mutable parameters.The null checks combined with defensive copies (
new Dictionary<string, double>(...)) properly protect internal state from external mutation. The additional empty check prevents adding meaningless entries.src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (5)
48-57: Past issues successfully resolved.The constructor now properly handles seeding: it uses the configured
RandomSeedwhen provided (for reproducible testing) or a time-based seed otherwise. This addresses the previous concern about hardcoded seeds weakening statistical validity.
277-283: Box-Muller transform correctly avoidslog(0).Using
1.0 - random.NextDouble()ensuresu1is in the range (0, 1], preventingMath.Log(0)from producing-Infinityand resultingNaNnoise values. This fix resolves the previous critical issue.
313-329: Confidence intervals now use exact Clopper-Pearson method.Both
ComputeLowerBoundandComputeUpperBoundnow delegate toStatisticsHelper.CalculateClopperPearsonInterval, which provides exact coverage guarantees using the Beta distribution. This correctly addresses the previous issue where a hardcoded z-score of 1.96 ignored theconfidenceparameter.
248-257: Deserialization properly implemented.The
Deserializemethod now parses the JSON byte array and restores theoptionsfield, fixing the previous no-op implementation. The null-coalescing operator provides a safe default if deserialization fails.
216-229: Median calculation correctly handles even-sized lists.The implementation now checks if the count is even and averages the two middle elements, or takes the single middle element for odd counts. This properly addresses the previous issue where even-sized lists gave the upper middle value.
src/AiDotNet.Tensors/Operators/AtanhOperator.cs (1)
3-3: Clean refactoring to centralize Atanh computation.The changes successfully delegate scalar Atanh calculations to
MathHelper.Atanh, removing conditional compilation branches and centralizing the implementation. The float variant correctly casts the double result. This simplification improves maintainability without affecting correctness.Also applies to: 19-19, 85-85
src/AiDotNet.Tensors/Engines/CpuEngine.cs (1)
1251-1256: CentralizingAtanhgeneric fallback viaMathHelper.Atanhlooks goodUsing
numOps.ToDouble(vector[i])together withMathHelper.AtanhandnumOps.FromDouble(...)keeps the implementation consistent with the numeric operations abstraction and removes framework-specific#iflogic from this method. No issues spotted with the loop or conversions.src/AdversarialRobustness/README.md (1)
1-371: Well-structured and comprehensive documentation.The README provides excellent coverage of the adversarial robustness module with clear sections for attacks, defenses, certified robustness, AI alignment, and safety infrastructure. The code examples are practical and the academic references are properly cited.
src/AdversarialRobustness/Attacks/FGSMAttack.cs (2)
52-83: LGTM!The
GenerateAdversarialExampleimplementation correctly applies FGSM perturbation with proper input validation, epsilon-scaled sign of gradient, targeted/untargeted handling, and clipping to valid [0,1] range.
97-156: LGTM!The gradient computation correctly routes to analytic gradients when the model supports
IInputGradientComputable<T>and falls back to finite differences otherwise. The cross-entropy gradient formula∂L/∂z = p - one_hot(target)is correctly implemented.src/AdversarialRobustness/Attacks/CWAttack.cs (4)
53-111: LGTM!The C&W attack implementation correctly uses tanh-space parameterization for automatic box constraints, iterative gradient descent optimization, and tracks the best adversarial example by minimizing L2 perturbation among successful attacks. The initialization
atanh(2x-1)correctly inverts the(tanh(w)+1)/2transformation.
170-213: LGTM!The analytic gradient correctly applies the chain rule through the tanh parameterization. The derivative
dx_adv/dw = (1 - tanh²(w))/2correctly computes the Jacobian of the(tanh(w)+1)/2transformation.
222-266: LGTM!The attack loss gradient correctly implements the C&W hinge-like loss. For untargeted attacks, the gradient maximizes
max_other - true_logit; for targeted attacks, it maximizestarget_logit - max_other. The sparsity (only non-zero at relevant indices) when the loss is active is correct.
271-309: Finite-difference fallback is computationally expensive but necessary.This O(n) model-call-per-iteration fallback is appropriate for models that don't implement
IInputGradientComputable<T>. The analytic gradient path (lines 147-150) is preferred when available.src/AdversarialRobustness/Attacks/PGDAttack.cs (3)
59-103: LGTM!The PGD attack correctly implements iterative gradient-based perturbation with optional random start, proper projection back into the epsilon-ball, and clipping to valid [0,1] range. The targeted/untargeted handling via sign negation is correct.
125-151: LGTM!The random starting point now correctly projects the perturbation to the appropriate norm ball (L2 or L-infinity) before applying it to the input, addressing the previous concern about L2 budget violations for high-dimensional inputs.
308-345: LGTM!The softmax implementation correctly handles the edge case where
sum <= 0by falling back to a uniform distribution, preventing NaN/Infinity values. The numerical stability trick of subtracting the maximum logit is properly applied.src/AdversarialRobustness/Defenses/AdversarialTraining.cs (4)
37-52: LGTM!The constructor properly initializes the PGD attack with reasonable defaults (epsilon/4 step size, 10 iterations, random start) derived from the defense options.
91-181: LGTM!The robustness evaluation correctly computes clean accuracy, adversarial accuracy, average perturbation size, and attack success rate. The exception handling for failed attacks (lines 162-171) appropriately catches specific exception types and counts them as defended samples.
197-206: LGTM!The deserialization now properly restores state from the byte array, addressing the previous concern about the no-op implementation. The null-coalescing fallback to a new
AdversarialDefenseOptions<T>()ensures the options field is never null after deserialization.
294-335: LGTM!The
PreprocessingPredictiveModelwrapper correctly applies preprocessing before delegating to the inner model'sPredictmethod, while transparently forwarding all other interface methods to maintain the expected behavior..github/workflows/commitlint-fix.yml (4)
11-14: Concurrency control properly implemented.The concurrency block correctly prevents race conditions by serializing runs per PR and canceling in-progress runs when new events arrive.
43-57: Robust empty-commit handling.The fallback for
grep -cprevents failures on empty input, and the early exit cleanly handles the zero-commits case.
166-205: Clear handling of both single and multi-commit scenarios.The distinction between amending single commits (preserving history) and squashing multiple commits (with clear documentation of why) addresses prior concerns about destructive behavior. The detailed commit message on lines 195-204 explains the consolidation and guides reviewers to the PR for full context.
235-336: Dependabot handling is clean and consistent.The separate job properly isolates dependabot-specific logic, checks and fixes subject case, and provides clear feedback via PR comments.
src/AdversarialRobustness/Safety/SafetyFilter.cs (8)
39-87: Constructor and pattern initialization look solid.The initialization logic correctly validates options, and
InitializePatternsproperly centralizes pattern setup for both construction and deserialization. The switch expression provides sensible defaults for unknown categories.
89-184: Input validation logic is comprehensive and well-structured.The multi-layered validation (length, numeric validity, jailbreak, harmful content) provides strong defense-in-depth. Safety score compounding via
Math.Minappropriately reflects cumulative risk.
186-263: Output filtering implements appropriate risk-based actions.The tiered response (block for severe violations, sanitize for moderate) aligns well with safety requirements. The sanitization implementation now performs real modifications, addressing earlier concerns.
402-414: Safety score computation is well-balanced.The weighted combination of validation, harm, and jailbreak scores (0.4/0.4/0.2) provides a comprehensive safety assessment. Clamping ensures the result stays within valid bounds.
422-477: Serialization and persistence logic is robust.The symmetry between
Serialize/Deserializeis maintained by callingInitializePatternsafter deserialization. File operations include appropriate validation and directory creation.
502-543: Sanitization implementation now provides meaningful protection.The updated logic correctly zeros out harmful regions when findings are available, or applies global dampening based on harm score. This addresses the earlier concern about no-op sanitization.
545-566: Logging implementation handles failures gracefully.Specific exception catching for
IOExceptionandUnauthorizedAccessExceptionprevents logging failures from crashing the safety filter, while the null/whitespace check onLogFilePathprovides a reasonable fallback.
479-500: I was unable to verify the claims in this review comment due to repository access limitations. The codebase could not be cloned in the verification environment, preventing me from:
- Confirming whether
ConvertToTextis actually a placeholder method- Checking for existing tokenizer/decoder implementations in the project
- Examining test expectations and how the method integrates with pattern matching
- Assessing the actual impact on the safety filtering pipeline
To complete verification, this review comment would require direct access to the repository or manual inspection of:
- The
SafetyFilter.csimplementation and any related decoder interfaces- Test files for
ConvertToTextexpectations- How vector-based inputs flow through the pattern matching system
- Whether tokenizer libraries like
Microsoft.ML.TokenizersorSharpTokenare already referenced
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 (1)
src/Helpers/StatisticsHelper.cs (1)
6975-7061: PACF implementation is incorrect; ACF normalization is non‑standardThere are two issues in the ACF/PACF block:
CalculateAutoCorrelationFunctionnormalization
- You compute
varianceusing the sample formula (denominatorn-1), but normalize each lag by(n - lag) * variance. For lag 0 this yieldsacf[0] = (n-1)/ninstead of exactly 1, and the denominator changes with lag.- Standard ACF uses a fixed denominator
Σ (xₜ-μ)², independent of lag, givingacf[0] == 1by construction.
CalculatePartialAutoCorrelationFunction(PACF) algorithm is wrong
- It recomputes ACF on a truncated prefix
temp[0..k-1]each iteration instead of using the full‑series ACF, which breaks the Durbin–Levinson recurrence.- More critically,
phi[k, j]is updated usingphi[k, k]beforephi[k, k]has been computed (you setphi[k,k]=0at the start of the loop and only assign the true value after the numerator/denominator step). This means the update step is effectively a no‑op andphi[k,j]never incorporates the newphi[k,k].- As a result, PACF values will be wrong for all lags
k > 1.This is a functional correctness problem for PACF consumers and should be fixed before relying on these APIs.
Suggested fix for ACF + PACF
A minimal, more standard implementation:
- public static Vector<T> CalculateAutoCorrelationFunction(Vector<T> series, int maxLag) - { - var n = series.Length; - var acf = new T[maxLag + 1]; - var mean = CalculateMean(series); - var variance = CalculateVariance(series); - - for (int lag = 0; lag <= maxLag; lag++) - { - var sum = _numOps.Zero; - for (int i = 0; i < n - lag; i++) - { - sum = _numOps.Add(sum, _numOps.Multiply( - _numOps.Subtract(series[i], mean), - _numOps.Subtract(series[i + lag], mean))); - } - acf[lag] = _numOps.Divide(sum, _numOps.Multiply(_numOps.FromDouble(n - lag), variance)); - } - - return new Vector<T>(acf); - } + public static Vector<T> CalculateAutoCorrelationFunction(Vector<T> series, int maxLag) + { + int n = series.Length; + var acf = new T[maxLag + 1]; + T mean = CalculateMean(series); + + // Denominator: sum of squared deviations (fixed for all lags) + T denom = _numOps.Zero; + for (int i = 0; i < n; i++) + { + T diff = _numOps.Subtract(series[i], mean); + denom = _numOps.Add(denom, _numOps.Multiply(diff, diff)); + } + + if (_numOps.Equals(denom, _numOps.Zero)) + return new Vector<T>(acf); // all zeros; acf[0] remains 0 + + for (int lag = 0; lag <= maxLag; lag++) + { + T sum = _numOps.Zero; + for (int i = 0; i < n - lag; i++) + { + sum = _numOps.Add(sum, _numOps.Multiply( + _numOps.Subtract(series[i], mean), + _numOps.Subtract(series[i + lag], mean))); + } + acf[lag] = _numOps.Divide(sum, denom); + } + + return new Vector<T>(acf); + } @@ - public static Vector<T> CalculatePartialAutoCorrelationFunction(Vector<T> series, int maxLag) - { - var pacf = new T[maxLag + 1]; - pacf[0] = _numOps.One; - - var phi = new T[maxLag + 1, maxLag + 1]; - - for (int k = 1; k <= maxLag; k++) - { - var temp = new T[k]; - for (int j = 0; j < k; j++) - { - temp[j] = series[j]; - } - - var acf = CalculateAutoCorrelationFunction(new Vector<T>(temp), k); - - phi[k, k] = _numOps.Zero; - if (k > 1) - { - for (int j = 1; j < k; j++) - { - phi[k, j] = _numOps.Subtract(phi[k - 1, j], _numOps.Multiply(phi[k, k], phi[k - 1, k - j])); - } - } - - var numerator = acf[k]; - for (int j = 1; j < k; j++) - { - numerator = _numOps.Subtract(numerator, _numOps.Multiply(phi[k - 1, j], acf[k - j])); - } - - var denominator = _numOps.One; - for (int j = 1; j < k; j++) - { - denominator = _numOps.Subtract(denominator, _numOps.Multiply(phi[k - 1, j], acf[j])); - } - - phi[k, k] = _numOps.Divide(numerator, denominator); - pacf[k] = phi[k, k]; - } - - return new Vector<T>(pacf); - } + public static Vector<T> CalculatePartialAutoCorrelationFunction(Vector<T> series, int maxLag) + { + // Use full-series ACF once + Vector<T> acf = CalculateAutoCorrelationFunction(series, maxLag); + + var pacf = new T[maxLag + 1]; + pacf[0] = _numOps.One; + + var phi = new T[maxLag + 1, maxLag + 1]; + + for (int k = 1; k <= maxLag; k++) + { + // Durbin–Levinson recursion + T numerator = acf[k]; + for (int j = 1; j < k; j++) + { + numerator = _numOps.Subtract(numerator, _numOps.Multiply(phi[k - 1, j], acf[k - j])); + } + + T denominator = _numOps.One; + for (int j = 1; j < k; j++) + { + denominator = _numOps.Subtract(denominator, _numOps.Multiply(phi[k - 1, j], acf[j])); + } + + phi[k, k] = _numOps.Divide(numerator, denominator); + + // Update lower-order coefficients + for (int j = 1; j < k; j++) + { + phi[k, j] = _numOps.Subtract(phi[k - 1, j], _numOps.Multiply(phi[k, k], phi[k - 1, k - j])); + } + + pacf[k] = phi[k, k]; + } + + return new Vector<T>(pacf); + }
♻️ Duplicate comments (5)
src/AdversarialRobustness/Safety/ContentClassifierBase.cs (2)
44-48: Virtual method call in constructor is risky.Calling
GetDefaultCategories()(a virtual method) in the constructor can lead to unexpected behavior if a derived class overrides it and accesses uninitialized fields. The derived class constructor hasn't run yet at this point.This issue was already flagged by static analysis. Consider passing categories explicitly or using a factory pattern.
72-86: Consider using LINQ for batch classification.This was already flagged by static analysis. The foreach loop could be replaced with
.Select(...)for a more functional style:🔎 Suggested refactor
public virtual ContentClassificationResult<T>[] ClassifyBatch(Matrix<T> contents) { if (contents == null) { throw new ArgumentNullException(nameof(contents)); } - var results = new ContentClassificationResult<T>[contents.Rows]; - for (int i = 0; i < contents.Rows; i++) - { - results[i] = Classify(contents.GetRow(i)); - } - - return results; + return Enumerable.Range(0, contents.Rows) + .Select(i => Classify(contents.GetRow(i))) + .ToArray(); }src/AdversarialRobustness/Safety/RuleBasedContentClassifier.cs (1)
191-198: Inefficient use ofContainsKeyfollowed by indexer.This was already flagged by static analysis. Use
TryGetValueor the collection expression pattern:🔎 Suggested fix
- if (!_categoryPatterns.ContainsKey(category)) - { - _categoryPatterns[category] = new List<string>(); - SupportedCategories = _categoryPatterns.Keys.ToArray(); - } - - _categoryPatterns[category].Add(pattern); + if (!_categoryPatterns.TryGetValue(category, out var patterns)) + { + patterns = new List<string>(); + _categoryPatterns[category] = patterns; + SupportedCategories = _categoryPatterns.Keys.ToArray(); + } + + patterns.Add(pattern);src/AdversarialRobustness/Safety/SafetyFilter.cs (1)
309-316: Guard against division by zero in confidence calculation.Line 312 divides by
totalPatternswithout verifying it's non-zero. While the currentInitializePatternsadds patterns unconditionally, future changes could leavejailbreakPatternsempty, causingNaNresults.🔎 Defensive fix
- if (matchedPatterns > 0) + if (matchedPatterns > 0 && totalPatterns > 0) { result.JailbreakDetected = true; result.ConfidenceScore = Math.Min(1.0, (double)matchedPatterns / totalPatterns * 2.0); result.Severity = result.ConfidenceScore; result.JailbreakType = matchedPatterns > 2 ? "Sophisticated" : "Basic"; result.RecommendedActions = new[] { "Block", "Log", "Alert" }; }src/Helpers/StatisticsHelper.cs (1)
1159-1260: Clopper–Pearson / Beta CDF APIs look correct;CalculateInverseBetaCDFhas unused localsThe new
CalculateBetaCDF,CalculateInverseBetaCDF,CalculateClopperPearsonInterval, andCalculateClopperPearsonLowerBoundimplementations follow the standard formulas (two‑sided and one‑sided CP bounds via Beta quantiles) with appropriate argument validation and boundary handling; the bisection inCalculateInverseBetaCDFis robust and monotone-safe.One small cleanup: in
CalculateInverseBetaCDFthe localsdouble a = _numOps.ToDouble(alpha); double b = _numOps.ToDouble(beta);are never used. They can be removed to silence the “useless assignment to local variable” warning already reported by static analysis.
Proposed minimal cleanup
- double p = _numOps.ToDouble(probability); - double a = _numOps.ToDouble(alpha); - double b = _numOps.ToDouble(beta); + double p = _numOps.ToDouble(probability);Also applies to: 1282-1363
🧹 Nitpick comments (21)
tests/AiDotNet.Tests/UnitTests/AdversarialRobustness/RLHFAlignmentTests.cs (3)
205-216: Consider using less strict precision for floating-point comparisons.Line 215 uses
precision: 10for comparing floating-point values. While this works for simple arithmetic averages, a precision of 6-8 decimal places is typically more appropriate for double comparisons and reduces the risk of flakiness from rounding differences.🔎 Suggested adjustment
- Assert.Equal(expectedOverall, metrics.OverallAlignmentScore, precision: 10); + Assert.Equal(expectedOverall, metrics.OverallAlignmentScore, precision: 6);
479-496: Consider using less strict precision for success rate comparison.Line 495 uses
precision: 10for comparing the success rate calculation. Similar to the earlier test at Line 215, a precision of 6-8 decimal places is more typical and provides better resilience against rounding variations.🔎 Suggested adjustment
- Assert.Equal((double)vulnerableCount / 3, results.SuccessRate, precision: 10); + Assert.Equal((double)vulnerableCount / 3, results.SuccessRate, precision: 6);
1072-1087: Consider clarifying the comment for the extreme bias condition.Line 1079 states "Extreme bias (mean > 0.8)" but the condition at Line 1077 checks
mean > 0.7. While the output does produce mean = 0.9 (which is > 0.8), the comment could be clearer about whether it's describing the trigger threshold or the output characteristic.🔎 Suggested clarification
else if (mean > 0.7) { - // Extreme bias (mean > 0.8) + // Extreme bias output (triggered by mean > 0.7, produces output mean = 0.9) return new Vector<double>(new double[] { 0.9, 0.9, 0.9 }); }tests/AiDotNet.Tests/UnitTests/AdversarialRobustness/ModelCardTests.cs (2)
117-155: Consider verifying specific items in collection tests.Tests for Limitations, EthicalConsiderations, Recommendations, and Caveats only verify the count of added items but don't assert that the specific items were actually added. While this passes, verifying the actual content would make the tests more robust.
Example improvement for Limitations test
var card = new ModelCard(); card.Limitations.Add("Limited to English text"); card.Limitations.Add("May not perform well on short texts"); Assert.Equal(2, card.Limitations.Count); + Assert.Contains("Limited to English text", card.Limitations); + Assert.Contains("May not perform well on short texts", card.Limitations);Similar improvements can be applied to EthicalConsiderations, Recommendations, and Caveats tests.
347-362: Simplify section-counting logic.The manual counting logic for verifying the absence of "## Robustness Metrics" is verbose. This could be simplified for better readability.
🔎 Proposed refactor
[Fact] public void Generate_OmitsRobustnessSection_WhenEmpty() { var card = new ModelCard(); var result = card.Generate(); - // Count occurrences of "## Robustness Metrics" - int count = 0; - int index = 0; - while ((index = result.IndexOf("## Robustness Metrics", index)) != -1) - { - count++; - index++; - } - Assert.Equal(0, count); + Assert.DoesNotContain("## Robustness Metrics", result); }tests/AiDotNet.Tests/UnitTests/AdversarialRobustness/AdversarialTrainingTests.cs (2)
93-162: LGTM! ApplyDefense tests validate core wrapping behavior.The tests correctly verify model wrapping logic, null validation, and that defended models can make predictions. The use of
Assert.SameandAssert.NotSameappropriately validates wrapping behavior.Optional: Consider adding negative tests for null data/labels
While null model is tested, adding tests for null
dataandlabelsparameters would make the coverage more complete:[Fact] public void ApplyDefense_WithNullData_ThrowsException() { var options = new AdversarialDefenseOptions<double>(); var defense = new AdversarialTraining<double>(options); var model = new MockClassificationModel(); var labels = new Vector<int>(new[] { 0, 1, 2 }); Assert.Throws<ArgumentNullException>(() => defense.ApplyDefense(null!, labels, model)); } [Fact] public void ApplyDefense_WithNullLabels_ThrowsException() { var options = new AdversarialDefenseOptions<double>(); var defense = new AdversarialTraining<double>(options); var model = new MockClassificationModel(); var data = new Matrix<double>(3, 4); Assert.Throws<ArgumentNullException>(() => defense.ApplyDefense(data, null!, model)); }
601-614: Consider stronger assertions for empty data edge case.The test verifies that evaluation doesn't crash with empty data, but the comment on line 613 suggests uncertainty about expected behavior. For better test clarity and regression detection, consider asserting specific expected values (e.g.,
Assert.True(double.IsNaN(metrics.CleanAccuracy))orAssert.Equal(0.0, metrics.CleanAccuracy)) based on the intended behavior.Optional: Add explicit assertions for empty data metrics
var metrics = defense.EvaluateRobustness(model, testData, labels, attack); Assert.NotNull(metrics); -// With 0 rows, metrics should be NaN or 0 depending on implementation +// Empty data should return zero metrics (no samples to evaluate) +Assert.Equal(0.0, metrics.CleanAccuracy); +Assert.Equal(0.0, metrics.AdversarialAccuracy); +Assert.Equal(0.0, metrics.AttackSuccessRate);Or, if NaN is expected:
+Assert.True(double.IsNaN(metrics.CleanAccuracy)); +Assert.True(double.IsNaN(metrics.AdversarialAccuracy));src/AdversarialRobustness/Safety/IContentClassifier.cs (1)
59-95: Result class is well-structured with sensible defaults.The
ContentClassificationResult<T>class provides good default values. Consider usinginitsetters instead ofsetfor immutability after construction if the result is not meant to be mutated post-creation.src/AdversarialRobustness/Safety/ContentClassifierBase.cs (1)
188-201: Hardcoded action thresholds could be configurable.The thresholds
0.8(Block) and0.5(Warn) are hardcoded. Consider making these configurable properties to allow consumers to adjust sensitivity for different use cases.tests/AiDotNet.Tests/AdversarialRobustness/ContentClassifierTests.cs (1)
173-183: Theory test has only one test case.This
[Theory]with a single[InlineData]doesn't leverage the parameterization benefit. Consider either adding more test cases or converting to a[Fact].🔎 Suggested enhancement
[Theory] [InlineData("Hello world", "Allow")] +[InlineData("", "Allow")] +[InlineData("This is safe content", "Allow")] public void ClassifyText_ReturnsAppropriateAction(string text, string expectedAction)Or if only testing safe content, convert to
[Fact].src/AdversarialRobustness/Safety/RuleBasedContentClassifier.cs (1)
204-210: Consider usingTryGetValuefor consistency.Same pattern as
AddPattern. UsingTryGetValuehere would be more idiomatic:🔎 Suggested fix
public void ClearCategory(string category) { - if (_categoryPatterns.ContainsKey(category)) + if (_categoryPatterns.TryGetValue(category, out var patterns)) { - _categoryPatterns[category].Clear(); + patterns.Clear(); } }src/AdversarialRobustness/Safety/SafetyFilter.cs (1)
37-38: Consider marking fields asreadonly.The
jailbreakPatternsandharmfulContentPatternsfields are initialized once and never reassigned. Marking themreadonlyprevents accidental reassignment and signals immutability of the references.🔎 Proposed fix
- private List<string> jailbreakPatterns; - private Dictionary<string, List<string>> harmfulContentPatterns; + private readonly List<string> jailbreakPatterns; + private readonly Dictionary<string, List<string>> harmfulContentPatterns;src/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cs (1)
479-552: Consider implementing proper Softmax bound propagation.The current Softmax handling (pass-through) is noted as a limitation. While acceptable for an initial implementation, proper Softmax bound propagation requires considering all elements together (as the comment notes). Consider adding a TODO or documenting this as a known limitation in the class remarks.
src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs (3)
562-582: Refactor empty if-blocks for clarity.The conditional logic is correct but uses empty if-blocks with comments, which is confusing. The intent is to keep CROWN bounds when tighter, otherwise use IBP bounds.
🔎 Proposed simplification
// Take the tighter of CROWN and IBP bounds for (int i = 0; i < outputDim; i++) { - if (NumOps.GreaterThan(outputLower[i], ibpLower[i])) - { - // CROWN lower is tighter - } - else + if (!NumOps.GreaterThan(outputLower[i], ibpLower[i])) { outputLower[i] = ibpLower[i]; } - if (NumOps.LessThan(outputUpper[i], ibpUpper[i])) - { - // CROWN upper is tighter - } - else + if (!NumOps.LessThan(outputUpper[i], ibpUpper[i])) { outputUpper[i] = ibpUpper[i]; } }
721-727: Sigmoid, Tanh, and LeakyReLU use identity passthrough instead of CROWN relaxation.These activations are monotonic, so proper CROWN relaxation could compute tighter bounds using the derivative at the interval midpoint or optimal tangent lines. The current passthrough is functionally safe but may produce looser bounds than necessary, reducing certification rates.
Would you like me to provide implementations for Sigmoid/Tanh/LeakyReLU CROWN relaxations similar to the ReLU case?
909-924: Potential numerical overflow in Sigmoid and Tanh computations.For extreme input values,
NumOps.Exp(x)can overflow to infinity, causing NaN results. Consider using numerically stable formulations:
- Sigmoid: For x > 0, use
1 / (1 + exp(-x)); for x < 0, useexp(x) / (1 + exp(x)).- Tanh: Use
Math.Tanhor a stable implementation clamping exp arguments.This is a minor concern if pre-activation bounds are reasonably constrained in practice.
src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (1)
318-334: Duplicate interval computation inComputeLowerBoundandComputeUpperBound.Both methods compute the same Clopper-Pearson interval via
StatisticsHelper<T>.CalculateClopperPearsonInterval, then extract different parts. Since they're called together inCertifyPrediction, consider computing the interval once.🔎 Proposed refactor
+ private (double Lower, double Upper) ComputeConfidenceInterval(double pA, int n, double confidence) + { + int successes = (int)Math.Round(pA * n); + var interval = StatisticsHelper<T>.CalculateClopperPearsonInterval(successes, n, NumOps.FromDouble(confidence)); + return (NumOps.ToDouble(interval.Lower), NumOps.ToDouble(interval.Upper)); + } + private double ComputeLowerBound(double pA, int n, double confidence) { - // Use exact Clopper-Pearson confidence interval via Beta distribution - // This provides guaranteed coverage, unlike the normal approximation - int successes = (int)Math.Round(pA * n); - var interval = StatisticsHelper<T>.CalculateClopperPearsonInterval(successes, n, NumOps.FromDouble(confidence)); - return NumOps.ToDouble(interval.Lower); + return ComputeConfidenceInterval(pA, n, confidence).Lower; } private double ComputeUpperBound(double pA, int n, double confidence) { - // Use exact Clopper-Pearson confidence interval via Beta distribution - // This provides guaranteed coverage, unlike the normal approximation - int successes = (int)Math.Round(pA * n); - var interval = StatisticsHelper<T>.CalculateClopperPearsonInterval(successes, n, NumOps.FromDouble(confidence)); - return NumOps.ToDouble(interval.Upper); + return ComputeConfidenceInterval(pA, n, confidence).Upper; }Then in
CertifyPrediction, callComputeConfidenceIntervalonce:var (pA_lower, pA_upper) = ComputeConfidenceInterval(pA, options.NumSamples, options.ConfidenceLevel);src/Helpers/StatisticsHelper.cs (2)
1006-1037: LogGamma implementation looks correct; minor allocation/perf nitThe Numerical Recipes–style
LogGammaimplementation is mathematically sound for positivexand consistent with how the rest of the file converts throughdouble. The only concern is thatcoefficientsis allocated on every call; if this becomes a hot path, consider making it astatic readonly double[]field to avoid repeated allocations. You may also want to guard explicitly against non‑positivexto fail fast instead of returning NaNs for out‑of‑domain inputs.
6614-6704: Adjusted Rand Index implementation matches standard definitionThe
CalculateAdjustedRandIndexroutine correctly builds the contingency table, row/column marginals, and uses the standard ARI formulaARI = (Index - Expected) / (Max - Expected)with proper combinatorics. Edge-case handling forn < 2returning 1 and for a zero denominator is reasonable. The use ofDictionary<(T,T), int>and suppression of CS8714 is acceptable here givenTis constrained to numeric types viaINumericOperations<T>.If you want to tighten things further, you could:
- Precompute
comb2(count)via a small helper to avoid repeating(long)count * (count - 1) / 2.0logic.- Optionally short‑circuit early when both label vectors are identical to avoid building the contingency table in that trivial case.
tests/AiDotNet.Tests/UnitTests/Statistics/ClopperPearsonTests.cs (2)
97-145: Consider adding invalid parameter tests forCalculateInverseBetaCDF.The round-trip consistency test is excellent for verifying mathematical correctness. However, unlike
CalculateBetaCDF, there are no tests for invalidalphaandbetaparameters in the inverse function. If the production code validates these parameters, adding similar tests would improve coverage consistency.🔎 Suggested test addition
[Fact] public void CalculateInverseBetaCDF_InvalidAlpha_ThrowsArgumentException() { Assert.Throws<ArgumentOutOfRangeException>(() => StatisticsHelper<double>.CalculateInverseBetaCDF(0.5, 0.0, 1.0)); Assert.Throws<ArgumentOutOfRangeException>(() => StatisticsHelper<double>.CalculateInverseBetaCDF(0.5, -1.0, 1.0)); } [Fact] public void CalculateInverseBetaCDF_InvalidBeta_ThrowsArgumentException() { Assert.Throws<ArgumentOutOfRangeException>(() => StatisticsHelper<double>.CalculateInverseBetaCDF(0.5, 1.0, 0.0)); Assert.Throws<ArgumentOutOfRangeException>(() => StatisticsHelper<double>.CalculateInverseBetaCDF(0.5, 1.0, -1.0)); }
387-410: Consider adding float type tests forCalculateInverseBetaCDFandCalculateClopperPearsonLowerBound.The current float type tests cover
CalculateBetaCDFandCalculateClopperPearsonInterval. For consistency, consider adding similar tests for the other two methods if float precision is relevant for those APIs.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs(1 hunks)src/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cs(1 hunks)src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs(1 hunks)src/AdversarialRobustness/Safety/ContentClassifierBase.cs(1 hunks)src/AdversarialRobustness/Safety/IContentClassifier.cs(1 hunks)src/AdversarialRobustness/Safety/RuleBasedContentClassifier.cs(1 hunks)src/AdversarialRobustness/Safety/SafetyFilter.cs(1 hunks)src/Helpers/StatisticsHelper.cs(2 hunks)tests/AiDotNet.Tests/AdversarialRobustness/CROWNVerificationTests.cs(1 hunks)tests/AiDotNet.Tests/AdversarialRobustness/ContentClassifierTests.cs(1 hunks)tests/AiDotNet.Tests/AdversarialRobustness/IntervalBoundPropagationTests.cs(1 hunks)tests/AiDotNet.Tests/AdversarialRobustness/SafetyFilterTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/AdversarialRobustness/AdversarialAttackTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/AdversarialRobustness/AdversarialTrainingTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/AdversarialRobustness/ModelCardTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/AdversarialRobustness/RLHFAlignmentTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/AdversarialRobustness/RandomizedSmoothingTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Statistics/ClopperPearsonTests.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
tests/AiDotNet.Tests/AdversarialRobustness/IntervalBoundPropagationTests.cstests/AiDotNet.Tests/UnitTests/AdversarialRobustness/RLHFAlignmentTests.cstests/AiDotNet.Tests/UnitTests/AdversarialRobustness/AdversarialAttackTests.cstests/AiDotNet.Tests/UnitTests/AdversarialRobustness/AdversarialTrainingTests.cstests/AiDotNet.Tests/UnitTests/AdversarialRobustness/RandomizedSmoothingTests.cstests/AiDotNet.Tests/AdversarialRobustness/ContentClassifierTests.cssrc/AdversarialRobustness/Safety/IContentClassifier.cssrc/AdversarialRobustness/Safety/SafetyFilter.cssrc/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cstests/AiDotNet.Tests/AdversarialRobustness/CROWNVerificationTests.cssrc/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cstests/AiDotNet.Tests/UnitTests/AdversarialRobustness/ModelCardTests.cssrc/AdversarialRobustness/Safety/RuleBasedContentClassifier.cssrc/AdversarialRobustness/Safety/ContentClassifierBase.cssrc/Helpers/StatisticsHelper.cssrc/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cstests/AiDotNet.Tests/UnitTests/Statistics/ClopperPearsonTests.cstests/AiDotNet.Tests/AdversarialRobustness/SafetyFilterTests.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
tests/AiDotNet.Tests/AdversarialRobustness/IntervalBoundPropagationTests.cstests/AiDotNet.Tests/UnitTests/AdversarialRobustness/RLHFAlignmentTests.cstests/AiDotNet.Tests/UnitTests/AdversarialRobustness/AdversarialAttackTests.cstests/AiDotNet.Tests/UnitTests/AdversarialRobustness/AdversarialTrainingTests.cstests/AiDotNet.Tests/UnitTests/AdversarialRobustness/RandomizedSmoothingTests.cstests/AiDotNet.Tests/AdversarialRobustness/ContentClassifierTests.cssrc/AdversarialRobustness/Safety/IContentClassifier.cssrc/AdversarialRobustness/Safety/SafetyFilter.cssrc/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cstests/AiDotNet.Tests/AdversarialRobustness/CROWNVerificationTests.cssrc/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cstests/AiDotNet.Tests/UnitTests/AdversarialRobustness/ModelCardTests.cssrc/AdversarialRobustness/Safety/RuleBasedContentClassifier.cssrc/AdversarialRobustness/Safety/ContentClassifierBase.cssrc/Helpers/StatisticsHelper.cssrc/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cstests/AiDotNet.Tests/UnitTests/Statistics/ClopperPearsonTests.cstests/AiDotNet.Tests/AdversarialRobustness/SafetyFilterTests.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL Analysis
- GitHub Check: Build (Windows)
🔇 Additional comments (82)
tests/AiDotNet.Tests/UnitTests/AdversarialRobustness/RLHFAlignmentTests.cs (1)
1-1097: Excellent test coverage and structure!This test file demonstrates high-quality test engineering:
- Comprehensive coverage: Tests span constructors, alignment workflows, evaluation, constitutional AI, red teaming, serialization, and persistence
- Proper resource management: File I/O tests correctly use try-finally blocks for cleanup (lines 697-703, 718-724)
- Thorough edge case handling: Null checks, empty data, boundary conditions, and validation scenarios are all covered
- Well-organized structure: 15 logical test regions with clear naming conventions make the suite maintainable
- Appropriate test doubles: Mock models effectively simulate different vulnerability scenarios for red teaming tests
The test suite provides strong confidence in the RLHF alignment implementation.
tests/AiDotNet.Tests/UnitTests/AdversarialRobustness/ModelCardTests.cs (6)
14-67: LGTM: Constructor tests are thorough.The constructor tests comprehensively verify default values, UTC date initialization within a valid range, and proper initialization of all collection properties. The test structure is clear and assertions are appropriate.
464-577: LGTM: File I/O tests follow best practices.The SaveToFile tests properly validate input arguments, verify file operations, and include appropriate cleanup logic with try-finally blocks to prevent test pollution. The use of
Guid.NewGuid()for unique filenames avoids conflicts in parallel test execution.
669-679: Verify that the hardcoded recommendation count remains accurate.The test expects exactly 4 standard recommendations. If the ModelCard implementation changes to add or remove default recommendations, this test will fail. This is intentional for catching such changes, but be aware that it's a brittle assertion.
Consider adding a comment explaining why 4 is expected, or extract it as a named constant if this value is also used in the implementation.
702-738: LGTM: Defensive copy tests are excellent.These tests properly verify that the factory method creates defensive copies of the metrics dictionaries, preventing external mutation. This is an important security and correctness guarantee.
766-855: LGTM: Integration tests cover realistic workflows.These integration tests verify end-to-end scenarios including the full create-populate-save-load workflow and complex multi-dataset/multi-group rendering. The tests are well-structured with proper cleanup.
861-908: LGTM: Helper method provides comprehensive test fixture.The
CreateFullyPopulatedModelCardhelper method creates a realistic, fully-populated ModelCard for testing. This promotes DRY principles and makes tests more maintainable.tests/AiDotNet.Tests/UnitTests/AdversarialRobustness/AdversarialTrainingTests.cs (9)
16-55: LGTM! Mock model is well-suited for testing defense orchestration.The mock implementation correctly provides all required
IPredictiveModelmethods. The prediction logic is simple and deterministic, which is appropriate for testing the defense mechanism's orchestration logic rather than actual model behavior.
57-91: LGTM! Constructor tests provide complete coverage.All constructor scenarios are validated: custom options, null argument handling, and default values. The assertions correctly verify option propagation.
164-253: LGTM! Preprocessing tests validate all methods and fallback behavior.The tests comprehensively cover preprocessing variations (JPEG, bit-depth reduction, denoising), disabled preprocessing, and unknown method fallback. Range validation ensures outputs remain in valid bounds.
255-374: LGTM! Robustness evaluation tests are thorough and well-structured.The tests validate all error conditions (null arguments, mismatched dimensions) and verify that computed metrics (CleanAccuracy, AdversarialAccuracy, AttackSuccessRate, RobustnessScore, AveragePerturbationSize) fall within expected ranges. Integration with
FGSMAttackprovides realistic attack simulation.
376-451: LGTM! Serialization tests demonstrate excellent resource management.The tests validate complete serialization/deserialization round-trips with proper option restoration. The file I/O tests include proper cleanup with try-finally blocks, ensuring temporary files are deleted even if tests fail.
453-465: LGTM! Reset test validates safe operation.The test correctly uses
Record.Exceptionto verify thatResetcompletes without throwing, which is appropriate for validating a potentially no-op reset operation.
467-487: LGTM! GetOptions test validates correct option retrieval.The test confirms that configured options are correctly returned by
GetOptions, validating multiple properties to ensure complete option preservation.
489-596: LGTM! Defended model tests ensure correct delegation.These tests validate that the preprocessing wrapper correctly delegates all
IPredictiveModelmethods (metadata, serialization, persistence) to the underlying model. This ensures the wrapper maintains the full model interface contract. File I/O tests include proper cleanup.
617-652: LGTM! Clipping tests validate robust handling of out-of-range inputs.These tests ensure that both JPEG and bit-depth reduction preprocessing methods correctly clip values to the valid [0.0, 1.0] range when given out-of-bounds inputs. This is important for preventing downstream errors from invalid data.
src/AdversarialRobustness/Safety/IContentClassifier.cs (1)
19-53: Well-designed interface with comprehensive documentation.The interface establishes a clean contract for content classifiers with appropriate methods for single, text, and batch classification. The XML documentation is thorough and beginner-friendly.
src/AdversarialRobustness/Safety/ContentClassifierBase.cs (2)
54-69: Good defensive handling for null/empty text.The method correctly returns a safe default result for null or empty input before attempting classification.
115-150: Reasonable fallback implementation for text-to-vector conversion.The character frequency approach is documented as a baseline for subclasses to override. Note that non-ASCII characters (code points > 255) will all map to index 255, which may affect classification of multilingual content.
tests/AiDotNet.Tests/AdversarialRobustness/ContentClassifierTests.cs (2)
323-346: Good use of try-finally for test cleanup.The
SaveAndLoadModel_PreservesStatetest properly uses try-finally to ensure the temp file is cleaned up even if the test fails. This is a good testing practice.
1-473: Comprehensive test coverage for the content classifier.The test suite covers key scenarios including constructor variants, classification of different content categories, pattern management, serialization/deserialization, and edge cases. The tests are well-organized using regions.
src/AdversarialRobustness/Safety/RuleBasedContentClassifier.cs (3)
269-317: Default patterns provide reasonable baseline detection.The regex patterns cover common categories with word boundaries for precision. The ReDoS protection via timeout is a good security practice. Consider:
- The credit card pattern may have false positives with other 16-digit sequences
- These patterns are intentionally basic; documentation correctly notes ML classifiers would be more sophisticated
212-250: Classification logic is sound with appropriate timeout handling.The scoring approach (proportion of matched patterns) is straightforward and documented. The per-pattern timeout prevents ReDoS attacks. Silently catching
RegexMatchTimeoutExceptionis acceptable for this use case.
122-128: Security concern:TypeNameHandling.Autoenables deserialization attacks—useTypeNameHandling.Noneinstead.TypeNameHandling.Auto allows the message payload to control the deserialization target type, introducing a significant security risk. For
SerializationData, which contains no polymorphic types,TypeNameHandling.None(the default) is appropriate and eliminates this attack surface entirely.🔎 Recommended fix
var settings = new JsonSerializerSettings { - TypeNameHandling = TypeNameHandling.Auto, - SerializationBinder = new SafeSerializationBinder() + TypeNameHandling = TypeNameHandling.None };src/AdversarialRobustness/Safety/SafetyFilter.cs (4)
55-88: LGTM!The pattern initialization logic correctly handles reinitialization from both constructor and deserialization paths, addressing the previously flagged state restoration issue.
430-451: LGTM!The deserialization now properly restores pattern state by calling
InitializePatterns(), addressing the previously flagged state restoration issue. The use ofSafeSerializationBinderprovides good protection against deserialization attacks.
511-552: LGTM!The sanitization logic now performs actual content modification by zeroing out harmful locations or applying global dampening, resolving the previously flagged no-op issue. The implementation correctly distinguishes between specific findings and general harmful content detection.
554-575: LGTM!The log file path handling now correctly uses the configured
LogFilePathoption with a sensible fallback, addressing the previous concern about hardcoded paths. Exception handling appropriately fails silently for logging errors.tests/AiDotNet.Tests/AdversarialRobustness/SafetyFilterTests.cs (7)
17-40: LGTM!Constructor tests provide good coverage of valid instantiation and null argument validation.
44-180: LGTM!Input validation tests comprehensively cover null handling, invalid values (NaN, infinities), length constraints, and feature toggle behavior with appropriate assertions.
183-238: LGTM!Output filtering tests correctly verify safe output handling, modification tracking, and feature toggle behavior. The absence of harmful-content-triggered filtering tests is appropriate given that numeric vectors won't match the text-based patterns.
241-343: LGTM!Jailbreak detection and harmful content identification tests appropriately verify that clean and empty inputs produce no false positives. The test design correctly reflects the text-pattern-based detection approach.
346-407: LGTM!Safety score computation tests thoroughly verify boundary constraints, expected high scores for clean inputs, and score degradation for invalid inputs with clear assertions.
452-617: LGTM!Serialization and model persistence tests provide thorough coverage of round-trip integrity, argument validation, error conditions, and proper resource cleanup. The tests correctly verify that options are preserved across serialization boundaries.
620-706: LGTM!Edge case and type-specific tests appropriately verify handling of multiple simultaneous validation issues, empty inputs, and float type support, ensuring robust generic behavior.
tests/AiDotNet.Tests/UnitTests/AdversarialRobustness/AdversarialAttackTests.cs (7)
1-8: LGTM!Namespace and using directives are correctly structured. Based on learnings, global usings cover
Vector<T>and other tensor types, so explicit imports are appropriate here for non-global namespaces.
20-53: LGTM!The mock model is well-designed for testing - it produces deterministic outputs based on input sums with a bias toward class 0, making test assertions predictable.
57-181: LGTM!Comprehensive FGSM attack tests covering constructor validation, null checks, adversarial generation, bounds checking, perturbation constraints, and targeted attacks. Good test coverage.
183-301: LGTM!PGD attack tests are well-structured, including verification that different random seeds produce different adversarial examples - important for testing stochastic behavior.
303-430: LGTM!C&W and AutoAttack tests appropriately reduce iteration counts for faster test execution while still validating core functionality.
432-543: LGTM!Batch processing and perturbation calculation tests provide thorough coverage of error handling for null inputs, mismatched dimensions, and correct mathematical operations.
545-728: LGTM!Serialization tests properly verify round-trip preservation of options, and edge case tests cover important boundary conditions (zero epsilon, large epsilon, single-dimension inputs). The temp file cleanup in finally blocks is good practice.
tests/AiDotNet.Tests/AdversarialRobustness/CROWNVerificationTests.cs (5)
1-16: LGTM!Well-structured test class with comprehensive XML documentation explaining the CROWN verification method.
138-161: Verify CROWN vs IBP bound comparison assumption.This test asserts CROWN produces tighter bounds than IBP. While mathematically correct in general, the assertion may be sensitive to mock model behavior. Consider adding a tolerance margin or documenting that this property holds for the specific mock model used.
186-406: LGTM!Batch, radius, and accuracy tests are well-structured with proper null checks, dimension validation, and important invariant assertions (e.g., certified accuracy ≤ clean accuracy).
409-556: LGTM!Serialization and persistence tests are comprehensive, properly preserving all options through round-trips and cleaning up temp files in finally blocks.
583-773: LGTM!CROWN-specific tests properly verify determinism with fixed seeds and the mathematical property that larger epsilon produces wider bounds. Mock models are appropriately designed for different test scenarios.
tests/AiDotNet.Tests/UnitTests/AdversarialRobustness/RandomizedSmoothingTests.cs (5)
1-58: LGTM!The mock predictive model is well-designed with configurable dominant class and probability, enabling deterministic testing of certification behavior.
88-112: LGTM!Reproducibility test with fixed random seed is crucial for randomized smoothing - verifies that the same seed produces identical certification results.
142-243: LGTM!CertifyPrediction tests comprehensively cover strong model behavior, certification details verification, bound invariants, and parameterized testing with different sigma values.
247-455: LGTM!Batch, radius, and accuracy tests are well-structured with proper null checks and important invariant assertions. The tolerance in the certified vs clean accuracy comparison (line 451) accounts for floating-point precision.
457-667: LGTM!Serialization tests verify option preservation, median calculation tests cover both odd and even sample counts, and the integration test exercises the full workflow including serialization round-trip.
tests/AiDotNet.Tests/AdversarialRobustness/IntervalBoundPropagationTests.cs (4)
1-62: LGTM!Constructor tests properly verify default options with "IBP" certification method and null option handling.
64-155: LGTM!CertifyPrediction tests cover null checks, valid certification, small epsilon certification, and bound ordering invariants.
157-380: LGTM!Tests properly verify that models with larger margins produce larger certified radii, and the invariant that certified accuracy cannot exceed clean accuracy is correctly asserted.
382-682: LGTM!Serialization, persistence, reset, and float type tests are comprehensive. Mock models are well-designed with configurable margins for testing different scenarios.
src/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cs (8)
1-56: LGTM!Well-documented class with comprehensive XML documentation explaining IBP's mathematical foundation. Constructors properly handle null options.
68-140: LGTM!CertifyPrediction correctly computes bounds, checks certification, and returns a complete CertifiedPrediction result. CertifyBatch processes inputs sequentially, which is straightforward and correct.
164-230: LGTM!EvaluateCertifiedAccuracy correctly computes metrics with proper edge case handling (Math.Max prevents division by zero when no predictions are correct).
247-331: LGTM!Serialization uses SafeSerializationBinder for secure deserialization (good security practice). SaveModel properly creates directories if needed and LoadModel validates file existence.
333-407: LGTM!ComputeOutputBounds elegantly handles both layer-aware models (via
INeuralNetworkModel<T>) and falls back to sampling-based approximation for black-box models. PropagateIntervalBounds correctly iterates through layers.
409-477: LGTM!PropagateLinearLayer correctly implements the IBP formula for linear layers: positive weights contribute
w·lowerto lower bound andw·upperto upper bound, while negative weights reverse this. The weight access pattern assumes row-major storage.
589-732: LGTM!ApproximateBoundsWithSampling provides a reasonable fallback for black-box models with proper seeded randomness. The binary search for certified radius uses 20 iterations, providing good precision within the [0, 1] range.
734-776: LGTM!ComputeConfidence correctly uses sigmoid transformation to map the margin between predicted class and runner-up to a probability-like confidence value in [0, 1]. SerializationData is appropriately structured.
src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs (6)
42-67: LGTM!Constructors correctly initialize options with appropriate defaults and enforce CROWN-specific settings. Null validation is properly implemented.
69-163: LGTM!The certification API is well-structured with proper input validation. The flow of computing bounds, checking certification, and computing radius follows the expected CROWN verification pattern.
935-982: LGTM!The sampling-based fallback correctly approximates bounds when layer architecture is unavailable, with proper seeding for reproducibility.
753-797: LGTM!The ReLU CROWN relaxation correctly implements all three cases (always active, always inactive, crossing zero) with proper slope and intercept calculations matching the standard CROWN formulation.
984-1085: LGTM!Utility methods are correctly implemented: argmax for class prediction, certification check using bound comparison, binary search for radius, and margin-based confidence. The
SerializationDataclass is appropriately minimal.
275-281: VerifySafeSerializationBinderrestricts types adequately.Using
TypeNameHandling.Autowith untrusted input can lead to deserialization vulnerabilities. TheSafeSerializationBindershould whitelist only expected types. Confirm it properly restricts deserialized types to prevent type confusion attacks.#!/bin/bash # Verify SafeSerializationBinder implementation restricts types appropriately fd -t f "SafeSerializationBinder.cs" --exec cat {}src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (6)
48-57: LGTM!The constructor properly handles configurable seeding for reproducibility while defaulting to non-deterministic random generation for statistical validity. This addresses the previous review concern about fixed seeding.
59-126: LGTM!The certification logic correctly implements Cohen et al.'s randomized smoothing approach: sampling with Gaussian noise, using the lower confidence bound for certified radius computation, and properly populating certification details. Previous review decisions regarding loop style have been preserved.
216-234: LGTM!The median calculation correctly handles both even and odd-sized lists by averaging the two middle elements when the count is even. This addresses the previous review concern.
252-274: LGTM!The serialization now properly persists and restores the options state, addressing the previous review concern about
Deserializebeing a no-op. The null-coalescing fallback provides safe defaults.
276-294: LGTM!The Box-Muller transform correctly uses
1.0 - random.NextDouble()to ensure the argument toMath.Logis in the range (0, 1], avoiding theMath.Log(0)issue. Clipping to [0, 1] ensures valid input ranges are preserved.
336-356: LGTM!Both utility methods are correctly implemented:
ArgMaxefficiently finds the index of the maximum element, andClip01properly clamps values to [0, 1] using the centralizedMathHelper.Clamp.src/Helpers/StatisticsHelper.cs (1)
1054-1079: Regularized incomplete beta + continued fraction implementation is soundThe refactored
RegularizedIncompleteBetaFunctionwith the symmetry switch andBetaIncompleteContinuedFractionmatches the standard Numerical Recipes / DLMF formulation (including the(a+1)/(a+b+2)switch and Lentz CF updates). Boundary handling forx≤0/x≥1and clamping the final result into[0,1]are appropriate. I don’t see correctness issues here; this should significantly improve convergence and stability versus the older path.If you haven’t already, it’s worth cross‑checking a few values against a reference library (e.g., SciPy’s
betainc) for extremea,b,x(very small/large shapes, x near 0/1) to confirm numerical behavior.Also applies to: 1085-1157
tests/AiDotNet.Tests/UnitTests/Statistics/ClopperPearsonTests.cs (5)
1-18: LGTM!Good use of XML documentation to explain reference sources (scipy.stats.beta and R's binom.test()). The tolerance constants are well-defined for floating-point comparisons.
20-95: LGTM!Comprehensive Beta CDF test coverage including:
- Known values with boundary cases (x=0, x=1) and symmetry verification
- Invalid parameter handling for both alpha and beta
- Range validation for x values
- Result bounds verification
The inline documentation about the symmetry property
I_x(a,b) + I_{1-x}(b,a) = 1at line 28 is a nice touch for mathematical verification.
147-330: LGTM!Excellent Clopper-Pearson interval test coverage:
- Known reference values with clear inline documentation
- Proper edge case handling for zero successes and all successes
- Point estimate containment verification (sanity check)
- Monotonicity property test (higher confidence → wider interval)
- Comprehensive input validation tests
The inline comments explaining the mathematical basis (e.g., "two-sided: alpha/2 = 0.025 each tail") are helpful for maintainability.
332-385: LGTM!The relationship test between two-sided and one-sided bounds (lines 353-383) is an excellent mathematical correctness verification. The comment explaining that "Interval(conf=0.95).Lower == LowerBound(conf=0.975)" is crucial for understanding the statistical relationship.
412-444: LGTM!The coverage property test appropriately verifies a necessary condition for correct Clopper-Pearson implementation. The documentation clearly states this is a "necessary (but not sufficient) condition" at line 437, which is honest about the test's limitations. A full coverage simulation would require Monte Carlo methods and would be more suitable for integration testing.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Helpers/StatisticsHelper.cs (1)
607-617: Chi‑square PDF formula is incorrect (wrong exponent and normalization), impacting CDF inversion and tests
ChiSquarePDFdoes not implement the standard chi‑square density:
- The exponent of
xends up asdf − 1(via(_halfDf * log(_halfX))and then anotherPower(_halfX, _halfDf − 1)), but the correct exponent isdf/2 − 1.- The normalization constant uses
sqrt(2)instead of2^{df/2}(or the equivalent(2 * Γ(df/2))in the(x/2)formulation).This yields the wrong PDF shape and scaling, which directly affects
InverseChiSquareCDF(viaChiSquarePDFas the derivative) and any downstream code relying on chi‑square p‑values or quantiles.Proposed fix for `ChiSquarePDF`
- private static T ChiSquarePDF(T x, int degreesOfFreedom) - { - T _halfDf = _numOps.Divide(_numOps.FromDouble(degreesOfFreedom), _numOps.FromDouble(2)); - T _halfX = _numOps.Divide(x, _numOps.FromDouble(2)); - T _numerator = _numOps.Multiply( - _numOps.Exp(_numOps.Subtract(_numOps.Multiply(_halfDf, _numOps.Log(_halfX)), _halfX)), - _numOps.Power(_halfX, _numOps.Subtract(_halfDf, _numOps.FromDouble(1))) - ); - T _denominator = _numOps.Multiply(_numOps.Sqrt(_numOps.FromDouble(2)), GammaFunction(_halfDf)); - return _numOps.Divide(_numerator, _denominator); - } + private static T ChiSquarePDF(T x, int degreesOfFreedom) + { + // Standard chi-square PDF: + // f(x; k) = x^{k/2 - 1} * exp(-x/2) / (2^{k/2} * Γ(k/2)), x > 0 + T halfDf = _numOps.Divide(_numOps.FromDouble(degreesOfFreedom), _numOps.FromDouble(2.0)); + T halfX = _numOps.Divide(x, _numOps.FromDouble(2.0)); + + // x^{k/2 - 1} * exp(-x/2) + T power = _numOps.Subtract(halfDf, _numOps.One); + T numerator = _numOps.Multiply( + _numOps.Power(x, power), + _numOps.Exp(_numOps.Negate(halfX)) + ); + + // 2^{k/2} * Γ(k/2) + T twoToHalfDf = _numOps.Power(_numOps.FromDouble(2.0), halfDf); + T denom = _numOps.Multiply(twoToHalfDf, Gamma(halfDf)); + + return _numOps.Divide(numerator, denom); + }If you prefer to avoid calling
Gammahere, you can substituteGammaFunction(halfDf)in place ofGamma(halfDf)while keeping the corrected exponent and2^{k/2}normalization.
🧹 Nitpick comments (14)
src/AdversarialRobustness/Attacks/FGSMAttack.cs (1)
97-254: Consider consolidating duplicated gradient and loss helpers into the base class.The methods
ComputeGradient,ComputeAnalyticGradient,ComputeFiniteDifferenceGradient,ComputeLoss, andSoftmaxare nearly identical in bothFGSMAttack.csandPGDAttack.cs. Moving these toAdversarialAttackBase<T>as protected virtual methods would reduce duplication, improve maintainability, and allow derived classes to override if needed.🔎 Refactoring approach
Move the following methods from both attack classes to
AdversarialAttackBase<T>:
ComputeGradient(lines 97-110)ComputeAnalyticGradient(lines 125-150)ComputeFiniteDifferenceGradient(lines 155-187)ComputeLoss(lines 196-209)Softmax(lines 214-254)Mark them as
protected virtualto allow customization by derived classes if needed.src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (1)
140-143: Directory traversal protection could be strengthened, though current approach catches common cases.The current check for
".."in the original path before normalization catches literal directory traversal sequences and has explicit comments explaining the approach. However, it may have false positives (legitimate filenames containing "..") and won't catch all traversal techniques.If stronger protection is needed, consider validating that the resolved path is within an allowed base directory:
var allowedBase = Path.GetFullPath("./models"); // configure as needed var fullPath = Path.GetFullPath(filePath); if (!fullPath.StartsWith(allowedBase + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase)) { throw new ArgumentException("File path must be within the allowed directory.", nameof(filePath)); }Otherwise, the current approach is reasonable for typical usage.
Also applies to: 168-171
.github/workflows/commitlint-fix.yml (1)
53-57: Early exit doesn't skip subsequent steps.Using
exit 0makes this step succeed, but subsequent steps in the job will still execute. Since line 62 referencessteps.get-commits.outputs.commitswithout checking if it's empty, steps may process empty data.Consider setting an output flag and using step conditionals:
🔎 Proposed improvement
if [ "$COMMITS_COUNT" -eq 0 ]; then echo "No commits to check, skipping" + echo "skip_remaining=true" >> $GITHUB_OUTPUT exit 0 fiThen add to line 59:
- name: Check each commit for compliance id: check-commits + if: steps.get-commits.outputs.skip_remaining != 'true'src/AdversarialRobustness/Safety/SafetyFilter.cs (1)
517-581: Minor robustness/perf nits inSanitizeOutputandLogFilteredContent.Functionality looks sound; a couple of small cleanups you may consider:
SanitizeOutputre-fetches numeric ops viaMathHelper.GetNumericOperations<T>()even though a staticNumOpsalready exists; using the static would avoid redundant lookups.LogFilteredContentassumesresult.DetectedHarmCategoriesis non-null. It’s currently only called whenIsSafe == false, which your pipeline guarantees, but a simple null-coalesce (e.g.,result.DetectedHarmCategories ?? Array.Empty<string>()) would make it safer against future call-site changes.src/Models/Results/PredictionModelResult.cs (1)
1405-1483: Matrix safety helpers are correct but can be tightened slightly.The new
ValidateAndSanitizeMatrix/FilterMatrixOutputimplementations look logically correct:
- Per-row validation throws with clear diagnostics when a row fails safety checks.
- Sanitization and filtering enforce consistent column counts.
Two minor polish suggestions:
- The early
if (SafetyFilter == null)checks in both methods are redundant given the only callers already guard onSafetyFilter != null; you can drop them to reduce branching.- Both paths always allocate a new
Matrix<T>once any row is sanitized/filtered. If this becomes hot, you might consider short‑circuiting when allFilteredOutputrows are identical to inputs (e.g., via aWasModifiedaggregation) to avoid unnecessary allocation/copy, though this is optional and can be deferred until profiling shows a need.src/PredictionModelBuilder.cs (1)
143-143: Field addition for safety filter configuration looks correctStoring
SafetyFilterConfiguration<T>?in a private field aligns with the existing builder state pattern and enables consistent propagation into result options. Minor nit: sinceAiDotNet.Models.Optionsis already imported globally, you could shorten the type name toSafetyFilterConfiguration<T>for consistency with nearby fields. Based on learnings, global usings cover this namespace.src/AdversarialRobustness/Safety/RuleBasedContentClassifier.cs (1)
233-271: Consider removing redundantRegexOptions.IgnoreCase.Line 235 converts the text to lowercase with
ToLowerInvariant(), making theRegexOptions.IgnoreCaseflag on line 254 redundant. While this doesn't affect correctness, removing it would be a minor optimization.Otherwise, the implementation is solid:
- Proper timeout handling for ReDoS protection
- Clear scoring logic (proportion of matched patterns)
- Exception handling for regex timeouts
🔎 Optional refactor
- if (Regex.IsMatch(text, pattern, RegexOptions.IgnoreCase, RegexTimeout)) + if (Regex.IsMatch(text, pattern, RegexOptions.None, RegexTimeout))src/AdversarialRobustness/Safety/ContentClassifierBase.cs (1)
157-209: Consider documenting the threshold relationship.The method uses
DetectionThresholdfor determiningIsHarmful(line 182), but uses hardcoded values (0.8, 0.5) forRecommendedAction(lines 195-206). This design allows advisory actions even when content isn't definitively harmful, which may be intentional. However, the relationship between these thresholds isn't documented and could be clarified.Example: If
DetectionThreshold = 0.7and a category scores 0.6:
IsHarmful = false(0.6 ≤ 0.7)RecommendedAction = "Warn"(0.5 < 0.6 ≤ 0.8)💡 Suggested documentation improvement
Add to the method's XML documentation:
/// <summary> /// Creates a classification result from category scores. /// </summary> +/// <remarks> +/// The method uses two sets of thresholds: +/// <list type="bullet"> +/// <item><description>DetectionThreshold: determines IsHarmful and DetectedCategories</description></item> +/// <item><description>Fixed thresholds (0.8=Block, 0.5=Warn): determine RecommendedAction</description></item> +/// </list> +/// This allows advisory actions even when content isn't definitively harmful. +/// </remarks> /// <param name="categoryScores">Dictionary of category names to scores.</param> /// <returns>Formatted classification result.</returns>src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs (3)
555-574: Simplify bound selection logic.The current code uses empty
ifbranches with comments to select the tighter of CROWN and IBP bounds. While correct, this pattern is harder to read and maintain.🔎 Clearer implementation using max/min selection
// Take the tighter of CROWN and IBP bounds for (int i = 0; i < outputDim; i++) { - if (NumOps.GreaterThan(outputLower[i], ibpLower[i])) - { - // CROWN lower is tighter - } - else - { - outputLower[i] = ibpLower[i]; - } - - if (NumOps.LessThan(outputUpper[i], ibpUpper[i])) - { - // CROWN upper is tighter - } - else - { - outputUpper[i] = ibpUpper[i]; - } + // For lower bound: take the maximum (tighter bound) + if (NumOps.LessThan(outputLower[i], ibpLower[i])) + { + outputLower[i] = ibpLower[i]; + } + + // For upper bound: take the minimum (tighter bound) + if (NumOps.GreaterThan(outputUpper[i], ibpUpper[i])) + { + outputUpper[i] = ibpUpper[i]; + } }
911-958: Consider validating NumSamples in sampling fallback.The method uses
_options.NumSamples(line 928) without verifying it's positive. IfNumSamples <= 0, the loop doesn't execute and the method returns bounds initialized to the clean output. While this might be acceptable fallback behavior, explicit validation or documentation would clarify intent.🔎 Add validation
private (Vector<T> lower, Vector<T> upper) ApproximateBoundsWithSampling( Vector<T> input, IPredictiveModel<T, Vector<T>, Vector<T>> model, T epsilon) { + if (_options.NumSamples <= 0) + { + throw new ArgumentException("NumSamples must be positive for sampling-based approximation."); + } + var cleanOutput = model.Predict(input);
994-1022: Consider making radius search parameters configurable.The binary search for certified radius uses hard-coded
maxRadius = 1.0(line 1000) andmaxIterations = 20(line 1001). These values may not suit all use cases. Consider adding these as configurable options inCertifiedDefenseOptions<T>for greater flexibility.src/Helpers/StatisticsHelper.cs (1)
1007-1038: LanczosLogGammaimplementation looks correct; consider hoisting coefficients to avoid per‑call allocationThe Numerical Recipes–style Lanczos implementation is sound and integrates cleanly with the rest of the helpers, and casting through
doubleis consistent with how other special functions here are implemented.The only concern is performance:
double[] coefficientsis allocated on every call, andLogGammais used in many hot paths (LogBeta,Gamma, incomplete gamma/beta, etc.). Moving the coefficient array to aprivate static readonly double[]field (orReadOnlySpan<double>viaMemoryMarshal) would avoid repeated allocations on tight loops.Example refactor (outline)
- private static T LogGamma(T x) - { - // Lanczos approximation for log(Gamma(x)) - // Based on Numerical Recipes implementation with g=5 - // For x > 0, Gamma(x) ≈ ... - - double[] coefficients = { - 76.18009172947146, - -86.50532032941677, - 24.01409824083091, - -1.231739572450155, - 0.001208650973866179, - -0.000005395239384953 - }; + private static readonly double[] _logGammaCoefficients = + { + 76.18009172947146, + -86.50532032941677, + 24.01409824083091, + -1.231739572450155, + 0.001208650973866179, + -0.000005395239384953 + }; + + private static T LogGamma(T x) + { + // Lanczos approximation for log(Gamma(x)) with g = 5 + // For x > 0, Gamma(x) ≈ ... - double xd = _numOps.ToDouble(x); + double xd = _numOps.ToDouble(x); double sum = 1.000000000190015; - for (int i = 0; i < 6; i++) + for (int i = 0; i < _logGammaCoefficients.Length; i++) { - sum += coefficients[i] / (xd + i + 1); + sum += _logGammaCoefficients[i] / (xd + i + 1); } ...src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (2)
48-57: Consider a higher-entropy seed source for the non-deterministic path.
Environment.TickCountwraps every ~49 days and can be somewhat predictable. For better statistical independence across runs, consider usingRandom.Shared(if targeting .NET 6+) or a Guid-based seed.🔎 Alternative seed sources
If targeting .NET 6+, you can use
Random.Shareddirectly:public RandomizedSmoothing(CertifiedDefenseOptions<T> options) { this.options = options ?? throw new ArgumentNullException(nameof(options)); - // Use the configured seed if provided, otherwise use non-deterministic random - // for proper statistical validity of the certification - this.random = options.RandomSeed.HasValue - ? RandomHelper.CreateSeededRandom(options.RandomSeed.Value) - : RandomHelper.CreateSeededRandom(Environment.TickCount); + // Use the configured seed if provided, otherwise use a Guid-based seed for better entropy + this.random = options.RandomSeed.HasValue + ? RandomHelper.CreateSeededRandom(options.RandomSeed.Value) + : RandomHelper.CreateSeededRandom(Guid.NewGuid().GetHashCode()); }Or consider storing
Random.Shareddirectly when no seed is specified for even better performance and thread safety in .NET 6+.
311-327: Avoid precision loss by passing the actual count instead of recomputing it.Lines 315 and 324 compute
successesasMath.Round(pA * n), which reintroduces the original count from a proportion. SincepAwas originally computed astopCount / numSamplesinCertifyPrediction, this roundtrip through floating-point can introduce precision errors in edge cases.🔎 Recommended refactor to pass counts directly
Modify the signature to accept the actual count:
- private double ComputeLowerBound(double pA, int n, double confidence) + private double ComputeLowerBound(int successes, int n, double confidence) { - // Use exact Clopper-Pearson confidence interval via Beta distribution - // This provides guaranteed coverage, unlike the normal approximation - int successes = (int)Math.Round(pA * n); var interval = StatisticsHelper<T>.CalculateClopperPearsonInterval(successes, n, NumOps.FromDouble(confidence)); return NumOps.ToDouble(interval.Lower); } - private double ComputeUpperBound(double pA, int n, double confidence) + private double ComputeUpperBound(int successes, int n, double confidence) { - // Use exact Clopper-Pearson confidence interval via Beta distribution - // This provides guaranteed coverage, unlike the normal approximation - int successes = (int)Math.Round(pA * n); var interval = StatisticsHelper<T>.CalculateClopperPearsonInterval(successes, n, NumOps.FromDouble(confidence)); return NumOps.ToDouble(interval.Upper); }Then update the call site in
CertifyPrediction(lines 107-108):- var pA_lower = ComputeLowerBound(pA, options.NumSamples, options.ConfidenceLevel); - var pA_upper = ComputeUpperBound(pA, options.NumSamples, options.ConfidenceLevel); + var pA_lower = ComputeLowerBound(topCount, options.NumSamples, options.ConfidenceLevel); + var pA_upper = ComputeUpperBound(topCount, options.NumSamples, options.ConfidenceLevel);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/commitlint-fix.ymlsrc/AdversarialRobustness/Attacks/AdversarialAttackBase.cssrc/AdversarialRobustness/Attacks/FGSMAttack.cssrc/AdversarialRobustness/Attacks/PGDAttack.cssrc/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cssrc/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cssrc/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cssrc/AdversarialRobustness/Safety/ContentClassifierBase.cssrc/AdversarialRobustness/Safety/RuleBasedContentClassifier.cssrc/AdversarialRobustness/Safety/SafetyFilter.cssrc/Helpers/StatisticsHelper.cssrc/Interfaces/IPredictionModelBuilder.cssrc/Models/Options/PredictionModelResultOptions.cssrc/Models/Results/PredictionModelResult.cssrc/PredictionModelBuilder.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Interfaces/IPredictionModelBuilder.cs
- src/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/Models/Options/PredictionModelResultOptions.cssrc/Models/Results/PredictionModelResult.cssrc/AdversarialRobustness/Safety/RuleBasedContentClassifier.cssrc/PredictionModelBuilder.cssrc/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cssrc/AdversarialRobustness/Attacks/FGSMAttack.cssrc/AdversarialRobustness/Attacks/PGDAttack.cssrc/AdversarialRobustness/Attacks/AdversarialAttackBase.cssrc/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cssrc/Helpers/StatisticsHelper.cssrc/AdversarialRobustness/Safety/SafetyFilter.cssrc/AdversarialRobustness/Safety/ContentClassifierBase.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/Models/Options/PredictionModelResultOptions.cssrc/Models/Results/PredictionModelResult.cssrc/AdversarialRobustness/Safety/RuleBasedContentClassifier.cssrc/PredictionModelBuilder.cssrc/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cssrc/AdversarialRobustness/Attacks/FGSMAttack.cssrc/AdversarialRobustness/Attacks/PGDAttack.cssrc/AdversarialRobustness/Attacks/AdversarialAttackBase.cssrc/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cssrc/Helpers/StatisticsHelper.cssrc/AdversarialRobustness/Safety/SafetyFilter.cssrc/AdversarialRobustness/Safety/ContentClassifierBase.cs
📚 Learning: 2025-12-19T19:05:13.598Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 445
File: src/Interfaces/IPredictionModelBuilder.cs:7-8
Timestamp: 2025-12-19T19:05:13.598Z
Learning: In ooples/AiDotNet, IAggregationStrategy<TModel> is declared in AiDotNet.Interfaces (src/Interfaces/IAggregationStrategy.cs). For src/Interfaces/IPredictionModelBuilder.cs (also in AiDotNet.Interfaces), referencing IAggregationStrategy does not require an additional using. Avoid flagging missing using directives for this case in future reviews.
Applied to files:
src/Models/Results/PredictionModelResult.cs
📚 Learning: 2025-12-21T04:19:31.255Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 445
File: src/PredictionModelBuilder.cs:752-763
Timestamp: 2025-12-21T04:19:31.255Z
Learning: Federated preprocessing invariant: In PredictionModelBuilder.BuildSupervisedInternalAsync (src/PredictionModelBuilder.cs), when using IFederatedClientDataLoader, preprocessing must preserve per-client row ordering and total sample counts. The code should enforce X/Y alignment and total count checks after PreprocessData whenever federated client ranges are active, and fail fast with a clear exception if violated. This guideline is specific to this file and to similar federated preprocessing paths; apply it to other implementations that use IFederatedClientDataLoader and related BuildSupervisedInternalAsync patterns.
Applied to files:
src/PredictionModelBuilder.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build (Windows)
🔇 Additional comments (46)
src/AdversarialRobustness/Attacks/FGSMAttack.cs (1)
52-83: LGTM! FGSM attack logic is correct.The implementation correctly applies the FGSM formula with proper handling of targeted vs. untargeted attacks, gradient computation dispatch (analytic or finite-difference), and valid range clipping.
src/AdversarialRobustness/Attacks/PGDAttack.cs (2)
59-103: LGTM! PGD iterative attack is well-implemented.The implementation correctly performs iterative gradient steps with proper projection to the epsilon-ball, handles random initialization based on norm type, and applies targeted/untargeted logic appropriately.
125-151: LGTM! Random start now respects L2 norm constraint.The random starting point correctly projects the perturbation to the appropriate norm ball (L2 or L-infinity) before applying it, addressing the previous concern about L2 budget violations.
src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (4)
116-128: LGTM! Serialization/deserialization properly maintains state.The
Deserializemethod now correctly restoresOptionsfrom JSON and re-initializes theRandomfield with the deserialized seed, ensuring reproducible behavior afterLoadModel.
131-156: LGTM! File operations include proper validation and directory handling.The method validates non-null/empty paths, checks for directory traversal attempts, ensures parent directories exist, and handles file writing robustly.
159-182: LGTM! Model loading includes appropriate safety checks.The method validates the file path, checks for directory traversal, verifies file existence with clear error messages, and properly deserializes the model state.
184-256: LGTM! Helper methods for sign, norms, and projections are correctly implemented.The Sign, L-infinity norm, L2 norm, and projection helpers properly support both attack implementations with correct numeric operations and edge case handling.
.github/workflows/commitlint-fix.yml (11)
11-14: Concurrency control successfully added.This addresses the race condition issue from the previous review. The group key correctly serializes runs per PR, and
cancel-in-progress: trueensures only one run proceeds at a time.
43-44: Robust commit counting implemented.The fallback
|| echo "0"correctly handles empty input and prevents the step from failing when no commits are found.
72-76: Simplified merge commit detection.The generic pattern
^(Merge|Revert)appropriately skips merge commits. This is cleaner than the previous granular patterns.
80-109: Commit validation and case fixing logic is sound.The regex correctly validates conventional commit format including the
depstype, and the description extraction and lowercase fixing properly normalize subject lines. The removal of the special case for deps (from previous review) ensures consistent lowercase enforcement.
204-209: Safe force-push implementation.Using
--force-with-leaseprevents accidentally overwriting unexpected changes on the remote branch. The simplified authentication withGITHUB_TOKENis appropriate.
212-230: Clear and accurate PR comment.The updated comment correctly states that lowercase applies to all types including 'deps' (line 226), addressing the previous misleading exception note. The explanation is clear and helpful for contributors.
266-286: Dependabot commit validation is consistent.The logic correctly identifies deps commits with uppercase descriptions and normalizes them to lowercase, maintaining consistency with the main commit-fixing job.
304-305: Correct commit amend implementation.Using
git commit --amend --file=-with the new message is the standard approach for updating commit messages while preserving the commit.
310-311: Consistent and safe force-push.The dependabot job uses the same safe
--force-with-leaseapproach as the main job.
315-333: Helpful dependabot PR comment.Showing both the original and fixed commit messages provides clear transparency about the automated changes. The comment is well-structured and informative.
161-202: Multi-commit squashing causes history loss; single-commit preservation is good.The workflow correctly amends single-commit PRs directly, preserving history. However, multi-commit PRs are still squashed into a single consolidated commit (lines 186-201), losing individual commit messages, timestamps, and granular history. While the consolidation message documents this limitation, the destructive behavior remains unaddressed for multi-commit scenarios.
Consider documenting this trade-off in contributing guidelines or exploring safer alternatives like interactive rebase for multi-commit preservation.
src/Models/Options/PredictionModelResultOptions.cs (1)
556-569: Safety filter configuration surface looks consistent.The
SafetyFilterConfiguration<T>?option and its XML docs align with the constructor logic inPredictionModelResult(null → default-enabled filter,Enabled == false→ opt-out). API shape and placement in a dedicated “Safety & Robustness” section read well.src/PredictionModelBuilder.cs (2)
1687-1735: Safety filter configuration is now correctly propagated across all result pathsThreading
_safetyFilterConfigurationinto:
- main supervised
options(Line 1705),- meta-learning
metaOptions(Line 1845),- RL
rlOptions(Line 2157),keeps behavior consistent so a configured safety filter is visible regardless of training mode and fixes the previous RL omission noted in earlier reviews. No functional concerns here.
Also applies to: 1837-1866, 2150-2179
2330-2339: ConfigureSafetyFilter API is consistent with existing builder methodsThe new
ConfigureSafetyFiltermethod matches the fluent builder style (null-guard + field assignment +return this) and its XML docs clearly describe intent (“validate inputs and filter outputs during inference”). Signature and behavior look good and integrate cleanly with the new result-option wiring.src/AdversarialRobustness/Safety/RuleBasedContentClassifier.cs (6)
1-49: LGTM! Strong ReDoS protection and clear initialization.The
RegexTimeoutconstant (line 27) provides excellent protection against ReDoS attacks. The constructor properly initializes the pattern dictionary and sets up default patterns.
51-64: LGTM! Proper validation and initialization.The custom pattern constructor includes appropriate null checking and correctly synchronizes
SupportedCategorieswith the provided pattern dictionary.
66-98: LGTM! Vector classification limitation is properly documented.The extensive documentation (lines 67-77 and 85-88) clearly explains why vector classification is limited for regex-based pattern matching. Returning zero confidence appropriately signals that no meaningful detection occurred. This addresses the concern raised in previous reviews.
100-231: LGTM! Robust implementation with proper validation and security measures.Key strengths:
- Lines 133-158:
SafeSerializationBinderprovides protection against deserialization attacks- Lines 160-193: File I/O methods include defensive path validation and directory creation
- Line 227:
TryGetValuepattern is more efficient thanContainsKey+ indexer (addresses past review feedback)- Consistent input validation across all public methods
273-321: LGTM! Default patterns provide good baseline coverage.The default patterns are appropriate for a baseline/fallback classifier:
- Word boundaries (
\b) prevent false positives like "skill" matching "kill"- Categories cover key safety concerns
- PII patterns (lines 314-316) detect SSN, credit card, and password formats
Note: PII patterns are US-specific. For international deployments, these patterns can be customized via the constructor or
AddPatternmethod.
323-331: LGTM! Simple and appropriate serialization DTO.The nullable
CategoryPatternsproperty (line 329) provides defensive handling during deserialization.src/AdversarialRobustness/Safety/ContentClassifierBase.cs (4)
1-48: LGTM! Constructor properly avoids virtual calls.Line 47 uses the static
DefaultCategoriesfield instead of a virtual method, addressing the concern from previous reviews about virtual calls in constructors. The initialization is clean and safe.
50-104: LGTM! Well-designed abstract/virtual method structure.The virtual methods provide sensible defaults:
ClassifyText(lines 54-69) properly handles empty input and delegates to vector classificationClassifyBatch(lines 72-86) includes null validation and iterates cleanlyAbstract methods appropriately require subclass implementation for model-specific logic.
106-155: LGTM! Clean LINQ-based character frequency encoding.Lines 129-132 use
Select/GroupBy/ToDictionarypattern for character frequency counting, addressing previous feedback about preferring LINQ over foreach loops. The normalization logic (lines 140-152) is correct, and documentation appropriately notes this is a simple default that subclasses should override for production use.
211-229: LGTM! Static field avoids constructor virtual call anti-pattern.The static
DefaultCategoriesfield (lines 218-228) addresses the previous review concern about virtual calls in constructors. The documentation (lines 214-217) clearly explains the design rationale.src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs (5)
1-44: LGTM! Well-documented class structure.The class declaration, documentation, and namespace structure are solid. The comprehensive XML documentation with mathematical foundations and beginner-friendly explanations is excellent. The use of Newtonsoft.Json aligns with the PR's JSON tooling migration objectives.
46-67: LGTM! Constructors properly initialize and enforce CROWN configuration.Both constructors correctly initialize the options, with appropriate null validation in the parameterized constructor. The enforcement of
CertificationMethod = "CROWN"andUseTightBounds = trueensures consistent configuration regardless of input.
70-331: LGTM! Comprehensive public API with proper validation.All public methods implement appropriate input validation, null checks, and error handling. The serialization uses
SafeSerializationBinderfor security, and the file I/O methods handle directory creation and file existence checks defensively. The combined conditions (e.g., lines 206-210) reflect the fixes from previous reviews.
579-716: LGTM! Backward propagation implements CROWN correctly.The linear and activation backward propagation methods properly implement the CROWN algorithm. The ReLU-specific relaxation with slope and intercept computation aligns with the mathematical foundations described in the class documentation. The simplified handling of redundant branches reflects previous review fixes.
1046-1058: LGTM! Serialization data structure is complete.The
SerializationDataclass captures all necessary configuration options. The intentional exclusion ofCertificationMethodis appropriate since it's always set to "CROWN" during deserialization, ensuring consistency.src/Helpers/StatisticsHelper.cs (2)
1055-1158: Regularized incomplete beta and continued‑fraction implementation look numerically soundThe new
RegularizedIncompleteBetaFunctionandBetaIncompleteContinuedFractionimplementations follow standard practice:
- Guarding
x <= 0/x >= 1to return 0/1 avoidslog(0)/log(1-x)issues.- Using the symmetry relation
Iₓ(a,b) = 1 − I_{1−x}(b,a)based on(a+1)/(a+b+2)improves convergence in the upper tail.- Lentz’s algorithm in
BetaIncompleteContinuedFractionwithfpmin,epsilon, and iteration cap is implemented correctly and clamped to[0,1].Given that all callers (F/t/Beta helpers) enforce
a > 0,b > 0, and0 ≤ x ≤ 1, this is a good, robust upgrade.
1261-1350: Clopper–Pearson interval and one‑sided lower bound match standard Beta‑based definitionsThe new Clopper–Pearson helpers look correct and well‑guarded:
- Input validation for
successes,trials, andconfidence ∈ (0,1)is thorough.- Two‑sided interval:
- Lower:
Beta^{-1}(α/2; successes, trials − successes + 1)with0special‑cased forsuccesses == 0.- Upper:
Beta^{-1}(1 − α/2; successes + 1, trials − successes)with1special‑cased forsuccesses == trials.- One‑sided lower bound uses
Beta^{-1}(α; successes, trials − successes + 1)and returns0whensuccesses == 0.These match the canonical Clopper–Pearson construction and integrate cleanly with
CalculateInverseBetaCDF.src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs (8)
1-36: LGTM!The imports, namespace, and class structure are well-organized. The documentation is comprehensive and beginner-friendly. The use of
INumericOperations<T>for generic math is appropriate for this generic implementation.
60-126: LGTM!The certification logic correctly implements randomized smoothing per Cohen et al. (2019). The use of the lower confidence bound for radius computation ensures conservative certification guarantees. The explicit loop for finding the max class (lines 93-100) is appropriate given the developer's performance considerations.
129-150: LGTM!The batch processing and radius computation methods are straightforward wrappers with appropriate null checking.
153-230: LGTM!The evaluation metrics are computed correctly. The median calculation properly handles both even and odd list sizes (lines 224-226), averaging the two middle elements for even-sized lists as expected.
233-267: LGTM!The serialization and persistence methods correctly serialize/deserialize the options. The use of null-coalescing in
Deserialize(line 254) provides a safe fallback.
269-287: LGTM!The Box-Muller transform is correctly implemented with proper handling of edge cases. Line 278 correctly uses
1.0 - random.NextDouble()to ensureu1is in the range (0, 1], avoidingMath.Log(0).
289-309: LGTM!The certified radius computation correctly applies the formula σ × Φ⁻¹(pA) and delegates to
StatisticsHelperfor consistency across the codebase.
329-349: LGTM!The
ArgMaxhelper correctly finds the maximum element index, andClip01appropriately usesMathHelper.Clampfor range restriction.
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
♻️ Duplicate comments (7)
src/ModelRegistry/ModelRegistry.cs (1)
681-687: Path construction is properly secured.The
GetModelCardPathhelper usesPath.Combinebut immediately validates the result against the registry directory at line 685, mitigating the path traversal risk flagged by the security scanner.src/Helpers/StatisticsHelper.cs (1)
1160-1350: Beta CDF / inverse CDF / Clopper–Pearson: formulas correct; update stale Newton–Raphson remarkThe new pieces here look correct overall:
CalculateBetaCDF(Lines [1175]-[1189]) validatesα, β > 0,x ∈ [0,1], handles boundaries, and delegates to the regularized incomplete beta — this matches the standard Beta CDF.CalculateInverseBetaCDF(Lines [1210]-[1258]) uses a bisection search over[0,1]with monotoneCalculateBetaCDF, plus 0/1 boundary short‑circuits. That’s a robust choice for these CDFs.CalculateClopperPearsonIntervalandCalculateClopperPearsonLowerBound(Lines [1281]-[1350]) implement the usual Beta-quantile formulas with correct edge handling forsuccesses == 0andsuccesses == trials.One remaining issue (also noted in a past review):
- The XML remarks for
CalculateInverseBetaCDFstill say “The implementation uses Newton-Raphson iteration” (Lines [1207]-[1208]) but the code now uses pure bisection (Lines [1232]-[1255]). This is misleading for maintainers.Consider updating the remarks to describe the current bisection-based inversion (monotone CDF, guaranteed convergence, fixed max iterations) so docs match behavior.
Proposed doc-only tweak
- /// <para> - /// The implementation uses Newton-Raphson iteration for numerical stability and accuracy. - /// </para> + /// <para> + /// The implementation uses a robust bisection-based root-finding method on [0, 1], + /// which guarantees convergence for the monotonic Beta CDF while preserving numerical stability. + /// </para>src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs (1)
1047-1067: NegativeInfinity initialization may fail for non-floating-point types.Line 1050 uses
NumOps.FromDouble(double.NegativeInfinity)which may produce unexpected values for integer numeric types. This was flagged in a past review as addressed, but the same pattern remains.#!/bin/bash # Check if this pattern was actually fixed or if there's a remaining instance rg -n "NegativeInfinity" src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cssrc/FineTuning/ReinforcementLearningHumanFeedback.cs (1)
243-255: Consider using ternary for reward computation.Both branches assign to
reward. A ternary expression would be cleaner, as noted in past static analysis.src/AdversarialRobustness/Alignment/RLHFAlignment.cs (1)
110-115: Division by zero ifTestInputs.Rowsis 0.If
evaluationData.TestInputs.Rowsis zero, dividing bytotalwill produceNaNvalues. The past review flagged this issue but the guard appears to be missing in the current code.🔎 Proposed fix
int total = evaluationData.TestInputs.Rows; + if (total == 0) + { + return metrics; + } + metrics.HelpfulnessScore = (double)helpfulCount / total; metrics.HarmlessnessScore = (double)harmlessCount / total; metrics.HonestyScore = (double)honestCount / total; metrics.PreferenceMatchRate = totalPreferenceMatch / total;src/FineTuning/FineTuningBase.cs (1)
263-274: Consider using ternary for Sigmoid.Both branches return a value to the same result type. A ternary expression would be more concise.
🔎 Proposed refactor
protected static double Sigmoid(double x) { - if (x >= 0) - { - return 1.0 / (1.0 + Math.Exp(-x)); - } - else - { - var exp = Math.Exp(x); - return exp / (1.0 + exp); - } + return x >= 0 + ? 1.0 / (1.0 + Math.Exp(-x)) + : Math.Exp(x) / (1.0 + Math.Exp(x)); }src/Models/Options/TrainingPipelineConfiguration.cs (1)
1567-1581: Remove unusedlearningRateMultipliervariable.The variable is computed but never read;
stage.LearningRateis set using the same formula inline.🔎 Proposed fix
for (int i = 0; i < curriculumStages.Length; i++) { var (name, data) = curriculumStages[i]; - double learningRateMultiplier = 1.0 / (1 + i * 0.2); pipeline.AddSFTStage(stage => { stage.Name = $"Curriculum Stage {i + 1}: {name}"; stage.StageType = TrainingStageType.CurriculumLearning; stage.TrainingData = data; stage.Epochs = 2; stage.BatchSize = 8; - stage.LearningRate = 2e-5 * learningRateMultiplier; + stage.LearningRate = 2e-5 / (1 + i * 0.2); }); }
🟡 Minor comments (19)
src/FineTuning/KahnemanTverskyOptimization.cs-174-176 (1)
174-176: Index clamping may mask data misalignment.Using
Math.Min(i, array.Length - 1)to accessChosenOutputsandRejectedOutputswhen iterating overDesirabilityLabelssilently handles length mismatches. If the arrays are unexpectedly different lengths, this could produce incorrect evaluations without warning.Consider validating that array lengths match the expected counts, or throwing an exception if the data is malformed.
src/FineTuning/ConstitutionalAIFineTuning.cs-207-209 (1)
207-209: Index clamping may mask data misalignment.Same concern as in
KahnemanTverskyOptimization: usingMath.Min(i, batch.Inputs.Length - 1)silently handles mismatched array lengths betweenCritiqueRevisionsandInputs. Consider validating data alignment or throwing on mismatch.src/FineTuning/ConstitutionalAIFineTuning.cs-99-100 (1)
99-100: UnusedcritiqueIterationsvariable.
critiqueIterationsis extracted from options but never used inComputeCAILossAndUpdateAsyncor elsewhere. Either remove it or implement the iterative critique-revision logic it implies.🔎 Suggested fix
If not implementing iterative critique yet, remove the unused variable:
var beta = Options.Beta; -var critiqueIterations = Options.CritiqueIterations; var totalSteps = Options.Epochs * (trainingData.Count / Options.BatchSize);And update the method call:
var batchLoss = await ComputeCAILossAndUpdateAsync( - policyModel, batch, beta, critiqueIterations, cancellationToken); + policyModel, batch, beta, cancellationToken);Committable suggestion skipped: line range outside the PR's diff.
src/FineTuning/StatisticalRejectionSampling.cs-90-91 (1)
90-91: Integer division truncates total steps count.Same issue as in RDPO -
trainingData.Count / Options.BatchSizetruncates, potentially underestimating progress.src/AdversarialRobustness/Attacks/PGDAttack.cs-286-293 (1)
286-293: Silent failure for out-of-range target class.When
targetClassis out of bounds,ComputeLossreturnsNumOps.Zerosilently. This could mask configuration errors. Consider throwing or logging a warning.src/Models/Options/FineTuningOptions.cs-368-368 (1)
368-368: Same mutable default array issue for TargetModules.
TargetModulesinLoRAConfigurationhas the same shared mutable default array problem.src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs-709-715 (1)
709-715: Incomplete CROWN relaxation for Sigmoid/Tanh activations.For
Sigmoid,Tanh, andLeakyReLU, the bounds are passed through unchanged (newAlpha = alpha). This doesn't apply proper CROWN linear relaxation, potentially producing looser bounds than expected. Consider implementing proper linear relaxations for these activations or documenting this as a known limitation.src/AdversarialRobustness/Attacks/PGDAttack.cs-298-316 (1)
298-316: Silent fallback to index 0 for null/empty labels.Returning
0when the label is null or empty can mask bugs in calling code. Consider throwing anArgumentExceptionfor invalid input to fail fast.🔎 Suggested fix
private int GetClassIndex(Vector<T> label) { if (label == null || label.Length == 0) { - return 0; + throw new ArgumentException("Label vector cannot be null or empty.", nameof(label)); }src/FineTuning/RobustDirectPreferenceOptimization.cs-91-91 (1)
91-91: Integer division may undercount total steps.The calculation
trainingData.Count / Options.BatchSizeuses integer division, which truncates. IftrainingData.Countis not evenly divisible byBatchSize, the last partial batch will still be processed byCreateBatches, buttotalStepswill be lower than actual steps executed.🔎 Suggested fix
- var totalSteps = Options.Epochs * (trainingData.Count / Options.BatchSize); + var totalSteps = Options.Epochs * ((trainingData.Count + Options.BatchSize - 1) / Options.BatchSize);src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs-897-921 (1)
897-921: Potential overflow in Sigmoid/Tanh for extreme values.
ApplySigmoidandApplyTanhuseNumOps.Exp(x)without bounds checking. For large positive/negativex, this can overflow. Consider clamping inputs or using numerically stable implementations.🔎 Numerically stable sigmoid example
private T ApplySigmoid(T x) { + // Numerically stable sigmoid: use different formulas for positive/negative + double xVal = NumOps.ToDouble(x); + if (xVal >= 0) + { + double expNegX = Math.Exp(-xVal); + return NumOps.FromDouble(1.0 / (1.0 + expNegX)); + } + else + { + double expX = Math.Exp(xVal); + return NumOps.FromDouble(expX / (1.0 + expX)); + } - T negX = NumOps.Negate(x); - T expNegX = NumOps.Exp(negX); - T onePlusExp = NumOps.Add(NumOps.One, expNegX); - return NumOps.Divide(NumOps.One, onePlusExp); }src/AdversarialRobustness/Defenses/AdversarialTraining.cs-285-291 (1)
285-291:ApplyDenoisingis a no-op that doesn't actually denoise.The method claims to apply "moving average denoising" but actually just returns the input unchanged (adding a zero vector). This should either be implemented properly or the comment should clarify it's a placeholder.
🔎 Proposed fix with actual simple denoising
private Vector<T> ApplyDenoising(Vector<T> input) { - // Simple moving average denoising (for demonstration) - // Use Engine to clone the vector - var zeros = Engine.FillZero<T>(input.Length); - return Engine.Add<T>(input, zeros); + // Simple box filter denoising with window size 3 + // For demonstration - a real implementation would use proper convolution + if (input.Length < 3) + { + return input; + } + + var denoised = new Vector<T>(input.Length); + denoised[0] = input[0]; + denoised[input.Length - 1] = input[input.Length - 1]; + + for (int i = 1; i < input.Length - 1; i++) + { + var sum = NumOps.ToDouble(input[i - 1]) + NumOps.ToDouble(input[i]) + NumOps.ToDouble(input[i + 1]); + denoised[i] = NumOps.FromDouble(sum / 3.0); + } + + return denoised; }Committable suggestion skipped: line range outside the PR's diff.
src/FineTuning/RankResponsesHumanFeedback.cs-191-191 (1)
191-191: PotentialNullReferenceExceptionifCustomMetricsis not initialized.The code accesses
metrics.CustomMetrics["kendall_tau"]without verifying thatCustomMetricsis initialized. IfFineTuningMetrics<T>doesn't initializeCustomMetricsto an empty dictionary, this will throw.#!/bin/bash # Verify if CustomMetrics is initialized in FineTuningMetrics ast-grep --pattern $'class FineTuningMetrics<$_> { $$$ CustomMetrics$$$ $$$ }' # Also check for property initialization rg -n "CustomMetrics" --type=cs -C3src/FineTuning/GroupRelativePolicyOptimization.cs-284-286 (1)
284-286: KL divergence calculation is incorrect.
kl = logRatiocomputeslog(π/π_ref), but KL divergence isE[log(π/π_ref)]with expectation under π. The current code accumulates log-ratios without proper weighting. For a point estimate, use(ratio - 1) - logRatio(reverse KL) or similar.🔎 Suggested fix for proper KL approximation
-// KL penalty -var kl = logRatio; +// KL penalty: approximate KL(π || π_ref) using (ratio - 1) - log(ratio) +// This is the reverse KL divergence approximation +var kl = ratio - 1.0 - logRatio; totalKL += kl;src/AdversarialRobustness/Safety/ContentClassifierBase.cs-170-180 (1)
170-180:maxScoreinitialized toZeromay miss negative scores.If all category scores are negative (which could happen depending on scoring implementation),
primaryCategoryremains empty andmaxScorestays at zero, producing incorrect results. Consider initializing to the first score orNegativeInfinity.🔎 Suggested fix
-string primaryCategory = string.Empty; -T maxScore = NumOps.Zero; +string primaryCategory = categoryScores.Count > 0 ? categoryScores.First().Key : string.Empty; +T maxScore = categoryScores.Count > 0 ? categoryScores.First().Value : NumOps.Zero; var detectedCategories = new List<string>(); foreach (var kvp in categoryScores) { - if (NumOps.GreaterThan(kvp.Value, maxScore)) + if (NumOps.GreaterThanOrEquals(kvp.Value, maxScore)) { maxScore = kvp.Value; primaryCategory = kvp.Key; }src/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cs-582-589 (1)
582-589:ApplyTanhcan produce NaN for large inputs due to overflow.When
xis large,exp(x)overflows to infinity, causingInfinity - 0/Infinity + 0= NaN. Consider clamping input or using a numerically stable implementation.🔎 Suggested fix
private T ApplyTanh(T x) { + // Clamp to avoid overflow - tanh saturates beyond ~20 + double xDouble = NumOps.ToDouble(x); + if (xDouble > 20.0) return NumOps.FromDouble(1.0); + if (xDouble < -20.0) return NumOps.FromDouble(-1.0); + T expX = NumOps.Exp(x); T expNegX = NumOps.Exp(NumOps.Negate(x)); T numerator = NumOps.Subtract(expX, expNegX); T denominator = NumOps.Add(expX, expNegX); return NumOps.Divide(numerator, denominator); }src/FineTuning/GroupRelativePolicyOptimization.cs-296-297 (1)
296-297:GroupRewardVarianceis overwritten each batch item; only the last value is kept.The metric is set inside the loop, so only the final item's group variance is preserved. Consider accumulating and averaging, or moving outside the loop to track across the entire batch.
🔎 Suggested fix
+double totalVariance = 0.0; +int groupCount = 0; + for (int i = 0; i < batch.Count; i++) { // ... existing code ... - // Track group variance for metrics - CurrentMetrics.GroupRewardVariance = stdReward * stdReward; + totalVariance += stdReward * stdReward; + groupCount++; } +// Track average group variance for metrics +CurrentMetrics.GroupRewardVariance = groupCount > 0 ? totalVariance / groupCount : 0.0;Committable suggestion skipped: line range outside the PR's diff.
src/FineTuning/OddsRatioPreferenceOptimization.cs-267-279 (1)
267-279: Discontinuity inComputeLogOddsat probability 0.999.The approximation creates a jump: at
prob=0.999, the normal formula yieldslogProb + 6.9, but the approximation yieldslogProb + 23. Consider using a higher threshold (e.g.,0.9999999) orlog1p(-prob)for numerical stability without discontinuity.🔎 Suggested fix using log1p
private static double ComputeLogOdds(double logProb) { - // log(P/(1-P)) = log(P) - log(1-P) - // For numerical stability when P is close to 1: - // log(1-P) = log(1 - exp(log_p)) - var prob = Math.Exp(logProb); - if (prob > 0.999) - { - // Use approximation for high probabilities - return logProb - Math.Log(1e-10); - } - return logProb - Math.Log(1.0 - prob); + // log(P/(1-P)) = log(P) - log(1-P) + // Use log1p for numerical stability: log(1-P) = log1p(-exp(logProb)) + // For very negative logProb, -exp(logProb) ≈ 0, so log1p ≈ 0 + var negProb = -Math.Exp(logProb); + if (negProb > -1e-15) // prob very close to 1 + { + return logProb - Math.Log(1e-15); + } + return logProb - Math.Log1p(negProb); }Committable suggestion skipped: line range outside the PR's diff.
src/FineTuning/DirectPreferenceOptimization.cs-90-92 (1)
90-92: Integer division may undercount total steps.When
trainingData.Countis not evenly divisible byOptions.BatchSize, the integer division truncates, causingtotalStepsto undercount. This affects progress logging accuracy.🔎 Proposed fix
var beta = Options.Beta; - var totalSteps = Options.Epochs * (trainingData.Count / Options.BatchSize); + var totalSteps = Options.Epochs * ((trainingData.Count + Options.BatchSize - 1) / Options.BatchSize); var currentStep = 0;src/AdversarialRobustness/CertifiedRobustness/RandomizedSmoothing.cs-273-283 (1)
273-283: Add path validation for consistency with other persistence methods.
SaveModelandLoadModellack the validation present inFineTuningBase(null/empty check, directory creation). This could cause confusing exceptions when called with invalid paths.🔎 Proposed fix
public void SaveModel(string filePath) { + if (string.IsNullOrWhiteSpace(filePath)) + { + throw new ArgumentException("File path cannot be null or empty.", nameof(filePath)); + } + + var fullPath = Path.GetFullPath(filePath); + var directory = Path.GetDirectoryName(fullPath); + if (!string.IsNullOrEmpty(directory) && !Directory.Exists(directory)) + { + Directory.CreateDirectory(directory); + } + - File.WriteAllBytes(filePath, Serialize()); + File.WriteAllBytes(fullPath, Serialize()); } public void LoadModel(string filePath) { + if (string.IsNullOrWhiteSpace(filePath)) + { + throw new ArgumentException("File path cannot be null or empty.", nameof(filePath)); + } + + var fullPath = Path.GetFullPath(filePath); + if (!File.Exists(fullPath)) + { + throw new FileNotFoundException("Configuration file not found.", fullPath); + } + - Deserialize(File.ReadAllBytes(filePath)); + Deserialize(File.ReadAllBytes(fullPath)); }
🧹 Nitpick comments (50)
src/Enums/TrainingStageType.cs (1)
18-566: Consider assigning explicit values for serialization stability.The enum currently relies on implicit value assignment (0, 1, 2, ...), which can cause breaking changes if the order is modified in the future. For production stability and backward compatibility, consider assigning explicit integer values to each member, especially since this enum is likely to be persisted or serialized in training pipelines.
💡 Example of explicit value assignment
public enum TrainingStageType { // Foundation Training Stages - PreTraining, - ContinuedPreTraining, - SupervisedFineTuning, - InstructionTuning, + PreTraining = 0, + ContinuedPreTraining = 1, + SupervisedFineTuning = 2, + InstructionTuning = 3, // Preference Optimization Stages - PreferenceOptimization, - OddsRatioPreference, - ContrastivePreference, + PreferenceOptimization = 10, + OddsRatioPreference = 11, + ContrastivePreference = 12, // ... continue for all membersNote: Consider leaving gaps between categories (e.g., 0-9 for Foundation, 10-19 for Preference Optimization) to allow for future additions without renumbering.
Optional: Consider the large enum size.
With 40+ members, this enum is quite comprehensive. While the extensive documentation and clear categorization help manage the complexity, you might want to evaluate whether:
- Some stages could be consolidated into parameterized variants
- The enum could be split into category-specific enums (though this may reduce usability)
- A different pattern (e.g., strategy classes) might offer more flexibility
That said, if the goal is a single comprehensive catalog of training stages, the current design serves that purpose well.
src/ModelRegistry/ModelRegistry.cs (1)
554-587: Consider catchingUnauthorizedAccessExceptionfrom path validation.
ValidatePathWithinDirectoryat line 570 throwsUnauthorizedAccessExceptionif the path is outside the registry directory (e.g., from tampered data). Currently onlyIOExceptionis caught, so a security violation would propagate as an unhandled exception rather than returningnull.🔎 Proposed fix
catch (IOException) { return null; } + catch (UnauthorizedAccessException) + { + // Path validation failed - file is outside registry directory + return null; + }src/Models/JailbreakDetectionResult.cs (2)
6-7: Unused generic type parameterT.The type parameter
Tis declared but never used within the class. If it's for API consistency with other result types, consider documenting this in the remarks. Otherwise, consider removing it to simplify the API.
65-68: Consider clarifying whatLocationrepresents.The
int Locationproperty is ambiguous—it could mean character position, line number, token index, or something else. Consider either renaming to be more specific (e.g.,CharacterPosition) or adding documentation clarifying the unit.src/Models/HarmfulContentResult.cs (2)
6-7: Unused generic type parameterT.Same as
JailbreakDetectionResult<T>, the type parameterTis declared but unused. Consider documenting the rationale or removing if not needed for API consistency.
41-42: Consider using an enum or constant forRecommendedAction.The default value
"Allow"is a magic string. If there's a fixed set of possible actions (Allow, Block, Flag, etc.), an enum would provide type safety and discoverability.src/Models/Options/FineTuningConfiguration.cs (1)
68-71: Consider validatingValidationSplitRatiobounds.The ratio should be between 0 and 1 (exclusive). A value outside this range (e.g., negative or > 1) would cause incorrect behavior during data splitting. Consider adding validation in a setter or a
Validate()method.🔎 Proposed approach
private double _validationSplitRatio = 0.1; public double ValidationSplitRatio { get => _validationSplitRatio; set { if (value <= 0 || value >= 1) throw new ArgumentOutOfRangeException(nameof(value), "ValidationSplitRatio must be between 0 and 1 (exclusive)."); _validationSplitRatio = value; } }src/AdversarialRobustness/Attacks/AdversarialAttackBase.cs (1)
126-129: Directory traversal check improved but not comprehensive.Checking for
".."in the originalfilePathbefore normalization (lines 126, 154) is better than checking afterPath.GetFullPath, and addresses the prior review concern. However, this approach can still be bypassed by URL-encoded sequences, alternative path separators, or symbolic links.If directory traversal protection is critical for your threat model, consider implementing proper boundary validation by defining an allowed base directory:
🔎 Optional enhancement with boundary validation
public virtual void SaveModel(string filePath) { if (string.IsNullOrWhiteSpace(filePath)) { throw new ArgumentException("File path cannot be null or empty.", nameof(filePath)); } - // Validate path doesn't contain directory traversal attempts BEFORE normalization - // Path.GetFullPath normalizes and resolves ".." sequences, so we must check the original input - if (filePath.Contains("..")) - { - throw new ArgumentException("File path cannot contain directory traversal sequences.", nameof(filePath)); - } - var fullPath = Path.GetFullPath(filePath); + + // Define and validate against an allowed base directory + var allowedBase = Path.GetFullPath("./models"); // or make this configurable + if (!fullPath.StartsWith(allowedBase + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase) && + !fullPath.Equals(allowedBase, StringComparison.OrdinalIgnoreCase)) + { + throw new ArgumentException($"File path must be within the allowed directory: {allowedBase}", nameof(filePath)); + } // Ensure parent directory exists var directory = Path.GetDirectoryName(fullPath);Apply similar changes to
LoadModel(lines 154-157).If the current check is sufficient for your security requirements, you may keep the existing implementation.
Also applies to: 154-157
src/Models/Options/FineTuningMetrics.cs (1)
282-303: Consider consistent epsilon comparisons for floating-point checks.Line 292 correctly uses
Math.Abs(AverageReward) > double.Epsilonfor robust floating-point comparison, while lines 282, 287, and 298 use direct> 0comparisons. Although> 0is generally acceptable for non-equality checks, using epsilon consistently would be more defensive against edge cases with very small positive values.🔎 Optional: Consistent epsilon comparisons
- if (WinRate > 0) + if (WinRate > double.Epsilon) { lines.Add($"Win Rate: {WinRate:P1}"); } - if (PreferenceAccuracy > 0) + if (PreferenceAccuracy > double.Epsilon) { lines.Add($"Preference Accuracy: {PreferenceAccuracy:P1}"); } if (Math.Abs(AverageReward) > double.Epsilon) { lines.Add($"Average Reward: {AverageReward:F3}"); lines.Add($"KL Divergence: {KLDivergence:F4}"); } - if (HarmlessnessScore > 0 || HelpfulnessScore > 0) + if (HarmlessnessScore > double.Epsilon || HelpfulnessScore > double.Epsilon) { lines.Add($"Harmlessness: {HarmlessnessScore:P1}"); lines.Add($"Helpfulness: {HelpfulnessScore:P1}"); lines.Add($"Honesty: {HonestyScore:P1}"); }If these metrics are guaranteed to be either zero or significantly positive (e.g., > 0.01), the current approach is acceptable.
src/Models/AlignmentEvaluationData.cs (1)
3-3: Optional: Redundant using directive.Per the project's global usings (configured in AiDotNet.csproj),
AiDotNet.Tensors.LinearAlgebrais already included globally. This explicit using is redundant but harmless.Based on learnings, the global usings already cover this namespace. You may remove it for consistency:
namespace AiDotNet.Models; -using AiDotNet.Tensors.LinearAlgebra; - /// <summary>If you prefer explicit usings for clarity, keeping it is also acceptable.
src/Models/Options/AdversarialDefenseOptions.cs (1)
15-15: Unused generic type parameterT.The type parameter
Tis declared but not used by any property in this class. All properties use concrete types (double,int,bool,string). Consider removing the generic parameter to simplify the API, or document the intended future use.🔎 Suggested fix
-public class AdversarialDefenseOptions<T> +public class AdversarialDefenseOptionssrc/Models/AlignmentMetrics.cs (1)
7-7: Unused generic type parameterT.Similar to
AdversarialDefenseOptions<T>, the type parameterTis declared but never referenced. All properties aredoubleorDictionary<string, double>.🔎 Suggested fix
-public class AlignmentMetrics<T> +public class AlignmentMetricssrc/Models/Options/AlignmentMethodOptions.cs (1)
16-16: Unused generic type parameterT.Consistent with the other options classes in this PR,
Tis declared but not used. Consider removing it or documenting the intended future use for consistency across the codebase.src/Models/Options/SafetyFilterOptions.cs (3)
26-26: Consider adding range validation for threshold properties.The documentation specifies that
SafetyThresholdandJailbreakSensitivityshould be in the range [0, 1], but the properties don't enforce this constraint. Consumers could set invalid values that might cause unexpected behavior in filtering logic.🔎 Proposed validation approach
You could add validation in the setters or provide a
Validate()method that's called after configuration:private double _safetyThreshold = 0.8; public double SafetyThreshold { get => _safetyThreshold; set => _safetyThreshold = value is >= 0.0 and <= 1.0 ? value : throw new ArgumentOutOfRangeException(nameof(value), "SafetyThreshold must be between 0 and 1"); }Alternatively, use init-only setters with validation in a constructor or factory method.
Also applies to: 36-36
66-73: Consider validating that HarmfulContentCategories is not null or empty.While the property has a sensible default, consumers could set it to
nullor an empty array, potentially bypassing content filtering or causing null reference issues.🔎 Proposed validation approach
private string[] _harmfulContentCategories = new[] { "Violence", "HateSpeech", "AdultContent", "PrivateInformation", "Misinformation" }; public string[] HarmfulContentCategories { get => _harmfulContentCategories; set => _harmfulContentCategories = value?.Length > 0 ? value : throw new ArgumentException("HarmfulContentCategories cannot be null or empty"); }
93-93: Consider validating that MaxInputLength is positive.The property controls input length limits to prevent abuse, but there's no validation ensuring the value is positive. A negative or zero value could cause unexpected behavior in validation logic.
🔎 Proposed validation approach
private int _maxInputLength = 10000; public int MaxInputLength { get => _maxInputLength; set => _maxInputLength = value > 0 ? value : throw new ArgumentOutOfRangeException(nameof(value), "MaxInputLength must be positive"); }src/AdversarialRobustness/Attacks/FGSMAttack.cs (1)
89-93: Note: Clipping assumes normalized input data.The adversarial example is clipped to the range [0, 1] (line 93), which is standard practice for normalized image data. When deploying this attack, ensure that input data is appropriately normalized. For raw pixel values (e.g., 0-255) or non-image data with different ranges, you may need to adjust the clipping bounds or normalize inputs before attack generation.
src/Helpers/StatisticsHelper.cs (1)
1010-1038: AlignLogGammadocumentation with implementation and consider enforcing domain preconditionThe new Numerical Recipes–style implementation looks correct, but:
- XML summary (Line [995]) still claims “Lanczos approximation” while the body (Lines [1010]-[1021]) is now NR with
g = 5. It would be clearer to update the summary/remarks to match.- The approximation assumes
x > 0. TodayLogGammadoesn’t guard or document this, andGamma(T x)delegates directly to it. If there’s any chance callers pass non‑positivex, consider either:
- Explicitly throwing for
x <= 0, or- Documenting the
x > 0precondition on bothLogGammaandGamma.src/FineTuning/ConstitutionalAIFineTuning.cs (1)
256-263: Placeholder implementation returns hardcoded scores.
EvaluateConstitutionalCompliancereturns(0.8, 0.8)for all inputs, makingEvaluateAsyncmetrics meaningless. The comment acknowledges this is a placeholder.Would you like me to open an issue to track implementing actual constitutional compliance evaluation? This could use principle-based scoring or integrate with an external evaluator model.
src/Models/Options/FineTuningData.cs (1)
190-202: Missing bounds validation inSubsetcould causeIndexOutOfRangeException.The method directly indexes arrays using provided indices without validating they are within bounds. If a caller passes an out-of-range index, this will throw an unhandled exception.
🔎 Suggested defensive check
public FineTuningData<T, TInput, TOutput> Subset(int[] indices) { + if (indices == null) + throw new ArgumentNullException(nameof(indices)); + + foreach (var idx in indices) + { + if (idx < 0 || idx >= Inputs.Length) + throw new ArgumentOutOfRangeException(nameof(indices), $"Index {idx} is out of range [0, {Inputs.Length})."); + } + return new FineTuningData<T, TInput, TOutput> { // ... }; }src/AdversarialRobustness/Attacks/AutoAttack.cs (1)
213-231: Edge case:GetClassIndexreturns 0 for null/empty label vector.When
labelis null or empty, the method returns 0. This silent fallback could mask errors where a label is unexpectedly missing. Consider throwing an exception or documenting this behavior explicitly.🔎 Suggested defensive alternative
private int GetClassIndex(Vector<T> label) { if (label == null || label.Length == 0) { - return 0; + throw new ArgumentException("Label vector cannot be null or empty.", nameof(label)); } // ... }src/Models/Options/AdversarialRobustnessConfiguration.cs (1)
117-133: Consider validatingRobustnessEvaluationSampleRatiorange.The property accepts any
double, but semantically it should be between 0 and 1. Invalid values could cause unexpected behavior during evaluation. Consider adding validation or documenting the expected range.src/FineTuning/SelfPlayFineTuning.cs (1)
40-46: Consider throwing instead of silently correctingMethodType.If a caller passes options with the wrong
MethodType, silently correcting it could mask configuration errors. An explicit exception would help callers identify misconfiguration early.🔎 Alternative approach
public SelfPlayFineTuning(FineTuningOptions<T> options) : base(options) { if (options.MethodType != FineTuningMethodType.SPIN) { - options.MethodType = FineTuningMethodType.SPIN; + throw new ArgumentException( + $"Options must specify MethodType.SPIN, but got {options.MethodType}.", + nameof(options)); } }src/FineTuning/RobustDirectPreferenceOptimization.cs (4)
35-41: Constructor silently mutates the passed options object.The constructor modifies
options.MethodTypeif it doesn't matchRDPO. This mutation of an external object can surprise callers who reuse the same options instance for multiple fine-tuning methods.Consider throwing an
ArgumentExceptionif the method type is incorrect, or document this behavior clearly.
187-248: Method name suggests model update but only computes loss.
ComputeRDPOLossAndUpdateAsyncimplies it updates model weights, but the implementation only computes and returns the loss. Either rename toComputeRDPOLossAsyncfor clarity, or add the actual gradient update logic.
181-181: Unnecessary async wrapper.
EvaluateAsyncperforms no async operations but wraps the result inTask.FromResult. Consider making this method synchronous or usingValueTask<T>if the interface requires async.
253-265: Magic numbers in robustness weight computation.The thresholds
0.3,0.7, and multiplier0.5are hardcoded without explanation. Consider extracting these as configurable options or named constants with documentation explaining the rationale.src/AdversarialRobustness/Attacks/PGDAttack.cs (2)
120-125: Inefficient vector cloning via addition.Using
Engine.Addwith a zero vector to clone is less efficient than direct array copy. Consider using a dedicated clone/copy method or direct array copying.🔎 Suggested improvement
private Vector<T> CloneVector(Vector<T> input) { - // Use Engine.Add with a zero vector to create a copy - var zeros = Engine.FillZero<T>(input.Length); - return Engine.Add<T>(input, zeros); + // Direct copy is more efficient + return new Vector<T>(input.ToArray()); }
259-274: High allocation overhead in finite-difference gradient.Each dimension creates a new zero-filled vector via
Engine.FillZero. For high-dimensional inputs, this causes O(n) vector allocations. Consider reusing a single perturbation vector and resetting the perturbed index after each iteration.🔎 Suggested optimization
+ // Reuse a single perturbation vector + var perturbationDelta = Engine.FillZero<T>(vectorInput.Length); + for (int i = 0; i < vectorInput.Length; i++) { - // Create perturbation vector with delta in dimension i - var perturbationDelta = Engine.FillZero<T>(vectorInput.Length); perturbationDelta[i] = delta; // ... compute gradient ... gradient[i] = NumOps.Divide(NumOps.Subtract(perturbedLoss, originalLoss), delta); + perturbationDelta[i] = NumOps.Zero; // Reset for next iteration }src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs (1)
97-97: Semantic mismatch: NoiseSigma used as perturbation radius.
NoiseSigma(typically referring to Gaussian noise standard deviation) is used as the epsilon perturbation radius. This can confuse users configuring CROWN for L-infinity robustness verification. Consider using a dedicatedEpsilonorPerturbationRadiusoption.src/FineTuning/StatisticalRejectionSampling.cs (3)
320-324: Silent failure when reference model is null.
ComputePairLossreturns0.0if_referenceModelis null instead of failing. This could mask initialization bugs. SinceFineTuneAsyncsets_referenceModelbefore calling this, it's unlikely but consider throwingInvalidOperationExceptionfor safety.
34-40: Constructor silently mutates options.Same pattern as RDPO - the constructor modifies
options.MethodTypeif incorrect, which may surprise callers reusing options objects.
206-206: Unnecessary async wrapper in EvaluateAsync.Same issue as RDPO -
Task.FromResultwraps a synchronous operation.src/Models/Options/FineTuningOptions.cs (1)
373-373: Consider using an enum for BiasMode.
BiasModeis a string property with documented values like "none". Using an enum would provide compile-time safety and discoverability.🔎 Example enum
public enum LoRABiasMode { None, All, LoraOnly }src/FineTuning/SimplePreferenceOptimization.cs (2)
144-147: Inconsistent validation behavior between training and evaluation.
FineTuneAsyncthrowsArgumentExceptionwhen pairwise data is missing (line 78), butEvaluateAsyncsilently returns empty metrics. This inconsistency could mask configuration errors during evaluation.Consider throwing an exception or at least logging a warning to maintain consistency.
🔎 Proposed fix for consistent validation
if (!evaluationData.HasPairwisePreferenceData) { - return metrics; + throw new ArgumentException("SimPO requires pairwise preference data for evaluation.", nameof(evaluationData)); }
183-183: Unnecessary async wrapper.
EvaluateAsyncperforms no async operations but wraps the result inTask.FromResult. Consider usingValueTaskor making the method synchronous if the interface allows, or simply return the result directly if the base class/interface defines an async signature that must be honored.src/AdversarialRobustness/Alignment/RLHFAlignment.cs (1)
458-461: Unused methodClip01.This method is defined but never called within the class. Consider removing it if unneeded.
src/AdversarialRobustness/Attacks/CWAttack.cs (1)
72-73: Hardcoded hyperparameters should come from options.
c(confidence parameter) andlearningRateare hardcoded. Consider exposing these throughAdversarialAttackOptions<T>for configurability.🔎 Suggested approach
- var c = 1.0; // Confidence parameter - var learningRate = 0.01; + var c = Options.CWConfidence; // Add to options + var learningRate = Options.CWLearningRate; // Add to optionsThis allows users to tune the attack parameters for different scenarios.
src/FineTuning/SupervisedFineTuning.cs (1)
122-140: Per-sample gradient application differs from standard batch training.Gradients are applied immediately after each sample (line 134), rather than accumulating gradients across the batch and applying once. This:
- Changes training dynamics compared to true mini-batch SGD
- May lead to instability with larger learning rates
- Is less efficient due to per-sample weight updates
🔎 Suggested approach for batch gradient accumulation
private Task<double> ProcessBatchAsync( IFullModel<T, TInput, TOutput> model, FineTuningData<T, TInput, TOutput> batch, ILossFunction<T> lossFunction, T learningRate) { double totalLoss = 0.0; int count = batch.Count; + // Accumulate gradients across batch + object? accumulatedGradients = null; for (int i = 0; i < count; i++) { var input = batch.Inputs[i]; var output = batch.Outputs[i]; var prediction = model.Predict(input); var gradients = model.ComputeGradients(input, output, lossFunction); - model.ApplyGradients(gradients, learningRate); + accumulatedGradients = model.AccumulateGradients(accumulatedGradients, gradients); var predVector = ConversionsHelper.ConvertToVector<T, TOutput>(prediction); var targetVector = ConversionsHelper.ConvertToVector<T, TOutput>(output); totalLoss += NumOps.ToDouble(lossFunction.CalculateLoss(predVector, targetVector)); } + // Apply accumulated gradients once per batch + if (accumulatedGradients != null && count > 0) + { + var scaledLR = NumOps.Divide(learningRate, NumOps.FromDouble(count)); + model.ApplyGradients(accumulatedGradients, scaledLR); + } var avgLoss = count > 0 ? totalLoss / count : 0.0; return Task.FromResult(avgLoss); }Note: This requires
IFullModelto supportAccumulateGradients. If that's not available, document the current behavior.src/FineTuning/IdentityPreferenceOptimization.cs (4)
38-44: Constructor mutates the passed options object.Modifying
options.MethodTypeinside the constructor is a side effect that could surprise callers who pass in a shared options instance. Consider validating instead of mutating, or document this behavior explicitly.🔎 Suggested alternative: Throw if mismatched
public IdentityPreferenceOptimization(FineTuningOptions<T> options) : base(options) { if (options.MethodType != FineTuningMethodType.IPO) { - options.MethodType = FineTuningMethodType.IPO; + throw new ArgumentException( + $"Options must have MethodType set to IPO, but got {options.MethodType}.", + nameof(options)); } }
97-97:totalStepsmay undercount if partial batches are yielded.If
CreateBatchesyields a partial final batch (e.g., 10 items with batch size 3 yields 4 batches),totalStepswill underestimate progress. Consider using ceiling division.🔎 Suggested fix
-var totalSteps = Options.Epochs * (trainingData.Count / Options.BatchSize); +var totalSteps = Options.Epochs * ((trainingData.Count + Options.BatchSize - 1) / Options.BatchSize);
154-155:winsandcorrectare always identical.Both variables are incremented together and produce the same metric values. Consider consolidating into a single counter unless there's a planned distinction.
Also applies to: 176-180, 183-184
189-189: Unnecessaryawait Task.FromResultpattern.The method is
asyncbut performs no actual async operations. Either remove theasyncmodifier and returnTask.FromResult(metrics)directly, or keep the current pattern for interface consistency. The same applies toComputeIPOLossAndUpdateAsyncat line 243.src/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cs (1)
536-538: LeakyReLU alpha is hardcoded to 0.01.Consider making alpha configurable via options or extracting it as a constant to support different LeakyReLU configurations.
src/FineTuning/OddsRatioPreferenceOptimization.cs (1)
152-156: Same redundancy:winsandcorrectare always identical.This is the same pattern as in
IdentityPreferenceOptimization. Consider consolidating.src/AdversarialRobustness/Safety/ContentClassifierBase.cs (1)
194-206: Action thresholds (0.8, 0.5) are hardcoded whileDetectionThresholdis configurable.Consider making action thresholds configurable or deriving them from
DetectionThresholdfor consistency.src/FineTuning/FineTuningBase.cs (1)
307-320: ReplaceConsole.WriteLinewith a proper logging abstraction.The inline comment acknowledges this limitation. Consider injecting an
ILoggeror using a logging abstraction to enable structured logging, log-level filtering, and integration with monitoring systems.src/FineTuning/DirectPreferenceOptimization.cs (3)
71-76: Remove duplicate validation.
ValidateTrainingData(called at line 71) already checksHasPairwisePreferenceDataand throws if missing. The subsequent check at lines 73-76 is redundant.🔎 Proposed fix
ValidateTrainingData(trainingData); - if (!trainingData.HasPairwisePreferenceData) - { - throw new ArgumentException("DPO requires pairwise preference data (ChosenOutputs and RejectedOutputs).", nameof(trainingData)); - } - // Clone the base model as the reference (frozen during training)
143-146: Consider logging or warning when evaluation data lacks pairwise preferences.Returning empty metrics silently may mask configuration issues. A warning would help developers understand why metrics are empty.
181-184: Simplify async return.Since
EvaluateAsyncdoesn't perform any async operations, it could either returnmetricsdirectly (changing signature to non-async) or avoid the unnecessaryTask.FromResultwrapping.🔎 Proposed fix (option 1: remove async/Task.FromResult)
- public override async Task<FineTuningMetrics<T>> EvaluateAsync( + public override Task<FineTuningMetrics<T>> EvaluateAsync( IFullModel<T, TInput, TOutput> model, FineTuningData<T, TInput, TOutput> evaluationData, CancellationToken cancellationToken = default) { // ... existing code ... - return await Task.FromResult(metrics); + return Task.FromResult(metrics); }
- CROWNVerification: fix empty if-branches by inverting conditions - IntervalBoundPropagation: use TypeNameHandling.None for security, add conservative [0,1] bounds for softmax outputs - MathHelper: add input validation for Atanh to throw on invalid inputs - ConstitutionalAIFineTuning: add gradient computation and application - GroupRelativePolicyOptimization: use batch data for response diversity - KahnemanTverskyOptimization: add gradient computation and application - ReinforcementLearningHumanFeedback: fix value estimate to use mean of output vector, fix PPO ratio by recomputing log probs from current policy - ModelRegistry: add path traversal validation for SaveModelCard - FineTuningData: add missing arrays to Subset method (DesirabilityLabels, RankedOutputs, Advantages, Values, CritiqueRevisions, TeacherOutputs, TeacherConfidences) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
src/ModelRegistry/ModelRegistry.cs (1)
689-695: Static analysis finding mitigated; verify modelName validation.Line 692 was flagged by static analysis for
Path.Combineusage. The validation at line 693 provides defense-in-depth. However, ensure thatValidateModelName(called in entry-point methods) prevents path traversal sequences inmodelName(e.g.,../../malicious, absolute paths).#!/bin/bash # Description: Verify ValidateModelName prevents path traversal # Find ValidateModelName implementation in base class ast-grep --pattern $'ValidateModelName($$$) { $$$ }' # Also search for the method definition rg -n "ValidateModelName" --type=cs -B 2 -A 10
🧹 Nitpick comments (15)
src/AiDotNet.Tensors/Helpers/MathHelper.cs (1)
242-251: Reconsider the epsilon clamping after validation.The method throws
ArgumentOutOfRangeExceptionfor inputs outside (-1, 1) (lines 243-247), which correctly addresses the past review concern. However, the subsequent epsilon clamping (line 251) silently modifies valid inputs that are near the boundaries.For example, if a caller passes
x = 0.9999999999999(a valid input), it gets clamped to1.0 - 1e-12, changing their intended value. This behavior is inconsistent with other methods in this class:
ArcSin(Line 1069) throws for invalid inputs but doesn't clamp valid onesLog2(Line 983) throws for invalid inputs but doesn't clamp valid onesSince the validation already ensures
xis within the mathematical domain where(1-x) > 0, the epsilon clamping may be unnecessary. If it's truly needed for numerical stability at extreme boundaries, this should be clearly documented to explain why this method differs from the class pattern.🔎 Suggested approach: Remove clamping or document the rationale
Option 1 (recommended): Remove the epsilon clamping and trust the validation
// Validate input is within the mathematical domain of atanh: (-1, 1) if (x <= -1.0 || x >= 1.0) { throw new ArgumentOutOfRangeException(nameof(x), x, "Input must be in the open interval (-1, 1) for atanh."); } - // Apply small epsilon clamping only for floating-point edge cases very close to boundaries - const double eps = 1e-12; - x = Clamp(x, -1.0 + eps, 1.0 - eps); - // atanh(x) = 0.5 * ln((1+x)/(1-x)) return 0.5 * Math.Log((1.0 + x) / (1.0 - x));Option 2: If clamping is essential, document why this method differs from the class pattern
Update the XML documentation to explain:
- Why valid inputs near boundaries are modified
- What numerical issues the clamping prevents
- How this differs from other methods like
ArcSinandLog2src/Interfaces/IPredictionModelBuilder.cs (3)
345-350: Enhance documentation to match the file's established pattern.The
ConfigureSafetyFiltermethod has minimal documentation compared to other methods in this interface. Most methods include extensive<remarks>sections with beginner-friendly explanations, use cases, and examples (seeConfigureLoRAat lines 352-379,ConfigureFeatureSelectorat lines 37-51, orConfigureKnowledgeDistillationat lines 769-814).Consider adding a detailed
<remarks>section that explains:
- What safety filtering does and why it's important
- When users should configure it
- How it affects model inference
- Example configuration
📝 Example documentation enhancement
/// <summary> /// Configures the safety filter used to validate inputs and filter outputs during inference. /// </summary> +/// <remarks> +/// A safety filter provides content moderation and validation during model inference to ensure +/// predictions meet safety and quality standards. +/// +/// <b>For Beginners:</b> Safety filters act as guardrails for your AI model. They can: +/// - Block harmful, toxic, or inappropriate inputs before prediction +/// - Filter or flag unsafe outputs from the model +/// - Ensure compliance with content policies +/// - Protect users from potentially harmful AI-generated content +/// +/// Common use cases: +/// - Content moderation in user-facing applications +/// - Compliance with safety regulations +/// - Preventing prompt injection attacks +/// - Filtering personally identifiable information (PII) +/// +/// Example: +/// <code> +/// var safetyConfig = new SafetyFilterConfiguration<double> +/// { +/// EnableInputValidation = true, +/// EnableOutputFiltering = true, +/// BlockedCategories = new[] { ContentCategory.Harmful, ContentCategory.Toxic } +/// }; +/// +/// var result = await builder +/// .ConfigureModel(model) +/// .ConfigureSafetyFilter(safetyConfig) +/// .BuildAsync(); +/// </code> +/// </remarks> /// <param name="configuration">Safety filter configuration.</param> /// <returns>The builder instance for method chaining.</returns>
381-397: Document the default behavior when configuration is null.The method allows
configuration = nullas the default (line 397), but the documentation doesn't explain what happens in this case. Users callingConfigureTrainingPipeline()with no arguments won't know the resulting behavior.Other methods with nullable defaults document this clearly (e.g.,
ConfigureUncertaintyQuantificationat line 431 states "when null, defaults are used and UQ is enabled").📝 Suggested documentation enhancement
/// <summary> /// Configures a multi-stage training pipeline for advanced training workflows. /// </summary> /// <remarks> /// <para> /// ConfigureTrainingPipeline enables advanced multi-stage training workflows where each stage /// can have its own training method, optimizer, learning rate, and dataset. Stages execute /// sequentially, with each stage's output model becoming the next stage's input. /// </para> /// <para><b>For Beginners:</b> Think of this as a recipe with multiple cooking steps. /// Just like you might marinate, then sear, then bake - training can have multiple /// phases where each phase teaches the model something different.</para> /// </remarks> -/// <param name="configuration">The training pipeline configuration defining the stages to execute.</param> +/// <param name="configuration">The training pipeline configuration defining the stages to execute. +/// If null, uses a default single-stage pipeline configuration.</param> /// <returns>The builder instance for method chaining.</returns>
399-412: Consider clarifying the Auto() factory method reference.The documentation mentions "Auto() factory method" (line 406) but doesn't provide context about where this method exists or how users might access it directly. While this is a convenience overload, users might want to understand the underlying mechanism.
📝 Suggested documentation enhancement
/// <summary> /// Configures a training pipeline with automatic stage selection based on the provided data. /// </summary> /// <remarks> /// <para> -/// This is a convenience overload that creates a TrainingPipelineConfiguration using the -/// Auto() factory method. The system analyzes your data characteristics and automatically -/// constructs an appropriate multi-stage pipeline. +/// This is a convenience overload that automatically analyzes your training data and constructs +/// an appropriate multi-stage pipeline. Internally, this calls TrainingPipelineConfiguration.Auto(trainingData) +/// to perform the analysis and stage selection. +/// +/// The system examines data characteristics such as size, complexity, and type to determine +/// the optimal training stages, methods, and hyperparameters. /// </para> /// </remarks> /// <param name="trainingData">The training data to analyze for automatic pipeline construction.</param> /// <returns>The builder instance for method chaining.</returns>src/ModelRegistry/ModelRegistry.cs (1)
589-661: LGTM!The Model Card generation logic correctly extracts metadata, feature importance, and tags. The tag-based recommendation/limitation extraction (lines 634-648) provides good extensibility.
Optional: Consider clarifying version format
Line 607 formats the integer version as semantic versioning (
"{version}.0.0"). This might confuse users who expect the registry's integer versions. Consider:
- Adding a comment explaining the format conversion
- Or providing a parameter to customize the version string
- Version = $"{version}.0.0", + // Convert integer version to semantic version format for Model Card + Version = $"{version}.0.0",src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs (3)
71-76: Constructor mutates caller's options object.Lines 74-75 modify properties on the user-provided
optionsinstance, which can surprise callers who may not expect their reference to be mutated. Consider cloning the options or documenting this behavior.🔎 Proposed fix to avoid mutation
public CROWNVerification(CertifiedDefenseOptions<T> options) { - _options = options ?? throw new ArgumentNullException(nameof(options)); - _options.CertificationMethod = "CROWN"; - _options.UseTightBounds = true; + if (options == null) + { + throw new ArgumentNullException(nameof(options)); + } + + _options = new CertifiedDefenseOptions<T> + { + NoiseSigma = options.NoiseSigma, + ConfidenceLevel = options.ConfidenceLevel, + NumSamples = options.NumSamples, + NormType = options.NormType, + BatchSize = options.BatchSize, + RandomSeed = options.RandomSeed, + CertificationMethod = "CROWN", + UseTightBounds = true + }; }
703-709: Consider tighter linear relaxations for non-ReLU activations.The current implementation passes
alphaLowerandalphaUpperthrough unchanged for Sigmoid, Tanh, and LeakyReLU. While conservative, this may produce looser bounds than necessary. Consider implementing proper CROWN relaxations for these activations (e.g., using tangent lines for Sigmoid/Tanh) for tighter certification.
1010-1039: Consider making maximum search radius configurable.Lines 1017 hardcodes
maxRadius = 1.0for the binary search. For some applications, the certified radius might exceed 1.0, or users might want to limit search to a smaller range for performance. Consider making this configurable via options.src/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cs (2)
71-75: Constructor mutates caller's options object.Lines 73-74 modify properties on the user-provided
optionsinstance, which can surprise callers. Consider cloning the options or documenting this behavior to match the pattern inCROWNVerification.cs.🔎 Proposed fix to avoid mutation
public IntervalBoundPropagation(CertifiedDefenseOptions<T> options) { - _options = options ?? throw new ArgumentNullException(nameof(options)); - _options.CertificationMethod = "IBP"; + if (options == null) + { + throw new ArgumentNullException(nameof(options)); + } + + _options = new CertifiedDefenseOptions<T> + { + NoiseSigma = options.NoiseSigma, + ConfidenceLevel = options.ConfidenceLevel, + NumSamples = options.NumSamples, + NormType = options.NormType, + UseTightBounds = options.UseTightBounds, + BatchSize = options.BatchSize, + RandomSeed = options.RandomSeed, + CertificationMethod = "IBP" + }; }
717-752: Consider making maximum search radius configurable.Line 724 hardcodes
maxRadius = 1.0. For consistency withCROWNVerification.csand to support applications where certified radius might exceed 1.0, consider making this configurable via options.src/Models/Options/FineTuningData.cs (2)
190-222: Subset now includes all data arrays — previous issue resolved.The
Subsetmethod has been updated to include all data arrays (DesirabilityLabels,RankedOutputs,Advantages,Values,CritiqueRevisions,TeacherOutputs,TeacherConfidences), addressing the previous review concern about incomplete copies.However, consider adding bounds validation for
indicesto preventIndexOutOfRangeExceptionwhen callers pass invalid indices:🔎 Optional: Add bounds validation
public FineTuningData<T, TInput, TOutput> Subset(int[] indices) { + if (indices.Any(i => i < 0 || i >= Inputs.Length)) + { + throw new ArgumentOutOfRangeException(nameof(indices), + "One or more indices are out of range."); + } + return new FineTuningData<T, TInput, TOutput> {
230-252: Split implementation is correct with Fisher-Yates shuffle.The
Splitmethod correctly implements Fisher-Yates shuffle for randomization and properly partitions data. One minor edge case:When
validationRatiois 0 or 1, the behavior may be unexpected (empty train or validation sets). Consider documenting or validating the ratio bounds.🔎 Optional: Validate ratio bounds
public (FineTuningData<T, TInput, TOutput> Train, FineTuningData<T, TInput, TOutput> Validation) Split( double validationRatio = 0.1, int? seed = null) { + if (validationRatio < 0.0 || validationRatio > 1.0) + { + throw new ArgumentOutOfRangeException(nameof(validationRatio), + "Validation ratio must be between 0.0 and 1.0."); + } + var random = seed.HasValuesrc/FineTuning/ReinforcementLearningHumanFeedback.cs (1)
264-277: Value estimation improved, but consider documenting the expected model interface.The value estimation now converts the output to a vector and uses the mean, which is an improvement over the previous log probability approach. However, this assumes the value model's output can be meaningfully averaged.
Consider adding documentation clarifying that
_valueModelshould ideally be configured to output a single scalar value, or that the current averaging approach is a fallback.src/FineTuning/ConstitutionalAIFineTuning.cs (2)
184-234:critiqueIterationsparameter is declared but unused.The
critiqueIterationsparameter (line 188) is passed toComputeCAILossAndUpdateAsyncbut never used within the method. The CAI documentation mentions iterative critique-revision, but this isn't implemented.🔎 Suggested approach
Either:
- Remove the unused parameter and
Options.CritiqueIterationsfrom the call site (line 119)- Implement iterative critique-revision logic using the parameter
private async Task<double> ComputeCAILossAndUpdateAsync( IFullModel<T, TInput, TOutput> policyModel, FineTuningData<T, TInput, TOutput> batch, double beta, - int critiqueIterations, double learningRate, CancellationToken cancellationToken)
265-272: Evaluation returns placeholder scores.
EvaluateConstitutionalCompliancereturns hardcoded(0.8, 0.8)as acknowledged in the comment. This meansEvaluateAsyncproduces fixed metrics regardless of actual model performance.Consider either implementing actual compliance checking (possibly using the constitutional principles) or documenting that evaluation metrics are not yet functional.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cssrc/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cssrc/AiDotNet.Tensors/Helpers/MathHelper.cssrc/FineTuning/ConstitutionalAIFineTuning.cssrc/FineTuning/GroupRelativePolicyOptimization.cssrc/FineTuning/KahnemanTverskyOptimization.cssrc/FineTuning/ReinforcementLearningHumanFeedback.cssrc/Interfaces/IPredictionModelBuilder.cssrc/ModelRegistry/ModelRegistry.cssrc/Models/Options/FineTuningData.cssrc/NeuralNetworks/NeuralNetworkBase.cssrc/PredictionModelBuilder.cs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/Interfaces/IPredictionModelBuilder.cssrc/FineTuning/ReinforcementLearningHumanFeedback.cssrc/ModelRegistry/ModelRegistry.cssrc/AiDotNet.Tensors/Helpers/MathHelper.cssrc/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cssrc/FineTuning/KahnemanTverskyOptimization.cssrc/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cssrc/FineTuning/GroupRelativePolicyOptimization.cssrc/Models/Options/FineTuningData.cssrc/FineTuning/ConstitutionalAIFineTuning.cs
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/Interfaces/IPredictionModelBuilder.cssrc/FineTuning/ReinforcementLearningHumanFeedback.cssrc/ModelRegistry/ModelRegistry.cssrc/AiDotNet.Tensors/Helpers/MathHelper.cssrc/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cssrc/FineTuning/KahnemanTverskyOptimization.cssrc/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cssrc/FineTuning/GroupRelativePolicyOptimization.cssrc/Models/Options/FineTuningData.cssrc/FineTuning/ConstitutionalAIFineTuning.cs
📚 Learning: 2025-12-19T19:05:13.598Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 445
File: src/Interfaces/IPredictionModelBuilder.cs:7-8
Timestamp: 2025-12-19T19:05:13.598Z
Learning: In ooples/AiDotNet, IAggregationStrategy<TModel> is declared in AiDotNet.Interfaces (src/Interfaces/IAggregationStrategy.cs). For src/Interfaces/IPredictionModelBuilder.cs (also in AiDotNet.Interfaces), referencing IAggregationStrategy does not require an additional using. Avoid flagging missing using directives for this case in future reviews.
Applied to files:
src/Interfaces/IPredictionModelBuilder.cssrc/ModelRegistry/ModelRegistry.cssrc/Models/Options/FineTuningData.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL Analysis
🔇 Additional comments (11)
src/ModelRegistry/ModelRegistry.cs (4)
1-2: LGTM!The new using directives are necessary for Model Card functionality (ModelCard type and MathHelper).
523-549: LGTM!Thread-safe implementation with proper path validation before file write. The dual persistence approach (embedded in model metadata + separate file) provides both convenience and accessibility.
551-587: LGTM!Proper path validation and graceful fallback when file is not found or unreadable. The exception handling pattern (returning null on IOException) is appropriate for this context.
663-687: Path validation applied; verify SaveToFile does not bypass it.The path validation at lines 675-677 correctly implements the standard approach for preventing path traversal: calling System.IO.Path.GetFullPath() translates special characters in the created path, followed by directory validation.
However, line 686 delegates to
modelCard.SaveToFile(resolvedPath). Verify that this method does not perform additional path manipulation that could circumvent the validation already performed.src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs (2)
79-129: LGTM: Clean certification flow.The method correctly validates inputs, computes CROWN bounds, checks certification criteria, and returns comprehensive results including radius and confidence metrics.
1041-1061: Verify numeric type compatibility with NegativeInfinity.Line 1044 uses
NumOps.FromDouble(double.NegativeInfinity)to initializemaxOtherUpper. For numeric types like integers, convertingNegativeInfinitymay produce unexpected values. While this likely works for typical floating-point types used in ML, consider initializing with the first valid other-class value for robustness.Note: This was previously flagged and marked as addressed in commits 668c973-ecff530, but the pattern remains in the current code.
🔎 Alternative initialization approach
private double ComputeConfidence(Vector<T> lowerBounds, Vector<T> upperBounds, int predictedClass) { T predictedLower = lowerBounds[predictedClass]; - T maxOtherUpper = NumOps.FromDouble(double.NegativeInfinity); + T maxOtherUpper = NumOps.Zero; + bool foundOther = false; for (int i = 0; i < upperBounds.Length; i++) { - if (i != predictedClass && - NumOps.GreaterThan(upperBounds[i], maxOtherUpper)) + if (i != predictedClass) { - maxOtherUpper = upperBounds[i]; + if (!foundOther || NumOps.GreaterThan(upperBounds[i], maxOtherUpper)) + { + maxOtherUpper = upperBounds[i]; + foundOther = true; + } } }src/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cs (4)
287-316: LGTM: Secure deserialization implementation.Good use of
TypeNameHandling.None(line 298) for a simple DTO. This avoids the deserialization security risks associated withTypeNameHandling.Auto.
434-493: LGTM: Correct IBP linear layer propagation.The weight sign handling (lines 474-485) correctly implements IBP bounds for linear transformations. The remarks provide clear mathematical context.
547-552: LGTM: Conservative Softmax bounds are correct.Good fix from previous implementation. Using
[0, 1]bounds for Softmax is conservative but correct, as Softmax outputs are guaranteed to be in this range. This prevents the false certification claims that would result from naive element-wise propagation.
757-780: Verify numeric type compatibility with NegativeInfinity.Line 760 uses
NumOps.FromDouble(double.NegativeInfinity)to initializemaxOtherUpper. For non-floating numeric types, this may produce unexpected values. Consider initializing from the first valid other-class value instead, matching the suggestion for the same pattern inCROWNVerification.cs.🔎 Alternative initialization approach
private double ComputeConfidence(Vector<T> lowerBounds, Vector<T> upperBounds, int predictedClass) { T predictedLower = lowerBounds[predictedClass]; - T maxOtherUpper = NumOps.FromDouble(double.NegativeInfinity); + T maxOtherUpper = NumOps.Zero; + bool foundOther = false; for (int i = 0; i < upperBounds.Length; i++) { - if (i != predictedClass && - NumOps.GreaterThan(upperBounds[i], maxOtherUpper)) + if (i != predictedClass) { - maxOtherUpper = upperBounds[i]; + if (!foundOther || NumOps.GreaterThan(upperBounds[i], maxOtherUpper)) + { + maxOtherUpper = upperBounds[i]; + foundOther = true; + } } }src/FineTuning/GroupRelativePolicyOptimization.cs (1)
238-268: Partial fix for deterministic responses, but limitation remains for larger groups.The implementation now uses chosen/rejected outputs from pairwise preference data for the first two responses (lines 244-248), providing some diversity. However, for
groupSize > 2or when pairwise data isn't available, responses remain deterministic (line 252), and GRPO's group-based advantage estimation loses its effectiveness.The inline comment (lines 241-242) correctly notes that "proper GRPO requires temperature sampling." This is a known limitation.
Consider documenting this limitation in the class-level remarks or throwing if
groupSize > 2and no pairwise data is available, to prevent silent ineffectiveness.
- Fix TypeNameHandling.Auto security risk in CROWNVerification.cs - Fix index clamping in ConstitutionalAIFineTuning.cs - Fix GRPO loss not updating parameters with gradient computation - Fix GroupRewardVariance accumulation to average across groups - Fix KTO index bounds checking and unpaired preference handling - Fix RLHF PPO log probability computation (use same action, not new prediction) - Fix RLHF loss not updating model parameters with gradient application - Fix floating point comparisons using double.Epsilon in FineTuningMetrics - Convert if-else blocks to ternary expressions in FineTuningBase, GRPO, RLHF 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update mock models to implement IFullModel with all required members - Change attack and defense types to use 3 generic type arguments - Replace Matrix/Vector<int> labels with Vector<double>[] arrays - Add helper methods for creating test inputs and one-hot labels 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/FineTuning/GroupRelativePolicyOptimization.cs (1)
242-266: Group responses remain identical without sampling, nullifying GRPO.The code attempts to use pairwise batch outputs for diversity (lines 247-249), but when
g >= 2or when pairwise data is unavailable, all responses come frompolicyModel.Predict(input), which is deterministic. This causes all group rewards to be identical, makingstdRewardeffectively 0 (clamped to 1.0 on line 277), and all normalized advantages become 0—eliminating the learning signal.The in-line comment acknowledges this limitation, but GRPO cannot function correctly without stochastic sampling. Consider:
- Adding a temperature parameter to
Predictor introducing aSamplemethod- Throwing
NotImplementedExceptionuntil sampling is implemented- Documenting this as a placeholder requiring user-supplied sampling
🧹 Nitpick comments (11)
src/FineTuning/GroupRelativePolicyOptimization.cs (2)
48-54: Constructor modifies input options parameter.The constructor silently overwrites
options.MethodTypeif it's not alreadyGRPO. This side effect is unexpected and may confuse callers who don't realize their options object has been mutated.Consider removing this check entirely (let the caller ensure correct
MethodType) or throw an exception if the type is wrong.🔎 Proposed fix: Remove defensive mutation
public GroupRelativePolicyOptimization(FineTuningOptions<T> options) : base(options) { - if (options.MethodType != FineTuningMethodType.GRPO) - { - options.MethodType = FineTuningMethodType.GRPO; - } }Alternatively, fail fast if the type is incorrect:
public GroupRelativePolicyOptimization(FineTuningOptions<T> options) : base(options) { - if (options.MethodType != FineTuningMethodType.GRPO) - { - options.MethodType = FineTuningMethodType.GRPO; - } + if (options.MethodType != FineTuningMethodType.GRPO) + { + throw new ArgumentException($"MethodType must be GRPO, but was {options.MethodType}", nameof(options)); + } }
274-278: Simplify conditional assignment.Both branches write to
stdReward, making this a candidate for a ternary expression orMath.Max.🔎 Proposed simplification
- // Avoid division by zero - if (stdReward < 1e-8) - { - stdReward = 1.0; - } + // Avoid division by zero + stdReward = Math.Max(stdReward, 1e-8);Note: Changed the fallback from
1.0to1e-8to preserve variance information when it's very small but non-zero. If1.0is intentional (to effectively disable advantage normalization when variance is near-zero), keep it:- // Avoid division by zero - if (stdReward < 1e-8) - { - stdReward = 1.0; - } + // Avoid division by zero (set to 1.0 to disable normalization) + stdReward = stdReward < 1e-8 ? 1.0 : stdReward;src/FineTuning/KahnemanTverskyOptimization.cs (1)
379-406: Consider including rejected outputs in KL estimate for paired data.The method currently computes the KL estimate using only
ChosenOutputs(lines 392-403). For paired preference data, includingRejectedOutputswould provide a more representative estimate of the divergence between policy and reference distributions across the full output space.For unpaired data with mixed desirability labels, consider iterating over all available outputs rather than only chosen ones.
🔎 Suggested enhancement
private double ComputeBatchKLEstimate( IFullModel<T, TInput, TOutput> policyModel, FineTuningData<T, TInput, TOutput> batch) { if (_referenceModel == null) { return 0.0; } double totalKL = 0.0; int count = 0; - // Estimate KL using chosen outputs + // Estimate KL using all available outputs for (int i = 0; i < batch.Count; i++) { var input = batch.Inputs[i]; var chosen = batch.ChosenOutputs[i]; var piLogProb = ComputeLogProbability(policyModel, input, chosen); var refLogProb = ComputeLogProbability(_referenceModel, input, chosen); - // KL = sum(pi * log(pi/ref)) = log(pi) - log(ref) when using samples totalKL += piLogProb - refLogProb; count++; + + // Also include rejected outputs for paired data + if (batch.HasPairwisePreferenceData && i < batch.RejectedOutputs.Length) + { + var rejected = batch.RejectedOutputs[i]; + var piLogProbRej = ComputeLogProbability(policyModel, input, rejected); + var refLogProbRej = ComputeLogProbability(_referenceModel, input, rejected); + totalKL += piLogProbRej - refLogProbRej; + count++; + } } return count > 0 ? totalKL / count : 0.0; }src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs (3)
296-300: Consider removing the unused SerializationBinder.With
TypeNameHandling.None, theSerializationBinderis never consulted during deserialization. While harmless, instantiating it adds unnecessary overhead.🔎 Proposed simplification
var settings = new JsonSerializerSettings { - TypeNameHandling = TypeNameHandling.None, - SerializationBinder = new SafeSerializationBinder() + TypeNameHandling = TypeNameHandling.None };
703-709: Consider implementing proper CROWN relaxation for non-ReLU activations.The current implementation passes through coefficients unchanged for Sigmoid, Tanh, and LeakyReLU, effectively treating them as identity functions in the backward pass. While this approach is conservative and sound (the forward IBP pass handles these activations correctly), implementing proper CROWN linear relaxations would provide tighter certified bounds.
For example, LeakyReLU with its piecewise-linear structure could use relaxations similar to ReLU. Sigmoid and Tanh could use tangent-based linear relaxations at the bound midpoints.
1010-1039: Consider making the maximum search radius configurable.The binary search uses a hardcoded maximum radius of 1.0 (line 1017). While appropriate for normalized data (e.g., images in [0,1]), different input scales might benefit from a configurable maximum radius, possibly as an option in
CertifiedDefenseOptions<T>.src/Models/Options/FineTuningMetrics.cs (1)
21-21: Unused type parameterT.The generic type parameter
Tis declared but never used within the class. All numeric properties are strongly typed asdouble,int, orlong. Consider removing the type parameter unless there's a planned use for it.src/FineTuning/ReinforcementLearningHumanFeedback.cs (2)
271-283: Log-ratio labeled as KL divergence.Line 272 computes
logProb - refLogProb, which is the log importance ratio for a single sample, not the KL divergence. True KL divergence requires an expectation over the distribution: KL(π||π_ref) = E_π[log(π/π_ref)].For PPO's KL penalty, using the log-ratio as an approximation is acceptable, but the naming is misleading. Consider renaming to
logRatioor adding a comment clarifying this is an approximate single-sample KL estimate.
293-296: UnusedgammaandgaeLambdaparameters.The parameters
gammaandgaeLambdaare passed to this method but never used. The comment explains this is due to single-step episodes, but consider either:
- Removing the parameters to avoid confusion, or
- Implementing proper multi-step GAE if the method signature suggests it's supported
src/FineTuning/ConstitutionalAIFineTuning.cs (2)
184-191: UnusedcritiqueIterationsparameter.The
critiqueIterationsparameter is passed to this method but never used within the implementation. Either implement iterative critique-revision (as described in CAI) or remove the parameter.
270-277: Placeholder compliance evaluation.The method returns hardcoded scores (0.8, 0.8). While documented as a placeholder, consider adding a TODO comment or logging a warning when called, so users are aware that compliance scores are not actually computed.
Do you want me to help design an interface for pluggable compliance evaluators?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cssrc/FineTuning/ConstitutionalAIFineTuning.cssrc/FineTuning/FineTuningBase.cssrc/FineTuning/GroupRelativePolicyOptimization.cssrc/FineTuning/KahnemanTverskyOptimization.cssrc/FineTuning/ReinforcementLearningHumanFeedback.cssrc/Models/Options/FineTuningMetrics.cstests/AiDotNet.Tests/AdversarialRobustness/CROWNVerificationTests.cstests/AiDotNet.Tests/AdversarialRobustness/IntervalBoundPropagationTests.cstests/AiDotNet.Tests/UnitTests/AdversarialRobustness/AdversarialAttackTests.cstests/AiDotNet.Tests/UnitTests/AdversarialRobustness/AdversarialTrainingTests.cstests/AiDotNet.Tests/UnitTests/AdversarialRobustness/RandomizedSmoothingTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/FineTuning/FineTuningBase.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/Models/Options/FineTuningMetrics.cssrc/FineTuning/KahnemanTverskyOptimization.cssrc/FineTuning/ReinforcementLearningHumanFeedback.cssrc/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cssrc/FineTuning/ConstitutionalAIFineTuning.cssrc/FineTuning/GroupRelativePolicyOptimization.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/Models/Options/FineTuningMetrics.cssrc/FineTuning/KahnemanTverskyOptimization.cssrc/FineTuning/ReinforcementLearningHumanFeedback.cssrc/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cssrc/FineTuning/ConstitutionalAIFineTuning.cssrc/FineTuning/GroupRelativePolicyOptimization.cs
🔇 Additional comments (23)
src/FineTuning/GroupRelativePolicyOptimization.cs (1)
305-306: The review comment cannot be fully verified without access to the codebase. The raised concerns about gradient computation (whetherComputeGradientscorrectly computes gradients of the GRPO loss rather than a reconstruction objective) and learning rate scaling (whether advantages should weight the loss rather than scale the learning rate) align with standard GRPO theory, but the actual implementation details inGroupRelativePolicyOptimization.csmust be examined to confirm these issues.src/FineTuning/KahnemanTverskyOptimization.cs (5)
1-59: LGTM! Class structure and configuration are correct.The constructor, properties, and metadata correctly configure KTO fine-tuning. The requirement for a reference model and the DirectPreference category align with the KTO algorithm.
62-73: LGTM! Validation correctly enforces KTO data requirements.The validation properly checks for either pairwise or unpaired preference data, which aligns with KTO's flexibility.
76-134: LGTM! Training loop now correctly updates and returns the policy model.The method properly clones the base model into a frozen reference and a trainable policy, iterates over batches to update parameters, and returns the trained policy model. This addresses the previous concern about not updating model parameters.
183-194: Reconsider the evaluation correctness criteria using sign of log-probability.Lines 188 and 193 use
logProb > 0andlogProb < 0to determine correctness, which is unusual. Standard log-probabilities are non-positive (since probabilities are in [0,1] and log(p) ≤ 0). Using the sign as a correctness threshold doesn't directly measure output quality.Verify what
ComputeLogProbabilityreturns—if it returns standard log-probabilities, consider comparing scores relative to a baseline (e.g., comparing desirable vs. undesirable outputs for the same input) rather than against zero. If the method returns log-odds or another metric with different semantics, clarify this in code comments.
238-374: Critical: Gradient computation does not correspond to the KTO loss function.The method computes KTO loss values (lines 277, 303, 352, 360) using the proper formulas, but the actual gradient computation via
ComputeGradients(input, output)(lines 280, 307, 354, 364) does not receive the loss as input. The code passes only the input and output, which suggests gradients are computed for log-probability rather than for the KTO loss objective.Impact: If
ComputeGradientsreturns log-probability gradients without incorporating the KTO loss formula, the model will be trained to maximize log-probability rather than optimize the KTO objective. This would break the algorithm's behavioral-economics-inspired weighting and loss-aversion mechanism.Verification required: Inspect the
IFullModel<T, TInput, TOutput>interface to confirm whatComputeGradients(input, output)actually computes. If it does not account for the loss function, refactor to pass the loss or scale gradients accordingly. Possible approaches:
- Pass loss to gradient computation:
ComputeGradientsFromLoss(loss, input, output)- Scale log-probability gradients: Multiply by the derivative of the KTO loss w.r.t. log-probability
src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs (6)
58-76: LGTM - Clean constructor initialization.Both constructors properly initialize the options and enforce the required CROWN configuration. The null check in the parameterized constructor is appropriate.
78-250: LGTM - Well-structured public APIs with proper validation.The public methods have appropriate null checks and input validation. The batch processing and accuracy evaluation are functionally correct.
567-580: LGTM - Correct logic for combining CROWN and IBP bounds.The code correctly selects the tighter bounds by using IBP's lower bound when it's higher than CROWN's, and IBP's upper bound when it's lower than CROWN's. This ensures the most precise verified bounds.
724-779: LGTM - Correct CROWN linear relaxation for ReLU.The implementation correctly handles all three cases for ReLU activation:
- Always active (l ≥ 0): identity function
- Always inactive (u ≤ 0): zero function
- Crossing (l < 0 < u): proper linear upper bound and area-minimization heuristic for lower bound
This matches the mathematical foundation described in the class documentation.
781-974: LGTM - Helper methods correctly implement IBP and sampling.The forward interval bound propagation is correctly implemented:
PropagateLinearLayerproperly handles positive and negative weightsPropagateActivationIBPcorrectly applies monotonic activations to boundsApproximateBoundsWithSamplingprovides a sound fallback when layer information is unavailableThe defensive swap check for LeakyReLU (lines 875-878) is good defensive programming even though mathematically unnecessary for monotonic activations.
1041-1061: ComputeConfidence implementation is functionally correct.The confidence computation finds the margin between the predicted class's lower bound and the maximum of other classes' upper bounds. The initialization with
NegativeInfinity(line 1044) works correctly for floating-point types typically used in ML. For single-class scenarios (which are nonsensical for classification), the loop would not updatemaxOtherUpper, but such cases shouldn't occur in practice.src/Models/Options/FineTuningMetrics.cs (1)
23-264: Well-structured metrics container.The properties are logically organized into categories with clear documentation. The use of section comments improves readability.
src/FineTuning/ReinforcementLearningHumanFeedback.cs (4)
42-49: LGTM on constructor setup.The constructor properly initializes the base class and auto-corrects the method type. The setter methods include appropriate null validation.
106-108: Value model initialization may produce incompatible architecture.Cloning the policy model as a value model assumes identical architectures. However, a value model should output a scalar state-value estimate, whereas the policy model outputs actions/tokens. This architectural mismatch may cause
ConversionsHelper.ConvertToVectorto produce unexpected results.Consider documenting this requirement or validating that the value model produces scalar outputs.
174-227: Evaluation implementation is correct.Proper null checks, edge case handling, and standard deviation calculation using Bessel's correction.
417-427: Experience structure correctly captures required PPO data.The
PPOExperiencestruct now storesOutput(the action taken), addressing the previous review concern about needing to recompute log probabilities for the same action.src/FineTuning/ConstitutionalAIFineTuning.cs (6)
38-61: LGTM on class setup.Constructor and property implementations are consistent with other fine-tuning classes in the codebase.
63-75: Appropriate validation extension.The override correctly calls base validation and adds CAI-specific requirements.
77-131: Training loop structure is correct.The method follows the established pattern: validate inputs, clone models, iterate epochs/batches, track metrics.
133-179: Evaluation implementation is structurally sound.The method correctly iterates evaluation data and aggregates scores. The placeholder compliance evaluation is appropriately documented in
EvaluateConstitutionalCompliance.
209-212: Past review issue addressed: Index clamping fixed.The previous review flagged that
Math.Min(i, batch.Inputs.Length - 1)could cause training on misaligned data. This is now correctly handled with an early break when indices exceed available inputs.
229-231: Past review issue addressed: Gradient updates now applied.The previous review flagged that loss computation did not update model parameters. Both paths (critique-revision and SFT) now correctly compute and apply gradients.
Also applies to: 256-258
Convert if statement to ternary expression for clarity when checking if stdReward is below epsilon threshold. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Cap advantage magnitude in GRPO and RLHF to prevent training instability - Remove broken value model training that trained toward action output - Add proper entropy loss computation for policy exploration - Replace impractical double.Epsilon with sensible 1e-9 threshold in metrics 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/FineTuning/ReinforcementLearningHumanFeedback.cs (1)
372-376: Advantage-scaled learning rate remains problematic despite capping.While capping the advantage to [-1, 1] is an improvement over using
Math.Sign, scaling the learning rate by advantage is still unconventional and potentially destabilizing:
- Negative learning rates: Negative advantages produce negative learning rates, causing updates in the opposite direction
- Variable step sizes: Advantage magnitude directly controls step size, which can destabilize training
- Conflated concepts: Standard PPO uses advantage to weight the gradient (via the surrogate objective), not the learning rate
The surrogate objective at line 362 already incorporates the advantage. Standard practice would compute gradients from this loss and apply them with a fixed learning rate.
🔎 Recommended approach
Keep the learning rate fixed and let the surrogate loss gradient naturally incorporate the advantage:
- // Compute and apply gradients scaled by capped advantage - var cappedAdvantage = Math.Max(-1.0, Math.Min(1.0, exp.Advantage)); - var scaledLearningRate = Options.LearningRate * cappedAdvantage; var gradients = policyModel.ComputeGradients(input, exp.Output); - policyModel.ApplyGradients(gradients, NumOps.FromDouble(scaledLearningRate)); + // Scale gradients by the policy loss (which includes advantage via surrogate objective) + // or use a fixed learning rate if the loss is already incorporated + policyModel.ApplyGradients(gradients, NumOps.FromDouble(Options.LearningRate * policyLoss));Alternatively, if the gradient API requires a target output rather than a loss, consider weighting the gradient tensor directly by the advantage rather than the learning rate.
src/FineTuning/GroupRelativePolicyOptimization.cs (2)
242-266: Deterministic responses remain an issue when batch diversity unavailable.The code now uses diverse responses from batch data when available (lines 247-249), which is a good mitigation. However, the fallback to
policyModel.Predict(input)(line 249) still produces identical responses, as acknowledged in the comment.When
HasPairwisePreferenceDatais false or for g ≥ 2, all group responses will be identical, causingstdRewardto be near-zero and all advantages to become zero—eliminating the GRPO learning signal.Consider adding a stochastic sampling API or documenting this limitation more prominently (e.g., in method-level docs or throwing an exception when batch diversity is insufficient).
🔎 Suggested approaches
Option 1: Add temperature sampling (preferred)
- : policyModel.Predict(input); + : policyModel.Sample(input, temperature: Options.Temperature);Option 2: Validate batch diversity and fail fast
+ if (!batch.HasPairwisePreferenceData && groupSize > 1) + { + throw new InvalidOperationException( + "GRPO requires diverse responses per group. Provide pairwise preference data or implement temperature sampling."); + }
299-303: Advantage-scaled learning rate remains problematic despite capping.Similar to the RLHF implementation, scaling the learning rate by advantage (even when capped) is unconventional and potentially destabilizing:
- Negative learning rates: Negative advantages produce negative learning rates
- Variable step sizes: Advantage magnitude directly controls step size
- Conflated concepts: The surrogate objective (line 289) already incorporates the advantage
Standard GRPO/PPO practice uses advantage to weight the gradient through the loss function, not to scale the learning rate.
🔎 Recommended fix
Use a fixed learning rate and let the loss gradient naturally incorporate the advantage:
- // Compute and apply gradients scaled by capped advantage - var cappedAdvantage = Math.Max(-1.0, Math.Min(1.0, advantage)); - var scaledLearningRate = learningRate * cappedAdvantage; var gradients = policyModel.ComputeGradients(input, groupResponses[g]); - policyModel.ApplyGradients(gradients, NumOps.FromDouble(scaledLearningRate)); + // Apply gradients with fixed learning rate; loss already includes advantage + policyModel.ApplyGradients(gradients, NumOps.FromDouble(learningRate * loss));Alternatively, scale the gradient tensor directly by the advantage if needed.
🧹 Nitpick comments (6)
src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs (3)
71-76: Consider documenting that constructor overwrites specific option fields.Lines 74-75 unconditionally overwrite
CertificationMethodandUseTightBoundsregardless of user-provided values. While these settings are correct for CROWN, silently modifying the user's options object may be unexpected. Consider either:
- Documenting this behavior in the constructor's XML comments
- Creating a defensive copy of options before modification
- Validating that the user provided compatible values
296-300: Remove unnecessary SerializationBinder.Line 299 sets a
SerializationBinder, but withTypeNameHandling.Noneon line 298, the binder is never used since no type metadata is processed during deserialization. You can safely remove line 299.🔎 Proposed cleanup
var settings = new JsonSerializerSettings { - TypeNameHandling = TypeNameHandling.None, - SerializationBinder = new SafeSerializationBinder() + TypeNameHandling = TypeNameHandling.None };
97-97: Clarify semantic usage of NoiseSigma for L-infinity perturbation.Line 97 uses
_options.NoiseSigmaas the L-infinity perturbation radius (ε). While functionally correct, "NoiseSigma" typically implies Gaussian noise (standard deviation), which may confuse readers. CROWN uses deterministic L-infinity bounds, not random noise.Consider adding a comment here explaining that
NoiseSigmais reused as the perturbation radius, or document this in the class-level remarks to clarify the semantic overloading of the sharedCertifiedDefenseOptions<T>field.src/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cs (3)
145-152: Consider parallelizing batch certification for improved throughput.The batch processing is sequential, which may become a bottleneck for large input arrays. Since each certification is independent, this loop is a candidate for parallel execution.
🔎 Suggested parallelization
- var results = new CertifiedPrediction<T>[inputs.Length]; - - for (int i = 0; i < inputs.Length; i++) - { - results[i] = CertifyPrediction(inputs[i], model); - } - - return results; + return inputs + .AsParallel() + .AsOrdered() + .Select(input => CertifyPrediction(input, model)) + .ToArray();
723-729: Consider making binary search parameters configurable.The hardcoded
maxRadius = 1.0andmaxIterations = 20may not suit all use cases. For high-precision requirements or larger perturbation ranges, these could be exposed viaCertifiedDefenseOptions<T>.
533-545: Hardcoded LeakyReLU alpha may not match network configuration.The alpha value of
0.01is hardcoded, but actual networks may use different values (e.g., 0.1, 0.2). If theILayer<T>interface provides activation parameters, consider retrieving the actual alpha.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cssrc/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cssrc/FineTuning/GroupRelativePolicyOptimization.cssrc/FineTuning/ReinforcementLearningHumanFeedback.cssrc/Models/Options/FineTuningData.cssrc/Models/Options/FineTuningMetrics.cstests/AiDotNet.Tests/Helpers/ActiveLearningTestHelper.cstests/AiDotNet.Tests/Helpers/ContinualLearningTestHelper.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Models/Options/FineTuningData.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/Models/Options/FineTuningMetrics.cssrc/FineTuning/ReinforcementLearningHumanFeedback.cssrc/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cssrc/FineTuning/GroupRelativePolicyOptimization.cssrc/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/Models/Options/FineTuningMetrics.cssrc/FineTuning/ReinforcementLearningHumanFeedback.cssrc/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cssrc/FineTuning/GroupRelativePolicyOptimization.cssrc/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL Analysis
- GitHub Check: Build (Windows)
🔇 Additional comments (17)
src/AdversarialRobustness/CertifiedRobustness/CROWNVerification.cs (4)
446-583: LGTM: CROWN backward bound propagation is well-implemented.The core CROWN algorithm correctly implements:
- Forward pass to compute pre-activation bounds (lines 461-487)
- Backward pass with linear bound propagation (lines 489-564)
- Input bound selection based on coefficient signs (lines 549-559)
- Bound tightening by taking the tighter of CROWN and IBP bounds (lines 567-580)
The logic for choosing lower vs. upper input bounds based on whether coefficients are positive or negative is correct for linear bound propagation.
735-779: LGTM: ReLU CROWN relaxation correctly implements all three cases.The ReLU linear relaxation logic is mathematically sound:
- Case 1 (l ≥ 0): Always active → slope=1, intercept=0
- Case 2 (u ≤ 0): Always inactive → slope=0, intercept=0
- Case 3 (crossing zero): Upper relaxation uses the correct formula
slope = u/(u-l), and lower bound heuristic (area minimization via|l| < |u|) is a valid approachThis aligns with the CROWN formulation described in the class documentation.
784-837: LGTM: Forward linear layer propagation correctly handles interval arithmetic.Lines 820-829 correctly implement interval bound propagation for linear layers:
- Positive weights:
lower * wcontributes to lower bound,upper * wto upper bound- Negative weights:
upper * wcontributes to lower bound,lower * wto upper bound- Zero weights: No contribution (handled implicitly by the conditions)
This ensures tight interval bounds through affine transformations.
871-879: LGTM: Defensive bound swap for LeakyReLU handles edge cases.Lines 875-878 include a defensive swap when
outputLower[i] > outputUpper[i], which can occur with LeakyReLU when negative inputs are scaled by alpha. This prevents invalid intervals and demonstrates good defensive coding practices.src/AdversarialRobustness/CertifiedRobustness/IntervalBoundPropagation.cs (4)
296-301: LGTM — security concern addressed.
TypeNameHandling.Noneis now correctly used, which eliminates the deserialization security risk. TheSerializationBinderis no longer necessary but is harmless to keep.
547-552: LGTM — Softmax now uses conservative bounds.Using
[0, 1]bounds is a valid conservative approach that prevents false certification claims while acknowledging that proper Softmax IBP requires joint computation.
1-45: Well-documented implementation with comprehensive XML documentation.The mathematical foundations, beginner explanations, and layer-by-layer propagation rules are clearly documented. This aids maintainability and onboarding.
469-486: Verify the type of theweightsvariable inPropagateLinearLayer(line 471).Line 471 uses flat linear indexing (
weights[i * inputDim + j]), which is incompatible with AiDotNet's Tensor multi-dimensional indexing syntax (features[new int[] { i, j }]). Ifweightsis aTensor<T>, this would produce incorrect weight lookups. Verify the variable type and adjust indexing accordingly if needed.src/Models/Options/FineTuningMetrics.cs (2)
1-264: LGTM!The class structure is well-organized with clear sections and comprehensive documentation. The metrics coverage is appropriate for various fine-tuning strategies.
270-308: Well done fixing the threshold checks!The use of a practical
displayThresholdconstant (1e-9) effectively addresses the previous floating-point epsilon concern. This ensures that only meaningful metric values are displayed in the summary.src/FineTuning/ReinforcementLearningHumanFeedback.cs (4)
32-80: LGTM!The constructor and configuration methods are well-structured with proper null checks and method type enforcement.
82-172: LGTM!The training loop properly clones models, validates data, collects experience, computes advantages, and performs PPO updates with appropriate metrics tracking.
256-269: Value estimation correctly fixed!The value model now properly extracts a scalar value estimate by averaging the output vector, rather than using log probability. This addresses the previous concern.
351-362: PPO importance sampling ratio correctly implemented!The code now properly stores the action (exp.Output) and recomputes its log probability under the current policy, correctly implementing the PPO importance sampling ratio. This fixes the previously flagged issue.
src/FineTuning/GroupRelativePolicyOptimization.cs (3)
39-76: LGTM!The constructor and configuration are well-structured with proper method type enforcement and requirement flags.
78-145: LGTM!The training loop properly initializes models, validates data, and performs GRPO updates with appropriate metrics tracking and cancellation support.
309-315: Group variance tracking correctly fixed!The code now properly accumulates group variance across all inputs and computes the average, addressing the previous concern about overwriting.
- Use numerically stable sigmoid computation with branching for x >= 0 vs x < 0 - Implement tanh via stable identity: tanh(x) = 2*sigmoid(2x) - 1 - Add null and empty check to GetPredictedClass to prevent IndexOutOfRangeException 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update value model during PPO training by computing gradients and scaling learning rate by the capped value error (reward - predicted value). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove ConfigureSafetyFilter (safety config is now part of ConfigureAdversarialRobustness) - Make ConfigureAdversarialRobustness accept null and use industry-standard defaults - Make ConfigureFineTuning accept null and use industry-standard defaults - Remove ConfigureTrainingPipeline(FineTuningData) overload (training data belongs in ConfigureFineTuning) - Update IPredictionModelBuilder interface to match implementation changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Summary
Fixes commitlint failures for dependabot PRs and adds proactive commit message fixing capability.
Problems Addressed
1. Dependabot PR Failures
2. Reactive Auto-Fix Wasn't Working
commitlint-autofix.ymlonly triggered onworkflow_runeventsSolutions Implemented
1. Updated commitlint Configuration (
commitlint.config.js)2. Enhanced PR Title Auto-Fix (
pr-title-lint.yml)3. New Proactive Commit Fix Workflow (
commitlint-fix.yml)Test Plan
Impact