Setup experience software policy checks - #47075
Merged
Merged
Conversation
…ftware (#45309) Gate Windows/Linux setup-experience software on an associated team policy (a policy whose install-software automation points at the same installer): - Migration adds setup_experience_status_results.policy_id (FK to policies, ON DELETE SET NULL). Internal column (json:"-"), no API change. - EnqueueSetupExperienceItems resolves the gating policy at enqueue time for Windows/Linux installers only (lowest id on ties; teamID 0 maps to global policies). VPP and Apple-platform rows are never gated. - During setup, policyQueriesForHost un-skips ONLY the host's gating policies via PolicyQueriesForHostFiltered, never the whole team policy set. - SetupExperienceNextStep uses the policy as a gate only: pass -> skip the install (success); fail -> install via the normal ForSetupExperience path so the item inherits the setup-experience retry count and RequireAllSoftware- Windows handling. Out-of-scope gating policy falls back to installing. - processSoftwareForNewlyFailingPolicies suppresses the automation for in-setup hosts so the software is not double-installed. - Result freshness is enforced via policy_membership.updated_at >= last_enrolled_at; init requests a refetch when gated items exist so a re-enrolled host gets a fresh result promptly. Premium, Windows/Linux only. The whole path is cron-free.
#45309) - changes/ changelog entry. - Datastore tests: enqueue records policy_id for an associated Windows/Linux installer and NULL for un-gated/macOS items; No-team host gated by its team_id=0 policy; GetSetupExperiencePolicyIDsForHost (pending vs terminal); GetSetupExperiencePolicyResult freshness (updated_at >= last_enrolled_at); PolicyQueriesForHostFiltered scoping and out-of-scope exclusion. - Service tests: gate control flow (pass->skip, fail->ForSetupExperience install with no PolicyID, awaiting-policy holds running, out-of-scope falls back to install, running item re-checked each poll). - osquery test: policy automation suppressed for in-setup hosts (no double install), fires normally otherwise. Also fixed the enqueue team scoping to match GetPoliciesWithAssociatedInstaller (team_id = teamID; No-team uses 0, not NULL).
…perience-policy-checks
) - Datastore test: an install linked to a non-terminal setup-experience row (what the gated install path produces) is returned by GetSoftwareInstallDetails with MaxRetries = setupExperienceSoftwareInstallsRetries (3 attempts), while a non-setup install gets 0. This is the retry parity linchpin. - Service test: SaveHostSoftwareInstallResult on a setup-experience install treats an intermediate failure (retries_remaining > 0) as recorded-only, not updating the setup item or canceling setup; the final failure cancel (under require_all_software_windows) is covered by the existing windows cancel test.
getvictor
changed the title
getvictor
changed the title
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Action performedFull review finished. |
Code Review by Qodo
1.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds “policy-gated” setup experience installs for Windows and Linux: when a setup-experience software installer is also referenced by an install-software policy automation, Fleet will run that policy during setup and skip the install if it passes, while preventing duplicate installs from policy automation during setup.
Changes:
- Add
policy_idtosetup_experience_status_resultsand record the gating policy at enqueue time (Windows/Linux only). - During setup experience, distribute only gating-policy queries (filtered) and suppress policy automation installs that would double-install.
- Implement server-side gating logic in setup experience: wait for a fresh policy result, skip on pass, install on fail, and fall back to install when policy scope excludes the host.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/service/setup_experience_test.go | Adds regression coverage for intermediate install failures not canceling setup experience. |
| server/service/osquery.go | Limits policy distribution during setup experience to gating policies and suppresses policy automation installs for gated items. |
| server/service/osquery_test.go | Updates/extends tests for setup-experience policy distribution and automation suppression behavior. |
| server/mock/datastore_mock.go | Extends datastore mock with filtered policy queries + setup-experience policy gating helpers. |
| server/fleet/setup_experience.go | Adds PolicyID to setup experience status model for internal gating. |
| server/fleet/datastore.go | Extends datastore interface with setup-experience policy gating/query APIs. |
| server/datastore/mysql/setup_experience.go | Records policy_id on enqueued Windows/Linux setup-experience software items; adds query to list gating policy IDs. |
| server/datastore/mysql/setup_experience_test.go | Adds MySQL integration tests for gating behavior, freshness, and retry inheritance. |
| server/datastore/mysql/schema.sql | Updates schema to include policy_id column + FK/index on setup experience status results. |
| server/datastore/mysql/policies.go | Implements PolicyQueriesForHostFiltered and GetSetupExperiencePolicyResult. |
| server/datastore/mysql/migrations/tables/20260608110503_AddPolicyGateToSetupExperienceResults.go | Migration adding policy_id column and FK to setup_experience_status_results. |
| server/datastore/mysql/migrations/tables/20260608110503_AddPolicyGateToSetupExperienceResults_test.go | Migration test validating FK behavior (ON DELETE SET NULL) and nullability. |
| ee/server/service/setup_experience.go | Implements policy-gated setup experience flow (wait/skip/install + out-of-scope fallback). |
| ee/server/service/setup_experience_test.go | Unit tests for the policy-gated setup experience next-step behavior. |
| ee/server/service/orbit.go | Requests host refetch when setup experience has gated items to ensure prompt policy distribution. |
| changes/45309-setup-experience-policy-checks | Adds release-note entry for the new Premium feature behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR implements policy-gated Windows/Linux setup-experience installations: a DB schema/field marks gated installer rows; datastore APIs expose filtered policy queries, per-policy results, and helpers to clear membership/reset host policy clocks; enqueue SQL computes/persists the gating marker; osquery filtering and automation suppression limit distributed queries and installs during setup; and the setup-experience service polls, advances gated items (pass → skip, fail → install, pending → wait with timeout), and resets host policy state after completion. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
server/fleet/datastore.go (2)
942-944: ⚡ Quick winDefine the empty-filter contract explicitly.
PolicyQueriesForHostFilteredis the guardrail that keeps setup from un-skipping unrelated team policies. Please document thatpolicyIDs == nilorlen(policyIDs) == 0must return an empty map rather than falling back toPolicyQueriesForHost, so implementations and mocks can't diverge here.🤖 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/fleet/datastore.go` around lines 942 - 944, The comment for PolicyQueriesForHostFiltered must explicitly state the empty-filter contract: when policyIDs is nil or has length zero the function MUST return an empty map (and no error) rather than delegating to or falling back to PolicyQueriesForHost; update the doc comment for PolicyQueriesForHostFiltered to document this behavior so all implementations and mocks (e.g., any mock of PolicyQueriesForHostFiltered) follow the contract and do not accidentally return team-wide policies.
946-948: ⚡ Quick winPin this to the latest definitive result.
The contract currently says “a” fresh pass/fail result after
since. If multiple evaluations exist after re-enrollment or repeated policy runs, callers need the most recent definitive result; otherwise setup can skip or install based on stale data.🤖 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/fleet/datastore.go` around lines 946 - 948, The GetSetupExperiencePolicyResult contract must return the latest definitive (non-null pass/fail) evaluation recorded at or after the provided since timestamp, not just any fresh result; update the implementation of GetSetupExperiencePolicyResult to query/filter evaluations for the given hostID and policyID with recorded_at >= since (or equivalent timestamp column), require the result be definitive (i.e., pass/fail not NULL), order by the evaluation timestamp/updated_at DESC and LIMIT 1, and return that single most recent bool value (or nil if none); reference the GetSetupExperiencePolicyResult function to locate and change the SQL/ORM query and result handling.
🤖 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/datastore/mysql/policies.go`:
- Around line 1253-1269: GetSetupExperiencePolicyResult is using
policy_membership.updated_at to decide freshness but RecordPolicyQueryExecutions
(and policiesNeedingMembershipWrite) intentionally skip writes when the pass
value is unchanged, which can leave gating policies forever returning nil; fix
by aligning the freshness signal with write semantics — either update a
per-policy "last_reported" timestamp whenever a host reports a policy (even if
pass/fail is unchanged) in
RecordPolicyQueryExecutions/policiesNeedingMembershipWrite, or add and maintain
a dedicated reported_at column and change GetSetupExperiencePolicyResult to
compare against that reported_at instead of updated_at so re-enrollment without
value change still counts as fresh.
---
Nitpick comments:
In `@server/fleet/datastore.go`:
- Around line 942-944: The comment for PolicyQueriesForHostFiltered must
explicitly state the empty-filter contract: when policyIDs is nil or has length
zero the function MUST return an empty map (and no error) rather than delegating
to or falling back to PolicyQueriesForHost; update the doc comment for
PolicyQueriesForHostFiltered to document this behavior so all implementations
and mocks (e.g., any mock of PolicyQueriesForHostFiltered) follow the contract
and do not accidentally return team-wide policies.
- Around line 946-948: The GetSetupExperiencePolicyResult contract must return
the latest definitive (non-null pass/fail) evaluation recorded at or after the
provided since timestamp, not just any fresh result; update the implementation
of GetSetupExperiencePolicyResult to query/filter evaluations for the given
hostID and policyID with recorded_at >= since (or equivalent timestamp column),
require the result be definitive (i.e., pass/fail not NULL), order by the
evaluation timestamp/updated_at DESC and LIMIT 1, and return that single most
recent bool value (or nil if none); reference the GetSetupExperiencePolicyResult
function to locate and change the SQL/ORM query and result handling.
🪄 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: fa2ca06e-2af7-4a2a-9dca-568a8ca28b70
📒 Files selected for processing (16)
changes/45309-setup-experience-policy-checksee/server/service/orbit.goee/server/service/setup_experience.goee/server/service/setup_experience_test.goserver/datastore/mysql/migrations/tables/20260608110503_AddPolicyGateToSetupExperienceResults.goserver/datastore/mysql/migrations/tables/20260608110503_AddPolicyGateToSetupExperienceResults_test.goserver/datastore/mysql/policies.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/setup_experience.goserver/datastore/mysql/setup_experience_test.goserver/fleet/datastore.goserver/fleet/setup_experience.goserver/mock/datastore_mock.goserver/service/osquery.goserver/service/osquery_test.goserver/service/setup_experience_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #47075 +/- ##
==========================================
+ Coverage 67.16% 67.22% +0.05%
==========================================
Files 2926 2927 +1
Lines 226389 226714 +325
Branches 11683 11683
==========================================
+ Hits 152050 152400 +350
+ Misses 60600 60549 -51
- Partials 13739 13765 +26
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:
|
…perience-policy-checks # Conflicts: # server/datastore/mysql/schema.sql
This reverts commit 9f961ab.
…perience-policy-checks # Conflicts: # server/datastore/mysql/schema.sql
Member
Author
|
@coderabbitai full review |
Member
Author
|
/agentic_review |
Contributor
✅ Action performedFull review finished. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…perience-policy-checks # Conflicts: # server/datastore/mysql/schema.sql
ksykulev
reviewed
Jun 9, 2026
Member
Author
|
@ksykulev ready for re-review |
ksykulev
approved these changes
Jun 10, 2026
7 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 #45309
If software is linked to policies, we run the policy during setup experience to determine if software should be installed. We install on failing policies.
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.
Timeouts are implemented and retries are limited to avoid infinite loops
Testing
Summary by CodeRabbit