Conversation
Added the MLVScan.Core project to the solution.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughAdds cross-platform scanning infrastructure: new abstractions (IConfigManager, IPlatformEnvironment), platform implementations for MelonLoader and BepInEx (5.x and 6.x), shared plugin scanner/disabler bases, adapters/services (resolvers, loggers, config manager, report generator), hashing utility, and a multi-configuration build restructure. Changes
Sequence DiagramsequenceDiagram
participant Loader as Platform Loader
participant Patcher as Patcher/Entry
participant Logger as IScanLogger
participant Config as IConfigManager
participant Scanner as PluginScannerBase / AssemblyScanner
participant Disabler as PluginDisablerBase
participant Reporter as ReportGenerator
Loader->>Patcher: Initialize / Early hook
Patcher->>Logger: create logger instance
Patcher->>Config: LoadConfig()
Config-->>Patcher: ScanConfig
Patcher->>Scanner: ScanAllPlugins()
loop each plugin file
Scanner->>Scanner: enumerate files & compute hash
Scanner->>Config: IsHashWhitelisted(hash)?
alt not whitelisted
Scanner->>Scanner: run AssemblyScanner -> findings
Scanner-->>Patcher: accumulate findings
end
end
alt suspicious plugins found
Patcher->>Disabler: DisableSuspiciousPlugins(results)
Disabler->>Disabler: compute hash, rename/move -> DisabledPluginInfo
Disabler-->>Patcher: list of disabled plugins
Patcher->>Reporter: GenerateReports(disabledList, results)
Reporter-->>Patcher: report files produced
else none
Patcher->>Logger: log clean scan / auto-scan disabled
end
Patcher-->>Loader: Finish
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
MLVScan.csproj (1)
254-276:⚠️ Potential issue | 🟠 MajorRemove duplicate ItemGroup and ILRepack Target with hardcoded paths.
Lines 254-276 duplicate the configuration already defined in lines 192-231. The duplicate ILRepack target at line 266 also contains hardcoded local paths (
D:\SteamLibrary\...) that will fail on other machines. This appears to be leftover local development configuration that should be removed.🐛 Proposed fix: Remove duplicate sections
<!-- Import local.build.props if it exists for local dev overrides --> <Import Project="local.build.props" Condition="Exists('local.build.props')" /> - - <ItemGroup> - <!-- Use local project reference when UseLocalMLVScanCore is set --> - <ProjectReference Include="..\MLVScan.Core\MLVScan.Core.csproj" Condition="'$(UseLocalMLVScanCore)' == 'true'" /> - - <!-- Use NuGet package by default (when local.build.props doesn't exist or UseLocalMLVScanCore is not set) --> - <PackageReference Include="MLVScan.Core" Version="*" GeneratePathProperty="true" Condition="'$(UseLocalMLVScanCore)' != 'true'" /> - - <PackageReference Include="ILRepack" Version="2.0.44" GeneratePathProperty="true" /> - <PackageReference Include="Mono.Cecil" Version="0.11.6" /> - <PackageReference Include="System.Text.Json" Version="6.0.11" /> - </ItemGroup> - - <Target Name="ILRepack" AfterTargets="Build"> - <ItemGroup> - <InputAssemblies Include="$(TargetPath)" /> - <!-- Use local build output when UseLocalMLVScanCore is true --> - <InputAssemblies Include="..\MLVScan.Core\bin\Release\netstandard2.1\MLVScan.Core.dll" Condition="'$(UseLocalMLVScanCore)' == 'true'" /> - <!-- Use NuGet package when UseLocalMLVScanCore is false or not set --> - <InputAssemblies Include="$(PkgMLVScan_Core)\lib\netstandard2.1\MLVScan.Core.dll" Condition="'$(UseLocalMLVScanCore)' != 'true'" /> - </ItemGroup> - <Exec Command="$(PkgILRepack)\tools\ILRepack.exe /parallel /union /lib:"D:\SteamLibrary\steamapps\common\Schedule I_alternate\Schedule I_Data\Managed" /lib:"D:\SteamLibrary\steamapps\common\Schedule I_alternate\MelonLoader\net35" /out:"$(OutputPath)$(AssemblyName).merged.dll" "@(InputAssemblies, '" "')"" /> - <Copy SourceFiles="$(OutputPath)$(AssemblyName).merged.dll" DestinationFiles="$(TargetPath)" /> - </Target> - - <Target Name="PostBuild" AfterTargets="ILRepack"> - <Exec Command="COPY "$(TargetPath)" "D:\SteamLibrary\steamapps\common\Schedule I_alternate\Plugins"" /> - </Target> </Project>Move any local development overrides to
local.build.props(which is already imported and gitignored).
🤖 Fix all issues with AI agents
In `@BepInEx/5/BepInEx5Patcher.cs`:
- Around line 82-101: ScanAllPlugins() can return an empty list when auto-scan
is disabled, causing the current _logger.LogInfo("No suspicious plugins
detected.") to be misleading; update the control flow in the BepInEx5Patcher
scan block to check config.EnableAutoScan (or equivalent config property) before
logging that message: if EnableAutoScan is false, log an explicit message
indicating auto-scan is disabled (e.g., via _logger.LogInfo) instead of saying
"No suspicious plugins detected", otherwise keep the existing "No suspicious
plugins detected." branch; locate the logic around
pluginScanner.ScanAllPlugins(), pluginDisabler.DisableSuspiciousPlugins(),
reportGenerator.GenerateReports(), and _logger to implement the conditional
logging.
In `@BepInEx/6/IL2CPP/BepInEx6IL2CppPatcher.cs`:
- Around line 71-91: The log "No suspicious plugins detected." is misleading
when ScanAllPlugins() returned empty because auto-scan may be disabled; update
the logic around pluginScanner.ScanAllPlugins() to check the configuration flag
(config.EnableAutoScan) before emitting _logger.LogInfo and either skip the
message or log a clearer message indicating auto-scan is disabled; locate code
using pluginScanner.ScanAllPlugins(), the subsequent conditional that logs via
_logger.LogInfo, and adjust it to branch on config.EnableAutoScan (or change the
log text) so the message accurately reflects whether a scan ran or auto-scan is
turned off.
In `@BepInEx/6/Mono/BepInEx6MonoPatcher.cs`:
- Around line 71-91: The log "No suspicious plugins detected." can be misleading
because pluginScanner.ScanAllPlugins() returns an empty list when
config.EnableAutoScan is false; update BepInEx6MonoPatcher to check
config.EnableAutoScan before interpreting an empty scan as "no suspicious
plugins": if EnableAutoScan is false, log a message that auto-scan is disabled
(or skip the message) instead of calling _logger.LogInfo; otherwise proceed to
call pluginScanner.ScanAllPlugins(), then run
pluginDisabler.DisableSuspiciousPlugins and reportGenerator.GenerateReports as
before and log the blocked-count warnings.
In `@BepInEx/BepInExReportGenerator.cs`:
- Around line 84-90: The default call-chain prefix string is missing a closing
bracket; update the switch/default return from "[???" to "[???]" wherever the
prefix is chosen for CallChainNodeType (the switch that assigns the variable
named prefix based on CallChainNodeType, e.g., in BepInExReportGenerator where
EntryPoint/IntermediateCall/SuspiciousDeclaration are handled); ensure both
occurrences of that switch/prefix assignment are corrected so the default case
returns "[???]".
In `@MelonLoader/MelonEnvironment.cs`:
- Around line 53-69: The ManagedDirectory getter in MelonEnvironment currently
hardcodes "Schedule I_Data"; update it to detect the Unity data folder
dynamically by scanning directories under _gameRoot for any folder name that
ends with "_Data" and using Path.Combine(foundDataFolderPath, "Managed") as
managedPath; if no "_Data" folder is found, fall back to the existing Il2Cpp
path logic (the il2cppPath variable and Directory.Exists check) and finally
return string.Empty if neither exists. Ensure you reference and update the
ManagedDirectory property and the managedPath/il2cppPath resolution logic so the
detection works for arbitrary GameName_Data folders.
In `@MelonLoader/MelonLoaderPlugin.cs`:
- Around line 355-362: The switch expression that computes the prefix (variable
prefix using node.NodeType and CallChainNodeType) has a typo in the default arm
("[???") missing the closing bracket; update the default case to the intended
string "[???]" so writer.WriteLine($" {prefix} {node.Location}") prints the
correct bracketed tag.
- Around line 231-238: The switch expression that sets prefix for
CallChainNodeType has a typo in the default arm returning "[???"; update the
default arm to return "[???]" so the string is properly closed—modify the switch
in MelonLoaderPlugin.cs where prefix is assigned (the switch over
CallChainNodeType with arms EntryPoint, IntermediateCall, SuspiciousDeclaration)
and ensure LoggerInstance.Msg still uses the corrected prefix with
node.Location.
In `@MelonLoader/MelonLoaderServiceFactory.cs`:
- Around line 41-44: CreateConfigManager currently returns _configManager which
may be null if MelonConfigManager construction failed; update
CreateConfigManager to validate _configManager and throw a clear
InvalidOperationException (or a more specific exception) when it's null rather
than returning null, e.g., check _configManager inside CreateConfigManager and
throw with a descriptive message like "Configuration manager unavailable" so
callers get a fail-fast error; alternatively, if you prefer nullable returns,
add XML documentation to CreateConfigManager declaring the return may be null
(and update callers) — reference CreateConfigManager and the _configManager
field to locate the change.
In `@MelonLoader/MelonPluginScanner.cs`:
- Around line 30-37: GetScanDirectories currently uses the static
MelonEnvironment.GameRootDirectory while other methods (e.g., IsSelfAssembly)
use the injected _environment, which is inconsistent; change GetScanDirectories
to use the injected _environment.GameRootDirectory when joining with each entry
from Config.ScanDirectories so all environment access consistently uses the
_environment instance (update references to MelonEnvironment.GameRootDirectory
to _environment.GameRootDirectory and keep yielding
Path.Combine(_environment.GameRootDirectory, scanDir)).
In `@MLVScan.sln`:
- Around line 68-91: The solution-level configuration entries currently map
MLVScan.Core to Debug|Any CPU for all MelonLoader/BepInEx/BepInEx6*
configurations (see entries like
{CBE47B78-2460-4C71-B9E6-CBEC26AAED73}.MelonLoader|Any CPU.ActiveCfg and the
similar BepInEx/BepInEx6* lines), causing Debug binaries to be produced when
building those solution configs; fix by changing those mappings to Release|Any
CPU (or add matching Release configurations in the MLVScan.Core project and map
to them) so the solution builds produce release-like Core assemblies for the
MelonLoader/BepInEx/BepInEx6* solution configurations.
In `@README.md`:
- Around line 35-45: Update the "Key Differences" section in README.md to
explicitly document BepInEx version-specific details: state whether BepInEx 5.x
and 6.x share the same configuration file path and report directory (confirm if
both use "BepInEx/config/MLVScan.json" and "BepInEx/MLVScan/Reports/" or note
differences), and add a brief bullet explaining any runtime differences between
BepInEx 6.x IL2CPP and Mono builds (e.g., installer/loader behavior, plugin
compatibility, and whether config/report locations change or plugins require
different packaging). Keep the existing MelonLoader entries
(MelonPreferences.cfg and UserData/MLVScan/Reports/) and mirror the format used
now, adding explicit version labels "BepInEx 5.x" and "BepInEx 6.x (IL2CPP /
Mono)" with one-line notes about behavioral differences if present.
- Line 9: Update the README line that currently reads "Supports **MelonLoader**
and **BepInEx 5.x**." to explicitly include BepInEx 6.x and its variants; edit
that string to something like "Supports MelonLoader, BepInEx 5.x, and BepInEx
6.x (IL2CPP and Mono)" and, if present elsewhere in README, add a short note or
bullet explaining the BepInEx 6.x support scope (IL2CPP and Mono) so users can
find compatibility information for the newly added files/features.
- Around line 20-23: Update the README Quick Start to document BepInEx 6.x
support and the DLL variants: change or expand the "For BepInEx 5.x" heading to
"For BepInEx 5.x / 6.x" (or add a separate "For BepInEx 6.x" section) and add
clear guidance about which DLL to download — `MLVScan.BepInEx.dll` for BepInEx
5.x, and the appropriate 6.x variant for IL2CPP or Mono (list the IL2CPP and
Mono DLL names used in the repo). Also update the supported platforms list at
the top (line ~9) to include "BepInEx 6.x". Ensure the install step still shows
dropping the DLL into `BepInEx/patchers/` and call out selecting the correct DLL
for your runtime (IL2CPP vs Mono).
In `@Services/DeveloperReportGenerator.cs`:
- Around line 213-219: The switch that sets prefix for CallChainNodeType returns
a malformed default string "[???" — update the default arm in the switch
expression (the code that assigns prefix) to return the corrected string "[???]"
(i.e., add the missing closing bracket) so the default case matches the
formatting of the other branches.
- Around line 93-99: The switch that assigns the local variable prefix based on
node.NodeType has a typo in the default arm returning "[???" (missing the
closing bracket); update the default case for the switch (the "_" arm) to return
"[???]" so the output is properly formatted—look for the prefix assignment using
node.NodeType and CallChainNodeType in DeveloperReportGenerator and correct the
default string literal.
In `@Services/PluginScannerBase.cs`:
- Around line 132-136: The use of results.Add(filePath, actualFindings) in
PluginScannerBase (when evaluating actualFindings.Count against
Config.SuspiciousThreshold) can throw ArgumentException if the same filePath is
encountered multiple times (overlapping dirs/symlinks); change the insertion to
a safe assignment such as using the Dictionary indexer (results[filePath] =
actualFindings) or results.TryAdd(filePath, actualFindings) so duplicate keys
are handled without exceptions, keeping the rest of the logic (Logger.Warning
and actualFindings handling) intact; check callers like GetScanDirectories() for
overlapping paths if you want to de-duplicate earlier.
🧹 Nitpick comments (9)
Services/HashUtility.cs (1)
18-42: Consider using a result pattern to distinguish success from failure.Returning error messages as strings from a method expected to return hashes is error-prone. Callers must remember to validate with
IsValidHash()after every call, or risk treating an error message as a hash value.♻️ Suggested alternative using out parameter
-public static string CalculateFileHash(string filePath) +public static bool TryCalculateFileHash(string filePath, out string hash, out string error) { + hash = null; + error = null; try { if (!File.Exists(filePath)) - return $"File not found: {filePath}"; + { + error = $"File not found: {filePath}"; + return false; + } using var sha256 = SHA256.Create(); using var stream = new FileStream(filePath, FileMode.Open, FileAccess.Read, FileShare.ReadWrite); - var hash = sha256.ComputeHash(stream); - return BitConverter.ToString(hash).Replace("-", "").ToLowerInvariant(); + var hashBytes = sha256.ComputeHash(stream); + hash = BitConverter.ToString(hashBytes).Replace("-", "").ToLowerInvariant(); + return true; } catch (UnauthorizedAccessException) { - return "Access denied"; + error = "Access denied"; + return false; } // ... similar for other exceptions }BepInEx/BepInExConfigManager.cs (1)
116-118: Redundant case-insensitive comparison after lowercase conversion.The hash is converted to lowercase via
ToLowerInvariant(), then compared usingStringComparer.OrdinalIgnoreCase. Since the hash is already lowercase, the case-insensitive comparer is unnecessary. TheMelonConfigManagerimplementation (lines 154-160 in relevant snippets) has the same redundancy.🔧 Simplify comparison
public bool IsHashWhitelisted(string hash) { if (string.IsNullOrWhiteSpace(hash)) return false; - return _config.WhitelistedHashes.Contains( - hash.ToLowerInvariant(), - StringComparer.OrdinalIgnoreCase); + return _config.WhitelistedHashes.Contains(hash.ToLowerInvariant()); }Note: This assumes
WhitelistedHashesis always stored in lowercase (whichSetWhitelistedHashesensures). If external sources can populateWhitelistedHasheswithout normalization, keep the case-insensitive comparer.MelonLoader/MelonEnvironment.cs (1)
33-51: Side effects in property getters.Both
DataDirectoryandReportsDirectorycreate directories when accessed. While this ensures directories exist when needed, it can cause unexpected behavior if callers only want to check the path without creating it. This pattern is acceptable for this use case but worth documenting in the property's XML summary.Services/DeveloperReportGenerator.cs (1)
253-260: Inconsistent placeholder format.The
DataFlowNodeTypedefault case uses"[????]"(4 question marks) whileCallChainNodeTypeuses"[???]"(3 question marks, once fixed). Consider using a consistent placeholder format across both switch expressions.Services/PluginScannerBase.cs (1)
128-130: Consider extracting magic string to a constant.The filter condition
f.Location != "Assembly scanning"uses a magic string. Consider extracting this to a named constant for maintainability and to avoid typos if used elsewhere.Services/PluginDisablerBase.cs (1)
121-137: Consider validating hash before storing inDisabledPluginInfo.
HashUtility.CalculateFileHashreturns error strings (e.g.,"File not found: ...","Access denied") on failure rather than throwing. The current code stores whatever is returned asFileHash, which could lead to error messages being reported as hashes in downstream reports.🛡️ Proposed fix to validate hash
protected virtual DisabledPluginInfo DisablePlugin(string pluginPath) { var fileHash = HashUtility.CalculateFileHash(pluginPath); + if (!HashUtility.IsValidHash(fileHash)) + { + Logger.Warning($"Could not compute hash for {Path.GetFileName(pluginPath)}: {fileHash}"); + fileHash = "hash-unavailable"; + } var disabledPath = GetDisabledPath(pluginPath);MelonLoader/MelonLoaderPlugin.cs (1)
205-209: Minor formatting:elseon separate line without precedingifblock visible.The
elseat line 209 appears disconnected due to theifblock ending with developer mode reporting. While syntactically correct, the indentation/spacing is unusual.✨ Suggested formatting fix
if (isDeveloperMode && _developerReportGenerator != null) { _developerReportGenerator.GenerateConsoleReport(modName, actualFindings); } - else + else {BepInEx/BepInExPluginScanner.cs (1)
39-55: Consider logging exceptions inIsSelfAssemblycatch block.The empty catch block silently swallows all exceptions. While returning
falseis a safe default, logging the exception would help diagnose path-related issues during development or debugging.✨ Optional: Add debug logging
protected override bool IsSelfAssembly(string filePath) { try { var selfPath = _environment.SelfAssemblyPath; if (string.IsNullOrEmpty(selfPath)) return false; return Path.GetFullPath(filePath).Equals( Path.GetFullPath(selfPath), StringComparison.OrdinalIgnoreCase); } - catch + catch (Exception ex) { + // Path normalization can fail for invalid paths + Logger.Debug($"IsSelfAssembly check failed for {filePath}: {ex.Message}"); return false; } }BepInEx/BepInExEnvironment.cs (1)
25-28: Minor:PluginDirectoriesallocates a new array on each access.This is a low-impact concern since this property is unlikely to be called frequently in hot paths. Consider caching if profiling shows it's an issue.
fix: Inaccurate logging Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@BepInEx/5/BepInEx5Patcher.cs`:
- Around line 98-110: There is an extra closing brace in BepInEx5Patcher.cs that
breaks the try block and places _logger.LogInfo("MLVScan preloader scan
complete."); outside its intended scope; remove the stray `}` immediately before
that log line so the conditional blocks (the else-if on config.EnableAutoScan
and the surrounding try) remain balanced, keeping the final _logger.LogInfo call
inside the try block and preserving proper indentation in the BepInEx5Patcher
class.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Refactored MLVScan.csproj to improve ILRepack library handling and removed redundant build targets. Fixed a minor typo in MelonLoaderPlugin.cs. Expanded and clarified README instructions for BepInEx 5.x and 6.x usage and installation.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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)
MelonLoader/MelonLoaderPlugin.cs (1)
103-135:⚠️ Potential issue | 🟡 MinorAvoid “No suspicious mods found” when auto-scan is off.
Whenforceis false and auto-scan is disabled,ScanAllPluginsreturns empty and this method logs “No suspicious mods found,” which can be misleading. Consider branching on config/force before logging.🔧 Suggested fix
- else - { - LoggerInstance.Msg("No suspicious mods found"); - } + else if (!force && _configManager?.Config?.EnableAutoScan == false) + { + LoggerInstance.Msg("Automatic scanning is disabled in configuration."); + } + else + { + LoggerInstance.Msg("No suspicious mods found"); + }
🤖 Fix all issues with AI agents
In @.github/workflows/build.yml:
- Around line 25-33: The MelonLoader CI build fails because MLVScan.csproj
expects MSBuild properties $(MelonLoaderPath) and $(GameManagedPath); update the
"Build MelonLoader" step so dotnet build supplies those properties (e.g. add
-p:MelonLoaderPath=<path> -p:GameManagedPath=<path> or map them from workflow
environment variables/secrets) or skip/gate the "Build MelonLoader" step until
those dependencies are provisioned; target symbols: the "Build MelonLoader"
workflow step and MLVScan.csproj, and the MSBuild properties MelonLoaderPath and
GameManagedPath.
In `@BepInEx/5/BepInEx5Patcher.cs`:
- Around line 82-106: The code currently calls pluginScanner.ScanAllPlugins()
unconditionally; change the control flow to check config.EnableAutoScan before
invoking ScanAllPlugins so scanning is skipped when auto-scan is disabled.
Concretely, move the config.EnableAutoScan check above the
pluginScanner.ScanAllPlugins() call (or return/skip early when false), so
ScanAllPlugins(), pluginDisabler.DisableSuspiciousPlugins(scanResults) and
reportGenerator.GenerateReports(disabledPlugins, scanResults) only run when
config.EnableAutoScan is true; keep the existing logging paths
(_logger.LogInfo/_logger.LogWarning) but ensure the "Automatic scanning is
disabled" message is emitted when EnableAutoScan is false.
In `@MelonLoader/MelonLoaderServiceFactory.cs`:
- Around line 68-77: CreatePluginScanner is passing a potentially null
_configManager into the MelonPluginScanner constructor which leads
PluginScannerBase to throw ArgumentNullException; ensure you never pass null by
checking _configManager before instantiation and using a safe wrapper or
fallback that implements the same contract (e.g., a fallback
MelonConfigManagerWrapper built from _fallbackConfig) or pass only config values
instead of the null _configManager; update CreatePluginScanner to use the safe
wrapper/fallback when _configManager is null so MelonPluginScanner and
PluginScannerBase always receive a non-null config manager.
In `@MLVScan.csproj`:
- Around line 23-51: The PropertyGroup defs overwrite existing compiler
constants; update each DefineConstants assignment (e.g., the entries in the
PropertyGroup for MelonLoader, BepInEx, BepInEx6Mono, and BepInEx6IL2CPP) to
preserve defaults by appending $(DefineConstants) so platform flags are added
rather than replacing existing symbols like DEBUG/TRACE/framework constants.
In `@Services/PluginScannerBase.cs`:
- Around line 85-101: ScanDirectory currently uses Directory.GetFiles(...,
SearchOption.AllDirectories) which will throw on inaccessible nested folders and
abort the entire scan; change it to use Directory.EnumerateFiles with an
EnumerationOptions instance (RecurseSubdirectories = true, IgnoreInaccessible =
true) so inaccessible subdirectories are skipped and scanning continues, then
iterate the resulting enumerable and call ScanSingleFile for each pluginFile
while keeping the existing try/catch and Logger.Error/Info calls intact.
🧹 Nitpick comments (3)
README.md (1)
20-23: Consider adding BepInEx 6.x Quick Start instructions.The Quick Start section covers MelonLoader and BepInEx 5.x, but BepInEx 6.x is only mentioned in Key Differences (lines 48-53). Since 6.x uses
[PatcherPlugin]attribute-based packaging and may have different DLL names, users might benefit from explicit Quick Start instructions for BepInEx 6.x variants (IL2CPP/Mono).If the installation process is identical to 5.x, consider adding a note like: "For BepInEx 6.x, follow the same steps using
MLVScan.BepInEx6Mono.dllorMLVScan.BepInEx6IL2CPP.dll."BepInEx/5/BepInEx5Patcher.cs (1)
22-34: Consider documenting the source of default whitelist hashes.The hardcoded SHA256 hashes are described as "BepInEx ecosystem - known safe plugins" but there's no indication of which specific plugins they represent. This makes future maintenance difficult when plugins are updated.
Consider adding inline comments identifying each hash, or referencing an external document/wiki that maps hashes to plugin names and versions.
MLVScan.csproj (1)
192-198: Pin MLVScan.Core version for reproducible builds.
Version="*"makes restores non-deterministic and can break ILRepack inputs. Recommend pinning to a known version (or a property).🔧 Suggested change
- <PackageReference Include="MLVScan.Core" Version="*" GeneratePathProperty="true" /> + <PackageReference Include="MLVScan.Core" Version="$(MLVScanCoreVersion)" GeneratePathProperty="true" />Please verify the latest stable MLVScan.Core version you intend to ship and set
MLVScanCoreVersionaccordingly.
| // Scan all plugins | ||
| var scanResults = pluginScanner.ScanAllPlugins(); | ||
|
|
||
| if (scanResults.Count > 0) | ||
| { | ||
| // Disable suspicious plugins | ||
| var disabledPlugins = pluginDisabler.DisableSuspiciousPlugins(scanResults); | ||
|
|
||
| // Generate reports for disabled plugins | ||
| if (disabledPlugins.Count > 0) | ||
| { | ||
| reportGenerator.GenerateReports(disabledPlugins, scanResults); | ||
|
|
||
| _logger.LogWarning($"MLVScan blocked {disabledPlugins.Count} suspicious plugin(s)."); | ||
| _logger.LogWarning("Check BepInEx/MLVScan/Reports/ for details."); | ||
| } | ||
| } | ||
| else if (!config.EnableAutoScan) | ||
| { | ||
| _logger.LogInfo("Automatic scanning is disabled in configuration."); | ||
| } | ||
| else | ||
| { | ||
| _logger.LogInfo("No suspicious plugins detected."); | ||
| } |
There was a problem hiding this comment.
Inconsistent EnableAutoScan behavior compared to BepInEx6MonoPatcher.
This patcher calls ScanAllPlugins() unconditionally (line 83), then checks config.EnableAutoScan in the else-if branch (line 99). However, BepInEx6MonoPatcher checks EnableAutoScan before scanning (see relevant snippet lines 70-74).
Current behavior here: scanning happens even when EnableAutoScan is false, wasting resources.
🔧 Suggested fix to match BepInEx6MonoPatcher pattern
- // Scan all plugins
- var scanResults = pluginScanner.ScanAllPlugins();
-
- if (scanResults.Count > 0)
+ if (!config.EnableAutoScan)
+ {
+ _logger.LogInfo("Auto-scan is disabled. Skipping plugin scan.");
+ }
+ else
{
- // Disable suspicious plugins
- var disabledPlugins = pluginDisabler.DisableSuspiciousPlugins(scanResults);
+ var scanResults = pluginScanner.ScanAllPlugins();
- // Generate reports for disabled plugins
- if (disabledPlugins.Count > 0)
+ if (scanResults.Count > 0)
{
- reportGenerator.GenerateReports(disabledPlugins, scanResults);
+ var disabledPlugins = pluginDisabler.DisableSuspiciousPlugins(scanResults);
- _logger.LogWarning($"MLVScan blocked {disabledPlugins.Count} suspicious plugin(s).");
- _logger.LogWarning("Check BepInEx/MLVScan/Reports/ for details.");
+ if (disabledPlugins.Count > 0)
+ {
+ reportGenerator.GenerateReports(disabledPlugins, scanResults);
+
+ _logger.LogWarning($"MLVScan blocked {disabledPlugins.Count} suspicious plugin(s).");
+ _logger.LogWarning("Check BepInEx/MLVScan/Reports/ for details.");
+ }
+ }
+ else
+ {
+ _logger.LogInfo("No suspicious plugins detected.");
}
}
- else if (!config.EnableAutoScan)
- {
- _logger.LogInfo("Automatic scanning is disabled in configuration.");
- }
- else
- {
- _logger.LogInfo("No suspicious plugins detected.");
- }🤖 Prompt for AI Agents
In `@BepInEx/5/BepInEx5Patcher.cs` around lines 82 - 106, The code currently calls
pluginScanner.ScanAllPlugins() unconditionally; change the control flow to check
config.EnableAutoScan before invoking ScanAllPlugins so scanning is skipped when
auto-scan is disabled. Concretely, move the config.EnableAutoScan check above
the pluginScanner.ScanAllPlugins() call (or return/skip early when false), so
ScanAllPlugins(), pluginDisabler.DisableSuspiciousPlugins(scanResults) and
reportGenerator.GenerateReports(disabledPlugins, scanResults) only run when
config.EnableAutoScan is true; keep the existing logging paths
(_logger.LogInfo/_logger.LogWarning) but ensure the "Automatic scanning is
disabled" message is emitted when EnableAutoScan is false.
| public MelonPluginScanner CreatePluginScanner() | ||
| { | ||
| var config = _configManager?.Config ?? _fallbackConfig; | ||
| return new MelonPluginScanner( | ||
| _scanLogger, | ||
| _resolverProvider, | ||
| config, | ||
| _configManager, | ||
| _environment); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "MelonLoaderServiceFactory.cs" -type fRepository: ifBars/MLVScan
Length of output: 100
🏁 Script executed:
head -n 80 $(find . -name "MelonLoaderServiceFactory.cs" -type f | head -1)Repository: ifBars/MLVScan
Length of output: 2901
🏁 Script executed:
find . -name "MelonPluginScanner.cs" -type fRepository: ifBars/MLVScan
Length of output: 93
🏁 Script executed:
cat -n $(find . -name "MelonPluginScanner.cs" -type f | head -1) | head -n 100Repository: ifBars/MLVScan
Length of output: 4468
🏁 Script executed:
find . -name "PluginScannerBase.cs" -type fRepository: ifBars/MLVScan
Length of output: 89
🏁 Script executed:
cat -n $(find . -name "PluginScannerBase.cs" -type f | head -1)Repository: ifBars/MLVScan
Length of output: 6068
_configManager can be null when passed to MelonPluginScanner, causing an ArgumentNullException.
If MelonConfigManager initialization fails in the constructor (lines 33-38), _configManager remains null. When CreatePluginScanner() passes this null value on line 75, PluginScannerBase throws ArgumentNullException on line 32 of PluginScannerBase.cs. This should be guarded before instantiation, or a fallback configuration wrapper should be provided instead of passing null directly.
🤖 Prompt for AI Agents
In `@MelonLoader/MelonLoaderServiceFactory.cs` around lines 68 - 77,
CreatePluginScanner is passing a potentially null _configManager into the
MelonPluginScanner constructor which leads PluginScannerBase to throw
ArgumentNullException; ensure you never pass null by checking _configManager
before instantiation and using a safe wrapper or fallback that implements the
same contract (e.g., a fallback MelonConfigManagerWrapper built from
_fallbackConfig) or pass only config values instead of the null _configManager;
update CreatePluginScanner to use the safe wrapper/fallback when _configManager
is null so MelonPluginScanner and PluginScannerBase always receive a non-null
config manager.
| <PropertyGroup Condition="'$(Configuration)'=='MelonLoader'"> | ||
| <DefineConstants>MELONLOADER</DefineConstants> | ||
| <AssemblyName>MLVScan.MelonLoader</AssemblyName> | ||
| <LangVersion>latest</LangVersion> | ||
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <NoWarn>$(NoWarn);MSB3277</NoWarn> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- ============================================ --> | ||
| <!-- BepInEx Configuration --> | ||
| <!-- ============================================ --> | ||
| <PropertyGroup Condition="'$(Configuration)'=='BepInEx'"> | ||
| <DefineConstants>BEPINEX</DefineConstants> | ||
| <AssemblyName>MLVScan.BepInEx</AssemblyName> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- ============================================ --> | ||
| <!-- BepInEx 6.x Mono Configuration --> | ||
| <!-- ============================================ --> | ||
| <PropertyGroup Condition="'$(Configuration)'=='BepInEx6Mono'"> | ||
| <DefineConstants>BEPINEX;BEPINEX6;BEPINEX6_MONO</DefineConstants> | ||
| <AssemblyName>MLVScan.BepInEx6.Mono</AssemblyName> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- ============================================ --> | ||
| <!-- BepInEx 6.x IL2CPP Configuration --> | ||
| <!-- ============================================ --> | ||
| <PropertyGroup Condition="'$(Configuration)'=='BepInEx6IL2CPP'"> | ||
| <TargetFramework>net6.0</TargetFramework> | ||
| <DefineConstants>BEPINEX;BEPINEX6;BEPINEX6_IL2CPP</DefineConstants> | ||
| <AssemblyName>MLVScan.BepInEx6.IL2CPP</AssemblyName> |
There was a problem hiding this comment.
Preserve default compiler constants when adding platform defines.
Setting DefineConstants without $(DefineConstants) overwrites defaults like DEBUG, TRACE, and framework constants. This can change compilation behavior across configs.
🔧 Suggested fix
- <DefineConstants>MELONLOADER</DefineConstants>
+ <DefineConstants>$(DefineConstants);MELONLOADER</DefineConstants>
...
- <DefineConstants>BEPINEX</DefineConstants>
+ <DefineConstants>$(DefineConstants);BEPINEX</DefineConstants>
...
- <DefineConstants>BEPINEX;BEPINEX6;BEPINEX6_MONO</DefineConstants>
+ <DefineConstants>$(DefineConstants);BEPINEX;BEPINEX6;BEPINEX6_MONO</DefineConstants>
...
- <DefineConstants>BEPINEX;BEPINEX6;BEPINEX6_IL2CPP</DefineConstants>
+ <DefineConstants>$(DefineConstants);BEPINEX;BEPINEX6;BEPINEX6_IL2CPP</DefineConstants>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <PropertyGroup Condition="'$(Configuration)'=='MelonLoader'"> | |
| <DefineConstants>MELONLOADER</DefineConstants> | |
| <AssemblyName>MLVScan.MelonLoader</AssemblyName> | |
| <LangVersion>latest</LangVersion> | |
| <ImplicitUsings>enable</ImplicitUsings> | |
| <NoWarn>$(NoWarn);MSB3277</NoWarn> | |
| </PropertyGroup> | |
| <!-- ============================================ --> | |
| <!-- BepInEx Configuration --> | |
| <!-- ============================================ --> | |
| <PropertyGroup Condition="'$(Configuration)'=='BepInEx'"> | |
| <DefineConstants>BEPINEX</DefineConstants> | |
| <AssemblyName>MLVScan.BepInEx</AssemblyName> | |
| </PropertyGroup> | |
| <!-- ============================================ --> | |
| <!-- BepInEx 6.x Mono Configuration --> | |
| <!-- ============================================ --> | |
| <PropertyGroup Condition="'$(Configuration)'=='BepInEx6Mono'"> | |
| <DefineConstants>BEPINEX;BEPINEX6;BEPINEX6_MONO</DefineConstants> | |
| <AssemblyName>MLVScan.BepInEx6.Mono</AssemblyName> | |
| </PropertyGroup> | |
| <!-- ============================================ --> | |
| <!-- BepInEx 6.x IL2CPP Configuration --> | |
| <!-- ============================================ --> | |
| <PropertyGroup Condition="'$(Configuration)'=='BepInEx6IL2CPP'"> | |
| <TargetFramework>net6.0</TargetFramework> | |
| <DefineConstants>BEPINEX;BEPINEX6;BEPINEX6_IL2CPP</DefineConstants> | |
| <AssemblyName>MLVScan.BepInEx6.IL2CPP</AssemblyName> | |
| <PropertyGroup Condition="'$(Configuration)'=='MelonLoader'"> | |
| <DefineConstants>$(DefineConstants);MELONLOADER</DefineConstants> | |
| <AssemblyName>MLVScan.MelonLoader</AssemblyName> | |
| <NoWarn>$(NoWarn);MSB3277</NoWarn> | |
| </PropertyGroup> | |
| <!-- ============================================ --> | |
| <!-- BepInEx Configuration --> | |
| <!-- ============================================ --> | |
| <PropertyGroup Condition="'$(Configuration)'=='BepInEx'"> | |
| <DefineConstants>$(DefineConstants);BEPINEX</DefineConstants> | |
| <AssemblyName>MLVScan.BepInEx</AssemblyName> | |
| </PropertyGroup> | |
| <!-- ============================================ --> | |
| <!-- BepInEx 6.x Mono Configuration --> | |
| <!-- ============================================ --> | |
| <PropertyGroup Condition="'$(Configuration)'=='BepInEx6Mono'"> | |
| <DefineConstants>$(DefineConstants);BEPINEX;BEPINEX6;BEPINEX6_MONO</DefineConstants> | |
| <AssemblyName>MLVScan.BepInEx6.Mono</AssemblyName> | |
| </PropertyGroup> | |
| <!-- ============================================ --> | |
| <!-- BepInEx 6.x IL2CPP Configuration --> | |
| <!-- ============================================ --> | |
| <PropertyGroup Condition="'$(Configuration)'=='BepInEx6IL2CPP'"> | |
| <TargetFramework>net6.0</TargetFramework> | |
| <DefineConstants>$(DefineConstants);BEPINEX;BEPINEX6;BEPINEX6_IL2CPP</DefineConstants> | |
| <AssemblyName>MLVScan.BepInEx6.IL2CPP</AssemblyName> |
🤖 Prompt for AI Agents
In `@MLVScan.csproj` around lines 23 - 51, The PropertyGroup defs overwrite
existing compiler constants; update each DefineConstants assignment (e.g., the
entries in the PropertyGroup for MelonLoader, BepInEx, BepInEx6Mono, and
BepInEx6IL2CPP) to preserve defaults by appending $(DefineConstants) so platform
flags are added rather than replacing existing symbols like
DEBUG/TRACE/framework constants.
| protected virtual void ScanDirectory(string directoryPath, Dictionary<string, List<ScanFinding>> results) | ||
| { | ||
| var pluginFiles = Directory.GetFiles(directoryPath, "*.dll", SearchOption.AllDirectories); | ||
| Logger.Info($"Found {pluginFiles.Length} plugin files in {directoryPath}"); | ||
|
|
||
| foreach (var pluginFile in pluginFiles) | ||
| { | ||
| try | ||
| { | ||
| ScanSingleFile(pluginFile, results); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Logger.Error($"Error scanning {Path.GetFileName(pluginFile)}: {ex.Message}"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*.csproj" -type f | head -20Repository: ifBars/MLVScan
Length of output: 74
🏁 Script executed:
cat -n ./MLVScan.csproj | head -50Repository: ifBars/MLVScan
Length of output: 2684
🌐 Web query:
EnumerationOptions IgnoreInaccessible .NET framework compatibility netstandard2.1
💡 Result:
EnumerationOptions.IgnoreInaccessibleis part ofSystem.IO.EnumerationOptionsand is available when targeting .NET Standard 2.1 (and implemented by .NET Core 2.1+ / .NET 5+) [1][2].- .NET Framework cannot consume
netstandard2.1libraries, so anything you build fornetstandard2.1(including code that usesIgnoreInaccessible) is not compatible with .NET Framework projects [3]. - In practice, the
Directory.*/DirectoryInfo.*overloads that takeEnumerationOptionsare available on modern .NET, but not on .NET Framework (so you can’t “just use it” there even if you compile conditionally) [4].
What to do if you need .NET Framework compatibility
- Target
netstandard2.0(or multi-targetnetstandard2.0;net6.0+) and, on thenetstandard2.0/ .NET Framework side, fall back toSearchOption+ manual recursion / try-catch forUnauthorizedAccessException, etc. [3][4].
Sources:
[1] https://apisof.net/catalog/2fee51443e60e4d55edeb6b745569790
[2] https://gist.github.com/JeremyKuhne/42b951b427e3db791e3e965f5054dae2
[3] https://devblogs.microsoft.com/dotnet/announcing-net-core-3-preview-3/
[4] https://stackoverflow.com/questions/62919724/does-vb-net-support-enumerationoptions
🌐 Web query:
Directory.GetFiles UnauthorizedAccessException inaccessible directory behavior
💡 Result:
Directory.GetFiles(path, pattern, SearchOption.AllDirectories) will throw UnauthorizedAccessException and abort the entire call as soon as it hits the first subdirectory it can’t enter (e.g., C:\System Volume Information). Catching around GetFiles doesn’t help you “get the rest”, because no results are returned once the exception is thrown. [2][3]
If you’re on modern .NET (Core 2.1+ / .NET 5+)
Use the overload that takes EnumerationOptions and set IgnoreInaccessible = true (and recurse via RecurseSubdirectories = true):
var opts = new EnumerationOptions
{
RecurseSubdirectories = true,
IgnoreInaccessible = true,
};
foreach (var file in Directory.EnumerateFiles(root, "*", opts))
{
// process
}IgnoreInaccessible is specifically meant to skip files/directories when access is denied. [1]
If you’re on .NET Framework (no EnumerationOptions)
You generally need to do your own recursion and try/catch per directory, skipping the ones that throw, because the built-in recursive GetFiles(...AllDirectories) can’t continue past access-denied folders. [3]
Sources: Microsoft EnumerationOptions.IgnoreInaccessible docs [1]; Stack Overflow discussions of GetFiles(...AllDirectories) failing on access-denied subfolders [2][3].
Handle inaccessible subdirectories to prevent scan abort.
Directory.GetFiles(..., SearchOption.AllDirectories) throws UnauthorizedAccessException or IOException when encountering any inaccessible nested directory, aborting the entire scan with no partial results. This leaves plugins undiscovered if any directory in the tree is inaccessible (e.g., system protected folders).
Use Directory.EnumerateFiles with EnumerationOptions { RecurseSubdirectories = true, IgnoreInaccessible = true } to skip inaccessible directories and continue scanning:
Suggested fix
- var pluginFiles = Directory.GetFiles(directoryPath, "*.dll", SearchOption.AllDirectories);
+ var pluginFiles = Directory.EnumerateFiles(
+ directoryPath,
+ "*.dll",
+ new EnumerationOptions { RecurseSubdirectories = true, IgnoreInaccessible = true })
+ .ToArray();
Logger.Info($"Found {pluginFiles.Length} plugin files in {directoryPath}");This API is supported on both netstandard2.1 and net6.0 (your target frameworks).
🤖 Prompt for AI Agents
In `@Services/PluginScannerBase.cs` around lines 85 - 101, ScanDirectory currently
uses Directory.GetFiles(..., SearchOption.AllDirectories) which will throw on
inaccessible nested folders and abort the entire scan; change it to use
Directory.EnumerateFiles with an EnumerationOptions instance
(RecurseSubdirectories = true, IgnoreInaccessible = true) so inaccessible
subdirectories are skipped and scanning continues, then iterate the resulting
enumerable and call ScanSingleFile for each pluginFile while keeping the
existing try/catch and Logger.Error/Info calls intact.
1 participant
Add comprehensive BepInEx 5.x and 6.x support (including IL2CPP and Mono variants) to MLVScan.
Release Notes
Contributor Statistics