[getvictor]

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 for more information.

• Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)

• 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

For unreleased bug fixes in a release candidate, one of:

• Confirmed that the fix is not expected to adversely impact load test results
• Alerted the release DRI if additional load testing is needed

Database migrations

• Checked table schema to confirm autoupdate
• Checked schema for all modified table for columns that will auto-update timestamps during migration.
• Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects.
• Ensured the correct collation is explicitly set for character columns (COLLATE utf8mb4_unicode_ci).

New Fleet configuration settings

• Setting(s) is/are explicitly excluded from GitOps

If you didn't check the box above, follow this checklist for GitOps-enabled settings:

• Verified that the setting is exported via fleetctl generate-gitops
• Verified the setting is documented in a separate PR to the GitOps documentation
• Verified that the setting is cleared on the server if it is not supplied in a YAML file (or that it is documented as being optional)
• Verified that any relevant UI is disabled when GitOps mode is enabled

fleetd/orbit/Fleet Desktop

• Verified compatibility with the latest released version of Fleet (see Must rule)
• If the change applies to only one platform, confirmed that runtime.GOOS is used as needed to isolate changes
• Verified that fleetd runs on macOS, Linux and Windows
• Verified auto-update works from the released version of component to the new version (see tools/tuf/test)

Summary by CodeRabbit

• Bug Fixes

  • Prevents duplicate macOS software entries when users rename apps, reusing records by bundle ID.
  • Ensures app names and sources update consistently, improving accuracy of titles and installed versions.
  • Improves handling of browser extensions across variants and version changes.

• Performance

  • Optimizes ingestion with batched updates for name changes, reducing overhead and improving consistency.

[codecov]

Codecov Report

❌ Patch coverage is 83.82353% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.09%. Comparing base (907ea5d) to head (55ee5a8).

Files with missing lines	Patch %	Lines
server/datastore/mysql/software.go	83.82%	7 Missing and 4 partials ⚠️

Additional details and impacted files

@@                       Coverage Diff                        @@
##           victor/33298-software-ingestion   #33390   +/-   ##
================================================================
  Coverage                            64.08%   64.09%           
================================================================
  Files                                 2059     2059           
  Lines                               205256   205242   -14     
  Branches                              6767     6767           
================================================================
- Hits                                131548   131547    -1     
+ Misses                               63332    63313   -19     
- Partials                             10376    10382    +6     

Flag	Coverage Δ
backend	65.21% <83.82%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[getvictor]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

Walkthrough

Revises macOS software ingestion to handle renamed apps by deduplicating via bundle IDs and batch-renaming matched entries. Introduces a helper to update names and sources in bulk. Expands tests to cover rename scenarios mixed with new software, browser extensions handling, and updated expectations for name and source updates.

Changes

Cohort / File(s)	Summary
Datastore ingestion logic server/datastore/mysql/software.go	Adds bundle-ID-based rename aggregation and batch update via updateTargetedBundleIDs. Replaces per-item updates with a CASE-based batched UPDATE for software.name and software.name_source. Minor comments adjusted.
Datastore tests server/datastore/mysql/software_test.go	Adds tests for mixed rename/new software, browser extensions without bundle IDs, and updates existing rename-only test and assertions to reflect name/name_source updates. Introduces new test helpers and renames one test function.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Host as Host
  participant API as Ingest API
  participant DS as Datastore (MySQL)

  Host->>API: Report installed software (names, bundle IDs, versions)
  API->>DS: Link by bundle ID
  Note over DS: Identify existing entries matching bundle IDs
  DS-->>API: Matched software IDs
  API->>DS: Insert new software (no bundle ID match)
  Note over API,DS: Build softwareRenames map for bundle-ID matches with differing names
  API->>DS: updateTargetedBundleIDs(softwareRenames)
  activate DS
  DS->>DS: Batched UPDATE (CASE) name and name_source
  deactivate DS
  DS-->>API: Completed updates
  API-->>Host: Ingestion result

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

