Fix reinstall hardening modules - #16
Conversation
|
Warning Review limit reached
More reviews will be available in 5 minutes and 58 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR refactors the declarative Windows setup automation into a modular, state-driven architecture. It introduces PowerShell modules for bootstrap state management, WinGet package installation with verification and retry, backup/restore path remapping, declarative registry configuration, and ISO payload validation. Progress is persisted to ChangesModular Architecture and Infrastructure
WinGet Installation with Progress and Elevated Retry
Backup and Restore Path Remapping
ISO Build System with Module Payload
Bootstrap Execution Integration
Documentation, Testing, and Quality Assurance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1850901adb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| $taskUser = if ($env:USERDOMAIN) { "$($env:USERDOMAIN)\$($env:USERNAME)" } else { $env:USERNAME } | ||
| $taskCommand = "powershell.exe -NoProfile -ExecutionPolicy Bypass -File `"$runnerPath`"" | ||
|
|
||
| $createOutput = @(schtasks.exe /Create /F /TN $taskName /SC ONCE /ST $taskTime /TR $taskCommand /RL LIMITED /RU $taskUser 2>&1) |
There was a problem hiding this comment.
When an admin WinGet install reports that it cannot run elevated, this retry path creates a task with /RU $taskUser but no /RP, /NP, or /IT; Microsoft's schtasks /create documentation says /rp is the password for the /ru account and that omitting it prompts for that user's run-as password (see the /rp parameter and alternate-account examples at https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/schtasks-create). In the unattended/elevated bootstrap flow that prompt is not handled, so packages that require the non-admin retry can hang or fail instead of continuing the reinstall.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build-iso.ps1 (1)
103-170: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winLocal functions overridden by module.
Get-UnattendSetupFileReferencesandValidate-StagedIsoLayoutare defined locally butStagedSetupPayload.ps1(dot-sourced at lines 172-176) also defines them. The module's versions will override these, making this local code dead. Either remove the local definitions or remove the module loading.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-iso.ps1` around lines 103 - 170, The local functions Get-UnattendSetupFileReferences and Validate-StagedIsoLayout are duplicated by the dot-sourced StagedSetupPayload.ps1 and thus get overridden; fix by keeping only one authoritative definition: either delete these local function definitions and rely on the implementations in StagedSetupPayload.ps1, or stop dot-sourcing that file and instead import/namespace or rename the module’s functions (or rename these local functions) so there is no collision; update any call sites to use the chosen implementation (referencing Get-UnattendSetupFileReferences and Validate-StagedIsoLayout) and ensure only one definition remains in scope.
🧹 Nitpick comments (4)
tests/Quality.Tests.ps1 (2)
25-25: ⚡ Quick winRename variable to avoid shadowing
$matchesautomatic variable.PSScriptAnalyzer correctly flags that
$matchesis a PowerShell automatic variable set by the-matchoperator. While the code works in this scope, shadowing automatic variables is poor practice and can cause confusion.♻️ Proposed fix
- $matches = foreach ($file in $files) { + $conflictMarkers = foreach ($file in $files) { Select-String -LiteralPath $file.FullName -Pattern '^(<<<<<<<|\|\|\|\|\|\|\||=======|>>>>>>>)' } - $matches | Should -BeNullOrEmpty + $conflictMarkers | Should -BeNullOrEmpty🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Quality.Tests.ps1` at line 25, The variable $matches is shadowing PowerShell's automatic $matches; rename the loop-assigned variable (e.g., change "$matches = foreach ($file in $files) {" to "$fileMatches = foreach ($file in $files) {") and update all references inside the foreach block and any later uses from $matches to the new name (e.g., $fileMatches) to avoid colliding with the automatic $matches variable.Source: Linters/SAST tools
22-22: 💤 Low valueConsider including
.ymlfiles in conflict marker check.The file type filter includes
.ps1,.md,.json, and.xml, but excludes.ymlfiles. GitHub Actions workflows (.github/workflows/*.yml) are text files that could also contain unresolved merge conflicts. Consider adding.ymlto the extension list unless the omission is intentional.♻️ Proposed enhancement
Where-Object { $_.FullName -notmatch '\\.git\\' -and - $_.Extension -in @(".ps1", ".md", ".json", ".xml") + $_.Extension -in @(".ps1", ".md", ".json", ".xml", ".yml") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Quality.Tests.ps1` at line 22, The conflict-check filter in tests/Quality.Tests.ps1 uses the expression $_.Extension -in @(".ps1", ".md", ".json", ".xml") and omits YAML workflow files; update that array in the test to include ".yml" (and optionally ".yaml") so GitHub Actions workflow files are scanned for conflict markers by the conflict marker check in this script.modules/BackupManifest.ps1 (1)
64-64: 💤 Low valueInconsistent
TrimStartargument.Line 64 uses
TrimStart('\\')(two backslash chars) while line 96 usesTrimStart('\')(single). Both work sinceTrimStartremoves any characters in the set, but the double-backslash is misleading. Consider usingTrimStart('\')for consistency with the rest of the file.Suggested fix
- $relativePath = $expandedPath.Substring($ManifestBackupRoot.Length).TrimStart('\\') + $relativePath = $expandedPath.Substring($ManifestBackupRoot.Length).TrimStart('\')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/BackupManifest.ps1` at line 64, The TrimStart call that computes $relativePath uses TrimStart('\\') which is inconsistent and misleading compared to other uses (e.g., the TrimStart('\') at line 96); update the TrimStart invocation on the $relativePath assignment to use a single-escaped backslash character (TrimStart('\')) to match the rest of the file and remove the misleading double-backslash.apply-registry.ps1 (1)
20-42: Dead code after module delegation inapply-registry.ps1
Normalize-RegistryPathandConvert-RegistryTypeare defined inapply-registry.ps1(lines 20-42) but never called: whenInvoke-DeclarativeConfigexists the script immediately returns (lines 44-46), otherwise it throws (line 48). These helpers are instead implemented and used inmodules/DeclarativeConfig.ps1, so the duplicates here are redundant—removing them would reduce maintenance/confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apply-registry.ps1` around lines 20 - 42, Remove the unused duplicate helper functions Normalize-RegistryPath and Convert-RegistryType from apply-registry.ps1: locate and delete both function definitions (Normalize-RegistryPath { ... } and Convert-RegistryType { ... }) since Invoke-DeclarativeConfig short-circuits this script and the canonical implementations live in modules/DeclarativeConfig.ps1; after removal, verify there are no other references to these symbols in the file and run the test/validation steps to ensure no regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/TROUBLESHOOTING.md`:
- Around line 131-133: The new fenced code blocks in TROUBLESHOOTING.md (e.g.,
the block containing "WinGet reported success but winget list did not verify the
package" and the other blocks noted around the same sections) are missing a
language identifier; update each triple-backtick fence to include `text` (for
example change ``` to ```text) so MD040 is satisfied and plain output snippets
are explicitly marked.
In `@modules/BootstrapRun.ps1`:
- Around line 184-186: The loop building $summaryLines currently prints raw
status strings from $SummaryItems; change it to map those statuses to the
required symbols (OK -> ✓, WARN -> ⚠, FAIL -> ✗) before formatting. Inside the
foreach over $SummaryItems, derive a $symbol using a switch or hashtable keyed
by $item.Status and then use that $symbol in the format string when appending to
$summaryLines; update the reference to $item.Status in the format to $symbol so
the summary shows ✓/⚠/✗ for completed/skipped/failed entries.
In `@modules/StagedSetupPayload.ps1`:
- Line 85: The call to Write-Success in StagedSetupPayload.ps1 may fail when the
module is used standalone because Write-Success isn't guaranteed to be defined;
update the end of the validation flow to check for the presence of Write-Success
(e.g., via Get-Command -Name Write-Success -ErrorAction SilentlyContinue) and
only call it if present, otherwise emit an equivalent fallback message (using
Write-Host or Write-Output) so the module runs safely outside build-iso.ps1;
reference the Write-Success invocation in StagedSetupPayload.ps1 to locate where
to add the conditional guard and fallback.
In `@modules/WinGetInstall.ps1`:
- Around line 339-345: The current check uses -not $missingPackages which fails
for an empty generic list; replace the condition with an explicit count check
(e.g. use $missingPackages.Count -eq 0, or to be defensive: if (-not
$missingPackages -or $missingPackages.Count -eq 0)) so the fast-path that calls
Write-Log, Set-Content (MarkerPath, appsHash), Add-SummaryItem (SummaryStep) and
Set-StepState (StepId) executes when no packages are missing.
In `@restore-backup.ps1`:
- Around line 112-149: The local Resolve-BackupSourcePath function duplicates
the one provided by the BackupManifest.ps1 module and is being overridden;
remove the local Resolve-BackupSourcePath definition and instead ensure the
BackupManifest.ps1 module is dot-sourced before any code that calls
Resolve-BackupSourcePath so the module implementation is used consistently;
update code ordering to load BackupManifest.ps1 early (or keep only one
canonical implementation in BackupManifest.ps1) and delete the redundant local
function block.
In `@tests/Quality.Tests.ps1`:
- Line 8: The path filter in the Where-Object pipeline uses a Windows-only
backslash pattern (the expression $_.FullName -notmatch '\\.git\\'), so update
the regex to handle both POSIX and Windows separators; replace the pattern with
a cross-platform check such as $_.FullName -notmatch '([\\/]\.git([\\/]|$))' in
the Where-Object clause to exclude .git directories on all platforms.
- Line 21: The -notmatch regex is Windows-specific: $_.FullName -notmatch
'\\.git\\' won't exclude .git directories on Unix. Update the match used with
$_.FullName to be platform-agnostic (e.g., match either slash direction or check
path segments) so .git is excluded on all OSes; replace the '\\.git\\' literal
with a cross-platform pattern (e.g., use a regex that allows both '/' and '\' or
check Path.DirectorySeparatorChar or split the path) where the -notmatch is
applied.
---
Outside diff comments:
In `@build-iso.ps1`:
- Around line 103-170: The local functions Get-UnattendSetupFileReferences and
Validate-StagedIsoLayout are duplicated by the dot-sourced
StagedSetupPayload.ps1 and thus get overridden; fix by keeping only one
authoritative definition: either delete these local function definitions and
rely on the implementations in StagedSetupPayload.ps1, or stop dot-sourcing that
file and instead import/namespace or rename the module’s functions (or rename
these local functions) so there is no collision; update any call sites to use
the chosen implementation (referencing Get-UnattendSetupFileReferences and
Validate-StagedIsoLayout) and ensure only one definition remains in scope.
---
Nitpick comments:
In `@apply-registry.ps1`:
- Around line 20-42: Remove the unused duplicate helper functions
Normalize-RegistryPath and Convert-RegistryType from apply-registry.ps1: locate
and delete both function definitions (Normalize-RegistryPath { ... } and
Convert-RegistryType { ... }) since Invoke-DeclarativeConfig short-circuits this
script and the canonical implementations live in modules/DeclarativeConfig.ps1;
after removal, verify there are no other references to these symbols in the file
and run the test/validation steps to ensure no regressions.
In `@modules/BackupManifest.ps1`:
- Line 64: The TrimStart call that computes $relativePath uses TrimStart('\\')
which is inconsistent and misleading compared to other uses (e.g., the
TrimStart('\') at line 96); update the TrimStart invocation on the $relativePath
assignment to use a single-escaped backslash character (TrimStart('\')) to match
the rest of the file and remove the misleading double-backslash.
In `@tests/Quality.Tests.ps1`:
- Line 25: The variable $matches is shadowing PowerShell's automatic $matches;
rename the loop-assigned variable (e.g., change "$matches = foreach ($file in
$files) {" to "$fileMatches = foreach ($file in $files) {") and update all
references inside the foreach block and any later uses from $matches to the new
name (e.g., $fileMatches) to avoid colliding with the automatic $matches
variable.
- Line 22: The conflict-check filter in tests/Quality.Tests.ps1 uses the
expression $_.Extension -in @(".ps1", ".md", ".json", ".xml") and omits YAML
workflow files; update that array in the test to include ".yml" (and optionally
".yaml") so GitHub Actions workflow files are scanned for conflict markers by
the conflict marker check in this script.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6eef5b72-19da-4e6f-a753-7193b4d2c84a
📒 Files selected for processing (17)
CONTEXT.mdapply-registry.ps1bootstrap.ps1build-iso.ps1docs/TROUBLESHOOTING.mdmodules/BackupManifest.ps1modules/BootstrapRun.ps1modules/DeclarativeConfig.ps1modules/StagedSetupPayload.ps1modules/WinGetInstall.ps1preflight-backup.ps1restore-backup.ps1tests/ArchitectureModules.Tests.ps1tests/BackupRestore.Tests.ps1tests/Bootstrap.Tests.ps1tests/BuildIso.Tests.ps1tests/Quality.Tests.ps1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e294c901ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| [Parameter(Mandatory)][object[]]$Rules, | ||
| [Parameter(Mandatory)][object[]]$RepoFiles, | ||
| [Parameter(Mandatory)][object]$Exports, | ||
| [Parameter(Mandatory)][object[]]$Failures |
There was a problem hiding this comment.
When a backup completes without failed rules, preflight-backup.ps1 calls New-BackupManifest -Failures @($failedRules) with an empty collection. Mandatory collection parameters reject empty arrays unless they opt in with AllowEmptyCollection, so the normal successful backup path can fail before writing backup-manifest.json; the same pattern can also affect RepoFiles when repo-file backup is disabled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
declarative-windows/bootstrap.ps1
Line 1084 in 6801aab
Preserve registry value kinds when updating tweaks
When these tweak values already exist with the wrong registry kind (for example a REG_SZ "0" left by a prior manual tweak), this now only casts the data and calls Set-ItemProperty without -Type, so PowerShell updates the value but does not force it back to REG_DWORD/REG_SZ; Windows policies such as Explorer DWORDs can then keep being ignored despite the data looking correct. The Registry provider does support Set-ItemProperty -Type <RegistryValueKind> on Windows (see Microsoft docs for the Registry-provider parameter), so the previous type argument should be preserved or replaced with an equivalent New-ItemProperty -PropertyType ... -Force path.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2786064d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue | ||
| } | ||
|
|
||
| Set-ItemProperty -LiteralPath $registryPath -Name $entry.name -Value $desiredValue -Force |
There was a problem hiding this comment.
When a customized config\registry.json contains a DWord entry that needs to be created, this refactor converts the value to an int but no longer passes the converted $valueType to the Registry provider. The previous implementation supplied the requested type, and the bootstrap tweak path still uses New-ItemProperty -PropertyType for the same reason; without doing that here, new registry values can be created with the wrong kind and Windows policies/tweaks that require REG_DWORD may be ignored even though the script reports success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2786064d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue | ||
| } | ||
|
|
||
| Set-ItemProperty -LiteralPath $registryPath -Name $entry.name -Value $desiredValue -Force |
There was a problem hiding this comment.
When registry.json contains a DWORD entry whose value is missing or has the wrong kind, the new apply-registry.ps1 path delegates here and this call no longer passes the computed $valueType. That lets the registry provider infer/create the value kind instead of enforcing the declared type, so the fallback can report success while leaving policies that require REG_DWORD as a string/default-kind value and the bootstrap then marks the registry step done.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3b8843bb9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| return $packageIds | Sort-Object -Unique |
There was a problem hiding this comment.
When apps.json or optional-apps.json contains exactly one PackageIdentifier, this pipeline return is assigned as a scalar string in Invoke-WingetManifestInstall; the later $packageIds[$index] access then indexes the string, so an id like Microsoft.PowerToys becomes M and WinGet checks/installs the wrong package. Wrap the sorted result or the call site in an array so single-entry manifests are processed as one package id.
Useful? React with 👍 / 👎.
1 participant
Summary
Testing
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests