diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0bafbe7..7270a2c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -80,36 +80,19 @@ jobs: shell: pwsh run: | Set-PSRepository PSGallery -InstallationPolicy Trusted - # Upper bound: Pester 6 will change semantics, and it must not arrive silently - Install-Module Pester -Force -Scope CurrentUser -MinimumVersion 5.0 -MaximumVersion 5.99.99 + + # The supported range lives in tools/Invoke-Tests.ps1, which the release gate + # also runs. Repeating the bound here is how CI and the gate drifted apart on + # the analyzer, so ask the script which range it will accept. + $range = (./tools/Invoke-Tests.ps1 -RequiredPesterRange) -split ' ' + Install-Module Pester -Force -Scope CurrentUser -MinimumVersion $range[0] -MaximumVersion $range[1] - name: Run Pester Tests shell: pwsh run: | - $config = New-PesterConfiguration - $config.Run.Path = "./tests" - $config.Run.PassThru = $true - $config.Output.Verbosity = "Detailed" - $config.TestResult.Enabled = $true - $config.TestResult.OutputFormat = "NUnitXml" - $config.TestResult.OutputPath = "./test-results.xml" - - $result = Invoke-Pester -Configuration $config - - # v2.17: skipped tests fail the build. The integration suite silently skips - # itself without administrator rights, and a green run that verified nothing - # is worse than a red one. GitHub Windows runners are elevated, so any skip - # here means something is actually wrong. - $notRun = $result.SkippedCount + $result.NotRunCount - if ($notRun -gt 0) { - Write-Host "::error::$notRun test(s) did not run - integration coverage is missing" - exit 1 - } - if ($result.FailedCount -gt 0) { - Write-Host "::error::$($result.FailedCount) test(s) failed" - exit 1 - } - Write-Host "::notice::All $($result.PassedCount) tests passed" + # Same script, same version bound and same "a skipped test fails the run" rule + # that tools/Invoke-ReleaseCheck.ps1 applies locally. + ./tools/Invoke-Tests.ps1 -FailOnProblem -ResultPath ./test-results.xml - name: Upload Test Results uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 diff --git a/CHANGELOG.md b/CHANGELOG.md index ec049b9..6fb3391 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,208 @@ Windows Update driver listing, run-to-run delta and HTML report. See CLAUDE.md. --- +## [2.20] - 2026-07-22 + +A correctness and honesty round driven by a full audit of the code base, a third-party +review and an independent Codex pass. No new cleanup features. Several places reported +success while doing nothing, one path check could be bypassed, and the fast disk-cleanup +path turned out to have been unreachable since it was written. + +### Security + +- **A junction could be used to clean a protected root.** The protected-path check + compared text, and `GetFullPath` does not resolve reparse points, so a link whose + visible path looked harmless while pointing at `Program Files` passed the guard; + enumerating that link then listed the target's contents. The cleanup root is now + resolved to its final target before the rules are applied. Measured on a live + filesystem: links found deeper in a tree were already safe, so only the root needed it, + and a link pointing somewhere harmless is still cleaned normally + +### Fixed + +- **Storage Sense was unreachable, so every run used the slow path.** The scheduled task + was looked up under `\Microsoft\Windows\DiskCleanup\`, where it does not exist; the real + one lives under `\Microsoft\Windows\DiskFootprint\`. Every run therefore fell back to + `cleanmgr`, which took 901 seconds of an 1101-second run on a real workstation and did + not finish. The task is now found by name, so the fast path is reachable at all. + **This does not by itself make that 901-second run fast**: on the workstation it was + measured on, the task is found and then fails with `0x80040154`, so the fallback still + runs - what removes the wait there is the new `-SkipDiskCleanup`. Two guards matter as + much as the lookup: a task that ran and *failed* no longer counts as success, and + neither does one that exits 0 without freeing anything measurable. Either would have + suppressed all 23 Disk Cleanup handlers while reporting success +- **Four operations reported success while doing nothing**: `npm cache clean` (its exit + code was never read, so a locked cache printed "cleaned"), event logs (a total + enumeration failure produced "cleared (0 logs)"), privacy traces (the success line was + appended without checking, because `Remove-Item -ErrorAction SilentlyContinue` cannot + throw), and the winget source update (only job completion was checked, not its result) +- **A failed log write was invisible.** All writer errors were swallowed, so a run could + continue destroying files while the log silently stopped recording. The first failure is + now reported once and surfaces as `LoggingDegraded` in the result JSON +- **A failed result-JSON write exited 0.** The code raised a warning while its own comment + said it must be loud, and the exit code is decided by the error count alone - so a run + that failed to produce the file the user asked for reported success +- **Disk Cleanup left running past its timeout was reported as an aside.** It is now a + warning with `DiskCleanupPending` in the result JSON: the totals printed after it are + partial. Its registry configuration is also no longer swept while it is still running +- **Delivery Optimization warned on healthy systems.** The measurement covered the whole + folder while the supported cmdlet only removes cached content, so leftover service logs + read as "nothing freed". Reproduced twice on the en-US stand +- **Browser cleanup could invent freed bytes.** The after-measurement returned 0 both for + "empty" and for "could not read", so a folder that became unreadable counted as fully + freed +- **The restore-point frequency override could stay applied forever.** It was only + repaired when the child process had been killed; a child that exited normally after its + own restore failed had its marker cleared anyway +- **`-SkipCleanup` was ignored by three functions when they were called directly**, which + contradicted the documented contract for anyone dot-sourcing the script +- Stand tooling: `Deploy-StandRunner` ignored ssh's exit code, and `New-StandVM` accepted + a PowerShell installation on file existence alone, ignoring msiexec's result + +Found by an independent review **of the fixes above**, before release: + +- **A winget that cannot start was the one silent path left in that block.** Only a + non-zero exit code was reported. When the winget entry is a Windows Apps stub that + passes `Test-Path` but will not launch, the job still completes, the error is swallowed + and no exit code is ever set - and the guard short-circuited on exactly that null. The + package list was then built from stale data without a word in the log +- **A partial event-log enumeration failure still read as success.** The check used the + *filtered* list, so 40 readable channels out of 510 with 470 errors printed "Event logs + cleared (40 logs)". The unfiltered result decides now, and channels that could not be + listed are reported separately from channels that failed to clear +- **The npm cache measurement had the defect this release fixed for browsers.** Both + sides used the walker that answers 0 for "empty" and for "unreadable" alike, so a cache + that became unreadable after the clean counted as fully freed +- **The browser measurement subtracted two different file sets.** "Before" used the raw + walker (inaccessible files skipped silently), "after" used the checked one (refuses to + answer on any error), so a real deletion could land in the "nothing freed" branch. Both + sides are now measured per path with the same function, and a single unmeasurable + folder no longer discards the delta for all thirty. A second review pass caught the + first attempt at this: clamping each path at zero separately would have reported 100 MB + when one cache shrank by 100 MB while another was recreated and grew by 80 MB, so the + measurable pairs are summed first and clamped once +- **Killing the restore-point child raced its own cleanup.** `Kill` returns once + termination has been requested, not once the process is gone, so the repair could read + the creation frequency while the dying child was still writing 0 into it, see a healthy + value, and delete the marker - producing exactly the damage the marker exists to record. + The wait is bounded, so a child that outlives it keeps the marker rather than being + assumed dead +- **Storage Sense re-resolved its task by name while waiting**, quietly undoing the rule + that refuses to guess between same-named tasks; and a task that disappeared after five + seconds was reported as "did not finish within 120 seconds", a number that never + happened +- **A thousands separator was read as a decimal point, dividing sizes by a thousand.** + `"1,234 KB"` - the ordinary en-US form, and exactly what the shell returns for Recycle + Bin entries when the exact size property is unavailable - was read as 1.234 KB. The old + rule said a lone separator is always the decimal point. The obvious repair would have + been worse than the defect: measured on .NET, `AllowThousands` does not validate the + grouping shape, so parsing with the current culture reads `"1,5"` as 15 on en-US, that + is a tenfold over-read replacing a thousandfold under-read. The grouping shape is + checked first now (one to three digits, then groups of exactly three), and the culture + is consulted only for a string that could honestly be read either way +- **A test file that failed to load kept CI green.** Measured on Pester 5.7.1: a parse + error yields `Result=Failed` and `FailedContainersCount=1` while the failed, skipped and + not-run counters all stay at 0 - so counting tests alone could not see an entire test + file going missing. Both `tools/Invoke-Tests.ps1` and the release gate now check it + +Found by a full pre-release review (four specialised reviewers plus a cross-engine pass), +after the stand had already passed on both machines: + +- **The Storage Sense verdict crashed on every real failure code, and the test for it was + green because of the defect.** `LastTaskResult` is a `UInt32`; every HRESULT failure has + the high bit set, so `0x80040154` arrives as 2147746132 and the `[int]` cast threw. The + exception left `Invoke-StorageSense`, the phase was recorded as failed, and + `Clear-WindowsOld` never ran. The test passed the PowerShell literal `0x80040154`, which + the parser types as `Int32` -2147221164, so the cast succeeded there. Two reviewers found + this independently; the fix parses instead of casting, and the test now uses the type + Windows actually supplies +- **A missing baseline counted as proof that Storage Sense had run.** When the pre-run task + info could not be read, `-not $LastRunBefore` was true, so the first evidence check + returned "finished" for a task that might never have started - and a stale success code + plus any unrelated free-space growth then skipped all 23 Disk Cleanup handlers. That + state is now its own outcome and always falls back +- **A `cleanmgr.exe` that never started produced fifteen minutes of fabricated progress.** + `Start-Process` leaves `$null` when the executable is missing or blocked, and + `$null.HasExited` is `$null`, so the wait loop ran its full course and then reported that + an elevated process was still deleting in the background +- **Two more fail-open holes in the protected-path guard.** An ancestor that could not be + examined was silently treated as "not a link", and exhausting the resolution bound + returned the partially resolved path instead of `$null`. Both let a path be judged on its + text, which is the bypass this release exists to close +- **A stale comment in that same guard asserted the opposite of the code.** This is how the + fail-open bootstrap shipped in 2.17, and that claim reached SECURITY.md before anyone + checked it +- Storage Sense also: a two-minute timeout is a warning again rather than a silent INFO, + "task stopped" is claimed only after checking that it stopped, an ambiguous lookup no + longer also says the task was not found, and an unmapped drive is treated as absent + rather than as unexaminable +- **`-SkipDiskCleanup` skipped the registry sweep it promised to run, and was credited with + bytes it never freed** - the step was measured even when switched off, so unrelated + free-space growth was reported as `DiskCleanup freed approximately N MB` +- **A release note pasted into the wrong help block satisfied the release gate on its own.** + The gate matched `.RELEASENOTES` against the whole file, so the real entry could have been + deleted while the check stayed green. It is scoped to the `.RELEASENOTES` section now, and + a test pins that the note exists in exactly one place + +A fourth review round, on the fixes above: + +- **The repair for the missing baseline gave up too early.** Returning "unverifiable" after + ten seconds left a slow-starting task free to begin afterwards, with Disk Cleanup already + running alongside it. The whole wait window is used to watch now, and being seen running + is accepted as evidence on its own, because it needs nothing to compare against +- **The ancestor walk climbed past the root of a UNC share**, where `Get-Item` cannot + succeed, so the newly fail-closed rule refused every UNC cleanup root. The walk stops at + the volume root +- **Scoping the release-note check to the PSScriptInfo block was still too wide** - a + matching line under any other field satisfied it. It reads the `.RELEASENOTES` section + +### Added + +- **`-SkipDiskCleanup`** skips only the Storage Sense / Disk Cleanup step. Until now the + only way to avoid the slowest step was `-SkipCleanup`, which suppresses every category +- Result JSON fields `LoggingDegraded` and `DiskCleanupPending` +- `tools/Invoke-Tests.ps1`: one definition of the supported Pester range, the test path and + the "a skipped test fails the run" rule, used by both CI and the release gate + +### Changed + +- **The release gate no longer claims things it did not verify.** "In sync with origin" was + read from the local remote-tracking ref without ever fetching, and the Pester version was + unbounded while CI pinned an upper bound - with Pester 6 published, one `Install-Module` + would have split the two + +### Documentation + +- `docs/troubleshooting.md` explains why the same application can be offered for update on + every run, which is a question WinClean will be blamed for and cannot fix in code. Two + causes, both reproduced on a live machine: `winget` refuses to manage user-scope packages + from an elevated process (and WinClean requires elevation), and an installer that records + a stale version in its own uninstall entry makes the offer permanent, because that entry + is what `winget` compares against rather than the installed executable. Includes the + read-only diagnosis and both ways out + +### Tests + +- 376 to 452 automated tests. New coverage: the junction guard (a link to a protected root + is refused, a harmless one is not), a fresh per-run statistics object, the registry value + counter, and a mocked event-log enumeration failure +- **The Storage Sense rewrite had no tests at all** - the largest gap in this release, and + the one place where a defect ("exit code 0 proves a cleanup happened") had already been + found. Its three decisions are now separate functions with 15 behavioural tests: which + task to use and when to refuse to guess, whether a run counts as a cleanup, and how the + wait ends. The wait no longer needs a scheduler or two real minutes to be tested +- Three deliberate mutations were used to confirm the new tests can fail: removing the + free-space requirement, collapsing "task vanished" into "timed out", and letting the + selector pick the first of several same-named tasks. All three were caught, and the file + was restored by hash after each +- Two logging tests could not fail: their only assertion sat inside `if (Test-Path $log)`, + so a missing log file - the very defect they exist to catch - made them pass. Both were + verified by mutation after the fix +- The version tests compared against the regex `2\.1[3-9]`, which stopped matching at 2.20 + and would have failed on the bump itself + +--- + ## [2.19] - 2026-07-22 A contract-correctness and documentation round, following a second external review (this time diff --git a/CLAUDE.md b/CLAUDE.md index 3e7ff08..127ee22 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -43,9 +43,9 @@ CleanScript/ │ └── logo.svg # Логотип проекта ├── get.ps1 # Bootstrap: разовый запуск одной командой (irm | iex) ├── install.ps1 # Bootstrap: установка/обновление + ярлык (RunAs admin) -├── tests/ # Pester тесты (376 всего; счётчик - прогоном, не грепом) -│ ├── Helpers.Tests.ps1 # Unit-тесты helper-функций (118, дот-сорсят продукт - нужны права админа) -│ ├── Fixes.Tests.ps1 # Валидационные тесты исправлений (148) +├── tests/ # Pester тесты (452 всего; счётчик - прогоном, не грепом) +│ ├── Helpers.Tests.ps1 # Unit-тесты helper-функций (159, дот-сорсят продукт - нужны права админа) +│ ├── Fixes.Tests.ps1 # Валидационные тесты исправлений (183) │ ├── Integration.Tests.ps1 # Интеграционные тесты в песочнице ФС (67, требуют admin) │ ├── StandHelpers.Tests.ps1 # Чистые хелперы стенда: dead-man + версионный гейт ассертов (17, без admin) │ └── Docs.Tests.ps1 # Гварды документации: нет тире + нет битых внутренних ссылок (26, без admin) @@ -198,7 +198,7 @@ Publish-PSResource -Path .\WinClean.ps1 -Repository PSGallery -ApiKey $env:PSGAL **Проверки (4 job'а, счёт сверять gh run view --json jobs, а не этой строкой):** 1. **lint** - PSScriptAnalyzer (Error+Warning) через общий ools/Invoke-Lint.ps1 - тот же, что зовёт релиз-гейт 2. **syntax** - Проверка синтаксиса PowerShell -3. **test** - Pester тесты (376; интеграционные требуют admin - на GitHub runners это выполняется) +3. **test** - Pester тесты (452; интеграционные требуют admin - на GitHub runners это выполняется) 4. **smoke** - прогон -ReportOnly + геометрия рамок + result JSON **Исключения PSScriptAnalyzer** (допустимые для CLI): @@ -208,7 +208,7 @@ Publish-PSResource -Path .\WinClean.ps1 -Repository PSGallery -ApiKey $env:PSGAL ### Pester тесты (v2.13+) -- `tests/Helpers.Tests.ps1` - 118 unit-тестов (дот-сорсят WinClean.ps1), `tests/Fixes.Tests.ps1` - 148, `tests/Integration.Tests.ps1` - 67 (песочница ФС, требуют admin), `tests/StandHelpers.Tests.ps1` - 17 (без admin), `tests/Docs.Tests.ps1` - 26 (гварды доков, без admin) +- `tests/Helpers.Tests.ps1` - 159 unit-тестов (дот-сорсят WinClean.ps1), `tests/Fixes.Tests.ps1` - 183, `tests/Integration.Tests.ps1` - 67 (песочница ФС, требуют admin), `tests/StandHelpers.Tests.ps1` - 17 (без admin), `tests/Docs.Tests.ps1` - 26 (гварды доков, без admin) - Особенности: функции в BeforeAll (не AST), regex для locale-независимости, отдельные It блоки --- @@ -357,7 +357,7 @@ pwsh tools/Invoke-ReleaseCheck.ps1 # версия во всех pwsh tools/Invoke-ReleaseCheck.ps1 -IncludeStand # + боевой прогон на VM (минуты) pwsh tools/Invoke-ReleaseCheck.ps1 -VerifyPublished # ПОСЛЕ выпуска: ассеты релиза и SHA256 -Invoke-Pester ./tests -Output Detailed # 376 Pester тестов (считать прогоном, не грепом) +Invoke-Pester ./tests -Output Detailed # 452 Pester тестов (считать прогоном, не грепом) pwsh tools/Invoke-SmokeTest.ps1 # Смоук: ReportOnly + геометрия UI pwsh tools/proxmox/Invoke-StandTest.ps1 -Mode Report # Стенд на Proxmox (RU=VM 190, EN: -ConfigPath ...en.json = VM 191) # Ночная матрица: cron 03:30 на proxmos (/opt/winclean-stand, /etc/cron.d/winclean-stand), отчёт в Telegram diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ffda3ad..de2057a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -128,7 +128,7 @@ Invoke-Pester ./tests/Integration.Tests.ps1 All PRs automatically run: - PSScriptAnalyzer (linting) - Syntax check -- Pester tests (376 tests) +- Pester tests (452 tests) ### Release-impacting changes @@ -251,7 +251,7 @@ Invoke-Pester ./tests/Integration.Tests.ps1 Все PR автоматически проходят: - PSScriptAnalyzer (линтинг) - Проверка синтаксиса -- Pester тесты (376 тестов) +- Pester тесты (452 тестов) ### Изменения, влияющие на релиз diff --git a/README.md b/README.md index f08cb08..1829e1e 100644 --- a/README.md +++ b/README.md @@ -221,6 +221,7 @@ cd WinClean | `-SkipDevCleanup` | Skip developer caches (npm, pip, etc.) | `false` | | `-SkipDockerCleanup` | Skip Docker/WSL cleanup | `false` | | `-SkipVSCleanup` | Skip Visual Studio cleanup | `false` | +| `-SkipDiskCleanup` | Skip only the Storage Sense / Disk Cleanup step, which can be the slowest | `false` | | `-DisableTelemetry` | Disable Windows telemetry via Group Policy | `false` | | `-ReportOnly` | **Dry run** - show what would be done | `false` | | `-LogPath` | Custom log file path | Auto | @@ -302,7 +303,7 @@ WinClean is built to be safe to run on a working machine. The short version: ``` ┌────────────────────────────────────────────────────────────────┐ -│ WinClean v2.19 │ +│ WinClean v2.20 │ ├────────────────────────────────────────────────────────────────┤ │ PREPARATION │ │ ├─ ✓ Check Administrator Rights │ @@ -321,7 +322,8 @@ WinClean is built to be safe to run on a working machine. The short version: ├────────────────────────────────────────────────────────────────┤ │ DEEP CLEANUP │ │ ├─ 🔧 DISM Component Cleanup │ -│ ├─ 💾 Storage Sense, or Disk Cleanup (up to 23 handlers) │ +│ ├─ 💾 Storage Sense, then Disk Cleanup unless it proved │ +│ │ it worked (up to 23 handlers) │ │ ├─ 🚗 Driver Store (superseded packages) │ │ ├─ 🧹 Stale Kernel Dumps (older than 30 days) │ │ └─ 📁 Windows.old Removal (prompted; 15s default = yes) │ diff --git a/README_RU.md b/README_RU.md index ce9cda4..d0fec39 100644 --- a/README_RU.md +++ b/README_RU.md @@ -221,6 +221,7 @@ cd WinClean | `-SkipDevCleanup` | Пропустить кэши разработчика (npm, pip и т.д.) | `false` | | `-SkipDockerCleanup` | Пропустить очистку Docker/WSL | `false` | | `-SkipVSCleanup` | Пропустить очистку Visual Studio | `false` | +| `-SkipDiskCleanup` | Пропустить только шаг Storage Sense / Disk Cleanup, он бывает самым долгим | `false` | | `-DisableTelemetry` | Отключить телеметрию Windows через групповую политику | `false` | | `-ReportOnly` | **Тестовый режим** - показать, что будет сделано | `false` | | `-LogPath` | Путь к файлу лога | Авто | @@ -302,7 +303,7 @@ WinClean создан так, чтобы его можно было безопа ``` ┌────────────────────────────────────────────────────────────────┐ -│ WinClean v2.19 │ +│ WinClean v2.20 │ ├────────────────────────────────────────────────────────────────┤ │ ПОДГОТОВКА │ │ ├─ ✓ Проверка прав администратора │ @@ -321,7 +322,8 @@ WinClean создан так, чтобы его можно было безопа ├────────────────────────────────────────────────────────────────┤ │ ГЛУБОКАЯ ОЧИСТКА │ │ ├─ 🔧 Очистка компонентов DISM │ -│ ├─ 💾 Storage Sense или Очистка диска (до 23 обработчиков) │ +│ ├─ 💾 Storage Sense, затем Очистка диска, если он не доказал │ +│ │ что сработал (до 23 обработчиков) │ │ ├─ 🚗 Хранилище драйверов (устаревшие пакеты) │ │ ├─ 🧹 Старые дампы ядра (старше 30 дней) │ │ └─ 📁 Удаление Windows.old (запрос; 15 сек по умолч. = да) │ diff --git a/WinClean.ps1 b/WinClean.ps1 index 70b7d8e..ee51e92 100644 --- a/WinClean.ps1 +++ b/WinClean.ps1 @@ -1,5 +1,5 @@ <#PSScriptInfo -.VERSION 2.19 +.VERSION 2.20 .GUID 8f7c3b2a-1d4e-5f6a-9b8c-0d1e2f3a4b5c .AUTHOR bivlked .COMPANYNAME @@ -12,6 +12,7 @@ .REQUIREDSCRIPTS .EXTERNALSCRIPTDEPENDENCIES .RELEASENOTES + v2.20: Correctness and honesty round - a junction could bypass protected-path checks, four operations reported success while doing nothing, Storage Sense was unreachable so on machines where it works the slow Disk Cleanup no longer runs; where it fails, the new -SkipDiskCleanup is what removes the wait v2.19: Contract and documentation round - -SkipCleanup now skips ALL cleanup categories, result JSON gains a tri-state PhasesSkipped, AppUpdatesCount renamed to AppUpdatesOffered (offered, not installed), full docs overhaul v2.18: Correctness and hardening follow-up from external code review - diskpart failure detection, driver-store accounting, strict superseded-version rule, exact bootstrap host allowlist v2.17: Silent failure hardening - operations that quietly do nothing now say so instead of reporting success @@ -28,7 +29,7 @@ <# .SYNOPSIS - WinClean - Ultimate Windows 11 Maintenance Script v2.19 + WinClean - Ultimate Windows 11 Maintenance Script v2.20 .DESCRIPTION Комплексный скрипт для обновления и очистки Windows 11: - Обновление Windows (включая драйверы) @@ -42,8 +43,18 @@ - Подробный цветной вывод + лог-файл .NOTES Author: biv - Version: 2.19 + Version: 2.20 Requires: PowerShell 7.1+, Windows 11, Administrator rights + Changes in 2.20: + - SECURITY: a junction whose target is a protected root could be used as a cleanup + root - the path check compared text and never resolved the link + - Storage Sense was looked up at a path where it does not exist, so every run fell + back to Disk Cleanup (15 of 18 minutes on a real workstation). Where Storage Sense + itself fails, the fallback still runs - use -SkipDiskCleanup there + - npm, event logs, privacy traces and winget source update no longer report success + when they did nothing + - Added -SkipDiskCleanup to skip only the slow Disk Cleanup step + - Result JSON gains LoggingDegraded and DiskCleanupPending Changes in 2.19: - -SkipCleanup now skips the ENTIRE cleanup group (system, deep, developer, Docker/WSL, Visual Studio), matching the documented "skip all cleanup" contract. @@ -257,6 +268,13 @@ Пропустить очистку Docker/WSL .PARAMETER SkipVSCleanup Пропустить очистку Visual Studio +.PARAMETER SkipDiskCleanup + Пропустить только шаг Storage Sense / Disk Cleanup (штатная утилита Windows). + Этот шаг бывает самым долгим: на реальной рабочей станции cleanmgr занял 15 минут + из 18 и не успел завершиться, тогда как на чистой виртуальной машине те же + 23 категории отрабатывают за 10 секунд. Причина разницы НЕ установлена: измерение + показывает лишь, что стоимость зависит от накопленного состояния машины. Остальная + очистка при этом выполняется - в отличие от -SkipCleanup, который гасит её целиком .PARAMETER DisableTelemetry Отключить телеметрию Windows (через групповую политику) .PARAMETER ReportOnly @@ -282,6 +300,7 @@ param( [switch]$SkipDevCleanup, [switch]$SkipDockerCleanup, [switch]$SkipVSCleanup, + [switch]$SkipDiskCleanup, [switch]$DisableTelemetry, [switch]$ReportOnly, [string]$LogPath, @@ -298,7 +317,22 @@ $OutputEncoding = [System.Text.Encoding]::UTF8 $PSDefaultParameterValues['*:Encoding'] = 'utf8' # Statistics storage (synchronized hashtable for safe concurrent access) -$script:Stats = [hashtable]::Synchronized(@{ +function New-RunStats { + <# + .SYNOPSIS + Builds a fresh per-run statistics object + .DESCRIPTION + v2.20: this was a literal assigned once when the script loaded, and Start-WinClean + reset only the phase buckets and the step counter - while the comment there + described "dot-source and call Start-WinClean twice" as the case being handled. + Everything else survived: freed bytes, per-category totals, update counts, warning + and error counts, RebootRequired, Aborted and StartTime. A second run in the same + session therefore reported the first run's bytes and errors, and computed its + duration from the moment the script was dot-sourced. + + One definition, used both at load time and at the start of every run. + #> + return [hashtable]::Synchronized(@{ TotalFreedBytes = [long]0 FreedByCategory = @{} WindowsUpdatesCount = 0 @@ -316,6 +350,9 @@ $script:Stats = [hashtable]::Synchronized(@{ # 'unknown' must not be mistaken for a verified state by consumers of the JSON ControlledFolderAccess = 'unknown' Aborted = $null # v2.17: set when the run stops before finishing + # v2.20: cleanmgr outlived its timeout and is still deleting in the background. The + # totals reported by this run are partial, and a consumer must not read them as final. + DiskCleanupPending = $false # v2.17 (p.11 of the audit): which top-level phases ran to completion vs threw. # Before this, one exception anywhere in the run silently skipped every phase # after it - Developer Cleanup, Docker/WSL, Visual Studio, Deep System Cleanup, @@ -330,7 +367,10 @@ $script:Stats = [hashtable]::Synchronized(@{ PhasesCompleted = @() PhasesFailed = @() PhasesSkipped = @() -}) + }) +} + +$script:Stats = New-RunStats # Progress activities seen so far, so all of them can be closed at the end (v2.16) $script:ProgressActivities = @() @@ -339,6 +379,10 @@ $script:ProgressActivities = @() # the check costs up to ~15s offline and is called from two separate update phases $script:InternetConnectionCache = $null +# Latched by Write-Log when the log file cannot be written, so the failure is reported +# once instead of on every call - and surfaces in the result JSON as LoggingDegraded +$script:LogWriteFailed = $false + # Initialize log path (script scope for access in functions) if (-not $LogPath) { $script:LogPath = Join-Path $env:TEMP "WinClean_$((Get-Date).ToString('yyyyMMdd_HHmmss')).log" @@ -352,7 +396,7 @@ if (-not $LogPath) { } # Script version (single source of truth for version checking) -$script:Version = "2.19" +$script:Version = "2.20" # Protected paths that should never be deleted $script:ProtectedPaths = @( @@ -415,7 +459,29 @@ function Write-Log { $script:LogWriterPath = $script:LogPath } $script:LogWriter.WriteLine($logMessage) - } catch { } + } catch { + # v2.20: this used to be an empty catch, so a log that stopped being written + # (full volume, revoked permissions, the v2.14 case where cleanup deleted the + # log out from under us) was invisible: destructive work carried on, the final + # JSON said ErrorsCount=0, and LogPath pointed at a truncated file. + # + # Latched: one console line, not one per call - Write-Log fires hundreds of + # times per run. Deliberately Write-Host and not Write-Log, which would + # recurse straight back into this catch. + # v2.20, corrected in review: drop the writer so the NEXT call reopens it. + # Without this the guard above stays satisfied by a dead writer object and + # every later line is silently discarded for the rest of the run - the empty + # catch would simply have moved from the first failure to all the others. + try { if ($script:LogWriter) { $script:LogWriter.Dispose() } } catch { } + $script:LogWriter = $null + $script:LogWriterPath = $null + + if (-not $script:LogWriteFailed) { + $script:LogWriteFailed = $true + $script:Stats.WarningsCount++ + Write-Host " [WARN] Log file could not be written ($($_.Exception.Message)) - the run continues, but $($script:LogPath) may be incomplete" -ForegroundColor Yellow + } + } } # Console output with colors @@ -1239,11 +1305,29 @@ function ConvertFrom-HumanReadableSize { an unhandled exception instead of returning 0), the word form of bytes, and "MiB"/"GiB"/etc binary-unit spelling (this script's own *B literals are already 1024-based, so the multiplier is identical to KB/MB/GB/TB). + v2.20: a lone separator is no longer assumed to be the decimal point. The grouping + SHAPE decides first, and only a string that could honestly be read either way is + settled by the culture - so the result for such a string is culture-dependent. + .PARAMETER SizeString + The text to convert, e.g. "2.5 GB", "1,234 KB", "816 КБ". + .PARAMETER Culture + Consulted only for an ambiguous grouping such as "1,234", where en-US means 1234 + and ru-RU means 1.234. Defaults to the current culture, which is what the Shell + used to format the string this function's only caller parses. Shapes that cannot + be a grouping ("1,5", "1,2345") are decimal in every culture. .EXAMPLE ConvertFrom-HumanReadableSize "2.5 GB" # Returns 2684354560 ConvertFrom-HumanReadableSize "512MB" # Returns 536870912 + .EXAMPLE + ConvertFrom-HumanReadableSize "1,234 KB" -Culture ([cultureinfo]'en-US') # 1263616 + ConvertFrom-HumanReadableSize "1,234 KB" -Culture ([cultureinfo]'ru-RU') # 1264 #> - param([string]$SizeString) + param( + [string]$SizeString, + # Only consulted for a genuinely ambiguous string (see the disambiguation below). + # Injectable so the rule can be tested without changing the machine's locale. + [cultureinfo]$Culture = [cultureinfo]::CurrentCulture + ) if (-not $SizeString) { return 0 } @@ -1263,15 +1347,53 @@ function ConvertFrom-HumanReadableSize { $numberPart = $Matches[1] $unit = $Matches[2].ToUpper() - # Decimal-separator ambiguity ("1.234,5" EU vs "1,234.5" US vs a lone "," or "."): - # whichever mark appears LAST is the decimal point; anything earlier was a - # thousands grouping and is dropped. + # Decimal-separator disambiguation. + # + # When BOTH marks appear the answer is certain: whichever comes LAST is the decimal + # point and the earlier one was grouping ("1.234,5" EU against "1,234.5" US). + # + # A LONE mark is the hard case, and v2.20 is where it was fixed. The old rule was + # "a lone mark is the decimal point", which read the ordinary en-US thousands form + # "1,234 KB" as 1.234 KB - low by a factor of a thousand, on the shell fallback that + # measures the Recycle Bin. + # The obvious repair is worse. Handing the string to [double]::TryParse with the + # current culture looks right and is not: measured on .NET, AllowThousands does NOT + # validate the grouping shape, so en-US reads "1,5" as 15 and "1,2345" as 12345 - + # trading a 1000x under-read for a 10x over-read, and breaking "1,5 GB". + # So the SHAPE is checked here first, and the culture is consulted only for a string + # that could honestly be either reading. $lastComma = $numberPart.LastIndexOf(',') $lastDot = $numberPart.LastIndexOf('.') - if ($lastComma -gt $lastDot) { - $numberPart = $numberPart.Replace('.', '').Replace(',', '.') - } elseif ($lastDot -gt $lastComma) { - $numberPart = $numberPart.Replace(',', '') + + if ($lastComma -ge 0 -and $lastDot -ge 0) { + if ($lastComma -gt $lastDot) { + $numberPart = $numberPart.Replace('.', '').Replace(',', '.') + } else { + $numberPart = $numberPart.Replace(',', '') + } + } elseif ($lastComma -ge 0 -or $lastDot -ge 0) { + $sep = if ($lastComma -ge 0) { ',' } else { '.' } + + # A thousands grouping is "1-3 digits, then one or more groups of exactly 3". + # "1,5" and "1,2345" cannot be that, so there the mark is the decimal point and + # no culture can argue otherwise. + if ($numberPart -match "^\d{1,3}($([regex]::Escape($sep))\d{3})+$") { + $isDecimal = $Culture.NumberFormat.NumberDecimalSeparator -eq $sep + if (-not $isDecimal -and $Culture.NumberFormat.NumberGroupSeparator -ne $sep) { + # The culture uses this mark for neither purpose - ru-RU groups with a + # no-break space and would call a lone dot meaningless. Our own + # Format-FileSize writes invariant text, so fall back to reading it that + # way: a dot is the decimal point, a comma is grouping. + $isDecimal = ($sep -eq '.') + } + if ($isDecimal) { + $numberPart = $numberPart.Replace(',', '.') + } else { + $numberPart = $numberPart.Replace($sep, '') + } + } else { + $numberPart = $numberPart.Replace(',', '.') + } } $multiplier = switch ($unit) { @@ -1290,10 +1412,123 @@ function ConvertFrom-HumanReadableSize { } } +function Resolve-PathThroughLinks { + <# + .SYNOPSIS + Resolves a path through reparse points at ANY level, not just the last segment + .DESCRIPTION + v2.20, corrected in review. The first version of the link-aware protected-path + check only asked whether the leaf itself was a reparse point. That closed the + obvious case ("C:\cache" is a junction to "C:\") and left the real one open: + "C:\cache\Windows" has no reparse attribute on the leaf, GetFullPath does not + resolve the junction above it, and the textual comparison never matches - measured, + with 120 real C:\Windows children visible through the link. + + So every ancestor is examined, the deepest link found is resolved, and the walk + restarts on the rebuilt path (a resolved target can itself sit under another link). + .OUTPUTS + The fully resolved path, or $null when a link cannot be resolved - the caller must + treat that as "unknown", never as "fine". + #> + param([Parameter(Mandatory)][string]$Path) + + $current = $Path + # Bounded: a link loop would otherwise spin here forever + for ($round = 0; $round -lt 64; $round++) { + # Raised in review: the ancestor walk used to climb past the root of a UNC share. + # Split-Path turns \\server\share into \\server, which is not a filesystem object, + # so Get-Item failed, the fail-closed rule above answered $null, and every UNC + # cleanup root was refused. Nothing above the volume root is ours to inspect. + $rootPath = '' + try { $rootPath = [System.IO.Path]::GetPathRoot($current).TrimEnd('\', '/') } catch { $rootPath = '' } + + $probe = $current + $tail = @() + $changed = $false + + while ($probe) { + # Raised in review: an ancestor that could not be examined used to be silently + # classified as "not a link" and the walk carried on upward, so an unreadable + # junction ancestor answered "safe to empty". That is the half of the guard + # which closes the real attack (C:\cache\Windows where C:\cache is the link), + # and it was the half that failed open. Unknown is not safe. + $item = $null + try { $item = Get-Item -LiteralPath $probe -Force -ErrorAction Stop } catch { return $null } + + if ($item -and ($item.Attributes -band [System.IO.FileAttributes]::ReparsePoint)) { + $target = $null + try { $target = $item.ResolveLinkTarget($true) } catch { $target = $null } + if (-not $target) { return $null } + + $current = if ($tail.Count -gt 0) { + Join-Path $target.FullName ($tail -join [System.IO.Path]::DirectorySeparatorChar) + } else { + $target.FullName + } + $changed = $true + break + } + + $parent = Split-Path $probe -Parent + if (-not $parent -or $parent -eq $probe) { break } + if ($rootPath -and $parent.Length -lt $rootPath.Length) { break } + $tail = ,(Split-Path $probe -Leaf) + $tail + $probe = $parent + } + + # Nothing left to resolve: this is the fully resolved answer + if (-not $changed) { return $current } + } + + # The bound was exhausted while links were still being followed, which means the chain + # could not be resolved. The contract for that is $null. Returning the partially + # resolved path (raised in review) handed Test-PathProtected a value that still + # contained a link and was then judged on its text alone - fail-open in the one + # function this release rewrote to fail closed. + return $null +} + +function Get-RegistryValueCount { + <# + .SYNOPSIS + Counts the real values under a registry key, ignoring PowerShell's own metadata + .DESCRIPTION + v2.20. Privacy cleanup used to announce "cleared" without looking, because + Remove-Item with -ErrorAction SilentlyContinue never throws. Confirming the result + needs a before/after count, and Get-ItemProperty decorates every key with PSPath, + PSParentPath, PSChildName, PSDrive and PSProvider - counting those would make an + emptied key look like it still holds five entries. + + Tri-state on purpose (corrected in review before release): 0 for a key that is + absent or genuinely empty, the count for a readable key, and $null when the key is + there but cannot be read. The first draft returned 0 for unreadable too, which + recreated the very bug it was written to fix: a delete that failed, followed by an + unreadable after-read, would have counted as 0 and been reported as cleared. + .OUTPUTS + [int] the number of values, or $null when the key exists but cannot be read + #> + param([Parameter(Mandatory)][string]$Key) + + try { + $props = Get-ItemProperty -LiteralPath $Key -ErrorAction Stop + } catch [System.Management.Automation.ItemNotFoundException] { + return 0 # not there at all - nothing to clear, and nothing to worry about + } catch { + return $null # there, but unreadable: refuse to answer rather than answer 0 + } + + if (-not $props) { return 0 } + + return @($props.PSObject.Properties | Where-Object { + $_.Name -notin 'PSPath', 'PSParentPath', 'PSChildName', 'PSDrive', 'PSProvider' + }).Count +} + function Test-PathProtected { <# .SYNOPSIS - Checks whether a path is a protected root itself (v2.17: normalized) + Checks whether a path must be refused as a bulk-cleanup root (v2.17: normalized, + v2.20: link-aware) .DESCRIPTION Guards the roots listed in $script:ProtectedPaths against being emptied. Paths are resolved with GetFullPath first, otherwise the check is trivially @@ -1303,8 +1538,25 @@ function Test-PathProtected { Only the roots themselves are protected, not everything below them: the script legitimately cleans %SystemRoot%\Temp and other subfolders. Callers that must never touch a subtree pass explicit paths instead. + + v2.20: the checks above compare TEXT, and GetFullPath does not resolve reparse + points (measured, not assumed). A junction whose visible path looks innocent can + therefore point at a protected root, and enumerating that junction lists the + TARGET's children - deleting them deletes the real files. So an existing link is + resolved to its final target and the same rules are applied to that. + + Only the cleanup ROOT needs this. Links found deeper in the tree are already + harmless: Get-ChildItem -Recurse does not descend into a reparse point, and + Remove-Item on a junction removes the link and leaves the target intact (both + measured on a live filesystem). + .PARAMETER SkipLinkResolution + Internal. Set when re-checking an already-resolved target so a pathological link + chain cannot recurse. #> - param([string]$Path) + param( + [string]$Path, + [switch]$SkipLinkResolution + ) if ([string]::IsNullOrWhiteSpace($Path)) { return $true } # nothing sane to clean @@ -1337,6 +1589,50 @@ function Test-PathProtected { return $true } } + + # v2.20: resolve a link root and re-check the real target (see .DESCRIPTION). + # A path that is not there answers $false - there is nothing to delete through it, and + # callers probe optional locations constantly. A path that EXISTS but cannot be + # examined answers $true; the two are decided separately below. + # + # (This comment described the opposite of the code until review caught it: the first + # draft lumped "cannot be inspected" in with "does not exist". A comment asserting a + # safety property the code does not have is how the fail-open bootstrap shipped in + # v2.17, and it ended up copied into SECURITY.md.) + if (-not $SkipLinkResolution) { + # A path that cannot be inspected is not the same as a path that is not there. + # Access denied, an I/O error, a path too long: none of them mean "safe to empty", + # and collapsing them all to "not protected" is fail-open (raised in review). + # Same shape as Get-FolderSizeChecked: not-found is an answer, anything else is not. + try { + $null = Get-Item -LiteralPath $fullPath -Force -ErrorAction Stop + } catch [System.Management.Automation.ItemNotFoundException] { + return $false # nothing there to clean through + } catch [System.IO.DirectoryNotFoundException] { + return $false + } catch [System.IO.FileNotFoundException] { + return $false + } catch [System.Management.Automation.DriveNotFoundException] { + # An unmapped or removed drive is a not-found answer too (raised in review). + # Without this it fell to the refuse arm below, and every cleanup target on a + # removable drive became a "Protected path skipped" WARNING - noise in exactly + # the channel this release uses as its silent-failure alarm. + return $false + } catch { + return $true # exists in some form but cannot be examined - refuse + } + + $resolved = Resolve-PathThroughLinks -Path $fullPath + if (-not $resolved) { + # A link that cannot be resolved: the real target is unknown, so protection + # cannot be verified. Refuse rather than guess. + return $true + } + if ($resolved -ne $fullPath) { + return (Test-PathProtected -Path $resolved -SkipLinkResolution) + } + } + return $false } @@ -1693,6 +1989,7 @@ function New-SystemRestorePoint { Set-RunMarker -Phase 'RestorePointFrequencyOverride' -Data @{ PreviousValue = $prevFreqOuter } $childKilled = $false + $childExited = $true # only meaningful once a kill has been attempted try { $proc = Start-Process -FilePath 'powershell.exe' ` -ArgumentList @('-NoProfile', '-NoLogo', '-ExecutionPolicy', 'Bypass', '-EncodedCommand', $encodedCommand) ` @@ -1702,6 +1999,13 @@ function New-SystemRestorePoint { if (-not $proc.WaitForExit($timeoutMs)) { $proc.Kill($true) $childKilled = $true + # v2.20, corrected in review: Kill returns once termination has been + # REQUESTED, not once the tree is gone. Without this wait the finally + # below could read the creation frequency while the dying child was still + # writing 0 into it, see a non-zero value, conclude there was nothing to + # repair and delete the marker - leaving the frequency pinned at 0 with no + # record of it, which is exactly the damage this mechanism exists for. + $childExited = $proc.WaitForExit(5000) throw "restore point creation timed out after $($timeoutMs / 1000) seconds" } @@ -1713,15 +2017,29 @@ function New-SystemRestorePoint { # is still in place - repair it here and now. Clearing the marker # unconditionally would throw away the record of exactly the damage this # mechanism exists for, so it survives whenever the repair did not. - if ($childKilled) { - if (Restore-RestorePointFrequency -PreviousValue $prevFreqOuter) { - Clear-RunMarker - } else { - Write-Log "Restore point child was killed and its registry override could not be undone - the next run will retry" -Level WARNING + # + # v2.20: this repair now runs on BOTH paths. A child that exits normally can + # still have failed its own finally (a transient registry error), and the + # parent then deleted the marker anyway - leaving the creation frequency + # pinned at 0 indefinitely with nothing left to make a later run retry. + # Restore-RestorePointFrequency is idempotent by design: it returns $true + # without touching anything when the value is no longer 0, so verifying on the + # normal path costs nothing and turns an assumption into a check. + if (Restore-RestorePointFrequency -PreviousValue $prevFreqOuter) { + if ($childKilled -and -not $childExited) { + # Raised in review: the wait above has a bound, and a child that + # outlives it can still write the override AFTER this check passed. + # Keeping the marker costs one repair attempt on the next run; + # clearing it here would lose the only record that anything happened. + Write-Log "Restore point child was killed but had not exited 5 seconds later - the marker is kept, because it can still re-apply the override after this check" -Level WARNING $script:Stats.WarningsCount++ + } else { + Clear-RunMarker } } else { - Clear-RunMarker + $how = if ($childKilled) { 'was killed' } else { 'exited normally' } + Write-Log "Restore point child $how but its registry override could not be undone - the marker is kept so the next run retries" -Level WARNING + $script:Stats.WarningsCount++ } } @@ -2014,12 +2332,43 @@ function Update-Applications { if (-not $ReportOnly) { Write-Log "Updating winget sources..." -Level INFO # Run with timeout to prevent hanging - $job = Start-Job -ScriptBlock { param($path) & $path source update 2>&1 } -ArgumentList $wingetPath + # v2.20: the job's own exit code is returned as well. Only completion was + # checked before, and a completed job is not a successful winget: a corrupt + # source fails, the job still reaches Completed, and the upgrade list below + # was then built from stale data without a word in the log. + $job = Start-Job -ScriptBlock { + param($path) + & $path source update 2>&1 | Out-String + $LASTEXITCODE + } -ArgumentList $wingetPath $completed = $job | Wait-Job -Timeout 120 # 2 minutes timeout if (-not $completed) { $job | Stop-Job Write-Log "Winget source update timed out - package list may be stale" -Level WARNING $script:Stats.WarningsCount++ + } else { + # v2.20, corrected in review: "no usable result" is a failure in its own + # right. When the winget entry is a WindowsApps AppExecLink stub - passes + # Test-Path, but will not start once App Installer is deregistered - the + # job still reaches Completed, Receive-Job swallows the error and + # $LASTEXITCODE is never set, so the last output element is $null. The old + # guard short-circuited on exactly that and said nothing, which made an + # unusable winget the one silent path left in this block. Measured by the + # reviewer: one output element, and it was $null. + # [int] on a non-numeric last line would also have thrown; TryParse cannot. + $jobState = $job.State + $jobOutput = @($job | Receive-Job -ErrorAction SilentlyContinue) + $sourceExit = if ($jobOutput.Count -gt 0) { $jobOutput[-1] } else { $null } + $exitValue = 0 + $exitKnown = $null -ne $sourceExit -and [int]::TryParse([string]$sourceExit, [ref]$exitValue) + + if ($jobState -ne 'Completed' -or -not $exitKnown) { + Write-Log "Winget source update produced no usable exit code (job state: $jobState) - package list may be stale" -Level WARNING + $script:Stats.WarningsCount++ + } elseif ($exitValue -ne 0) { + Write-Log "Winget source update failed (exit code $exitValue) - package list may be stale" -Level WARNING + $script:Stats.WarningsCount++ + } } $job | Remove-Job -Force -ErrorAction SilentlyContinue } @@ -2305,9 +2654,6 @@ function Clear-BrowserCaches { # Get browser names for logging $browserNames = ($allPaths | Select-Object -ExpandProperty Browser -Unique) -join ', ' - # Measure size before cleanup - $sizeBefore = ($allPaths | ForEach-Object { Get-FolderSize -Path $_.Path } | Measure-Object -Sum).Sum - if ($ReportOnly) { # v2.18: $sizeBefore comes from Get-FolderSize, which returns 0 for BOTH an # empty cache and an unreadable one, so "Would clean ... - 0 B" announced a @@ -2325,6 +2671,15 @@ function Clear-BrowserCaches { } } } else { + # v2.20, corrected in review: both sides are now measured PER PATH with the + # SAME function. "Before" used Get-FolderSize (raw enumerator, inaccessible + # files silently skipped, reparse points excluded) while "after" used + # Get-FolderSizeChecked, so the two numbers did not describe the same set of + # files and a genuine deletion could be subtracted into the "nothing freed" + # branch. Measuring before only in this branch also stops ReportOnly from + # walking every cache twice. + $beforeMeasurements = @($allPaths | ForEach-Object { Get-FolderSizeChecked -Path $_.Path }) + # Actual cleanup $allPaths | ForEach-Object -Parallel { $item = $_ @@ -2339,11 +2694,38 @@ function Clear-BrowserCaches { } } -ThrottleLimit 8 - # Measure size after cleanup to get actual freed space - $sizeAfter = ($allPaths | ForEach-Object { Get-FolderSize -Path $_.Path } | Measure-Object -Sum).Sum - $sizeAfter = [long]($sizeAfter ?? 0) # Ensure non-null value - # Protect against negative values (can happen if browser recreates files during cleanup) - $freedSpace = [math]::Max(0, $sizeBefore - $sizeAfter) + # Measure size after cleanup to get actual freed space. + # + # v2.20: checked measurement. Get-FolderSize returns 0 both for "empty" and for + # "could not read", so an after-walk that lost access - the cache folder is + # being recreated by a browser that just started, ACLs changed mid-run - turned + # into "freed everything we measured before". The delta is only computed when + # every folder answered; otherwise the bytes stay unattributed instead of being + # invented. + $afterMeasurements = @($allPaths | ForEach-Object { Get-FolderSizeChecked -Path $_.Path }) + + # Pair the two sides path by path. The previous rule discarded the delta for + # ALL caches when a single one of ~30 could not be measured; only the paths + # that actually failed are dropped now. + $measuredBefore = [long]0 + $measuredAfter = [long]0 + $afterUnmeasured = 0 + for ($i = 0; $i -lt $allPaths.Count; $i++) { + $beforeOne = $beforeMeasurements[$i] + $afterOne = $afterMeasurements[$i] + if ($null -eq $beforeOne -or $null -eq $afterOne) { + $afterUnmeasured++ + continue + } + $measuredBefore += $beforeOne + $measuredAfter += $afterOne + } + + # Clamped once over the total, not per path (raised in review). TotalFreedBytes + # means net space reclaimed, so clamping each path separately would report + # 100 MB when one cache shrank by 100 MB while another was recreated and grew + # by 80 MB - inventing 80 MB of "freed" space the disk never got back. + $freedSpace = [math]::Max(0, $measuredBefore - $measuredAfter) # Update statistics with actual freed space (not estimated) if ($freedSpace -gt 0) { @@ -2353,6 +2735,11 @@ function Clear-BrowserCaches { } $script:Stats.FreedByCategory["Browser"] += $freedSpace Write-Log "Browser caches cleaned ($browserNames) - $(Format-FileSize $freedSpace)" -Level SUCCESS + if ($afterUnmeasured -gt 0) { + Write-Log "Browser caches: $afterUnmeasured folder(s) could not be measured - their share is not included above" -Level DETAIL + } + } elseif ($afterUnmeasured -gt 0) { + Write-Log "Browser caches ($browserNames): cleaned, but $afterUnmeasured folder(s) could not be measured - freed space not counted" -Level DETAIL } else { # v2.16: was logged as SUCCESS. A running browser locks its cache, so # "cleaned" with zero freed was a plain lie to the user @@ -2692,8 +3079,23 @@ function Clear-SystemCaches { # Nothing to measure: say so instead of claiming a clean-up happened (v2.16) Write-Log "Delivery Optimization: cmdlet ran, cache location not found - freed size unknown" -Level DETAIL } elseif ($doSizeBefore -gt 0) { - Write-Log "Delivery Optimization: nothing freed, $(Format-FileSize $doSizeBefore) still present" -Level WARNING - $script:Stats.WarningsCount++ + # v2.20: this was a WARNING and fired on healthy systems. The measurement + # covers the WHOLE Delivery Optimization folder, but the supported cmdlet + # only removes cached content - the service's own logs and state files stay + # and are not ours to delete. So "size did not change" after a cmdlet that + # reported success is not evidence of failure, it is evidence that the + # remainder was never cache. Reproduced on the EN stand VM twice in a row + # (2 MB left behind), where it pushed the run over its warning budget. + # + # A genuine failure still surfaces: the cmdlet runs with -ErrorAction Stop, + # so an actual error lands in the catch below as a warning. + # Deliberately does NOT claim the cache was cleared: an unchanged folder + # size cannot prove that every remaining byte is non-cache. It states what + # is actually known - the cmdlet reported success and this much is still + # on disk - and leaves the reader to judge (tightened in review; the first + # wording asserted "cache cleared", which is the opposite failure of the + # warning it replaced). + Write-Log "Delivery Optimization: cmdlet reported success, $(Format-FileSize $doSizeBefore) still on disk (the folder also holds service logs and state, which it does not remove)" -Level DETAIL } else { Write-Log "Delivery Optimization cache was already empty" -Level DETAIL } @@ -2732,7 +3134,17 @@ function Clear-EventLogs { # Enumerate only logs worth clearing: enabled, non-empty, Administrative/Operational. # This skips ~1000 Analytic/Debug/empty channels - much faster and avoids # chronic partial-failure warnings (v2.14; was: wevtutil el over all channels) - $logs = Get-WinEvent -ListLog * -ErrorAction SilentlyContinue | Where-Object { + # v2.20: keep the enumeration errors. A wholesale failure (Event Log service down, + # WMI broken) produced an empty list, zero failed clears and therefore the success + # branch: "Event logs cleared (0 logs)" while nothing was touched. + # v2.20, corrected in review: the enumeration result is kept BEFORE filtering. + # Deciding on the filtered list meant that 40 readable channels out of 510 with + # 470 enumeration errors produced a plain "Event logs cleared (40 logs)" SUCCESS, + # and that an empty filter result on a perfectly healthy machine was reported as + # a failed enumeration. Those are different states and now read differently. + $enumErrors = $null + $allLogs = @(Get-WinEvent -ListLog * -ErrorAction SilentlyContinue -ErrorVariable enumErrors) + $logs = $allLogs | Where-Object { $_.RecordCount -gt 0 -and $_.IsEnabled -and $_.LogName -ne 'Security' -and # Keep the main Security log (exact match) @@ -2750,11 +3162,37 @@ function Clear-EventLogs { } } - if ($failedCount -gt 0) { - Write-Log "Event logs cleared: $clearedCount, failed: $failedCount" -Level WARNING + $enumErrorCount = @($enumErrors).Count + + if ($allLogs.Count -eq 0) { + # Nothing could be listed at all - the Event Log service is down or the API + # is broken. Clearing zero channels is not a clean system. + Write-Log "Event logs: channels could not be enumerated ($enumErrorCount error(s)) - nothing was cleared" -Level WARNING $script:Stats.WarningsCount++ } else { - Write-Log "Event logs cleared ($clearedCount logs)" -Level SUCCESS + # Partial loss is reported separately from the clearing result, because the + # channels that failed to list were never even candidates and their records + # are still on disk. + # + # DETAIL, not WARNING (raised in review): a listing error is a gap in coverage, + # not the failure of an operation we claimed to perform, and this machine is + # not evidence about anyone else's - measured here on 25H2 it is 510 channels + # with zero errors, but third-party or protected channels can error out + # routinely elsewhere, and a warning that fires every run teaches people to + # ignore warnings. Total failure below stays a warning. Worded as errors rather + # than channels because that is what was counted. + if ($enumErrorCount -gt 0) { + Write-Log "Event logs: $enumErrorCount error(s) while listing channels - whatever they refer to was never considered for clearing" -Level DETAIL + } + + if ($failedCount -gt 0) { + Write-Log "Event logs cleared: $clearedCount, failed: $failedCount" -Level WARNING + $script:Stats.WarningsCount++ + } elseif ($clearedCount -eq 0) { + Write-Log "Event logs: no channel needed clearing" -Level DETAIL + } else { + Write-Log "Event logs cleared ($clearedCount logs)" -Level SUCCESS + } } } catch { Write-Log "Error clearing event logs: $_" -Level WARNING @@ -2819,58 +3257,67 @@ function Clear-PrivacyTraces { $clearedItems = @() - # Clear Run dialog history (RunMRU) - $runMruKey = "HKCU:\Software\Microsoft\Windows\CurrentVersion\Explorer\RunMRU" - if (Test-Path $runMruKey) { - try { - # Get current values before clearing - $mruValues = Get-ItemProperty -Path $runMruKey -ErrorAction SilentlyContinue - $valueCount = ($mruValues.PSObject.Properties | Where-Object { $_.Name -match '^[a-z]$' }).Count - - # Remove the key and recreate it empty - Remove-Item -Path $runMruKey -Force -ErrorAction SilentlyContinue - New-Item -Path $runMruKey -Force -ErrorAction SilentlyContinue | Out-Null - - if ($valueCount -gt 0) { - $clearedItems += "Run history ($valueCount entries)" - } - } catch { - Write-Log "Could not clear Run history: $_" -Level WARNING + # v2.20: success is now confirmed by looking, not by the absence of an exception. + # Remove-Item with -ErrorAction SilentlyContinue cannot throw, so the catch blocks + # here were dead code and "$clearedItems += ..." ran unconditionally: the log said + # "Privacy traces cleared: Explorer typed paths" even when policy or permissions had + # rejected the deletion and the key was still sitting there, fully populated. + # RunMRU joins this list rather than keeping its own copy of the same logic: it used + # the pre-count as proof of success in exactly the way the others did (caught in + # review before release, when the first version of this fix left it behind). + $historyKeys = @( + @{ Path = "HKCU:\Software\Microsoft\Windows\CurrentVersion\Explorer\RunMRU"; Label = 'Run history' } + @{ Path = "HKCU:\Software\Microsoft\Windows\CurrentVersion\Explorer\TypedPaths"; Label = 'Explorer typed paths' } + @{ Path = "HKCU:\Software\Microsoft\Windows\CurrentVersion\Explorer\WordWheelQuery"; Label = 'Explorer search history' } + ) + foreach ($entry in $historyKeys) { + if (-not (Test-Path $entry.Path)) { continue } + + $before = Get-RegistryValueCount -Key $entry.Path + if ($null -eq $before) { + # Present but unreadable: we cannot tell whether there is anything to clear, + # and silently moving on would look identical to "there was nothing" + Write-Log "$($entry.Label): key could not be read - left untouched" -Level WARNING + $script:Stats.WarningsCount++ + continue } - } + # An empty key is not a trace that was cleared - it is a trace that was not there + if ($before -eq 0) { continue } - # Clear TypedPaths (Explorer address bar history) - $typedPathsKey = "HKCU:\Software\Microsoft\Windows\CurrentVersion\Explorer\TypedPaths" - if (Test-Path $typedPathsKey) { - try { - Remove-Item -Path $typedPathsKey -Force -ErrorAction SilentlyContinue - New-Item -Path $typedPathsKey -Force -ErrorAction SilentlyContinue | Out-Null - $clearedItems += "Explorer typed paths" - } catch { } - } + Remove-Item -Path $entry.Path -Force -ErrorAction SilentlyContinue + New-Item -Path $entry.Path -Force -ErrorAction SilentlyContinue | Out-Null - # Clear WordWheelQuery (Explorer search history) - $searchKey = "HKCU:\Software\Microsoft\Windows\CurrentVersion\Explorer\WordWheelQuery" - if (Test-Path $searchKey) { - try { - Remove-Item -Path $searchKey -Force -ErrorAction SilentlyContinue - New-Item -Path $searchKey -Force -ErrorAction SilentlyContinue | Out-Null - $clearedItems += "Explorer search history" - } catch { } + $after = Get-RegistryValueCount -Key $entry.Path + if ($null -eq $after) { + Write-Log "$($entry.Label): result could not be verified - not counted as cleared" -Level WARNING + $script:Stats.WarningsCount++ + } elseif ($after -eq 0) { + $clearedItems += $entry.Label + } else { + Write-Log "$($entry.Label): $after of $before entries remain - not cleared" -Level WARNING + $script:Stats.WarningsCount++ + } } # Clear Recent documents folder $recentFolder = [Environment]::GetFolderPath('Recent') - if (Test-Path $recentFolder) { - try { - $recentCount = (Get-ChildItem -Path $recentFolder -Force -ErrorAction SilentlyContinue).Count - Get-ChildItem -Path $recentFolder -Force -ErrorAction SilentlyContinue | ForEach-Object { + if (-not [string]::IsNullOrWhiteSpace($recentFolder) -and (Test-Path $recentFolder)) { + $recentBefore = @(Get-ChildItem -LiteralPath $recentFolder -Force -ErrorAction SilentlyContinue).Count + if ($recentBefore -gt 0) { + Get-ChildItem -LiteralPath $recentFolder -Force -ErrorAction SilentlyContinue | ForEach-Object { Remove-Item -LiteralPath $_.FullName -Recurse -Force -ErrorAction SilentlyContinue } - if ($recentCount -gt 0) { - $clearedItems += "Recent documents ($recentCount items)" + # Report what actually went, not what was there before the attempt + $recentAfter = @(Get-ChildItem -LiteralPath $recentFolder -Force -ErrorAction SilentlyContinue).Count + $removed = $recentBefore - $recentAfter + if ($removed -gt 0) { + $clearedItems += "Recent documents ($removed items)" } - } catch { } + if ($recentAfter -gt 0) { + Write-Log "Recent documents: $recentAfter of $recentBefore items could not be removed (in use?)" -Level WARNING + $script:Stats.WarningsCount++ + } + } } if ($clearedItems.Count -gt 0) { @@ -3070,7 +3517,11 @@ function Clear-DeveloperCaches { .SYNOPSIS Cleans developer tool caches (npm, pip, nuget, composer, etc.) #> - if ($SkipDevCleanup) { + # v2.20: matches the contract the dispatcher enforces (-SkipCleanup suppresses the + # whole cleanup group). Unreachable through Start-WinClean, which already gates the + # phase - this is the guard a direct caller of the dot-sourced function gets, and it + # disagreed with the documented meaning of -SkipCleanup. + if ($SkipCleanup -or $SkipDevCleanup) { Write-Log "Developer cache cleanup skipped (parameter)" -Level INFO return } @@ -3091,20 +3542,51 @@ function Clear-DeveloperCaches { $npm = Get-Command npm -ErrorAction SilentlyContinue if ($npm) { try { - $sizeBefore = Get-FolderSize $npmCache + # v2.20, corrected in review: both sides are measured with the checked + # variant. Get-FolderSize answers 0 for "empty" and for "could not + # read" alike, so a cache that became unreadable after the clean - npm + # recreating it, ACLs changing mid-run - produced "freed everything we + # measured before" and put invented bytes into TotalFreedBytes. The + # same defect was fixed for browser caches in this release; npm kept it. + $sizeBefore = Get-FolderSizeChecked -Path $npmCache & npm cache clean --force 2>&1 | Out-Null - $sizeAfter = Get-FolderSize $npmCache - $freed = $sizeBefore - $sizeAfter - + # v2.20: npm fails without throwing (EPERM on a locked cache is the + # common case). The exit code was never read, so a failed clean that + # freed nothing landed in the "else" branch below and was logged as + # SUCCESS - the same lie fixed for browser caches in v2.16. + $npmExit = $LASTEXITCODE + $sizeAfter = Get-FolderSizeChecked -Path $npmCache + $npmMeasured = ($null -ne $sizeBefore -and $null -ne $sizeAfter) + $freed = if ($npmMeasured) { [math]::Max(0, $sizeBefore - $sizeAfter) } else { 0 } + + # Credit whatever really went, whether or not npm then failed if ($freed -gt 0) { $script:Stats.TotalFreedBytes += $freed if (-not $script:Stats.FreedByCategory.ContainsKey("Developer")) { $script:Stats.FreedByCategory["Developer"] = 0 } $script:Stats.FreedByCategory["Developer"] += $freed + } + + # v2.20, corrected in review: the exit code is checked FIRST. Testing + # "$freed -gt 0" first meant a partial failure (npm removes 400 MB, then + # hits EPERM on a locked file and exits 1) reported plain success and + # skipped the fallback - the exit code was read and then ignored in + # exactly the case where it mattered. + if ($npmExit -ne 0) { + Write-Log "npm cache clean failed (exit code $npmExit)$(if ($freed -gt 0) { " after freeing $(Format-FileSize $freed)" }) - removing the cache directly" -Level WARNING + $script:Stats.WarningsCount++ + Remove-FolderContent -Path $npmCache -Category "Developer" -Description "npm cache" + } elseif ($freed -gt 0) { Write-Log "npm cache cleaned - $(Format-FileSize $freed)" -Level SUCCESS + } elseif (-not $npmMeasured) { + # Cleaned, but "how much" has no honest answer - say that instead + # of calling an unreadable cache an empty one. + Write-Log "npm cache cleaned, but its size could not be measured - freed space not counted" -Level DETAIL } else { - Write-Log "npm cache cleaned (via npm)" -Level SUCCESS + # npm succeeded and there was nothing to reclaim. Not a cleanup + # success worth announcing, and definitely not freed bytes. + Write-Log "npm cache was already empty" -Level DETAIL } } catch { Remove-FolderContent -Path $npmCache -Category "Developer" -Description "npm cache" @@ -3237,7 +3719,8 @@ function Clear-DockerWSL { .SYNOPSIS Cleans Docker images, containers, and WSL2 disk #> - if ($SkipDockerCleanup) { + # v2.20: see Clear-DeveloperCaches - same contract, same reason + if ($SkipCleanup -or $SkipDockerCleanup) { Write-Log "Docker/WSL cleanup skipped (parameter)" -Level INFO return } @@ -3423,7 +3906,8 @@ function Clear-VisualStudio { .SYNOPSIS Cleans Visual Studio caches and temporary files #> - if ($SkipVSCleanup) { + # v2.20: see Clear-DeveloperCaches - same contract, same reason + if ($SkipCleanup -or $SkipVSCleanup) { Write-Log "Visual Studio cleanup skipped (parameter)" -Level INFO return } @@ -4011,6 +4495,155 @@ function Invoke-DISMCleanup { } } +function Select-StorageSenseTask { + <# + .SYNOPSIS + Picks the single Storage Sense task out of a lookup result + .DESCRIPTION + Pure decision, split out in v2.20 so the rule can be tested without a scheduler. + Windows ships exactly one task with this name; silently taking the first of + several means starting something nobody identified, so ambiguity yields no task + and says why. + Returns Task (the task or $null) and Reason ('ok' | 'none' | 'ambiguous'). + #> + param([object[]]$Tasks) + + $found = @($Tasks | Where-Object { $null -ne $_ }) + if ($found.Count -eq 0) { return @{ Task = $null; Reason = 'none' } } + if ($found.Count -gt 1) { return @{ Task = $null; Reason = 'ambiguous' } } + return @{ Task = $found[0]; Reason = 'ok' } +} + +function Get-StorageSenseVerdict { + <# + .SYNOPSIS + Decides whether a Storage Sense run counts as a cleanup that actually happened + .DESCRIPTION + Pure decision, split out in v2.20 so the rule can be tested. Two things must both + hold before Disk Cleanup is skipped: the task reported success, AND free space + actually grew. + A result of 0 on its own is not evidence. Storage Sense obeys its own settings; + switched off in Settings, the task still starts, does nothing it is not allowed to + do, and exits 0. Skipping all 23 cleanmgr handlers on that basis would free + nothing and report success - the defect this release exists to remove, and one + that only became reachable once this branch stopped being dead code. + $null TaskResult means "could not be read" and $null FreedBytes means "could not + be measured"; neither may be read as success. + Returns Done ($true only when cleanmgr may be skipped) and Reason + ('unreadable' | 'failed' | 'not-measured' | 'nothing-freed' | 'success'). + #> + param( + [AllowNull()][object]$TaskResult, + [AllowNull()][object]$FreedBytes + ) + + if ($null -eq $TaskResult) { return @{ Done = $false; Reason = 'unreadable' } } + + # [int] would THROW here, and precisely on the codes this function exists to catch. + # LastTaskResult is a UInt32 and every HRESULT failure has the high bit set, so its + # unsigned value exceeds Int32.MaxValue: the 0x80040154 cited above arrives as + # 2147746132. The exception escaped Invoke-StorageSense, Invoke-Phase recorded + # DeepSystemCleanup as failed, and Clear-WindowsOld never ran. + # The first test written for this passed the PowerShell literal 0x80040154, which the + # parser types as Int32 -2147221164 - so the cast succeeded and the test was green + # BECAUSE of the defect. Tests for this function must use the production type. + $resultValue = [long]0 + if (-not [long]::TryParse([string]$TaskResult, [ref]$resultValue)) { + return @{ Done = $false; Reason = 'unreadable' } + } + if ($resultValue -ne 0) { return @{ Done = $false; Reason = 'failed' } } + if ($null -eq $FreedBytes) { return @{ Done = $false; Reason = 'not-measured' } } + if ([long]$FreedBytes -le 0) { return @{ Done = $false; Reason = 'nothing-freed' } } + return @{ Done = $true; Reason = 'success' } +} + +function Wait-StorageSenseTask { + <# + .SYNOPSIS + Waits for the Storage Sense task to stop running + .DESCRIPTION + Split out in v2.20 so the wait can be tested without a scheduler and without + actually waiting two minutes: the caller injects how to read the task, how to + read its info, and how to wait. + + 'vanished' is a distinct outcome because the loop used to break on a task that + disappeared while leaving its finished flag false, so the caller announced "did + not finish within 120 seconds" after five - a number that never happened. + + Returns Outcome ('finished' | 'vanished' | 'timeout' | 'unverifiable'), Elapsed + seconds and the last Task seen. 'unverifiable' means the task was never observed + running AND its previous run time could not be read before the start, so there is + nothing to compare against - it is not a failure, but it is not evidence of a run + either, and the caller must not treat it as one. + #> + param( + [scriptblock]$GetTask, + [scriptblock]$GetTaskInfo, + [AllowNull()][object]$LastRunBefore, + [int]$TimeoutSeconds = 120, + [int]$CheckInterval = 5, + [scriptblock]$Wait = { param($seconds) Start-Sleep -Seconds $seconds } + ) + + $elapsed = 0 + $wasRunning = $false + $task = $null + + while ($elapsed -lt $TimeoutSeconds) { + & $Wait $CheckInterval + $elapsed += $CheckInterval + + # Task states are language-independent: Ready, Running, Disabled + $task = & $GetTask + if (-not $task) { + return @{ Outcome = 'vanished'; Elapsed = $elapsed; Task = $null } + } + + if ($task.State -eq 'Running') { + $wasRunning = $true + continue + } + + if ($wasRunning) { + # Was running and is not any more + return @{ Outcome = 'finished'; Elapsed = $elapsed; Task = $task } + } + + if ($elapsed -ge 10) { + # Never observed as Running - it may simply have been quicker than the poll + # interval. The task's own LastRunTime moving is the evidence. + # + # Raised in review: without a baseline there is no evidence to weigh, and the + # old disjunction let the MISSING baseline satisfy the test, because -not $null + # is true. Any readable task info then returned "finished" for a task that may + # never have started - a false success that goes on to skip all 23 cleanmgr + # handlers. + # + # Corrected again in the next review round: the first repair returned here at + # once, which gave a slow-starting task no chance to be seen and let cleanmgr + # start alongside it. Keep watching instead - being observed Running is direct + # evidence and needs no baseline - and only answer "unverifiable" if the whole + # window passes without ever seeing it. + if ($null -ne $LastRunBefore) { + $infoNow = & $GetTaskInfo $task + if ($infoNow -and $infoNow.LastRunTime -ne $LastRunBefore) { + return @{ Outcome = 'finished'; Elapsed = $elapsed; Task = $task } + } + } + } + } + + # Never seen running, and there was no previous run time to compare against: the window + # is over and nothing about this invocation was ever observed. That is not a timeout in + # the useful sense - there is simply no evidence either way, and the caller must not + # read it as one. + if (-not $wasRunning -and $null -eq $LastRunBefore) { + return @{ Outcome = 'unverifiable'; Elapsed = $elapsed; Task = $task } + } + + return @{ Outcome = 'timeout'; Elapsed = $elapsed; Task = $task } +} + function Invoke-StorageSense { <# .SYNOPSIS @@ -4018,79 +4651,211 @@ function Invoke-StorageSense { #> Write-Log "Storage Sense" -Level SECTION + # v2.20: the one step users asked to be able to switch off on its own. Until now the + # only way was -SkipCleanup, which also suppresses temp files, browsers, dev caches, + # Docker/WSL, Visual Studio and the driver store - everything, to avoid one step. if ($ReportOnly) { Write-Log "Would run: Storage Sense" -Level DETAIL return } + # cleanmgr's saved selection lives under this sageset. Both are needed up front: the + # sweep below runs on every path that is allowed to change the system, not only when + # cleanmgr is reached. (-ReportOnly returns above because it promises to change + # nothing at all; -SkipDiskCleanup returns AFTER the sweep, see below.) + $sageset = 9999 + $regPath = "HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\VolumeCaches" + + # Sweep leftovers from a previous run before doing anything else. The cleanmgr branch + # deliberately skips its own sweep while cleanmgr is still running (it would pull the + # configuration out from under it), and Storage Sense returns before that branch is + # reached - so a timed-out run followed only by successful Storage Sense runs would + # otherwise leave these flags in the registry forever. Caught in review before release. + # + # Gated on cleanmgr not running: a previous run's cleanmgr may still be working in the + # background, and sweeping now would pull its configuration out from under it - which + # is precisely what the finally below refuses to do (also raised in review). + if (-not (Get-Process -Name 'cleanmgr' -ErrorAction SilentlyContinue)) { + Get-ChildItem -Path $regPath -ErrorAction SilentlyContinue | ForEach-Object { + Remove-ItemProperty -Path $_.PSPath -Name "StateFlags$sageset" -Force -ErrorAction SilentlyContinue + } + } + + # After the sweep, deliberately (raised in review). The flag means "do not run this + # step", not "do not touch anything": returning before the sweep left a timed-out + # previous run's StateFlags in the registry forever, which is the exact case the sweep + # was added for. + if ($SkipDiskCleanup) { + Write-Log "Storage Sense / Disk Cleanup skipped (parameter)" -Level INFO + return + } + # Try Storage Sense first (Windows 11) - # Use Get-ScheduledTask for language-independent status checking - $ssTaskPath = "\Microsoft\Windows\DiskCleanup\" + # + # v2.20: the task was looked up at "\Microsoft\Windows\DiskCleanup\", where it does not + # exist - that folder holds SilentCleanup. The real one lives under + # "\Microsoft\Windows\DiskFootprint\". So this branch was UNREACHABLE on every machine + # and every run fell through to the legacy cleanmgr path: 10 seconds on a fresh VM, + # 15 minutes on a real workstation (measured: 901s of a 1101s run, and it did not even + # finish). Searching by name instead of by a hardcoded path also survives the folder + # moving again. $ssTaskName = "StorageSense" - $task = Get-ScheduledTask -TaskPath $ssTaskPath -TaskName $ssTaskName -ErrorAction SilentlyContinue + $ssTasks = @(Get-ScheduledTask -TaskName $ssTaskName -ErrorAction SilentlyContinue) + # Windows ships exactly one. If a machine somehow has more, say so and take none: + # silently picking the first of several tasks with the same name means starting + # something nobody identified (raised in review). + $ssSelection = Select-StorageSenseTask -Tasks $ssTasks + + # Set by every branch that has already said why cleanmgr is being used, so the + # fallback below cannot contradict it. Raised in review: an ambiguous lookup logged + # both "2 tasks with that name - not guessing" and, two lines later, "task not found". + $ssExplained = $false + + if ($ssSelection.Reason -eq 'ambiguous') { + $ssExplained = $true + Write-Log "Storage Sense: $($ssTasks.Count) tasks with that name ($(($ssTasks | ForEach-Object { $_.TaskPath }) -join ', ')) - not guessing, using Disk Cleanup" -Level INFO + } + $task = $ssSelection.Task + + # v2.20, corrected in review: pin the exact task. Later lookups searched by name only + # and took the first hit, which quietly undid the refusal-to-guess rule above the + # moment a second same-named task appeared while we were waiting. + $ssTaskPath = if ($task) { $task.TaskPath } else { $null } + + # Raised in review, then measured: -TaskPath rejects $null with a binding error that + # -ErrorAction SilentlyContinue does NOT suppress, so a task object without a path + # would turn every later lookup into an exception - and the wait would read that as + # "the task vanished". The path is only passed when there is one. + $ssLookup = @{ TaskName = $ssTaskName; ErrorAction = 'SilentlyContinue' } + if ($ssTaskPath) { $ssLookup['TaskPath'] = $ssTaskPath } + + # Verified below; only a task that demonstrably ran successfully skips cleanmgr + $storageSenseDone = $false # A disabled task cannot be started - fall back to cleanmgr instead of # waiting the full timeout for a task that never runs (v2.14) if ($task -and $task.State -ne 'Disabled') { Write-Log "Running Storage Sense..." -Level INFO - # Record time before running to compare with LastRunTime - $startTime = Get-Date - Start-ScheduledTask -TaskPath $ssTaskPath -TaskName $ssTaskName -ErrorAction SilentlyContinue - - # Wait for task to complete with timeout - $timeout = 120 # 2 minutes max - $elapsed = 0 - $checkInterval = 5 - $wasRunning = $false - - while ($elapsed -lt $timeout) { - Start-Sleep -Seconds $checkInterval - $elapsed += $checkInterval - - # Get current task state (language-independent: Ready, Running, Disabled) - $task = Get-ScheduledTask -TaskPath $ssTaskPath -TaskName $ssTaskName -ErrorAction SilentlyContinue - if ($task) { - $state = $task.State - - if ($state -eq 'Running') { - $wasRunning = $true - } elseif ($wasRunning -and $state -eq 'Ready') { - # Task was running and now finished - Write-Log "Storage Sense completed" -Level SUCCESS - break - } elseif (-not $wasRunning -and $elapsed -ge 10) { - # Task didn't start running within 10 seconds - check LastRunTime - $taskInfo = Get-ScheduledTaskInfo -TaskPath $ssTaskPath -TaskName $ssTaskName -ErrorAction SilentlyContinue - if ($taskInfo -and $taskInfo.LastRunTime -gt $startTime) { - Write-Log "Storage Sense completed" -Level SUCCESS - break + # v2.20: compare against the task's OWN previous run time, not wall-clock. The old + # code compared LastRunTime with $startTime taken in this process, which is a + # different clock granularity and rounds the wrong way. + $infoBefore = $task | Get-ScheduledTaskInfo -ErrorAction SilentlyContinue + $lastRunBefore = if ($infoBefore) { $infoBefore.LastRunTime } else { $null } + + # Free space before the task, so its success can be judged on evidence rather than + # on an exit code (see the verification below) + $sysDriveLetter = ($env:SystemDrive).TrimEnd(':') + $freeBefore = $null + try { $freeBefore = (Get-PSDrive -Name $sysDriveLetter -ErrorAction Stop).Free } catch { $freeBefore = $null } + + $started = $false + try { + $task | Start-ScheduledTask -ErrorAction Stop + $started = $true + } catch { + # v2.20: the start used to be -ErrorAction SilentlyContinue with the result + # ignored, so a task that never started still cost the full 120s wait before + # anything was reported + Write-Log "Storage Sense could not be started ($($_.Exception.Message)) - using Disk Cleanup" -Level INFO + $ssExplained = $true + } + + if ($started) { + $timeout = 120 # 2 minutes max + $waitResult = Wait-StorageSenseTask ` + -GetTask { @(Get-ScheduledTask @ssLookup) | Select-Object -First 1 } ` + -GetTaskInfo { param($t) $t | Get-ScheduledTaskInfo -ErrorAction SilentlyContinue } ` + -LastRunBefore $lastRunBefore ` + -TimeoutSeconds $timeout ` + -CheckInterval 5 + # Only overwrite when the wait actually saw a task: on 'vanished' the original + # selection is kept, so the fallback below does not also announce "task not + # found" for something that was found and then disappeared. + if ($waitResult.Task) { $task = $waitResult.Task } + + if ($waitResult.Outcome -eq 'finished') { + # v2.20 fail-closed: a task that ran and FAILED used to be logged as + # "Storage Sense completed" and cleanmgr was skipped - a silent failure that + # would have left the machine uncleaned while the run reported success. + # Verified on a live machine where this task returns 0x80040154. + $infoAfter = $task | Get-ScheduledTaskInfo -ErrorAction SilentlyContinue + $taskResult = if ($infoAfter) { $infoAfter.LastTaskResult } else { $null } + + # $null means "could not be measured", which is not the same answer as + # "freed nothing" - the old code collapsed both into 0. + $freedBySense = $null + try { + $freeAfter = (Get-PSDrive -Name $sysDriveLetter -ErrorAction Stop).Free + if ($null -ne $freeBefore) { $freedBySense = $freeAfter - $freeBefore } + } catch { $freedBySense = $null } + + $verdict = Get-StorageSenseVerdict -TaskResult $taskResult -FreedBytes $freedBySense + $storageSenseDone = $verdict.Done + + switch ($verdict.Reason) { + 'success' { Write-Log "Storage Sense completed - $(Format-FileSize $freedBySense)" -Level SUCCESS } + 'unreadable' { Write-Log "Storage Sense ran but its result could not be read - using Disk Cleanup as well" -Level INFO } + 'failed' { Write-Log ("Storage Sense failed (task result 0x{0:X8}) - using Disk Cleanup instead" -f $taskResult) -Level INFO } + 'not-measured' { Write-Log "Storage Sense reported success but free space could not be measured - running Disk Cleanup as well" -Level INFO } + 'nothing-freed' { Write-Log "Storage Sense reported success but freed nothing measurable - running Disk Cleanup as well" -Level INFO } + default { Write-Log "Storage Sense verdict '$($verdict.Reason)' not recognised - running Disk Cleanup as well" -Level INFO } + } + } elseif ($waitResult.Outcome -eq 'vanished') { + # v2.20, corrected in review: this path used to fall into the timeout + # message below, so a task that disappeared after five seconds was + # reported as having failed to finish within 120 - a number that never + # happened on that run. + Write-Log "Storage Sense task disappeared while being watched - using Disk Cleanup instead" -Level INFO + } elseif ($waitResult.Outcome -eq 'unverifiable') { + Write-Log "Storage Sense: its previous run time could not be read, so there is no way to tell whether this run happened - using Disk Cleanup instead" -Level INFO + } else { + # WARNING, not INFO (restored in review): falling back to cleanmgr is a + # fine outcome, but this particular one leaves a task we asked for and + # could not account for, and cleanmgr is about to start alongside it. The + # v2.20 draft downgraded this to INFO and dropped the counter, so a run + # where Storage Sense hung for two minutes printed COMPLETED SUCCESSFULLY + # in green. + Write-Log "Storage Sense did not finish within $timeout seconds - using Disk Cleanup instead" -Level WARNING + $script:Stats.WarningsCount++ + + $task = @(Get-ScheduledTask @ssLookup) | Select-Object -First 1 + if ($task -and $task.State -eq 'Running') { + # The stop used to be fire-and-forget with an unconditional "stopped" + # line after it (raised in review). Two cleaners deleting at once is + # exactly the state the free-space accounting cannot describe, so the + # claim is now made only when the task actually stopped. + $task | Stop-ScheduledTask -ErrorAction SilentlyContinue + $taskAfterStop = @(Get-ScheduledTask @ssLookup) | Select-Object -First 1 + if ($taskAfterStop -and $taskAfterStop.State -eq 'Running') { + Write-Log "Storage Sense task could not be stopped and is still running - Disk Cleanup will run alongside it, so the freed figures below cover both" -Level WARNING + $script:Stats.WarningsCount++ + } else { + Write-Log "Storage Sense task stopped" -Level INFO } } } + + # Every branch above has stated its own reason + $ssExplained = $true } + } - if ($elapsed -ge $timeout) { - Write-Log "Storage Sense timed out after $timeout seconds" -Level WARNING - # Force stop the task if still running - $task = Get-ScheduledTask -TaskPath $ssTaskPath -TaskName $ssTaskName -ErrorAction SilentlyContinue - if ($task -and $task.State -eq 'Running') { - Stop-ScheduledTask -TaskPath $ssTaskPath -TaskName $ssTaskName -ErrorAction SilentlyContinue - Write-Log "Storage Sense task stopped" -Level INFO + if (-not $storageSenseDone) { + # Fallback to cleanmgr. Every other reason for landing here (ambiguous lookup, + # start failed, task failed, timed out, vanished, unverifiable) has already said + # so in its own words and set $ssExplained, so only the two states that produce no + # message of their own are reported here. + if (-not $ssExplained) { + if (-not $task) { + Write-Log "Storage Sense task not found, using Disk Cleanup..." -Level INFO + } elseif ($task.State -eq 'Disabled') { + Write-Log "Storage Sense task is disabled, using Disk Cleanup..." -Level INFO } - $script:Stats.WarningsCount++ - } - } else { - # Fallback to cleanmgr - if ($task) { - Write-Log "Storage Sense task is disabled, using Disk Cleanup..." -Level INFO - } else { - Write-Log "Storage Sense task not found, using Disk Cleanup..." -Level INFO } - # Configure cleanup categories - $sageset = 9999 - $regPath = "HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\VolumeCaches" + # Configure cleanup categories ($sageset / $regPath are set at the top of the + # function, because the leftover sweep needs them on every path) # Note (v2.14): "Previous Installations" removed - Windows.old deletion must go # through Clear-WindowsOld which asks for user confirmation. @@ -4127,6 +4892,8 @@ function Invoke-StorageSense { "Windows Upgrade Log Files" ) + # Defined before the try so the finally can always ask whether it exists + $cleanmgr = $null try { # Set StateFlags for cleanup categories. # v2.16: count what was actually armed. Previously a failed write was @@ -4154,9 +4921,27 @@ function Invoke-StorageSense { return } - # Run cleanmgr with progress feedback and reasonable timeout - $cleanmgr = Start-Process -FilePath "cleanmgr.exe" -ArgumentList "/sagerun:$sageset" ` - -WindowStyle Hidden -PassThru + # Run cleanmgr with progress feedback and reasonable timeout. + # + # Raised in review, then measured: Start-Process on a missing or blocked + # executable leaves the variable $null, and $null.HasExited is also $null - so + # "-not $cleanmgr.HasExited" is TRUE and the loop below reported progress every + # minute for the full fifteen, then set DiskCleanupPending and warned that an + # elevated process was still deleting. All for a process that never started. + # Reachable on Server SKUs without Desktop Experience and under AppLocker/WDAC. + $cleanmgr = $null + try { + $cleanmgr = Start-Process -FilePath "cleanmgr.exe" -ArgumentList "/sagerun:$sageset" ` + -WindowStyle Hidden -PassThru -ErrorAction Stop + } catch { + $cleanmgr = $null + } + + if (-not $cleanmgr) { + Write-Log "Disk Cleanup could not be started (cleanmgr.exe is missing or blocked) - it cleaned nothing" -Level WARNING + $script:Stats.WarningsCount++ + return + } # v2.16: raised from 420s. cleanmgr regularly needs longer on a workstation # with a large component store, and killing it produced a warning on every @@ -4176,10 +4961,15 @@ function Invoke-StorageSense { } if (-not $cleanmgr.HasExited) { - # Let it finish in the background instead of killing it. Not a warning - # either: cleanmgr simply takes longer than we are willing to wait, and - # killing it mid-delete was both pointless and misreported as "continuing" (v2.16) - Write-Log "Disk Cleanup exceeded $maxWait seconds - leaving it to finish in the background" -Level INFO + # Still killing it would be worse - cleanmgr keeps working after a kill and + # the deletion is mid-flight (v2.16). But this is not an informational + # event either: everything measured after this point is partial, the run + # is about to print a total and write its JSON while an elevated process + # is still deleting, and the freed bytes it goes on to reclaim are counted + # by nobody. + $script:Stats.DiskCleanupPending = $true + Write-Log "Disk Cleanup exceeded $maxWait seconds and is still running - it continues in the background, so the freed figures below are partial" -Level WARNING + $script:Stats.WarningsCount++ } elseif ($cleanmgr.ExitCode -ne 0) { # v2.16: the exit code used to be ignored entirely, so a crash one second # in was still logged as a success @@ -4193,8 +4983,19 @@ function Invoke-StorageSense { # v2.16: sweep every handler, not just the ones from $categories - flags left # by an interrupted run or by an older version of this list stayed forever # (four such leftovers were found on a live machine). - Get-ChildItem -Path $regPath -ErrorAction SilentlyContinue | ForEach-Object { - Remove-ItemProperty -Path $_.PSPath -Name "StateFlags$sageset" -Force -ErrorAction SilentlyContinue + # + # v2.20: but NOT while cleanmgr is still running. The timeout branch above + # deliberately leaves it working in the background, and this sweep then pulled + # its configuration out from under it. Whether cleanmgr re-reads the flags per + # handler or only once at startup is not something to guess at while it holds + # an elevated deletion loop. The flags are swept by the next run's own sweep, + # which is exactly the leftover case v2.16 added it for. + if ($cleanmgr -and -not $cleanmgr.HasExited) { + Write-Log "Disk Cleanup is still running - its registry configuration will be swept by the next run" -Level DETAIL + } else { + Get-ChildItem -Path $regPath -ErrorAction SilentlyContinue | ForEach-Object { + Remove-ItemProperty -Path $_.PSPath -Name "StateFlags$sageset" -Force -ErrorAction SilentlyContinue + } } } } @@ -4450,6 +5251,7 @@ function Write-ResultJson { SkipDevCleanup = [bool]$SkipDevCleanup SkipDockerCleanup = [bool]$SkipDockerCleanup SkipVSCleanup = [bool]$SkipVSCleanup + SkipDiskCleanup = [bool]$SkipDiskCleanup DisableTelemetry = [bool]$DisableTelemetry } TotalFreedBytes = [long]$script:Stats.TotalFreedBytes @@ -4462,6 +5264,13 @@ function Write-ResultJson { WarningsCount = $script:Stats.WarningsCount ErrorsCount = $script:Stats.ErrorsCount RebootRequired = [bool]$script:Stats.RebootRequired + # v2.20: true when writing the log file failed at some point. The run still + # completed, but LogPath below points at an incomplete file - an automated + # consumer must not treat that log as the full record of what happened. + LoggingDegraded = [bool]$script:LogWriteFailed + # v2.20: true when Disk Cleanup outlived its timeout and was left running. + # TotalFreedBytes is then a lower bound, not the final figure. + DiskCleanupPending = [bool]$script:Stats.DiskCleanupPending # 'enabled' means cleanup figures are understated (Defender blocked some # deletions without reporting an error); 'unknown' means the check itself # failed, so the figures are unverified rather than confirmed good @@ -4487,10 +5296,13 @@ function Write-ResultJson { $result | ConvertTo-Json -Depth 4 | Out-File -FilePath $Path -Encoding utf8 Write-Log "Result JSON written: $Path" -Level INFO } catch { - # Must be loud: an automated stand reads this file, and a stale copy from the - # previous run would be reported as a successful fresh run - Write-Log "Failed to write result JSON: $_" -Level WARNING - $script:Stats.WarningsCount++ + # v2.20: this comment said "must be loud" while the code raised a warning, and the + # exit code is decided by ErrorsCount alone - so a run that failed to produce the + # artefact the user explicitly asked for still exited 0 and printed "completed with + # warnings". The stand then reads a stale file from the previous run as a fresh + # result. The user asked for this file: not producing it is a failure of the run. + Write-Log "Failed to write result JSON: $_" -Level ERROR + $script:Stats.ErrorsCount++ } } @@ -4543,14 +5355,14 @@ function Start-WinClean { Remove-Item -LiteralPath $ResultJsonPath -Force -ErrorAction SilentlyContinue } - # v2.19: reset the per-run phase buckets and step counter. The script normally runs - # once per process (entry-point guarded), but dot-sourcing it and calling Start-WinClean - # twice in one session would otherwise accumulate phase names across runs and leave the - # progress counter where the last run stopped. - $script:Stats.PhasesCompleted = @() - $script:Stats.PhasesFailed = @() - $script:Stats.PhasesSkipped = @() - $script:Stats.CurrentStep = 0 + # v2.19 reset the phase buckets and the step counter here; v2.20 makes the run + # genuinely fresh. The partial version left freed bytes, warning/error counts, + # Aborted and StartTime from a previous call in the same session, so the second + # run's summary and JSON described both runs at once. See New-RunStats. + $script:Stats = New-RunStats + $script:ProgressActivities = @() + $script:InternetConnectionCache = $null + $script:LogWriteFailed = $false # Initialize log "WinClean v$($script:Version) - Started at $(Get-Date)" | Out-File -FilePath $script:LogPath -Encoding utf8 @@ -4698,7 +5510,16 @@ function Start-WinClean { Clear-KernelDumps # Neither of these reports what it freed, so measure the drive around them Measure-FreeSpaceGain -Category 'ComponentStore' -Operation { Invoke-DISMCleanup } - Measure-FreeSpaceGain -Category 'DiskCleanup' -Operation { Invoke-StorageSense } + # Raised in review: measuring around a step the user switched off credited it + # with whatever the drive happened to gain meanwhile - DISM releasing files a + # moment earlier is enough - and printed "DiskCleanup freed approximately + # 300.00 MB" for something that executed nothing. Invoke-StorageSense is still + # called so its registry leftovers get swept; it just is not measured. + if ($SkipDiskCleanup) { + Invoke-StorageSense + } else { + Measure-FreeSpaceGain -Category 'DiskCleanup' -Operation { Invoke-StorageSense } + } Clear-WindowsOld } @@ -4727,8 +5548,12 @@ function Start-WinClean { Show-FinalStatistics # Release the log file handle (v2.17, p.7): a stand or the user may want to # move/zip the log right after the run finishes. + # v2.20: guarded. Dispose on a writer whose stream already failed throws, and this + # is the last statement of the outer finally - the exception escaped Start-WinClean + # and the entry point never reached its exit-code check, so a run with errors could + # still exit 0 (raised in review). if ($script:LogWriter) { - $script:LogWriter.Dispose() + try { $script:LogWriter.Dispose() } catch { } $script:LogWriter = $null $script:LogWriterPath = $null } diff --git a/docs/result-json.md b/docs/result-json.md index c9d04c6..279a96b 100644 --- a/docs/result-json.md +++ b/docs/result-json.md @@ -26,6 +26,8 @@ This page documents every field, gives a full sample, and explains how to consum | `WarningsCount` | number | Count of warnings raised during the run. Warnings are the silent-failure alarm; treat a non-zero value as something to inspect. | | `ErrorsCount` | number | Count of errors raised during the run. A healthy run reports `0`. | | `RebootRequired` | bool | `true` when a change (a Windows update, an app update finishing on reboot) needs a restart to take effect. | +| `LoggingDegraded` | bool | v2.20. `true` when writing the log file failed at some point during the run. The run itself still completed, but `LogPath` points at an incomplete file: do not read that log as the full record of what happened. | +| `DiskCleanupPending` | bool | v2.20. `true` when Disk Cleanup outlived its timeout and was left running in the background. `TotalFreedBytes` is then a lower bound rather than the final figure, because deletion continued after this file was written. | | `ControlledFolderAccess` | string | Tri-state, see below. Reflects whether Defender's Controlled Folder Access may have silently blocked deletions. | | `Aborted` | string or null | `null` unless the run stopped early for a known reason (currently only a declined pending reboot, `"PendingRebootDeclined"`, sets it). When set, the phase arrays below are incomplete by design. Note `null` does not by itself prove every phase ran - see the invariant note below. | | `PhasesCompleted` | array of string | Phases whose action ran to completion without an uncaught exception. | @@ -45,6 +47,7 @@ An object of booleans mirroring the run's switches: | `SkipDevCleanup` | bool | | `SkipDockerCleanup` | bool | | `SkipVSCleanup` | bool | +| `SkipDiskCleanup` | bool | | `DisableTelemetry` | bool | `-ReportOnly`, `-LogPath` and `-ResultJsonPath` are not repeated here (`ReportOnly` and `LogPath` are top-level fields; the JSON path is the file you are reading). @@ -90,7 +93,7 @@ VisualStudioCleanup, DeepSystemCleanup, DiskSpaceReport, Telemetry ```json { - "Version": "2.19", + "Version": "2.20", "Timestamp": "2026-07-21T03:15:42.1234567+00:00", "DurationSeconds": 196.4, "ReportOnly": false, @@ -101,6 +104,7 @@ VisualStudioCleanup, DeepSystemCleanup, DiskSpaceReport, Telemetry "SkipDevCleanup": false, "SkipDockerCleanup": false, "SkipVSCleanup": false, + "SkipDiskCleanup": false, "DisableTelemetry": false }, "TotalFreedBytes": 3201171456, @@ -117,6 +121,8 @@ VisualStudioCleanup, DeepSystemCleanup, DiskSpaceReport, Telemetry "ErrorsCount": 0, "RebootRequired": false, "ControlledFolderAccess": "disabled", + "LoggingDegraded": false, + "DiskCleanupPending": false, "Aborted": null, "PhasesCompleted": [ "Preparation", @@ -182,4 +188,4 @@ The Proxmox stand (`tools/proxmox/Invoke-StandTest.ps1`) reads this JSON and fai - `ControlledFolderAccess` is not `"unknown"`. - In report modes (`Report`, `ReportNoCleanup`): `ReportOnly` is `true` and `TotalFreedBytes == 0` (a preview that frees bytes is a regression). - In full modes: `TotalFreedBytes` is well above a trivial threshold. -- The phase arrays are disjoint, their union is the nine known phases, and any skip flag in `Parameters` is reflected in `PhasesSkipped` (for example, the `ReportNoCleanup` mode sets `-SkipCleanup` and expects the whole cleanup group to be skipped). These phase checks apply only to result JSON produced by 2.19 or newer: the nightly also runs a pass against the latest published release, which can predate the schema, and asserting it there would fail the run for version skew rather than for a defect. When they are skipped, the harness says so. +- The phase arrays are disjoint, their union is the nine known phases, and a skip flag that gates a whole phase is reflected in `PhasesSkipped` (for example, the `ReportNoCleanup` mode sets `-SkipCleanup` and expects the whole cleanup group to be skipped). `-SkipDiskCleanup` is the exception and deliberately so: it suppresses one step inside `DeepSystemCleanup`, not the phase, so that phase still lands in `PhasesCompleted`. These phase checks apply only to result JSON produced by 2.19 or newer: the nightly also runs a pass against the latest published release, which can predate the schema, and asserting it there would fail the run for version skew rather than for a defect. When they are skipped, the harness says so. diff --git a/docs/safety.md b/docs/safety.md index 4622b89..544239c 100644 --- a/docs/safety.md +++ b/docs/safety.md @@ -30,6 +30,8 @@ C:\Users\\ Volume roots (for example `C:\`) are protected explicitly as well. Path protection is not a naive string match: it normalizes short (8.3) names such as `PROGRA~1` and resolves `..` traversal before comparing, so a path that only looks different on the surface cannot slip past the check. +Since v2.20 the check also follows links. Path normalization works on text and does not resolve reparse points, so a junction whose visible path looked harmless while pointing at a protected root used to pass the guard, and enumerating that junction lists the target's contents. A cleanup root is now resolved to its final target before the rules are applied, and a link that cannot be resolved is refused rather than guessed at. Only the root needs this: a link found deeper inside a tree is already harmless, because the recursive walk does not descend into reparse points and deleting a junction removes the link and leaves its target intact. + ## Preview mode (`-ReportOnly`) `-ReportOnly` performs a dry run: it walks the same logic a real run would and reports what would be cleaned and how much space it would free. It makes no maintenance changes - it installs nothing, deletes none of the caches or files it would clean, and creates no restore point. It does still write its own log file (and the result JSON if you passed `-ResultJsonPath`, removing any pre-existing one first on a best-effort basis), set the process TLS version, and perform the read-only update and connectivity checks unless you also pass `-SkipUpdates`. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 909791b..f9aed5e 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -27,6 +27,57 @@ If `winget` is not installed, the application-update phase is skipped and the ru --- +## The same application is offered for update on every single run + +WinClean asks `winget` what can be upgraded and then asks it to upgrade. When the same package keeps appearing run after run, the loop is almost always outside WinClean. There are two distinct causes, and they need different answers. + +### Cause 1: the package is installed for the current user, and WinClean runs elevated + +WinClean requires administrator rights, and `winget` refuses to manage **user-scope** packages from an elevated process: + +``` +The package installed for user scope cannot be uninstalled when running with administrator privileges. +``` + +An upgrade replaces the installed package, so it hits the same wall. Such packages will be offered, fail, and be offered again on the next run, forever. WinClean reports the failure honestly, but only as a warning that some upgrades failed: `winget upgrade --all` runs as one batch and returns a single exit code, so the message cannot name the package. It cannot fix it either, because a run cannot drop its own privileges half way through. + +**Fix:** upgrade those packages yourself from a **normal, non-elevated** PowerShell window: + +```powershell +winget upgrade --id +``` + +### Cause 2: the package's own installer records the wrong version + +`winget` decides whether an upgrade exists by reading `DisplayVersion` from the package's uninstall entry in the registry, **not** by looking at the installed executable. If an installer writes fresh files but records a stale version in its own entry, the loop is permanent: the registry says 1.14.1, winget offers 1.14.7, the installer places 1.14.7 and writes 1.14.1 again. + +**Diagnosis** (read-only, run in any PowerShell window): + +```powershell +# what winget reads +Get-ChildItem 'HKCU:\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall', + 'HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall', + 'HKLM:\SOFTWARE\WOW6432Node\Microsoft\Windows\CurrentVersion\Uninstall' | + ForEach-Object { Get-ItemProperty $_.PSPath } | + Where-Object DisplayName -like '**' | + Select-Object DisplayName, DisplayVersion, InstallLocation + +# what is actually installed +(Get-Item '\.exe').VersionInfo.FileVersion +``` + +If the two disagree, the installer is at fault, not winget and not WinClean. Report it to the application's vendor, and meanwhile choose one of: + +- correct the recorded version so winget stops offering it, and let a genuinely newer release be offered normally: + ```powershell + Set-ItemProperty '' -Name DisplayVersion -Value '' + ``` +- or silence the package entirely with `winget pin add --id `, accepting that real updates are silenced too. + +Either way the summary line stays truthful: it counts updates **offered**, not installed. See [result-json.md](result-json.md) for the `AppUpdatesOffered` field. + +--- + ## Windows updates are skipped (PowerShell Gallery unreachable) WinClean installs the `PSWindowsUpdate` module from the PowerShell Gallery. If the Gallery is unreachable (offline, proxy, TLS), the module cannot install and the Windows-update phase is skipped. The rest of the run continues normally. @@ -97,6 +148,7 @@ Then check the log file to see exactly what was changed. - **Очистка почти ничего не освобождает:** включён Controlled Folder Access - добавьте `pwsh.exe` в список разрешённых приложений. В JSON поле `ControlledFolderAccess` = `enabled`. - **Обновления приложений пропущены:** не найден `winget` - установите App Installer из Microsoft Store. +- **Одно и то же приложение предлагается к обновлению каждый прогон:** причина вне WinClean. Либо пакет установлен для пользователя, а WinClean работает с правами администратора, и `winget` в повышенном контексте такие пакеты трогать отказывается - обновите его сами в обычном окне PowerShell (`winget upgrade --id `). Либо установщик пакета кладёт свежие файлы, но пишет в свою же запись в реестре старую версию, а `winget` смотрит именно туда: сверьте `DisplayVersion` из ветки `Uninstall` с `FileVersion` установленного exe. - **Обновления Windows пропущены:** недоступен PowerShell Gallery - модуль `PSWindowsUpdate` не ставится, прогон продолжается. - **Нужны права администратора и PowerShell 7.1+:** откройте `Win+X` -> Терминал (Администратор), вкладка PowerShell 7. - **One-liner не запускается:** это fail-closed - при отсутствии ассета или несовпадении SHA256 запуск отменяется намеренно; скачайте вручную со страницы Releases. diff --git a/docs/what-is-cleaned.md b/docs/what-is-cleaned.md index 9551c5a..beb4206 100644 --- a/docs/what-is-cleaned.md +++ b/docs/what-is-cleaned.md @@ -103,7 +103,7 @@ As of v2.19 the rule requires a strictly newer version; a package of the same ve ## Disk Cleanup -The normal path is **Storage Sense** (the built-in scheduled task), when it is present and enabled. Only if Storage Sense is missing or disabled does WinClean **fall back to `cleanmgr`**, arming StateFlags for up to 23 registry handler categories (system caches, error dumps, update leftovers, thumbnail cache, and similar). In the fallback, only categories that actually exist on the machine are armed; a run that could arm nothing is skipped and is not reported as a success. `DownloadsFolder` is deliberately excluded, because it holds user files. +The normal path is **Storage Sense** (the built-in scheduled task), when it is present and enabled. WinClean **falls back to `cleanmgr`** whenever Storage Sense did not demonstrably do the work: it is missing, ambiguous, disabled, could not be started, failed with an error code, returned a result that could not be read, finished without freeing anything measurable, ran out of the two-minute wait, or disappeared while being watched. On a machine where the task exists but fails, that fallback is the normal outcome, so use `-SkipDiskCleanup` if the step is too slow there. The fallback arms StateFlags for up to 23 registry handler categories (system caches, error dumps, update leftovers, thumbnail cache, and similar). In the fallback, only categories that actually exist on the machine are armed; a run that could arm nothing is skipped and is not reported as a success. `DownloadsFolder` is deliberately excluded, because it holds user files. ## Windows.old diff --git a/get.ps1 b/get.ps1 index 56836da..5446241 100644 --- a/get.ps1 +++ b/get.ps1 @@ -75,7 +75,8 @@ if (-not $principal.IsInRole([Security.Principal.WindowsBuiltInRole]::Administra # 3. Parse arguments BEFORE touching the network, and validate them against # WinClean's actual parameter set - a typo should not cost a download. $switchParams = @('SkipUpdates', 'SkipCleanup', 'SkipRestore', 'SkipDevCleanup', - 'SkipDockerCleanup', 'SkipVSCleanup', 'DisableTelemetry', 'ReportOnly') + 'SkipDockerCleanup', 'SkipVSCleanup', 'SkipDiskCleanup', + 'DisableTelemetry', 'ReportOnly') $valueParams = @('LogPath', 'ResultJsonPath') $splat = @{} diff --git a/tests/Fixes.Tests.ps1 b/tests/Fixes.Tests.ps1 index f3cb76c..69f9981 100644 --- a/tests/Fixes.Tests.ps1 +++ b/tests/Fixes.Tests.ps1 @@ -433,12 +433,30 @@ Describe "v2.15: positional binding hardening" -Tag "Fix", "V215" { #region Script Version Verification Describe "Script Version" -Tag "Version" { - It "Version is 2.13 or higher" { - $scriptContent | Should -Match '\$script:Version\s*=\s*[''"]2\.1[3-9]' + <# + v2.20: these used to be the regex '2\.1[3-9]', which stopped matching the moment the + version crossed 2.19 - the tests failed on the version bump itself rather than on any + defect. Compare versions as versions, and check the invariant that actually matters: + the two places agree with each other. + #> + BeforeAll { + $script:declaredVersion = if ($scriptContent -match '(?m)^\$script:Version\s*=\s*"([\d.]+)"') { $Matches[1] } else { $null } + $script:scriptInfoVersion = if ($scriptContent -match '(?m)^\.VERSION\s+([\d.]+)\s*$') { $Matches[1] } else { $null } + } + + It "Declares a parseable version" { + $script:declaredVersion | Should -Not -BeNullOrEmpty + { [version]$script:declaredVersion } | Should -Not -Throw + } + + It "Is 2.13 or higher" { + [version]$script:declaredVersion | Should -BeGreaterOrEqual ([version]'2.13') } - It "PSScriptInfo version matches" { - $scriptContent | Should -Match '\.VERSION\s+2\.1[3-9]' + It "PSScriptInfo carries the same version as `$script:Version" { + # PSGallery publishes from PSScriptInfo while the banner and the update check read + # $script:Version - a mismatch ships a package that lies about its own version + $script:scriptInfoVersion | Should -Be $script:declaredVersion } } @@ -632,14 +650,28 @@ Describe "v2.16: Disk Cleanup timeout" -Tag "Fix", "V216" { $scriptContent | Should -Match '\$maxWait = 900' } - It "Exceeding the wait is not counted as a warning" { - $scriptContent | Should -Match 'leaving it to finish in the background' + It "Exceeding the wait is reported as a warning (changed in v2.20)" { + # v2.16 logged this at INFO because killing cleanmgr was worse than waiting, and + # that part still holds. But the consequence was never stated: everything measured + # after this point is partial, and the run prints its total and writes its JSON + # while an elevated process is still deleting. Silence made a partial result look + # like a final one. + $body = Get-FunctionBody -Name 'Invoke-StorageSense' + $body | Should -Match 'still running - it continues in the background' + $body | Should -Match '\$script:Stats\.DiskCleanupPending = \$true' } It "cleanmgr is not killed on timeout" { # Killing it mid-delete achieved nothing and contradicted the log message $scriptContent | Should -Not -Match '\$cleanmgr \| Stop-Process' } + + It "Registry configuration is not pulled from under a still-running cleanmgr (v2.20)" { + # The finally block swept StateFlags immediately after deciding to let cleanmgr + # keep working, which removed the configuration it was running on + $body = Get-FunctionBody -Name 'Invoke-StorageSense' + $body | Should -Match 'if \(\$cleanmgr -and -not \$cleanmgr\.HasExited\)' + } } #endregion @@ -788,9 +820,12 @@ Describe "v2.16: silent failures are reported" -Tag "Fix", "V216", "SilentFailur $scriptContent | Should -Match '(?s)Winget source update timed out[^\r\n]*\r?\n\s*\$script:Stats\.WarningsCount\+\+' } - It "Result JSON write failure counts as a warning" { - # An automated stand would otherwise read the previous run's file as fresh - $scriptContent | Should -Match '(?s)Failed to write result JSON[^\r\n]*\r?\n\s*\$script:Stats\.WarningsCount\+\+' + It "Result JSON write failure counts as an error, not a warning (v2.20)" { + # An automated stand would otherwise read the previous run's file as fresh. + # Changed deliberately in v2.20: the exit code is decided by ErrorsCount alone, + # so as a warning this failure still exited 0 - the run reported success while + # the artefact the user explicitly requested was missing. + $scriptContent | Should -Match '(?s)Failed to write result JSON[^\r\n]*\r?\n\s*\$script:Stats\.ErrorsCount\+\+' } It "Downloaded updates are not counted as installed" { @@ -1070,3 +1105,326 @@ Describe "v2.19: get.ps1 forwards exactly WinClean's parameter set" -Tag "Fix", } #endregion + +#region V220: silent failures that reported success + +Describe "v2.20: an operation that did nothing does not report success" -Tag "Fix", "V220" { + + Context "Event log enumeration failure" { + <# + Behavioural, not a grep: Get-WinEvent is mocked to fail the way a stopped Event Log + service fails. The old code then had an empty list, zero failed clears, and took the + success branch - "Event logs cleared (0 logs)" while nothing was touched. Nothing is + actually cleared here either, because the loop never has a channel to run on. + #> + It "Warns instead of claiming success when no channel can be listed" { + Mock Get-WinEvent { + Write-Error 'The Event Log service is unavailable' -ErrorAction SilentlyContinue + @() + } + + # Prove the mock is in effect BEFORE the product is allowed to touch anything. + # If it ever stopped intercepting (a Pester upgrade, a scoping change, the + # product switching to another API), the real enumeration would return the real + # channels and this test would clear the developer's own event logs for real - + # and only fail afterwards (raised in review). + @(Get-WinEvent -ListLog *).Count | Should -Be 0 -Because 'the mock must intercept before Clear-EventLogs runs' + + $warningsBefore = $script:Stats.WarningsCount + Clear-EventLogs + $script:Stats.WarningsCount | Should -BeGreaterThan $warningsBefore + } + } + + Context "npm exit code" { + # npm fails without throwing (EPERM on a locked cache), so the native exit code is + # the only signal. Scoped to the function body: a match anywhere in the file would + # stay green if this handling were deleted. + It "Captures the npm exit code and uses it" { + $body = Get-FunctionBody -Name 'Clear-DeveloperCaches' + $body | Should -Match '\$npmExit\s*=\s*\$LASTEXITCODE' + $body | Should -Match '\$npmExit\s*-ne\s*0' + } + + It "No longer reports a bare success when nothing was freed" { + $body = Get-FunctionBody -Name 'Clear-DeveloperCaches' + $body | Should -Not -Match 'npm cache cleaned \(via npm\)' + } + } + + Context "winget source update exit code" { + It "Reads the source-update exit code, not just job completion" { + $body = Get-FunctionBody -Name 'Update-Applications' + $body | Should -Match 'Winget source update failed' + } + } + + Context "Per-run state" { + It "Start-WinClean builds a fresh stats object instead of patching three fields" { + $body = Get-FunctionBody -Name 'Start-WinClean' + $body | Should -Match '\$script:Stats\s*=\s*New-RunStats' + $body | Should -Match '\$script:InternetConnectionCache\s*=\s*\$null' + } + } + + Context "Logging failure is visible" { + It "Latches the first log write failure instead of swallowing every one" { + $body = Get-FunctionBody -Name 'Write-Log' + $body | Should -Match '\$script:LogWriteFailed' + # Write-Log must not report its own failure through Write-Log + $body | Should -Match 'Write-Host' + } + + It "Surfaces it in the result JSON so a consumer knows the log is incomplete" { + $body = Get-FunctionBody -Name 'Write-ResultJson' + $body | Should -Match 'LoggingDegraded' + } + } + + Context "Privacy traces are confirmed, not assumed" { + It "Compares the value count before and after instead of appending unconditionally" { + $body = Get-FunctionBody -Name 'Clear-PrivacyTraces' + $body | Should -Match 'Get-RegistryValueCount' + # The old shape: success text appended right after a SilentlyContinue delete + $body | Should -Match '\$after\s*-eq\s*0' + } + } +} + +#endregion + +#region V220R: findings from the pre-release review of v2.20 + +Describe "v2.20 review: measurements and failures answer honestly" -Tag "Fix", "V220R" { + <# + These cover the defects an independent review found in the v2.20 fixes themselves. + Behavioural coverage for the Storage Sense decisions lives in Helpers.Tests.ps1 + (Select-StorageSenseTask / Get-StorageSenseVerdict / Wait-StorageSenseTask); what is + left here is code that cannot be reached without a real scheduler, browser or winget. + #> + + Context "Browser caches: both sides of the subtraction describe the same files" { + It "Does not measure 'before' with the raw walker and 'after' with the checked one" { + $body = Get-FunctionBody -Name 'Clear-BrowserCaches' + # Get-FolderSize skips inaccessible files silently; Get-FolderSizeChecked + # refuses to answer at all. Mixing them subtracted two different file sets. + $body | Should -Not -Match 'sizeBefore\s*=.*Get-FolderSize\s' + } + + It "Pairs the measurements per path instead of discarding the whole delta" { + $body = Get-FunctionBody -Name 'Clear-BrowserCaches' + $body | Should -Match '\$beforeMeasurements' + $body | Should -Match '\$afterUnmeasured\+\+' + } + } + + Context "npm cache: an unreadable cache is not an emptied one" { + It "Measures both sides with the checked variant" { + $body = Get-FunctionBody -Name 'Clear-DeveloperCaches' + $body | Should -Match '\$sizeBefore = Get-FolderSizeChecked' + $body | Should -Match '\$sizeAfter = Get-FolderSizeChecked' + } + + It "Says so instead of calling it empty when the size is unknown" { + $body = Get-FunctionBody -Name 'Clear-DeveloperCaches' + $body | Should -Match '\$npmMeasured' + } + } + + Context "Event logs: a partial enumeration failure is not a success" { + It "Decides on the unfiltered channel list" { + $body = Get-FunctionBody -Name 'Clear-EventLogs' + $body | Should -Match '\$allLogs\.Count -eq 0' + # The old discriminator read the FILTERED list, so 40 readable channels out of + # 510 with 470 errors produced a plain success line + $body | Should -Not -Match 'if \(-not \$logs -and \$enumErrors\)' + } + + It "Reports channels that could not be listed separately from clearing failures" { + $body = Get-FunctionBody -Name 'Clear-EventLogs' + $body | Should -Match '\$enumErrorCount' + } + } + + Context "winget: an unusable executable is reported, not passed over" { + It "Treats a missing exit code as a failure rather than short-circuiting on null" { + $body = Get-FunctionBody -Name 'Update-Applications' + $body | Should -Match '\[int\]::TryParse' + $body | Should -Match '\$jobState' + # The old guard: a null exit code silently satisfied it + $body | Should -Not -Match '0 -ne \[int\]\$sourceExit' + } + } + + Context "Restore point: the killed child is gone before the registry is judged" { + It "Waits for the process to actually exit after Kill" { + $body = Get-FunctionBody -Name 'New-SystemRestorePoint' + # Ordering, not proximity: a distance-bounded regex would break the next time + # the comment between the two lines grows. + $killAt = $body.IndexOf('Kill($true)') + $waitAt = $body.IndexOf('WaitForExit(5000)') + $killAt | Should -BeGreaterOrEqual 0 + $waitAt | Should -BeGreaterThan $killAt + } + } + + Context "Storage Sense: the task is pinned and the decisions are delegated" { + It "Looks the task up by name alone exactly once - the initial discovery" { + $body = Get-FunctionBody -Name 'Invoke-StorageSense' + $byNameOnly = [regex]::Matches($body, 'Get-ScheduledTask -TaskName \$ssTaskName').Count + $byNameOnly | Should -Be 1 + # Every later lookup goes through the pinned parameter set + [regex]::Matches($body, 'Get-ScheduledTask @ssLookup').Count | Should -BeGreaterThan 1 + } + + It "Never passes a null TaskPath, which throws a binding error -ErrorAction cannot suppress" { + $body = Get-FunctionBody -Name 'Invoke-StorageSense' + $body | Should -Match "if \(\`$ssTaskPath\) \{ \`$ssLookup\['TaskPath'\] = \`$ssTaskPath \}" + } + + It "Uses the helpers that carry the tested rules" { + $body = Get-FunctionBody -Name 'Invoke-StorageSense' + $body | Should -Match 'Select-StorageSenseTask' + $body | Should -Match 'Get-StorageSenseVerdict' + $body | Should -Match 'Wait-StorageSenseTask' + } + + It "Distinguishes a task that disappeared from one that ran out of time" { + $body = Get-FunctionBody -Name 'Invoke-StorageSense' + $body | Should -Match "'vanished'" + } + } + + Context "A test file that fails to load cannot slip past green" { + It "Invoke-Tests fails the run on a container that never produced tests" { + # Measured on Pester 5.7.1: a parse error gives Result=Failed and + # FailedContainersCount=1 while Failed/Skipped/NotRun are all 0 + $text = Get-Content (Join-Path $PSScriptRoot '..' 'tools' 'Invoke-Tests.ps1') -Raw + $text | Should -Match 'FailedContainersCount' + $text | Should -Match '\$failedContainers -gt 0' + } + + It "The release gate applies the same rule as CI" { + $text = Get-Content (Join-Path $PSScriptRoot '..' 'tools' 'Invoke-ReleaseCheck.ps1') -Raw + $text | Should -Match 'FailedContainersCount' + } + } +} + +#endregion + +#region V220R2: second review round, before release + +Describe "v2.20 pre-release review: fixes to the fixes" -Tag "Fix", "V220R2" { + <# + Five reviewers (four specialised agents plus a cross-engine pass) went over this + release. The behavioural coverage for what they found lives in Helpers.Tests.ps1; + what remains here is code that needs a scheduler, a missing cleanmgr.exe or a + registry hive to reach. + #> + + Context "Storage Sense: a step that did not happen cannot look like one that did" { + It "Refuses to wait on a process that never started" { + $body = Get-FunctionBody -Name 'Invoke-StorageSense' + # Measured: Start-Process on a missing exe leaves $null, and $null.HasExited is + # $null, so '-not $cleanmgr.HasExited' is TRUE - the loop reported progress for + # the full fifteen minutes and then set DiskCleanupPending for a process that + # did not exist. + $body | Should -Match 'if \(-not \$cleanmgr\) \{' + $startAt = $body.IndexOf('Start-Process -FilePath "cleanmgr.exe"') + $guardAt = $body.IndexOf('if (-not $cleanmgr) {') + $startAt | Should -BeGreaterOrEqual 0 + $guardAt | Should -BeGreaterThan $startAt + } + + It "Claims the task stopped only after checking that it stopped" { + $body = Get-FunctionBody -Name 'Invoke-StorageSense' + $body | Should -Match '\$taskAfterStop' + # The unconditional claim must be gone: the success line has to sit in an else + $body | Should -Match 'Stop-ScheduledTask[\s\S]{0,400}?\$taskAfterStop' + } + + It "Counts a two-minute timeout as a warning again" { + $body = Get-FunctionBody -Name 'Invoke-StorageSense' + $body | Should -Match 'did not finish within \$timeout seconds[^\n]*-Level WARNING' + } + + It "Does not contradict itself about whether the task was found" { + $body = Get-FunctionBody -Name 'Invoke-StorageSense' + # An ambiguous lookup used to log "2 tasks with that name" and then, two lines + # later, "task not found" + $body | Should -Match '\$ssExplained' + $body | Should -Match 'if \(-not \$ssExplained\)' + } + + It "Sweeps the registry leftovers before honouring -SkipDiskCleanup" { + $body = Get-FunctionBody -Name 'Invoke-StorageSense' + $sweepAt = $body.IndexOf('Remove-ItemProperty -Path $_.PSPath') + $skipAt = $body.IndexOf('if ($SkipDiskCleanup) {') + $sweepAt | Should -BeGreaterOrEqual 0 + $skipAt | Should -BeGreaterThan $sweepAt + } + + It "Does not measure free space around a step the user switched off" { + $body = Get-FunctionBody -Name 'Start-WinClean' + # Bracketing a no-op with two drive reads credited DiskCleanup with whatever + # the drive gained meanwhile + $body | Should -Match 'if \(\$SkipDiskCleanup\) \{\s*\r?\n\s*Invoke-StorageSense' + } + } + + Context "Link resolution fails closed on every unknown" { + It "Returns null when the resolution bound is exhausted" { + $body = Get-FunctionBody -Name 'Resolve-PathThroughLinks' + # Falling out of the bounded loop means "could not resolve", and the caller + # only fails closed on $null + $body | Should -Match 'if \(-not \$changed\) \{ return \$current \}' + $body.TrimEnd() | Should -Match 'return \$null\s*\}$' + } + + It "Returns null when an ancestor cannot be examined" { + $body = Get-FunctionBody -Name 'Resolve-PathThroughLinks' + $body | Should -Match 'catch \{ return \$null \}' + } + } + + Context "The release note exists in exactly one place" { + It "Appears exactly once inside PSScriptInfo, and its text is not duplicated outside" { + # A version bump pasted the whole v2.20 release-note sentence into + # Invoke-Phase's comment-based help, and that stray copy satisfied the release + # gate's .RELEASENOTES check on its own. + # Note the count is taken INSIDE the block: "vX.Y:" also opens legitimate prose + # in other help blocks, so a whole-file count would forbid ordinary comments. + $version = if ($scriptContent -match '(?m)^\$script:Version\s*=\s*"([\d.]+)"') { $Matches[1] } else { $null } + $version | Should -Not -BeNullOrEmpty + + $psScriptInfo = if ($scriptContent -match '(?s)<#PSScriptInfo(.*?)#>') { $Matches[1] } else { '' } + $psScriptInfo | Should -Not -BeNullOrEmpty + ([regex]::Matches($psScriptInfo, "(?m)^\s*v$([regex]::Escape($version)):")).Count | Should -Be 1 + + # And the sentence itself belongs to that one place only + $noteLine = ([regex]::Match($psScriptInfo, "(?m)^\s*v$([regex]::Escape($version)):.*$")).Value.Trim() + $noteLine.Length | Should -BeGreaterThan 40 + ([regex]::Matches($scriptContent, [regex]::Escape($noteLine))).Count | Should -Be 1 + } + + It "The gate looks for it only inside the .RELEASENOTES section" { + # Scoping to the whole PSScriptInfo block was still too wide: a "vX.Y:" line + # under any other field satisfied it while .RELEASENOTES was empty + $gate = Get-Content (Join-Path $PSScriptRoot '..' 'tools' 'Invoke-ReleaseCheck.ps1') -Raw + $gate | Should -Match '\$releaseNotes' + $gate | Should -Match "What = '\.RELEASENOTES first line'; Ok = \`$releaseNotes" + $gate | Should -Not -Match "What = '\.RELEASENOTES first line'; Ok = \`$scriptText" + } + + It "Resolve-PathThroughLinks stops the ancestor walk at the volume root" { + # The walk climbed past the root of a UNC share, Get-Item on \\server failed, + # and the fail-closed rule then refused every UNC cleanup root + $body = Get-FunctionBody -Name 'Resolve-PathThroughLinks' + $body | Should -Match 'GetPathRoot' + $body | Should -Match '\$parent\.Length -lt \$rootPath\.Length' + } + } +} + +#endregion diff --git a/tests/Helpers.Tests.ps1 b/tests/Helpers.Tests.ps1 index 092eea8..4029b2b 100644 --- a/tests/Helpers.Tests.ps1 +++ b/tests/Helpers.Tests.ps1 @@ -187,6 +187,64 @@ Describe "ConvertFrom-HumanReadableSize" -Tag "Unit", "Helper" { ConvertFrom-HumanReadableSize -SizeString $SizeString | Should -Be $Expected } + It "Reads the en-US thousands form instead of dividing it by a thousand (v2.20)" { + # The defect: a lone separator was always taken for the decimal point, so the + # ordinary "1,234 KB" from an en-US shell was read as 1.234 KB + $enUS = [cultureinfo]::GetCultureInfo('en-US') + ConvertFrom-HumanReadableSize -SizeString "1,234 KB" -Culture $enUS | Should -Be 1263616 + } + + It "Still reads the same string as a decimal where the culture says so" { + # ru-RU writes 1.234 as "1,234" and groups thousands with a no-break space, + # so the identical text means something else there + $ruRU = [cultureinfo]::GetCultureInfo('ru-RU') + ConvertFrom-HumanReadableSize -SizeString "1,234 KB" -Culture $ruRU | Should -Be 1264 + } + + It "Does not let the culture override a shape that cannot be a grouping" { + # Guards the repair that would have been worse than the defect: .NET's + # AllowThousands does not validate grouping, so a culture-first parse reads + # "1,5" as 15 on en-US. Three digits are required after the separator. + $enUS = [cultureinfo]::GetCultureInfo('en-US') + ConvertFrom-HumanReadableSize -SizeString "1,5 GB" -Culture $enUS | Should -Be 1610612736 + ConvertFrom-HumanReadableSize -SizeString "1,2345 MB" -Culture $enUS | Should -Be 1294467 + } + + It "Handles repeated grouping: '12,345,678 B' on en-US" { + $enUS = [cultureinfo]::GetCultureInfo('en-US') + ConvertFrom-HumanReadableSize -SizeString "12,345,678 B" -Culture $enUS | Should -Be 12345678 + } + + It "Applies the same rule to a lone dot: '' reads '1.234 KB' as " -ForEach @( + @{ Culture = 'en-US'; Expected = 1264 } # dot is the decimal point there + @{ Culture = 'de-DE'; Expected = 1263616 } # dot groups thousands there + ) { + ConvertFrom-HumanReadableSize -SizeString "1.234 KB" -Culture ([cultureinfo]::GetCultureInfo($Culture)) | + Should -Be $Expected + } + + It "Falls back to the invariant reading when the culture uses the mark for neither purpose" { + # ru-RU: decimal is ',' and thousands are grouped with a no-break space, so a + # lone '.' matches neither rule. Without the fallback the code would treat it + # as grouping and read "1.234 KB" as 1234 KB - a thousandfold over-read on the + # maintainer's own default locale, and no other test reaches this branch. + $ruRU = [cultureinfo]::GetCultureInfo('ru-RU') + $ruRU.NumberFormat.NumberDecimalSeparator | Should -Be ',' + $ruRU.NumberFormat.NumberGroupSeparator | Should -Not -Be '.' + ConvertFrom-HumanReadableSize -SizeString "1.234 KB" -Culture $ruRU | Should -Be 1264 + } + + It "Leaves an unambiguous two-separator string alone whatever the culture" { + # Both marks present means the last one is the decimal point, and no culture + # can make that ambiguous + foreach ($c in 'en-US', 'ru-RU', 'de-DE') { + ConvertFrom-HumanReadableSize -SizeString "1,234.5 MB" -Culture ([cultureinfo]::GetCultureInfo($c)) | + Should -Be 1294467072 + ConvertFrom-HumanReadableSize -SizeString "1.234,5 MB" -Culture ([cultureinfo]::GetCultureInfo($c)) | + Should -Be 1294467072 + } + } + It "Converts binary-unit spellings: ''" -ForEach @( @{ SizeString = "1 MiB"; Expected = 1048576 } @{ SizeString = "1.5 GiB"; Expected = 1610612736 } @@ -572,6 +630,102 @@ Describe "Get-FolderSizeChecked" -Tag "Unit", "Helper" { #endregion +#region New-RunStats Tests (v2.20) + +Describe "New-RunStats" -Tag "Unit", "Helper", "V220" { + <# + v2.19 reset only the phase buckets and the step counter while claiming to handle + "dot-source and call Start-WinClean twice". Everything else survived into the second + run's summary and JSON. These tests pin the whole object, not just the three arrays. + #> + It "Returns a fresh object rather than the live one" { + $saved = $script:Stats + try { + $script:Stats.TotalFreedBytes = 123456 + $script:Stats.WarningsCount = 7 + $script:Stats.ErrorsCount = 3 + $script:Stats.Aborted = 'PendingRebootDeclined' + $script:Stats.PhasesCompleted = @('Preparation') + + $fresh = New-RunStats + + $fresh.TotalFreedBytes | Should -Be 0 + $fresh.WarningsCount | Should -Be 0 + $fresh.ErrorsCount | Should -Be 0 + $fresh.Aborted | Should -BeNullOrEmpty + @($fresh.PhasesCompleted).Count | Should -Be 0 + @($fresh.FreedByCategory.Keys).Count | Should -Be 0 + } finally { + $script:Stats = $saved + } + } + + It "Starts the clock at creation, not at dot-source time" { + # DurationSeconds in the result JSON is computed from StartTime; a stale value + # made the second run in a session look hours long + $before = Get-Date + Start-Sleep -Milliseconds 20 + (New-RunStats).StartTime | Should -BeGreaterThan $before + } + + It "Is still a synchronized hashtable" { + # Parallel cleanup blocks rely on this + (New-RunStats).GetType().Name | Should -Be 'SyncHashtable' + } +} + +#endregion + +#region Get-RegistryValueCount Tests (v2.20) + +Describe "Get-RegistryValueCount" -Tag "Unit", "Helper" { + <# + Extracted in v2.20 so privacy cleanup can confirm a deletion instead of announcing it. + Tested against a key this suite creates itself - never against the user's real Explorer + history, which the product function does touch. + #> + BeforeAll { + $script:probeKey = "HKCU:\Software\WinCleanTest_$(Get-Random)" + New-Item -Path $script:probeKey -Force | Out-Null + } + + AfterAll { + Remove-Item -LiteralPath $script:probeKey -Recurse -Force -ErrorAction SilentlyContinue + } + + It "Counts nothing for a key that holds no values" { + # The PowerShell metadata (PSPath, PSProvider and friends) must not be counted, + # or an emptied key would look like it still holds five entries + Get-RegistryValueCount -Key $script:probeKey | Should -Be 0 + } + + It "Counts the real values" { + Set-ItemProperty -Path $script:probeKey -Name 'url1' -Value 'a' + Set-ItemProperty -Path $script:probeKey -Name 'url2' -Value 'b' + Get-RegistryValueCount -Key $script:probeKey | Should -Be 2 + } + + It "Returns 0 for a key that does not exist" { + Get-RegistryValueCount -Key "HKCU:\Software\WinCleanTest_NoSuchKey_$(Get-Random)" | Should -Be 0 + } + + It "Returns null - not 0 - for a key that exists but cannot be read" { + # The distinction is the whole point. The first draft of this helper returned 0 + # for an unreadable key, which recreated the bug it was written to fix: a delete + # that failed followed by an unreadable check would have counted as "cleared". + # Mocked rather than ACL-denied: creating a genuinely unreadable HKCU key on the + # developer's own machine is a side effect a unit test has no business leaving. + Mock Get-ItemProperty { throw [System.UnauthorizedAccessException]::new('denied') } + # Strictly $null, not "null or empty": the caller decides with `$null -eq $before`, + # and an empty array satisfies BeNullOrEmpty while failing that test, so the + # unreadable key would have fallen through and been reported as cleared. + $count = Get-RegistryValueCount -Key $script:probeKey + ($null -eq $count) | Should -BeTrue + } +} + +#endregion + #region Test-PathProtected Tests Describe "Test-PathProtected" -Tag "Unit", "Helper", "Security" { @@ -595,6 +749,84 @@ Describe "Test-PathProtected" -Tag "Unit", "Helper", "Security" { Test-PathProtected -Path "D:\Projects\Test" | Should -BeFalse } + Context "Reparse points (v2.20)" { + <# + Measured, not assumed: [System.IO.Path]::GetFullPath does NOT resolve a junction, + so every textual check above passes for a link whose target is protected, while + Get-ChildItem on that link lists the TARGET's children. The guard must resolve the + link. The second test is the one that keeps the fix honest - refusing every link + would be trivially "safe" and would break anyone who redirected a cache folder. + #> + BeforeAll { + $script:linkSandbox = Join-Path ([System.IO.Path]::GetTempPath()) "WinCleanLinkTest_$(Get-Random)" + $script:innocentTarget = Join-Path $script:linkSandbox 'innocent-target' + New-Item -ItemType Directory -Path $script:innocentTarget -Force | Out-Null + + $script:linkToProtected = Join-Path $script:linkSandbox 'looks-harmless' + $script:linkToInnocent = Join-Path $script:linkSandbox 'redirected-cache' + $script:linkToDrive = Join-Path $script:linkSandbox 'volume-link' + New-Item -ItemType Junction -Path $script:linkToProtected -Target $env:ProgramFiles -ErrorAction SilentlyContinue | Out-Null + New-Item -ItemType Junction -Path $script:linkToInnocent -Target $script:innocentTarget -ErrorAction SilentlyContinue | Out-Null + New-Item -ItemType Junction -Path $script:linkToDrive -Target "$env:SystemDrive\" -ErrorAction SilentlyContinue | Out-Null + } + + AfterAll { + # Deleting a junction removes the link and leaves the target alone (measured), + # but remove the links explicitly anyway before the sandbox + foreach ($l in $script:linkToProtected, $script:linkToInnocent, $script:linkToDrive) { + if ($l -and (Test-Path -LiteralPath $l)) { + Remove-Item -LiteralPath $l -Force -Recurse -ErrorAction SilentlyContinue + } + } + Remove-Item -LiteralPath $script:linkSandbox -Recurse -Force -ErrorAction SilentlyContinue + } + + It "Refuses a junction that points at a protected root" { + Test-Path -LiteralPath $script:linkToProtected | Should -BeTrue -Because 'without the junction this test proves nothing' + Test-PathProtected -Path $script:linkToProtected | Should -BeTrue + } + + It "Still allows a junction that points somewhere harmless" { + Test-Path -LiteralPath $script:linkToInnocent | Should -BeTrue -Because 'without the junction this test proves nothing' + Test-PathProtected -Path $script:linkToInnocent | Should -BeFalse + } + + It "Refuses a path whose ANCESTOR is a junction into a protected area" { + # The case the first version of this fix missed, found by review and measured: + # the leaf carries no reparse attribute, GetFullPath does not resolve the link + # above it, and 120 real C:\Windows children were visible through it. + $throughLink = Join-Path $script:linkToDrive 'Windows' + Test-Path -LiteralPath $throughLink | Should -BeTrue -Because 'the path must really resolve through the junction' + Test-PathProtected -Path $throughLink | Should -BeTrue + } + + It "Does not refuse a deep path under a harmless junction" { + # The other half: refusing everything under any link would be trivially safe + # and would break anyone whose cache folder is redirected + $deep = Join-Path $script:linkToInnocent 'sub' + New-Item -ItemType Directory -Path $deep -Force | Out-Null + Test-PathProtected -Path $deep | Should -BeFalse + } + + It "Refuses a path whose links could not be resolved at all" { + # Resolve-PathThroughLinks answers $null for "I could not work out where this + # really points" - an unreadable ancestor, or a chain deeper than the loop + # bound. Both used to return a partially resolved path instead, which was then + # judged on its text and came back "safe to empty". + Mock Resolve-PathThroughLinks { $null } + Test-PathProtected -Path $script:linkToInnocent | Should -BeTrue + } + + It "Treats a drive that is not mounted as absent, not as unexaminable" { + # DriveNotFoundException is a not-found answer. Landing in the refuse arm made + # every cleanup target on an unmapped drive a "Protected path skipped" WARNING, + # which is noise in the channel this release uses as its failure alarm. + $free = 'QWXYZJ'.ToCharArray() | Where-Object { -not (Test-Path "${_}:\") } | Select-Object -First 1 + $free | Should -Not -BeNullOrEmpty -Because 'the test needs a drive letter that is genuinely unmapped' + Test-PathProtected -Path "${free}:\SomeFolder" | Should -BeFalse + } + } + It "Handles trailing slashes correctly" { # Path with trailing slash should match protected path without trailing slash Test-PathProtected -Path "$env:SystemRoot\" | Should -BeTrue @@ -698,24 +930,33 @@ Describe "Write-Log" -Tag "Unit", "Helper" { # Allow small delay for file write Start-Sleep -Milliseconds 100 - if (Test-Path $script:LogPath) { - $logContent = Get-Content $script:LogPath -Raw - $logContent | Should -Match $uniqueMsg - } + # v2.20: the assertion used to sit inside `if (Test-Path $script:LogPath)`, so a + # missing log file made this test pass. A missing log file is exactly the defect + # it exists to catch (v2.14: the log was deleted by the script's own temp cleanup), + # which made it a test that could not fail for its own bug. Assert the + # precondition instead of hiding behind it. + Test-Path $script:LogPath | Should -BeTrue -Because 'Write-Log must create the log file' + (Get-Content $script:LogPath -Raw) | Should -Match $uniqueMsg } It "Respects -NoLog switch" { $noLogMsg = "NoLog message $(Get-Random)" - $sizeBefore = if (Test-Path $script:LogPath) { (Get-Item $script:LogPath).Length } else { 0 } - Write-Log -Message $noLogMsg -Level INFO -NoLog + # Anchor write: without an existing log file "the message is absent from the log" + # is true for the boring reason that there is no log at all + Write-Log -Message "Anchor $(Get-Random)" -Level INFO + Start-Sleep -Milliseconds 100 + Test-Path $script:LogPath | Should -BeTrue -Because 'the -NoLog check is meaningless without a real log file' + $sizeBefore = (Get-Item $script:LogPath).Length + Write-Log -Message $noLogMsg -Level INFO -NoLog Start-Sleep -Milliseconds 100 - if (Test-Path $script:LogPath) { - $logContent = Get-Content $script:LogPath -Raw - $logContent | Should -Not -Match $noLogMsg - } + # Two independent facts. The size comparison is what $sizeBefore was computed for + # since the test was written and never actually used until v2.20: it catches a + # -NoLog that writes something else to the file, which a text match would miss. + (Get-Content $script:LogPath -Raw) | Should -Not -Match $noLogMsg + (Get-Item $script:LogPath).Length | Should -Be $sizeBefore } } @@ -921,4 +1162,212 @@ Describe "Show-Banner" -Tag "Unit", "Helper", "V219" { } } +# The Storage Sense decision logic. Until v2.20 this branch was unreachable (it looked +# the task up under a folder it does not live in), so the whole of it shipped for six +# versions without a single test - and the first defect found in it after it came alive +# was "exit code 0 counted as proof that a cleanup happened". These cover the rules that +# decide whether all 23 cleanmgr handlers get skipped. + +Describe "Select-StorageSenseTask" -Tag "Unit", "Helper", "V220" { + + It "returns the single task when exactly one was found" { + $one = [pscustomobject]@{ TaskName = 'StorageSense'; TaskPath = '\Microsoft\Windows\DiskFootprint\' } + $result = Select-StorageSenseTask -Tasks @($one) + $result.Reason | Should -Be 'ok' + $result.Task.TaskPath | Should -Be '\Microsoft\Windows\DiskFootprint\' + } + + It "reports 'none' and no task when nothing was found" { + $result = Select-StorageSenseTask -Tasks @() + $result.Reason | Should -Be 'none' + $result.Task | Should -BeNullOrEmpty + } + + It "refuses to guess when several tasks share the name" { + # Starting the first of several same-named tasks means starting something nobody + # identified. The rule is "take none", not "take one". + $tasks = @( + [pscustomobject]@{ TaskName = 'StorageSense'; TaskPath = '\Microsoft\Windows\DiskFootprint\' } + [pscustomobject]@{ TaskName = 'StorageSense'; TaskPath = '\Custom\' } + ) + $result = Select-StorageSenseTask -Tasks $tasks + $result.Reason | Should -Be 'ambiguous' + $result.Task | Should -BeNullOrEmpty + } + + It "ignores null entries in the lookup result" { + $one = [pscustomobject]@{ TaskName = 'StorageSense'; TaskPath = '\Microsoft\Windows\DiskFootprint\' } + $result = Select-StorageSenseTask -Tasks @($null, $one, $null) + $result.Reason | Should -Be 'ok' + } +} + +Describe "Get-StorageSenseVerdict" -Tag "Unit", "Helper", "V220" { + + It "does NOT accept a successful exit code as proof that anything was cleaned" { + # The defect this whole rule exists for: Storage Sense obeys its own settings, so + # when it is switched off in Settings the task starts, does nothing and exits 0. + # Treating that as done suppresses every cleanmgr handler and frees nothing. + $verdict = Get-StorageSenseVerdict -TaskResult 0 -FreedBytes 0 + $verdict.Done | Should -BeFalse + $verdict.Reason | Should -Be 'nothing-freed' + } + + It "accepts success only when free space actually grew" { + $verdict = Get-StorageSenseVerdict -TaskResult 0 -FreedBytes ([long]5MB) + $verdict.Done | Should -BeTrue + $verdict.Reason | Should -Be 'success' + } + + It "is fail-closed on a non-zero task result IN THE TYPE WINDOWS ACTUALLY SUPPLIES" { + # LastTaskResult is a UInt32. Every HRESULT failure has the high bit set, so its + # unsigned value exceeds Int32.MaxValue: 0x80040154 arrives as 2147746132 and the + # old [int] cast THREW on it, taking down the whole DeepSystemCleanup phase. + # + # The first version of this test wrote the PowerShell literal 0x80040154, which the + # parser types as Int32 -2147221164. The cast succeeded, the test was green, and it + # was green BECAUSE of the defect. Passing the production type is the entire point. + $realValue = [uint32]2147746132 + $realValue | Should -BeOfType [uint32] + $verdict = Get-StorageSenseVerdict -TaskResult $realValue -FreedBytes ([long]5MB) + $verdict.Done | Should -BeFalse + $verdict.Reason | Should -Be 'failed' + } + + It "does not throw on the full UInt32 range" { + { Get-StorageSenseVerdict -TaskResult ([uint32]::MaxValue) -FreedBytes ([long]5MB) } | Should -Not -Throw + } + + It "treats a result that is not a number as unreadable rather than throwing" { + $verdict = Get-StorageSenseVerdict -TaskResult 'not-a-number' -FreedBytes ([long]5MB) + $verdict.Done | Should -BeFalse + $verdict.Reason | Should -Be 'unreadable' + } + + It "is fail-closed when the task result could not be read" { + $verdict = Get-StorageSenseVerdict -TaskResult $null -FreedBytes ([long]5MB) + $verdict.Done | Should -BeFalse + $verdict.Reason | Should -Be 'unreadable' + } + + It "distinguishes 'could not measure' from 'freed nothing'" { + $verdict = Get-StorageSenseVerdict -TaskResult 0 -FreedBytes $null + $verdict.Done | Should -BeFalse + $verdict.Reason | Should -Be 'not-measured' + } + + It "treats a shrinking drive as no cleanup rather than as success" { + $verdict = Get-StorageSenseVerdict -TaskResult 0 -FreedBytes ([long](-1MB)) + $verdict.Done | Should -BeFalse + $verdict.Reason | Should -Be 'nothing-freed' + } +} + +Describe "Wait-StorageSenseTask" -Tag "Unit", "Helper", "V220" { + + # No real waiting: the wait itself is injected, which is the reason this loop was + # split out of Invoke-StorageSense at all. + BeforeEach { + $script:wssCalls = 0 + $script:noWait = { } + } + + It "reports 'finished' once a running task stops" { + $getTask = { + $script:wssCalls++ + if ($script:wssCalls -lt 3) { + [pscustomobject]@{ State = 'Running' } + } else { + [pscustomobject]@{ State = 'Ready' } + } + } + $result = Wait-StorageSenseTask -GetTask $getTask -GetTaskInfo { param($t) $null } ` + -LastRunBefore ([datetime]'2026-07-22 10:00') -TimeoutSeconds 60 -CheckInterval 5 -Wait $script:noWait + $result.Outcome | Should -Be 'finished' + $result.Elapsed | Should -Be 15 + } + + It "reports 'vanished' with the real elapsed time, not the full timeout" { + # The bug this covers: the loop used to break on a disappearing task while leaving + # its finished flag false, so the caller announced "did not finish within 120 + # seconds" after ten - a number that never happened on that run. + $getTask = { + $script:wssCalls++ + if ($script:wssCalls -eq 1) { [pscustomobject]@{ State = 'Running' } } else { $null } + } + $result = Wait-StorageSenseTask -GetTask $getTask -GetTaskInfo { param($t) $null } ` + -LastRunBefore $null -TimeoutSeconds 120 -CheckInterval 5 -Wait $script:noWait + $result.Outcome | Should -Be 'vanished' + $result.Elapsed | Should -Be 10 + $result.Task | Should -BeNullOrEmpty + } + + It "reports 'timeout' when the task never stops running" { + $result = Wait-StorageSenseTask -GetTask { [pscustomobject]@{ State = 'Running' } } ` + -GetTaskInfo { param($t) $null } -LastRunBefore $null ` + -TimeoutSeconds 20 -CheckInterval 5 -Wait $script:noWait + $result.Outcome | Should -Be 'timeout' + $result.Elapsed | Should -Be 20 + } + + It "accepts a moved LastRunTime as evidence for a task too quick to be seen running" { + $result = Wait-StorageSenseTask -GetTask { [pscustomobject]@{ State = 'Ready' } } ` + -GetTaskInfo { param($t) [pscustomobject]@{ LastRunTime = [datetime]'2026-07-22 11:00' } } ` + -LastRunBefore ([datetime]'2026-07-22 10:00') -TimeoutSeconds 60 -CheckInterval 5 -Wait $script:noWait + $result.Outcome | Should -Be 'finished' + # Not before the 10-second mark: a task that was never seen running needs the + # evidence, and the first poll is too early to distinguish it from a slow start + $result.Elapsed | Should -Be 10 + } + + It "refuses to call a task finished when there is no baseline to compare against" { + # The fail-open this replaced: '-not $LastRunBefore' is TRUE when the pre-run read + # failed, so the disjunction short-circuited and ANY readable task info returned + # 'finished' for a task that may never have started. That false success went on to + # skip all 23 cleanmgr handlers. + $result = Wait-StorageSenseTask -GetTask { [pscustomobject]@{ State = 'Ready' } } ` + -GetTaskInfo { param($t) [pscustomobject]@{ LastRunTime = [datetime]'2026-07-22 11:00' } } ` + -LastRunBefore $null -TimeoutSeconds 60 -CheckInterval 5 -Wait $script:noWait + $result.Outcome | Should -Be 'unverifiable' + # The whole window is used to watch. The first repair returned at ten seconds, + # which gave a slow-starting task no chance to be seen and let Disk Cleanup start + # alongside it (raised in the next review round). + $result.Elapsed | Should -Be 60 + } + + It "still accepts being seen running as evidence when there is no baseline" { + # Direct observation needs no comparison: if the task was Running and then was not, + # it ran, whether or not its previous run time could be read. + $script:wssCalls = 0 + $getTask = { + $script:wssCalls++ + if ($script:wssCalls -lt 3) { [pscustomobject]@{ State = 'Running' } } else { [pscustomobject]@{ State = 'Ready' } } + } + $result = Wait-StorageSenseTask -GetTask $getTask -GetTaskInfo { param($t) $null } ` + -LastRunBefore $null -TimeoutSeconds 60 -CheckInterval 5 -Wait $script:noWait + $result.Outcome | Should -Be 'finished' + $result.Elapsed | Should -Be 15 + } + + It "actually waits when no wait scriptblock is injected" { + # Every other test here injects a no-op wait, so the default was unpinned: changing + # it to an empty block left all of them green while production spun through its + # whole timeout in microseconds and fell back to Disk Cleanup on every run. + $sw = [System.Diagnostics.Stopwatch]::StartNew() + $result = Wait-StorageSenseTask -GetTask { $null } -GetTaskInfo { param($t) $null } ` + -LastRunBefore $null -TimeoutSeconds 2 -CheckInterval 1 + $sw.Stop() + $result.Outcome | Should -Be 'vanished' + $sw.Elapsed.TotalMilliseconds | Should -BeGreaterThan 800 + } + + It "does not call a task finished when LastRunTime never moved" { + $sameTime = [datetime]'2026-07-22 10:00' + $result = Wait-StorageSenseTask -GetTask { [pscustomobject]@{ State = 'Ready' } } ` + -GetTaskInfo { param($t) [pscustomobject]@{ LastRunTime = $sameTime } } ` + -LastRunBefore $sameTime -TimeoutSeconds 20 -CheckInterval 5 -Wait $script:noWait + $result.Outcome | Should -Be 'timeout' + } +} + #endregion diff --git a/tools/Invoke-ReleaseCheck.ps1 b/tools/Invoke-ReleaseCheck.ps1 index 757315b..dbe585f 100644 --- a/tools/Invoke-ReleaseCheck.ps1 +++ b/tools/Invoke-ReleaseCheck.ps1 @@ -68,9 +68,23 @@ $version = if ($scriptText -match '(?m)^\$script:Version\s*=\s*"([\d.]+)"') { $M if (-not $version) { Add-Result -Name 'Version can be determined' -Passed $false -Detail 'no $script:Version assignment found' } else { + # v2.20: the two PSScriptInfo checks are scoped to the PSScriptInfo block instead of + # the whole file. Searching everywhere meant a stray copy of the release-note line + # elsewhere in the script satisfied the check on its own - which actually happened, + # pasted into Invoke-Phase's comment-based help by a version bump, and the gate would + # have stayed green with the real .RELEASENOTES entry deleted. A gate that an accident + # can satisfy is the same class of problem as a gate weaker than CI. + $psScriptInfo = if ($scriptText -match '(?s)<#PSScriptInfo(.*?)#>') { $Matches[1] } else { '' } + + # Narrowed once more in review: scoping to the whole PSScriptInfo block still let a + # "vX.Y:" line sitting under some OTHER field satisfy the check while .RELEASENOTES + # itself was empty. Take the section, not the block. + $releaseNotes = if ($psScriptInfo -match '(?s)\.RELEASENOTES(.*?)(?=\r?\n\s*\.[A-Z]|$)') { $Matches[1] } else { '' } + $versionSites = @( - @{ What = 'PSScriptInfo .VERSION'; Ok = $scriptText -match "(?m)^\.VERSION\s+$([regex]::Escape($version))\s*$" } - @{ What = '.RELEASENOTES first line'; Ok = $scriptText -match "(?m)^\s*v$([regex]::Escape($version)):" } + @{ What = 'PSScriptInfo block is present'; Ok = [bool]$psScriptInfo } + @{ What = 'PSScriptInfo .VERSION'; Ok = $psScriptInfo -match "(?m)^\.VERSION\s+$([regex]::Escape($version))\s*$" } + @{ What = '.RELEASENOTES first line'; Ok = $releaseNotes -match "(?m)^\s*v$([regex]::Escape($version)):" } @{ What = 'SYNOPSIS'; Ok = $scriptText -match "Maintenance Script v$([regex]::Escape($version))" } @{ What = 'NOTES Version'; Ok = $scriptText -match "(?m)^\s*Version:\s+$([regex]::Escape($version))\s*$" } @{ What = 'NOTES Changes in'; Ok = $scriptText -match "Changes in $([regex]::Escape($version)):" } @@ -143,19 +157,33 @@ if (Get-Module -ListAvailable PSScriptAnalyzer) { # --- 6. Pester, and the count the docs claim --------------------------------- # Counting It blocks by hand is wrong here: -ForEach multiplies them. +# +# v2.20: runs through tools/Invoke-Tests.ps1, the same script CI runs, so the gate cannot +# execute a different Pester version or a laxer rule than CI. It used to call Invoke-Pester +# directly with no version bound while CI pinned an upper bound, and PowerShell loads the +# highest installed version - so installing Pester 6 would silently split the two. $pesterCount = $null -if (Get-Module -ListAvailable Pester) { - $pester = Invoke-Pester -Path (Join-Path $repoRoot 'tests') -PassThru -Output None +$pester = $null +try { + $pester = & (Join-Path $PSScriptRoot 'Invoke-Tests.ps1') -Quiet +} catch { + Add-Result -Name 'Pester available in the supported range' -Passed $false -Detail "$_" +} +if ($pester) { $pesterCount = $pester.TotalCount # Skipped tests count as a failure of the gate. The integration suite - the only # layer that executes real cleanup code - skips itself without administrator rights, # and a release must never go out on "176 of 204 passed, the rest silently absent". $notRun = $pester.SkippedCount + $pester.NotRunCount + # v2.20: a test file that fails to LOAD contributes no tests, so the three counters + # above stay at zero while its whole coverage is gone (measured on Pester 5.7.1). + $failedContainers = $pester.FailedContainersCount Add-Result -Name "Pester: $($pester.PassedCount)/$($pester.TotalCount) passed, none skipped" ` - -Passed ($pester.FailedCount -eq 0 -and $notRun -eq 0) ` + -Passed ($pester.FailedCount -eq 0 -and $notRun -eq 0 -and $failedContainers -eq 0) ` -Detail $( if ($pester.FailedCount) { ($pester.Failed | Select-Object -First 3 | ForEach-Object { $_.ExpandedPath }) -join '; ' } elseif ($notRun) { "$notRun тест(ов) не выполнено - нужны права администратора" } + elseif ($failedContainers) { "$failedContainers тест-файл(ов) не загрузились - их тесты не выполнялись" } else { '' }) $countClaims = @( @@ -168,8 +196,6 @@ if (Get-Module -ListAvailable Pester) { } Add-Result -Name "Docs agree that there are $pesterCount tests" -Passed (-not $wrongCounts) ` -Detail $(if ($wrongCounts) { "устарел счётчик: $($wrongCounts -join ', ')" } else { '' }) -} else { - Add-Result -Name 'Pester available' -Passed $false -Detail 'module not installed' } # --- 7. Smoke run (ReportOnly) ----------------------------------------------- @@ -189,13 +215,42 @@ try { Add-Result -Name 'Working tree is clean' -Passed ($dirty.Count -eq 0) ` -Detail $(if ($dirty) { "$($dirty.Count) файл(ов) не закоммичено" } else { '' }) - $branchInfo = (& git status -sb | Select-Object -First 1) - # "## main" without "...origin/main" means there is no upstream at all - that is not - # "in sync", it is "nothing to sync with" - $hasUpstream = $branchInfo -match '\.\.\.' - $notPushed = $branchInfo -match '\[(ahead|behind)' - Add-Result -Name 'Branch is in sync with origin' -Passed ($hasUpstream -and -not $notPushed) ` - -Detail $(if (-not $hasUpstream) { "нет upstream: $branchInfo" } elseif ($notPushed) { $branchInfo } else { '' }) + # v2.20: this used to read `git status -sb`, which compares against the LOCAL + # remote-tracking ref. Without a fetch it confirms a state that may no longer exist: + # the gate would report "in sync with origin" while origin had already moved. Ask the + # remote, and fail closed when it cannot be asked - an unverifiable claim is not a + # passing check. + $fetchOut = & git fetch --prune 2>&1 + $fetchOk = $LASTEXITCODE -eq 0 + + $upstream = & git rev-parse --abbrev-ref --symbolic-full-name '@{upstream}' 2>$null + # No upstream at all is not "in sync", it is "nothing to sync with" + $hasUpstream = ($LASTEXITCODE -eq 0) -and $upstream + + # Counted, not assumed: if rev-list itself fails or answers something unparseable, the + # zeros it was initialised with would read as "in sync" - the same shape of lie this + # whole check was rewritten to remove (caught in review). + $ahead = $behind = 0 + $countsOk = $false + if ($hasUpstream) { + $counts = @((& git rev-list --left-right --count 'HEAD...@{upstream}') -split '\s+' | Where-Object { $_ -ne '' }) + if ($LASTEXITCODE -eq 0 -and $counts.Count -ge 2 -and + $counts[0] -match '^\d+$' -and $counts[1] -match '^\d+$') { + $ahead = [int]$counts[0] + $behind = [int]$counts[1] + $countsOk = $true + } + } + + $syncDetail = + if (-not $fetchOk) { "git fetch не удался - состояние origin неизвестно: $(($fetchOut | Select-Object -Last 1))" } + elseif (-not $upstream) { 'у ветки нет upstream' } + elseif (-not $countsOk) { 'git rev-list не дал сравнимого ответа - расхождение с origin не проверено' } + elseif ($ahead -or $behind) { "ahead $ahead, behind $behind (upstream: $upstream)" } + else { '' } + + Add-Result -Name 'Branch is in sync with origin (verified against the remote)' ` + -Passed ($fetchOk -and $hasUpstream -and $countsOk -and $ahead -eq 0 -and $behind -eq 0) -Detail $syncDetail } finally { Pop-Location } @@ -203,7 +258,12 @@ try { # --- 9. Stand run (opt-in) --------------------------------------------------- if ($IncludeStand) { Write-Host " ...running the stand VM, this takes a few minutes" -ForegroundColor DarkGray - $standOut = & pwsh -NoProfile -File (Join-Path $PSScriptRoot 'proxmox\Invoke-StandTest.ps1') -Mode Full -Source local 2>&1 + # Join-Path per segment: a backslash inside the argument is a literal character on + # Linux/macOS, not a separator, so "proxmox\Invoke-StandTest.ps1" would be one odd + # filename there. This gate is not deployed to the Proxmox host today (only the four + # scripts in Deploy-StandRunner are), so this is hygiene rather than a live defect. + $standScript = Join-Path (Join-Path $PSScriptRoot 'proxmox') 'Invoke-StandTest.ps1' + $standOut = & pwsh -NoProfile -File $standScript -Mode Full -Source local 2>&1 $standPassed = $LASTEXITCODE -eq 0 Add-Result -Name 'Stand test (full run on a VM)' -Passed $standPassed ` -Detail (($standOut | Where-Object { $_ -match 'STAND TEST|freed|warnings' } | Select-Object -Last 2) -join ' | ') diff --git a/tools/Invoke-SmokeTest.ps1 b/tools/Invoke-SmokeTest.ps1 index ae198fb..aa07d38 100644 --- a/tools/Invoke-SmokeTest.ps1 +++ b/tools/Invoke-SmokeTest.ps1 @@ -23,7 +23,10 @@ [CmdletBinding()] param( [string]$ScriptPath = (Join-Path $PSScriptRoot '..' 'WinClean.ps1'), - [string]$OutDir = (Join-Path $env:TEMP "WinCleanSmoke_$(Get-Date -Format 'yyyyMMdd_HHmmss')"), + # GetTempPath() instead of $env:TEMP: the variable is Windows-specific and an empty + # one would resolve the join to a drive root (see the empty-env-var trap this project + # already hit once). Same value on Windows, defined everywhere else. + [string]$OutDir = (Join-Path ([System.IO.Path]::GetTempPath()) "WinCleanSmoke_$(Get-Date -Format 'yyyyMMdd_HHmmss')"), [switch]$IncludeUpdates ) diff --git a/tools/Invoke-Tests.ps1 b/tools/Invoke-Tests.ps1 new file mode 100644 index 0000000..2fcf1a8 --- /dev/null +++ b/tools/Invoke-Tests.ps1 @@ -0,0 +1,113 @@ +#Requires -Version 7.1 +<# +.SYNOPSIS + Single source of truth for how this repository runs its Pester suite. + +.DESCRIPTION + Both the CI test job and tools/Invoke-ReleaseCheck.ps1 call this script, for the same + reason tools/Invoke-Lint.ps1 exists: a gate that runs a different configuration from + CI can report green while CI fails, and a gate that is weaker than CI is worse than + no gate because it is believed. + + Two things were duplicated before and are now stated once: + + - The supported Pester range. CI installed Pester with an upper bound while the gate + ran whatever `Get-Module -ListAvailable` returned, and PowerShell loads the highest + version present. With Pester 6 published, a single `Install-Module Pester` on the + workstation would have moved the gate to a major version CI never runs. + - The rule that a skipped test fails the run. The integration suite is the only layer + that executes real cleanup code and it skips itself without administrator rights, + so "passed, the rest silently absent" must never count as success. + +.PARAMETER Quiet + Return the Pester result object and print nothing. Used by the release gate, which + prints its own PASS/FAIL line. + +.PARAMETER FailOnProblem + Exit with code 1 when a test failed or did not run. Used by CI. + +.PARAMETER RequiredPesterRange + Print the supported version range as " " and exit without importing + anything. CI uses this to install exactly the range this script will accept. + +.PARAMETER ResultPath + Optional NUnit XML result file (CI publishes it as an artifact). + +.OUTPUTS + With -Quiet, the Pester result object. +#> +[CmdletBinding()] +param( + [switch]$Quiet, + [switch]$FailOnProblem, + [switch]$RequiredPesterRange, + [string]$ResultPath +) + +$ErrorActionPreference = 'Stop' +$repoRoot = Split-Path $PSScriptRoot -Parent + +# Pester 6 changes semantics; it must arrive deliberately, not by whoever ran +# Install-Module last. Raising this bound is a decision, not a side effect. +$minPester = '5.0' +$maxPester = '5.99.99' + +# Answered before anything is imported, so CI can ask for the range and then install it +if ($RequiredPesterRange) { return "$minPester $maxPester" } + +$module = Get-Module -ListAvailable Pester | + Where-Object { $_.Version -ge [version]$minPester -and $_.Version -le [version]$maxPester } | + Sort-Object Version -Descending | + Select-Object -First 1 + +if (-not $module) { + $found = (Get-Module -ListAvailable Pester | ForEach-Object { $_.Version.ToString() }) -join ', ' + throw "Pester $minPester..$maxPester is required (installed: $(if ($found) { $found } else { 'none' })). " + + "Install it with: Install-Module Pester -MinimumVersion $minPester -MaximumVersion $maxPester -Scope CurrentUser -Force" +} +Import-Module $module.Path -Force -ErrorAction Stop + +$config = New-PesterConfiguration +$config.Run.Path = Join-Path $repoRoot 'tests' +$config.Run.PassThru = $true +$config.Output.Verbosity = if ($Quiet) { 'None' } else { 'Detailed' } +if ($ResultPath) { + $config.TestResult.Enabled = $true + $config.TestResult.OutputFormat = 'NUnitXml' + $config.TestResult.OutputPath = $ResultPath +} + +$result = Invoke-Pester -Configuration $config + +# A test that did not run verified nothing. GitHub Windows runners are elevated, so a +# skip there means something is actually wrong rather than "no admin rights". +$notRun = $result.SkippedCount + $result.NotRunCount + +# A container that fails during DISCOVERY contributes no tests at all, so counting tests +# alone cannot see it. Measured on Pester 5.7.1 with a deliberately unparsable test file: +# Result=Failed and FailedContainersCount=1, while FailedCount, SkippedCount and +# NotRunCount were all 0. Without this, a syntax error in a test file would delete that +# file's entire coverage and still leave CI green. +$failedContainers = $result.FailedContainersCount + +if (-not $Quiet) { + Write-Host "" + Write-Host "Pester $($module.Version): $($result.PassedCount)/$($result.TotalCount) passed, $($result.FailedCount) failed, $notRun did not run, $failedContainers file(s) failed to load" + if ($result.FailedCount -gt 0) { + Write-Host "::error::$($result.FailedCount) test(s) failed" + } + if ($notRun -gt 0) { + Write-Host "::error::$notRun test(s) did not run - coverage is missing" + } + if ($failedContainers -gt 0) { + Write-Host "::error::$failedContainers test file(s) failed to load - their tests never ran" + } +} + +if ($Quiet) { + # Data for the release gate. Emitting it in verbose mode too would dump the whole + # result object onto the console after the summary above. + $result +} + +if ($FailOnProblem -and ($result.FailedCount -gt 0 -or $notRun -gt 0 -or $failedContainers -gt 0)) { exit 1 } diff --git a/tools/proxmox/Deploy-StandRunner.ps1 b/tools/proxmox/Deploy-StandRunner.ps1 index e73a304..d7304bd 100644 --- a/tools/proxmox/Deploy-StandRunner.ps1 +++ b/tools/proxmox/Deploy-StandRunner.ps1 @@ -95,7 +95,13 @@ rm -f /tmp/pwsh.tar.gz /tmp/pwsh-release.json # Remote configs are synced: stale stand.config*.json from renamed/removed # stands would otherwise stay in the nightly matrix forever. Write-Host "[2/4] Copying harness and configs..." -ForegroundColor Cyan +# v2.20: the exit code was discarded. If the rm half failed (permissions, a busy file), +# the upload below would proceed and deployment would report success while a renamed or +# removed stand config stayed behind - the nightly matrix would keep driving an obsolete VM. $null = ssh -o BatchMode=yes $target "mkdir -p $remoteDir/results && rm -f $remoteDir/stand.config*.json" +if ($LASTEXITCODE -ne 0) { + throw "Remote preparation failed (exit $LASTEXITCODE): could not create $remoteDir/results or clear old stand configs" +} $files = @( (Join-Path $PSScriptRoot 'StandCommon.ps1') diff --git a/tools/proxmox/New-StandVM.ps1 b/tools/proxmox/New-StandVM.ps1 index 83def08..90f45e1 100644 --- a/tools/proxmox/New-StandVM.ps1 +++ b/tools/proxmox/New-StandVM.ps1 @@ -62,11 +62,19 @@ $rel = Invoke-RestMethod 'https://api.github.com/repos/PowerShell/PowerShell/rel $asset = $rel.assets | Where-Object { $_.name -like 'PowerShell-*-win-x64.msi' } | Select-Object -First 1 $msi = 'C:\Windows\Temp\ps7.msi' Invoke-WebRequest -Uri $asset.browser_download_url -OutFile $msi -Start-Process msiexec.exe -ArgumentList "/i `"$msi`" /quiet /norestart" -Wait -Test-Path 'C:\Program Files\PowerShell\7\pwsh.exe' +# v2.20: -PassThru so the MSI result is actually available. Existence of pwsh.exe was the +# only check before, and msiexec can lay down a partial install and return 1603 - that +# broken installation would then be snapshotted as the stand baseline. +$msiProc = Start-Process msiexec.exe -ArgumentList "/i `"$msi`" /quiet /norestart" -Wait -PassThru +# 0 = success, 3010 = success but a reboot is pending +if ($msiProc.ExitCode -notin 0, 3010) { "MSIEXEC_FAILED:$($msiProc.ExitCode)"; exit 1 } +# Existence is not enough either - run it and require the version this stand needs +$ver = & 'C:\Program Files\PowerShell\7\pwsh.exe' -NoProfile -Command '$PSVersionTable.PSVersion.ToString()' +if ([version]$ver -lt [version]'7.1') { "PWSH_TOO_OLD:$ver"; exit 1 } +"PWSH_OK:$ver" '@ - if ($install.Output -notmatch 'True') { - throw "PowerShell 7 installation failed: $($install.Error)" + if ($install.Output -notmatch 'PWSH_OK:') { + throw "PowerShell 7 installation failed: $($install.Output) $($install.Error)" } Write-Host "PowerShell 7 installed." -ForegroundColor Green } else {