• sgress454
• sharon-fdm

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The PR description is just the unfilled repository template with the "Related issue" blank and all checklist items left unchecked, and it provides no summary of the actual code changes, motivation, or testing performed. Because it lacks a concise description of what changed (e.g., bundle ID rename handling, name normalization, deduplication, and added tests), reviewers cannot assess scope, risks, or required QA. This is insufficient for a productive review.	Please update the PR description with a short summary of the changes and motivation (mention bundle ID/name normalization and deduplication behavior and tests added), include the related issue number if applicable, and add the path to any changes file; complete the submitter checklist especially the testing and "Changes file added" items. Add brief testing notes describing how the behavior was validated and any load or migration considerations, and call out any database compatibility concerns or steps to reproduce the original issue so reviewers can verify the fix.
Docstring Coverage	⚠️ Warning	Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title "Software ingestion fixes (merging Konstantin's PR)" correctly identifies the primary intent—fixes to software ingestion—and aligns with the changes to bundle ID matching, name normalization, and deduplication present in the diff and tests. It is concise and reads as a single sentence, though the parenthetical about merging another PR is extraneous. The title is related and not misleading for reviewers scanning history.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch 33298-33140-merge

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

• Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
• Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

server/datastore/mysql/software.go (1)

748-750: Bug: clobbers previously computed titles (map reinit wipes entries)

Reinitializing incomingChecksumToTitle here erases mappings already populated for software without bundle_identifier, causing missing title_id assignment downstream.

Apply this diff:

-		// no-op code change
-		incomingChecksumToTitle = make(map[string]fleet.SoftwareTitle, len(newSoftwareChecksums))
+		// keep previously collected titles (do not reinitialize incomingChecksumToTitle)

Please add/adjust a test to cover a mixed batch of software with and without bundle_identifier to prevent regressions.

🧹 Nitpick comments (5)

server/datastore/mysql/software.go (1)

483-524: Defensive CASE: add ELSE name to avoid accidental NULLs

If an id slips into the WHERE but not into the CASE (unexpected but possible under refactors), name would become NULL. Make the CASE default to the current value.

Apply this diff:

