List categories device endpoint - #46938
Conversation
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/39018-self-service-categories #46938 +/- ##
=====================================================================
Coverage ? 66.83%
=====================================================================
Files ? 2807
Lines ? 224031
Branches ? 11328
=====================================================================
Hits ? 149736
Misses ? 60697
Partials ? 13598
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:
|
![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.
|
@claude review once |
| func (svc *Service) ListSelfServiceSoftwareCategoriesForHost(ctx context.Context, host *fleet.Host) ([]fleet.SoftwareCategory, error) { | ||
| teamID := ptr.ValOrZero(host.TeamID) | ||
|
|
||
| categories, err := svc.ds.ListSoftwareCategories(ctx, teamID) | ||
| if err != nil { | ||
| return nil, ctxerr.Wrap(ctx, err, "list self-service software categories for host") | ||
| } | ||
| return categories, nil | ||
| } |
There was a problem hiding this comment.
🟡 Nit: this PR's new code imports server/ptr and calls ptr.ValOrZero(host.TeamID) in ListSelfServiceSoftwareCategoriesForHost, but the repo convention in .claude/CLAUDE.md says not to use the legacy server/ptr package in new code. The test file added in the same PR already follows the new convention (new(uint(7))), so the EE service is internally inconsistent. Consider inlining: var teamID uint; if host.TeamID != nil { teamID = *host.TeamID } and dropping the ptr import.
Extended reasoning...
What the issue is
ee/server/service/categories.go (added in this PR) imports github.com/fleetdm/fleet/v4/server/ptr and calls ptr.ValOrZero(host.TeamID) on line ~41 inside the brand-new ListSelfServiceSoftwareCategoriesForHost function. The repository's contributor convention at .claude/CLAUDE.md line 42 states: "Use Go 1.26 new(expression) for pointer values ... Do NOT use the legacy server/ptr package in new code — it exists throughout the codebase but is superseded by new(expr)." So this new import + new call sites is, by the letter of the rule, a convention violation.
Why this is only a nit, not a functional bug
The function works correctly: ptr.ValOrZero returns 0 when host.TeamID is nil, which is exactly what the downstream svc.ds.ListSoftwareCategories(ctx, teamID) expects for a no-team host (verified by the new test case "no-team host queries team 0"). There is no functional defect, no nil-deref risk, no incorrect behavior — the test cases all pass and the code is clearly correct.
Addressing the refutation
One verifier raised a fair objection: ptr.ValOrZero is a deref-or-zero helper, not a pointer-creation helper like ptr.String/ptr.Uint, and new(expr) doesn't replace it directly. They also noted ptr.ValOrZero has ~35-64 active uses across the codebase, suggesting the project isn't actually enforcing the rule for this helper. Both points are valid — the CLAUDE.md examples are pointer-creation helpers, and a strict reading of "superseded by new(expr)" only literally covers pointer creation.
That said, the CLAUDE.md rule says "Do NOT use the legacy server/ptr package in new code" as a blanket statement about the package, not just specific functions in it. And the same PR's test file (ee/server/service/categories_test.go lines 22/37/52) uses new(uint(7)) — so the PR author is aware of the convention and applying it elsewhere; the production file is inconsistent with the PR's own test file. That inconsistency is the strongest argument for the nit.
Step-by-step proof of the convention violation
.claude/CLAUDE.md:42declares: "Do NOT use the legacy server/ptr package in new code."- The PR diff adds a brand-new import line:
+\t"github.com/fleetdm/fleet/v4/server/ptr"inee/server/service/categories.go:13. - The PR diff adds a brand-new function
ListSelfServiceSoftwareCategoriesForHost(lines 40-48) whose very first statement isteamID := ptr.ValOrZero(host.TeamID). - The same PR adds
ee/server/service/categories_test.gocontainingTeamID: new(uint(7))on lines 22, 37, and 52 — applying the new-style convention. - Result: identical PR, two files, two different conventions for pointer-to-uint handling.
How to fix
Trivial — drop the import and inline the dereference:
var teamID uint
if host.TeamID != nil {
teamID = *host.TeamID
}This is a 3-line replacement for 1 line plus removing one import. Pure style cleanup; no behavior change. Severity: nit, not blocking.
There was a problem hiding this comment.
I think we should adjust the repo convention, because ptr.ValOrZero is very useful for team_id related stuff and new(type(value)) is the opposite afaik.
a471b6f
into
feat/39018-self-service-categories
2 participants
Related issue: Resolves #46829
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