Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request updates the PR template markdown content, restructures the build workflow from monolithic to per-component builds triggered on pull requests to 2.X branches, and introduces conditional dependency resolution in Sharphound.csproj that switches between NuGet packages and pre-built local library references based on path existence. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub as GitHub<br/>(PR Trigger)
participant SharpHoundCommon as SharpHoundCommon<br/>(Component 1)
participant SharpHound as SharpHound<br/>(Component 2)
participant BuildSystem as Build System
GitHub->>GitHub: PR opened/synchronized on 2.X
GitHub->>BuildSystem: Trigger workflow
BuildSystem->>SharpHoundCommon: Checkout repository
activate SharpHoundCommon
BuildSystem->>BuildSystem: Build SharpHoundCommon solution
deactivate SharpHoundCommon
BuildSystem->>SharpHound: Checkout repository
activate SharpHound
BuildSystem->>BuildSystem: Build SharpHound project<br/>(with versioning)
deactivate SharpHound
BuildSystem->>GitHub: Report build status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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: 4
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
31-36: Pin the SharpHoundCommon checkout to a stable ref.Using
ref: mainmakes2.XPR builds non-reproducible: CI can start failing or changing behavior because of unrelated commits in the external repo. A tag, commit SHA, or matching maintenance branch is safer here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 31 - 36, The GitHub Actions checkout step using actions/checkout@v4 for repository SpecterOps/SharpHoundCommon currently sets ref: main; change that to a stable immutable ref (a release tag, commit SHA, or a maintenance branch) to make CI reproducible — locate the checkout step (uses: actions/checkout@v4 with repository: SpecterOps/SharpHoundCommon and ref: main) and replace ref: main with a specific tag (e.g., v2.x.y) or the exact commit SHA or a dedicated maintenance branch name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/pull_request_template.md:
- Around line 30-34: Replace the hardcoded issue URL on the checklist line that
currently contains "Associated an issue:
https://github.com/SpecterOps/BloodHound/issues/672" with a neutral
example/placeholder (e.g., "Associated an issue:
https://github.com/owner/repo/issues/NNN" or "Associated an issue: <issue
link>") so the pull request template doesn't carry an unrelated, pre-filled
issue; update the checklist entry text near "[ ] I have met the contributing
prerequisites" to use that placeholder instead.
In @.github/workflows/build.yml:
- Around line 46-50: The Restore Dependencies and Build steps ("Restore
Dependencies" and "Build") run from $GITHUB_WORKSPACE but the repo was checked
out into the SharpHound/ subfolder, so dotnet restore and dotnet build won't
find the solution; update those steps to either set working-directory:
SharpHound (or the appropriate subpath) or pass the solution path to the
commands (e.g., dotnet restore SharpHound/YourSolution.sln and dotnet build -c
${{ matrix.release.type }} -p:Version=0.0.0-rolling+${{ github.sha }}
SharpHound/YourSolution.sln) so the commands run against the checked-out project
files.
In `@Sharphound.csproj`:
- Around line 38-49: The ItemGroup conditions currently only check
$(CommonLibPath) so if $(CommonLibPath) exists but $(RPCPath) does not, the
project switches to local References with a missing SharpHoundRPC; update the
ItemGroup Condition expressions to require both paths: change the package-using
branch condition from !Exists('$(CommonLibPath)') to !Exists('$(CommonLibPath)')
Or !Exists('$(RPCPath)') and change the local-Reference branch from
Exists('$(CommonLibPath)') to Exists('$(CommonLibPath)') And
Exists('$(RPCPath)') so both $(CommonLibPath) and $(RPCPath) must exist before
using the local <Reference> entries for SharpHoundCommonLib and SharpHoundRPC.
- Around line 17-20: The hardcoded local DLL paths (CommonLibPath and RPCPath)
use "bin\Debug\..." so Release builds can't find them; update the property
values for CommonLibPath and RPCPath to use the MSBuild configuration variable
(e.g. replace "bin\Debug" with "bin\$(Configuration)") so both Debug and Release
artifacts are located correctly, then ensure any existence checks like
Exists('$(CommonLibPath)') continue to reference the updated properties so the
project validates the locally built DLLs instead of falling back to NuGet.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 31-36: The GitHub Actions checkout step using actions/checkout@v4
for repository SpecterOps/SharpHoundCommon currently sets ref: main; change that
to a stable immutable ref (a release tag, commit SHA, or a maintenance branch)
to make CI reproducible — locate the checkout step (uses: actions/checkout@v4
with repository: SpecterOps/SharpHoundCommon and ref: main) and replace ref:
main with a specific tag (e.g., v2.x.y) or the exact commit SHA or a dedicated
maintenance branch name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b726a48-3f96-4dd0-a182-0937ef939da5
📒 Files selected for processing (3)
.github/pull_request_template.md.github/workflows/build.ymlSharphound.csproj
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sharphound.csproj (1)
38-49:⚠️ Potential issue | 🟠 MajorRequire both local artifacts before switching away from NuGet.
The condition still only checks
$(CommonLibPath). IfSharpHoundCommonLib.dllexists butSharpHoundRPC.dlldoesn't, Line 42 still selects the local-reference branch and leavesSharpHoundRPCpointing at a missing file.Suggested fix
- <ItemGroup Condition="!Exists('$(CommonLibPath)')"> + <ItemGroup Condition="!Exists('$(CommonLibPath)') Or !Exists('$(RPCPath)')"> <PackageReference Include="SharpHoundCommon" Version="4.5.3" /> <PackageReference Include="SharpHoundRPC" Version="4.5.3" /> </ItemGroup> - <ItemGroup Condition="Exists('$(CommonLibPath)')"> + <ItemGroup Condition="Exists('$(CommonLibPath)') And Exists('$(RPCPath)')"> <Reference Include="SharpHoundCommonLib"> <HintPath>$(CommonLibPath)</HintPath> </Reference>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sharphound.csproj` around lines 38 - 49, The ItemGroup conditions only check Exists('$(CommonLibPath)') so the project can select local references even if SharpHoundRPC (RPCPath) is missing; change the logic so the package-based ItemGroup is used if either CommonLibPath or RPCPath is missing and the local-reference ItemGroup is used only when both exist. Concretely, update the first ItemGroup Condition to check for !Exists('$(CommonLibPath)') OR !Exists('$(RPCPath)') and update the second ItemGroup Condition to require Exists('$(CommonLibPath)') AND Exists('$(RPCPath)') so both SharpHoundCommonLib and SharpHoundRPC are present before switching away from NuGet.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 38-39: The "Build SharpHoundCommon" step runs dotnet build from
the repository root which can cause NuGet to miss the dependency's local
nuget.config; update that step (the step named "Build SharpHoundCommon" that
currently runs `dotnet build SharpHoundCommon/SharpHoundCommon.sln -c ${{
matrix.release.type }}`) to execute the build from the SharpHoundCommon checkout
directory—either by setting the GitHub Actions working-directory to
"SharpHoundCommon" for that step or by changing the command to run the build
from that directory—so the project's local nuget.config is discovered and used
during restore/build.
- Around line 46-50: Remove the explicit "Restore SharpHound Dependencies" step
that runs `dotnet restore SharpHound/Sharphound.csproj` because
Sharphound.csproj uses $(Configuration) and restoring without `-c` causes a
config mismatch; instead rely on the existing "Build SharpHound" step which runs
`dotnet build SharpHound/Sharphound.csproj -c ${{ matrix.release.type }}`,
ensuring restore and build use the same `matrix.release.type` configuration and
the project picks the correct local vs NuGet references.
---
Duplicate comments:
In `@Sharphound.csproj`:
- Around line 38-49: The ItemGroup conditions only check
Exists('$(CommonLibPath)') so the project can select local references even if
SharpHoundRPC (RPCPath) is missing; change the logic so the package-based
ItemGroup is used if either CommonLibPath or RPCPath is missing and the
local-reference ItemGroup is used only when both exist. Concretely, update the
first ItemGroup Condition to check for !Exists('$(CommonLibPath)') OR
!Exists('$(RPCPath)') and update the second ItemGroup Condition to require
Exists('$(CommonLibPath)') AND Exists('$(RPCPath)') so both SharpHoundCommonLib
and SharpHoundRPC are present before switching away from NuGet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a3ae223-e0c6-4a95-b3e8-f3fa14bfd172
📒 Files selected for processing (2)
.github/workflows/build.ymlSharphound.csproj
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
44-45:⚠️ Potential issue | 🟠 MajorRemove the explicit restore step to avoid configuration mismatch.
Sharphound.csprojuses$(Configuration)to conditionally switch between NuGet packages and local file references (seeCondition="!Exists('$(CommonLibPath)')"andCondition="Exists('$(CommonLibPath)')"). The restore at Line 45 runs without-c, defaulting toDebug, while the build at Line 48 uses-c ${{ matrix.release.type }}. This causes restore to evaluate the wrong conditional references when buildingRelease.Since
dotnet buildperforms an implicit restore with the same properties, the explicit restore step is unnecessary and reintroduces a previously-addressed issue.Suggested fix
- name: Checkout SharpHound uses: actions/checkout@v4 with: path: SharpHound - - name: Restore SharpHound Dependencies - run: dotnet restore SharpHound/Sharphound.csproj - - name: Build SharpHound run: dotnet build SharpHound/Sharphound.csproj -c ${{ matrix.release.type }} -p:Version=0.0.0-rolling+${{ github.sha }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 44 - 45, Remove the explicit "Restore SharpHound Dependencies" step that runs "dotnet restore SharpHound/Sharphound.csproj" because it restores without the -c flag and can evaluate Condition="Exists('$(CommonLibPath)')" incorrectly; instead rely on the implicit restore performed by the subsequent "dotnet build -c ${{ matrix.release.type }}" invocation so the same Configuration property is used, i.e., delete the step named "Restore SharpHound Dependencies" (the explicit dotnet restore against SharpHound/Sharphound.csproj) from the GitHub Actions workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Line 33: Update the checkout reference and remove the inline TODO: replace the
temporary branch reference "ref: lfalslev/bed-7214" and its accompanying TODO
with a stable ref (e.g., "ref: v4" or a release/tag variable) so the workflow no
longer depends on the feature branch; ensure the "ref" key in the checkout step
is updated and the TODO comment is deleted.
In `@src/Runtime/ObjectProcessors.cs`:
- Around line 54-55: Remove the temporary test change: delete the TODO comment
and the instantiation that passes an empty string to the CertAbuseProcessor and
revert the initialization of _certAbuseProcessor to the original/pre- test call;
locate the line where _certAbuseProcessor is set (using CertAbuseProcessor,
context.LDAPUtils, RegistryAccessor, SAMServerAccessor) and restore the prior
constructor parameters/signature, then remove the temporary commit that added
the pipeline test.
---
Duplicate comments:
In @.github/workflows/build.yml:
- Around line 44-45: Remove the explicit "Restore SharpHound Dependencies" step
that runs "dotnet restore SharpHound/Sharphound.csproj" because it restores
without the -c flag and can evaluate Condition="Exists('$(CommonLibPath)')"
incorrectly; instead rely on the implicit restore performed by the subsequent
"dotnet build -c ${{ matrix.release.type }}" invocation so the same
Configuration property is used, i.e., delete the step named "Restore SharpHound
Dependencies" (the explicit dotnet restore against SharpHound/Sharphound.csproj)
from the GitHub Actions workflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd3b400b-1736-4b46-8ad8-45ff8bed58e2
📒 Files selected for processing (2)
.github/workflows/build.ymlsrc/Runtime/ObjectProcessors.cs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 44-48: The explicit dotnet restore step for Sharphound.csproj runs
without the -c flag which causes $(Configuration)/Exists('$(CommonLibPath)')
logic in Sharphound.csproj to evaluate as Debug while the subsequent dotnet
build uses matrix.release.type (e.g. Release), leading to mismatched package vs
local DLL resolution; fix by removing the explicit "dotnet restore
SharpHound/Sharphound.csproj" step entirely so build's implicit restore uses the
same configuration, or if you must keep it, change that step to pass -c ${{
matrix.release.type }} so restore and build use the same configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a07c2a4-7c66-4b3c-8b4a-7f184be6df80
📒 Files selected for processing (1)
.github/workflows/build.yml
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
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit