Validate --pluginassembly at parse time with a native FileInfo option#792
Merged
Conversation
Use Option<FileInfo> with AcceptExistingOnly so a missing plugin path fails at argument parsing with a clear "File does not exist" message before the command runs, and simplify PluginLoader.Load to take a FileInfo and drop the empty-path, GetFullPath, and File.Exists guards (the CLI now guarantees an existing path). Scope is limited to --pluginassembly, the one path option that is a pure input whose validation this removes; --mediafiles/--settingsfile/--schemafile keep their current handling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Updates PlexCleaner’s custom plugin command to validate --pluginassembly at argument-parse time (instead of inside the handler) by switching the option to a native FileInfo with AcceptExistingOnly(), and adjusts the plugin loader/tests accordingly.
Changes:
- Convert
--pluginassemblyfromstringtoOption<FileInfo>and enable parse-time existence validation viaAcceptExistingOnly(). - Update
PluginLoader.Loadto accept aFileInfoand remove the prior manual path validation. - Update plugin loader tests and add a defensive null-check in
CustomCommandfor the bound option.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| PlexCleaner/CommandLineOptions.cs | Changes --pluginassembly to Option<FileInfo> and applies AcceptExistingOnly(); binds to CommandLineOptions.PluginAssembly as FileInfo?. |
| PlexCleaner/PluginLoader.cs | Updates PluginLoader.Load signature to accept FileInfo and removes manual path validation logic. |
| PlexCleaner/Program.cs | Adds a null guard before calling PluginLoader.Load in the custom command handler. |
| PlexCleanerTests/PluginLoaderTests.cs | Updates tests to pass FileInfo into PluginLoader.Load. |
FileInfo.FullName resolves the path and can throw on a malformed or too-long path; move it inside the try so any error is logged via LogAndHandle and returns null instead of throwing, and log the file name in the catch since it does not resolve the path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- PluginLoader.Load: guard against a null FileInfo with ArgumentNullException so the catch block never dereferences a null argument - CustomCommand: log an explicit error before returning when the plugin path is missing so the failure is visible even though the option is required - CommandLineTests: cover the custom command, an existing plugin path binds and a missing path fails at parse time via AcceptExistingOnly Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Track the assembly path in a single variable that starts as the file name and upgrades to the resolved full path inside the try, so the catch logs the most specific location available (full path when resolved, file name if FullName threw) instead of only the ambiguous file name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AcceptExistingOnly was applied unconditionally, so under an AOT build a non-existent plugin path would fail at parse instead of reaching CustomCommand and its "requires a non-AOT build" message. Gate both Required and AcceptExistingOnly behind PLUGINS, matching the command handler. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Follow-up to #791. Converts the
--pluginassemblyoption fromstringto a nativeOption<FileInfo>withAcceptExistingOnly(), so a missing plugin path fails at argument parsing with a clearFile does not existmessage before the command handler runs.This lets
PluginLoader.Loadtake aFileInfoand drop its manual validation — the empty-path check,Path.GetFullPath, andFile.Existsguards are removed (the CLI now guarantees an existing, resolved path). The remainingtry/catchcovers genuine assembly-load/reflection errors only.Scope
Deliberately limited to
--pluginassembly, the one path option that is a pure input whose validation this actually removes:--mediafiles—GetFilesstill needs itstry/catchfor enumeration/permission/cancellation, so a native type would only add aFileSystemInfo→stringconversion without removing code (and the collectionAcceptExistingOnlysupport is inconsistent). Left asList<string>.--settingsfile/--schemafile— write paths fordefaultsettings/createschema, so must-exist would be wrong. Unchanged.Testing
TreatWarningsAsErrors), format clean, full suite green (162).--pluginassembly /no/such.dll→File does not exist: '/no/such.dll'., exit 1, before the handler.No version bump (part of unreleased 3.20).