Open
Conversation
WalkthroughInjects an IStrategyExecutor into RegistryProcessor and emits per-attempt CSVComputerStatus events. Adds short-circuit status reporting when port scanning is skipped, input validation in a registry strategy, small import cleanups, and extensive unit tests for strategy execution and status reporting. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RegistryProcessor
participant IStrategyExecutor
participant ComputerStatusEvent
Client->>RegistryProcessor: ReadRegistrySettings(target)
RegistryProcessor->>IStrategyExecutor: CollectAsync(target, queries, strategies)
loop per strategy attempt
IStrategyExecutor->>IStrategyExecutor: Execute strategy attempt
alt attempt fails
IStrategyExecutor-->>RegistryProcessor: FailureAttempt (reason, type)
RegistryProcessor->>ComputerStatusEvent: Publish CSVComputerStatus (failure)
else attempt succeeds
IStrategyExecutor-->>RegistryProcessor: StrategyExecutorResult (WasSuccessful=true, SuccessfulStrategy)
end
end
alt overall success
RegistryProcessor->>ComputerStatusEvent: Publish CSVComputerStatus (success, strategy)
RegistryProcessor->>Client: APIResult(Collected=true, data)
else overall failure
RegistryProcessor->>Client: APIResult(Collected=false, combined FailureReason)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 2
🧹 Nitpick comments (4)
src/CommonLib/Processors/ComputerAvailability.cs (1)
98-110: LGTM – minor: missing_log.LogTracefor parity with the other success path.Every other exit path in this method emits a trace log before or alongside
SendComputerStatus. The normal (port-scan performed) success path at line 126 logs"{ComputerName} is available for enumeration"before its status send, but the new_skipPortScansuccess branch is silent.🔍 Proposed trace log for consistency
if (_skipPortScan) { + _log.LogTrace("{ComputerName} is available for enumeration (port scan skipped)", computerName); await SendComputerStatus(new CSVComputerStatus { Status = CSVComputerStatus.StatusSuccess, Task = "ComputerAvailability", ComputerName = computerName, ObjectId = objectId, }); return new ComputerStatus { Connectable = true, Error = null }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/Processors/ComputerAvailability.cs` around lines 98 - 110, Add a trace log to the _skipPortScan success branch to match the other success path: call _log.LogTrace with a message like "{ComputerName} is available for enumeration" (using the computerName variable) immediately before calling SendComputerStatus in the block that constructs the CSVComputerStatus and returns a ComputerStatus; this keeps logging parity with the other path that logs before SendComputerStatus.test/unit/Facades/MockExtentions.cs (1)
36-46: Inconsistent nullability annotation on the formatter parameter.The existing
VerifyLogContainsandVerifyLogmethods useFunc<It.IsAnyType, Exception?, string>(nullableException?), but this new method usesFunc<It.IsAnyType, Exception, string>. While nullable reference types are erased at runtime and this likely won't cause a Moq matching failure, it should be consistent with the other helpers for clarity.Proposed fix
public static void VerifyNoLogs<T>(this Mock<ILogger<T>> mockLogger, LogLevel logLevel) { mockLogger.Verify( x => x.Log( logLevel, It.IsAny<EventId>(), It.IsAny<It.IsAnyType>(), It.IsAny<Exception>(), - It.IsAny<Func<It.IsAnyType, Exception, string>>()), + It.IsAny<Func<It.IsAnyType, Exception?, string>>()), Times.Never); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/Facades/MockExtentions.cs` around lines 36 - 46, The VerifyNoLogs<T> helper uses a non-nullable formatter signature which is inconsistent with VerifyLog and VerifyLogContains; update the formatter parameter type in VerifyNoLogs<T> from Func<It.IsAnyType, Exception, string> to Func<It.IsAnyType, Exception?, string> so the nullable Exception annotation matches the other helper methods (locate the VerifyNoLogs<T> method and adjust the formatter type accordingly).test/unit/RegistryProcessorTests.cs (1)
152-170: PreferThrowsAsyncoverThrowsfor async method setup.
_mockStrategyExecutor.Setup(...).Throws(exception)throws synchronously before aTaskis returned, which is atypical for async methods. Using.ThrowsAsync(exception)returns a faultedTaskinstead, more closely simulating real-world async failure behavior.Proposed fix
_mockStrategyExecutor.Setup(se => se.CollectAsync( It.IsAny<string>(), It.IsAny<IEnumerable<RegistryQuery>>(), It.IsAny<IEnumerable<ICollectionStrategy<RegistryQueryResult, RegistryQuery>>>())) - .Throws(exception); + .ThrowsAsync(exception);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/RegistryProcessorTests.cs` around lines 152 - 170, The test RegistryProcessor_ReadRegistrySettings_HandlesException is setting up the async CollectAsync call with .Throws(exception) which triggers a synchronous throw; update the mock setup for _mockStrategyExecutor.Setup(se => se.CollectAsync(...)) to use .ThrowsAsync(exception) so the setup returns a faulted Task and better simulates async failure, keeping the rest of the assertions (results and log verification) unchanged.src/CommonLib/Processors/RegistryProcessor.cs (1)
146-148: Consider using the standard delegate-capture pattern for event invocation.
ComputerStatusEventcould theoretically become null between the null-check andInvokeif a handler is removed concurrently. The idiomatic C# pattern captures the delegate first:Proposed fix
private async Task SendComputerStatus(CSVComputerStatus status) { - if (ComputerStatusEvent is not null) await ComputerStatusEvent.Invoke(status); + var handler = ComputerStatusEvent; + if (handler is not null) await handler.Invoke(status); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/Processors/RegistryProcessor.cs` around lines 146 - 148, The current SendComputerStatus method checks ComputerStatusEvent for null then awaits ComputerStatusEvent.Invoke(status), which can race if handlers are removed concurrently; fix it by capturing the event to a local variable (e.g., var handler = ComputerStatusEvent), check that local for null, and then await handler.Invoke(status) so the captured delegate cannot become null between the check and invocation; update the SendComputerStatus method to use this delegate-capture pattern.
🤖 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/CommonLib/Processors/RegistryProcessor.cs`:
- Around line 92-97: The success message in RegistryProcessor (inside
ReadRegistrySettings) should not infer the successful strategy via
_strategies[collectedData.FailureAttempts?.Count() ?? 0]; add a
SuccessfulStrategyType property to StrategyExecutorResult<T>, set that property
to the strategy's GetType() on the success path inside
StrategyExecutor.CollectAsync when a strategy succeeds, and update
RegistryProcessor to use collectedData.SuccessfulStrategyType?.Name (or
fallback) for the Task string instead of indexing _strategies; this removes
fragile coupling to _strategies ordering and prevents IndexOutOfRangeException.
In `@test/unit/RegistryProcessorTests.cs`:
- Around line 164-165: The Assert.Equal call in RegistryProcessorTests.cs has
its arguments reversed: swap them so the expected value is first and the actual
is second; change the assertion to call Assert.Equal(exception.ToString(),
results.FailureReason) (leave Assert.False(results.Collected) as-is) so xUnit's
expected/actual diagnostics are correct.
---
Nitpick comments:
In `@src/CommonLib/Processors/ComputerAvailability.cs`:
- Around line 98-110: Add a trace log to the _skipPortScan success branch to
match the other success path: call _log.LogTrace with a message like
"{ComputerName} is available for enumeration" (using the computerName variable)
immediately before calling SendComputerStatus in the block that constructs the
CSVComputerStatus and returns a ComputerStatus; this keeps logging parity with
the other path that logs before SendComputerStatus.
In `@src/CommonLib/Processors/RegistryProcessor.cs`:
- Around line 146-148: The current SendComputerStatus method checks
ComputerStatusEvent for null then awaits ComputerStatusEvent.Invoke(status),
which can race if handlers are removed concurrently; fix it by capturing the
event to a local variable (e.g., var handler = ComputerStatusEvent), check that
local for null, and then await handler.Invoke(status) so the captured delegate
cannot become null between the check and invocation; update the
SendComputerStatus method to use this delegate-capture pattern.
In `@test/unit/Facades/MockExtentions.cs`:
- Around line 36-46: The VerifyNoLogs<T> helper uses a non-nullable formatter
signature which is inconsistent with VerifyLog and VerifyLogContains; update the
formatter parameter type in VerifyNoLogs<T> from Func<It.IsAnyType, Exception,
string> to Func<It.IsAnyType, Exception?, string> so the nullable Exception
annotation matches the other helper methods (locate the VerifyNoLogs<T> method
and adjust the formatter type accordingly).
In `@test/unit/RegistryProcessorTests.cs`:
- Around line 152-170: The test
RegistryProcessor_ReadRegistrySettings_HandlesException is setting up the async
CollectAsync call with .Throws(exception) which triggers a synchronous throw;
update the mock setup for _mockStrategyExecutor.Setup(se =>
se.CollectAsync(...)) to use .ThrowsAsync(exception) so the setup returns a
faulted Task and better simulates async failure, keeping the rest of the
assertions (results and log verification) unchanged.
7 tasks
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/unit/RegistryProcessorTests.cs (1)
152-156: Prefer.ThrowsAsync()over.Throws()for the asyncCollectAsyncmock.Moq documents
ThrowsAsyncas the correct counterpart toThrowsfor async methods — synchronous methods use.Throws(), async methods use.ThrowsAsync(). With.Throws(), the mock throws synchronously at the call site before aTaskis ever returned. While this still gets caught by thetry-catchinReadRegistrySettingstoday, it doesn't accurately model the faulted-Taskpath that a real async implementation would produce.♻️ Suggested fix
- .Throws(exception); + .ThrowsAsync(exception);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/RegistryProcessorTests.cs` around lines 152 - 156, The test is mocking an async method but uses Moq.Throws which throws synchronously; change the mock setup on _mockStrategyExecutor for CollectAsync to use .ThrowsAsync(exception) so the mock returns a faulted Task (rather than throwing at call time) to more accurately simulate async failure; update the Setup call that targets CollectAsync and replace .Throws(...) with .ThrowsAsync(...) while keeping the same exception instance so ReadRegistrySettings exercises the faulted-Task path.src/SharpHoundRPC/Registry/StrategyExecutorResult.cs (1)
11-12:WasSuccessfulis a public field while all other members are properties.Since this class is being touched anyway to add
SuccessfulStrategy, convertingWasSuccessfulto a property would make the class consistent.♻️ Suggested refactor
- public bool WasSuccessful = false; + public bool WasSuccessful { get; set; } = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SharpHoundRPC/Registry/StrategyExecutorResult.cs` around lines 11 - 12, Convert the public field WasSuccessful in class StrategyExecutorResult to a public auto-property (public bool WasSuccessful { get; set; } = false;) so it matches the rest of the class style (like SuccessfulStrategy) and preserves the default value; update any references that access the field if necessary to use the property name (WasSuccessful) — leave SuccessfulStrategy as is.src/SharpHoundRPC/Registry/StrategyExecutor.cs (1)
7-13: Consider movingIStrategyExecutorto its own file.Conventionally, interfaces live in dedicated files (e.g.,
IStrategyExecutor.cs). Co-locating it withStrategyExecutor.csmakes it harder to discover and violates the single-responsibility principle for file organisation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SharpHoundRPC/Registry/StrategyExecutor.cs` around lines 7 - 13, Move the IStrategyExecutor interface out of StrategyExecutor.cs into its own file named IStrategyExecutor.cs: create a new file containing the IStrategyExecutor definition (preserving the existing namespace and the generic CollectAsync<T, TQuery> signature and return type Task<StrategyExecutorResult<T>>), remove the interface from StrategyExecutor.cs so that only the StrategyExecutor class remains there, and ensure any using directives and project file entries (if needed) are updated so the compiler still resolves IStrategyExecutor correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/SharpHoundRPC/Registry/StrategyExecutor.cs`:
- Around line 7-13: Move the IStrategyExecutor interface out of
StrategyExecutor.cs into its own file named IStrategyExecutor.cs: create a new
file containing the IStrategyExecutor definition (preserving the existing
namespace and the generic CollectAsync<T, TQuery> signature and return type
Task<StrategyExecutorResult<T>>), remove the interface from StrategyExecutor.cs
so that only the StrategyExecutor class remains there, and ensure any using
directives and project file entries (if needed) are updated so the compiler
still resolves IStrategyExecutor correctly.
In `@src/SharpHoundRPC/Registry/StrategyExecutorResult.cs`:
- Around line 11-12: Convert the public field WasSuccessful in class
StrategyExecutorResult to a public auto-property (public bool WasSuccessful {
get; set; } = false;) so it matches the rest of the class style (like
SuccessfulStrategy) and preserves the default value; update any references that
access the field if necessary to use the property name (WasSuccessful) — leave
SuccessfulStrategy as is.
In `@test/unit/RegistryProcessorTests.cs`:
- Around line 152-156: The test is mocking an async method but uses Moq.Throws
which throws synchronously; change the mock setup on _mockStrategyExecutor for
CollectAsync to use .ThrowsAsync(exception) so the mock returns a faulted Task
(rather than throwing at call time) to more accurately simulate async failure;
update the Setup call that targets CollectAsync and replace .Throws(...) with
.ThrowsAsync(...) while keeping the same exception instance so
ReadRegistrySettings exercises the faulted-Task path.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/SharpHoundRPC/Registry/StrategyExecutor.cs (1)
42-46:{ex.InnerException}emits the full stack trace intoFailureReason.In C# string interpolation,
{ex.InnerException}callsToString()on the exception, which includes the exception type, message, and the full stack trace. For aFailureReasonfield that feeds into CSV status events, this produces a very long multi-line value that may cause parsing issues or excessively noisy logs. If only concise identification is needed, preferex.InnerException.Messageor$"{ex.InnerException.GetType().Name}: {ex.InnerException.Message}".♻️ Proposed change
- var innerException = ex.InnerException != null - ? $"\nInner Exception: {ex.InnerException}" - : string.Empty; + var innerException = ex.InnerException != null + ? $"\nInner Exception: {ex.InnerException.GetType().Name}: {ex.InnerException.Message}" + : string.Empty;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SharpHoundRPC/Registry/StrategyExecutor.cs` around lines 42 - 46, The current construction of innerException uses {ex.InnerException} which calls ToString() and injects a multi-line stack trace into attempt.FailureReason; change it to include only a concise identifier such as ex.InnerException.Message or $"{ex.InnerException.GetType().Name}: {ex.InnerException.Message}" and guard for null (use ex.InnerException != null ? ... : string.Empty) so StrategyExecutor.cs still sets attempt.FailureReason but without the full stack trace.RPCTest/Registry/StrategyExecutorTests.cs (1)
122-128:FakeCollectionStrategy.ExecuteAsyncthrows synchronously — considerTask.FromExceptioninstead.
throw exceptionhere is synchronous and executes before returning aTask. It works because C#'s async state machine catches synchronous throws insidetry/catchblocks ofasyncmethods. However, the conventional way to simulate a failed async operation in tests is to return a faultedTask:- if (exception is not null) - throw exception; - - return Task.FromResult(results ?? []); + if (exception is not null) + return Task.FromException<IEnumerable<RegistryQueryResult>>(exception); + + return Task.FromResult(results ?? Enumerable.Empty<RegistryQueryResult>());This ensures the test exercises the
await-on-faulted-Task path, which is what real strategy implementations would produce.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RPCTest/Registry/StrategyExecutorTests.cs` around lines 122 - 128, The FakeCollectionStrategy.ExecuteAsync currently throws the stored exception synchronously which bypasses the async faulted-Task behavior tests should exercise; change the method so that when exception is non-null it returns Task.FromException<IEnumerable<RegistryQueryResult>>(exception) instead of throwing, and otherwise return Task.FromResult(results ?? Enumerable.Empty<RegistryQueryResult>()); update ExecuteAsync to use Task.FromException for the error path and Task.FromResult for the success path.SharpHoundCommon.sln (1)
12-13: Consider placingRPCTestunder thetest/folder for consistency.
CommonLibTestlives attest\unit\whileRPCTestis being added at the solution root. Aligning the two test projects under a commontest/subtree keeps the repository structure consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SharpHoundCommon.sln` around lines 12 - 13, The RPCTest project is added at the solution root while other tests (e.g., CommonLibTest) live under test\unit; move RPCTest into the repository's test subtree for consistency by updating the solution to reference RPCTest from a test/ location and relocating the RPCTest project folder accordingly; ensure the Project entry for "RPCTest" (the GUID {F1E6714E-72B3-4295-9940-0EAA69695201} and project name RPCTest) is updated in the solution file to point to the new relative path and adjust any project references or build items that assume the old path.RPCTest/RPCTest.csproj (1)
11-12:xunitandxunit.runner.visualstudioversion 2.4.1 are significantly outdated.The latest xunit v2 is
2.9.3, last updated January 2025. The xunit.net v2 getting-started template now usesxunit 2.9.3andxunit.runner.visualstudio 3.1.1. Using a 2019-era release means missing years of bug fixes and improvements. Consider upgrading to match the current stable baseline, or aligning with whatever versionCommonLibTestuses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RPCTest/RPCTest.csproj` around lines 11 - 12, The project references outdated test packages: update the PackageReference entries for "xunit" and "xunit.runner.visualstudio" in RPCTest.csproj to current stable versions (e.g., set xunit to 2.9.3 and xunit.runner.visualstudio to 3.1.1) or align them with the versions used by CommonLibTest; after editing the PackageReference Version attributes for "xunit" and "xunit.runner.visualstudio", run a restore/build and execute the test suite to verify compatibility and fix any breaking changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@RPCTest/Registry/StrategyExecutorTests.cs`:
- Around line 92-109: The test CollectAsync_SecondStrategySuccessful uses two
FakeCollectionStrategy instances so the assertion
Assert.Equal(successfulStrategy.GetType(), result.SuccessfulStrategy) doesn't
prove the executor picked the successful one; change the test to use a distinct
fake type for the successful strategy (e.g., add a sealed
FakeSuccessCollectionStrategy or another inner test-only class) and update the
assertion to expect that concrete type (or compare the instance identity) so the
check validates that StrategyExecutor stored the successful strategy rather than
just the same type as the failed one.
In `@RPCTest/RPCTest.csproj`:
- Around line 10-15: The project file is missing an explicit PackageReference to
Microsoft.NET.Test.Sdk which prevents dotnet test from running; open the
RPCTest.csproj ItemGroup where PackageReference entries for xunit and
xunit.runner.visualstudio are defined and add a PackageReference for
Microsoft.NET.Test.Sdk (specify a recent stable version, e.g. 17.x), and
optionally update PackageReference versions for xunit (from 2.4.1 to 2.9.3) and
xunit.runner.visualstudio (from 2.4.1 to 2.4.3) to match CommonLibTest.
---
Nitpick comments:
In `@RPCTest/Registry/StrategyExecutorTests.cs`:
- Around line 122-128: The FakeCollectionStrategy.ExecuteAsync currently throws
the stored exception synchronously which bypasses the async faulted-Task
behavior tests should exercise; change the method so that when exception is
non-null it returns
Task.FromException<IEnumerable<RegistryQueryResult>>(exception) instead of
throwing, and otherwise return Task.FromResult(results ??
Enumerable.Empty<RegistryQueryResult>()); update ExecuteAsync to use
Task.FromException for the error path and Task.FromResult for the success path.
In `@RPCTest/RPCTest.csproj`:
- Around line 11-12: The project references outdated test packages: update the
PackageReference entries for "xunit" and "xunit.runner.visualstudio" in
RPCTest.csproj to current stable versions (e.g., set xunit to 2.9.3 and
xunit.runner.visualstudio to 3.1.1) or align them with the versions used by
CommonLibTest; after editing the PackageReference Version attributes for "xunit"
and "xunit.runner.visualstudio", run a restore/build and execute the test suite
to verify compatibility and fix any breaking changes.
In `@SharpHoundCommon.sln`:
- Around line 12-13: The RPCTest project is added at the solution root while
other tests (e.g., CommonLibTest) live under test\unit; move RPCTest into the
repository's test subtree for consistency by updating the solution to reference
RPCTest from a test/ location and relocating the RPCTest project folder
accordingly; ensure the Project entry for "RPCTest" (the GUID
{F1E6714E-72B3-4295-9940-0EAA69695201} and project name RPCTest) is updated in
the solution file to point to the new relative path and adjust any project
references or build items that assume the old path.
In `@src/SharpHoundRPC/Registry/StrategyExecutor.cs`:
- Around line 42-46: The current construction of innerException uses
{ex.InnerException} which calls ToString() and injects a multi-line stack trace
into attempt.FailureReason; change it to include only a concise identifier such
as ex.InnerException.Message or $"{ex.InnerException.GetType().Name}:
{ex.InnerException.Message}" and guard for null (use ex.InnerException != null ?
... : string.Empty) so StrategyExecutor.cs still sets attempt.FailureReason but
without the full stack trace.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
RPCTest/RPCTest.csprojRPCTest/Registry/StrategyExecutorTests.csSharpHoundCommon.slnsrc/SharpHoundRPC/Registry/StrategyExecutor.cs
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.
Description
Add CompStatus Logging To Registry Processor
RunLog on Collection Failure
RegistryProcessor Unit Tests
Motivation and Context
BED-7374
How Has This Been Tested?
Unit Tests Pass
Validated in lab that:
Comp Status Logs on Success
RunLog and CompStatus Log on Failure
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores