Always display Windows ESP error when software install fails#47522
Always display Windows ESP error when software install fails#47522getvictor wants to merge 4 commits into
Conversation
|
@coderabbitai full review |
|
/agentic_review |
✅ Action performedFull review finished. |
Code Review by Qodo
1.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis pull request implements soft-block finalization for Windows setup experience (ESP) when software installations fail without requiring all software to succeed. When a setup-experience item fails and Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/service/microsoft_mdm.go (1)
2342-2344:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve software-failure detection when the 3-hour timeout has already elapsed.
Line 2342 short-circuits the Stage 3 scan whenever
timedOutis true, sohasSoftwareFailurestays false even if some setup-experience rows are already inFailure. That makes therequire_all_software_windows=falsepath fall through to silent release instead of surfacing the continuable ESP error, which contradicts the feature goal of always showing an ESP error when software install failures occurred. Re-list or inspect terminal Stage 3 results before decidingshouldWarn/release on timeout so timeout+failure still lands on the failure UI.Also applies to: 2554-2561
🤖 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 `@server/service/microsoft_mdm.go` around lines 2342 - 2344, The timeout short-circuits the Stage 3 scan (variable timedOut computed from device.AwaitingConfigurationAt) and returns before evaluating hasSoftwareFailure, so setups with terminal ESP rows in Failure are ignored; change the logic so that when timedOut is true you still inspect/re-list the Stage 3 setup-experience rows (or otherwise evaluate terminal Stage 3 results) to set hasSoftwareFailure before deciding shouldWarn/release for the require_all_software_windows=false path—ensure the check that computes hasSoftwareFailure runs even when timedOut is true (same fix applied to the similar block around the code handling lines 2554-2561).
🤖 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 `@server/service/microsoft_mdm_test.go`:
- Around line 1920-1922: Update the test expectation so the continuable error
message exactly matches the acceptance criteria: modify the expected string used
in the assertion (the literal compared to errCmd.Items[0].Data.Content or the
variable CustomErrorText used in this test) to include the missing sentence "If
unavailable, contact your IT admin." and restore the exact reset phrasing
required by the spec; ensure the full multi-sentence message matches the product
text character-for-character so the assertion against
errCmd.Items[0].Data.Content passes.
In `@server/service/microsoft_mdm.go`:
- Around line 2566-2578: The switch currently uses
microsoft_mdm.ESPTimeoutErrorText / ESPSoftwareFailureErrorText and
ESPSoftwareFailureContinuableErrorText but the actual user copy is wrong; update
the hard-block branch (shouldBlock) to pass the exact required hard-block string
(or update ESPTimeoutErrorText/ESPSoftwareFailureErrorText constants) and update
the soft-block branch (shouldWarn) so ESPSoftwareFailureContinuableErrorText (or
its constant) includes the final sentence "If unavailable, contact your IT
admin." Ensure you only change the text constants or the strings passed to
buildESPBlockCommands(provID, ..., espBlockButtonsReset) and
buildESPBlockCommands(provID, ..., espBlockButtonsResetAndContinue) so the
displayed copy matches the spec.
---
Outside diff comments:
In `@server/service/microsoft_mdm.go`:
- Around line 2342-2344: The timeout short-circuits the Stage 3 scan (variable
timedOut computed from device.AwaitingConfigurationAt) and returns before
evaluating hasSoftwareFailure, so setups with terminal ESP rows in Failure are
ignored; change the logic so that when timedOut is true you still
inspect/re-list the Stage 3 setup-experience rows (or otherwise evaluate
terminal Stage 3 results) to set hasSoftwareFailure before deciding
shouldWarn/release for the require_all_software_windows=false path—ensure the
check that computes hasSoftwareFailure runs even when timedOut is true (same fix
applied to the similar block around the code handling lines 2554-2561).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c83690ce-e907-490d-9504-1c65be52f9bb
📒 Files selected for processing (5)
server/mdm/microsoft/esp_csp.goserver/mdm/microsoft/esp_csp_test.goserver/service/microsoft_mdm.goserver/service/microsoft_mdm_property_test.goserver/service/microsoft_mdm_test.go
There was a problem hiding this comment.
Pull request overview
This PR updates the Windows Enrollment Status Page (ESP) finalization logic so that when setup-experience software installs fail and require_all_software_windows is false, Windows still shows the ESP failure UI with a “Continue anyway” option and a dynamic list of failed software names.
Changes:
- Add a “soft block” ESP path (failure UI + Continue anyway) when software fails but
require_all_software_windows=false. - Introduce helper formatting for dynamic failed-software lists with truncation.
- Expand unit + property-based tests to cover warn/soft-block behavior and invariants.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/microsoft_mdm.go | Adds soft-block decision path and configurable ESP recovery buttons (Reset vs Reset+Continue). |
| server/service/microsoft_mdm_test.go | Adds example-based tests for soft-block behavior, in-flight waiting, and timeout release behavior. |
| server/service/microsoft_mdm_property_test.go | Extends PBT model to include warn/soft-block decision + command-shape expectations. |
| server/mdm/microsoft/esp_csp.go | Adds dynamic error-text builder for continuable software failures (name list + truncation). |
| server/mdm/microsoft/esp_csp_test.go | Adds tests for name-joining and truncation behavior for the new error text helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47522 +/- ##
==========================================
+ Coverage 67.19% 67.20% +0.01%
==========================================
Files 3489 3492 +3
Lines 228536 228774 +238
Branches 11909 11909
==========================================
+ Hits 153557 153745 +188
- Misses 61152 61187 +35
- Partials 13827 13842 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
|
/agentic_review |
✅ Action performedFull review finished. |
|
Code review by qodo was updated up to the latest commit a97ce81 |
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Related issue: Resolves #45948
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
For unreleased bug fixes in a release candidate, one of:
Database migrations
COLLATE utf8mb4_unicode_ci).New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsfleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit