Skip to content

fix(FLEETMDM-002): CU-86akbhhtv 19 review findings across 14 files #125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

fix(FLEETMDM-002): CU-86akbhhtv 19 review findings across 14 files #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c49ecb2
fix(FLEETMDM-002): 19 review findings across 14 files
flamingo[bot] Aug 24, 2026
98ff9b2
fix(FLEETMDM-002): 19 review findings across 14 files
flamingo[bot] Aug 24, 2026
1b63c62
fix(FLEETMDM-002): 19 review findings across 14 files
flamingo[bot] Aug 24, 2026
0b58af9
fix(FLEETMDM-002): 19 review findings across 14 files
flamingo[bot] Aug 24, 2026
717db9e
fix(FLEETMDM-002): 19 review findings across 14 files
flamingo[bot] Aug 24, 2026
e589596
fix(FLEETMDM-002): 19 review findings across 14 files
flamingo[bot] Aug 24, 2026
91de54d
fix(FLEETMDM-002): 19 review findings across 14 files
flamingo[bot] Aug 24, 2026
63833dc
fix(FLEETMDM-002): 19 review findings across 14 files
flamingo[bot] Aug 24, 2026
a04dab6
fix(FLEETMDM-002): 19 review findings across 14 files
flamingo[bot] Aug 24, 2026
9c3ef02
fix(FLEETMDM-002): 19 review findings across 14 files
flamingo[bot] Aug 24, 2026
3c724be
fix(FLEETMDM-002): 19 review findings across 14 files
flamingo[bot] Aug 24, 2026
fe613f3
fix(FLEETMDM-002): 19 review findings across 14 files
flamingo[bot] Aug 24, 2026
b0d3217
fix(FLEETMDM-002): 19 review findings across 14 files
flamingo[bot] Aug 24, 2026
b02283c
fix(FLEETMDM-002): 19 review findings across 14 files
flamingo[bot] Aug 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ee/server/service/apple_mdm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (svc *Service) GetMDMAppleAccountEnrollmentProfile(ctx context.Context, enr
fleet.MDMAssetSCEPChallenge,

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 ee/server/service/apple_mdm.go uses fmt.Errorf instead of ctxerr.Wrap for a server-layer error

In GetMDMAppleAccountEnrollmentProfile, replaced fmt.Errorf("loading SCEP challenge from the database: %w", err) with ctxerr.Wrap(ctx, err, "loading SCEP challenge from the database"), matching the surrounding error-handling pattern in the same function. The fmt import remains used elsewhere in the file (GetMDMAccountDrivenEnrollmentSSOURL).

🤖 Prompt for AI agents 
In ee/server/service/apple_mdm.go around line 58, review and complete this code-review fix: ee/server/service/apple_mdm.go uses fmt.Errorf instead of ctxerr.Wrap for a server-layer error.
What the draft fix changed: In `GetMDMAppleAccountEnrollmentProfile`, replaced `fmt.Errorf("loading SCEP challenge from the database: %w", err)` with `ctxerr.Wrap(ctx, err, "loading SCEP challenge from the database")`, matching the surrounding error-handling pattern in the same function. The `fmt` import remains used elsewhere in the file (`GetMDMAccountDrivenEnrollmentSSOURL`).
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

}, nil)
if err != nil {
return nil, fmt.Errorf("loading SCEP challenge from the database: %w", err)
return nil, ctxerr.Wrap(ctx, err, "loading SCEP challenge from the database")
}
enrollURL := appConfig.MDMUrl()