-		updateStmt := fmt.Sprintf(
-			`UPDATE software SET name = CASE id %s END, name_source = 'bundle_4.67' WHERE id IN (%s)`,
+		updateStmt := fmt.Sprintf(
+			`UPDATE software SET name = CASE id %s ELSE name END, name_source = 'bundle_4.67' WHERE id IN (%s)`,
 			strings.Join(updateCases, " "),
 			strings.Join(updateIDs, ","),
 		)

server/datastore/mysql/software_test.go (4)

1882-1897: Avoid brittle assertion on name_source; assert prefix instead.

Hard‑coding "bundle_4.67" makes the test fragile if the internal marker changes. Prefer checking it indicates a bundle-ID match.

Apply this diff:

-	// Verify the name_source was updated
-	var nameSource string
-	err = ds.writer(ctx).GetContext(ctx, &nameSource,
-		`SELECT name_source FROM software WHERE id = ?`, originalSoftwareID)
-	require.NoError(t, err)
-	require.Equal(t, "bundle_4.67", nameSource, "Name source should indicate bundle ID match")
+	// Verify the name_source was updated (bundle-ID match)
+	var nameSource string
+	err = ds.writer(ctx).GetContext(ctx, &nameSource,
+		`SELECT name_source FROM software WHERE id = ?`, originalSoftwareID)
+	require.NoError(t, err)
+	require.True(t, strings.HasPrefix(nameSource, "bundle_"), "Name source should indicate bundle ID match, got %q", nameSource)

---

8987-9042: Strengthen assertions: verify names actually changed on the reused rows.

You confirm ID reuse and absence of duplicates; add a quick check that the names were updated to the new values.

Apply this diff after verifying IDs (before the COUNT query):

   // Verify the IDs are the same (software was reused, not recreated)
   for _, s := range host.Software {
     originalID, ok := originalIDs[s.BundleIdentifier]
     require.True(t, ok, "Bundle ID %s should exist", s.BundleIdentifier)
     require.Equal(t, originalID, s.ID,
       "Software ID should be reused for bundle ID %s", s.BundleIdentifier)
   }
 
+  // Verify names were updated on the reused rows
+  expectedNames := map[string]string{
+    "com.example.app":     "Renamed.app",
+    "com.example.another": "AlsoRenamed.app",
+  }
+  for _, s := range host.Software {
+    require.Equal(t, expectedNames[s.BundleIdentifier], s.Name)
+  }

---

9043-9134: Make the name_source check resilient; assert a bundle_ prefix.*

Same brittleness as above; use HasPrefix so internal versioning doesn’t break tests.

Apply this diff:

-			// Verify name_source was updated
-			var nameSource string
-			err = ds.writer(ctx).GetContext(ctx, &nameSource,
-				`SELECT name_source FROM software WHERE id = ?`, s.ID)
-			require.NoError(t, err)
-			require.Equal(t, "bundle_4.67", nameSource, "Name source should indicate bundle ID match")
+			// Verify name_source was updated (bundle-ID match)
+			var nameSource string
+			err = ds.writer(ctx).GetContext(ctx, &nameSource,
+				`SELECT name_source FROM software WHERE id = ?`, s.ID)
+			require.NoError(t, err)
+			require.True(t, strings.HasPrefix(nameSource, "bundle_"), "Name source should indicate bundle ID match, got %q", nameSource)

Also, consider filtering title counts to what this test created (to avoid coupling to fixtures/migrations):

-	err = ds.writer(ctx).GetContext(ctx, &titleCount,
-		`SELECT COUNT(DISTINCT id) FROM software_titles`)
+	err = ds.writer(ctx).GetContext(ctx, &titleCount,
+		`SELECT COUNT(DISTINCT id) FROM software_titles WHERE name IN ('Slack 2.app','Chrome.app','CustomTool')`)

---

9135-9259: Validate browser extension invariants and reduce name-based coupling.

• Assert ExtensionID and Browser are set and bundle_identifier is empty for all entries.
• Optional: prefer keying by extensionID+browser to avoid name ambiguity.

Apply this diff after loading host.Software in Test Case 1:

   // Verify extensions were added correctly
   err = ds.LoadHostSoftware(ctx, host, false)
   require.NoError(t, err)
   require.Len(t, host.Software, 4, "Should have 4 browser extensions")
 
+  // Invariants for browser extensions
+  for _, s := range host.Software {
+    require.Equal(t, "browser_plugins", s.Source)
+    require.Empty(t, s.BundleIdentifier)
+    require.NotEmpty(t, s.ExtensionID)
+    require.NotEmpty(t, s.Browser)
+  }

Please confirm that "browser_plugins" is the intended canonical source string across the ingestion pipeline (earlier tests use "chrome_extensions"). If not, update to the canonical value.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 907ea5d and 0f91a85.

📒 Files selected for processing (3)

• changes/29053-sw-names-on-ingest (1 hunks)
• server/datastore/mysql/software.go (2 hunks)
• server/datastore/mysql/software_test.go (4 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.go

⚙️ CodeRabbit configuration file

When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.

Files:

• server/datastore/mysql/software.go
• server/datastore/mysql/software_test.go

🔇 Additional comments (3)

server/datastore/mysql/software.go (2)

426-428: No action required (comment-only change).

---

434-459: LGTM: Consolidated rename planning before batch update

Collecting rename targets from both newly linked and pre-existing bundle-ID matches is sound and keeps the UPDATE minimal.

server/datastore/mysql/software_test.go (1)

57-59: Nice additions to the test matrix.

Adding rename-only, rename-with-new, and browser extensions scenarios meaningfully expands coverage.

[coderabbitai]

⚠️ Potential issue

Nit: casing and wording

Prefer “macOS” and hyphenate “end‑user”.

Apply this diff:

-* Re-added and optimized fix for macos software ingestion to prevent duplicate software due to end user renaming software on host.
+* Re-added and optimized fix for macOS software ingestion to prevent duplicate software due to end-user renaming software on host.

📝 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

	* Re-added and optimized fix for macos software ingestion to prevent duplicate software due to end user renaming software on host.
	* Re-added and optimized fix for macOS software ingestion to prevent duplicate software due to end-user renaming software on host.

🤖 Prompt for AI Agents

In changes/29053-sw-names-on-ingest around line 1, update the commit/description
text to use the correct casing and hyphenation: change “macos” to “macOS” and
“end user” to “end‑user” (with a hyphen), e.g. “Re-added and optimized fix for
macOS software ingestion to prevent duplicate software due to end‑user renaming
software on host.”