Updated Windows setup experience to not cancel for BYOD.#45994
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
/agentic_review |
Code Review by Qodo
1. Replica-lag bypasses cancel gate
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Windows “cancel setup experience if software fails” behavior so that cancellation (and the associated canceled_setup_experience activity) only occurs when the host is actually in a Fleet-tracked Windows ESP flow, preventing BYOD/manual enrollment paths from incorrectly triggering cancellation.
Changes:
- Gates the Windows cancel cascade on
mdm_windows_enrollments.awaiting_configurationbeingPendingorActive, with a device-name fallback for the host-UUID linking race window. - Reworks/expands unit tests into a Windows cancel-gate matrix to cover ESP vs non-ESP enrollment paths and the fallback lookup behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/service/setup_experience.go | Changes Windows cancellation gating logic to rely on awaiting_configuration (Pending/Active) and adds a device-name fallback when host_uuid isn’t linked yet. |
| server/service/setup_experience_test.go | Updates Windows setup experience tests to validate the new gate behavior across multiple enrollment scenarios (including the race-window fallback). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
WalkthroughThis PR changes how Windows setup-experience cancellation is gated when software installation fails. Instead of checking whether a host is a BYOD device via the "MDMNotInOOBE" enrollment status, the code now gates on Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 1
🤖 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/setup_experience.go`:
- Around line 308-315: The fallback using
MDMWindowsGetUnlinkedEnrolledDeviceWithDeviceName(ctx, host.ComputerName) is too
broad because ComputerName is not unique; restrict the match before trusting
device.AwaitingConfiguration to avoid flipping cancellation for the wrong
machine. Change the lookup to include an additional discriminator (e.g.,
hardware UUID or serial) or add/replace with a datastore method like
MDMWindowsGetUnlinkedEnrolledDeviceWithDeviceNameAndHardwareID and then, after
calling the lookup, verify the returned device's hardware identifier (e.g.,
device.HardwareUUID or device.SerialNumber) equals the host's (host.HardwareUUID
or host.HardwareSerial) before assigning awaiting = device.AwaitingConfiguration
and emitting cancellation.
🪄 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: 37da39e7-47e7-45be-be3e-a40e37edc8d1
📒 Files selected for processing (2)
server/service/setup_experience.goserver/service/setup_experience_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45994 +/- ##
=======================================
Coverage 66.81% 66.81%
=======================================
Files 2754 2754
Lines 220112 220115 +3
Branches 10981 10981
=======================================
+ Hits 147071 147074 +3
Misses 59757 59757
Partials 13284 13284
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:
|
![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.
…6028) <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #45946 Unreleased bug. # Checklist for submitter ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually For unreleased bug fixes in a release candidate, one of: - [x] Confirmed that the fix is not expected to adversely impact load test results <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit **Bug Fixes** - Enhanced Windows setup experience cancellation to accurately validate device enrollment status before processing cancellations - Improved device identification reliability during concurrent operations through robust fallback mechanisms that reference recent enrollment records when initial lookups fail <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/45994?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit f9c9ad0)
](https://app.coderabbit.ai/change-stack/fleetdm/fleet/pull/45994?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
Related issue: Resolves #45946
Unreleased bug.
Checklist for submitter
Testing
For unreleased bug fixes in a release candidate, one of:
Summary by CodeRabbit
Bug Fixes