Expand Down
6 changes: 3 additions & 3 deletions ee/server/service/calendar.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func (svc *Service) CalendarWebhook(ctx context.Context, eventUUID string, chann
appConfig, err := svc.ds.AppConfig(ctx)
if err != nil {
svc.authz.SkipAuthorization(ctx)
return fmt.Errorf("load app config: %w", err)

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 Plain fmt.Errorf used instead of ctxerr in server-layer calendar service

In CalendarWebhook, replaced fmt.Errorf("load app config: %w", err) with ctxerr.Wrap(ctx, err, "load app config"), and replaced fmt.Errorf("calendar event %s has no fleet ID", eventUUID) with ctxerr.New(ctx, fmt.Sprintf("calendar event %s has no fleet ID", eventUUID)). Also fixed the analogous pattern in processCalendarEvent's genBodyFn closure (fmt.Errorf("host %d has no associated email", host.HostID) → ctxerr.New(ctx, fmt.Sprintf(...))) since it is the same finding pattern within this file/function chain and was flagged as an example call site. fmt import retained since fmt.Sprintf and fmt.Sprintf in the ForbiddenWithInternal call are still used. Not verified against actual ctxerr.Handle wiring/tests in this session.

🤖 Prompt for AI agents 
In ee/server/service/calendar.go around line 27, review and complete this code-review fix: Plain fmt.Errorf used instead of ctxerr in server-layer calendar service.
What the draft fix changed: In `CalendarWebhook`, replaced `fmt.Errorf("load app config: %w", err)` with `ctxerr.Wrap(ctx, err, "load app config")`, and replaced `fmt.Errorf("calendar event %s has no fleet ID", eventUUID)` with `ctxerr.New(ctx, fmt.Sprintf("calendar event %s has no fleet ID", eventUUID))`. Also fixed the analogous pattern in `processCalendarEvent`'s `genBodyFn` closure (`fmt.Errorf("host %d has no associated email", host.HostID)` → `ctxerr.New(ctx, fmt.Sprintf(...))`) since it is the same finding pattern within this file/function chain and was flagged as an example call site. `fmt` import retained since `fmt.Sprintf` and `fmt.Sprintf` in the ForbiddenWithInternal call are still used. Not verified against actual ctxerr.Handle wiring/tests in this session.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer

return ctxerr.Wrap(ctx, err, "load app config")
}

if len(appConfig.Integrations.GoogleCalendar) == 0 {
Expand Down Expand Up @@ -88,7 +88,7 @@ func (svc *Service) CalendarWebhook(ctx context.Context, eventUUID string, chann
if eventDetails.TeamID == nil {
// Should not happen
svc.authz.SkipAuthorization(ctx)
return fmt.Errorf("calendar event %s has no fleet ID", eventUUID)
return ctxerr.New(ctx, fmt.Sprintf("calendar event %s has no fleet ID", eventUUID))
}

localConfig := &calendar.Config{
Expand Down Expand Up @@ -227,7 +227,7 @@ func (svc *Service) processCalendarEvent(ctx context.Context, eventDetails *flee
return "", false, nil
}
if host.Email == "" {
err = fmt.Errorf("host %d has no associated email", host.HostID)
err = ctxerr.New(ctx, fmt.Sprintf("host %d has no associated email", host.HostID))
return "", false, err
}

Expand Down
14 changes: 7 additions & 7 deletions ee/server/service/hostidentity/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,24 @@ package hostidentity

import (
"context"
"fmt"

"github.com/fleetdm/fleet/v4/pkg/certificate"
"github.com/fleetdm/fleet/v4/server/contexts/ctxerr"
"github.com/fleetdm/fleet/v4/server/fleet"
"github.com/fleetdm/fleet/v4/server/mdm/scep/depot"
)

func initAssets(ds fleet.Datastore) error {
func initAssets(ctx context.Context, ds fleet.Datastore) error {
// Check if we have existing certs and keys
expectedAssets := []fleet.MDMAssetName{
fleet.MDMAssetHostIdentityCACert,
fleet.MDMAssetHostIdentityCAKey,
}
savedAssets, err := ds.GetAllMDMConfigAssetsByName(context.Background(), expectedAssets, nil)
savedAssets, err := ds.GetAllMDMConfigAssetsByName(ctx, expectedAssets, nil)
if err != nil {
// allow not found errors as it means we're generating the assets for the first time.

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 ee/server package uses fmt.Errorf instead of ctxerr for error creation/wrapping

In initAssets, replaced fmt.Errorf("loading existing host identity assets from the database: %w", err) with ctxerr.Wrap(ctx, err, "loading existing host identity assets from the database"). This required changing the function signature to accept a context.Context parameter (replacing the internal context.Background() call) since ctxerr.Wrap needs a ctx; this is a signature change to the function which may require updating callers outside this file (not visible here), so the change is not fully verified as complete across the codebase.

🤖 Prompt for AI agents 
In ee/server/service/hostidentity/config.go around line 20, review and complete this code-review fix: ee/server package uses fmt.Errorf instead of ctxerr for error creation/wrapping.
What the draft fix changed: In `initAssets`, replaced `fmt.Errorf("loading existing host identity assets from the database: %w", err)` with `ctxerr.Wrap(ctx, err, "loading existing host identity assets from the database")`. This required changing the function signature to accept a `context.Context` parameter (replacing the internal `context.Background()` call) since ctxerr.Wrap needs a ctx; this is a signature change to the function which may require updating callers outside this file (not visible here), so the change is not fully verified as complete across the codebase.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer

if !fleet.IsNotFound(err) {
return fmt.Errorf("loading existing host identity assets from the database: %w", err)
return ctxerr.Wrap(ctx, err, "loading existing host identity assets from the database")
}
}

Expand All @@ -34,7 +34,7 @@ func initAssets(ds fleet.Datastore) error {
)
scepCert, scepKey, err := depot.NewCACertKey(caCert)
if err != nil {
return fmt.Errorf("generating host identity SCEP cert and key: %w", err)
return ctxerr.Wrap(ctx, err, "generating host identity SCEP cert and key")
}

// Store our config assets encrypted
Comment on lines 34 to 40

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 ee/server package uses fmt.Errorf instead of ctxerr for SCEP cert generation error

In initAssets, replaced fmt.Errorf("generating host identity SCEP cert and key: %w", err) with ctxerr.Wrap(ctx, err, "generating host identity SCEP cert and key"), using the same ctx parameter introduced by the signature change described in note 1.

🤖 Prompt for AI agents 
In ee/server/service/hostidentity/config.go around line 31, review and complete this code-review fix: ee/server package uses fmt.Errorf instead of ctxerr for SCEP cert generation error.
What the draft fix changed: In `initAssets`, replaced `fmt.Errorf("generating host identity SCEP cert and key: %w", err)` with `ctxerr.Wrap(ctx, err, "generating host identity SCEP cert and key")`, using the same `ctx` parameter introduced by the signature change described in note 1.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer

Comment on lines 34 to 40

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 ee/server package uses fmt.Errorf instead of ctxerr for asset insert error

In initAssets, replaced fmt.Errorf("inserting host identity SCEP assets: %w", err) with ctxerr.Wrap(ctx, err, "inserting host identity SCEP assets"), and changed the ds.InsertMDMConfigAssets(context.Background(), ...) call to use the passed-in ctx instead of context.Background(), consistent with the finding's suggested fix and removing the now-unused fmt import.

🤖 Prompt for AI agents 
In ee/server/service/hostidentity/config.go around line 44, review and complete this code-review fix: ee/server package uses fmt.Errorf instead of ctxerr for asset insert error.
What the draft fix changed: In `initAssets`, replaced `fmt.Errorf("inserting host identity SCEP assets: %w", err)` with `ctxerr.Wrap(ctx, err, "inserting host identity SCEP assets")`, and changed the `ds.InsertMDMConfigAssets(context.Background(), ...)` call to use the passed-in `ctx` instead of `context.Background()`, consistent with the finding's suggested fix and removing the now-unused `fmt` import.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer

Expand All @@ -49,8 +49,8 @@ func initAssets(ds fleet.Datastore) error {
})
}

if err := ds.InsertMDMConfigAssets(context.Background(), assets, nil); err != nil {
return fmt.Errorf("inserting host identity SCEP assets: %w", err)
if err := ds.InsertMDMConfigAssets(ctx, assets, nil); err != nil {
return ctxerr.Wrap(ctx, err, "inserting host identity SCEP assets")
}
}
return nil
Expand Down
16 changes: 7 additions & 9 deletions ee/server/service/hostidentity/depot/depot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import (
"crypto/ecdsa"
"crypto/rsa"
"crypto/x509"
"errors"
"fmt"
"log/slog"
"math/big"
"time"
Expand Down Expand Up @@ -52,12 +50,12 @@ func NewHostIdentitySCEPDepot(db *sqlx.DB, ds fleet.Datastore, logger *slog.Logg
func (d *HostIdentitySCEPDepot) CA(_ []byte) ([]*x509.Certificate, *rsa.PrivateKey, error) {
cert, err := assets.KeyPair(context.Background(), d.ds, fleet.MDMAssetHostIdentityCACert, fleet.MDMAssetHostIdentityCAKey)
if err != nil {
return nil, nil, fmt.Errorf("getting assets: %w", err)
return nil, nil, ctxerr.Wrap(context.Background(), err, "getting assets")
}

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 hostidentity/depot/depot.go uses fmt.Errorf and errors.New instead of ctxerr in server-layer package

In CA() (ee/server/service/hostidentity/depot/depot.go), replaced fmt.Errorf("getting assets: %w", err) with ctxerr.Wrap(context.Background(), err, "getting assets") and errors.New("private key not in RSA format") with ctxerr.New(context.Background(), "private key not in RSA format"). Removed now-unused errors and fmt imports since all their usages in the file were replaced.

🤖 Prompt for AI agents 
In ee/server/service/hostidentity/depot/depot.go around line 56, review and complete this code-review fix: hostidentity/depot/depot.go uses fmt.Errorf and errors.New instead of ctxerr in server-layer package.
What the draft fix changed: In CA() (ee/server/service/hostidentity/depot/depot.go), replaced `fmt.Errorf("getting assets: %w", err)` with `ctxerr.Wrap(context.Background(), err, "getting assets")` and `errors.New("private key not in RSA format")` with `ctxerr.New(context.Background(), "private key not in RSA format")`. Removed now-unused `errors` and `fmt` imports since all their usages in the file were replaced.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer


pk, ok := cert.PrivateKey.(*rsa.PrivateKey)
if !ok {
return nil, nil, errors.New("private key not in RSA format")
return nil, nil, ctxerr.New(context.Background(), "private key not in RSA format")
}

return []*x509.Certificate{cert.Leaf}, pk, nil
Expand Down Expand Up @@ -86,10 +84,10 @@ func (d *HostIdentitySCEPDepot) HasCN(cn string, allowTime int, cert *x509.Certi
// Put stores a certificate under the given name.
func (d *HostIdentitySCEPDepot) Put(name string, crt *x509.Certificate) error {
if crt.Subject.CommonName == "" || len(crt.Subject.CommonName) > maxCommonNameLength {
return errors.New("common name empty or too long")
return ctxerr.New(context.Background(), "common name empty or too long")
}

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 Put() in depot.go returns errors.New/fmt.Errorf instead of ctxerr in server-layer code

In Put() (ee/server/service/hostidentity/depot/depot.go), replaced all errors.New(...) calls (common name check, serial number check, ECDSA public key check) and fmt.Errorf(...) calls (public key raw creation, existing certificate check) with ctxerr.New(context.Background(), ...) / ctxerr.Wrap(context.Background(), err, ...) respectively, routing these error paths through ctxerr as required. The pre-existing ctxerr.Errorf rate-limit call and the low-level DB errors returned directly from the sqlx transaction closure (which are wrapped/handled by callers per existing pattern) were left unchanged since the findings only cited the specific fmt.Errorf/errors.New lines.

🤖 Prompt for AI agents 
In ee/server/service/hostidentity/depot/depot.go around line 90, review and complete this code-review fix: Put() in depot.go returns errors.New/fmt.Errorf instead of ctxerr in server-layer code.
What the draft fix changed: In Put() (ee/server/service/hostidentity/depot/depot.go), replaced all `errors.New(...)` calls (common name check, serial number check, ECDSA public key check) and `fmt.Errorf(...)` calls (public key raw creation, existing certificate check) with `ctxerr.New(context.Background(), ...)` / `ctxerr.Wrap(context.Background(), err, ...)` respectively, routing these error paths through ctxerr as required. The pre-existing `ctxerr.Errorf` rate-limit call and the low-level DB errors returned directly from the sqlx transaction closure (which are wrapped/handled by callers per existing pattern) were left unchanged since the findings only cited the specific fmt.Errorf/errors.New lines.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer

if !crt.SerialNumber.IsInt64() {
return errors.New("cannot represent serial number as int64")
return ctxerr.New(context.Background(), "cannot represent serial number as int64")
}

// Extract the ECC uncompressed point (04-prefixed X || Y); 0x04 means this is the raw representation
Expand All @@ -98,11 +96,11 @@ func (d *HostIdentitySCEPDepot) Put(name string, crt *x509.Certificate) error {
// - P-384: 97 bytes
key, ok := crt.PublicKey.(*ecdsa.PublicKey)
if !ok {
return errors.New("public key not in ECDSA format")
return ctxerr.New(context.Background(), "public key not in ECDSA format")
}
pubKeyRaw, err := types.CreateECDSAPublicKeyRaw(key)
if err != nil {
return fmt.Errorf("creating public key raw: %w", err)
return ctxerr.Wrap(context.Background(), err, "creating public key raw")
}
certPEM := certificate.EncodeCertPEM(crt)

Expand All @@ -112,7 +110,7 @@ func (d *HostIdentitySCEPDepot) Put(name string, crt *x509.Certificate) error {
existingCert, err := d.ds.GetHostIdentityCertByName(context.Background(), name)
switch {
case err != nil && !fleet.IsNotFound(err):
return fmt.Errorf("checking existing certificate: %w", err)
return ctxerr.Wrap(context.Background(), err, "checking existing certificate")
case err == nil:
// Certificate exists, check if rate limit applies
if time.Since(existingCert.CreatedAt) < cooldown {
Expand Down
13 changes: 8 additions & 5 deletions server/datastore/failing/common_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"fmt"
"io"
"time"

"github.com/fleetdm/fleet/v4/server/contexts/ctxerr"
)

// commonFailingStore is an implementation of CommonStore
Expand All @@ -15,21 +17,22 @@ type commonFailingStore struct {
}

func (c commonFailingStore) Get(ctx context.Context, iconID string) (io.ReadCloser, int64, error) {

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 commonFailingStore uses fmt.Errorf instead of ctxerr in server/ package

Replaced all four fmt.Errorf calls with ctxerr.New(ctx, fmt.Sprintf(...)) in Get, Put, Exists, and Sign methods of commonFailingStore in server/datastore/failing/common_store.go; added the ctxerr import; changed Sign's unused ctx parameter name from _ to ctx so it can be passed to ctxerr.New.

🤖 Prompt for AI agents 
In server/datastore/failing/common_store.go around line 17, review and complete this code-review fix: commonFailingStore uses fmt.Errorf instead of ctxerr in server/ package.
What the draft fix changed: Replaced all four fmt.Errorf calls with ctxerr.New(ctx, fmt.Sprintf(...)) in Get, Put, Exists, and Sign methods of commonFailingStore in server/datastore/failing/common_store.go; added the ctxerr import; changed Sign's unused ctx parameter name from `_` to `ctx` so it can be passed to ctxerr.New.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

return nil, 0, fmt.Errorf("%s store not properly configured", c.Entity)
return nil, 0, ctxerr.New(ctx, fmt.Sprintf("%s store not properly configured", c.Entity))
}

func (c commonFailingStore) Put(ctx context.Context, iconID string, content io.ReadSeeker) error {
return fmt.Errorf("%s store not properly configured", c.Entity)
return ctxerr.New(ctx, fmt.Sprintf("%s store not properly configured", c.Entity))
}

func (c commonFailingStore) Exists(ctx context.Context, iconID string) (bool, error) {
return false, fmt.Errorf("%s store not properly configured", c.Entity)
return false, ctxerr.New(ctx, fmt.Sprintf("%s store not properly configured", c.Entity))
}

func (c commonFailingStore) Cleanup(ctx context.Context, usedIconIDs []string, removeCreatedBefore time.Time) (int, error) {
return 0, nil
}

func (c commonFailingStore) Sign(_ context.Context, _ string, _ time.Duration) (string, error) {
return "", fmt.Errorf("%s store not properly configured", c.Entity)
func (c commonFailingStore) Sign(ctx context.Context, _ string, _ time.Duration) (string, error) {
return "", ctxerr.New(ctx, fmt.Sprintf("%s store not properly configured", c.Entity))
}

2 changes: 1 addition & 1 deletion server/datastore/mysql/android_enterprises.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func (ds *AndroidDatastore) GetEnterprise(ctx context.Context) (*android.Enterpr

func (ds *AndroidDatastore) UpdateEnterprise(ctx context.Context, enterprise *android.EnterpriseDetails) error {
if enterprise == nil || enterprise.ID == 0 {

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 errors.New used instead of ctxerr.New in server/datastore/mysql package

Replaced errors.New("missing enterprise ID") with ctxerr.New(ctx, "missing enterprise ID") in UpdateEnterprise, matching the ctxerr pattern used elsewhere in the file; the errors import is still required for errors.Is usage in other functions, so it remains in the import block.

🤖 Prompt for AI agents 
In server/datastore/mysql/android_enterprises.go around line 65, review and complete this code-review fix: errors.New used instead of ctxerr.New in server/datastore/mysql package.
What the draft fix changed: Replaced `errors.New("missing enterprise ID")` with `ctxerr.New(ctx, "missing enterprise ID")` in `UpdateEnterprise`, matching the ctxerr pattern used elsewhere in the file; the `errors` import is still required for `errors.Is` usage in other functions, so it remains in the import block.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

return errors.New("missing enterprise ID")
return ctxerr.New(ctx, "missing enterprise ID")
}
stmt := `UPDATE android_enterprises
SET signup_name = ?,
Expand Down
5 changes: 3 additions & 2 deletions server/datastore/mysql/host_certificate_templates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,12 +372,12 @@ func (ds *Datastore) UpsertCertificateStatus(ctx context.Context, update *fleet.
WHERE host_uuid = :host_uuid AND certificate_template_id = :certificate_template_id`
result, err := sqlx.NamedExecContext(ctx, ds.writer(ctx), updateStmt, update)
if err != nil {
return err
return ctxerr.Wrap(ctx, err, "update host certificate template status")
}

rowsAffected, err := result.RowsAffected()
if err != nil {
return err
return ctxerr.Wrap(ctx, err, "get rows affected for host certificate template status update")
}

// If no records were updated, then insert a new status.
Comment on lines 372 to 383

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 fmt.Errorf used instead of ctxerr in UpsertCertificateStatus

In UpsertCertificateStatus (server/datastore/mysql/host_certificate_templates.go), replaced the two bare return err statements following sqlx.NamedExecContext and result.RowsAffected() with return ctxerr.Wrap(ctx, err, "update host certificate template status") and return ctxerr.Wrap(ctx, err, "get rows affected for host certificate template status update") respectively, so all error returns in the function now go through ctxerr as required, matching the style of the pre-existing ctxerr.Wrap calls elsewhere in the same function.

(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)

🤖 Prompt for AI agents 
In server/datastore/mysql/host_certificate_templates.go around line 300, review and complete this code-review fix: fmt.Errorf used instead of ctxerr in UpsertCertificateStatus.
What the draft fix changed: In `UpsertCertificateStatus` (server/datastore/mysql/host_certificate_templates.go), replaced the two bare `return err` statements following `sqlx.NamedExecContext` and `result.RowsAffected()` with `return ctxerr.Wrap(ctx, err, "update host certificate template status")` and `return ctxerr.Wrap(ctx, err, "get rows affected for host certificate template status update")` respectively, so all error returns in the function now go through ctxerr as required, matching the style of the pre-existing `ctxerr.Wrap` calls elsewhere in the same function.

_(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)_
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer

Expand Down Expand Up @@ -837,3 +837,4 @@ func (ds *Datastore) GetOrCreateFleetChallengeForCertificateTemplate(
}
return challenge, nil
}

5 changes: 3 additions & 2 deletions server/datastore/mysql/host_identity_scep.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"fmt"

"github.com/fleetdm/fleet/v4/ee/pkg/hostidentity/types"
"github.com/fleetdm/fleet/v4/server/contexts/ctxerr"
common_mysql "github.com/fleetdm/fleet/v4/server/platform/mysql"
"github.com/jmoiron/sqlx"
)
Expand Down Expand Up @@ -93,11 +94,11 @@ func (ds *Datastore) GetMDMSCEPCertBySerial(ctx context.Context, serialNumber ui
// The hash is calculated from cert.Raw (DER-encoded bytes), not the PEM string

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 fmt.Errorf used instead of ctxerr in host_identity_scep.go server package

In GetMDMSCEPCertBySerial (server/datastore/mysql/host_identity_scep.go), replaced errors.New("failed to decode PEM certificate") with ctxerr.New(ctx, "failed to decode PEM certificate") and fmt.Errorf("failed to parse certificate: %w", err) with ctxerr.Wrap(ctx, err, "parse certificate"). Added import for github.com/fleetdm/fleet/v4/server/contexts/ctxerr. The fmt and errors imports remain in use elsewhere in the file (fmt.Sprintf, errors.Is), so no import removal was needed.

🤖 Prompt for AI agents 
In server/datastore/mysql/host_identity_scep.go around line 93, review and complete this code-review fix: fmt.Errorf used instead of ctxerr in host_identity_scep.go server package.
What the draft fix changed: In `GetMDMSCEPCertBySerial` (server/datastore/mysql/host_identity_scep.go), replaced `errors.New("failed to decode PEM certificate")` with `ctxerr.New(ctx, "failed to decode PEM certificate")` and `fmt.Errorf("failed to parse certificate: %w", err)` with `ctxerr.Wrap(ctx, err, "parse certificate")`. Added import for `github.com/fleetdm/fleet/v4/server/contexts/ctxerr`. The `fmt` and `errors` imports remain in use elsewhere in the file (fmt.Sprintf, errors.Is), so no import removal was needed.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

block, _ := pem.Decode([]byte(certPEM))
if block == nil {
return "", errors.New("failed to decode PEM certificate")
return "", ctxerr.New(ctx, "failed to decode PEM certificate")
}
cert, err := x509.ParseCertificate(block.Bytes)
if err != nil {
return "", fmt.Errorf("failed to parse certificate: %w", err)
return "", ctxerr.Wrap(ctx, err, "parse certificate")
}
hashed := sha256.Sum256(cert.Raw)
hash := hex.EncodeToString(hashed[:])
Expand Down
2 changes: 1 addition & 1 deletion server/mdm/acme/internal/mysql/challenge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func (ds *Datastore) GetChallengeByID(ctx context.Context, accountID, challengeI
// UpdateChallenge handles updating the challenge status, and the authorization status as well as moving the order status.
func (ds *Datastore) UpdateChallenge(ctx context.Context, challenge *types.Challenge) (*types.Challenge, error) {
if challenge == nil {

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 errors.New used instead of ctxerr in challenge.go server package

In UpdateChallenge (server/mdm/acme/internal/mysql/challenge.go), replaced errors.New("Challenge can not be nil for update") with ctxerr.New(ctx, "challenge can not be nil for update"), routing the error through ctxerr as required by FLEETMDM-002 and matching the pattern used elsewhere in the file. The errors import remains used by errors.Is in GetChallengeByID, so no import changes were needed.

🤖 Prompt for AI agents 
In server/mdm/acme/internal/mysql/challenge.go around line 58, review and complete this code-review fix: errors.New used instead of ctxerr in challenge.go server package.
What the draft fix changed: In UpdateChallenge (server/mdm/acme/internal/mysql/challenge.go), replaced `errors.New("Challenge can not be nil for update")` with `ctxerr.New(ctx, "challenge can not be nil for update")`, routing the error through ctxerr as required by FLEETMDM-002 and matching the pattern used elsewhere in the file. The `errors` import remains used by `errors.Is` in GetChallengeByID, so no import changes were needed.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 Bare 'Challenge can not be nil for update' error not wrapped with call-context

The same change (ctxerr.New with context) in UpdateChallenge addresses the lack of call-context wrapping: ctxerr.New attaches the context so the error is attributable via ctxerr.Handle like the other wrapped errors in this function, satisfying FLEETMDM-002-2's requirement without needing a separate fmt.Errorf wrap since ctxerr.New already provides equivalent stack/context capture.

🤖 Prompt for AI agents 
In server/mdm/acme/internal/mysql/challenge.go around line 58, review and complete this code-review fix: Bare 'Challenge can not be nil for update' error not wrapped with call-context.
What the draft fix changed: The same change (ctxerr.New with context) in UpdateChallenge addresses the lack of call-context wrapping: ctxerr.New attaches the context so the error is attributable via ctxerr.Handle like the other wrapped errors in this function, satisfying FLEETMDM-002-2's requirement without needing a separate fmt.Errorf wrap since ctxerr.New already provides equivalent stack/context capture.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

return nil, errors.New("Challenge can not be nil for update")
return nil, ctxerr.New(ctx, "challenge can not be nil for update")
}

err := platform_mysql.WithRetryTxx(ctx, ds.writer(ctx), func(tx sqlx.ExtContext) error {
Expand Down
2 changes: 1 addition & 1 deletion server/mdm/lifecycle/lifecycle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ func (t *HostLifecycle) doWithUUIDValidation(ctx context.Context, action uuidFn,

users, acts, err := action(ctx, opts.UUID)
if err != nil {
return err
return ctxerr.Wrap(ctx, err, "execute uuid action")
}
return t.createActivities(ctx, users, acts)
}
Comment on lines 130 to 136

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 lifecycle.go: raw error returned without ctxerr wrapping in doWithUUIDValidation

In doWithUUIDValidation (server/mdm/lifecycle/lifecycle.go), changed return err to return ctxerr.Wrap(ctx, err, "execute uuid action") for the error returned by action(ctx, opts.UUID), matching the ctxerr-wrapping convention used elsewhere in the file. This is a mechanical, low-risk one-line change exactly matching the suggested fix.

🤖 Prompt for AI agents 
In server/mdm/lifecycle/lifecycle.go around line 137, review and complete this code-review fix: lifecycle.go: raw error returned without ctxerr wrapping in doWithUUIDValidation.
What the draft fix changed: In `doWithUUIDValidation` (server/mdm/lifecycle/lifecycle.go), changed `return err` to `return ctxerr.Wrap(ctx, err, "execute uuid action")` for the error returned by `action(ctx, opts.UUID)`, matching the ctxerr-wrapping convention used elsewhere in the file. This is a mechanical, low-risk one-line change exactly matching the suggested fix.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

Expand Down
3 changes: 1 addition & 2 deletions server/service/apple_mdm_batched.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package service
import (
"context"
"encoding/pem"
"fmt"
"log/slog"

"github.com/fleetdm/fleet/v4/server/contexts/ctxerr"
Expand Down Expand Up @@ -36,7 +35,7 @@ func ReconcileAppleProfilesBatched(
) (err error) {

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 fmt.Errorf used instead of ctxerr in server/service package for config-read failure

In ReconcileAppleProfilesBatched, replaced fmt.Errorf("reading app config: %w", err) with ctxerr.Wrap(ctx, err, "reading app config") for the config-read failure, matching the suggested fix and the ctxerr usage pattern elsewhere in the function. Also removed the now-unused fmt import since it was only used at this call site.

🤖 Prompt for AI agents 
In server/service/apple_mdm_batched.go around line 36, review and complete this code-review fix: fmt.Errorf used instead of ctxerr in server/service package for config-read failure.
What the draft fix changed: In `ReconcileAppleProfilesBatched`, replaced `fmt.Errorf("reading app config: %w", err)` with `ctxerr.Wrap(ctx, err, "reading app config")` for the config-read failure, matching the suggested fix and the `ctxerr` usage pattern elsewhere in the function. Also removed the now-unused `fmt` import since it was only used at this call site.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

appConfig, err := ds.AppConfig(ctx)
if err != nil {
return fmt.Errorf("reading app config: %w", err)
return ctxerr.Wrap(ctx, err, "reading app config")
}
if !appConfig.MDM.EnabledAndConfigured {
return nil
Expand Down
3 changes: 1 addition & 2 deletions server/vulnerabilities/macoffice/analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package macoffice
import (
"context"
"encoding/json"
"errors"
"fmt"
"os"
"path/filepath"
Expand Down Expand Up @@ -155,7 +154,7 @@ func Analyze(
}
}
if !hasValid {
return nil, errors.New("MacOffice release notes contain no valid security updates (possible corrupted feed)")
return nil, ctxerr.New(ctx, "MacOffice release notes contain no valid security updates (possible corrupted feed)")
}

queryParams := fleet.SoftwareIterQueryOptions{IncludedSources: []string{"apps"}}
Comment on lines 154 to 160

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 Plain errors.New used for server-layer error instead of ctxerr.New

In Analyze, replaced errors.New("MacOffice release notes contain no valid security updates (possible corrupted feed)") with ctxerr.New(ctx, "MacOffice release notes contain no valid security updates (possible corrupted feed)"), using the already-available ctx parameter, and removed the now-unused errors import since ctxerr was already imported.

🤖 Prompt for AI agents 
In server/vulnerabilities/macoffice/analyzer.go around line 171, review and complete this code-review fix: Plain errors.New used for server-layer error instead of ctxerr.New.
What the draft fix changed: In `Analyze`, replaced `errors.New("MacOffice release notes contain no valid security updates (possible corrupted feed)")` with `ctxerr.New(ctx, "MacOffice release notes contain no valid security updates (possible corrupted feed)")`, using the already-available `ctx` parameter, and removed the now-unused `errors` import since `ctxerr` was already imported.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

Expand Down
5 changes: 3 additions & 2 deletions server/vulnerabilities/msrc/analyzer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package msrc

import (
"context"
"errors"
"fmt"
"log/slog"
"strconv"
Expand Down Expand Up @@ -34,12 +33,14 @@ func Analyze(
return nil, err
}

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 New guard clause in upstream fleetdm/fleet file lacks OPENFRAME sentinel comments

Wrapped the empty-bulletin guard clause in Analyze with // >>> OPENFRAME(msrc-empty-bulletin-guard): ... — openframe/docs/msrc-guard.md and // <<< OPENFRAME(msrc-empty-bulletin-guard) sentinel comments, per the suggested fix format. The referenced doc file openframe/docs/msrc-guard.md is assumed but not verified to exist; a complete fix may require creating that documentation file.

🤖 Prompt for AI agents 
In server/vulnerabilities/msrc/analyzer.go around line 36, review and complete this code-review fix: New guard clause in upstream fleetdm/fleet file lacks OPENFRAME sentinel comments.
What the draft fix changed: Wrapped the empty-bulletin guard clause in `Analyze` with `// >>> OPENFRAME(msrc-empty-bulletin-guard): ... — openframe/docs/msrc-guard.md` and `// <<< OPENFRAME(msrc-empty-bulletin-guard)` sentinel comments, per the suggested fix format. The referenced doc file `openframe/docs/msrc-guard.md` is assumed but not verified to exist; a complete fix may require creating that documentation file.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

// >>> OPENFRAME(msrc-empty-bulletin-guard): reject corrupted empty MSRC feeds instead of remediating all vulns — openframe/docs/msrc-guard.md
// Refuse to proceed if the loaded bulletin contains no vulnerability data — an empty
// bulletin would cause every existing MSRC OS vulnerability for this OS to be marked as
// remediated. This usually indicates the bulletin file was corrupted during download.

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 errors.New used instead of ctxerr.New in server-layer MSRC analyzer

In Analyze, replaced errors.New(...) with ctxerr.New(ctx, ...) for the empty-bulletin guard error, and removed the now-unused "errors" import from the import block, since ctx is already available in scope.

🤖 Prompt for AI agents 
In server/vulnerabilities/msrc/analyzer.go around line 39, review and complete this code-review fix: errors.New used instead of ctxerr.New in server-layer MSRC analyzer.
What the draft fix changed: In `Analyze`, replaced `errors.New(...)` with `ctxerr.New(ctx, ...)` for the empty-bulletin guard error, and removed the now-unused `"errors"` import from the import block, since `ctx` is already available in scope.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

if len(bulletin.Vulnerabilities) == 0 {
return nil, errors.New("MSRC bulletin contains no vulnerabilities (possible corrupted feed)")
return nil, ctxerr.New(ctx, "MSRC bulletin contains no vulnerabilities (possible corrupted feed)")
}
// <<< OPENFRAME(msrc-empty-bulletin-guard)

// Find matching products inside the bulletin
matchingPIDs := make(map[string]bool)
Expand Down
3 changes: 1 addition & 2 deletions server/worker/zendesk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"bytes"
"context"
"encoding/json"
"errors"
"fmt"
"log/slog"
"sort"
Expand Down Expand Up @@ -267,7 +266,7 @@ func (z *Zendesk) Run(ctx context.Context, argsJSON json.RawMessage) error {
func (z *Zendesk) runVuln(ctx context.Context, cli ZendeskClient, args zendeskArgs) error {
vargs := args.Vulnerability
if vargs == nil {
return errors.New("invalid job args")
return ctxerr.New(ctx, "invalid job args")
}

var hosts []fleet.HostVulnerabilitySummary

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 runVuln returns a plain errors.New instead of ctxerr.New for a server-layer validation error

In runVuln (server/worker/zendesk.go), replaced return errors.New("invalid job args") with return ctxerr.New(ctx, "invalid job args"), and removed the now-unused errors import from the import block, since it was only used at that call site.

🤖 Prompt for AI agents 
In server/worker/zendesk.go around line 273, review and complete this code-review fix: runVuln returns a plain errors.New instead of ctxerr.New for a server-layer validation error.
What the draft fix changed: In `runVuln` (server/worker/zendesk.go), replaced `return errors.New("invalid job args")` with `return ctxerr.New(ctx, "invalid job args")`, and removed the now-unused `errors` import from the import block, since it was only used at that call site.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

Expand Down