chore: SHC -dev package workflows gracefully handle -rc packages BED-7878#215
Open
chore: SHC -dev package workflows gracefully handle -rc packages BED-7878#215
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
WalkthroughConsolidated Common library selection into a single Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions (publish.yml)"
participant MS as "MSBuild (Sharphound.csproj)"
participant FS as "Local filesystem (DLLs)"
participant NuGet as "NuGet registry"
participant Rel as "Release/upload step"
GH->>MS: invoke dotnet build -p:CommonSource=...
rect rgba(100,150,240,0.5)
MS->>MS: Normalize & Validate `CommonSource` (Dev | Stable | Local)
MS->>MS: Compute `_ResolvedCommonVersion` (dev wildcard if needed)
alt _CommonSource == Local
MS->>FS: Read `_CommonLibPath` / `_RPCPath`
MS-->>MS: Use `<Reference>` entries with local DLLs
else
MS->>NuGet: Resolve `SharpHoundCommon`/`SharpHoundRPC`@_ResolvedCommonVersion
MS-->>MS: Use `<PackageReference>` entries
end
end
MS->>Rel: produce package/artifact
Rel->>GH: upload artifacts and checksums
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 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/publish.yml:
- Line 39: The CI step passes the wrong MSBuild property name
(-p:CommonLibSource=Stable) so Sharphound.csproj still reads CommonSource and
uses its default; replace the property with -p:CommonSource=Stable in the
publish step (the dotnet build/dotnet publish invocation that currently sets
CommonLibSource) so the pipeline overrides the project’s CommonSource value
properly for tagged/stable builds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21c150f7-95fd-4b3b-bb65-d37b6747940e
📒 Files selected for processing (2)
.github/workflows/publish.ymlSharphound.csproj
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/publish.yml (1)
39-39: Consider aligning README flags with the new property name.The workflow uses
CommonSource, but README.md (Line 26-28) still showsCommonLibSource. Updating docs will reduce build-flag confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish.yml at line 39, Update README.md to match the workflow property name: replace references to CommonLibSource with CommonSource (the property used in the publish workflow and dotnet build call) and adjust any example build commands or flag documentation to use -p:CommonSource=${VERSION} (or the existing usage pattern). Ensure any explanatory text, examples, and CI instructions that mention CommonLibSource are updated to the new symbol CommonSource so documentation and the publish workflow stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/publish.yml:
- Line 39: Update README.md to match the workflow property name: replace
references to CommonLibSource with CommonSource (the property used in the
publish workflow and dotnet build call) and adjust any example build commands or
flag documentation to use -p:CommonSource=${VERSION} (or the existing usage
pattern). Ensure any explanatory text, examples, and CI instructions that
mention CommonLibSource are updated to the new symbol CommonSource so
documentation and the publish workflow stay consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c02f2639-8b13-4cd7-80ac-3c6e0a6a0a4f
📒 Files selected for processing (2)
.github/workflows/publish.ymlSharphound.csproj
🚧 Files skipped from review as they are similar to previous changes (1)
- Sharphound.csproj
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 `@README.md`:
- Around line 24-27: The README wording is inaccurate about how package versions
are chosen; update the text to state that Sharphound.csproj uses the
CommonLibsVersion MSBuild property to determine the SharpHoundCommon package
version (including any prerelease suffixes like -rc2 or -dev) rather than always
mapping "Dev" to a -dev build from main or "Stable" to the latest release;
change the instructions to explain that passing -p:CommonSource=Stable overrides
the source resolution but the resolved version still comes from
CommonLibsVersion, and reference the CommonLibsVersion property and
Sharphound.csproj so readers know where the resolution logic lives.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d206fd2-13b4-4580-94af-840d7bfa4444
📒 Files selected for processing (2)
README.mdSharphound.csproj
✅ Files skipped from review due to trivial changes (1)
- Sharphound.csproj
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sharphound.csproj (1)
30-36: Clever guard pattern for semver compatibility.The
!$(CommonLibsVersion.Contains('-'))condition is crucial:System.Version.Parsecannot handle semver prerelease suffixes (e.g.,-rc2,-dev123), so this guard ensures parsing is only attempted on clean version strings. This is the key fix enabling graceful handling of-rcpackages.Consider adding a brief comment explaining why the
-check exists, since it's not immediately obvious thatSystem.Versionwould fail on prerelease suffixes:📝 Optional documentation improvement
- <PropertyGroup Condition="'$(_CommonSource)' == 'dev' AND !$(CommonLibsVersion.Contains('-'))"> - <!-- Derive the dev version by incrementing the patch of the stable version e.g. 4.6.0 → 4.6.1-dev* --> + <!-- Skip derivation when version already has a prerelease suffix (e.g., -rc2, -dev*), + as System.Version.Parse cannot handle semver prerelease identifiers --> + <PropertyGroup Condition="'$(_CommonSource)' == 'dev' AND !$(CommonLibsVersion.Contains('-'))"> + <!-- Derive dev version by incrementing patch: 4.6.0 → 4.6.1-dev* -->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sharphound.csproj` around lines 30 - 36, Add a short inline comment explaining the purpose of the guard on the PropertyGroup Condition that uses !$(CommonLibsVersion.Contains('-')), noting that System.Version::Parse cannot handle semver prerelease suffixes (e.g., -rc2, -dev123) and therefore the check prevents parsing such strings; update the comment near the Condition or above the _Major/_Minor/_Patch property assignments referencing CommonLibsVersion and System.Version.Parse to make the reason immediately clear to future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sharphound.csproj`:
- Around line 30-36: Add a short inline comment explaining the purpose of the
guard on the PropertyGroup Condition that uses
!$(CommonLibsVersion.Contains('-')), noting that System.Version::Parse cannot
handle semver prerelease suffixes (e.g., -rc2, -dev123) and therefore the check
prevents parsing such strings; update the comment near the Condition or above
the _Major/_Minor/_Patch property assignments referencing CommonLibsVersion and
System.Version.Parse to make the reason immediately clear to future readers.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
README.md (1)
24-26:⚠️ Potential issue | 🟡 MinorDefault-version wording is still overly absolute
At Line 24, saying default build is the “next
-devprerelease” is inaccurate whenCommonLibsVersionis already prerelease (current csproj default is4.6.0-rc2), which resolves as-is. Please align this sentence with the table/rules below.Suggested doc tweak
-By default, the project builds against the next prerelease `-dev` version of the -[SharpHoundCommon Library](https://github.com/SpecterOps/SharpHoundCommon) -(tracking the v4 branch). +By default (`CommonSource=Dev`), the project resolves +[SharpHoundCommon Library](https://github.com/SpecterOps/SharpHoundCommon) +from `CommonLibsVersion`: prerelease values (for example `-rc`) are used as-is; +stable values derive the next patch `-dev*` version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 24 - 26, The sentence claiming the default build uses "the next `-dev` prerelease" is too absolute; update the README wording to match the rules/table: state that the build will use the `CommonLibsVersion` if it is already a prerelease (e.g., 4.6.0-rc2) and otherwise will resolve to the next `-dev` prerelease that tracks the v4 branch; reference the term CommonLibsVersion and the existing table/rules to ensure the sentence describes both cases rather than always asserting a `-dev` prerelease.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 17: Update the README text that currently reads ".net sdk" to use the
canonical product casing ".NET SDK"; locate the sentence in the README
containing the phrase ".net sdk" and replace it with ".NET SDK" to ensure
consistent and professional documentation.
---
Duplicate comments:
In `@README.md`:
- Around line 24-26: The sentence claiming the default build uses "the next
`-dev` prerelease" is too absolute; update the README wording to match the
rules/table: state that the build will use the `CommonLibsVersion` if it is
already a prerelease (e.g., 4.6.0-rc2) and otherwise will resolve to the next
`-dev` prerelease that tracks the v4 branch; reference the term
CommonLibsVersion and the existing table/rules to ensure the sentence describes
both cases rather than always asserting a `-dev` prerelease.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 19-22: Add a language identifier to the fenced code block that
contains the two commands ("dotnet restore ." and "dotnet build") in README.md
to satisfy markdownlint MD040; update the opening fence from ``` to something
like ```bash (or ```powershell if preferred) so the code block explicitly
declares the language.
- Line 28: The README uses incorrect MSBuild property names (CommonLibPath and
RPCPath); the project defines _CommonLibPath and _RPCPath, so update the README
to instruct users to override the actual MSBuild properties _CommonLibPath and
_RPCPath when building against local DLLs (e.g., dotnet build
-p:_CommonLibPath=... -p:_RPCPath=... -p:CommonSource=Local) to ensure the
overrides take effect.
- Line 17: Replace the awkward sentence "To build this project, using a .NET SDK
and run the following" in the README with a clear, grammatical instruction such
as "To build this project, install a .NET SDK and run the following:" (or
alternatively "To build this project, use a .NET SDK and run the following:") so
the prerequisite and action are unambiguous.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c3757b7-1ab5-4594-8330-f4af79648bf9
📒 Files selected for processing (2)
README.mdSharphound.csproj
🚧 Files skipped from review as they are similar to previous changes (1)
- Sharphound.csproj
lrfalslev
force-pushed
the
lfalslev/bed-7878
branch
from
953bafd to
1f565d4
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
README.md (1)
17-17:⚠️ Potential issue | 🟡 MinorFix the grammatical error in the prerequisite sentence.
The phrase "using a .NET SDK run" is grammatically incorrect. It should be "use a .NET SDK and run" to properly connect the prerequisite with the action.
✏️ Suggested correction
-To build this project, using a .NET SDK run the following: +To build this project, use a .NET SDK and run the following:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 17, The sentence "To build this project, using a .NET SDK run the following:" is grammatically incorrect; update the README line to a corrected phrasing such as "To build this project, use a .NET SDK and run the following:" so the prerequisite ("use a .NET SDK") is properly connected to the action ("run the following").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@README.md`:
- Line 17: The sentence "To build this project, using a .NET SDK run the
following:" is grammatically incorrect; update the README line to a corrected
phrasing such as "To build this project, use a .NET SDK and run the following:"
so the prerequisite ("use a .NET SDK") is properly connected to the action ("run
the following").
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
Rewrite SHC -dev publishing workflow and SH/SHE csprojs to consume -dev packages, while gracefully handling -rc package versions.
Upgrade .net and testing package versions.
Update readmes.
Motivation and Context
This PR addresses: BED-7878
How Has This Been Tested?
See
Manual Testing Instructionson ticket.Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
Chores
Refactor
Documentation