fix(FLEETMDM-002-2): 37 review findings across 32 files - #129
fix(FLEETMDM-002-2): 37 review findings across 32 files#129flamingo[bot] wants to merge 32 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| } | ||
|
|
||
| if len(data)+estimateAttributeSize(attributes) > pubsub.MaxPublishRequestBytes { | ||
| logPreview := log | ||
| if len(logPreview) > 100 { | ||
| logPreview = logPreview[:100] | ||
| } | ||
| w.logger.InfoContext(ctx, "dropping log over 10MB PubSub limit", | ||
| "size", len(data), | ||
| "log", string(log[:100])+"...", | ||
| "log", string(logPreview)+"...", | ||
| ) | ||
| continue | ||
| } |
There was a problem hiding this comment.
🦩 🟠 pubSubLogWriter.Write silently drops oversized messages without wrapping/propagating a warning error
In pubSubLogWriter.Write, added if result == nil { continue } in the second for _, result := range results loop, skipping oversized messages that were left as nil in the results slice, preventing the nil pointer dereference panic on result.Get(ctx).
🤖 Prompt for AI agents
In server/logging/pubsub.go around line 71, review and complete this code-review fix: pubSubLogWriter.Write silently drops oversized messages without wrapping/propagating a warning error.
What the draft fix changed: In `pubSubLogWriter.Write`, added `if result == nil { continue }` in the second `for _, result := range results` loop, skipping oversized messages that were left as nil in the `results` slice, preventing the nil pointer dereference panic on `result.Get(ctx)`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| } | ||
|
|
||
| if len(data)+estimateAttributeSize(attributes) > pubsub.MaxPublishRequestBytes { | ||
| logPreview := log | ||
| if len(logPreview) > 100 { | ||
| logPreview = logPreview[:100] | ||
| } | ||
| w.logger.InfoContext(ctx, "dropping log over 10MB PubSub limit", | ||
| "size", len(data), | ||
| "log", string(log[:100])+"...", | ||
| "log", string(logPreview)+"...", | ||
| ) | ||
| continue | ||
| } |
There was a problem hiding this comment.
🦩 🔴 pubSubLogWriter oversized-log slicing can panic on short log payloads
In pubSubLogWriter.Write's oversized-message branch, replaced the unguarded string(log[:100]) with a length-checked logPreview variable (if len(logPreview) > 100 { logPreview = logPreview[:100] }) before slicing, preventing an index-out-of-range panic when log is shorter than 100 bytes.
🤖 Prompt for AI agents
In server/logging/pubsub.go around line 72, review and complete this code-review fix: pubSubLogWriter oversized-log slicing can panic on short log payloads.
What the draft fix changed: In `pubSubLogWriter.Write`'s oversized-message branch, replaced the unguarded `string(log[:100])` with a length-checked `logPreview` variable (`if len(logPreview) > 100 { logPreview = logPreview[:100] }`) before slicing, preventing an index-out-of-range panic when `log` is shorter than 100 bytes.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -88,8 +88,7 @@ func run(cfg runCfg) error { | |||
|
|
|||
There was a problem hiding this comment.
🦩 🟠 os.Exit called immediately after printing error instead of returning wrapped error from loadOrMakeCSR failure
In run(), replaced the fmt.Println(err); os.Exit(1) block following loadOrMakeCSR with return fmt.Errorf("load or make CSR: %w", err), matching the suggested fix and the error-propagation contract used elsewhere in the function.
🤖 Prompt for AI agents
In server/mdm/scep/cmd/scepclient/scepclient.go around line 88, review and complete this code-review fix: os.Exit called immediately after printing error instead of returning wrapped error from loadOrMakeCSR failure.
What the draft fix changed: In run(), replaced the `fmt.Println(err); os.Exit(1)` block following `loadOrMakeCSR` with `return fmt.Errorf("load or make CSR: %w", err)`, matching the suggested fix and the error-propagation contract used elsewhere in the function.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| } | ||
| _, err := url.Parse(serverURL) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid server-url flag parameter %s", err) | ||
| return fmt.Errorf("invalid server-url flag parameter: %w", err) | ||
| } | ||
| if caFingerprint != "" && useKeyEnciphermentSelector { | ||
| return errors.New("ca-fingerprint and key-encipherment-selector can't be used at the same time") |
There was a problem hiding this comment.
🦩 🟠 Bare fmt.Errorf without %w wrapping loses causal chain in scepclient.go
In validateFlags(), changed fmt.Errorf("invalid server-url flag parameter %s", err) to fmt.Errorf("invalid server-url flag parameter: %w", err) so the underlying url.Parse error is wrapped with %w, preserving the error chain for errors.Is/As. Other fmt.Errorf calls in the file (e.g. FAILURE status, invalid hash length) do not wrap an underlying error and were left unchanged per the finding's own scope.
🤖 Prompt for AI agents
In server/mdm/scep/cmd/scepclient/scepclient.go around line 214, review and complete this code-review fix: Bare fmt.Errorf without %w wrapping loses causal chain in scepclient.go.
What the draft fix changed: In validateFlags(), changed `fmt.Errorf("invalid server-url flag parameter %s", err)` to `fmt.Errorf("invalid server-url flag parameter: %w", err)` so the underlying url.Parse error is wrapped with %w, preserving the error chain for errors.Is/As. Other fmt.Errorf calls in the file (e.g. FAILURE status, invalid hash length) do not wrap an underlying error and were left unchanged per the finding's own scope.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return fmt.Errorf("get last mod start date: %w", fmt.Errorf("unexpected status code %d", resp.StatusCode)) | ||
| return fmt.Errorf("unexpected status code %d fetching %s", resp.StatusCode, fileName) | ||
| } | ||
|
|
||
| lastModStartDate, err := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
🦩 🟠 downloadLatestGitHubAsset double-wraps the same error context redundantly
In downloadLatestGitHubAsset, the status-code error branch no longer wraps with the redundant "get last mod start date: %w" prefix (which duplicated the same context already applied to the client.Get error). It now returns fmt.Errorf("unexpected status code %d fetching %s", resp.StatusCode, fileName) directly, exactly matching the suggested fix, eliminating the doubled message.
🤖 Prompt for AI agents
In cmd/cve/generate.go around line 145, review and complete this code-review fix: downloadLatestGitHubAsset double-wraps the same error context redundantly.
What the draft fix changed: In `downloadLatestGitHubAsset`, the status-code error branch no longer wraps with the redundant `"get last mod start date: %w"` prefix (which duplicated the same context already applied to the `client.Get` error). It now returns `fmt.Errorf("unexpected status code %d fetching %s", resp.StatusCode, fileName)` directly, exactly matching the suggested fix, eliminating the doubled message.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| if err != nil && err != redigo.ErrNil { | ||
| return nil, err | ||
| return nil, fmt.Errorf("list failing policy sets: %w", err) | ||
| } |
There was a problem hiding this comment.
🦩 🟠 Bare error returns without context wrapping in redis_policy_set.go
In ListSets, wrapped the bare err returned from the SMEMBERS call with fmt.Errorf("list failing policy sets: %w", err), matching the suggested fix. In RemoveHosts, wrapped the bare err returned from the SREM call with fmt.Errorf("remove hosts from policy set: %w", err), consistent with the wrapping style used elsewhere in the file (e.g. addHostToPolicySet, removePolicySet). No other behavior changed.
🤖 Prompt for AI agents
In server/service/redis_policy_set/redis_policy_set.go around line 55, review and complete this code-review fix: Bare error returns without context wrapping in redis_policy_set.go.
What the draft fix changed: In `ListSets`, wrapped the bare `err` returned from the `SMEMBERS` call with `fmt.Errorf("list failing policy sets: %w", err)`, matching the suggested fix. In `RemoveHosts`, wrapped the bare `err` returned from the `SREM` call with `fmt.Errorf("remove hosts from policy set: %w", err)`, consistent with the wrapping style used elsewhere in the file (e.g. `addHostToPolicySet`, `removePolicySet`). No other behavior changed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| case errors.Is(err, fs.ErrNotExist): | ||
| files, err := os.ReadDir(dir) | ||
| if err != nil { | ||
| return "", err | ||
| return "", fmt.Errorf("read dir %q: %w", dir, err) | ||
| } | ||
|
|
||
| prefix := strings.Split(fileName, "-")[0] |
There was a problem hiding this comment.
🦩 🟠 LatestFile wraps os.ReadDir error without context
In LatestFile, changed return "", err after the os.ReadDir(dir) call to return "", fmt.Errorf("read dir %q: %w", dir, err), wrapping the error with the failing directory path as context while preserving the original error via %w.
🤖 Prompt for AI agents
In server/vulnerabilities/utils/utils.go around line 148, review and complete this code-review fix: LatestFile wraps os.ReadDir error without context.
What the draft fix changed: In LatestFile, changed `return "", err` after the `os.ReadDir(dir)` call to `return "", fmt.Errorf("read dir %q: %w", dir, err)`, wrapping the error with the failing directory path as context while preserving the original error via `%w`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| if tmID != nil { | ||
| tm, err := m.Datastore.TeamWithExtras(ctx, *tmID) // TODO see if we can convert this workflow to TeamLite | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, ctxerr.Wrap(ctx, err, "get team with extras") | ||
| } | ||
| team = tm | ||
| } |
There was a problem hiding this comment.
🦩 🟠 getTeamNoTeam returns bare TeamWithExtras error without wrapping context
In getTeamNoTeam (server/worker/macos_setup_assistant.go), changed return nil, err to return nil, ctxerr.Wrap(ctx, err, "get team with extras") for the error from m.Datastore.TeamWithExtras, matching the wrapping style used consistently elsewhere in the file. This preserves fleet.IsNotFound detection at call sites since ctxerr.Wrap maintains error unwrapping compatibility, so callers like runProfileChanged/runProfileDeleted/runHostsTransferred that check fleet.IsNotFound(err) continue to work correctly.
🤖 Prompt for AI agents
In server/worker/macos_setup_assistant.go around line 331, review and complete this code-review fix: getTeamNoTeam returns bare TeamWithExtras error without wrapping context.
What the draft fix changed: In `getTeamNoTeam` (server/worker/macos_setup_assistant.go), changed `return nil, err` to `return nil, ctxerr.Wrap(ctx, err, "get team with extras")` for the error from `m.Datastore.TeamWithExtras`, matching the wrapping style used consistently elsewhere in the file. This preserves `fleet.IsNotFound` detection at call sites since `ctxerr.Wrap` maintains error unwrapping compatibility, so callers like `runProfileChanged`/`runProfileDeleted`/`runHostsTransferred` that check `fleet.IsNotFound(err)` continue to work correctly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| defer os.Remove(notarizationZip) | ||
|
|
||
| if err := packaging.Notarize(notarizationZip, "com.fleetdm.desktop"); err != nil { | ||
| return err | ||
| return fmt.Errorf("notarize app: %w", err) | ||
| } | ||
|
|
||
| if err := packaging.Staple(appDir); err != nil { | ||
| return err | ||
| return fmt.Errorf("staple app: %w", err) | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🦩 🟠 Unwrapped error returns from packaging.Notarize/Staple break the wrapping convention used throughout this file
In createMacOSApp, the packaging.Notarize error is now wrapped as fmt.Errorf("notarize app: %w", err) and the packaging.Staple error is now wrapped as fmt.Errorf("staple app: %w", err), matching the file's existing error-wrapping convention. No other lines were changed.
🤖 Prompt for AI agents
In tools/desktop/desktop.go around line 246, review and complete this code-review fix: Unwrapped error returns from packaging.Notarize/Staple break the wrapping convention used throughout this file.
What the draft fix changed: In `createMacOSApp`, the `packaging.Notarize` error is now wrapped as `fmt.Errorf("notarize app: %w", err)` and the `packaging.Staple` error is now wrapped as `fmt.Errorf("staple app: %w", err)`, matching the file's existing error-wrapping convention. No other lines were changed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
|
|
||
| output, err := ghapi.RunCommandAndReturnOutput(command) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("gh command failed: %v", err) | ||
| return nil, fmt.Errorf("gh command failed: %w", err) | ||
| } | ||
|
|
||
| var bugs []BugIssue | ||
| if err := json.Unmarshal(output, &bugs); err != nil { | ||
| return nil, fmt.Errorf("failed to parse JSON response: %v", err) | ||
| return nil, fmt.Errorf("failed to parse JSON response: %w", err) | ||
| } | ||
|
|
||
| if len(bugs) == limit { |
There was a problem hiding this comment.
🦩 🟠 fetchOpenBugs discards underlying error context with %v instead of %w
In fetchOpenBugs (tools/github-manage/cmd/gm/bugs.go), changed fmt.Errorf("gh command failed: %v", err) to use %w and fmt.Errorf("failed to parse JSON response: %v", err) to use %w, preserving the underlying error chain for errors.Is/As per the finding's suggested fix. No other lines were touched.
🤖 Prompt for AI agents
In tools/github-manage/cmd/gm/bugs.go around line 216, review and complete this code-review fix: fetchOpenBugs discards underlying error context with %v instead of %w.
What the draft fix changed: In fetchOpenBugs (tools/github-manage/cmd/gm/bugs.go), changed `fmt.Errorf("gh command failed: %v", err)` to use `%w` and `fmt.Errorf("failed to parse JSON response: %v", err)` to use `%w`, preserving the underlying error chain for errors.Is/As per the finding's suggested fix. No other lines were touched.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
Closes 37 review findings across 32 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
server/logging/pubsub.go:71server/logging/pubsub.go:72server/mdm/scep/cmd/scepclient/scepclient.go:88server/mdm/scep/cmd/scepclient/scepclient.go:214cmd/cve/generate.go:145cmd/cve/generate.go:254server/mdm/apple/profile_verifier.go:60server/mdm/apple/profile_verifier.go:84orbit/pkg/platform/platform_windows.go:253orbit/pkg/platform/platform_windows.go:172server/service/client_setup.go:38server/mail/ses.go:105tools/android/android.go:199server/fleet/agent_options.go:63orbit/pkg/update/flag_runner.go:140orbit/cmd/desktop/desktop_linux.go:53server/datastore/mysql/migrations/tables/20240327115530_AddDDMTables.go:127server/datastore/mysql/migrations/tables/20250331042354_AddSCIMTables.go:68server/datastore/mysql/migrations/tables/20251124090450_AddHostPlatformFleetVar.go:24server/service/openframe/openframe_token_refresher.go:31tools/dibble/pkg/command/policies.go:38server/datastore/mysql/migrations/tables/20250304162702_AddCATables.go:21server/datastore/mysql/migrations/tables/20260409153714_AddApiEndpointPermissionsTables.go:12server/datastore/mysql/migrations/tables/20260522195225_AddManagedLocalAccountRotationColumns.go:22orbit/pkg/token/readwriter.go:96orbit/pkg/update/runner.go:315server/datastore/mysql/migrations/tables/20240815000001_AddSelfServiceToVPPAppsTeams.go:14server/datastore/mysql/migrations/tables/20260217141240_ResetInvalidPlatformOnLabels.go:11server/mdm/apple/cert.go:140server/mdm/internal/commonmdm/commonmdm.go:12server/mdm/nanodep/client/transport.go:227server/platform/http/post_json.go:32server/service/redis_policy_set/redis_policy_set.go:55server/vulnerabilities/utils/utils.go:148server/worker/macos_setup_assistant.go:331tools/desktop/desktop.go:246tools/github-manage/cmd/gm/bugs.go:216What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
f6d23861-693f-45a1-b5b3-570aa3bc9ea7Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.