Open
Conversation
WalkthroughIntroduces an IStrategyExecutor interface and injects it into RegistryProcessor; RegistryProcessor now emits per-attempt CSVComputerStatus events. Adds input validation to DotNetWmiRegistryStrategy, an early-exit status report when port scanning is skipped, small import cleanups, and extensive unit tests for registry collection flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RegistryProcessor
participant IStrategyExecutor
participant ComputerStatusEvent
Client->>RegistryProcessor: ReadRegistrySettings(targetMachine)
RegistryProcessor->>IStrategyExecutor: CollectAsync(targetMachine, queries, strategies)
loop per strategy attempt
IStrategyExecutor->>IStrategyExecutor: Execute strategy attempt
alt attempt fails
IStrategyExecutor-->>RegistryProcessor: attempt failure details
RegistryProcessor->>ComputerStatusEvent: Publish CSVComputerStatus (failure)
else attempt succeeds
IStrategyExecutor-->>RegistryProcessor: StrategyExecutorResult (WasSuccessful=true, SuccessfulStrategy)
end
end
alt overall success
RegistryProcessor->>ComputerStatusEvent: Publish CSVComputerStatus (success with 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)
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.
Comment on lines
92
to
97
| await SendComputerStatus(new CSVComputerStatus | ||
| { | ||
| Task = $"{nameof(ReadRegistrySettings)} - {_strategies[collectedData.FailureAttempts?.Count() ?? 0].GetType().Name}", | ||
| ComputerName = targetMachine, | ||
| Status = CSVComputerStatus.StatusSuccess | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine StrategyExecutorResult.cs
echo "=== Locating StrategyExecutorResult.cs ==="
fd "StrategyExecutorResult.cs" -type f
echo ""
echo "=== Locating StrategyExecutor.cs ==="
fd "StrategyExecutor.cs" -type f
echo ""
echo "=== Locating RegistryProcessor.cs ==="
fd "RegistryProcessor.cs" -type fRepository: SpecterOps/SharpHoundCommon
Length of output: 720
🏁 Script executed:
#!/bin/bash
# Examine the StrategyExecutorResult.cs file
if [ -f "$(fd 'StrategyExecutorResult.cs' -type f | head -1)" ]; then
cat "$(fd 'StrategyExecutorResult.cs' -type f | head -1)"
fiRepository: SpecterOps/SharpHoundCommon
Length of output: 241
🏁 Script executed:
#!/bin/bash
# Examine StrategyExecutor.cs - look for success path and WasSuccessful assignments
echo "=== StrategyExecutor.cs - Full file (if manageable) ==="
fd "StrategyExecutor.cs" -type f -exec wc -l {} \;
fd "StrategyExecutor.cs" -type f -exec head -200 {} \;
echo ""
echo "=== Searching for WasSuccessful assignments ==="
rg -n "WasSuccessful\s*=" --type csRepository: SpecterOps/SharpHoundCommon
Length of output: 1154
🏁 Script executed:
#!/bin/bash
# Examine RegistryProcessor.cs around lines 92-97
echo "=== RegistryProcessor.cs lines 85-105 ==="
sed -n '85,105p' src/CommonLib/Processors/RegistryProcessor.cs
echo ""
echo "=== Search for _strategies field in RegistryProcessor ==="
rg -n "_strategies" src/CommonLib/Processors/RegistryProcessor.cs -A2 -B2Repository: SpecterOps/SharpHoundCommon
Length of output: 2263
🏁 Script executed:
#!/bin/bash
# Examine StrategyExecutorResult.cs
echo "=== StrategyExecutorResult.cs ==="
cat src/SharpHoundRPC/Registry/StrategyExecutorResult.csRepository: SpecterOps/SharpHoundCommon
Length of output: 457
🏁 Script executed:
#!/bin/bash
# Examine StrategyExecutor.cs - look at the success path around line 38
echo "=== StrategyExecutor.cs context around WasSuccessful = true ==="
sed -n '30,50p' src/SharpHoundRPC/Registry/StrategyExecutor.cs
echo ""
echo "=== Full StrategyExecutor.cs (check file size) ==="
wc -l src/SharpHoundRPC/Registry/StrategyExecutor.csRepository: SpecterOps/SharpHoundCommon
Length of output: 982
🏁 Script executed:
#!/bin/bash
# Get more context from StrategyExecutor.cs - check the full file
cat src/SharpHoundRPC/Registry/StrategyExecutor.csRepository: SpecterOps/SharpHoundCommon
Length of output: 1991
🏁 Script executed:
#!/bin/bash
# Examine StrategyResult to see what info is available
echo "=== StrategyResult.cs or related ==="
fd "StrategyResult.cs" -type f
echo ""
echo "=== Search for StrategyResult definition and StrategyType ==="
rg -n "class StrategyResult|StrategyType" src/SharpHoundRPC/Registry/ -A3Repository: SpecterOps/SharpHoundCommon
Length of output: 1705
🏁 Script executed:
#!/bin/bash
# Verify the _strategies array definition in RegistryProcessor to confirm ordering assumptions
echo "=== RegistryProcessor._strategies initialization ==="
sed -n '27,37p' src/CommonLib/Processors/RegistryProcessor.csRepository: SpecterOps/SharpHoundCommon
Length of output: 780
🏁 Script executed:
#!/bin/bash
# Check if there are any other usages of FailureAttempts?.Count() that might have similar issues
echo "=== Search for FailureAttempts usage ==="
rg "FailureAttempts" src/ -n -A2 -B2Repository: SpecterOps/SharpHoundCommon
Length of output: 3475
Fragile strategy-name resolution via index arithmetic.
Line 94 infers the successful strategy by indexing _strategies[collectedData.FailureAttempts?.Count() ?? 0]. This silently couples RegistryProcessor to the internal iteration order of StrategyExecutor and the exact composition of _strategies. If StrategyExecutor ever skips a strategy without recording a failure attempt, reorders strategies, or if _strategies is modified independently, this will silently report the wrong strategy name—or throw IndexOutOfRangeException.
The successful strategy type is available in StrategyExecutor.CollectAsync at the point of success but is never captured in the result. Add a SuccessfulStrategyType property to StrategyExecutorResult<T> and set it on the success path, then use that instead of array indexing.
Proposed fix in StrategyExecutorResult
In StrategyExecutorResult.cs:
public class StrategyExecutorResult<T> {
public IEnumerable<T>? Results { get; set; } = null;
public IEnumerable<StrategyResult<T>>? FailureAttempts { get; set; } = null;
public bool WasSuccessful = false;
+ public Type? SuccessfulStrategyType { get; set; } = null;
}In StrategyExecutor.cs, set it on the success path:
return new StrategyExecutorResult<T> {
Results = results,
FailureAttempts = attempts,
- WasSuccessful = true
+ WasSuccessful = true,
+ SuccessfulStrategyType = strategy.GetType()
};Then in RegistryProcessor.cs:
await SendComputerStatus(new CSVComputerStatus
{
- Task = $"{nameof(ReadRegistrySettings)} - {_strategies[collectedData.FailureAttempts?.Count() ?? 0].GetType().Name}",
+ Task = $"{nameof(ReadRegistrySettings)} - {collectedData.SuccessfulStrategyType?.Name ?? "Unknown"}",
ComputerName = targetMachine,
Status = CSVComputerStatus.StatusSuccess
});🤖 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 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.
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.
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