Add multi-forest AD targeting and merged reporting - #2072
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds multi-forest Active Directory targeting. It routes AD queries through selected servers, runs tests per target, records AD context, creates target-specific reports, and merges results. ChangesMulti-forest Active Directory execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant Invoke-Maester
participant Connect-Maester
participant ActiveDirectory
participant ReportWriter
Operator->>Invoke-Maester: start AD tests with target servers
Invoke-Maester->>Connect-Maester: connect to current target
Connect-Maester->>ActiveDirectory: probe RootDSE
ActiveDirectory-->>Connect-Maester: return AD metadata
Connect-Maester-->>Invoke-Maester: return connection state
Invoke-Maester->>ActiveDirectory: execute AD tests
Invoke-Maester->>ReportWriter: write per-target and merged reports
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
powershell/tests/functions/ActiveDirectoryOptIn.Tests.ps1 (1)
107-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the session connection state is restored after the multi-target run.
The test covers per-target execution and the output files. It does not check that
Invoke-Maesterrestores$__MtSession.ADConnection.TargetServerand.TargetServersafter the loop. That restore is the state contract for the caller session, and it is currently not protected against a mid-loop failure. See the related comment onpowershell/public/Invoke-Maester.ps1lines 741 to 809.Add assertions inside
InModuleScope Maesterafter the run.💚 Proposed addition
Test-Path (Join-Path $outputFolder 'ADMultiForest.html') | Should -BeTrue (Get-ChildItem -Path $outputFolder -Filter 'ADMultiForest-dc01.contoso.com*.json').Count | Should -BeGreaterThan 0 (Get-ChildItem -Path $outputFolder -Filter 'ADMultiForest-dc01.fabrikam.net*.json').Count | Should -BeGreaterThan 0 + + InModuleScope Maester { + $__MtSession.ADConnection.TargetServer | Should -Be 'dc01.contoso.com' + @($__MtSession.ADConnection.TargetServers).Count | Should -Be 2 + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@powershell/tests/functions/ActiveDirectoryOptIn.Tests.ps1` around lines 107 - 129, Extend the multi-target test in `Runs AD tests once per connected forest target when multiple targets are configured` to assert after `Invoke-Maester` completes that `$__MtSession.ADConnection.TargetServer` and `.TargetServers` match their original values. Place these assertions inside `InModuleScope Maester` after the run while preserving the existing result and output-file checks.powershell/tests/functions/Connect-Maester.Tests.ps1 (1)
82-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReset
$__MtSession.ADConnectionafter these tests.Both tests write a connected Active Directory state into the module session. No
AfterEachclears it. The state persists for the rest of the Pester session, and other test files that import the same module instance can then observe an unexpected connected AD session.ActiveDirectoryOptIn.Tests.ps1clears this state in its ownAfterEach; apply the same pattern here.♻️ Proposed addition
+ AfterEach { + InModuleScope Maester { + $__MtSession.ADConnection = $null + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@powershell/tests/functions/Connect-Maester.Tests.ps1` around lines 82 - 128, Add an AfterEach cleanup for the Active Directory connection state created by the tests around Connect-Maester, resetting $__MtSession.ADConnection in the Maester module scope after each test. Follow the existing cleanup pattern used by ActiveDirectoryOptIn.Tests.ps1 and ensure both TargetServer tests leave no connected session state behind.powershell/public/Invoke-Maester.ps1 (1)
420-434: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op
EndOfJsonassignment and rename the merge-source variable.Line 420 reassigns
$mergedResult, which discards the object created at line 369. Only$mergedTenantssurvives. The reuse of one variable name for two different objects makes the merge flow hard to follow.Line 434 assigns
$mergedResult.EndOfJsonback onto$mergedResult. This is a no-op becauseSelect-Object *already copied the property.♻️ Proposed refactor
- $mergedResult = Merge-MtMaesterResult -MaesterResults $ForestResults - $mergedTenants = $mergedResult.Tenants + $tenantMergeResult = Merge-MtMaesterResult -MaesterResults $ForestResults + $mergedTenants = $tenantMergeResult.Tenants$mergedResult | Add-Member -NotePropertyName 'InvokeCommand' -NotePropertyValue $InvokeCommand -Force - $mergedResult | Add-Member -NotePropertyName 'EndOfJson' -NotePropertyValue $mergedResult.EndOfJson -Force🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@powershell/public/Invoke-Maester.ps1` around lines 420 - 434, In the merge flow around `$firstResult`, rename the source-copy variable currently assigned with `Select-Object *` to a distinct name so it is not confused with the earlier `$mergedResult` object; update all subsequent property additions and the emitted result reference accordingly. Remove the final `Add-Member` statement that assigns `$mergedResult.EndOfJson` back to the same property.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build/activeDirectory/Run-ADTests-And-CopyReports.ps1`:
- Around line 213-221: Update the invocation parameters used by Invoke-Maester
so the AD test execution includes every path in $validTestPaths rather than only
index 0, preserving the existing behavior and output for all discovered
locations.
In `@powershell/public/ad/domain/Test-MtAdTombstoneLifetime.ps1`:
- Line 1: Preserve UTF-8 with BOM encoding for the PowerShell scripts containing
emoji literals, retaining PowerShell 5.1 and Desktop compatibility. Apply this
encoding consistently to
powershell/public/ad/domain/Test-MtAdTombstoneLifetime.ps1 (lines 1-1),
powershell/public/ad/gpo/Test-MtAdGpoBlockedInheritanceCount.ps1 (lines 1-1),
powershell/public/ad/passwordpolicy/Test-MtAdAccountLockoutDuration.ps1 (lines
1-1), powershell/public/ad/passwordpolicy/Test-MtAdAccountLockoutThreshold.ps1
(lines 1-1),
powershell/public/ad/passwordpolicy/Test-MtAdFineGrainedPolicyAppliesTo.ps1
(lines 1-1),
powershell/public/ad/passwordpolicy/Test-MtAdFineGrainedPolicyCount.ps1 (lines
1-1), powershell/public/ad/passwordpolicy/Test-MtAdPasswordMinLength.ps1 (lines
1-1), and
powershell/public/ad/passwordpolicy/Test-MtAdPasswordReversibleEncryption.ps1
(lines 1-1); no script logic changes are needed.
In `@powershell/public/ad/group/Test-MtAdGroupEmptyNonPrivilegedCount.ps1`:
- Line 41: Propagate AD lookup failures instead of treating missing results as
valid empty data: update the membership lookups in
Test-MtAdGroupEmptyNonPrivilegedCount.ps1,
Test-MtAdGroupEmptyNonPrivilegedDetails.ps1,
Test-MtAdGroupMemberAccountTypeCount.ps1,
Test-MtAdGroupMemberAccountTypeDetails.ps1,
Test-MtAdGroupMemberDistinctGroupCount.ps1,
Test-MtAdGroupMemberForeignSidCount.ps1,
Test-MtAdGroupMemberForeignSidDetails.ps1, Test-MtAdGroupMemberTrustCount.ps1,
Test-MtAdGroupMemberTrustDetails.ps1, and
Test-MtAdGroupPrivilegedWithMembersCount.ps1 at the specified ranges to use
terminating errors, and update the OU/domain GPO and AD lookups in
Test-MtAdGpoUnlinkedTargetCount.ps1 at both specified ranges similarly. Ensure
failures reach the existing error handling or return an explicit partial/failed
state rather than reporting empty membership or complete GPO coverage.
In `@powershell/public/Invoke-Maester.ps1`:
- Around line 398-433: Update the merged-result construction around
$mergedResult and the $ForestResults aggregation to sum TotalDuration,
UserDuration, DiscoveryDuration, and FrameworkDuration across all forest
results, and set ExecutedAt to the earliest value. Clear or replace
ActiveDirectoryContext so the merged report does not retain one forest’s
context, while preserving the existing counter aggregation and report structure.
- Around line 796-805: Wrap the per-target Invoke-Maester call in the
multi-forest loop with error handling so failures from a target are caught
rather than terminating the run. Record each caught error in the existing result
or reporting flow, then continue iterating through subsequent targets and still
produce the merged report; preserve the current success handling that adds
ADTargetServer, ADForestName, and ADForestRootDomain in the target invocation
block.
- Line 734: Update the Write-MtProgress call’s -Status expression to use
PowerShell subexpression syntax around the full
$__MtSession.ADRunContext.ForestName property path, matching the correct
interpolation pattern on the preceding line.
- Around line 741-809: Wrap the multi-target AD processing loop beginning at
`foreach ($adTarget in $adTargets)` in a `try` block, and move restoration of
`$__MtSession.ADConnection.TargetServer`,
`$__MtSession.ADConnection.TargetServers`, and `$__MtSession.ADRunContext` into
a corresponding `finally` block. Preserve the existing per-target execution and
result aggregation while ensuring restoration runs when `Get-MtAdRunContext`,
`Clear-MtADCache`, or the nested `Invoke-Maester` call throws.
In `@powershell/tests/functions/Get-MtHtmlReport.Tests.ps1`:
- Around line 9-32: Add forest, domain, and target-server AD context to the
fixtures and assertions covering Get-MtHtmlReport, Import-MtMaesterResult, and
Merge-MtMaesterResult. In powershell/tests/functions/Get-MtHtmlReport.Tests.ps1
ranges 9-32 and 52-101, verify single and distinct merged contexts appear in
HTML; in powershell/tests/functions/Import-MtMaesterResult.Tests.ps1 ranges
20-21, 44-45, and 58-59, verify serialization, import, and merged-file expansion
preserve each context; in
powershell/tests/functions/Merge-MtMaesterResult.Tests.ps1 ranges 11-12, 31-32,
85-86, 118-119, and 144-145, cover distinct per-target contexts, three-or-more
results, and empty optional tenant metadata.
---
Nitpick comments:
In `@powershell/public/Invoke-Maester.ps1`:
- Around line 420-434: In the merge flow around `$firstResult`, rename the
source-copy variable currently assigned with `Select-Object *` to a distinct
name so it is not confused with the earlier `$mergedResult` object; update all
subsequent property additions and the emitted result reference accordingly.
Remove the final `Add-Member` statement that assigns `$mergedResult.EndOfJson`
back to the same property.
In `@powershell/tests/functions/ActiveDirectoryOptIn.Tests.ps1`:
- Around line 107-129: Extend the multi-target test in `Runs AD tests once per
connected forest target when multiple targets are configured` to assert after
`Invoke-Maester` completes that `$__MtSession.ADConnection.TargetServer` and
`.TargetServers` match their original values. Place these assertions inside
`InModuleScope Maester` after the run while preserving the existing result and
output-file checks.
In `@powershell/tests/functions/Connect-Maester.Tests.ps1`:
- Around line 82-128: Add an AfterEach cleanup for the Active Directory
connection state created by the tests around Connect-Maester, resetting
$__MtSession.ADConnection in the Maester module scope after each test. Follow
the existing cleanup pattern used by ActiveDirectoryOptIn.Tests.ps1 and ensure
both TargetServer tests leave no connected session state behind.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 319c7d63-2064-430f-83f5-a937e91d64d2
📒 Files selected for processing (47)
build/activeDirectory/README-ADTestRunner.mdbuild/activeDirectory/Run-ADTests-And-CopyReports.ps1build/activeDirectory/Use-LocalMaesterModule.ps1powershell/Maester.psd1powershell/Maester.psm1powershell/internal/Clear-ModuleVariable.ps1powershell/internal/ConvertTo-MtMaesterResult.ps1powershell/internal/Reset-MtProgressView.ps1powershell/internal/Set-MtProgressView.ps1powershell/public/Connect-Maester.ps1powershell/public/Get-MtADDomainState.ps1powershell/public/Get-MtADServerParameters.ps1powershell/public/Invoke-Maester.ps1powershell/public/ad/domain/Test-MtAdMachineAccountQuota.ps1powershell/public/ad/domain/Test-MtAdRidsRemaining.ps1powershell/public/ad/domain/Test-MtAdTombstoneLifetime.ps1powershell/public/ad/gpo/Test-MtAdGpoBlockedInheritanceCount.ps1powershell/public/ad/gpo/Test-MtAdGpoLinkedOUCount.ps1powershell/public/ad/gpo/Test-MtAdGpoUnlinkedTargetCount.ps1powershell/public/ad/group/Test-MtAdGroupEmptyNonPrivilegedCount.ps1powershell/public/ad/group/Test-MtAdGroupEmptyNonPrivilegedDetails.ps1powershell/public/ad/group/Test-MtAdGroupMemberAccountTypeCount.ps1powershell/public/ad/group/Test-MtAdGroupMemberAccountTypeDetails.ps1powershell/public/ad/group/Test-MtAdGroupMemberDistinctGroupCount.ps1powershell/public/ad/group/Test-MtAdGroupMemberForeignSidCount.ps1powershell/public/ad/group/Test-MtAdGroupMemberForeignSidDetails.ps1powershell/public/ad/group/Test-MtAdGroupMemberTrustCount.ps1powershell/public/ad/group/Test-MtAdGroupMemberTrustDetails.ps1powershell/public/ad/group/Test-MtAdGroupPrivilegedWithMembersCount.ps1powershell/public/ad/group/Test-MtAdGroupPrivilegedWithMembersDetails.ps1powershell/public/ad/passwordpolicy/Test-MtAdAccountLockoutDuration.ps1powershell/public/ad/passwordpolicy/Test-MtAdAccountLockoutThreshold.ps1powershell/public/ad/passwordpolicy/Test-MtAdFineGrainedPolicyAppliesTo.ps1powershell/public/ad/passwordpolicy/Test-MtAdFineGrainedPolicyCount.ps1powershell/public/ad/passwordpolicy/Test-MtAdFineGrainedPolicySettingCounts.ps1powershell/public/ad/passwordpolicy/Test-MtAdFineGrainedPolicyValueCount.ps1powershell/public/ad/passwordpolicy/Test-MtAdPasswordComplexityRequired.ps1powershell/public/ad/passwordpolicy/Test-MtAdPasswordHistoryCount.ps1powershell/public/ad/passwordpolicy/Test-MtAdPasswordMaxAge.ps1powershell/public/ad/passwordpolicy/Test-MtAdPasswordMinLength.ps1powershell/public/ad/passwordpolicy/Test-MtAdPasswordReversibleEncryption.ps1powershell/tests/functions/ActiveDirectoryOptIn.Tests.ps1powershell/tests/functions/Connect-Maester.Tests.ps1powershell/tests/functions/Get-MtHtmlReport.Tests.ps1powershell/tests/functions/Import-MtMaesterResult.Tests.ps1powershell/tests/functions/Merge-MtMaesterResult.Tests.ps1tests/maester-config.json
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
powershell/public/Invoke-Maester.ps1 (1)
516-528: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemove
OutputFilesfrom a copy, not from cached tenant results.
Merge-MtMaesterResultassignsTenants = @($collectedResults)without copying each input result. When report output is written,Write-MtMaesterOutputsusesSelect-Object *only for the top result, then removesOutputFilesfrom the nested tenantPSObjectinstances. In the multi-forest PassThru path, these tenant objects are the same elements later returned to the caller as$multiForestResults, so the returned results loseOutputFilesafter report generation. Clone or deep-copy each tenant result before removingOutputFiles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@powershell/public/Invoke-Maester.ps1` around lines 516 - 528, Update the tenant sanitization in Write-MtMaesterOutputs to clone or deep-copy each tenant result before removing its OutputFiles property, rather than mutating the objects held by Tenants and later returned as $multiForestResults. Keep the top-level copy behavior and output serialization unchanged.
🧹 Nitpick comments (5)
powershell/public/Invoke-Maester.ps1 (4)
413-415: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the discarded first
$mergedResultassignment.Line 413 assigns the output of
Merge-MtMaesterResultto$mergedResult. Line 479 overwrites the variable before any other use. Only$mergedTenantsfrom line 414 is used. Assign the merge output to a distinct variable to make the data flow clear.♻️ Proposed change
- $mergedResult = Merge-MtMaesterResult -MaesterResults $ForestResults - $mergedTenants = $mergedResult.Tenants + $mergedTenants = (Merge-MtMaesterResult -MaesterResults $ForestResults).Tenants🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@powershell/public/Invoke-Maester.ps1` around lines 413 - 415, Update the result-handling block around Merge-MtMaesterResult so the merge output is assigned to a distinct variable rather than $mergedResult, which is later overwritten. Preserve assigning $mergedTenants from that merge result and leave the subsequent first-result handling unchanged.
501-501: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the self-assigning
EndOfJsonmember.Line 501 reads
$mergedResult.EndOfJsonand writes the same value back to the same object. The value already comes from$firstResultthrough theSelect-Object *copy at line 479. If$firstResulthas noEndOfJsonproperty, this line sets the property to$null, which does not add the marker either. Delete the line, or set the literal marker value.♻️ Proposed change
$mergedResult | Add-Member -NotePropertyName 'InvokeCommand' -NotePropertyValue $InvokeCommand -Force - $mergedResult | Add-Member -NotePropertyName 'EndOfJson' -NotePropertyValue $mergedResult.EndOfJson -Force + $mergedResult | Add-Member -NotePropertyName 'EndOfJson' -NotePropertyValue 'EndOfJson' -Force🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@powershell/public/Invoke-Maester.ps1` at line 501, Remove the self-assigning EndOfJson Add-Member statement from the merged result construction; rely on the existing Select-Object * copy from $firstResult, or assign the intended literal marker value if the property must always be present.
340-346: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEscape single quotes in reconstructed values.
Lines 341 and 343 wrap values in single quotes without escaping. A value that contains
'produces a command string that cannot be pasted back into a shell. Double the quote character when you build the string.♻️ Proposed change
} elseif ($paramValue -is [array]) { - $invokeCommand += " -$paramName @('$($paramValue -join "', '")')" + $escapedValues = @($paramValue | ForEach-Object { "$_".Replace("'", "''") }) + $invokeCommand += " -$paramName @('$($escapedValues -join "', '")')" } elseif ($paramValue -is [string]) { - $invokeCommand += " -$paramName '$paramValue'" + $invokeCommand += " -$paramName '$($paramValue.Replace("'", "''"))'"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@powershell/public/Invoke-Maester.ps1` around lines 340 - 346, Update the array and string branches in the command reconstruction logic to escape embedded single quotes by doubling them before wrapping values in single quotes. Apply this to the values handled in the $paramValue array and string checks, while preserving existing formatting for values without quotes and other parameter types.
953-970: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
-Commentparameter of the helper.
Get-MtInvokeMaesterCommandaccepts-Commentand appends the" # "prefix at line 350. Lines 967 to 969 repeat that concatenation. Build$adContextPartsfirst, then call the helper once with-Comment.♻️ Proposed change
- $invokeMaesterCommand = Get-MtInvokeMaesterCommand -BoundParameters $PSBoundParameters - + $adContextParts = @() if ($__MtSession.ADRunContext) { - $adContextParts = @() if (-not [string]::IsNullOrWhiteSpace($__MtSession.ADRunContext.ForestName)) { @@ - - if ($adContextParts.Count -gt 0) { - $invokeMaesterCommand += " # " + ($adContextParts -join '; ') - } } + + $invokeMaesterCommand = Get-MtInvokeMaesterCommand -BoundParameters $PSBoundParameters -Comment ($adContextParts -join '; ')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@powershell/public/Invoke-Maester.ps1` around lines 953 - 970, The AD context comment is appended manually after Get-MtInvokeMaesterCommand, duplicating the helper’s comment formatting. In the Invoke-Maester flow, build the $adContextParts content first, then pass the joined value through Get-MtInvokeMaesterCommand’s -Comment parameter and remove the direct " # " concatenation, while preserving the existing behavior when no AD context parts exist.powershell/tests/functions/Get-MtHtmlReport.Tests.ps1 (1)
147-153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the root-domain assertion distinct.
Line 152 checks
*contoso.com*. Lines 150 and 151 already guarantee that substring, becauseforest-single.contoso.comanddc01.contoso.comboth contain it. The assertion cannot fail on its own, soForestRootDomainis not covered. Use a distinct root-domain value in the fixture, or assert the JSON property name together with the value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@powershell/tests/functions/Get-MtHtmlReport.Tests.ps1` around lines 147 - 153, Update the AD context metadata test around “Should contain AD context metadata in the output” so the ForestRootDomain assertion cannot pass solely because forest-single.contoso.com or dc01.contoso.com contains contoso.com. Use a distinct root-domain fixture value or assert the ForestRootDomain property name together with its value, while preserving the existing forest and domain-controller checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@powershell/public/Invoke-Maester.ps1`:
- Around line 846-861: Update the target output-path construction in the
recursive target handling to always assign targetParams['OutputFolderFileName']
using baseFileName and targetSuffix, including when no output parameters are
provided. Remove the conditional behavior that routes the default configuration
through ValidateAndSetOutputFiles, while preserving explicit output-file suffix
handling where applicable.
- Around line 895-899: Update the merged-report flow around
New-MtMergedAdForestResult to avoid calling it when multiForestResults is empty,
while preserving the existing merge and ADTargetFailures behavior when results
exist. Use a collection-count guard before invoking the function, or adjust its
parameter contract and handle the all-failure path there.
- Around line 383-411: Update ConvertTo-MtTimeSpan and ConvertTo-MtDateTime to
use the TryParse overloads that accept
[System.Globalization.CultureInfo]::InvariantCulture, preserving their existing
fallback values of [TimeSpan]::Zero and $null when parsing fails.
---
Outside diff comments:
In `@powershell/public/Invoke-Maester.ps1`:
- Around line 516-528: Update the tenant sanitization in Write-MtMaesterOutputs
to clone or deep-copy each tenant result before removing its OutputFiles
property, rather than mutating the objects held by Tenants and later returned as
$multiForestResults. Keep the top-level copy behavior and output serialization
unchanged.
---
Nitpick comments:
In `@powershell/public/Invoke-Maester.ps1`:
- Around line 413-415: Update the result-handling block around
Merge-MtMaesterResult so the merge output is assigned to a distinct variable
rather than $mergedResult, which is later overwritten. Preserve assigning
$mergedTenants from that merge result and leave the subsequent first-result
handling unchanged.
- Line 501: Remove the self-assigning EndOfJson Add-Member statement from the
merged result construction; rely on the existing Select-Object * copy from
$firstResult, or assign the intended literal marker value if the property must
always be present.
- Around line 340-346: Update the array and string branches in the command
reconstruction logic to escape embedded single quotes by doubling them before
wrapping values in single quotes. Apply this to the values handled in the
$paramValue array and string checks, while preserving existing formatting for
values without quotes and other parameter types.
- Around line 953-970: The AD context comment is appended manually after
Get-MtInvokeMaesterCommand, duplicating the helper’s comment formatting. In the
Invoke-Maester flow, build the $adContextParts content first, then pass the
joined value through Get-MtInvokeMaesterCommand’s -Comment parameter and remove
the direct " # " concatenation, while preserving the existing behavior when no
AD context parts exist.
In `@powershell/tests/functions/Get-MtHtmlReport.Tests.ps1`:
- Around line 147-153: Update the AD context metadata test around “Should
contain AD context metadata in the output” so the ForestRootDomain assertion
cannot pass solely because forest-single.contoso.com or dc01.contoso.com
contains contoso.com. Use a distinct root-domain fixture value or assert the
ForestRootDomain property name together with its value, while preserving the
existing forest and domain-controller checks.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7c9e3f71-8555-46a9-9e2a-d28a2e82bdf9
📒 Files selected for processing (5)
build/activeDirectory/Run-ADTests-And-CopyReports.ps1powershell/public/Invoke-Maester.ps1powershell/tests/functions/Get-MtHtmlReport.Tests.ps1powershell/tests/functions/Import-MtMaesterResult.Tests.ps1powershell/tests/functions/Merge-MtMaesterResult.Tests.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- build/activeDirectory/Run-ADTests-And-CopyReports.ps1
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
powershell/public/Invoke-Maester.ps1 (2)
323-345: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact secret-bearing parameters from
InvokeCommand.
TeamChannelWebhookUriis added to the reconstructed command as plaintext. Line 500 stores that command on the merged result, and Line 533 serializes it to JSON. This can disclose the webhook URL and its query credentials to report readers.Redact
TeamChannelWebhookUribefore appending it to the command.Proposed fix
$invokeCommand = 'Invoke-Maester' + $redactedParameters = @('TeamChannelWebhookUri') foreach ($param in $BoundParameters.GetEnumerator()) { $paramName = $param.Key $paramValue = $param.Value - if ($paramValue -is [switch]) { + if ($paramName -in $redactedParameters) { + $invokeCommand += " -$paramName '<redacted>'" + } elseif ($paramValue -is [switch]) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@powershell/public/Invoke-Maester.ps1` around lines 323 - 345, The Get-MtInvokeMaesterCommand function currently appends TeamChannelWebhookUri as plaintext; redact this parameter before adding it to $invokeCommand. Preserve the existing command reconstruction for all other parameters while ensuring the stored and serialized InvokeCommand cannot expose the webhook URL or query credentials.
841-871: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSend notifications only for the merged result.
When notification parameters are supplied, each recursive target call receives them in
$targetParams. Each child then sends email or Teams output at Lines 572-585. The parent sends the merged result again after the loop. A multi-forest run therefore sends one notification per target plus one merged notification.Remove notification parameters from
$targetParams. Send the merged notification once from the parent call.Proposed fix
$targetParams['PassThru'] = $true + $targetParams.Remove('MailRecipient') + $targetParams.Remove('TeamId') + $targetParams.Remove('TeamChannelId') + $targetParams.Remove('TeamChannelWebhookUri') try {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@powershell/public/Invoke-Maester.ps1` around lines 841 - 871, Update the recursive target-parameter construction around $targetParams so notification parameters are excluded before invoking Invoke-Maester for each target. Preserve all other target parameters and keep notification handling in the parent flow, sending only the merged result once after the target loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@powershell/public/Invoke-Maester.ps1`:
- Around line 323-345: The Get-MtInvokeMaesterCommand function currently appends
TeamChannelWebhookUri as plaintext; redact this parameter before adding it to
$invokeCommand. Preserve the existing command reconstruction for all other
parameters while ensuring the stored and serialized InvokeCommand cannot expose
the webhook URL or query credentials.
- Around line 841-871: Update the recursive target-parameter construction around
$targetParams so notification parameters are excluded before invoking
Invoke-Maester for each target. Preserve all other target parameters and keep
notification handling in the parent flow, sending only the merged result once
after the target loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6579d0fb-6c48-4645-920a-b186c6d6ad35
📒 Files selected for processing (1)
powershell/public/Invoke-Maester.ps1
|
PR updated with follow-up security/notification hardening in commit 7d0703d:\n\n- Redacted TeamChannelWebhookUri in InvokeCommand reconstruction so webhook/query credentials are not persisted.\n- Removed notification parameters from recursive per-target Invoke-Maester calls to keep notifications parent-only for merged multi-forest runs.\n\nValidation: Invoke-Pester on powershell/tests/functions/Invoke-Maester.Tests.ps1 and powershell/tests/functions/ActiveDirectoryOptIn.Tests.ps1 (13 passed, 0 failed). |
|
@bbk007 Thanks for your contributions! This looks like a lot of work and it is appreciated. Do you mind walking through the approach for the wrapper function? I can see there may be some logic where this makes sense but I don't want to make assumptions. It is enough of a departure from the current approach that wouldn't mind some discussion just to capture the rational as we work through these new capabilities. |
Hello @soulemike |
2 participants
Summary
Validation
Closes #2071
Summary by CodeRabbit
New Features
Bug Fixes
Documentation