You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(scripts): repair three defects in the locked-restore diagnosis - #341
Review leftovers from #339. Three defects, each measured before and after.
1. The NU1004 gate read only stdout
Invoke-LockedRestore.ps1 teed only stdout, so an NU1004 written to stderr left $output without it and the job kept the bare error the wrapper exists to explain. Both streams are read now.
Measured against a drifted fixture and a stand-in that reports NU1004 on stderr only:
advice printed
before
no
after
src/Lib/packages.lock.json is stale: Pkg (net10.0) locked [1.0.0, ), central 2.0.0 + Fix with: dotnet restore wip.slnx --force-evaluate
2>&1 on its own is not the fix. Windows PowerShell turns every redirected stderr line into an error record, and $ErrorActionPreference = 'Stop' raises it before the advice can run — measured: the restore aborts with the stderr text as a RemoteException. #334 settled that both scripts stay Windows PowerShell 5.1 compatible, because nothing available before CI can run pwsh, so the preference is lowered for that one call and restored in a finally.
SDK 10.0.403 writes NU1004 to stdout, so the diagnosis worked before this change. It was the gate that was fragile, not the message.
2. An interval range was compared as written
ConvertTo-RequestedRange returned a bracketed interval with whitespace collapsed rather than in NuGet's spelling. For Version="[1.0.0,2.0.0)" NuGet records "requested": "[1.0.0, 2.0.0)", so an in-sync lock file read as drift — and since the advice still requires a real NU1004, the result was not a spurious failure but a wrong instruction: run --force-evaluate, which repairs nothing. Both sides now normalize to one spelling.
Measured on a fixture whose props and lock hold the same interval: drift count 1 before, 0 after. A genuinely different bound is still reported.
Not reachable on main before this change — both central versions are plain versions — and the tests covered only V and [V].
3. The test script leaked an exit code
The silent-restore case left $LASTEXITCODE at the stand-in's 7. shell: pwsh ends a step with exit $LASTEXITCODE, so a run whose every assertion passed still reported failure; it only survived because a later real restore happened to reset the code.
Measured with the epilogue GitHub appends: every assertion passed, step exit 7 before, 0 after.
Also
Test-LockedRestore.ps1 covers all three: the interval pair in both directions, and a restore that fails on its error stream returning its own exit code.
Its first cleanup is now guarded against a path outside the allocated fixture, like the one below it already was.
A requested range that is present but empty is skipped rather than compared, which is what the function's description already claimed.
Verification
./scripts/Test-LockedRestore.ps1 passes on the host (Windows PowerShell 5.1, SDK 10.0.403): PASS: exact SDK, immutable locks, NU1004 on drift, the stale-lock diagnosis, and whole-solution lock refresh, exit 0. An unrelated restore failure still gets no --force-evaluate advice.
CI then ran the same script under pwsh on both windows-latest and ubuntu-latest at this HEAD: Tests (windows-latest), Tests (ubuntu-latest), Native AOT publish and Lifecycle against real WSLC all succeeded. Both hosts are covered, which is what the lowered preference exists for.
Read NU1004 from both of the restore's streams. Only stdout was teed, so an
NU1004 on stderr left the gate unset and the job kept the bare error the
wrapper exists to explain. Measured against a drifted fixture and a
stderr-only stand-in: no advice before, the stale lock file and the repair
command after. 2>&1 alone does not do it -- Windows PowerShell turns a
redirected stderr line into an error record and 'Stop' raises it before the
advice can run, so the preference is lowered for that one call and restored
after it. Both scripts have to keep running on 5.1, since nothing available
before CI runs pwsh.
Compare an interval range in the spelling NuGet records. "[1.0.0,2.0.0)" in
the props was compared as written against the "[1.0.0, 2.0.0)" in the lock
file, so an in-sync pair read as drift and the user was told to run
--force-evaluate, which repairs nothing. Both sides now normalize. Measured:
drift count 1 before, 0 after; a different bound is still reported.
Stop the test script leaking an exit code. The silent-restore case left
$LASTEXITCODE at the stand-in's 7, and `shell: pwsh` ends a step with
`exit $LASTEXITCODE`. Measured with that epilogue: every assertion passed and
the step exited 7.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhR19gpMx4ZBuq6pkP4fgY
No actionable comments were generated in the recent review.
Walkthrough
The locked-restore scripts now normalize version ranges, detect NU1004 on either restore output stream, and preserve the restore exit code. Regression tests cover range comparisons, restore failures, exit-code handling, and guarded cleanup.
Changes: Locked Restore Diagnosis
Layer / File(s)
Summary
Normalize and compare requested ranges scripts/Get-LockFileDrift.ps1, scripts/Test-LockedRestore.ps1
Range normalization applies NuGet’s comma-spacing convention. Drift checks skip entries with missing or null requested values and compare empty values. Tests cover spacing differences and stale ranges.
Capture restore output and exit status scripts/Invoke-LockedRestore.ps1, scripts/Test-LockedRestore.ps1, docs/reproducible-builds.md
The wrapper captures both restore output streams, detects stale-lock advice, and preserves the restore exit code. Tests cover stderr failures and validate the diagnosis path before cleanup. Documentation describes the behavior and regression coverage.
This change fixes how the locked-restore diagnosis reads restore output, compares version ranges, and cleans up in its test script. No concrete merge-blocking risk was identified.
Pre-merge checks: 3 · 1
Failed checks: 1 inconclusive
Check name
Status
Explanation
Resolution
Linked Issues check
[#340] Defect 1, the NU1004 stream gate, is verified in source. At head, scripts/Invoke-LockedRestore.ps1 runs & $Dotnet restore $Solution --locked-mode 2>&1 | Tee-Object -Variable output and passes…
Provide the content of scripts/Get-LockFileDrift.ps1 and scripts/Test-LockedRestore.ps1 at head, or the base and head diffs for those paths, so that the interval normalization (ConvertTo-NormalizedRange and ConvertTo-RequestedRange), the re…
Passed checks: 3 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title names the change type and scope, and it describes repairs to the locked-restore diagnosis. The PR objectives confirm three defects were fixed: stderr-only NU1004 reaching diagnosis, spacing-…
Out of Scope Changes check
The changed areas named in the PR summary and change summary map to [#340]. These are: the NU1004 stream gate in scripts/Invoke-LockedRestore.ps1 (verified in source); interval-range normalization in …
Full details: Title check
Explanation
The title names the change type and scope, and it describes repairs to the locked-restore diagnosis. The PR objectives confirm three defects were fixed: stderr-only NU1004 reaching diagnosis, spacing-only interval differences causing false stale-lock reports, and the test script leaking the stand-in exit code. The title does not name the individual defects, but it identifies the primary area of change accurately. It is 66 characters, within a reasonable length for a title.
Full details: Out of Scope Changes check
Explanation
The changed areas named in the PR summary and change summary map to [#340]. These are: the NU1004 stream gate in scripts/Invoke-LockedRestore.ps1 (verified in source); interval-range normalization in scripts/Get-LockFileDrift.ps1; the exit-code and cleanup-guard changes in scripts/Test-LockedRestore.ps1; and docs/reproducible-builds.md, which documents the same behavior. The supporting test coverage is part of the same objectives. No file or hunk in the supplied evidence changes unrelated behavior. The unverified contents of the two script files are noted in the linked-issue assessment and do not establish any unrelated change.
Full details: Linked Issues check
Explanation
[#340] Defect 1, the NU1004 stream gate, is verified in source. At head, scripts/Invoke-LockedRestore.ps1 runs & $Dotnet restore $Solution --locked-mode 2>&1 | Tee-Object -Variable output and passes the combined $output to Get-LockedRestoreAdvice. It lowers $ErrorActionPreference to 'Continue' for that one call and restores it in a finally block. It returns the captured $LASTEXITCODE via exit $code. This meets the stream, Windows PowerShell 5.1, and exit-code criteria. Defects 2 and 3 are not verified in source. Repository diffs for scripts/Get-LockFileDrift.ps1 and scripts/Test-LockedRestore.ps1 were unavailable, and file reads of those two paths were not made within the inspection budget. The interval-normalization fix ([#340] defect 2), the $LASTEXITCODE reset and the new test coverage ([#340] defect 3 and the acceptance criteria for scripts/Test-LockedRestore.ps1) rest on the PR description and the change summary only. The PR description also contradicts itself on empty requested ranges. Its 'Also' section says present-but-empty requested ranges are skipped. The commit summary and change summary say they are compared as drift. The issue does not state which behavior is required. The CI green criterion rests on the PR author's report and was not verified.
Resolution
Provide the content of scripts/Get-LockFileDrift.ps1 and scripts/Test-LockedRestore.ps1 at head, or the base and head diffs for those paths, so that the interval normalization (ConvertTo-NormalizedRange and ConvertTo-RequestedRange), the requested-range skip rule, the exit-code reset, and the new test assertions can be checked. Confirm the CI run status for the head commit. Also resolve the empty-requested behavior conflict between the PR description and the commit summary so the stated behavior matches the code.
Finishing Touches
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Reviewing files that changed from the base of the PR and between b363e05 and e5490a9.
Files selected for processing: 4
docs/reproducible-builds.md
scripts/Get-LockFileDrift.ps1
scripts/Invoke-LockedRestore.ps1
scripts/Test-LockedRestore.ps1
Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour.
… code
Review findings on the previous commit.
The new stderr test only read the exit code, so deleting the stream merge --
the change it exists to guard -- left it green while the diagnosis silently
disappeared. It now runs a copy of the wrapper rooted at a drifted fixture and
asserts the advice text. Measured: with 2>&1 removed the case fails and the
script exits 1; with it, exit 0.
Reading that output needed ReadAllText. Get-Content -Raw emits no object for
an empty file, and -notmatch against an empty collection filters instead of
comparing, so it returns an empty array -- falsy -- and the first form of the
assertion could not fail at all.
ConvertTo-NormalizedRange's mandatory parameter rejected the empty string, so
a props Version of only whitespace aborted the diagnosis of the restore that
was already failing. AllowEmptyString keeps the textual comparison the
function always did.
The IsNullOrWhiteSpace skip was wider than the null case it was added for: an
emptied requested range disagrees with any central version and is drift, which
the previous code reported. It is skipped only when null now, and both cases
are covered.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LhR19gpMx4ZBuq6pkP4fgY
The reason will be displayed to describe this comment to others. Learn more.
Approved at e5490a9ff17ed4dcdfd5db4a3326c5c86163cef9. No outstanding findings.
code-review (high) and security-review each ran once, against 40560c8c — the HEAD before the fixes below. The security review found no HIGH or MEDIUM newly-introduced vulnerability: the merged stderr text is consumed only by a -like '*NU1004*' substring test and never reaches Invoke-Expression; the lowered $ErrorActionPreference is saved, scoped to one call and restored in a finally, and a non-launchable $Dotnet still terminates; the [xml] casts and ConvertFrom-Json are pre-existing context lines.
code-review raised three findings, all fixed in e5490a9f and re-verified against the updated code:
Medium — the new stderr case asserted only the exit code. Deleting the stream merge, the change the case exists to guard, left it green while the diagnosis disappeared. It now runs a copy of the wrapper rooted at a drifted fixture and asserts the advice text. Verified by mutation: with 2>&1 removed the case fails and the script exits 1; with it in place, exit 0.
Low — ConvertTo-NormalizedRange's mandatory parameter rejected the empty string, so a PackageVersion whose Version is only whitespace aborted the diagnosis of the restore that was already failing. [AllowEmptyString()] restores the textual comparison the function always did, and a case covers it.
Low — the IsNullOrWhiteSpace skip was wider than the null case it was added for. An emptied requested range disagrees with any central version and is drift, which the previous code reported. It is skipped only when $null now, with a case for each.
Fixing the first surfaced a defect in the fix itself: Get-Content -Raw emits no object for an empty file, and -notmatch against an empty collection filters rather than compares, returning an empty array — falsy. The first form of that assertion could not fail at all. It reads through [IO.File]::ReadAllText now, and the mutation run above is what established the assertion has teeth.
Checks: Tests (windows-latest), Tests (ubuntu-latest), Native AOT publish and Lifecycle against real WSLC all succeeded at this HEAD; 0 blocking. The two Tests jobs run scripts/Test-LockedRestore.ps1 under pwsh, so both PowerShell hosts are covered — which is what the lowered preference exists for, since the 5.1 behaviour it works around is what the suggested 2>&1-only fix would have tripped.
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
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![slidict-reviewer[bot]](https://avatars.githubusercontent.com/u/105342999?s=60&v=4){kind=small}
Closes #340.
Review leftovers from #339. Three defects, each measured before and after.
1. The NU1004 gate read only stdout
Invoke-LockedRestore.ps1teed only stdout, so an NU1004 written to stderr left$outputwithout it and the job kept the bare error the wrapper exists to explain. Both streams are read now.Measured against a drifted fixture and a stand-in that reports NU1004 on stderr only:
src/Lib/packages.lock.json is stale: Pkg (net10.0) locked [1.0.0, ), central 2.0.0+Fix with: dotnet restore wip.slnx --force-evaluate2>&1on its own is not the fix. Windows PowerShell turns every redirected stderr line into an error record, and$ErrorActionPreference = 'Stop'raises it before the advice can run — measured: the restore aborts with the stderr text as aRemoteException. #334 settled that both scripts stay Windows PowerShell 5.1 compatible, because nothing available before CI can runpwsh, so the preference is lowered for that one call and restored in afinally.SDK 10.0.403 writes NU1004 to stdout, so the diagnosis worked before this change. It was the gate that was fragile, not the message.
2. An interval range was compared as written
ConvertTo-RequestedRangereturned a bracketed interval with whitespace collapsed rather than in NuGet's spelling. ForVersion="[1.0.0,2.0.0)"NuGet records"requested": "[1.0.0, 2.0.0)", so an in-sync lock file read as drift — and since the advice still requires a real NU1004, the result was not a spurious failure but a wrong instruction: run--force-evaluate, which repairs nothing. Both sides now normalize to one spelling.Measured on a fixture whose props and lock hold the same interval: drift count 1 before, 0 after. A genuinely different bound is still reported.
Not reachable on
mainbefore this change — both central versions are plain versions — and the tests covered onlyVand[V].3. The test script leaked an exit code
The silent-restore case left
$LASTEXITCODEat the stand-in's 7.shell: pwshends a step withexit $LASTEXITCODE, so a run whose every assertion passed still reported failure; it only survived because a later real restore happened to reset the code.Measured with the epilogue GitHub appends: every assertion passed, step exit 7 before, 0 after.
Also
Test-LockedRestore.ps1covers all three: the interval pair in both directions, and a restore that fails on its error stream returning its own exit code.requestedrange that is present but empty is skipped rather than compared, which is what the function's description already claimed.Verification
./scripts/Test-LockedRestore.ps1passes on the host (Windows PowerShell 5.1, SDK 10.0.403):PASS: exact SDK, immutable locks, NU1004 on drift, the stale-lock diagnosis, and whole-solution lock refresh, exit 0. An unrelated restore failure still gets no--force-evaluateadvice.CI then ran the same script under
pwshon bothwindows-latestandubuntu-latestat this HEAD:Tests (windows-latest),Tests (ubuntu-latest),Native AOT publishandLifecycle against real WSLCall succeeded. Both hosts are covered, which is what the lowered preference exists for.🤖 Generated with Claude Code
https://claude.ai/code/session_01LhR19gpMx4ZBuq6pkP4fgY
Summary by CodeRabbit
Bug Fixes
Documentation