Conversation
![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.
WalkthroughAdds detection and recording of Windows MDM wipe failures by returning a structured 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 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
🧹 Nitpick comments (1)
server/datastore/mysql/microsoft_mdm.go (1)
539-720: Consider test coverage for the new wipe-failure branch.The new
WipeFailedresult path (prefix‑"2" classification, population ofresult.WipeFailed, and the earlyreturn nil, errpaths) has no direct assertions visible in this diff. Given the downstream activity creation depends entirely on this return value, please ensuremicrosoft_mdm_test.go(or an integration test) covers at least: (a) successful wipe (noWipeFailed), (b) failed wipe with a 4xx/5xx status (WipeFailedpopulated), and (c) non‑wipe responses (result staysnil).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/microsoft_mdm.go` around lines 539 - 720, Add unit tests in microsoft_mdm_test.go that exercise Datastore.MDMWindowsSaveResponse for the three cases: (1) successful wipe where the matching command has a status code starting with "2" and assert result is nil and updateHostLockWipeStatusFromResultAndHostUUID was called with wipeSucceeded=true; (2) failed wipe where status starts with "4" or "5" and assert result.WipeFailed is populated with the expected HostUUID and updateHostLockWipeStatusFromResultAndHostUUID was called with wipeSucceeded=false; and (3) a non-wipe response (no TargetLocURI match) where result remains nil and no wipe-related update is made. Construct test inputs to create an enrolledDevice and matchingCmds, stub/spy the DB interactions or use an in-memory test DB, and assert the observed behavior of MDMWindowsSaveResponse, referencing the MDMWindowsSaveResponse method, the WipeFailed field on fleet.MDMWindowsSaveResponseResult, and the updateHostLockWipeStatusFromResultAndHostUUID side-effect to validate correct branching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@changes/42290-wipe-activity`:
- Line 1: Update the changelog entry text to use proper product casing and
include the missing article: change the sentence "Generate activity when the
windows mdm wipe command fails." to use "Activity" (product casing), "Windows
MDM" (product casing), and include "the" before the command, e.g. "Generate an
Activity when the Windows MDM wipe command fails." Apply this exact wording
where the changelog entry for changes/42290-wipe-activity is defined.
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 685-700: The code treats an empty wipeCmdStatus as a failure;
change the logic in the block that handles wipeCmdUUID so you only interpret a
concrete status when wipeCmdStatus is non-empty: if wipeCmdUUID != "" &&
wipeCmdStatus != "" then compute wipeSucceeded :=
strings.HasPrefix(wipeCmdStatus, "2"), call
updateHostLockWipeStatusFromResultAndHostUUID(...) with that boolean, and only
set result.WipeFailed when wipeSucceeded is false (i.e., a non-2xx concrete
status was returned); if wipeCmdStatus is empty, skip updating host_mdm_actions
and do not set result.WipeFailed. Also review whether status codes 214/215
should be treated as success and adjust the strings.HasPrefix/logic accordingly.
---
Nitpick comments:
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 539-720: Add unit tests in microsoft_mdm_test.go that exercise
Datastore.MDMWindowsSaveResponse for the three cases: (1) successful wipe where
the matching command has a status code starting with "2" and assert result is
nil and updateHostLockWipeStatusFromResultAndHostUUID was called with
wipeSucceeded=true; (2) failed wipe where status starts with "4" or "5" and
assert result.WipeFailed is populated with the expected HostUUID and
updateHostLockWipeStatusFromResultAndHostUUID was called with
wipeSucceeded=false; and (3) a non-wipe response (no TargetLocURI match) where
result remains nil and no wipe-related update is made. Construct test inputs to
create an enrolledDevice and matchingCmds, stub/spy the DB interactions or use
an in-memory test DB, and assert the observed behavior of
MDMWindowsSaveResponse, referencing the MDMWindowsSaveResponse method, the
WipeFailed field on fleet.MDMWindowsSaveResponseResult, and the
updateHostLockWipeStatusFromResultAndHostUUID side-effect to validate correct
branching.
🪄 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: d3c7f42c-b4f2-44cf-8b68-89e029378cab
⛔ Files ignored due to path filters (1)
docs/Contributing/reference/audit-logs.mdis excluded by!**/*.md
📒 Files selected for processing (9)
changes/42290-wipe-activityserver/datastore/mysql/mdm_test.goserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/fleet/activities.goserver/fleet/datastore.goserver/fleet/microsoft_mdm.goserver/mock/datastore_mock.goserver/service/microsoft_mdm.go
There was a problem hiding this comment.
Pull request overview
Adds a new audit-log activity for Windows MDM wipe failures so admins can observe failures (and trigger automations via webhooks) when a device reports a non-2xx wipe result.
Changes:
- Extend Windows MDM response processing to surface “wipe failed” events from the datastore up to the service layer and create a new activity (
wipe_failed_host). - Update datastore interfaces/mocks to return a result object from
MDMWindowsSaveResponse. - Document the new activity type and add a release-note entry.
Reviewed changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/microsoft_mdm.go | Creates wipe_failed_host activity when the datastore reports a wipe failure. |
| server/datastore/mysql/microsoft_mdm.go | Returns MDMWindowsSaveResponseResult when a wipe command response indicates failure. |
| server/fleet/microsoft_mdm.go | Defines result structs used to report notable Windows MDM response events. |
| server/fleet/datastore.go | Updates datastore interface signature for MDMWindowsSaveResponse. |
| server/mock/datastore_mock.go | Updates mock datastore function signatures/return types for the new result. |
| server/fleet/activities.go | Registers new ActivityTypeWipeFailedHost and includes it in ActivityDetailsList. |
| server/datastore/mysql/microsoft_mdm_test.go | Updates existing tests to accommodate new return value. |
| server/datastore/mysql/mdm_test.go | Updates existing tests to accommodate new return value. |
| docs/Contributing/reference/audit-logs.md | Documents the new wipe_failed_host audit log entry. |
| changes/42290-wipe-activity | Adds release note entry for wipe-failed activity. |
💡 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 #43795 +/- ##
========================================
Coverage 66.91% 66.92%
========================================
Files 2600 2600
Lines 208713 209017 +304
Branches 9188 9188
========================================
+ Hits 139668 139880 +212
- Misses 56328 56396 +68
- Partials 12717 12741 +24
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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/scripts.go`:
- Around line 2328-2333: The code ignores errors from res.RowsAffected() after
tx.ExecContext, which can hide failures used by Windows MDM wipe handling; call
n, err = res.RowsAffected() and if err != nil return 0, ctxerr.Wrap(ctx, err,
"rows affected after update host lock/wipe status via host uuid") so the
RowsAffected error is propagated instead of silently returning 0; update the
return to use that checked n (from RowsAffected) and preserve existing
ctxerr.Wrap usage around the new error path.
🪄 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: 25ec9cd3-dec9-43dd-abb1-c90cac66e458
📒 Files selected for processing (4)
changes/42290-wipe-activityserver/datastore/mysql/microsoft_mdm.goserver/datastore/mysql/microsoft_mdm_test.goserver/datastore/mysql/scripts.go
✅ Files skipped from review due to trivial changes (1)
- changes/42290-wipe-activity
🚧 Files skipped from review as they are similar to previous changes (2)
- server/datastore/mysql/microsoft_mdm_test.go
- server/datastore/mysql/microsoft_mdm.go
|
|
@marko-lisica Does the frontend copy need some tweaking here? |
|
@claude review once |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // if we received a Wipe command result, update the host's status | ||
| if wipeCmdUUID != "" { | ||
| if err := updateHostLockWipeStatusFromResultAndHostUUID(ctx, tx, enrolledDevice.HostUUID, | ||
| "wipe_ref", wipeCmdUUID, strings.HasPrefix(wipeCmdStatus, "2"), false, | ||
| ); err != nil { | ||
| wipeSucceeded := strings.HasPrefix(wipeCmdStatus, "2") | ||
| rowsAffected, err := updateHostLockWipeStatusFromResultAndHostUUID(ctx, tx, enrolledDevice.HostUUID, | ||
| "wipe_ref", wipeCmdUUID, wipeSucceeded, false, | ||
| ) | ||
| if err != nil { | ||
| return ctxerr.Wrap(ctx, err, "updating wipe command result in host_mdm_actions") | ||
| } | ||
|
|
||
| if wipeCmdStatus != "" && !wipeSucceeded && rowsAffected > 0 { | ||
| result = &fleet.MDMWindowsSaveResponseResult{ | ||
| WipeFailed: &fleet.MDMWindowsWipeResult{ | ||
| HostUUID: enrolledDevice.HostUUID, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
🟣 This is a pre-existing issue not introduced by this PR. When wipeCmdUUID != "" but wipeCmdStatus == "" (device response references the wipe command UUID but provides no Status element), the call to updateHostLockWipeStatusFromResultAndHostUUID with succeeded=false clears wipe_ref = NULL in host_mdm_actions, silently transitioning the host from 'wipe pending' to no-wipe state — this DB update behavior is structurally identical in both old and new code and predates this PR. The PR correctly guards the new activity-creation logic with wipeCmdStatus != "", so the net change from this PR is correct; the underlying DB-state issue should be tracked as a separate fix.
Extended reasoning...
The bug is real: when a Windows MDM device response includes a wipe command UUID in CmdRefUUIDs but provides no element for it (wipeCmdStatus == ""), the code still calls updateHostLockWipeStatusFromResultAndHostUUID with succeeded=false. Per buildHostLockWipeStatusUpdateStmt in scripts.go, a succeeded=false update for wipe_ref generates SET hma.wipe_ref = NULL, which silently clears the wipe-pending state in host_mdm_actions with no activity record and no user notification.
However, the refuter's analysis is correct: this behavior is pre-existing and was not introduced by this PR. Comparing old and new code shows the DB update call is structurally identical in both versions. Old code: updateHostLockWipeStatusFromResultAndHostUUID(ctx, tx, enrolledDevice.HostUUID, "wipe_ref", wipeCmdUUID, strings.HasPrefix(wipeCmdStatus, "2"), false). New code: wipeSucceeded := strings.HasPrefix(wipeCmdStatus, "2"); rowsAffected, err := updateHostLockWipeStatusFromResultAndHostUUID(ctx, tx, enrolledDevice.HostUUID, "wipe_ref", wipeCmdUUID, wipeSucceeded, false). An empty wipeCmdStatus evaluates to false in both versions, and the UPDATE fires identically.
What this PR added was new activity-creation logic, which is correctly gated: if wipeCmdStatus != "" && !wipeSucceeded && rowsAffected > 0 { result = &fleet.MDMWindowsSaveResponseResult{WipeFailed: ...} }. The test 'empty status does not signal wipe failure' validates this correctly — it checks result.WipeFailed == nil when status is empty, which passes because the activity guard is right.
Step-by-step proof: (1) Device sends a SyncML response with a element referencing wipeCmdUUID but no element for that UUID. (2) NewEnrichedSyncML adds the UUID to CmdRefUUIDs (both Status and Results CmdRefs are appended), so wipeCmdUUID gets assigned at line 661-664. (3) statusCode is never set for this UUID (lines 624-626 only populate CmdRefUUIDToStatus for Status commands), leaving wipeCmdStatus == "". (4) Old code: strings.HasPrefix("", "2") == false → updateHostLockWipeStatusFromResultAndHostUUID with succeeded=false → SET hma.wipe_ref = NULL. (5) New code: same evaluation path, same UPDATE — wipe_ref is cleared. The guard then correctly prevents result.WipeFailed from being set. (6) Net result: both old and new code silently clear wipe_ref; the PR does not make the situation worse.
The fix described by verifiers — wrapping the entire if wipeCmdUUID != "" block with wipeCmdStatus != "" — would address the pre-existing issue, but that is beyond the scope of this PR's intended change (adding the wipe-failed activity). The scenario is also non-standard in practice: Exec commands normally receive responses, not -only responses. The pre-existing nature and low probability of triggering reduce urgency, but the issue warrants a follow-up ticket.
There was a problem hiding this comment.
@getvictor do we want to address this feedback in this PR? I typically open up another PR for the "make the camp site better than you left it" feedback.
There was a problem hiding this comment.
Another PR makes sense. Thanks.
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good. Please take a look and resolve the remaining AI review comments.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/datastore/mysql/microsoft_mdm.go (1)
687-697:⚠️ Potential issue | 🟠 MajorSkip wipe-state updates when the device did not report a concrete status.
Line 689 still treats
wipeCmdStatus == ""as failure and Line 690 persists that failed state before Line 697 suppresses the activity. This can leavehost_mdm_actionsmarked failed for an unreported wipe response.Proposed fix
// if we received a Wipe command result, update the host's status - if wipeCmdUUID != "" { + if wipeCmdUUID != "" && wipeCmdStatus != "" { wipeSucceeded := strings.HasPrefix(wipeCmdStatus, "2") rowsAffected, err := updateHostLockWipeStatusFromResultAndHostUUID(ctx, tx, enrolledDevice.HostUUID, "wipe_ref", wipeCmdUUID, wipeSucceeded, false, ) @@ - if wipeCmdStatus != "" && !wipeSucceeded && rowsAffected > 0 { + if !wipeSucceeded && rowsAffected > 0 { result = &fleet.MDMWindowsSaveResponseResult{ WipeFailed: &fleet.MDMWindowsWipeResult{ HostUUID: enrolledDevice.HostUUID,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/microsoft_mdm.go` around lines 687 - 697, The code currently treats an empty wipeCmdStatus as a failure; change the logic so we only mark a wipe as failed when the device reported a concrete non-empty status that does not indicate success. Concretely, in the block handling wipeCmdUUID, compute wipeSucceeded only when wipeCmdStatus != "" (e.g., wipeSucceeded := wipeCmdStatus != "" && strings.HasPrefix(wipeCmdStatus, "2")) and only call updateHostLockWipeStatusFromResultAndHostUUID or interpret rowsAffected as a failure if wipeCmdStatus != ""; ensure the later check that logs or persists a failure uses wipeCmdStatus != "" && !wipeSucceeded && rowsAffected > 0 so empty/unknown statuses are skipped.
🤖 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 697-700: The code currently creates a
fleet.MDMWindowsSaveResponseResult with a fleet.MDMWindowsWipeResult that only
sets HostUUID, which drops the concrete failure status (wipeCmdStatus) needed by
activity/webhook consumers; update the MDMWindowsWipeResult type to add fields
for the command failure details (e.g., WipeCommandStatus string and optionally
RowsAffected or ErrorMessage), then populate those new fields here when
wipeCmdStatus != "" && !wipeSucceeded && rowsAffected > 0 (in addition to
HostUUID), and adjust the activity/webhook payload creation code to include
these new fields so the wipe-failed event carries the actual device status.
---
Duplicate comments:
In `@server/datastore/mysql/microsoft_mdm.go`:
- Around line 687-697: The code currently treats an empty wipeCmdStatus as a
failure; change the logic so we only mark a wipe as failed when the device
reported a concrete non-empty status that does not indicate success. Concretely,
in the block handling wipeCmdUUID, compute wipeSucceeded only when wipeCmdStatus
!= "" (e.g., wipeSucceeded := wipeCmdStatus != "" &&
strings.HasPrefix(wipeCmdStatus, "2")) and only call
updateHostLockWipeStatusFromResultAndHostUUID or interpret rowsAffected as a
failure if wipeCmdStatus != ""; ensure the later check that logs or persists a
failure uses wipeCmdStatus != "" && !wipeSucceeded && rowsAffected > 0 so
empty/unknown statuses are skipped.
🪄 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: 29f5fd57-917a-42d1-9948-8af46d9f8896
📒 Files selected for processing (3)
server/datastore/mysql/microsoft_mdm.goserver/fleet/activities.goserver/service/mdm_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/fleet/activities.go
| if wipeCmdStatus != "" && !wipeSucceeded && rowsAffected > 0 { | ||
| result = &fleet.MDMWindowsSaveResponseResult{ | ||
| WipeFailed: &fleet.MDMWindowsWipeResult{ | ||
| HostUUID: enrolledDevice.HostUUID, |
There was a problem hiding this comment.
Preserve the failure status for activity/webhook consumers.
The datastore has wipeCmdStatus here, but the structured result only carries HostUUID. That drops the failure reason/details requested for the wipe-failed event and prevents the service/webhook payload from including the concrete device status.
Proposed direction
result = &fleet.MDMWindowsSaveResponseResult{
WipeFailed: &fleet.MDMWindowsWipeResult{
- HostUUID: enrolledDevice.HostUUID,
+ HostUUID: enrolledDevice.HostUUID,
+ CommandUUID: wipeCmdUUID,
+ StatusCode: wipeCmdStatus,
},
}Also extend fleet.MDMWindowsWipeResult and the activity payload to expose these fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/datastore/mysql/microsoft_mdm.go` around lines 697 - 700, The code
currently creates a fleet.MDMWindowsSaveResponseResult with a
fleet.MDMWindowsWipeResult that only sets HostUUID, which drops the concrete
failure status (wipeCmdStatus) needed by activity/webhook consumers; update the
MDMWindowsWipeResult type to add fields for the command failure details (e.g.,
WipeCommandStatus string and optionally RowsAffected or ErrorMessage), then
populate those new fields here when wipeCmdStatus != "" && !wipeSucceeded &&
rowsAffected > 0 (in addition to HostUUID), and adjust the activity/webhook
payload creation code to include these new fields so the wipe-failed event
carries the actual device status.
There was a problem hiding this comment.
The activity struct matches the existing ActivityTypeWipedHost. The webhook consumer just needs to know which host failed, not the specific MDM status code. Would adding a StatusCode really be that much beneficial?
Related issue: Resolves #42290
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
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.Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests