Fix macOS software titles mis-named from embedded helper bundles (#44199) - #47831
Merged
Conversation
added 4 commits
June 18, 2026 06:24
…es under parent Reproduces #44199 by emitting embedded-helper paths (/some/path/Common_N.app/Contents/Library/LoginItems/<name>.app) when the new flag is set, enabling load tests of the ingestion filter and title backfill migration that follow. Default off — existing duplicate-bundle behavior unchanged.
…uery osquery's apps table reports both a parent app and any embedded login helper under Contents/Library/LoginItems as separate rows, often sharing a bundle identifier. The helper's name would clobber the software title's display name (e.g. AmphetamineLoginHelper instead of Amphetamine). Exclude paths nested under .app/Contents/ so only top-level bundles are ingested. Fixes #44199 prospectively. Existing mis-named titles are addressed by the follow-up backfill migration.
For macOS app titles whose software_titles.name was set from an
embedded helper bundle (e.g. AmphetamineLoginHelper) rather than the
parent app, recompute the name from the title's sibling software rows
using the same precedence as title creation:
1. Fleet-maintained app canonical name (if bundle id matches), else
2. longest-common-prefix of sibling names (trailing non-word chars
trimmed), else
3. shortest sibling name.
The migration drives off the indexed software_titles JOIN software
join (title_id) and only UPDATEs titles whose name actually changes —
deliberately not path-based, since host_software_installed_paths has
no global per-software source of truth and would force a full
unindexed TEXT scan inside the startup-blocking migration
transaction.
Pairs with the queries.go filter that prevents new mis-named titles
from forming.
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR fixes a macOS software title naming issue where a login-helper app nested inside another app bundle could be treated as the canonical title name. Future ingestion excludes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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.
🧹 Nitpick comments (1)
server/datastore/mysql/migrations/tables/20260618124430_FixEmbeddedBundleTitleNames.go (1)
105-120: 💤 Low valueConsider deterministic tie-breaking for equal-length names.
The slice at line 105 is built from map iteration, which is non-deterministic in Go. If two sibling names have identical length, the "shortest" selection depends on iteration order.
For this one-shot migration with typical helper naming patterns, the practical impact is minimal, but sorting
nameslexicographically before the shortest-selection loop would make results reproducible.♻️ Optional fix for deterministic selection
+import "sort" + func fixEmbeddedPickTitleName(siblings map[string]struct{}, bundleID string, fmaNames map[string]string) string { if name, ok := fmaNames[bundleID]; ok && name != "" { return name } names := make([]string, 0, len(siblings)) for n := range siblings { names = append(names, n) } + sort.Strings(names) prefix := fixEmbeddedLongestCommonPrefix(names)🤖 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/migrations/tables/20260618124430_FixEmbeddedBundleTitleNames.go` around lines 105 - 120, The names slice is built from non-deterministic map iteration over the siblings map, which means when multiple sibling names have identical length, the shortest name selection depends on the iteration order. Sort the names slice lexicographically before the loop that finds the shortest name by calling sort.Strings(names) after the prefix check and before the loop that iterates through names[1:] to select the shortest, ensuring deterministic results regardless of map iteration order.
🤖 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.
Nitpick comments:
In
`@server/datastore/mysql/migrations/tables/20260618124430_FixEmbeddedBundleTitleNames.go`:
- Around line 105-120: The names slice is built from non-deterministic map
iteration over the siblings map, which means when multiple sibling names have
identical length, the shortest name selection depends on the iteration order.
Sort the names slice lexicographically before the loop that finds the shortest
name by calling sort.Strings(names) after the prefix check and before the loop
that iterates through names[1:] to select the shortest, ensuring deterministic
results regardless of map iteration order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cfb47ebb-5c84-4c09-90ca-4bffb1a89949
⛔ Files ignored due to path filters (1)
docs/Contributing/product-groups/orchestration/understanding-host-vitals.mdis excluded by!**/*.md
📒 Files selected for processing (5)
changes/44199-embedded-bundle-title-namecmd/osquery-perf/agent.goserver/datastore/mysql/migrations/tables/20260618124430_FixEmbeddedBundleTitleNames.goserver/datastore/mysql/migrations/tables/20260618124430_FixEmbeddedBundleTitleNames_test.goserver/service/osquery_utils/queries.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47831 +/- ##
==========================================
- Coverage 68.11% 68.07% -0.04%
==========================================
Files 3709 3689 -20
Lines 235099 234997 -102
Branches 12352 12274 -78
==========================================
- Hits 160129 159976 -153
- Misses 60594 60634 +40
- Partials 14376 14387 +11
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:
|
Refactor Up_20260618124430 to delegate the title scan to fixEmbeddedScanTitles, where defer rows.Close() satisfies sqlclosecheck without the manual error-path close.
Contributor
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR fixes incorrect macOS software title names caused by embedded helper bundles (e.g., LoginItems) sharing the parent app’s bundle identifier, ensuring the parent app name wins both for new ingests and for existing data.
Changes:
- Filter macOS
appsrows whosepathindicates an embedded bundle under*.app/Contents/*during osquery ingestion. - Add a one-time MySQL migration to recompute affected
software_titles.namevalues using FMA name override → longest common prefix → shortest fallback. - Extend
cmd/osquery-perfto optionally generate embedded-bundle installed paths so the scenario is reproducible at scale.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/service/osquery_utils/queries.go | Excludes embedded *.app/Contents/* paths from the macOS apps software query to prevent future mis-naming. |
| server/datastore/mysql/schema.sql | Advances migration_status_tables seed data / AUTO_INCREMENT to include the new migration. |
| server/datastore/mysql/migrations/tables/20260618124430_FixEmbeddedBundleTitleNames.go | Implements the backfill migration that recomputes software_titles.name for affected app titles. |
| server/datastore/mysql/migrations/tables/20260618124430_FixEmbeddedBundleTitleNames_test.go | Adds coverage validating the migration’s name-picking precedence and out-of-scope behavior. |
| cmd/osquery-perf/agent.go | Adds --embedded_bundle_paths option to generate nested Contents/Library/LoginItems/... paths for duplicate-bundle scenarios. |
| changes/44199-embedded-bundle-title-name | User-visible change entry (content excluded from review by policy). |
| docs/Contributing/product-groups/orchestration/understanding-host-vitals.md | Documentation update (content excluded from review by policy). |
Files excluded by content exclusion policy (2)
- changes/44199-embedded-bundle-title-name
- docs/Contributing/product-groups/orchestration/understanding-host-vitals.md
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fixEmbeddedLoadFMANamesDarwin now wraps query/scan/iteration errors with descriptive messages, matching the sibling fixEmbeddedScanTitles helper. Makes migration failure traces pinpoint the failing step.
mostlikelee
marked this pull request as ready for review
June 18, 2026 18:49
mostlikelee
requested review from
a team,
sgress454 and
sharon-fdm
as code owners
June 18, 2026 18:49
![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.
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
| last_opened_time AS last_opened_at, | ||
| path AS installed_path | ||
| FROM apps | ||
| WHERE path NOT LIKE '%%.app/Contents/%%' |
Member
|
@mostlikelee Does this include dropping embedded apps that have different AppIDs? |
Contributor
Author
Indeed, that's how we spec'd with product |
Member
|
@mostlikelee Re-dump the schema and then it's good to go 👍 |
…itles-44199 # Conflicts: # server/datastore/mysql/schema.sql
Contributor
Author
|
@dantecatalfamo should be g2g |
dantecatalfamo
approved these changes
Jul 8, 2026
Contributor
Author
|
@sharon-fdm this needs an approval for host vitals docs |
sharon-fdm
approved these changes
Jul 9, 2026
sharon-fdm
left a comment
sharon-fdm
left a comment
Collaborator
There was a problem hiding this comment.
I only reviewed docs/Contributing/product-groups/orchestration/understanding-host-vitals.md
Relying on Dante's approval for the rest.
1 task
This was referenced Aug 10, 2026
allenhouchins
added a commit
that referenced
this pull request
Aug 10, 2026
**Related issue:** Resolves #50875 Docker Desktop on macOS never reported an installed version or "Installed" status, and offered "Install" on hosts that already had it. ## Root cause The FMA's `unique_identifier` was `com.electron.dockerdesktop`, which belongs to the embedded Electron bundle. The installed app reports a different identifier: | Path | CFBundleIdentifier | |---|---| | `/Applications/Docker.app` | `com.docker.docker` | | `/Applications/Docker.app/Contents/MacOS/Docker Desktop.app` | `com.electron.dockerdesktop` | The identifier was changed from `com.docker.docker` → `com.electron.dockerdesktop` in #37670 (Jan 5) as "the new bundle identifier … reflecting the current packaging". The top-level bundle never changed. That was latent until #47831 (#44199, Jul 9) added an embedded-bundle filter to the macOS software inventory query in `server/service/osquery_utils/queries.go`: ```sql FROM apps WHERE path NOT LIKE '%.app/Contents/%' ``` `/Applications/Docker.app/Contents/MacOS/Docker Desktop.app` matches that pattern, so the only row carrying the embedded identifier is filtered out and inventory keeps just `/Applications/Docker.app` → `com.docker.docker`. Since FMA↔inventory matching is by bundle identifier (`addSoftwareTitleToMatchingSoftware`), the title the FMA owns had zero installed versions. **Patch policies kept passing**, because they run the FMA's `exists`/`patched` SQL directly against the host's *unfiltered* `apps` table. That's also why #50041 needed a `.back` path exclusion, and why patch status and the software UI have disagreed since July. ## What changed **Catalog** — `unique_identifier` is now `com.docker.docker`, with `docker-desktop/darwin.json` regenerated. `outputs/apps.json` needed a hand-edit because `updateAppsListFile` (`cmd/maintained-apps/main.go`) only appends new apps and never updates an existing entry's identifier — filed separately. The remaining `com.electron.dockerdesktop` references are intentional and untouched: the install script's quit/relaunch targets (the Electron bundle is what responds to AppleScript) and the cask's zap paths. The `.back` exclusion in the patched query also stays — `/Applications/Docker.app.back` is a *top-level* bundle reporting `com.docker.docker` at a path the nested-bundle filter does not match, so a stale `.back` would otherwise show a false "Update available". **Migration** (`20260810152924_FixDockerDesktopBundleIdentifier`) — `fleet_maintained_apps` self-heals on catalog sync (`UpsertMaintainedApp` updates `unique_identifier` on duplicate slug) and `ReconcileMaintainedAppSoftwareNames` renames the existing "Docker" title, but an already-added installer's `software_installers.title_id` binding does not: - **No `com.docker.docker` title yet** → relabel the stale title in place, so everything already pointing at it stays correct. - **Title already exists** (the normal case — any host with Docker creates it) → merge the stale title into it: installer, install history, queued installs, patch policy, `software.title_id`, and per-team settings (icons, display names, pins, update schedules), then drop the stale title. ## Notes for reviewers - **Teams that already have an installer on the target title are skipped** rather than ending up with two installers on one title. `dedup_token` is the *version* for FMAs and the *storage_id* otherwise, so `idx_software_installers_dedup` would not have caught that collision. Those teams keep the pre-migration state instead of having data silently reshaped. - **The stale title is only deleted once nothing depends on it.** `fk_software_installers_title` is `ON DELETE SET NULL`, so deleting it while an installer still pointed at it would orphan that installer. Note also that `fk_patch_software_title_id` is `ON DELETE CASCADE` — re-pointing the patch policy is what keeps it from being deleted outright. - **Dangling-reference check:** the only title-referencing columns without an FK to `software_titles` are `software.title_id` (re-pointed), `software_titles_host_counts` (deleted; the cron recomputes), and `kernel_host_counts` / `in_house_app_install_tokens`, neither of which can hold a macOS app title. - **Naming lags briefly.** The existing title is named "Docker" (from osquery); `ReconcileMaintainedAppSoftwareNames` renames it to "Docker Desktop" on the next catalog sync, so there is a window after upgrade where the name is still "Docker". I left that to the sync rather than duplicating the rename logic in the migration. - **This fixes one app, not the class.** Any other macOS FMA keyed on a nested bundle fails the same silent way, and a green patch policy will not reveal it. An audit is filed separately. # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements). Table/column names in the migration's generated SQL come from hardcoded struct literals, never from data; all values are placeholders. ## Testing - [x] Added/updated automated tests Five migration tests cover: relabel-in-place, merge-into-inventory-title, duplicate per-team settings dropped, teams with an existing installer skipped (and the stale title consequently retained), and no-op when the FMA was never added. The merge test also asserts `updated_at` is not restamped. Updated the homebrew ingester test expectations for the new identifier. Verified: full `server/datastore/mysql/migrations/tables` suite passes (201s), `ee/maintained-apps/...`, `cmd/maintained-apps/...`, and the FMA datastore tests pass, `make lint-go-incremental` clean. Root cause was confirmed against real bundles rather than inferred — `PlistBuddy` on both Docker bundles for the identifiers above, and `lsregister -dump` to confirm LaunchServices registers the nested bundles (which is why the raw `apps` table sees them and patch policies pass). - [ ] QA'd all new/changed functionality manually Needs QA on a real instance: add the Docker Desktop FMA, confirm the installed version and "Installed" status appear on a host that already has it, and confirm an upgrade over an instance that already had the FMA added re-points the existing installer. For unreleased bug fixes in a release candidate, one of: - [x] Confirmed that the fix is not expected to adversely impact load test results The migration touches only rows tied to a single software title, and is a no-op on instances that never added the Docker Desktop FMA. ## Database migrations - [x] Checked schema for all modified table for columns that will auto-update timestamps during migration. - [x] Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects. Five of the written tables have `updated_at` as `ON UPDATE CURRENT_TIMESTAMP`: `software_installers`, `host_software_installs`, `software_install_upcoming_activities`, `policies`, and `software_title_team_pins`. Since this re-points a foreign key rather than modifying the records, each statement assigns `updated_at = updated_at` so MySQL leaves them alone, with a test asserting it. (Bumping them would have been cosmetic — none of these columns drives ordering, scheduling, or invalidation; policy membership uses `policy_membership.updated_at` and `hosts.policy_updated_at`, and the activity feed orders by the activities table — but preserving them is more faithful to what the migration actually does.) - [x] Ensured the correct collation is explicitly set for character columns (`COLLATE utf8mb4_unicode_ci`). No columns added or altered; this is a data-only migration. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved macOS Docker Desktop detection so installed versions and “Installed” status are reported accurately. * Ensured existing Docker Desktop installations and upgrade history remain correctly associated after detection updates. * Improved handling of stale application bundles during patch evaluation. * **Maintenance** * Updated detection data and migration coverage to support the corrected Docker Desktop identification. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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 #44199
osquery reports an app's embedded login helper under
Contents/Library/LoginItems/as a separateappsrow sharing the parent's bundle identifier; whichever row created thesoftware_titlesrow wins its name, often the helper. Two changes pair:queries.go): drop rows whosepathmatches%.app/Contents/%. Prospective.appstitles from sibling software names (FMA → longest-common-prefix → shortest), UPDATE only on diff.cmd/osquery-perfgains--embedded_bundle_pathsto nest duplicate-bundle paths so the bug is reproducible at scale.Migration perf (osquery-perf library, the largest realistic dataset available)
EXPLAIN ANALYZE: PK scan onsoftware_titles+title_id-indexed lookups onsoftware. No table scans, no filesort. Linear extrapolation to ~5× scale stays well under 1s.Checklist for submitter
Changes file added for user-visible changes in
changes/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
For unreleased bug fixes in a release candidate, one of:
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Contents.Database / Migration