Setup experience for Windows. - #44306
Merged
Merged
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
# Conflicts: # client/orbit_client_eua_test.go # orbit/pkg/packaging/windows_eua_test.go
…fleet into victor/42843-esp-status
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Actions performedFull review triggered. |
|
Persistent review updated to latest commit cd3836f |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/service/microsoft_mdm_property_test.go`:
- Around line 170-176: The timeout branch currently returns pbtESPRelease when
timedOut and requireAll is false, but per the spec a 3-hour ESP timeout must
always produce the timeout/failure UI; update the timedOut branch in the test
(where variables timedOut, requireAll are used) to always return pbtESPBlock and
false regardless of requireAll so the property encodes a blocking timeout path
(ensure the branch that reads pbtESPBlock/pbtESPRelease is the one updated).
In `@server/service/microsoft_mdm_test.go`:
- Around line 1675-1686: The test currently fails before hitting loadRequireAll
because HostLiteByIdentifierFunc returns an error causing
setupExperienceHostUUID() to return early; change the test to have
ds.HostLiteByIdentifierFunc return a valid *fleet.HostLite (so
setupExperienceHostUUID proceeds) and instead make the AppConfig/TeamLite lookup
fail (e.g., set ds.AppConfigByTeamIDFunc or ds.TeamLiteByIDFunc to return an
error) so getESPCommands(t.Context(), newActiveDevice()) exercises
loadRequireAll() and still returns an error and does not set
SetMDMWindowsAwaitingConfigurationFuncInvoked.
In `@server/service/microsoft_mdm.go`:
- Around line 2105-2115: The loadHost closure caches a nil host and sets
hostLoaded, which lets callers like setupExperienceHostUUID() and
loadRequireAll() dereference cachedHost and panic; change loadHost (the function
using svc.ds.HostLiteByIdentifier) to treat a nil result as an error by
returning a wrapped error when host == nil, and only set cachedHost and
hostLoaded after verifying host != nil so callers cannot receive a nil
cachedHost.
- Around line 2288-2307: The current logic computes shouldBlock := failed &&
requireAll which allows a pure timeout (timedOut == true, requireAll == false)
to go down the release path; change the condition so timeouts always block by
setting shouldBlock to true when timedOut is true (e.g., shouldBlock := timedOut
|| (failed && requireAll)); update the block so buildESPBlockCommands(provID,
errorText) is used for timedOut cases as well and
buildESPReleaseCommands(provID) is only used when neither timedOut nor a
failing-required-software condition applies.
🪄 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: 684a01bb-1eb4-4807-a074-69d1d3be3ee2
📒 Files selected for processing (12)
cmd/fleetctl/fleetctl/apply_test.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/fleet/datastore.goserver/mdm/microsoft/esp_csp.goserver/mock/datastore_mock.goserver/service/microsoft_mdm.goserver/service/microsoft_mdm_property_test.goserver/service/microsoft_mdm_test.goserver/service/orbit.goserver/service/orbit_test.goserver/service/setup_experience_test.go
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Member
Author
|
@ksykulev FYI: demo video: https://www.youtube.com/watch?v=_lDzsYG8LfQ |
ksykulev
reviewed
May 6, 2026
Member
Author
|
@ksykulev ready for re-review |
ksykulev
approved these changes
May 6, 2026
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue: Resolves #43859
This PR brings the Windows Autopilot setup experience to parity with macOS DEP. Windows hosts that enroll
through Autopilot now coordinate with Fleet during the OOBE Enrollment Status Page (ESP), so admin-defined
software installs run while the device is still waiting at the ESP screen, before the user can sign in. Fleet
holds the device on the ESP until profiles and setup-experience software all reach a terminal state, then
either releases the device to login or blocks it on a Reset PC failure screen. A new team-level setting
controls the policy: when enabled, any critical software install failure during ESP blocks the device with a
software-specific error message; when disabled, the device releases regardless of install outcomes (best
effort). A pure 3-hour timeout also forces a finalize, with a timeout-specific error message on the block
screen. The setting is premium-only and rejected when Windows MDM is not configured.
Beyond the gating itself, the PR adds the supporting machinery: orbit-driven setup-experience initialization
on Windows so installs are enqueued at the right moment, defense-in-depth cancellation of pending software
installs (both queue rows and status rows) whenever the device is going to block or time out, idempotent
re-enrollment cleanup so a device that resets and re-enrolls during ESP starts from a clean state. Internally, finalize is structured so a transient failure at any step (cancel, persist, or the state-machine transition) leaves the device retriable on the next management session rather than permanently stuck on "Working on it...". The behavior is exercised by example-based tests, a property-based test that randomly samples the wait/block/release decision matrix, and manual VM testing across Autopilot edge cases.
Checklist for submitter
Testing
Summary by CodeRabbit
Bug Fixes
New Features
Tests