Return Windows Enrollment Status Page (ESP)#43454
Merged
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the first pass of Windows Autopilot Enrollment Status Page (ESP) support in the Windows MDM management checkin flow, enabling Fleet to initialize ESP tracking and subsequently return ongoing ESP status updates on later checkins.
Changes:
- Add ESP command generation to the Windows MDM management response path, including an init phase (enqueue) and an active phase (inline status updates).
- Introduce SyncML builders for DMClient/EnrollmentStatusTracking CSP nodes used by ESP, plus status mapping helpers.
- Add datastore support for host-scoped Windows profile queries and updating
awaiting_configuration, with mocks + tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/microsoft_mdm.go | Adds ESP decision logic to management checkins and implements pending/active ESP flows. |
| server/service/microsoft_mdm_test.go | Adds unit tests covering ESP command behavior across awaiting states and grace period scenarios. |
| server/mock/datastore_mock.go | Extends datastore mock with new ESP-related methods. |
| server/mdm/microsoft/esp_csp.go | Adds SyncML templates/builders for ESP init + status update commands and status mapping. |
| server/mdm/microsoft/esp_csp_test.go | Adds tests validating ESP SyncML generation and XML escaping. |
| server/fleet/datastore.go | Extends datastore interface for SetMDMWindowsAwaitingConfiguration and host-scoped profile listing. |
| server/datastore/mysql/microsoft_mdm.go | Implements new datastore methods for awaiting-config updates and host-scoped Windows profile selection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Windows Autopilot ESP (Enrollment Status Page) support and related datastore APIs and tests. Changes include: service logic to generate ESP/DMClient SyncML commands and manage awaiting_configuration transitions (Pending→Active→None) with timeout handling; datastore additions Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![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: 5
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)
1945-1952:⚠️ Potential issue | 🟠 MajorThis can't satisfy the first-checkin ESP requirement.
getPendingMDMCmdsruns beforehandleESPInitial, andhandleESPInitialonly persists the new command before returningnil. The first post-orbit response therefore can't include the freshly generated ESP/DMClient init payload; it only becomes deliverable on the next management session.Also applies to: 1954-1967, 2084-2103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/microsoft_mdm.go` around lines 1945 - 1952, The code fetches pending MDM commands via getPendingMDMCmds before calling handleESPInitial, so a newly-persisted ESP/DMClient init command created in handleESPInitial won't be included in the same first post-orbit response; fix by calling handleESPInitial first (when requestAuthState == RequestAuthStateTrusted and when handling first-checkin scenarios), ensure handleESPInitial persists any generated commands, and only then call getPendingMDMCmds to populate resPendingCmds so the freshly created ESP/DMClient init payload is delivered immediately; apply the same reorder/flow change in the other affected blocks referenced (around lines 1954-1967 and 2084-2103).
🧹 Nitpick comments (2)
server/service/microsoft_mdm_test.go (2)
1099-1104: Isolate datastore mocks per subtest to prevent state leakage
ds/svcare shared across allt.Runcases, so function overrides from one case can affect later cases and hide unintended calls. Build a fresh store/service inside each subtest (or reset all function fields) for deterministic coverage.Suggested test-setup refactor
func TestGetESPCommands(t *testing.T) { ctx := t.Context() - ds := new(mock.Store) - logger := slog.New(slog.DiscardHandler) - svc := &Service{ds: ds, logger: logger} + newSUT := func() (*mock.Store, *Service) { + ds := new(mock.Store) + logger := slog.New(slog.DiscardHandler) + return ds, &Service{ds: ds, logger: logger} + } deviceID := "test-device-id" hostUUID := "test-host-uuid" now := time.Now() t.Run("no awaiting configuration", func(t *testing.T) { + ds, svc := newSUT() ds.MDMWindowsGetEnrolledDeviceWithDeviceIDFunc = func(ctx context.Context, mdmDeviceID string) (*fleet.MDMWindowsEnrolledDevice, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/microsoft_mdm_test.go` around lines 1099 - 1104, The test shares ds and svc across subtests causing mock function overrides to leak; inside TestGetESPCommands create a fresh mock.Store and Service for each t.Run (or explicitly reset all mock function fields on ds before each subtest) so that mock methods on mock.Store and the Service{ds: ds, logger: logger} are isolated per case and deterministic.
1175-1176: Assert host-scoped method input to lock in the core ESP behaviorThese stubs ignore
hUUID. Since host-scoped filtering is a key requirement, asserthUUID == hostUUIDso regressions in argument wiring are caught.Suggested assertion hardening in stubs
ds.ListMDMWindowsProfilesToInstallForHostFunc = func(ctx context.Context, hUUID string) ([]*fleet.MDMWindowsProfilePayload, error) { + require.Equal(t, hostUUID, hUUID) return nil, nil }ds.ListMDMWindowsProfilesToInstallForHostFunc = func(ctx context.Context, hUUID string) ([]*fleet.MDMWindowsProfilePayload, error) { + require.Equal(t, hostUUID, hUUID) return []*fleet.MDMWindowsProfilePayload{ {ProfileUUID: "prof-1", ProfileName: "WiFi Config"}, {ProfileUUID: "prof-2", ProfileName: "SCEP Cert"}, }, nil }Also applies to: 1222-1223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/microsoft_mdm_test.go` around lines 1175 - 1176, The stub for ds.ListMDMWindowsProfilesToInstallForHostFunc ignores the host UUID argument; update the stub to assert that the incoming hUUID equals the test's hostUUID (e.g., using the test assertion helper or require.Equal) so host-scoped wiring is validated, and apply the same assertion pattern to the other identical stub(s) in this file that accept hUUID to ensure all host-scoped methods check hUUID == hostUUID (reference the ds.ListMDMWindowsProfilesToInstallForHostFunc and any other stubs taking hUUID).
🤖 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/datastore/mysql/microsoft_mdm.go`:
- Around line 879-886: The UPDATE in SetMDMWindowsAwaitingConfiguration
unconditionally flips awaiting_configuration and can race; change the SQL in
function SetMDMWindowsAwaitingConfiguration to a compare-and-swap UPDATE that
includes the current expected awaiting_configuration (e.g., "... WHERE
mdm_device_id = ? AND awaiting_configuration = ?") or otherwise targets the
specific enrollment row, execute with both mdmDeviceID and expected status, then
inspect and return the RowsAffected (or an error if 0 rows were changed) so
callers can detect whether the transition actually succeeded.
In `@server/mdm/microsoft/esp_csp.go`:
- Around line 67-126: In the espInitialCommandTmpl template change the
BlockInStatusPage item metadata to use Format int (not bool) and update its Data
value from the boolean "true" to an integer (e.g., 1 for enabled, 0 for
disabled) so the declared Format matches the value; locate the BlockInStatusPage
block inside the espInitialCommandTmpl template (the Replace block with Target
LocURI containing FirstSyncStatus/BlockInStatusPage) and replace the Meta Format
and Data accordingly.
In `@server/service/microsoft_mdm_test.go`:
- Around line 1288-1290: The loop that builds rawParts ignores the error from
xml.Marshal (xml.Marshal(cmd)) so a marshal failure could be silently ignored;
update that loop to check the error returned by xml.Marshal and fail the test on
error (e.g., use t.Fatalf/t.Fatalf-style assertion or require.NoError) when
marshaling a cmd into rawParts, referencing the cmds loop and rawParts variable
so the test fails immediately if xml.Marshal returns an error.
In `@server/service/microsoft_mdm.go`:
- Around line 2007-2010: The grace timer starts before HostUUID is present: when
device.HostUUID is empty you return early, but AwaitingConfigurationAt (the
grace-window timestamp) is measured from MDM enrollment and can expire while the
host is still unassociated. Fix by resetting or initializing
device.AwaitingConfigurationAt at the moment you attach/set device.HostUUID (or
by basing the wait logic on the actual Orbit enrollment timestamp instead of
AwaitingConfigurationAt). Locate the code paths that set device.HostUUID and
update them to set device.AwaitingConfigurationAt = now() (or switch the check
to use the Orbit enrollment time field) and apply the same change for the
similar checks around the other block that touches AwaitingConfigurationAt (the
checks in the 2028–2036 region).
- Around line 1954-1967: The ESP commands are fetched unconditionally which can
append/enqueue ESP actions for unauthenticated requests; wrap the
svc.getESPCommands call and subsequent append of espCmds behind a check that the
request's auth state equals RequestAuthStateTrusted (the same auth state used in
processIncomingMDMCmds). Concretely, only call svc.getESPCommands(ctx, deviceID)
and append espCmds to allCmds when reqMsg (or the local authState variable used
in processIncomingMDMCmds) == RequestAuthStateTrusted; otherwise skip
fetching/appending ESP so createResponseSyncML and createResponseSyncML do not
include ESP for untrusted requests.
---
Outside diff comments:
In `@server/service/microsoft_mdm.go`:
- Around line 1945-1952: The code fetches pending MDM commands via
getPendingMDMCmds before calling handleESPInitial, so a newly-persisted
ESP/DMClient init command created in handleESPInitial won't be included in the
same first post-orbit response; fix by calling handleESPInitial first (when
requestAuthState == RequestAuthStateTrusted and when handling first-checkin
scenarios), ensure handleESPInitial persists any generated commands, and only
then call getPendingMDMCmds to populate resPendingCmds so the freshly created
ESP/DMClient init payload is delivered immediately; apply the same reorder/flow
change in the other affected blocks referenced (around lines 1954-1967 and
2084-2103).
---
Nitpick comments:
In `@server/service/microsoft_mdm_test.go`:
- Around line 1099-1104: The test shares ds and svc across subtests causing mock
function overrides to leak; inside TestGetESPCommands create a fresh mock.Store
and Service for each t.Run (or explicitly reset all mock function fields on ds
before each subtest) so that mock methods on mock.Store and the Service{ds: ds,
logger: logger} are isolated per case and deterministic.
- Around line 1175-1176: The stub for
ds.ListMDMWindowsProfilesToInstallForHostFunc ignores the host UUID argument;
update the stub to assert that the incoming hUUID equals the test's hostUUID
(e.g., using the test assertion helper or require.Equal) so host-scoped wiring
is validated, and apply the same assertion pattern to the other identical
stub(s) in this file that accept hUUID to ensure all host-scoped methods check
hUUID == hostUUID (reference the ds.ListMDMWindowsProfilesToInstallForHostFunc
and any other stubs taking hUUID).
🪄 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: 52eecd15-e796-4bb3-a370-f728b7c4baa3
📒 Files selected for processing (7)
server/datastore/mysql/microsoft_mdm.goserver/fleet/datastore.goserver/mdm/microsoft/esp_csp.goserver/mdm/microsoft/esp_csp_test.goserver/mock/datastore_mock.goserver/service/microsoft_mdm.goserver/service/microsoft_mdm_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43454 +/- ##
==========================================
- Coverage 66.91% 66.82% -0.09%
==========================================
Files 2600 2617 +17
Lines 208643 210848 +2205
Branches 9305 9192 -113
==========================================
+ Hits 139608 140902 +1294
- Misses 56327 57116 +789
- Partials 12708 12830 +122
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ksykulev
force-pushed
the
41379-eua-orbit
branch
from
4081172 to
a52da4a
Compare
getvictor
changed the title
# Conflicts: # client/orbit_client_eua_test.go # orbit/pkg/packaging/windows_eua_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}
Autopilot-enrolled devices now receive ESP hold commands (Replace/Add) alongside fleetd install commands. Update TestWindowsAutomaticEnrollmentCommands and TestWindowsAzureInitiatedEnrollmentAndMapping to find fleetd commands by GUID rather than asserting exact command counts. Fix ineffassign lint error on unused cmds variable.
The SendResponse ack for Autopilot devices now includes ESP hold commands alongside the Status command. Update assertions to check for Status presence rather than exact command count.
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 I forgot to set this one ready for review yesterday |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.go`:
- Around line 2119-2126: Capture both return values from
svc.ds.SetMDMWindowsAwaitingConfiguration into (changed, err) and use them: if
err != nil return nil, err (or wrap and return) so DB/CAS failures are surfaced
instead of silently sending releaseCmds; if err == nil and changed == false
treat as benign (already released) and return nil, nil (or no releaseCmds) to
avoid re-emitting commands; only proceed to log "releasing device" and return
releaseCmds when changed == true. Reference: SetMDMWindowsAwaitingConfiguration
and the surrounding release logic that produces releaseCmds (e.g.,
handleESPRelease).
- Line 2023: The code is sending ESPTimeoutSeconds (seconds) to the DMClient
TimeOutUntilSyncFailure (which expects minutes); update the newSyncMLCmdInt call
that sets
"./Device/Vendor/MSFT/DMClient/Provider/%s/FirstSyncStatus/TimeOutUntilSyncFailure"
to pass ESPTimeoutSeconds converted to minutes (divide by 60), then clamp the
resulting minutes into the CSP-required range (60–1440) before formatting;
locate the call to newSyncMLCmdInt and the microsoft_mdm.ESPTimeoutSeconds
symbol and replace the raw seconds value with the converted-and-clamped minutes
value.
🪄 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: 53b1dfac-bbbc-41f4-bea0-00ea08610eaa
📒 Files selected for processing (3)
server/service/integration_mdm_test.goserver/service/microsoft_mdm.goserver/service/microsoft_mdm_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/service/integration_mdm_test.go
ksykulev
reviewed
Apr 22, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
server/service/microsoft_mdm.go (1)
2037-2052:⚠️ Potential issue | 🟠 MajorSend the ESP initialization payload before completing DevicePreparation.
This path flips
Pending→Activeand only returnsDevicePreparation/InstallationState=3. It never registers the expected profile policies or ESP tracking entries for the host, so the ESP has no initialized DeviceSetup baseline before preparation is marked complete. Build the full ESP/DMClient initialization commands here, or keep the device in the hold state until that payload is sent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/microsoft_mdm.go` around lines 2037 - 2052, The current path flips awaiting state to Active via SetMDMWindowsAwaitingConfiguration and immediately returns a single DevicePreparation/InstallationState=3 dpCmd without sending the ESP/DMClient initialization payloads (profiles, policy registration and ESP tracking entries); update the logic so that before setting InstallationState=3 you construct and return the full ESP initialization command set (e.g., DMClient registration and profile/policy payloads and ESP tracking entries) using the same newSyncMLCmdInt/mdm_types.SyncMLCmd pattern, and only then mark DevicePreparation complete (or alternatively do NOT call SetMDMWindowsAwaitingConfiguration/return InstallationState=3 and keep the device in hold until those initialization commands are successfully queued); reference SetMDMWindowsAwaitingConfiguration, dpCmd, newSyncMLCmdInt and DevicePreparation/InstallationState in your changes.
🤖 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.go`:
- Around line 2072-2104: The current readiness check uses replica-backed
ListMDMWindowsProfilesToInstallForHost and the UI helper
GetHostMDMWindowsProfiles, which can miss just-queued or reserved install rows;
replace this with a writer/transaction-backed datastore query scoped to the
specific device.HostUUID/current enrollment that returns all install-profile
rows that must block ESP release (including Fleet-reserved profiles). Implement
or call a new datastore method (e.g. GetMDMWindowsProfilesForESPRelease or a
transactional variant) that runs under the writer/transaction, includes reserved
profiles, filters by OperationType == fleet.MDMOperationTypeInstall, and checks
Status for terminal states; use that method in place of the two existing calls
in the ESP release logic.
- Around line 2121-2133: The code currently flips the device ESP state from
fleet.WindowsMDMAwaitingConfigurationActive to
fleet.WindowsMDMAwaitingConfigurationNone by calling
svc.ds.SetMDMWindowsAwaitingConfiguration before the release command
(releaseCmds) is actually acknowledged; change this so the DB still records the
pending release and the device remains in Active until delivery is confirmed:
persist the outgoing release command (e.g., via a datastore method like
CreatePendingMDMCommand or extend svc.ds with a SavePendingRelease) tied to
device.MDMDeviceID and return releaseCmds, and only call
svc.ds.SetMDMWindowsAwaitingConfiguration(...,
fleet.WindowsMDMAwaitingConfigurationNone) when the agent explicitly
acknowledges ServerHasFinishedProvisioning on a subsequent checkin; ensure
subsequent checkins detect the pending release record and resend the idempotent
release command instead of skipping ESP, and keep logging with
svc.logger.InfoContext as before.
---
Duplicate comments:
In `@server/service/microsoft_mdm.go`:
- Around line 2037-2052: The current path flips awaiting state to Active via
SetMDMWindowsAwaitingConfiguration and immediately returns a single
DevicePreparation/InstallationState=3 dpCmd without sending the ESP/DMClient
initialization payloads (profiles, policy registration and ESP tracking
entries); update the logic so that before setting InstallationState=3 you
construct and return the full ESP initialization command set (e.g., DMClient
registration and profile/policy payloads and ESP tracking entries) using the
same newSyncMLCmdInt/mdm_types.SyncMLCmd pattern, and only then mark
DevicePreparation complete (or alternatively do NOT call
SetMDMWindowsAwaitingConfiguration/return InstallationState=3 and keep the
device in hold until those initialization commands are successfully queued);
reference SetMDMWindowsAwaitingConfiguration, dpCmd, newSyncMLCmdInt and
DevicePreparation/InstallationState in your changes.
🪄 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: e0110e87-2c05-42e1-bacc-adf7522f804a
📒 Files selected for processing (2)
server/datastore/mysql/microsoft_mdm.goserver/service/microsoft_mdm.go
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.
ksykulev
approved these changes
Apr 28, 2026
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 #42843
This change shows Windows Enrollment Status Page (ESP) during OOBE enrollment. It does not track/update the status of that page, so the end user does not actually see any progress on it. Its purpose is to block the user from proceeding to desktop until all the profiles have been sent to the device. Software apps are not being tracked/blocked in this PR.
This is what the final ESP screen looks for this PR before it takes the user to set up Windows Hello:
Checklist for submitter
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests