You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Below is a summary of compliance checks for this PR:
Security Compliance
⚪
Denial of service
Description: Converting ArrayRemoteValue to arrays/lists performs unbounded allocations based on remoteValues size (ToList(), Array.CreateInstance, and list growth), which could allow a remote endpoint to trigger excessive memory usage (DoS) by returning extremely large arrays. RemoteValue.cs [112-149]
Add implicit conversion support from Script.RemoteValue to .NET well-known types to enable patterns like await bidi.Script.EvaluateAsync(...).
Extend the set of supported .NET types for converting RemoteValue (beyond the few primitive examples already present).
⚪
Confirm EvaluateAsync (and any other BiDi Script APIs returning RemoteValue) actually uses these implicit operators / ConvertTo() end-to-end in real BiDi sessions.
Codebase Duplication Compliance
⚪
Codebase context is not defined
Follow the guide to enable codebase context checks.
Custom Compliance
🟢
Generic: Comprehensive Audit Trails
Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance.
Generic: Robust Error Handling and Edge Case Management
Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation
Status: Missing edge handling: Numeric and reflective conversions can throw runtime exceptions (e.g., overflow or reflection Invoke failures) without being caught/wrapped to provide actionable context or graceful degradation.
Referred Code
(NumberRemoteValuen,Typet)whent==typeof(short)=>(TResult)(object)Convert.ToInt16(n.Value),(NumberRemoteValuen,Typet)when t ==typeof(ushort)=>(TResult)(object)Convert.ToUInt16(n.Value),(NumberRemoteValuen,Typet)when t ==typeof(int)=>(TResult)(object)Convert.ToInt32(n.Value),(NumberRemoteValuen,Typet)when t ==typeof(uint)=>(TResult)(object)Convert.ToUInt32(n.Value),(NumberRemoteValuen,Typet)when t ==typeof(long)=>(TResult)(object)Convert.ToInt64(n.Value),(NumberRemoteValuen,Typet)when t ==typeof(ulong)=>(TResult)(object)Convert.ToUInt64(n.Value),(NumberRemoteValuen,Typet)when t ==typeof(double)=>(TResult)(object)n.Value,(NumberRemoteValuen,Typet)when t ==typeof(float)=>(TResult)(object)Convert.ToSingle(n.Value),(StringRemoteValues,Typet)when t ==typeof(string)=>(TResult)(object)s.Value,(ArrayRemoteValuea,Typet)when t.IsArray
=>ConvertRemoteValuesToArray<TResult>(a.Value,t.GetElementType()!),(ArrayRemoteValuea,Typet)when t.IsGenericType&&t.IsAssignableFrom(typeof(List<>).MakeGenericType(t.GetGenericArguments()[0]))
...(clipped 40lines)
Generic: Security-First Input Validation and Data Handling
Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities
Status: Unvalidated value ranges: Converting externally-sourced numeric RemoteValue data via Convert.ToInt16/ToUInt64/... lacks explicit range/NaN/Infinity validation and may throw or behave unexpectedly depending on upstream inputs.
Referred Code
(NumberRemoteValuen,Typet)whent==typeof(short)=>(TResult)(object)Convert.ToInt16(n.Value),(NumberRemoteValuen,Typet)when t ==typeof(ushort)=>(TResult)(object)Convert.ToUInt16(n.Value),(NumberRemoteValuen,Typet)when t ==typeof(int)=>(TResult)(object)Convert.ToInt32(n.Value),(NumberRemoteValuen,Typet)when t ==typeof(uint)=>(TResult)(object)Convert.ToUInt32(n.Value),(NumberRemoteValuen,Typet)when t ==typeof(long)=>(TResult)(object)Convert.ToInt64(n.Value),(NumberRemoteValuen,Typet)when t ==typeof(ulong)=>(TResult)(object)Convert.ToUInt64(n.Value),(NumberRemoteValuen,Typet)when t ==typeof(double)=>(TResult)(object)n.Value,(NumberRemoteValuen,Typet)when t ==typeof(float)=>(TResult)(object)Convert.ToSingle(n.Value),
The current implementation uses reflection for converting elements within arrays and lists, which can be slow. To improve performance, consider caching the reflected methods to avoid repeated lookups.
privatestaticTResultConvertRemoteValuesToGenericList<TResult>(IEnumerable<RemoteValue>?remoteValues,TypelistType){varelementType=listType.GetGenericArguments()[0];varlist=(System.Collections.IList)Activator.CreateInstance(listType)!;if(remoteValuesis not null){varconvertMethod=typeof(RemoteValue).GetMethod(nameof(ConvertTo))!.MakeGenericMethod(elementType);foreach(variteminremoteValues)
...(clipped 7lines)
privatestaticreadonlySystem.Collections.Concurrent.ConcurrentDictionary<Type,System.Reflection.MethodInfo>_converterCache=new();privatestaticTResultConvertRemoteValuesToArray<TResult>(IEnumerable<RemoteValue>?remoteValues,TypeelementType){if(remoteValuesisnull){return(TResult)(object)Array.CreateInstance(elementType,0);}varconvertMethod=_converterCache.GetOrAdd(elementType,
type =>typeof(RemoteValue).GetMethod(nameof(ConvertTo))!.MakeGenericMethod(type));varitems=remoteValues.ToList();vararray=Array.CreateInstance(elementType,items.Count);for(inti=0;i<items.Count;i++){varconvertedItem=convertMethod.Invoke(items[i],null);array.SetValue(convertedItem,i);}return(TResult)(object)array;}
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies a significant performance issue due to reflection in collection conversions, which is a valid and important concern for a library feature.
Medium
General
Generalize null conversion for all types
Generalize the NullRemoteValue conversion to support all nullable and reference types by replacing the hardcoded list with a check for !t.IsValueType or if the type is Nullable.
-(NullRemoteValue _, Type t) when- t == typeof(bool?) ||- t == typeof(short?) ||- t == typeof(ushort?) ||- t == typeof(int?) ||- t == typeof(uint?) ||- t == typeof(long?) ||- t == typeof(ulong?) ||- t == typeof(double?) ||- t == typeof(float?) ||- t == typeof(string)+(NullRemoteValue _, Type t) when !t.IsValueType || Nullable.GetUnderlyingType(t) is not null
=> default,
Apply / Chat
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that the hardcoded list of types for null conversion is brittle and replaces it with a more robust, generic check, improving maintainability.
Medium
Improve generic list conversion logic
Refine the ArrayRemoteValue conversion logic to distinguish between interfaces/abstract classes and concrete list types, creating a List for the former and an instance of the specific type for the latter.
-(ArrayRemoteValue a, Type t) when t.IsGenericType && t.IsAssignableFrom(typeof(List<>).MakeGenericType(t.GetGenericArguments()[0]))+(ArrayRemoteValue a, Type t) when t.IsGenericType && (t.IsInterface || t.IsAbstract) && t.IsAssignableFrom(typeof(List<>).MakeGenericType(t.GetGenericArguments()[0]))
=> ConvertRemoteValuesToGenericList<TResult>(a.Value, typeof(List<>).MakeGenericType(t.GetGenericArguments()[0])),
+(ArrayRemoteValue a, Type t) when t.IsGenericType && !t.IsAbstract && typeof(System.Collections.IList).IsAssignableFrom(t)+ => ConvertRemoteValuesToGenericList<TResult>(a.Value, t),
Apply / Chat
Suggestion importance[1-10]: 6
__
Why: This is a good suggestion that refines the new collection conversion logic, making it more flexible by handling concrete list types differently from interfaces, which improves the feature's versatility.
Low
Use a smaller tolerance for float comparison
In the CanConvertToFloat test, reduce the assertion tolerance from 0.1f to a much smaller value like 1e-6f to more accurately verify the precision of the float conversion.
Why: The suggestion correctly points out that the test tolerance for float conversion is too loose and proposes a stricter value, which improves the quality and reliability of the new test suite.
Low
Learned best practice
Cache reflection-based conversions
Cache the constructed generic ConvertTo delegate per elementType (or avoid reflection entirely when possible) to prevent repeated MethodInfo.Invoke calls and reduce runtime overhead.
+private static readonly Dictionary<Type, Func<RemoteValue, object?>> ConvertToCache = new();+
private static TResult ConvertRemoteValuesToArray<TResult>(IEnumerable<RemoteValue>? remoteValues, Type elementType)
{
if (remoteValues is null)
{
return (TResult)(object)Array.CreateInstance(elementType, 0);
}
- var convertMethod = typeof(RemoteValue).GetMethod(nameof(ConvertTo))!.MakeGenericMethod(elementType);+ if (!ConvertToCache.TryGetValue(elementType, out var converter))+ {+ var method = typeof(RemoteValue).GetMethod(nameof(ConvertTo))!.MakeGenericMethod(elementType);+ converter = rv => method.Invoke(rv, null);+ ConvertToCache[elementType] = converter;+ }+
var items = remoteValues.ToList();
var array = Array.CreateInstance(elementType, items.Count);
for (int i = 0; i < items.Count; i++)
{
- var convertedItem = convertMethod.Invoke(items[i], null);- array.SetValue(convertedItem, i);+ array.SetValue(converter(items[i]), i);
}
return (TResult)(object)array;
}
Apply / Chat
Suggestion importance[1-10]: 6
__
Why:
Relevant best practice - Avoid per-item reflection (MethodInfo.Invoke) in hot-path conversions; prefer type-safe generic code or cached delegates for performance and reliability.
Low
Ensure tests are discoverable
Change the test class to public (and consider naming it RemoteValueConversionTests) so NUnit test discovery is robust across tooling and configurations.
[To ensure code accuracy, apply this suggestion manually]
Suggestion importance[1-10]: 5
__
Why:
Relevant best practice - Make test fixtures publicly discoverable and follow conventional naming to ensure consistent test discovery across runners.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Related Issues
Contribues to #16719
💥 What does this PR do?
🔧 Implementation Notes
💡 Additional Considerations
🔄 Types of changes