Conversation
This reverts commit a3bafe8.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #40928 +/- ##
==========================================
+ Coverage 66.31% 66.32% +0.01%
==========================================
Files 2469 2469
Lines 197844 197910 +66
Branches 8675 8634 -41
==========================================
+ Hits 131205 131270 +65
+ Misses 54768 54762 -6
- Partials 11871 11878 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughRemoved the Windows-specific 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/service/osquery_utils/queries.go`:
- Around line 2234-2237: The Toolbox exclusion is currently case-sensitive
(`!strings.Contains(s.Name, "Toolbox")`), so change it to a case-insensitive
check by lowercasing s.Name before matching (e.g., replace that condition with
`!strings.Contains(strings.ToLower(s.Name), "toolbox")`) in the same expression
that also checks s.Source, s.Vendor and
jetbrainsBuildFormat.MatchString(s.Version).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97167797-a770-4092-9736-97ec6842815c
📒 Files selected for processing (5)
changes/26405-jetbrainsdocs/Contributing/product-groups/orchestration/understanding-host-vitals.mdserver/service/osquery_test.goserver/service/osquery_utils/queries.goserver/service/osquery_utils/queries_test.go
💤 Files with no reviewable changes (2)
- docs/Contributing/product-groups/orchestration/understanding-host-vitals.md
- server/service/osquery_test.go
| return s.Source == "programs" && | ||
| strings.Contains(strings.ToLower(s.Vendor), "jetbrains") && | ||
| !strings.Contains(s.Name, "Toolbox") && | ||
| jetbrainsBuildFormat.MatchString(s.Version) |
There was a problem hiding this comment.
Make Toolbox exclusion case-insensitive.
Line 2236 is case-sensitive, so a value like jetbrains toolbox can be incorrectly transformed.
🔧 Proposed fix
- !strings.Contains(s.Name, "Toolbox") &&
+ !strings.Contains(strings.ToLower(s.Name), "toolbox") &&📝 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.
| return s.Source == "programs" && | |
| strings.Contains(strings.ToLower(s.Vendor), "jetbrains") && | |
| !strings.Contains(s.Name, "Toolbox") && | |
| jetbrainsBuildFormat.MatchString(s.Version) | |
| return s.Source == "programs" && | |
| strings.Contains(strings.ToLower(s.Vendor), "jetbrains") && | |
| !strings.Contains(strings.ToLower(s.Name), "toolbox") && | |
| jetbrainsBuildFormat.MatchString(s.Version) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/service/osquery_utils/queries.go` around lines 2234 - 2237, The
Toolbox exclusion is currently case-sensitive (`!strings.Contains(s.Name,
"Toolbox")`), so change it to a case-insensitive check by lowercasing s.Name
before matching (e.g., replace that condition with
`!strings.Contains(strings.ToLower(s.Name), "toolbox")`) in the same expression
that also checks s.Source, s.Vendor and
jetbrainsBuildFormat.MatchString(s.Version).
There was a problem hiding this comment.
not concerned about this
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/service/osquery_utils/queries.go (1)
2232-2235:⚠️ Potential issue | 🟡 MinorMake Toolbox exclusion case-insensitive.
At Line 2234, Toolbox exclusion is still case-sensitive, so names like
jetbrains toolboxcan be incorrectly transformed.🔧 Suggested patch
- !strings.Contains(s.Name, "Toolbox") && + !strings.Contains(strings.ToLower(s.Name), "toolbox") &&🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/osquery_utils/queries.go` around lines 2232 - 2235, The Toolbox exclusion is currently case-sensitive; update the predicate that returns for JetBrains programs to perform a case-insensitive check on s.Name by replacing the !strings.Contains(s.Name, "Toolbox") check with a lowercase comparison, e.g. !strings.Contains(strings.ToLower(s.Name), "toolbox"), keeping the other conditions (s.Source, strings.ToLower(s.Vendor), and jetbrainsNameVersion.MatchString(s.Name)) unchanged.
🧹 Nitpick comments (1)
server/service/osquery_utils/queries_test.go (1)
126-126: Update stale test comment naming.Line 126 still says
customAppSanitizers, but the implementation now usescustomSanitizers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/osquery_utils/queries_test.go` at line 126, Update the stale inline test comment that references "customAppSanitizers" to the current name "customSanitizers" in the queries_test.go test (the comment above the TNMS test case); locate the comment near the TNMS test block and replace the old identifier so the comment matches the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/service/osquery_utils/queries.go`:
- Around line 2232-2235: The Toolbox exclusion is currently case-sensitive;
update the predicate that returns for JetBrains programs to perform a
case-insensitive check on s.Name by replacing the !strings.Contains(s.Name,
"Toolbox") check with a lowercase comparison, e.g.
!strings.Contains(strings.ToLower(s.Name), "toolbox"), keeping the other
conditions (s.Source, strings.ToLower(s.Vendor), and
jetbrainsNameVersion.MatchString(s.Name)) unchanged.
---
Nitpick comments:
In `@server/service/osquery_utils/queries_test.go`:
- Line 126: Update the stale inline test comment that references
"customAppSanitizers" to the current name "customSanitizers" in the
queries_test.go test (the comment above the TNMS test case); locate the comment
near the TNMS test block and replace the old identifier so the comment matches
the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a6bf797-731c-4182-a2d9-348a032ea277
📒 Files selected for processing (5)
changes/26405-jetbrainsdocs/Contributing/product-groups/orchestration/understanding-host-vitals.mdserver/service/osquery_test.goserver/service/osquery_utils/queries.goserver/service/osquery_utils/queries_test.go
💤 Files with no reviewable changes (2)
- server/service/osquery_test.go
- docs/Contributing/product-groups/orchestration/understanding-host-vitals.md
| // Software queries expect specific columns to be present. Reference the | ||
| // software_{macos|windows|linux} queries for the expected columns. | ||
| var SoftwareOverrideQueries = map[string]DetailQuery{ | ||
| // windows_jetbrains uses the version contained in the product-info.json file as exe installers |
There was a problem hiding this comment.
reverting the initial approach of adding a new detail query because the query was getting denylisted
mostlikelee
requested review from
a team,
sgress454 and
sharon-fdm
as code owners
|
mysql tests are failing because of https://fleetdm.slack.com/archives/C019WG4GH0A/p1772649082281189 |
| }, | ||
| { | ||
| // JetBrains products on Windows report build numbers (e.g., "253.31033.139") | ||
| // instead of marketing versions. This sanitizer extracts the version from |
| return s.Source == "programs" && | ||
| strings.Contains(strings.ToLower(s.Vendor), "jetbrains") && | ||
| !strings.Contains(s.Name, "Toolbox") && | ||
| jetbrainsNameVersion.MatchString(s.Name) |
| // all of our current on-ingest mutations use this table, | ||
| // so might as well let other stuff go faster and save some redundant checks inside the matchers | ||
| if s != nil && s.Source == "apps" { | ||
| if s == nil { |
| jetbrainsNameVersion.MatchString(s.Name) | ||
| }, | ||
| mutate: func(s *fleet.Software, logger *slog.Logger) { | ||
| if matches := jetbrainsNameVersion.FindStringSubmatch(s.Name); len(matches) == 2 { |
There was a problem hiding this comment.
Is there a clearer way to say "find a match and return it stripped of whitespace", or is this pretty
cannonical? Took me a minute to grok.
There was a problem hiding this comment.
this is pretty canonical when using regex, but agree the regex can be hard to grok. hesitant to parse with the strings package as we have set a pattern of using regex here.
|
Just the change file please, all else LGTM, will approve once (if?!) CI test queue gets sorted |
Co-authored-by: jacobshandling <61553566+jacobshandling@users.noreply.github.com>
sharon-fdm
left a comment
sharon-fdm
left a comment
There was a problem hiding this comment.
I only reviewed docs/Contributing/product-groups/orchestration/understanding-host-vitals.md
Relying on previous approvals for the rest.
3 participants
Related issue: Resolves #26405
This includes a revert of the original PR due to running into denylisting with the additional detail query. And adds a new software mutator that transforms the version string instead. Some refactoring done to
customAppSanitizerso it can be used for any source type.Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Added/updated automated tests
QA'd all new/changed functionality manually
For unreleased bug fixes in a release candidate, one of:
Summary by CodeRabbit
Bug Fixes
Documentation
Tests