Add patch when closed policies - #50726
Merged
Merged
Conversation
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #49415 # 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/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [ ] Input data is properly validated, `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. - [ ] Timeouts are implemented and retries are limited to avoid infinite loops - [ ] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [ ] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually - Tested with FLEET_DEV_MAINTAINED_APPS_BASE_URL pointed to a branch with the new queries. If there is an open query, adding the FMA adds it to `app_open_query`. If not, it remains empty. - Also tested a few of the `SELECT 1 WHERE NOT EXISTS (SELECT 1 FROM apps a JOIN processes p ON p.path LIKE concat(a.path, '/%%') WHERE a.bundle_identifier = '%s');` queries locally on apps available in dogfood self service, looks like it will work reasonably well for a start. ## Database migrations - [x] Checked schema for all modified table for columns that will auto-update timestamps during migration. - N/A Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects. - No timestamp updates - [x] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a policy option to skip patch installation while the managed application is open. * Added support for detecting open applications on macOS and Windows. * Maintained applications now preserve and use app-open checks during installation and updates. * Policy and installer responses now include the related configuration fields. * **Bug Fixes** * Ensured app-open settings are retained when installers are created, updated, or retrieved. * Existing policies and installers receive safe default values for the new settings. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
**Related issue:** Resolves #49418 Adds `patch_when_closed` support to GitOps for patch policies and round-trips it through `fleetctl generate-gitops`. - Validate `patch_when_closed` in the patch-policy YAML: reject an explicit `continuous_automations_enabled: false` alongside it (GitOps is declarative — the datastore would otherwise silently force it on), auto-set it when omitted, and reject a `pre_install_query` on the referenced Fleet-maintained app (Fleet manages that query). - Emit `patch_when_closed` from `fleetctl generate-gitops`. # Checklist for submitter - [x] Input data is properly validated, `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 - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## New Fleet configuration settings Follow this checklist for GitOps-enabled settings: - [x] Verified that the setting is exported via `fleetctl generate-gitops` - [x] Verified the setting is documented in a separate PR to [the GitOps documentation](https://github.com/fleetdm/fleet/blob/main/docs/Configuration/yaml-files.md) (#49022) - [x] Verified that the setting is cleared on the server if it is not supplied in a YAML file --------- Co-authored-by: Jonathan Katz <yehonatankatz@gmail.com>
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #49417 Makes an "app was open" result on a patch-when-closed policy install a **skip** rather than a failure: - Detects the skip in `SaveHostSoftwareInstallResult` — policy `patch_when_closed` + empty pre-install output — and labels the `installed_software` activity with `install_skipped_when_app_open`. - Doesn't let the skip consume a retry attempt: stores `attempt_number = 0` (excluded from the retry-sequence count) and doesn't queue an immediate retry — the next continuous-automation run re-fires. - Shows "The app was open" copy in install details, distinct from the generic pre-install-query-failed copy. - Keys on the managed policy flag, not on empty output, so an ordinary empty `pre_install_query` on a non-managed policy still fails and still counts toward the retry limit. The `changes/` entry for this feature lives on the base feature branch per the stacked-PR convention. # Checklist for submitter - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements). - [x] Timeouts are implemented and retries are limited to avoid infinite loops. ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one host's records do not affect another) - [x] QA'd all new/changed functionality manually - Drove the orbit `software_install/result` endpoint end-to-end against a running server: app-open result → `failed_install`, activity `install_skipped_when_app_open: true`, `attempt_number = 0`, no retry queued, and install details showing "The app was open". - Regression: an ordinary empty `pre_install_query` on a non-managed policy still fails, counts (`attempt_number = 1`), and queues a retry.
<!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #49416 - Adds patch_when_closed to the new policy, update policy, and update package endpoints - Sets pre_install_query in the software title endpoint to software_installers.app_open_query if the policy is a patch when closed policy (discussed in standup) - Deletes the existing pre_install_query if setting patch_when_closed (discussed in standup) - Fixes a small error in the patch_policy package - Fixes the default macOS query in patch_policy package to use to escape symbols in a.path # 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/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [ ] Input data is properly validated, `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. - [ ] Timeouts are implemented and retries are limited to avoid infinite loops - [ ] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [ ] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added a “patch when closed” option for patch policies and Fleet-maintained apps. * Added installer controls for enabling patching and configuring whether apps must be closed before installation. * Automatically manages pre-install behavior and continuous automation requirements for these policies. * **Bug Fixes** * Improved app detection based on application path prefixes on macOS. * Corrected RStudio process detection on Windows. * Added validation to prevent incompatible patch policy settings. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
**Related issue:** Resolves #49419 Adds the UI for patching Fleet-maintained apps when the app is closed. The patch toggle becomes a Deploy control with Force install and Patch checkboxes. Checking Patch reveals a radio group: Patch when app is closed, Force patch, or End user initiated. The control appears in the Add software flow, a new Deploy modal, and the edit-policy page. The change also adds skipped-install copy in the activity feed and install details, a Self-service toggle for Fleet-maintained apps, and GitOps and Premium gating. The PR also includes backend changes that expose the patch policy's continuous-automation state and a Fleet-maintained app's install query in the API. The UI reads both to show the correct Deploy options, so they ship in one PR. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually Eng QA walkthrough [part 1](https://drive.google.com/file/d/1Un-Z4QoTk2eXslQ_b8t95cE11QYq1SUt/view?usp=drive_link), [part 2](https://drive.google.com/file/d/12_1Eh_RHOJ7RTjXBxFgas0VhKWvhLS0r/view?usp=drive_link)
…ontend copy (#50542) <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #50522 Resolves #50523 # Checklist for submitter ## Testing - [x] Added/updated automated tests - [ ] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [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 - [ ] Alerted the release DRI if additional load testing is needed <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Pre-install queries are no longer exported for apps configured to patch when closed. * Updated automation activity labels to clearly identify skipped patches, including the software name when available. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…50691) - Sets continuous automations to off by default in the Deploy modal and Edit policy page <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves # # 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/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [ ] Input data is properly validated, `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. - [ ] Timeouts are implemented and retries are limited to avoid infinite loops - [ ] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [ ] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [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 - [ ] Alerted the release DRI if additional load testing is needed <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Continuous automation settings now behave consistently when creating or editing Force patch policies. - Manual patching disables continuous automation, while closed patching enables it. - Other patch selections preserve the current automation setting. - Saving existing policies now correctly retains the configured automation state. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
jkatz01
force-pushed
the
feat/39962-patch-when-closed
branch
2 times, most recently
from
August 6, 2026 21:56
0f668ab to
613c7cc
Compare
jkatz01
force-pushed
the
feat/39962-patch-when-closed
branch
from
August 7, 2026 14:14
613c7cc to
6ef4ba3
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #50726 +/- ##
==========================================
+ Coverage 68.40% 68.50% +0.10%
==========================================
Files 3963 3969 +6
Lines 255174 255705 +531
Branches 13420 13718 +298
==========================================
+ Hits 174555 175181 +626
+ Misses 65021 64916 -105
- Partials 15598 15608 +10
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:
|
cdcme
reviewed
Aug 7, 2026
| assert.Equal(t, 50, finalConfig.WebhookSettings.FailingPoliciesWebhook.HostBatchSize) | ||
| } | ||
|
|
||
|
|
jkatz01
marked this pull request as ready for review
August 7, 2026 15:52
RachelElysia
approved these changes
Aug 7, 2026
cdcme
approved these changes
Aug 7, 2026
Contributor
WalkthroughAdds the Premium Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.0)server/service/integration_enterprise_test.goast-grep timed out on this file 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: 9
🧹 Nitpick comments (2)
frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeployModal/DeployModal.tsx (1)
130-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the update condition.
The four clauses overlap.
patchOption !== initialPatchOptionis already true whenever the derived flags differ, except for the case where the stored policy flags do not match the derived option (for examplecontinuous_automations_enabled: truewithpatch_when_closed: false). Compute the flags once and compare them to the stored policy. This removes the duplicategetPatchPolicyFlags(patchOption)calls and makes the intent explicit.♻️ Proposed simplification
- } else if ( - patch && - patchPolicy && - (patchOption !== initialPatchOption || - patchHasAutomation !== (patchOption !== "manual") || - patchPolicy.patch_when_closed !== - getPatchPolicyFlags(patchOption).patch_when_closed || - patchPolicy.continuous_automations_enabled !== - getPatchPolicyFlags(patchOption).continuous_automations_enabled) - ) { - await teamPoliciesAPI.update(patchPolicy.id, { - team_id: teamId, - software_title_id: patchOption === "manual" ? null : softwareTitle.id, - ...getPatchPolicyFlags(patchOption), - }); - savedAnyChange = true; - } + } else if (patch && patchPolicy) { + const flags = getPatchPolicyFlags(patchOption); + const shouldAttachInstall = patchOption !== "manual"; + const isUnchanged = + patchHasAutomation === shouldAttachInstall && + patchPolicy.patch_when_closed === flags.patch_when_closed && + patchPolicy.continuous_automations_enabled === + flags.continuous_automations_enabled; + if (!isUnchanged) { + await teamPoliciesAPI.update(patchPolicy.id, { + team_id: teamId, + software_title_id: shouldAttachInstall ? softwareTitle.id : null, + ...flags, + }); + savedAnyChange = true; + } + }🤖 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 `@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeployModal/DeployModal.tsx` around lines 130 - 146, In the patch-policy update branch, compute getPatchPolicyFlags(patchOption) once and compare the resulting flags with patchPolicy’s stored flags alongside the patch option change check. Replace the overlapping four-clause condition with this explicit comparison, then reuse the computed flags in teamPoliciesAPI.update while preserving the existing payload and savedAnyChange behavior.server/mdm/maintainedapps/sync.go (1)
262-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the remote manifest path.
Set
Queries.Openin the manifest fixture and assertapp.AppOpenQueryafter remote hydration. The current test only verifies the cache-hit path, so removal or incorrect mapping of this assignment would pass.🤖 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/mdm/maintainedapps/sync.go` at line 262, Add test coverage for the remote manifest hydration path by setting Queries.Open in the manifest fixture and asserting that app.AppOpenQuery receives the same value after hydration. Keep the existing cache-hit coverage intact and target the mapping represented by app.AppOpenQuery = selected.Queries.Open.
🤖 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 `@changes/39962-patch-when-closed`:
- Line 1: Update the “Patch when closed” change note to explicitly state that
the option applies only to Fleet-maintained applications, without implying
support for other patch targets.
In `@ee/maintained-apps/ingesters/homebrew/ingester.go`:
- Around line 296-297: Update GenerateOpenQuery in
ee/maintained-apps/ingesters/homebrew/ingester.go at lines 296-297 to pass the
existing Orbit path exclusion while retaining precise app-specific filtering.
Update the per-app expected query in
ee/maintained-apps/ingesters/homebrew/ingester_test.go at lines 227-231 and add
coverage confirming the excluded Orbit path is not matched.
In
`@frontend/components/ActivityDetails/InstallDetails/SoftwareInstallDetailsModal/SoftwareInstallDetailsModal.tsx`:
- Around line 149-165: The installed-inventory override must not take precedence
over the app-open skip state. Update overrideFailureWithInstalled and its caller
so installSkippedWhenAppOpen remains eligible for the skipped-install branch
even when hostSoftware.installed_versions is non-empty, while preserving
existing overrides for other failure states. Add a regression test covering
installed inventory with install_skipped_when_app_open set to true.
In
`@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeployModal/DeployModal.tsx`:
- Around line 150-164: Update the unsaved-error branch in the DeployModal save
catch block to provide a fallback message when getErrorReason(error) returns an
empty string. Preserve the parsed reason when available, otherwise show a clear
save-failure message while continuing to pass the error as the response.
In
`@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tsx`:
- Around line 268-269: Update the canDeploySoftware condition in
SoftwareSummaryCard to also require hasValidTeamId, preventing the Deploy action
from being enabled without a team. Add a regression test covering the All Teams
view and verifying Deploy is not enabled or actionable when no valid team ID
exists.
In `@server/datastore/mysql/software_installers.go`:
- Around line 143-146: Update the PatchWhenClosed handling around
result.PreInstallCondition so AppOpenQuery is assigned only for normal automated
patch requests. Add eligibility checks excluding self-service, manual, and Setup
Experience install modes, while preserving existing force-install behavior and
leaving other request paths unchanged.
In `@server/fleet/datastore.go`:
- Around line 3014-3017: Update ClearPreInstallQueryForTitle and its
implementation in the software installers datastore to execute cleanup within
one transaction: lock the active installer row, recheck that it is still active
and Fleet-maintained, conditionally clear its pre-install query, and cancel
pending installs before committing. Ensure concurrent edits or promotions cannot
overwrite a newly submitted query or apply changes to a stale installer.
In `@server/service/orbit.go`:
- Around line 1863-1873: Update the app-open skip handling around isAppOpenSkip
to use a primary datastore read for the software install result. Propagate any
GetSoftwareInstallResults error immediately and stop before storing the result,
rather than leaving isAppOpenSkip false; preserve the existing policy and
PatchWhenClosed checks after a successful lookup.
In `@server/service/team_policies.go`:
- Around line 107-112: The policy creation flow at
server/service/team_policies.go:107-112 and update flow at
server/service/team_policies.go:805-809 must persist the policy and clear the
conflicting title query within the same datastore transaction. Roll back the
policy create/save when ClearPreInstallQueryForTitle fails, return the wrapped
error, and add rollback tests covering that failure so no conflicting
configuration remains persisted.
---
Nitpick comments:
In
`@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeployModal/DeployModal.tsx`:
- Around line 130-146: In the patch-policy update branch, compute
getPatchPolicyFlags(patchOption) once and compare the resulting flags with
patchPolicy’s stored flags alongside the patch option change check. Replace the
overlapping four-clause condition with this explicit comparison, then reuse the
computed flags in teamPoliciesAPI.update while preserving the existing payload
and savedAnyChange behavior.
In `@server/mdm/maintainedapps/sync.go`:
- Line 262: Add test coverage for the remote manifest hydration path by setting
Queries.Open in the manifest fixture and asserting that app.AppOpenQuery
receives the same value after hydration. Keep the existing cache-hit coverage
intact and target the mapping represented by app.AppOpenQuery =
selected.Queries.Open.
🪄 Autofix
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 Plus
Run ID: f52ab0b3-f76a-44d8-a363-057190cf027f
📒 Files selected for processing (109)
changes/39962-patch-when-closedcmd/fleetctl/fleetctl/api_test.gocmd/fleetctl/fleetctl/generate_gitops.gocmd/fleetctl/fleetctl/generate_gitops_test.gocmd/fleetctl/fleetctl/testdata/expectedHostDetailResponseJson.jsoncmd/fleetctl/fleetctl/testdata/expectedHostDetailResponseYaml.ymlcmd/fleetctl/fleetctl/testdata/generateGitops/expectedTeamPolicies.yamlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/fleets/team-a-thumbsup.ymlcmd/fleetctl/fleetctl/testdata/generateGitops/test_dir_premium/fleets/unassigned.ymlcmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.goee/maintained-apps/ingesters/homebrew/ingester.goee/maintained-apps/ingesters/homebrew/ingester_test.goee/maintained-apps/ingesters/winget/ingester.goee/maintained-apps/ingesters/winget/ingester_test.goee/maintained-apps/maintained_apps.goee/server/service/maintained_apps.goee/server/service/maintained_apps_auto_update.goee/server/service/software_installers.goee/server/service/software_installers_test.gofrontend/__mocks__/softwareMock.tsfrontend/components/ActivityDetails/InstallDetails/SoftwareInstallDetailsModal/SoftwareInstallDetailsModal.tests.tsxfrontend/components/ActivityDetails/InstallDetails/SoftwareInstallDetailsModal/SoftwareInstallDetailsModal.tsxfrontend/components/ActivityDetails/InstallDetails/constants.tsfrontend/interfaces/activity.tsfrontend/interfaces/policy.tsfrontend/interfaces/software.tsfrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tests.tsxfrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsxfrontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppDetailsPage/FleetAppDetailsForm/FleetAppDetailsForm.tests.tsxfrontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppDetailsPage/FleetAppDetailsForm/FleetAppDetailsForm.tsxfrontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppDetailsPage/FleetMaintainedAppDetailsPage.tests.tsxfrontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppDetailsPage/FleetMaintainedAppDetailsPage.tsxfrontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppDetailsPage/helpers.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPatchPolicyModal/AddPatchPolicyModal.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPatchPolicyModal/AddPatchPolicyModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPatchPolicyModal/_styles.scssfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPatchPolicyModal/index.tsfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeployModal/DeployModal.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeployModal/DeployModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeployModal/_styles.scssfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeployModal/index.tsfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditSoftwareModal/EditSoftwareModal.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditSoftwareModal/EditSoftwareModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/PoliciesModal/PoliciesModal.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tests.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tsxfrontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/SoftwareDetailsSummary.tests.tsxfrontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/SoftwareDetailsSummary.tsxfrontend/pages/SoftwarePage/components/forms/AdvancedOptionsFields/AdvancedOptionsFields.tsxfrontend/pages/SoftwarePage/components/forms/PackageAdvancedOptions/PackageAdvancedOptions.tsxfrontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tests.tsxfrontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tsxfrontend/pages/SoftwarePage/components/forms/SoftwareDeploySelector/SoftwareDeploySelector.tests.tsxfrontend/pages/SoftwarePage/components/forms/SoftwareDeploySelector/SoftwareDeploySelector.tsxfrontend/pages/SoftwarePage/components/forms/SoftwareDeploySelector/_styles.scssfrontend/pages/SoftwarePage/components/forms/SoftwareDeploySelector/index.tsfrontend/pages/SoftwarePage/components/forms/SoftwareDeploySlider/SoftwareDeploySlider.tests.tsxfrontend/pages/SoftwarePage/components/forms/SoftwareDeploySlider/SoftwareDeploySlider.tsxfrontend/pages/SoftwarePage/components/forms/SoftwareDeploySlider/index.tsfrontend/pages/SoftwarePage/components/forms/SoftwareVppForm/SoftwareVppForm.tests.tsxfrontend/pages/SoftwarePage/components/forms/SoftwareVppForm/SoftwareVppForm.tsxfrontend/pages/hosts/details/cards/Activity/ActivityItems/InstalledSoftwareActivityItem/InstalledSoftwareActivityItem.tests.tsxfrontend/pages/hosts/details/cards/Activity/ActivityItems/InstalledSoftwareActivityItem/InstalledSoftwareActivityItem.tsxfrontend/pages/policies/components/PolicyAutomationsFields/PolicyAutomationsFields.tests.tsxfrontend/pages/policies/components/PolicyAutomationsFields/PolicyAutomationsFields.tsxfrontend/pages/policies/details/components/PolicyAutomationActivityDetailsModal/PolicyAutomationActivityDetailsModal.tsxfrontend/pages/policies/details/components/PolicyAutomationsActivitiesTable/PolicyAutomationsActivitiesTable.tests.tsxfrontend/pages/policies/details/components/PolicyAutomationsActivitiesTable/PolicyAutomationsActivitiesTableConfig.tsxfrontend/pages/policies/details/components/PolicyAutomationsActivitiesTable/helpers.tsxfrontend/pages/policies/edit/components/PolicyForm/PolicyForm.tests.tsxfrontend/pages/policies/edit/components/PolicyForm/PolicyForm.tsxfrontend/pages/policies/hooks/useUpdatePolicyAutomations.tsfrontend/services/entities/software.tsfrontend/services/entities/team_policies.tests.tsfrontend/services/entities/team_policies.tsfrontend/test/handlers/software-handlers.tspkg/patch_policy/patch_policy.gopkg/patch_policy/patch_policy_test.gopkg/spec/gitops.gopkg/spec/gitops_test.goserver/datastore/mysql/migrations/tables/20260807140831_PatchWhenClosed.goserver/datastore/mysql/migrations/tables/20260807140831_PatchWhenClosed_test.goserver/datastore/mysql/policies.goserver/datastore/mysql/policies_test.goserver/datastore/mysql/schema.sqlserver/datastore/mysql/software_installers.goserver/datastore/mysql/software_installers_test.goserver/datastore/mysql/software_titles.goserver/datastore/mysql/teams_test.goserver/fleet/activities.goserver/fleet/api_policies.goserver/fleet/datastore.goserver/fleet/maintained_apps.goserver/fleet/policies.goserver/fleet/software_installer.goserver/fleet/software_test.goserver/mdm/maintainedapps/sync.goserver/mdm/maintainedapps/sync_test.goserver/mock/datastore_mock.goserver/service/global_policies.goserver/service/global_policies_test.goserver/service/integration_enterprise_test.goserver/service/orbit.goserver/service/orbit_test.goserver/service/software_installers.goserver/service/software_titles.goserver/service/team_policies.goserver/service/team_policies_test.goserver/webhooks/failing_policies_test.go
💤 Files with no reviewable changes (7)
- frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPatchPolicyModal/_styles.scss
- frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPatchPolicyModal/index.ts
- frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPatchPolicyModal/AddPatchPolicyModal.tests.tsx
- frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPatchPolicyModal/AddPatchPolicyModal.tsx
- frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/PoliciesModal/PoliciesModal.tsx
- server/datastore/mysql/teams_test.go
- frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppDetailsPage/helpers.tsx
| @@ -0,0 +1 @@ | |||
| - Added a "Patch when closed" option for patch policies that only patches an app on a host if the app is not running. | |||
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Limit the change note to Fleet-maintained applications.
The PR adds this option for Fleet-maintained app patching. The current text does not state this scope. Clarify the note to avoid promising support for other patch targets.
Based on the PR objective, this feature is limited to Fleet-maintained applications.
Proposed wording
-Added a "Patch when closed" option for patch policies that only patches an app on a host if the app is not running.
+Added a "Patch when closed" option for Fleet-maintained app patch policies. It patches the app only when the app is not running.🤖 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 `@changes/39962-patch-when-closed` at line 1, Update the “Patch when closed”
change note to explicitly state that the option applies only to Fleet-maintained
applications, without implying support for other patch targets.
Comment on lines
+296
to
+297
| out.Queries.Open = patch_policy.GenerateOpenQuery("darwin", out.UniqueIdentifier, "") | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve app-specific path scoping in the managed open query.
The Homebrew producer generates a bundle-identifier-only query, while the test asserts that generic query for every app. A running Orbit swiftDialog copy can therefore make the query fail and defer patching of the maintained app.
ee/maintained-apps/ingesters/homebrew/ingester.go#L296-L297: carry the existing Orbit path exclusion intoQueries.Open.ee/maintained-apps/ingesters/homebrew/ingester_test.go#L227-L231: use a per-app expected query and add coverage for the excluded Orbit path.
As per path instructions, retain precise filtering for the target app in this SQL query.
📍 Affects 2 files
ee/maintained-apps/ingesters/homebrew/ingester.go#L296-L297(this comment)ee/maintained-apps/ingesters/homebrew/ingester_test.go#L227-L231
🤖 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 `@ee/maintained-apps/ingesters/homebrew/ingester.go` around lines 296 - 297,
Update GenerateOpenQuery in ee/maintained-apps/ingesters/homebrew/ingester.go at
lines 296-297 to pass the existing Orbit path exclusion while retaining precise
app-specific filtering. Update the per-app expected query in
ee/maintained-apps/ingesters/homebrew/ingester_test.go at lines 227-231 and add
coverage confirming the excluded Orbit path is not matched.
Source: Path instructions
Comment on lines
+149
to
+165
| if (installSkippedWhenAppOpen && status === "failed_install") { | ||
| return ( | ||
| <IconStatusMessage | ||
| className={`${baseClass}__status-message`} | ||
| iconName={INSTALL_DETAILS_STATUS_ICONS.skipped_install} | ||
| iconColor="ui-fleet-black-50" | ||
| message={ | ||
| <span> | ||
| Fleet skipped install of <b>{software_title}</b> ({software_package} | ||
| ) on {formattedHost} | ||
| {displayTimeStamp}. The app was open. It will update once the user | ||
| closes it and policy runs again, or update via self service. | ||
| </span> | ||
| } | ||
| /> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not override an app-open skip with installed inventory.
When hostSoftware.installed_versions is non-empty, overrideFailureWithInstalled returns “is installed” before this branch runs. A deferred patch normally has an existing installed version, so the Host → Software view hides both the skipped status and its Details section.
Exclude installSkippedWhenAppOpen from the installed-inventory override. Add a regression test that passes installed inventory and install_skipped_when_app_open: true.
🤖 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
`@frontend/components/ActivityDetails/InstallDetails/SoftwareInstallDetailsModal/SoftwareInstallDetailsModal.tsx`
around lines 149 - 165, The installed-inventory override must not take
precedence over the app-open skip state. Update overrideFailureWithInstalled and
its caller so installSkippedWhenAppOpen remains eligible for the skipped-install
branch even when hostSoftware.installed_versions is non-empty, while preserving
existing overrides for other failure states. Add a regression test covering
installed inventory with install_skipped_when_app_open set to true.
Comment on lines
+150
to
+164
| } catch (error) { | ||
| if (savedAnyChange) { | ||
| // A partial save (e.g. Force install created, then the patch policy | ||
| // failed): tell the user some changes landed — mirroring the Add | ||
| // flow — rather than only the raw error, then refresh + close so a | ||
| // retry starts from the real state. | ||
| notify.error( | ||
| "Some changes were saved, but others couldn't be. Try again.", | ||
| { response: error } | ||
| ); | ||
| onSuccess(); | ||
| onExit(); | ||
| } else { | ||
| notify.error(getErrorReason(error), { response: error }); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect getErrorReason to see what it returns for non-Axios errors.
fd -t f 'errors.ts' frontend/interfaces --exec cat -n {}Repository: fleetdm/fleet
Length of output: 8474
🏁 Script executed:
#!/bin/bash
# Inspect the referenced DeployModal location and nearby error handling/comment context.
fd -t f 'DeployModal.tsx' frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeployModal --exec sh -c '
file="$1"
echo "== $file =="
wc -l "$file"
sed -n "70,175p" "$file" | cat -n
' sh {}Repository: fleetdm/fleet
Length of output: 4658
Add a fallback when getErrorReason returns an empty message.
getErrorReason returns "" for non-API payloads, so this toast can show no message when savedAnyChange is false. Use a direct message for errors getErrorReason does not parse, for example:
notify.error(getErrorReason(error) || "Couldn't save the deploy settings. Try again.", { response: error }).
🤖 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
`@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeployModal/DeployModal.tsx`
around lines 150 - 164, Update the unsaved-error branch in the DeployModal save
catch block to provide a fallback message when getErrorReason(error) returns an
empty string. Preserve the parsed reason when available, otherwise show a clear
save-failure message while continuing to pass the error as the response.
Comment on lines
+268
to
+269
| const canDeploySoftware = | ||
| canManageSoftware && isFleetMaintainedApp && !!isPremiumTier; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require a team before enabling Deploy.
canDeploySoftware does not require hasValidTeamId. When teamId is absent, line 319 passes onClickDeploy, but softwareInstallerOnTeam is false and line 372 does not render DeployModal. The Deploy action then has no visible result.
Include hasValidTeamId in canDeploySoftware. Add a regression test for the All Teams view.
Proposed fix
+ const hasValidTeamId = typeof teamId === "number" && teamId >= 0;
+ const softwareInstallerOnTeam = hasValidTeamId && softwareInstaller;
+
const canDeploySoftware =
- canManageSoftware && isFleetMaintainedApp && !!isPremiumTier;
+ canManageSoftware &&
+ isFleetMaintainedApp &&
+ !!isPremiumTier &&
+ hasValidTeamId;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const canDeploySoftware = | |
| canManageSoftware && isFleetMaintainedApp && !!isPremiumTier; | |
| const hasValidTeamId = typeof teamId === "number" && teamId >= 0; | |
| const softwareInstallerOnTeam = hasValidTeamId && softwareInstaller; | |
| const canDeploySoftware = | |
| canManageSoftware && | |
| isFleetMaintainedApp && | |
| !!isPremiumTier && | |
| hasValidTeamId; |
🤖 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
`@frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareSummaryCard/SoftwareSummaryCard.tsx`
around lines 268 - 269, Update the canDeploySoftware condition in
SoftwareSummaryCard to also require hasValidTeamId, preventing the Deploy action
from being enabled without a team. Add a regression test covering the All Teams
view and verifying Deploy is not enabled or actionable when no valid team ID
exists.
Comment on lines
+143
to
+146
| // A patch-when-closed policy install uses the installer's app open query as its pre-install condition. | ||
| if result.PatchWhenClosed { | ||
| result.PreInstallCondition = result.AppOpenQuery | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Exclude install modes that must bypass managed gating.
Line 143 replaces PreInstallCondition for every PatchWhenClosed result. It does not exclude self-service, manual, Setup Experience, or force install paths.
A request linked to a patch policy can receive the app-open condition when the product requires that request to install while the app is open. Add an eligibility check before this replacement. Apply AppOpenQuery only to normal automated patch requests.
As per PR objectives, managed gating must be ignored for self-service, manual, and Setup Experience installs, and force behavior must remain unchanged.
🤖 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/datastore/mysql/software_installers.go` around lines 143 - 146, Update
the PatchWhenClosed handling around result.PreInstallCondition so AppOpenQuery
is assigned only for normal automated patch requests. Add eligibility checks
excluding self-service, manual, and Setup Experience install modes, while
preserving existing force-install behavior and leaving other request paths
unchanged.
Comment on lines
+3014
to
+3017
| // ClearPreInstallQueryForTitle blanks the pre-install query on a title's active Fleet-maintained | ||
| // installer and cancels its pending installs. No-op when the query is already empty. | ||
| ClearPreInstallQueryForTitle(ctx context.Context, teamID uint, titleID uint) error | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make title query cleanup atomic.
The supplied implementation in server/datastore/mysql/software_installers.go, Lines 1993 through 2018, selects an active installer, updates it by ID, and then runs side effects as separate operations. A concurrent installer edit can overwrite a newly submitted user query. A concurrent promotion can also make the selected installer stale.
Lock and recheck the active row. Perform the conditional clear and pending-install cancellation in one transaction.
🤖 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 3014 - 3017, Update
ClearPreInstallQueryForTitle and its implementation in the software installers
datastore to execute cleanup within one transaction: lock the active installer
row, recheck that it is still active and Fleet-maintained, conditionally clear
its pre-install query, and cancel pending installs before committing. Ensure
concurrent edits or promotions cannot overwrite a newly submitted query or apply
changes to a stale installer.
Comment on lines
+1863
to
+1873
| // A patch-when-closed policy install whose managed app-open query returned no result means the | ||
| // app was open: a skip, not a failure. Key on the policy flag, not empty output, so an ordinary | ||
| // empty pre_install_query on a non-managed policy still fails and counts toward the retry cap. | ||
| isAppOpenSkip := false | ||
| if result.Status() == fleet.SoftwareInstallFailed && | ||
| result.PreInstallConditionOutput != nil && *result.PreInstallConditionOutput == "" { | ||
| if cur, curErr := svc.ds.GetSoftwareInstallResults(ctx, result.InstallUUID); curErr == nil && cur != nil { | ||
| isAppOpenSkip = cur.PolicyID != nil && cur.PatchWhenClosed | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not downgrade an app-open skip when the result lookup fails.
Lines 1869-1871 ignore lookup errors and leave isAppOpenSkip false. A read-replica lag or datastore error can then record a managed skip as a normal failure, consume a retry attempt, and queue an immediate retry.
Require a primary read and return the lookup error before storing the result.
🤖 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/service/orbit.go` around lines 1863 - 1873, Update the app-open skip
handling around isAppOpenSkip to use a primary datastore read for the software
install result. Propagate any GetSoftwareInstallResults error immediately and
stop before storing the result, rather than leaving isAppOpenSkip false;
preserve the existing policy and PatchWhenClosed checks after a successful
lookup.
Comment on lines
+107
to
+112
| //nolint:nilaway // ds.NewTeamPolicy returns an error whenever policy is nil | ||
| if policy.Type == fleet.PolicyTypePatch && policy.PatchWhenClosed && policy.PatchSoftwareTitleID != nil { | ||
| if err := svc.ds.ClearPreInstallQueryForTitle(ctx, teamID, *policy.PatchSoftwareTitleID); err != nil { | ||
| return nil, ctxerr.Wrap(ctx, err, "clear pre-install query for title") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make policy persistence and managed-query cleanup atomic.
Both paths persist PatchWhenClosed before clearing the conflicting pre-install query. If cleanup fails, the API returns an error but leaves a patch-when-closed policy active with the user pre-install query still stored.
server/service/team_policies.go#L107-L112: create the policy and clear the title query in one datastore transaction.server/service/team_policies.go#L805-L809: save the policy and clear the title query in the same datastore transaction.
Add rollback tests for a ClearPreInstallQueryForTitle failure. The feature must not persist a conflicting configuration.
📍 Affects 1 file
server/service/team_policies.go#L107-L112(this comment)server/service/team_policies.go#L805-L809
🤖 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/service/team_policies.go` around lines 107 - 112, The policy creation
flow at server/service/team_policies.go:107-112 and update flow at
server/service/team_policies.go:805-809 must persist the policy and clear the
conflicting title query within the same datastore transaction. Roll back the
policy create/save when ClearPreInstallQueryForTitle fails, return the wrapped
error, and add rollback tests covering that failure so no conflicting
configuration remains persisted.
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
4 participants
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 #39962
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.
Input data is properly validated,
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.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Database migrations
COLLATE utf8mb4_unicode_ci).New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsSummary by CodeRabbit
New Features
UI Improvements
Bug Fixes