Conversation
…policies response
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43420 +/- ##
==========================================
+ Coverage 66.88% 66.92% +0.04%
==========================================
Files 2590 2594 +4
Lines 207636 207784 +148
Branches 9323 9323
==========================================
+ Hits 138875 139065 +190
+ Misses 56129 56095 -34
+ Partials 12632 12624 -8
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:
|
![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.
|
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)
WalkthroughAdds validation and handling for patch-type policies: patch policies now require a non-empty team and a resolvable FleetMaintainedAppSlug (mapped into PolicySpec.PatchSoftwareTitleID), returning a bad-request error when unresolved. ApplyPolicySpecs and GitOps reconciliation match team-scoped patch policies by patch_software_title_id (not by name) and, when a patch target is unchanged but the spec name differs, update the stored policy name and checksum rather than deleting and recreating. Tests and an integration assertion cover invalid/global slugs and idempotent rename behavior. Possibly related PRs
🚥 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 docstrings
🧪 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: 3
🧹 Nitpick comments (2)
changes/43389-patch-policy-gitops-bugs (1)
2-2: Consider improving clarity and capitalization.Two optional improvements:
- Capitalize "API" for consistency with standard formatting.
- Add context about when the nil pointer dereference occurred (e.g., "when creating patch policies without a fleet_maintained_app_slug").
📝 Suggested enhancement
-- Fixed a nil pointer dereference in the contributor api spec/policies. +- Fixed a nil pointer dereference in the contributor API spec/policies when creating patch policies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@changes/43389-patch-policy-gitops-bugs` at line 2, Update the commit message to improve clarity and capitalization: change "contributor api spec/policies" to "contributor API spec/policies" and append context describing when the nil pointer dereference occurred, for example "Fixed a nil pointer dereference in the contributor API spec/policies when creating patch policies without a fleet_maintained_app_slug", referencing the field name fleet_maintained_app_slug to make the cause explicit.server/datastore/mysql/policies_test.go (1)
7751-7773: Strengthen negative-case assertions to validate the exact failure reason.These two checks currently accept any error. Tightening them to assert expected error text/type will prevent false positives from unrelated failures.
Proposed test hardening
require.Error(t, err) + require.ErrorContains(t, err, "fleet_maintained_app_slug") ... require.Error(t, err) + require.ErrorContains(t, err, "nonexistent-slug")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/policies_test.go` around lines 7751 - 7773, The tests calling ApplyPolicySpecs should assert the specific failure reason instead of any error: for the case with empty FleetMaintainedAppSlug (PolicySpec.Type == fleet.PolicyTypePatch) assert the error message or error type includes/equals a validation error mentioning "fleet_maintained_app_slug" (or the validation sentinel used by ApplyPolicySpecs), and for the case with a non-existent FleetMaintainedAppSlug assert the error indicates the slug was not found (e.g. contains "not found" or the specific repository sentinel error); update the two require.Error(t, err) lines to require the precise error via require.EqualError/require.ErrorContains or errors.Is against the expected sentinel so failures reflect the intended negative case in ApplyPolicySpecs/PolicySpec handling.
🤖 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/datastore/mysql/policies.go`:
- Around line 1481-1485: The validation only checks for fmaTitleID == nil and
misses the sql.ErrNoRows case from the earlier lookup, so convert the lookup
that sets fmaTitleID (the sqlx.GetContext / query that selects si.title_id
joining fleet_maintained_apps using spec.FleetMaintainedAppSlug and
teamNameToID[spec.Team]) to capture the returned error; if errors.Is(err,
sql.ErrNoRows) return a ctxerr.Wrap(ctx, &fleet.BadRequestError{Message:
fmt.Sprintf("fleet_maintained_app_slug %q is invalid for patch policy %q",
spec.FleetMaintainedAppSlug, spec.Name)}) otherwise wrap and return the original
error (e.g., ctxerr.Wrap(ctx, err, "get fma title id")), keeping the existing
nil check for NULL title_id.
- Around line 1611-1615: When renaming a patch policy in the
shouldUpdatePatchPolicyName branch, also update the policies.checksum to the
value derived from team_id + new name so the idx_policies_checksum uniqueness
key stays correct; modify the tx.ExecContext call in that branch (the UPDATE
executed when shouldUpdatePatchPolicyName) to set checksum alongside name by
computing it with the same checksum algorithm used elsewhere (use the existing
checksum routine or SQL expression that combines team_id and spec.Name) and
ensure it uses policyID to target the correct row.
In `@server/fleet/policies.go`:
- Around line 210-215: In verifyPatchPolicy, replace the generic
errPolicyInvalidPlatform return when a PolicyTypePatch has an empty team with a
patch-specific error (e.g., define and return errPatchPolicyRequiresTeam) so
callers see a clear "patch policies require a team" validation error; update or
add the new error variable and use it in verifyPatchPolicy (referencing
verifyPatchPolicy, PolicyTypePatch, errPolicyInvalidPlatform, and the new
errPatchPolicyRequiresTeam symbol).
---
Nitpick comments:
In `@changes/43389-patch-policy-gitops-bugs`:
- Line 2: Update the commit message to improve clarity and capitalization:
change "contributor api spec/policies" to "contributor API spec/policies" and
append context describing when the nil pointer dereference occurred, for example
"Fixed a nil pointer dereference in the contributor API spec/policies when
creating patch policies without a fleet_maintained_app_slug", referencing the
field name fleet_maintained_app_slug to make the cause explicit.
In `@server/datastore/mysql/policies_test.go`:
- Around line 7751-7773: The tests calling ApplyPolicySpecs should assert the
specific failure reason instead of any error: for the case with empty
FleetMaintainedAppSlug (PolicySpec.Type == fleet.PolicyTypePatch) assert the
error message or error type includes/equals a validation error mentioning
"fleet_maintained_app_slug" (or the validation sentinel used by
ApplyPolicySpecs), and for the case with a non-existent FleetMaintainedAppSlug
assert the error indicates the slug was not found (e.g. contains "not found" or
the specific repository sentinel error); update the two require.Error(t, err)
lines to require the precise error via require.EqualError/require.ErrorContains
or errors.Is against the expected sentinel so failures reflect the intended
negative case in ApplyPolicySpecs/PolicySpec handling.
🪄 Autofix (Beta)
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
Run ID: 89f02958-7529-463a-a441-a3bdecc4e640
📒 Files selected for processing (6)
changes/43389-patch-policy-gitops-bugsserver/datastore/mysql/policies.goserver/datastore/mysql/policies_test.goserver/fleet/policies.goserver/service/client.goserver/service/integration_enterprise_test.go
| if fmaTitleID == nil { | ||
| return ctxerr.Wrap(ctx, &fleet.BadRequestError{ | ||
| Message: fmt.Sprintf("fleet_maintained_app_slug must be set for patch policy: %s", spec.Name), | ||
| }) | ||
| } |
There was a problem hiding this comment.
This validation still misses the sql.ErrNoRows path.
fmaTitleID == nil only happens if the earlier slug lookup returned a row with NULL title_id. When the slug is invalid or not available for that team, the lookup at Line 1320 already fails with sql.ErrNoRows, so this block never runs and the request still bubbles up as an internal datastore error instead of the intended 400.
Suggested fix
err := sqlx.GetContext(ctx, queryerContext, &fmaTitleID, `
SELECT si.title_id FROM software_installers si
JOIN fleet_maintained_apps fma ON si.fleet_maintained_app_id = fma.id
WHERE fma.slug = ?
AND si.global_or_team_id = ?
`, spec.FleetMaintainedAppSlug, teamNameToID[spec.Team])
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
return ctxerr.Wrap(ctx, &fleet.BadRequestError{
Message: fmt.Sprintf("fleet_maintained_app_slug %q is invalid for patch policy %q", spec.FleetMaintainedAppSlug, spec.Name),
})
}
return ctxerr.Wrap(ctx, err, "get fma title id")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/datastore/mysql/policies.go` around lines 1481 - 1485, The validation
only checks for fmaTitleID == nil and misses the sql.ErrNoRows case from the
earlier lookup, so convert the lookup that sets fmaTitleID (the sqlx.GetContext
/ query that selects si.title_id joining fleet_maintained_apps using
spec.FleetMaintainedAppSlug and teamNameToID[spec.Team]) to capture the returned
error; if errors.Is(err, sql.ErrNoRows) return a ctxerr.Wrap(ctx,
&fleet.BadRequestError{Message: fmt.Sprintf("fleet_maintained_app_slug %q is
invalid for patch policy %q", spec.FleetMaintainedAppSlug, spec.Name)})
otherwise wrap and return the original error (e.g., ctxerr.Wrap(ctx, err, "get
fma title id")), keeping the existing nil check for NULL title_id.
There was a problem hiding this comment.
Doesn't matter because ds.getPatchPolicyInstaller(ctx, ptr.ValOrZero(teamID), *fmaTitleID) should return a proper error if the installer is not found
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/datastore/mysql/policies.go (1)
1481-1485:⚠️ Potential issue | 🟠 MajorInvalid slugs still escape as a datastore error.
This nil check only handles a row with
NULL title_id. When the slug lookup at Line 1320 returnssql.ErrNoRows,ApplyPolicySpecsexits earlier with a wrapped datastore error, so invalidfleet_maintained_app_slugvalues still won't surface as the intended 400 response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/policies.go` around lines 1481 - 1485, The nil-check for fmaTitleID only catches NULL DB rows but not the case where the slug lookup returns sql.ErrNoRows, causing an unexpected datastore error instead of a 400; inside ApplyPolicySpecs where you resolve fleet_maintained_app_slug (the slug lookup that sets fmaTitleID), detect sql.ErrNoRows and return a ctxerr.Wrap of &fleet.BadRequestError with Message like "fleet_maintained_app_slug must be set for patch policy: <spec.Name>" (same form as the existing nil branch), so invalid slugs produce the intended 400 response instead of a datastore error.
🧹 Nitpick comments (1)
server/datastore/mysql/policies_test.go (1)
7751-7773: Strengthen the negative assertions to validate error contractThese two cases currently only assert “some error.” Consider asserting the specific client-facing bad-request error shape/type so regressions to unrelated failures (or internal errors) don’t silently pass.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/datastore/mysql/policies_test.go` around lines 7751 - 7773, The tests that call ApplyPolicySpecs for the "patch-no-slug" and "patch-bad-slug" PolicySpec cases currently only use require.Error; change those assertions to verify the specific client-facing bad-request/validation error instead: use errors.Is()/require.ErrorIs or errors.As()/require.True with the project's sentinel bad-request/validation error (or the concrete validation error type) to assert the error from ds.ApplyPolicySpecs(ctx, user1.ID, ...) is a bad-request/validation error and additionally assert the error message or validation details mention FleetMaintainedAppSlug (e.g. check err.Error() contains "fleet_maintained_app_slug" or inspect the validation error fields) so the tests for ApplyPolicySpecs (PolicySpec with Type fleet.PolicyTypePatch and FleetMaintainedAppSlug empty or nonexistent) fail on wrong error shapes rather than any error.
🤖 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/datastore/mysql/policies.go`:
- Around line 1557-1564: The rename branch for patch policies only loads lastID
(in the if spec.Type == fleet.PolicyTypePatch path) which leaves
teamIDToPoliciesByName[spec.Name] empty and breaks the later
prev.NeedsFullMembershipCleanup check; after you SELECT id using
fmaTitleID/teamID (and set shouldUpdatePatchPolicyName), also load the full
previous policy row (or at least its NeedsFullMembershipCleanup value and name)
and populate teamIDToPoliciesByName[spec.Name] (or set
prev.NeedsFullMembershipCleanup) so the later logic that inspects prev by name
still sees the original policy for renamed patch policies (use the same
tx/context and the policies table query used elsewhere in policies.go).
---
Duplicate comments:
In `@server/datastore/mysql/policies.go`:
- Around line 1481-1485: The nil-check for fmaTitleID only catches NULL DB rows
but not the case where the slug lookup returns sql.ErrNoRows, causing an
unexpected datastore error instead of a 400; inside ApplyPolicySpecs where you
resolve fleet_maintained_app_slug (the slug lookup that sets fmaTitleID), detect
sql.ErrNoRows and return a ctxerr.Wrap of &fleet.BadRequestError with Message
like "fleet_maintained_app_slug must be set for patch policy: <spec.Name>" (same
form as the existing nil branch), so invalid slugs produce the intended 400
response instead of a datastore error.
---
Nitpick comments:
In `@server/datastore/mysql/policies_test.go`:
- Around line 7751-7773: The tests that call ApplyPolicySpecs for the
"patch-no-slug" and "patch-bad-slug" PolicySpec cases currently only use
require.Error; change those assertions to verify the specific client-facing
bad-request/validation error instead: use errors.Is()/require.ErrorIs or
errors.As()/require.True with the project's sentinel bad-request/validation
error (or the concrete validation error type) to assert the error from
ds.ApplyPolicySpecs(ctx, user1.ID, ...) is a bad-request/validation error and
additionally assert the error message or validation details mention
FleetMaintainedAppSlug (e.g. check err.Error() contains
"fleet_maintained_app_slug" or inspect the validation error fields) so the tests
for ApplyPolicySpecs (PolicySpec with Type fleet.PolicyTypePatch and
FleetMaintainedAppSlug empty or nonexistent) fail on wrong error shapes rather
than any error.
🪄 Autofix (Beta)
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
Run ID: 8aa4bdfa-4415-446e-81c9-a02461271918
📒 Files selected for processing (3)
server/datastore/mysql/policies.goserver/datastore/mysql/policies_test.goserver/fleet/policies.go
| // Patch policies are unique by patch_software_title_id so we need to get them by that, and update their name | ||
| // so that it doesn't get deleted later. | ||
| if spec.Type == fleet.PolicyTypePatch { | ||
| err = sqlx.GetContext(ctx, tx, &lastID, "SELECT id FROM policies WHERE patch_software_title_id = ? AND team_id = ?", fmaTitleID, teamID) | ||
| shouldUpdatePatchPolicyName = true | ||
| } else { | ||
| err = sqlx.GetContext(ctx, tx, &lastID, "SELECT id FROM policies WHERE name = ? AND team_id = ?", spec.Name, teamID) | ||
| } |
There was a problem hiding this comment.
Rename fallback loses the interrupted-cleanup retry signal.
This branch only reloads id. The later prev.NeedsFullMembershipCleanup check still depends on teamIDToPoliciesByName[spec.Name], which is empty for a renamed patch policy, so a GitOps retry after an interrupted cleanup won't immediately rerun cleanup for that policy. The cron fallback will eventually heal it, but this retry path still regresses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/datastore/mysql/policies.go` around lines 1557 - 1564, The rename
branch for patch policies only loads lastID (in the if spec.Type ==
fleet.PolicyTypePatch path) which leaves teamIDToPoliciesByName[spec.Name] empty
and breaks the later prev.NeedsFullMembershipCleanup check; after you SELECT id
using fmaTitleID/teamID (and set shouldUpdatePatchPolicyName), also load the
full previous policy row (or at least its NeedsFullMembershipCleanup value and
name) and populate teamIDToPoliciesByName[spec.Name] (or set
prev.NeedsFullMembershipCleanup) so the later logic that inspects prev by name
still sees the original policy for renamed patch policies (use the same
tx/context and the policies table query used elsewhere in policies.go).
There was a problem hiding this comment.
It seems like this situation would only happen the policy was renamed again after the cleanups job interruption?
IMO it might be fine to leave a comment rather than add confusing code, but fixing it is fine too.
Edit: This seems exceedingly unlikely, considering how we expect patch policies to be used. I also couldn't get needs_full_membership_cleanup to actually be 1 on a patch policy, I attempted to change the software installer automation in gitops but it didn't do anything. That might be a different problem but probably out of scope for this fix.
There was a problem hiding this comment.
However, after looking into all of this I (claude) added an extra check using that map to avoid updating the name unnecessarily (shouldn't matter anyway)
2 participants
Related issue: Resolves #43389
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
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
Summary by CodeRabbit