Repository navigation
Fix #62 (health-summary timezone) and #61 (robocopy credential warning) - #66
BangRocket wants to merge 2 commits into
Conversation
Get-SEBHealthSummary re-Parsed the JSON-deserialized metric timestamp, dropping it to Kind=Unspecified, so .ToUniversalTime() assumed local time and shifted the reported age by the host's UTC offset (~4h on a UTC-4 host). Coerce the stored value to UTC instead (SpecifyKind for a [datetime], AssumeUniversal+AdjustToUniversal for a string) at both the last-successful-age and 7-day-window sites. Tighten the test: a backup written exactly 5h ago must report ~5h on any host. Fixes #62.
Copy-SEBThrottled's robocopy strategy invokes the native exe as `& robocopy @args` and never references $Credential, so an operator passing -Credential with a robocopy transfer got a silent no-op (the copy ran as the calling identity) while BITS and Copy-Item honor it. Emit a Write-Warning on the robocopy path when -Credential is supplied, and correct the .PARAMETER Credential help to state which strategies honor it. Update the honesty-guard test to assert the warning is now emitted (and that the credential still never reaches the robocopy arg vector). Fixes #61.
📝 WalkthroughWalkthroughThe PR normalizes metric timestamps to UTC in ChangesUTC timestamp handling
Robocopy credential handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Modules/NetworkThrottle/Public/Copy-SEBThrottled.ps1`:
- Around line 133-137: The ignored-credential warning in the robocopy branch is
bypassing the module’s standard logging path by using direct warning output.
Update the $Credential check in Copy-SEBThrottled to route this message through
Write-SEBLog with WARN level (or the approved logging wrapper used elsewhere in
the module) so the warning is captured consistently in project logs while
preserving warning behavior.
- Around line 44-47: The help text for Copy-SEBThrottled.ps1 incorrectly claims
that Strategy 3 (Copy-Item) honors -Credential. Update the comment block near
the transfer strategy descriptions to remove that support statement and document
Strategy 3 as requiring pre-authenticated UNC access (for example via
New-PSDrive) instead. Keep the BITS/Strategy 1 credential wording, and make sure
the robocopy note remains unchanged.
In `@Tests/NetworkThrottle/Copy-SEBThrottled.Tests.ps1`:
- Around line 173-176: The warning assertion is too generic and can pass for
unrelated warnings; tighten it so it specifically verifies the
robocopy/Copy-SEBThrottled path. Update the existing Pester expectation around
the $warnings check in Copy-SEBThrottled.Tests.ps1 to include robocopy (or
calling identity) alongside Credential and ignored, so the test stays bound to
the issue `#61` contract and not to any other warning source.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 3bc9350f-306f-4974-9851-3f00de9595c8
📒 Files selected for processing (4)
Modules/MetricsCollector/Public/Get-SEBHealthSummary.ps1Modules/NetworkThrottle/Public/Copy-SEBThrottled.ps1Tests/MetricsCollector/Get-SEBHealthSummary.Tests.ps1Tests/NetworkThrottle/Copy-SEBThrottled.Tests.ps1
| An optional PSCredential object for authenticating to UNC paths. Honored by the BITS | ||
| (Strategy 1) and Copy-Item (Strategy 3) paths. The robocopy path (Strategy 2) CANNOT use | ||
| it -- robocopy runs as the calling identity and has no credential parameter -- so a | ||
| -Credential supplied on a robocopy transfer is ignored with a warning (see issue #61). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby lines.
file="Modules/NetworkThrottle/Public/Copy-SEBThrottled.ps1"
wc -l "$file"
cat -n "$file" | sed -n '1,220p'Repository: SandboxServers/SEBackup
Length of output: 12071
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for module logging conventions and any use of Copy-Item -Credential elsewhere.
rg -n --hidden --glob 'Modules/**/*.ps1' 'Write-Warning|Write-SEBLog|Copy-Item.*Credential|Credential' Modules/NetworkThrottle Modules | sed -n '1,220p'Repository: SandboxServers/SEBackup
Length of output: 34251
🌐 Web query:
PowerShell 7 Copy-Item -Credential FileSystem provider UNC support documentation
💡 Result:
The Copy-Item cmdlet in PowerShell 7 does not support the -Credential parameter when using the FileSystem provider [1][2][3]. Although the -Credential parameter appears in the syntax for Copy-Item, it is not functional for built-in PowerShell providers like the FileSystem provider [1][3][4]. If you attempt to use it, you will receive an error indicating that the provider does not support credentials [5]. The parameter is included in the cmdlet definition primarily for compatibility with third-party or custom PowerShell providers that may require explicit credentials to perform operations [4]. To access a UNC path with specific credentials in PowerShell 7, the recommended approach is to use the New-PSDrive cmdlet [6][7]. The FileSystem provider explicitly supports the -Credential parameter within New-PSDrive when mapping a network share (UNC path) [8][7]. Standard workflow: 1. Define your credentials using a PSCredential object [6][8]. 2. Create a temporary or persistent mapped drive using New-PSDrive with those credentials [8][7]. 3. Perform the Copy-Item operation using the drive letter or path associated with the new PSDrive [7]. Example: $cred = Get-Credential New-PSDrive -Name "RemoteShare" -PSProvider FileSystem -Root "\Server\Share" -Credential $cred Copy-Item -Path "C:\Source\file.txt" -Destination "RemoteShare:\file.txt"
Citations:
- 1: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.management/copy-item?view=powershell-7.5
- 2: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.management/copy-item?view=powershell-7.6
- 3: https://github.com/MicrosoftDocs/PowerShell-Docs/blob/main/reference/7.4/Microsoft.PowerShell.Management/Copy-Item.md
- 4: Beta.9 - Invoke-Item not accepting Credentials ends in error. PowerShell/PowerShell#5416
- 5: https://stackoverflow.com/questions/38938769/using-credential-with-copy-item-failure
- 6: https://stackoverflow.com/questions/67469217/powershell-unc-path-with-credentials
- 7: https://devblogs.microsoft.com/powershell/improving-the-filesystem-provider-through-community-feedback/
- 8: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.management/new-psdrive?view=powershell-7.6
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Modules/NetworkThrottle/Public/Copy-SEBThrottled.ps1"
echo "== target file =="
wc -l "$file"
cat -n "$file" | sed -n '1,220p'
echo
echo "== relevant usages =="
rg -n --hidden --glob 'Modules/**/*.ps1' 'Write-Warning|Write-SEBLog|Copy-Item.*-Credential|Copy-Item|robocopy|Strategy 2|Strategy 3' Modules/NetworkThrottle Modules | sed -n '1,220p'Repository: SandboxServers/SEBackup
Length of output: 46813
Remove Copy-Item -Credential from this help text. Copy-Item on the FileSystem provider does not honor -Credential in PowerShell 7, so Strategy 3 should be documented as requiring pre-authenticated access (for example via New-PSDrive) or limited to BITS only.
🤖 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 `@Modules/NetworkThrottle/Public/Copy-SEBThrottled.ps1` around lines 44 - 47,
The help text for Copy-SEBThrottled.ps1 incorrectly claims that Strategy 3
(Copy-Item) honors -Credential. Update the comment block near the transfer
strategy descriptions to remove that support statement and document Strategy 3
as requiring pre-authenticated UNC access (for example via New-PSDrive) instead.
Keep the BITS/Strategy 1 credential wording, and make sure the robocopy note
remains unchanged.
| if ($Credential) { | ||
| Write-Warning ("Copy-SEBThrottled: -Credential is ignored by the robocopy transfer strategy " + | ||
| "(robocopy runs as the calling identity and has no credential parameter). For a credentialed " + | ||
| "transfer use -UseBITS, or pre-authenticate the destination (e.g. a mapped/`New-PSDrive` share) " + | ||
| "so the current identity can reach it.") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Route the ignored-credential warning through the project logger.
This new branch writes directly with Write-Warning, so the event bypasses the standard module logging path. Please emit it via Write-SEBLog -Level WARN (or the project-approved wrapper that preserves warning behavior) so it is captured consistently in module logs. As per coding guidelines, “Use Write-SEBLog for all log output. Do not use Write-Host in module code.” As per path instructions, “Use Write-SEBLog for output, never Write-Host in module code.”
🤖 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 `@Modules/NetworkThrottle/Public/Copy-SEBThrottled.ps1` around lines 133 - 137,
The ignored-credential warning in the robocopy branch is bypassing the module’s
standard logging path by using direct warning output. Update the $Credential
check in Copy-SEBThrottled to route this message through Write-SEBLog with WARN
level (or the approved logging wrapper used elsewhere in the module) so the
warning is captured consistently in project logs while preserving warning
behavior.
Sources: Coding guidelines, Path instructions
| ($warnings -join ' ') | Should -Match 'Credential' # warned: not silently dropped | ||
| ($warnings -join ' ') | Should -Match 'ignored' | ||
| # Pester's -Match is case-insensitive; the credential (username or any '/credential' switch | ||
| # robocopy has no concept of) must appear nowhere in the arg vector. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Pin the warning assertion to robocopy specifically.
Matching only Credential and ignored would still pass on an unrelated warning from another layer. Add robocopy (or calling identity) to keep this test locked to the issue #61 contract.
🤖 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 `@Tests/NetworkThrottle/Copy-SEBThrottled.Tests.ps1` around lines 173 - 176,
The warning assertion is too generic and can pass for unrelated warnings;
tighten it so it specifically verifies the robocopy/Copy-SEBThrottled path.
Update the existing Pester expectation around the $warnings check in
Copy-SEBThrottled.Tests.ps1 to include robocopy (or calling identity) alongside
Credential and ignored, so the test stays bound to the issue `#61` contract and
not to any other warning source.
1 participant
Summary
Fixes the two open bug reports filed against the metrics/transfer code.
Fixes #62— Get-SEBHealthSummary timezone bugGet-SEBHealthSummaryre-[datetime]::Parse(...)d the metric timestamp thatConvertFrom-Jsonhad already deserialized asKind=Utc. Re-parsing a[datetime]stringifies it without an offset (Kind=Unspecified), and.ToUniversalTime()then assumes local time, so the reported "time since last backup" / 7-day success window was shifted by the host's UTC offset (a 4-hour drift on a UTC-4 host).Fix: coerce the stored value to UTC instead of re-parsing it (
SpecifyKind(Utc)for a[datetime],AssumeUniversal | AdjustToUniversalfor a string) at both the last-successful-age and 7-day-window sites. Tightened the test per the issue: a backup written exactly 5h ago must report ~5h on any host (was deliberately left loose to avoid locale flakiness).Fixes #61— Copy-SEBThrottled silently ignores -Credential on robocopyThe robocopy strategy runs
& robocopy @argsand never references$Credential(BITS and Copy-Item do honor it), so an operator passing-Credentialwith a robocopy transfer got a silent no-op running as the calling identity. Now warns on the robocopy path when-Credentialis supplied, and the.PARAMETER Credentialhelp states which strategies honor it. The honesty-guard test now asserts the warning is emitted (and that the credential still never reaches the robocopy arg vector).Testing
#62is fully verified on macOS PowerShell 7 (14/14 inGet-SEBHealthSummary.Tests.ps1, including the new timezone-invariant age assertion that fails pre-fix on a non-UTC host).#61's updated test exercises the robocopy path, which depends on therobocopyexecutable, so it runs in CI on Windows (norobocopyon the macOS dev host). The code change parses clean and introduces zero regressions in the non-robocopy paths.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation