Merged
Conversation
There was a problem hiding this comment.
Pull request overview
Adds new reflection utilities and stack helpers, alongside updated repo guidance and formatting rules.
Changes:
- Introduces
AssemblyContext/AssemblyContextOptionsto enumerate AppDomain assemblies with filtering and referenced-assembly traversal. - Adds stack
TryPophelpers forStack<T>andIDecorator<Stack<T>>, plus new tests covering the behavior. - Updates
AGENTS.mdand.editorconfigto clarify repository conventions and enforce consistent formatting.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Cuemon.Extensions.Collections.Generic.Tests/StackExtensionsTest.cs | Adds tests for Stack<T>.TryPop behavior across empty/non-empty stacks. |
| test/Cuemon.Core.Tests/Reflection/AssemblyContextTest.cs | Adds tests for AssemblyContext.GetCurrentDomainAssemblies defaults and options behavior. |
| test/Cuemon.Core.Tests/Reflection/AssemblyContextOptionsTest.cs | Adds tests for AssemblyContextOptions defaults, validation, and default filters. |
| src/Cuemon.Extensions.Collections.Generic/StackExtensions.cs | Adds Stack<T>.TryPop extension for NETSTANDARD2.0+ targets. |
| src/Cuemon.Core/Reflection/AssemblyContextOptions.cs | Adds options object with default filtering and validation. |
| src/Cuemon.Core/Reflection/AssemblyContext.cs | Adds assembly traversal API and recursive referenced-assembly enumeration. |
| src/Cuemon.Core/Extensions/Collections/Generic/StackDecoratorExtensions.cs | Adds TryPop for IDecorator<Stack<T>> to support older TFMs. |
| AGENTS.md | Rewrites agent guidance (layout, build/test, conventions, templates, commit style). |
| .editorconfig | Enforces global whitespace rules and adds C# namespace/top-level-statement preferences. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #148 +/- ##
==========================================
+ Coverage 80.53% 80.77% +0.24%
==========================================
Files 598 604 +6
Lines 18839 19009 +170
Branches 1936 1953 +17
==========================================
+ Hits 15172 15355 +183
+ Misses 3601 3587 -14
- Partials 66 67 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds assembly-discovery utilities with configurable filtering, safe TryPop extensions for stacks and decorated stacks, a global EditorConfig baseline and a comprehensive rewrite of AGENTS.md, plus unit tests covering the new reflection and stack behaviors. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant AC as AssemblyContext
participant AD as AppDomain
participant RR as ReferencedResolver
participant FL as FilterLogic
Client->>AC: GetCurrentDomainAssemblies(setup)
AC->>AC: Validate setup
AC->>AD: GetAssemblies()
AD-->>AC: assemblies[]
AC->>FL: Apply AssemblyFilter
FL-->>AC: filtered[]
alt IncludeReferencedAssemblies
AC->>RR: Initialize traversal (push filtered[])
loop traverse referenced assemblies
RR->>RR: Pop assembly
RR->>AD: GetReferencedAssemblyNames(assembly)
AD-->>RR: assemblyName[]
RR->>FL: Apply ReferencedAssemblyFilter
FL-->>RR: validRefs[]
RR->>RR: Push validRefs
end
RR-->>AC: reachable referenced assemblies
end
AC->>AC: Distinct & exclude ExcludedAssemblies
AC-->>Client: IReadOnlyList<Assembly>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.editorconfig (1)
105-113:⚠️ Potential issue | 🟡 MinorIncorrect IDE rule IDs in comment headers (two mismatches).
- Line 105:
# IDE0039should be# IDE0300— IDE0039 is "Use local function instead of lambda", not "collection expression for array".- Line 110:
# IDE0031should be# IDE0301— IDE0031 is "Use null propagation", not "collection expression for empty".The URLs and suppressed diagnostic IDs are both correct; only the comment header labels are wrong.
📝 Proposed fix
-# IDE0039: Use collection expression for array +# IDE0300: Use collection expression for array # https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0300 [*.{cs,vb}] dotnet_diagnostic.IDE0300.severity = none -# IDE0031: Use collection expression for empty +# IDE0301: Use collection expression for empty # https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0301 [*.{cs,vb}] dotnet_diagnostic.IDE0301.severity = none🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.editorconfig around lines 105 - 113, Replace the two incorrect comment headers that reference wrong rule IDs: change the header comment "# IDE0039: Use collection expression for array" to "# IDE0300: Use collection expression for array" and change "# IDE0031: Use collection expression for empty" to "# IDE0301: Use collection expression for empty" so the comment labels match the suppressed diagnostic IDs (refer to the existing commented lines "# IDE0039" and "# IDE0031" in the .editorconfig diff and update them to "# IDE0300" and "# IDE0301" respectively).
🧹 Nitpick comments (3)
.editorconfig (1)
183-187: DuplicateIDE0036entry — remove one.The block at lines 183–187 is an exact copy of the block at lines 177–181 (same comment, same section glob, same value). While harmless because they're identical, it's confusing noise.
♻️ Proposed fix
-# Order modifiers -# https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0036 -# Excluded becuase of inconsistency with other analyzers -[*.{cs,vb}] -dotnet_diagnostic.IDE0036.severity = none - # Use 'System.Threading.Lock'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.editorconfig around lines 183 - 187, Duplicate IDE0036 analyzer setting present; remove the redundant block so only one occurrence of the section with the glob "[*.{cs,vb}]" and "dotnet_diagnostic.IDE0036.severity = none" remains (keep a single copy and delete the other identical block).src/Cuemon.Core/Reflection/AssemblyContext.cs (2)
68-75:#if NET9_0_OR_GREATERfor nativeStack<T>.TryPopis misleading and fragile for future target additions.
Stack<T>.TryPophas been available since .NET Core 2.0 / .NET Standard 2.1, not since .NET 9. The current three-TFM setup (net9.0, net10.0, netstandard2.0) works by coincidence, but adding anynet5.0–net8.0target would send execution down theelsebranch and fail to compile:StackDecoratorExtensionsis only compiled whenNETSTANDARD2_0_OR_GREATERis defined, which is false for those TFMs.A semantically accurate condition avoids this latent trap:
♻️ Proposed refactor
private static bool TryPop(Stack<Assembly> stack, out Assembly assembly) { -#if NET9_0_OR_GREATER +#if NETCOREAPP2_0_OR_GREATER || NETSTANDARD2_1_OR_GREATER return stack.TryPop(out assembly); `#else` return Decorator.RawEnclose(stack).TryPop(out assembly); `#endif` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Cuemon.Core/Reflection/AssemblyContext.cs` around lines 68 - 75, The preprocessor check in TryPop incorrectly assumes Stack<T>.TryPop only exists on NET9; update the conditional around the TryPop implementation in the TryPop method so it uses a symbol covering frameworks that actually expose Stack<T>.TryPop (e.g. NETSTANDARD2_0_OR_GREATER || NETCOREAPP) instead of NET9_0_OR_GREATER, leaving the else branch that calls Decorator.RawEnclose(stack).TryPop(out assembly) intact; locate the TryPop method and adjust the `#if/`#else symbols accordingly (references: TryPop, Stack<Assembly>, Decorator.RawEnclose).
25-35: Avoid aSelectMany+ per-element array allocation when referenced assemblies are excluded.
(Func<Assembly, IEnumerable<Assembly>>)(a => new[] { a })materialises a one-element array for every assembly in the domain. WhenIncludeReferencedAssembliesisfalsetheSelectManystep is redundant; the filtered sequence can be used directly.♻️ Proposed refactor
-return AppDomain - .CurrentDomain - .GetAssemblies() - .Where(options.AssemblyFilter) - .SelectMany(options.IncludeReferencedAssemblies - ? a => GetReferencedAssemblies(a, options.ReferencedAssemblyFilter) - : (Func<Assembly, IEnumerable<Assembly>>)(a => new[] { a })) - .Distinct() - .Except(options.ExcludedAssemblies) - .ToList() - .AsReadOnly(); +var filtered = AppDomain + .CurrentDomain + .GetAssemblies() + .Where(options.AssemblyFilter); + +IEnumerable<Assembly> assemblies = options.IncludeReferencedAssemblies + ? filtered.SelectMany(a => GetReferencedAssemblies(a, options.ReferencedAssemblyFilter)) + : filtered; + +return assemblies + .Distinct() + .Except(options.ExcludedAssemblies) + .ToList() + .AsReadOnly();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Cuemon.Core/Reflection/AssemblyContext.cs` around lines 25 - 35, The current chain always calls SelectMany with a lambda that allocates a single-element array when options.IncludeReferencedAssemblies is false, causing unnecessary allocations; change the logic in the method that returns assemblies to branch on options.IncludeReferencedAssemblies: if true, use .SelectMany(a => GetReferencedAssemblies(a, options.ReferencedAssemblyFilter)); if false, skip SelectMany and use the filtered sequence from .GetAssemblies().Where(options.AssemblyFilter); then continue with .Distinct().Except(options.ExcludedAssemblies).ToList().AsReadOnly(); keep references to options.AssemblyFilter, options.ReferencedAssemblyFilter, GetReferencedAssemblies and options.ExcludedAssemblies so the rest of the pipeline is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.editorconfig:
- Line 5: The global setting "end_of_line = crlf" in the [*] section will force
CRLF for all files — change the .editorconfig so that the default is LF (or
remove the global CRLF) and move "end_of_line = crlf" into a Windows-specific
section (e.g., [*.{bat,ps1,txt}] or [*.{md,mdx}] as appropriate) while
explicitly adding overrides for Unix-executed files (e.g., [*.sh], [Makefile],
[*.py], and GitHub Actions YAML) with "end_of_line = lf"; update the [*] header
and add those overrides so shell scripts, YAML, and Python keep LF and Windows
files get CRLF.
In `@AGENTS.md`:
- Around line 26-30: Update the fenced code block containing the dotnet
build/pack commands so the opening triple-backtick includes the language
specifier "shell" (i.e., change ``` to ```shell) for the block that lists
"dotnet build -c Release", "dotnet build src/Cuemon.Core/Cuemon.Core.csproj -c
Release", and "dotnet pack -c Release" in AGENTS.md to enable proper syntax
highlighting and satisfy MD040.
- Around line 40-50: The fenced code block in AGENTS.md containing the dotnet
test commands lacks a language specifier; update that fence to include a
language tag (e.g., "shell" or "bash") so the block becomes ```shell (or
```bash) at the start and ``` at the end, ensuring the test command examples
(the dotnet test lines and the --filter example) are rendered with proper syntax
highlighting.
In `@src/Cuemon.Core/Reflection/AssemblyContextOptions.cs`:
- Around line 153-165: Update the XML <exception> documentation for
ValidateOptions() to include ExcludedAssemblies alongside AssemblyFilter and
ReferencedAssemblyFilter as reasons an InvalidOperationException may be thrown;
ensure the <exception cref="InvalidOperationException"> element lists all three
properties (AssemblyFilter, ExcludedAssemblies, ReferencedAssemblyFilter) so the
docs match the Validator.ThrowIfInvalidState checks in ValidateOptions().
In `@src/Cuemon.Extensions.Collections.Generic/StackExtensions.cs`:
- Around line 18-27: The TryPop extension in StackExtensions (public static bool
TryPop<T>(this Stack<T> stack, out T result)) lacks a null guard and will throw
a NullReferenceException on stack.Count; add a Validator.ThrowIfNull(stack) call
at the start of StackExtensions.TryPop to mirror the pattern used in
StackDecoratorExtensions.TryPop so it throws an ArgumentNullException for null
input and preserves current behavior otherwise.
In `@test/Cuemon.Core.Tests/Reflection/AssemblyContextOptionsTest.cs`:
- Around line 15-23: The test AssemblyContextOptions_ShouldHaveDefaultValues
currently omits asserting the default ExcludedAssemblies value; update the test
to assert that a newly constructed AssemblyContextOptions (sut) has
ExcludedAssemblies initialized and contains the expected containing assembly
name (e.g., "Cuemon.Core") by checking sut.ExcludedAssemblies is not null/empty
and that it contains the documented default entry for the Cuemon.Core assembly.
---
Outside diff comments:
In @.editorconfig:
- Around line 105-113: Replace the two incorrect comment headers that reference
wrong rule IDs: change the header comment "# IDE0039: Use collection expression
for array" to "# IDE0300: Use collection expression for array" and change "#
IDE0031: Use collection expression for empty" to "# IDE0301: Use collection
expression for empty" so the comment labels match the suppressed diagnostic IDs
(refer to the existing commented lines "# IDE0039" and "# IDE0031" in the
.editorconfig diff and update them to "# IDE0300" and "# IDE0301" respectively).
---
Nitpick comments:
In @.editorconfig:
- Around line 183-187: Duplicate IDE0036 analyzer setting present; remove the
redundant block so only one occurrence of the section with the glob
"[*.{cs,vb}]" and "dotnet_diagnostic.IDE0036.severity = none" remains (keep a
single copy and delete the other identical block).
In `@src/Cuemon.Core/Reflection/AssemblyContext.cs`:
- Around line 68-75: The preprocessor check in TryPop incorrectly assumes
Stack<T>.TryPop only exists on NET9; update the conditional around the TryPop
implementation in the TryPop method so it uses a symbol covering frameworks that
actually expose Stack<T>.TryPop (e.g. NETSTANDARD2_0_OR_GREATER || NETCOREAPP)
instead of NET9_0_OR_GREATER, leaving the else branch that calls
Decorator.RawEnclose(stack).TryPop(out assembly) intact; locate the TryPop
method and adjust the `#if/`#else symbols accordingly (references: TryPop,
Stack<Assembly>, Decorator.RawEnclose).
- Around line 25-35: The current chain always calls SelectMany with a lambda
that allocates a single-element array when options.IncludeReferencedAssemblies
is false, causing unnecessary allocations; change the logic in the method that
returns assemblies to branch on options.IncludeReferencedAssemblies: if true,
use .SelectMany(a => GetReferencedAssemblies(a,
options.ReferencedAssemblyFilter)); if false, skip SelectMany and use the
filtered sequence from .GetAssemblies().Where(options.AssemblyFilter); then
continue with
.Distinct().Except(options.ExcludedAssemblies).ToList().AsReadOnly(); keep
references to options.AssemblyFilter, options.ReferencedAssemblyFilter,
GetReferencedAssemblies and options.ExcludedAssemblies so the rest of the
pipeline is unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/Cuemon.Core.Tests/Reflection/AssemblyContextOptionsTest.cs (1)
35-41: Brittle exact-message assertion — preferAssert.Containsfor the expression fragment.
Assert.Equalon the fullInvalidOperationExceptionmessage combines a locale-sensitive CLR default ("Operation is not valid due to the current state of the object.") with an implementation-specific expression string. If the runtime locale, .NET version, or the guard-clause expression text ever changes, all three validation-error tests break simultaneously.The same pattern repeats at lines 52–58 and 69–75.
♻️ Proposed refactor (illustrative for all three blocks)
-Assert.Equal("Operation is not valid due to the current state of the object. (Expression 'AssemblyFilter is null')", sut2.Message); +Assert.Contains("AssemblyFilter is null", sut2.Message);-Assert.Equal("Operation is not valid due to the current state of the object. (Expression 'ExcludedAssemblies is null')", sut2.Message); +Assert.Contains("ExcludedAssemblies is null", sut2.Message);-Assert.Equal("Operation is not valid due to the current state of the object. (Expression 'ReferencedAssemblyFilter is null')", sut2.Message); +Assert.Contains("ReferencedAssemblyFilter is null", sut2.Message);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Cuemon.Core.Tests/Reflection/AssemblyContextOptionsTest.cs` around lines 35 - 41, Replace brittle exact-message assertions for the InvalidOperationException text with contains-based checks: instead of Assert.Equal(expectedFullMessage, sut2.Message) use Assert.Contains("AssemblyFilter is null", sut2.Message) when asserting the message from sut1.ValidateOptions(); likewise update the parallel assertions around Validator.ThrowIfInvalidOptions(sut1) and the two other blocks (the cases at lines 52–58 and 69–75) to check for the relevant expression fragment via Assert.Contains rather than exact string equality, while keeping the existing Assert.StartsWith/Assert.Contains for the outer message and the Assert.IsType check on sut3.InnerException unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@AGENTS.md`:
- Around line 41-51: Add a language specifier to the fenced code block
containing the dotnet test commands in AGENTS.md (the block starting with
"dotnet test test/Cuemon.Core.Tests/Cuemon.Core.Tests.csproj -c Release" and the
line with the filter string
"FullyQualifiedName~DateSpanTest.Parse_ShouldGetOneMonthOfDifference_UsingIso8601String");
change the opening fence from ``` to ```shell (or ```bash) so the test-commands
block complies with MD040.
- Around line 27-31: The fenced code block containing the dotnet commands (the
lines starting with "dotnet build -c Release", "dotnet build
src/Cuemon.Core/Cuemon.Core.csproj -c Release", and "dotnet pack -c Release") is
missing a language specifier which triggers MD040; add a language tag (e.g., use
"shell" or "bash") immediately after the opening ``` so the block becomes fenced
with ```shell to enable syntax highlighting and satisfy the linter.
---
Nitpick comments:
In `@test/Cuemon.Core.Tests/Reflection/AssemblyContextOptionsTest.cs`:
- Around line 35-41: Replace brittle exact-message assertions for the
InvalidOperationException text with contains-based checks: instead of
Assert.Equal(expectedFullMessage, sut2.Message) use
Assert.Contains("AssemblyFilter is null", sut2.Message) when asserting the
message from sut1.ValidateOptions(); likewise update the parallel assertions
around Validator.ThrowIfInvalidOptions(sut1) and the two other blocks (the cases
at lines 52–58 and 69–75) to check for the relevant expression fragment via
Assert.Contains rather than exact string equality, while keeping the existing
Assert.StartsWith/Assert.Contains for the outer message and the Assert.IsType
check on sut3.InnerException unchanged.
…ect' Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/Cuemon.Core/Reflection/AssemblyContextOptions.cs (3)
1-6:System.Linqis out of alphabetical order relative toSystem.Reflection.Guideline requires
usingdirectives to follow existing project ordering.Linqsorts beforeReflection; swapping lines 4 and 5 would restore alphabetical ordering forSystem.*directives.♻️ Proposed fix
using System; using System.Collections.Concurrent; using System.Collections.Generic; -using System.Reflection; using System.Linq; +using System.Reflection; using Cuemon.Configuration;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Cuemon.Core/Reflection/AssemblyContextOptions.cs` around lines 1 - 6, The using directives in AssemblyContextOptions.cs are out of the prescribed alphabetical order: move the System.Linq directive so it comes before System.Reflection (i.e., reorder the System.* usings to alphabetic order), ensuring the block containing System, System.Collections.Concurrent, System.Collections.Generic, System.Linq, System.Reflection, Cuemon.Configuration matches project ordering.
95-122: Two minor defensive concerns inHasFrameworkRootNamespace(Assembly).
- Missing null guard on
assembly(Line 97):assembly.FullNameis dereferenced before thestring.IsNullOrEmpty(key)guard on Line 98. Although the only caller (DefaultAssemblyFilter) already checks for null, a direct guard keeps the private method self-contained.- Unbounded static cache (Line 18 / Line 100):
FrameworkNamespaceCachegrows for the process lifetime. For typical applications this is negligible, but in Assembly Load Context (ALC) scenarios where many assemblies are loaded into isolated contexts, cache entries are never evicted. Consider documenting this as a known trade-off or capping cache size if ALC usage is anticipated.♻️ Proposed null-guard fix
private static bool HasFrameworkRootNamespace(Assembly assembly) { + if (assembly is null) { return false; } var key = assembly.FullName; if (string.IsNullOrEmpty(key)) { return false; } ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Cuemon.Core/Reflection/AssemblyContextOptions.cs` around lines 95 - 122, Add a null-check at the start of AssemblyContextOptions.HasFrameworkRootNamespace(Assembly) to return false immediately if the assembly argument is null before accessing assembly.FullName, and replace the current key assignment with a null-safe retrieval (e.g., guard or use assembly.FullName only after null check); additionally address the unbounded FrameworkNamespaceCache growth by documenting the trade-off in comments near FrameworkNamespaceCache and/or implementing a bounded cache strategy (LRU or size cap) for FrameworkNamespaceCache to avoid unbounded memory buildup in Assembly Load Context (ALC) scenarios.
58-78: Preferprivate static readonlyfields over static properties for immutable delegates.
DefaultAssemblyFilterandDefaultReferencedAssemblyFilterare never reassigned; exposing them as auto-properties adds a getter stub with no benefit over astatic readonlyfield.♻️ Proposed refactor
-private static Func<Assembly, bool> DefaultAssemblyFilter { get; } = assembly => +private static readonly Func<Assembly, bool> DefaultAssemblyFilter = assembly => { ... }; -private static Func<AssemblyName, bool> DefaultReferencedAssemblyFilter { get; } = assemblyName => +private static readonly Func<AssemblyName, bool> DefaultReferencedAssemblyFilter = assemblyName => { ... };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Cuemon.Core/Reflection/AssemblyContextOptions.cs` around lines 58 - 78, DefaultAssemblyFilter and DefaultReferencedAssemblyFilter are immutable delegates declared as private static properties; change them to private static readonly fields to avoid the unnecessary property getter overhead. Replace the auto-property declarations for DefaultAssemblyFilter (Func<Assembly, bool>) and DefaultReferencedAssemblyFilter (Func<AssemblyName, bool>) with equivalent private static readonly field declarations and initialize them with the same lambda bodies, keeping the existing delegate types and logic (including calls to HasFrameworkRootNamespace and the SystemPrefix/MicrosoftPrefix checks).src/Cuemon.Core/Patterns.cs (1)
88-88: UseIsRecoverableExceptioninstead of!IsFatalExceptionto stay consistent with line 66 and the stated intent of the API.
IsRecoverableException's own XML doc says it exists "to avoid double-negation", yet this overload uses the double-negated form. Both overloads should use the same, more readable expression.♻️ Proposed fix
- catch (Exception ex) when (!IsFatalException(ex)) + catch (Exception ex) when (IsRecoverableException(ex))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Cuemon.Core/Patterns.cs` at line 88, Replace the double-negation exception filter with the intended readable helper: in Patterns.cs change the catch filter that currently uses "when (!IsFatalException(ex))" to use "when (IsRecoverableException(ex))" so it matches the overload on line 66 and the API intent; update the catch clause referencing the same exception variable (ex) and ensure any surrounding comments or XML docs remain consistent with IsRecoverableException's purpose.test/Cuemon.Core.Tests/PatternsTest.cs (1)
144-175:ThreadInterruptedException(and optionallyExecutionEngineException) are missing from the fatal-exception test coverage.
IsFatalExceptionclassifies both as fatal, but neither appears inIsFatalException_ShouldReturnTrue_WhenExceptionIsFatalorIsRecoverableException_ShouldReturnFalse_WhenExceptionIsFatal.ThreadAbortExceptioncannot be constructed in .NET Core user code (internal constructor), so its omission is understandable, butThreadInterruptedExceptionhas a public constructor and should be included.ExecutionEngineExceptionis constructible but obsolete.✅ Proposed additions
Assert.True(Patterns.IsFatalException(new AccessViolationException())); Assert.True(Patterns.IsFatalException(new SEHException())); + Assert.True(Patterns.IsFatalException(new ThreadInterruptedException())); +#pragma warning disable CS0618 + Assert.True(Patterns.IsFatalException(new ExecutionEngineException())); +#pragma warning restore CS0618 }Assert.False(Patterns.IsRecoverableException(new AccessViolationException())); Assert.False(Patterns.IsRecoverableException(new SEHException())); + Assert.False(Patterns.IsRecoverableException(new ThreadInterruptedException())); +#pragma warning disable CS0618 + Assert.False(Patterns.IsRecoverableException(new ExecutionEngineException())); +#pragma warning restore CS0618 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Cuemon.Core.Tests/PatternsTest.cs` around lines 144 - 175, Add missing fatal-exception cases to the tests: update IsFatalException_ShouldReturnTrue_WhenExceptionIsFatal to include ThreadInterruptedException (constructed with new ThreadInterruptedException()) and optionally ExecutionEngineException if you want coverage despite obsolescence; likewise add corresponding assertions to IsRecoverableException_ShouldReturnFalse_WhenExceptionIsFatal to assert Patterns.IsRecoverableException returns false for new ThreadInterruptedException (and for new ExecutionEngineException() if included). Target the existing test methods IsFatalException_ShouldReturnTrue_WhenExceptionIsFatal and IsRecoverableException_ShouldReturnFalse_WhenExceptionIsFatal and use the Patterns.IsFatalException and Patterns.IsRecoverableException calls to add the new assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Cuemon.Core/Patterns.cs`:
- Around line 28-50: Both public methods IsFatalException and
IsRecoverableException lack null-argument validation; add a guard at the start
of each method that calls Validator.ThrowIfNull(exception, nameof(exception)) to
enforce the contract and fail fast on null input, keeping the rest of the logic
unchanged (IsRecoverableException should still return
!IsFatalException(exception)).
---
Duplicate comments:
In `@AGENTS.md`:
- Around line 41-51: The fenced code block containing test commands in AGENTS.md
is missing a language specifier (MD040); update that code fence (the
test-commands block with the dotnet test examples) to include a shell/bash
language tag (e.g., change the opening ``` to ```shell or ```bash) so the
Markdown linter recognizes it as a command block.
- Around line 27-31: The fenced code block containing the three build commands
("dotnet build -c Release", "dotnet build src/Cuemon.Core/Cuemon.Core.csproj -c
Release", "dotnet pack -c Release") is missing a language specifier which
triggers MD040; add a language tag (e.g., "shell") to the opening
triple-backtick fence so the block becomes ```shell ... ``` to enable syntax
highlighting and silence the markdownlint warning.
In `@src/Cuemon.Core/Reflection/AssemblyContextOptions.cs`:
- Around line 166-179: ValidateOptions() already checks AssemblyFilter,
ExcludedAssemblies and ReferencedAssemblyFilter and the XML doc lists all three,
so no code change is required; keep the current implementation of
ValidateOptions and its XML documentation as-is (references: ValidateOptions,
AssemblyFilter, ExcludedAssemblies, ReferencedAssemblyFilter).
---
Nitpick comments:
In `@src/Cuemon.Core/Patterns.cs`:
- Line 88: Replace the double-negation exception filter with the intended
readable helper: in Patterns.cs change the catch filter that currently uses
"when (!IsFatalException(ex))" to use "when (IsRecoverableException(ex))" so it
matches the overload on line 66 and the API intent; update the catch clause
referencing the same exception variable (ex) and ensure any surrounding comments
or XML docs remain consistent with IsRecoverableException's purpose.
In `@src/Cuemon.Core/Reflection/AssemblyContextOptions.cs`:
- Around line 1-6: The using directives in AssemblyContextOptions.cs are out of
the prescribed alphabetical order: move the System.Linq directive so it comes
before System.Reflection (i.e., reorder the System.* usings to alphabetic
order), ensuring the block containing System, System.Collections.Concurrent,
System.Collections.Generic, System.Linq, System.Reflection, Cuemon.Configuration
matches project ordering.
- Around line 95-122: Add a null-check at the start of
AssemblyContextOptions.HasFrameworkRootNamespace(Assembly) to return false
immediately if the assembly argument is null before accessing assembly.FullName,
and replace the current key assignment with a null-safe retrieval (e.g., guard
or use assembly.FullName only after null check); additionally address the
unbounded FrameworkNamespaceCache growth by documenting the trade-off in
comments near FrameworkNamespaceCache and/or implementing a bounded cache
strategy (LRU or size cap) for FrameworkNamespaceCache to avoid unbounded memory
buildup in Assembly Load Context (ALC) scenarios.
- Around line 58-78: DefaultAssemblyFilter and DefaultReferencedAssemblyFilter
are immutable delegates declared as private static properties; change them to
private static readonly fields to avoid the unnecessary property getter
overhead. Replace the auto-property declarations for DefaultAssemblyFilter
(Func<Assembly, bool>) and DefaultReferencedAssemblyFilter (Func<AssemblyName,
bool>) with equivalent private static readonly field declarations and initialize
them with the same lambda bodies, keeping the existing delegate types and logic
(including calls to HasFrameworkRootNamespace and the
SystemPrefix/MicrosoftPrefix checks).
In `@test/Cuemon.Core.Tests/PatternsTest.cs`:
- Around line 144-175: Add missing fatal-exception cases to the tests: update
IsFatalException_ShouldReturnTrue_WhenExceptionIsFatal to include
ThreadInterruptedException (constructed with new ThreadInterruptedException())
and optionally ExecutionEngineException if you want coverage despite
obsolescence; likewise add corresponding assertions to
IsRecoverableException_ShouldReturnFalse_WhenExceptionIsFatal to assert
Patterns.IsRecoverableException returns false for new ThreadInterruptedException
(and for new ExecutionEngineException() if included). Target the existing test
methods IsFatalException_ShouldReturnTrue_WhenExceptionIsFatal and
IsRecoverableException_ShouldReturnFalse_WhenExceptionIsFatal and use the
Patterns.IsFatalException and Patterns.IsRecoverableException calls to add the
new assertions.
Comment on lines
+28
to
+50
| public static bool IsFatalException(Exception exception) | ||
| { | ||
| #pragma warning disable CS0618 // ExecutionEngineException is obsolete | ||
| return exception is OutOfMemoryException | ||
| || exception is StackOverflowException | ||
| || exception is SEHException | ||
| || exception is AccessViolationException | ||
| || exception is ThreadAbortException | ||
| || exception is ThreadInterruptedException | ||
| || exception is ExecutionEngineException; | ||
| #pragma warning restore CS0618 | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Determines whether the specified <paramref name="exception"/> is considered recoverable and can be safely caught. | ||
| /// </summary> | ||
| /// <param name="exception">The exception to evaluate.</param> | ||
| /// <returns><c>true</c> if <paramref name="exception"/> is not considered fatal; otherwise, <c>false</c>.</returns> | ||
| /// <remarks>Designed for use as an exception filter: <c>catch (Exception ex) when (Patterns.IsRecoverableException(ex))</c> to avoid double-negation.</remarks> | ||
| public static bool IsRecoverableException(Exception exception) | ||
| { | ||
| return !IsFatalException(exception); | ||
| } |
There was a problem hiding this comment.
Add Validator.ThrowIfNull guards to both public methods.
Neither IsFatalException nor IsRecoverableException validates the exception parameter. While null is X returns false in C# (so no crash occurs), the current contract silently treats null as "not fatal / recoverable." Per the coding guidelines, public methods must use Validator.ThrowIfNull for parameter validation.
🛡️ Proposed fix
public static bool IsFatalException(Exception exception)
{
+ Validator.ThrowIfNull(exception);
`#pragma` warning disable CS0618 // ExecutionEngineException is obsolete
return exception is OutOfMemoryException public static bool IsRecoverableException(Exception exception)
{
+ Validator.ThrowIfNull(exception);
return !IsFatalException(exception);
}As per coding guidelines: "Use guard clauses and Validator.ThrowIfNull style patterns for error handling."
📝 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.
Suggested change
| public static bool IsFatalException(Exception exception) | |
| { | |
| #pragma warning disable CS0618 // ExecutionEngineException is obsolete | |
| return exception is OutOfMemoryException | |
| || exception is StackOverflowException | |
| || exception is SEHException | |
| || exception is AccessViolationException | |
| || exception is ThreadAbortException | |
| || exception is ThreadInterruptedException | |
| || exception is ExecutionEngineException; | |
| #pragma warning restore CS0618 | |
| } | |
| /// <summary> | |
| /// Determines whether the specified <paramref name="exception"/> is considered recoverable and can be safely caught. | |
| /// </summary> | |
| /// <param name="exception">The exception to evaluate.</param> | |
| /// <returns><c>true</c> if <paramref name="exception"/> is not considered fatal; otherwise, <c>false</c>.</returns> | |
| /// <remarks>Designed for use as an exception filter: <c>catch (Exception ex) when (Patterns.IsRecoverableException(ex))</c> to avoid double-negation.</remarks> | |
| public static bool IsRecoverableException(Exception exception) | |
| { | |
| return !IsFatalException(exception); | |
| } | |
| public static bool IsFatalException(Exception exception) | |
| { | |
| Validator.ThrowIfNull(exception); | |
| `#pragma` warning disable CS0618 // ExecutionEngineException is obsolete | |
| return exception is OutOfMemoryException | |
| || exception is StackOverflowException | |
| || exception is SEHException | |
| || exception is AccessViolationException | |
| || exception is ThreadAbortException | |
| || exception is ThreadInterruptedException | |
| || exception is ExecutionEngineException; | |
| `#pragma` warning restore CS0618 | |
| } | |
| /// <summary> | |
| /// Determines whether the specified <paramref name="exception"/> is considered recoverable and can be safely caught. | |
| /// </summary> | |
| /// <param name="exception">The exception to evaluate.</param> | |
| /// <returns><c>true</c> if <paramref name="exception"/> is not considered fatal; otherwise, <c>false</c>.</returns> | |
| /// <remarks>Designed for use as an exception filter: <c>catch (Exception ex) when (Patterns.IsRecoverableException(ex))</c> to avoid double-negation.</remarks> | |
| public static bool IsRecoverableException(Exception exception) | |
| { | |
| Validator.ThrowIfNull(exception); | |
| return !IsFatalException(exception); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Cuemon.Core/Patterns.cs` around lines 28 - 50, Both public methods
IsFatalException and IsRecoverableException lack null-argument validation; add a
guard at the start of each method that calls Validator.ThrowIfNull(exception,
nameof(exception)) to enforce the contract and fail fast on null input, keeping
the rest of the logic unchanged (IsRecoverableException should still return
!IsFatalException(exception)).
|
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
1 participant
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.
This pull request introduces several improvements to code style enforcement, documentation, and new APIs for assembly traversal and stack extension methods. The most significant changes include updating the
.editorconfigandAGENTS.mddocumentation to clarify and enforce conventions, and adding new utility features for working with assemblies and stacks.Key changes:
Code Style and Configuration
.editorconfigto enforce consistent code style, including UTF-8 encoding, CRLF line endings, 2-space indentation (except for.csfiles, which use 4), and preferences for file-scoped namespaces and explicit class declarations. Top-level statements are explicitly disallowed. [1] [2]Documentation and Guidelines
AGENTS.mdfor clarity and completeness: expanded repository layout, build/test/benchmark instructions, analyzer and code style rules, namespace and import conventions, test/benchmark templates, XML doc requirements, release notes, gitmoji commit style, and agent workflow.New APIs and Utilities
AssemblyContextstatic class inCuemon.Core/Reflection, providing methods to enumerate and filter assemblies in the current AppDomain, including recursive traversal of referenced assemblies with customizable filters and compatibility handling for stack operations.StackDecoratorExtensionsinCuemon.Core/Extensions/Collections/Generic, providing aTryPopextension method forIDecorator<Stack<T>>to safely pop elements in a .NET Standard 2.0+ compatible way.Summary by CodeRabbit
New Features
Documentation
Tests
Chores