chore: SHC -dev package workflows gracefully handle -rc packages BED-7878#297
chore: SHC -dev package workflows gracefully handle -rc packages BED-7878#297
Conversation
WalkthroughCI and test projects were retargeted to .NET 10.0 and CI workflows now install the 10.0 SDK. Test SDKs and tooling versions were upgraded. Several unit tests were modified (timing/sleep changes), one SmokeTest class was removed, and a small using-alias and a collection-expression change were introduced. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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
🧹 Nitpick comments (2)
.github/workflows/publish-dev-package.yml (1)
28-28: Consider keeping a single mutable-devversion if that’s still the team contract.Timestamped
-devversions create many artifacts; if downstream expects one “current state” dev package, this moves away from that model.Alternative aligned with single-current-dev strategy
- "dev_version=$base_version-dev.$date_stamp" >> $env:GITHUB_OUTPUT + "dev_version=$base_version-dev" >> $env:GITHUB_OUTPUTAnd in publish step:
- sleet push ./pkgs + sleet push --force ./pkgsBased on learnings: In the SpecterOps/SharpHoundCommon repository, the team prefers using
sleet push --forceto overwrite git-dev packages rather than creating unique versions with commit SHAs, as they want a single "current state" package for downstream consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish-dev-package.yml at line 28, The workflow currently builds a timestamped dev release via the env variable dev_version (using base_version and date_stamp); change it to emit a single mutable dev version instead (e.g., set dev_version to base_version + "-dev" without the date_stamp) and update the downstream publish step to use the overwrite/force semantics your team uses (for example, switch to sleet push --force or the equivalent CI publish flag) so the pipeline produces one current-state dev artifact rather than timestamped artifacts.test/unit/Facades/MockDirectoryObject.cs (1)
127-134: Align mock cert parsing behavior with production facade.The mock currently fails the whole call if any certificate byte array is invalid. Production (
src/CommonLib/DirectoryObjects/DirectoryEntryWrapper.cs, Lines 109-133) skips bad cert entries and returns successfully, so tests can become stricter than runtime behavior.Proposed alignment
public bool TryGetCertificateArrayProperty(string propertyName, out X509Certificate2[] value) { if (!TryGetByteArrayProperty(propertyName, out var b)) { - value = []; + value = []; return false; } - value = b.Select(X509CertificateLoader.LoadCertificate).ToArray(); + var certs = new List<X509Certificate2>(b.Length); + foreach (var certBytes in b) { + try { + certs.Add(X509CertificateLoader.LoadCertificate(certBytes)); + } + catch { + // mirror production behavior: skip invalid certs + } + } + value = certs.ToArray(); return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/Facades/MockDirectoryObject.cs` around lines 127 - 134, TryGetCertificateArrayProperty currently fails the whole call when any certificate byte array is invalid; change it to mirror production DirectoryEntryWrapper behavior by calling TryGetByteArrayProperty(propertyName, out var b) and, if true, iterate b, attempt X509CertificateLoader.LoadCertificate for each entry inside a try/catch, collect only successfully loaded X509Certificate2 instances (skip entries that throw or are invalid), set value to that collected array (empty array if none succeeded) and return true; if TryGetByteArrayProperty returns false, set value to an empty array and return false.
🤖 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-dev-package.yml:
- Around line 6-7: The workflow trigger currently only includes the feature
branch "lfalslev/bed-7878" and has commented out "v4", which prevents runs on
regular v4 pushes; update the triggers in the publish-dev-package workflow to
include the "v4" branch (restore or add "v4" alongside "lfalslev/bed-7878") so
dev package publishing runs for both the feature branch and normal v4 pushes.
---
Nitpick comments:
In @.github/workflows/publish-dev-package.yml:
- Line 28: The workflow currently builds a timestamped dev release via the env
variable dev_version (using base_version and date_stamp); change it to emit a
single mutable dev version instead (e.g., set dev_version to base_version +
"-dev" without the date_stamp) and update the downstream publish step to use the
overwrite/force semantics your team uses (for example, switch to sleet push
--force or the equivalent CI publish flag) so the pipeline produces one
current-state dev artifact rather than timestamped artifacts.
In `@test/unit/Facades/MockDirectoryObject.cs`:
- Around line 127-134: TryGetCertificateArrayProperty currently fails the whole
call when any certificate byte array is invalid; change it to mirror production
DirectoryEntryWrapper behavior by calling TryGetByteArrayProperty(propertyName,
out var b) and, if true, iterate b, attempt
X509CertificateLoader.LoadCertificate for each entry inside a try/catch, collect
only successfully loaded X509Certificate2 instances (skip entries that throw or
are invalid), set value to that collected array (empty array if none succeeded)
and return true; if TryGetByteArrayProperty returns false, set value to an empty
array and return false.
🪄 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: f25eb029-e1a8-4d5f-b115-8ba6b073e110
📒 Files selected for processing (8)
.github/workflows/build-and-test.yml.github/workflows/publish-dev-package.yml.github/workflows/publish.ymlRPCTest/RPCTest.csprojtest/unit/ACLProcessorTest.cstest/unit/CommonLibTest.csprojtest/unit/Facades/MockDirectoryObject.cstest/unit/SmokeTest.cs
💤 Files with no reviewable changes (1)
- test/unit/SmokeTest.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 `@src/CommonLib/Processors/ACLProcessor.cs`:
- Line 45: The public constructor ACLProcessor(ILdapUtils utils, string
TEST_BREAKING_CHANGE, ILogger log = null) includes an unused required parameter
TEST_BREAKING_CHANGE causing an unnecessary breaking change; remove that
parameter from the constructor signature (leave ACLProcessor(ILdapUtils utils,
ILogger log = null)), update any internal constructor body references if
present, and adjust all call sites to stop passing TEST_BREAKING_CHANGE (or
overload if backwards compatibility is required), and update any related XML
doc/comments for the constructor.
🪄 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: f20c82c2-a0b5-47ef-b5d1-70eaedeb89a6
📒 Files selected for processing (1)
src/CommonLib/Processors/ACLProcessor.cs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/publish-dev-package.yml (1)
6-7:⚠️ Potential issue | 🟠 MajorRestore
v4in the push trigger before merge.Line 6 scopes runs to a feature branch, and Line 7 comments out
v4; once merged, normalv4pushes won’t publish dev packages.Suggested fix
on: push: branches: - - "lfalslev/bed-7878" -# - "v4" + - "v4" + # - "lfalslev/bed-7878" # keep temporarily if still needed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/publish-dev-package.yml around lines 6 - 7, Restore the "v4" branch entry in the workflow push trigger so dev package publishing runs on v4 pushes again: in the push.branches list (where "lfalslev/bed-7878" was added and "v4" is commented out), uncomment or re-add the "v4" entry and keep the feature branch entry if needed so the push trigger includes both "v4" and "lfalslev/bed-7878".
🧹 Nitpick comments (2)
test/unit/AdaptiveTimeoutTest.cs (1)
82-84: Verify that testing withTask.CompletedTaskstill exercises the adaptive timeout logic meaningfully.Switching from
Task.Delay(40/50/60)toTask.CompletedTaskeliminates timing variability but also means observed latencies will be near-zero. The test still assertsadaptiveTimeoutResult < maxTimeoutat line 87, which should pass since the adaptive calculation should produce a small timeout for fast-completing tasks.If the intent is purely to stabilize the test, this is acceptable. However, consider whether a separate integration-style test with real delays (perhaps in a non-CI context) would be valuable to ensure the adaptive logic works correctly with actual latency variance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/AdaptiveTimeoutTest.cs` around lines 82 - 84, The test in AdaptiveTimeoutTest now calls adaptiveTimeout.ExecuteWithTimeout with Task.CompletedTask for LatencyObservation1/2/3 which yields near-zero observed latencies and may not meaningfully exercise the adaptive timeout algorithm; update the unit test to either (a) restore small deterministic delays (e.g. Task.Delay(40/50/60) or similar) when calling adaptiveTimeout.ExecuteWithTimeout so LatencyObservation1/2/3 produce non-zero latency samples, or (b) keep Task.CompletedTask but add a separate integration-style test that uses real Task.Delay to validate adaptive behavior; ensure the assertions around adaptiveTimeoutResult < maxTimeout still run against the variant that provides realistic latency samples and reference the AdaptiveTimeoutTest class and adaptiveTimeout.ExecuteWithTimeout calls for changes.test/unit/TimeoutTests.cs (1)
153-165: Align async and sync timeout test values for consistency.The async
ExecuteWithTimeout_Task_Timeout_Cancel(lines 154-165) andExecuteWithTimeout_Task_T_Timeout_Cancel(lines 214-226) tests use 500ms/600ms timeouts, while the sync equivalents use 1000ms/1200ms. Since both test the same cancellation behavior, consider aligning these values across all variants for consistency and to avoid future confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/TimeoutTests.cs` around lines 153 - 165, The async test ExecuteWithTimeout_Task_Timeout_Cancel uses 100ms/500ms/600ms split that mismatches the sync tests; update the timeout and delays in ExecuteWithTimeout_Task_Timeout_Cancel (the timeout variable and Task.Delay calls inside func and after ExecuteWithTimeout) to match the sync equivalents (use 1000ms for the timeout, 1000ms/1200ms delays as in the sync variants) so all ExecuteWithTimeout_*_Timeout_Cancel tests use consistent timing and verify the same cancellation behavior.
🤖 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-dev-package.yml:
- Line 13: Remove the invalid dependency "needs: build-and-test" from the
workflow because no job named build-and-test exists in this file; locate the
jobs block where the line "needs: build-and-test" appears and either delete that
needs entry or replace it with the correct existing job name(s) in this workflow
so the job dependency references only jobs defined in the same jobs: block.
---
Duplicate comments:
In @.github/workflows/publish-dev-package.yml:
- Around line 6-7: Restore the "v4" branch entry in the workflow push trigger so
dev package publishing runs on v4 pushes again: in the push.branches list (where
"lfalslev/bed-7878" was added and "v4" is commented out), uncomment or re-add
the "v4" entry and keep the feature branch entry if needed so the push trigger
includes both "v4" and "lfalslev/bed-7878".
---
Nitpick comments:
In `@test/unit/AdaptiveTimeoutTest.cs`:
- Around line 82-84: The test in AdaptiveTimeoutTest now calls
adaptiveTimeout.ExecuteWithTimeout with Task.CompletedTask for
LatencyObservation1/2/3 which yields near-zero observed latencies and may not
meaningfully exercise the adaptive timeout algorithm; update the unit test to
either (a) restore small deterministic delays (e.g. Task.Delay(40/50/60) or
similar) when calling adaptiveTimeout.ExecuteWithTimeout so
LatencyObservation1/2/3 produce non-zero latency samples, or (b) keep
Task.CompletedTask but add a separate integration-style test that uses real
Task.Delay to validate adaptive behavior; ensure the assertions around
adaptiveTimeoutResult < maxTimeout still run against the variant that provides
realistic latency samples and reference the AdaptiveTimeoutTest class and
adaptiveTimeout.ExecuteWithTimeout calls for changes.
In `@test/unit/TimeoutTests.cs`:
- Around line 153-165: The async test ExecuteWithTimeout_Task_Timeout_Cancel
uses 100ms/500ms/600ms split that mismatches the sync tests; update the timeout
and delays in ExecuteWithTimeout_Task_Timeout_Cancel (the timeout variable and
Task.Delay calls inside func and after ExecuteWithTimeout) to match the sync
equivalents (use 1000ms for the timeout, 1000ms/1200ms delays as in the sync
variants) so all ExecuteWithTimeout_*_Timeout_Cancel tests use consistent timing
and verify the same cancellation behavior.
🪄 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: 20053fdf-69bf-4f4a-ba21-1327284d9b80
📒 Files selected for processing (4)
.github/workflows/publish-dev-package.ymltest/unit/AdaptiveTimeoutTest.cstest/unit/CommonLibTest.csprojtest/unit/TimeoutTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/CommonLibTest.csproj
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/CommonLib/Processors/ACLProcessor.cs (1)
45-49:⚠️ Potential issue | 🔴 CriticalRevert the unused required constructor parameter to avoid a breaking compile change.
Line 45 introduces
BREAKING_COMMON_CHANGE_TESTas a required argument, but it is unused in Lines 47-49. This creates a no-op API break and currently invalidates existing constructor calls (e.g.,test/unit/ACLProcessorTest.csLine 47 and Line 78; snippet metadata indicates many more). Please remove it (or make it optional with a default) to preserve compatibility.💡 Proposed fix
- public ACLProcessor(ILdapUtils utils, string BREAKING_COMMON_CHANGE_TEST, ILogger log = null) + public ACLProcessor(ILdapUtils utils, ILogger log = null) { _utils = utils; _log = log ?? Logging.LogProvider.CreateLogger("ACLProc"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/Processors/ACLProcessor.cs` around lines 45 - 49, The ACLProcessor constructor signature was changed to require a new unused parameter BREAKING_COMMON_CHANGE_TEST which breaks existing callers; restore compatibility by removing the BREAKING_COMMON_CHANGE_TEST parameter from the ACLProcessor constructor (or make it optional with a default value) so existing instantiations compile, keeping the body that assigns _utils and _log unchanged (see the ACLProcessor constructor and fields _utils, _log).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/CommonLib/Processors/ACLProcessor.cs`:
- Around line 45-49: The ACLProcessor constructor signature was changed to
require a new unused parameter BREAKING_COMMON_CHANGE_TEST which breaks existing
callers; restore compatibility by removing the BREAKING_COMMON_CHANGE_TEST
parameter from the ACLProcessor constructor (or make it optional with a default
value) so existing instantiations compile, keeping the body that assigns _utils
and _log unchanged (see the ACLProcessor constructor and fields _utils, _log).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f395392e-408e-4487-b278-4b36409ff30d
📒 Files selected for processing (2)
.github/workflows/publish-dev-package.ymlsrc/CommonLib/Processors/ACLProcessor.cs
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/publish-dev-package.yml
mykeelium
left a comment
mykeelium
left a comment
There was a problem hiding this comment.
I am cool with this moving forward, I think we should at least decide on the notes, but they shouldn't be blocking to this. Let me know if/when I will mark approved, I got burned with auto merge.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/unit/AdaptiveTimeoutTest.cs (1)
82-90:⚠️ Potential issue | 🟡 Minor
adaptiveTimeoutResult < maxTimeoutcan still be flaky under runner stress.Line 87 still asserts strict less-than against
maxTimeout(1s). With 40/50/60msThread.Sleepsamples this should normally hold with large margin, but this is the same assertion flagged previously as fragile on stressed runners. If flakiness resurfaces, consider asserting against a more forgiving upper bound (e.g.,maxTimeout - TimeSpan.FromMilliseconds(100)) or require the result to be within a known reduced band rather than just< maxTimeout. Not blocking — just flagging since the related range widening was motivated by the same runner-stress issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/AdaptiveTimeoutTest.cs` around lines 82 - 90, The assertion adaptiveTimeoutResult < maxTimeout is fragile under runner load; update the test to assert a slightly looser upper bound (e.g., require adaptiveTimeoutResult < maxTimeout - TimeSpan.FromMilliseconds(100) or assert adaptiveTimeoutResult is within a reduced band) by changing the check after calling adaptiveTimeout.GetAdaptiveTimeout() (and keep the existing ExecuteWithTimeout/LatencyObservation calls unchanged) so the test no longer relies on a strict < maxTimeout threshold.
🧹 Nitpick comments (1)
test/unit/AdaptiveTimeoutTest.cs (1)
28-28: Consider keeping the delegate awaitable to exercise the async path.Switching the delegate to a synchronous
Thread.Sleep(50)blocks the calling thread and routes through theFunc<CancellationToken, T>/Actionoverload ofExecuteWithTimeoutrather than theFunc<CancellationToken, Task>overload. That's fine for stabilizing latency measurements, but it means these tests (NotEnoughSamplesAsync,AdaptiveDisabled,GetAdaptiveTimeout) no longer cover the async overload path at all — onlyTimeSpikeSafetyValve*and*ExcessiveTimeoutsstill exerciseTask.Delay. Worth confirming this is intentional; if you want to preserve async coverage, one option is to keep one of the three variants onTask.Delaywhile leaving the others synchronous for timing stability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/AdaptiveTimeoutTest.cs` at line 28, The test uses a synchronous Thread.Sleep(50) so ExecuteWithTimeout resolves via the Func<CancellationToken, T>/Action overload and no longer exercises the async overload; update the test(s) in AdaptiveTimeoutTest (e.g. NotEnoughSamplesAsync, AdaptiveDisabled, GetAdaptiveTimeout) to keep at least one variant passing an awaitable delegate (Func<CancellationToken, Task>) that uses Task.Delay(50) so the async path of ExecuteWithTimeout is covered while optionally leaving the others synchronous for timing stability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/unit/AdaptiveTimeoutTest.cs`:
- Around line 82-90: The assertion adaptiveTimeoutResult < maxTimeout is fragile
under runner load; update the test to assert a slightly looser upper bound
(e.g., require adaptiveTimeoutResult < maxTimeout -
TimeSpan.FromMilliseconds(100) or assert adaptiveTimeoutResult is within a
reduced band) by changing the check after calling
adaptiveTimeout.GetAdaptiveTimeout() (and keep the existing
ExecuteWithTimeout/LatencyObservation calls unchanged) so the test no longer
relies on a strict < maxTimeout threshold.
---
Nitpick comments:
In `@test/unit/AdaptiveTimeoutTest.cs`:
- Line 28: The test uses a synchronous Thread.Sleep(50) so ExecuteWithTimeout
resolves via the Func<CancellationToken, T>/Action overload and no longer
exercises the async overload; update the test(s) in AdaptiveTimeoutTest (e.g.
NotEnoughSamplesAsync, AdaptiveDisabled, GetAdaptiveTimeout) to keep at least
one variant passing an awaitable delegate (Func<CancellationToken, Task>) that
uses Task.Delay(50) so the async path of ExecuteWithTimeout is covered while
optionally leaving the others synchronous for timing stability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fda619b-419c-4d3a-8417-075c8dd028ed
📒 Files selected for processing (2)
test/unit/AdaptiveTimeoutTest.cstest/unit/Facades/MockDirectoryObject.cs
✅ Files skipped from review due to trivial changes (1)
- test/unit/Facades/MockDirectoryObject.cs
2 participants
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
Tests