fix(FLEETMDM-002-2): CU-86akj32d7 3 review findings across 3 files - #176
flamingo[bot] wants to merge 3 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -32,7 +32,7 @@ func sync( | |||
| ) error { | |||
| remote, url, err := ghClient.MacOfficeReleaseNotes(ctx) | |||
There was a problem hiding this comment.
🦩 🟠 sync() in macoffice/sync.go returns errors from ghClient/fsClient calls without wrapping context
In sync() (server/vulnerabilities/macoffice/sync.go), wrapped all four bare return err propagation points with contextual fmt.Errorf("...: %w", err) messages: "get remote release notes" for ghClient.MacOfficeReleaseNotes, "get local release notes" for fsClient.MacOfficeReleaseNotes, "download release notes" for both ghClient.Download call sites, and "delete out of date release notes" for fsClient.Delete. This preserves error unwrapping via %w while adding distinguishable context at each call boundary, addressing FLEETMDM-002-2 without altering control flow, style, or the top-level SyncFromGithub wrapping.
🤖 Prompt for AI agents
In server/vulnerabilities/macoffice/sync.go around line 33, review and complete this code-review fix: sync() in macoffice/sync.go returns errors from ghClient/fsClient calls without wrapping context.
What the draft fix changed: In sync() (server/vulnerabilities/macoffice/sync.go), wrapped all four bare `return err` propagation points with contextual `fmt.Errorf("...: %w", err)` messages: "get remote release notes" for ghClient.MacOfficeReleaseNotes, "get local release notes" for fsClient.MacOfficeReleaseNotes, "download release notes" for both ghClient.Download call sites, and "delete out of date release notes" for fsClient.Delete. This preserves error unwrapping via %w while adding distinguishable context at each call boundary, addressing FLEETMDM-002-2 without altering control flow, style, or the top-level SyncFromGithub wrapping.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| if err == nil && len(body) > 0 { | ||
| return nil, fmt.Errorf("get %s received status %d: %s", endpoint, response.StatusCode, body) | ||
| } | ||
| if err != nil { | ||
| return nil, fmt.Errorf("get %s received status %d, read response body: %w", endpoint, response.StatusCode, err) | ||
| } | ||
| return nil, fmt.Errorf("get %s received status %d", endpoint, response.StatusCode) | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 getRawBody swallows the read error on non-200 responses instead of wrapping it
In getRawBody (server/service/client_debug.go), the non-200 branch now checks the io.ReadAll error separately: if err != nil, the error is wrapped with %w into a new error message that also includes the status code, instead of being silently discarded and replaced by a generic status-only message. The success path (empty body, no error) still falls through to the generic status-only message unchanged.
🤖 Prompt for AI agents
In server/service/client_debug.go around line 18, review and complete this code-review fix: getRawBody swallows the read error on non-200 responses instead of wrapping it.
What the draft fix changed: In getRawBody (server/service/client_debug.go), the non-200 branch now checks the io.ReadAll error separately: if err != nil, the error is wrapped with %w into a new error message that also includes the status code, instead of being silently discarded and replaced by a generic status-only message. The success path (empty body, no error) still falls through to the generic status-only message unchanged.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| if machineId != "" { | ||
| request.Header.Add("x-machine-id", machineId) | ||
| } | ||
| } else { | ||
| log.Debug().Msg("machineIdProvider is nil, not adding x-machine-id header") | ||
| } | ||
| } | ||
| // <<< OPENFRAME(agent-openframe-mode) |
There was a problem hiding this comment.
🦩 🟠 machineId header injection swallows a nil machineIdProvider silently without any wrapped error or log context
In requestWithExternal (client/orbit_client.go), added an else branch to the if oc.machineIdProvider != nil check that logs log.Debug().Msg("machineIdProvider is nil, not adding x-machine-id header"), mirroring the existing Debug log used in the authToken-empty branch just above it. This gives log parity between the two header-injection paths without altering control flow, header behavior, or any other logic.
🤖 Prompt for AI agents
In client/orbit_client.go around line 190, review and complete this code-review fix: machineId header injection swallows a nil machineIdProvider silently without any wrapped error or log context.
What the draft fix changed: In `requestWithExternal` (client/orbit_client.go), added an `else` branch to the `if oc.machineIdProvider != nil` check that logs `log.Debug().Msg("machineIdProvider is nil, not adding x-machine-id header")`, mirroring the existing Debug log used in the authToken-empty branch just above it. This gives log parity between the two header-injection paths without altering control flow, header behavior, or any other logic.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 3 review findings across 3 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
server/vulnerabilities/macoffice/sync.go:33server/service/client_debug.go:18client/orbit_client.go:190What 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:
1446a072-096e-4294-8082-c7cadffe76deMerging 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.
ClickUp task: CU-86akj32d7 FleetMDM bulk review findings sweep (12 PRs)