fix(FLEETMDM-002-2): CU-86akj32d7 62 review findings across 40 files

[flamingo]

Closes 62 review findings across 40 files.

Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.

#	Fix confidence	Finding	Location
1	🟢 95 high	writeAPIKeys error message says 'finding home dir' when the actual failing operation is MkdirAll	orbit/pkg/packaging/macos_rcodesign.go:74
2	🟢 90 high	secure.MkdirAll called with file-permission mode 0o600 instead of a directory mode	orbit/pkg/packaging/macos_rcodesign.go:74
3	🟡 80 medium	rSign swallows underlying error detail by printing to stdout instead of wrapping into returned error	orbit/pkg/packaging/macos_rcodesign.go:32
4	🟡 80 medium	rNotarizeStaple swallows rcodesign notarize output instead of returning it in the error	orbit/pkg/packaging/macos_rcodesign.go:58
5	🔴 55 low — review closely	Private key material written to a predictable path in the shared OS temp directory	orbit/pkg/packaging/macos_rcodesign.go:14
6	🟢 90 high	WSTEPStoreCertificate returns raw DB error without ctxerr wrapping	server/datastore/mysql/wstep.go:31
7	🟢 90 high	WSTEPNewSerial returns raw DB errors without context wrapping	server/datastore/mysql/wstep.go:47
8	🟢 90 high	WSTEPAssociateCertHash returns raw DB error without wrapping	server/datastore/mysql/wstep.go:60
9	🔴 20 low — review closely	WSTEPNewSerial has unresolved TODOs about serial number safety left in production code	server/datastore/mysql/wstep.go:52
10	🟢 90 high	decoding certificate PEM data error is not actually derived from the underlying failure	server/mdm/assets/assets.go:139
11	🔴 55 low — review closely	CACertsAndKeyForDecryption silently falls back to only the current leaf cert on lookup errors, masking historical-cert loss	server/mdm/assets/assets.go:73
12	🟡 60 medium	New CACertsAndKeyForDecryption logic in shared assets.go lacks OPENFRAME sentinel comments	server/mdm/assets/assets.go:23
13	🟢 95 high	Origin-missing error in setHeaders wraps a nil error via %w	server/service/conditional_access_microsoft_proxy/conditional_access_microsoft_proxy.go:303
14	🟡 80 medium	GetMessageStatus interpolates unescaped query values into request URL	server/service/conditional_access_microsoft_proxy/conditional_access_microsoft_proxy.go:74
15	🟡 70 medium	post() builds request with nil body then overwrites Body field instead of passing body to NewRequest	server/service/conditional_access_microsoft_proxy/conditional_access_microsoft_proxy.go:192
16	🟢 90 high	errors.Wrap used instead of ctxerr/fmt.Errorf %w in server datastore package	server/datastore/mysql/migrations/tables/migration.go:29
17	🟡 88 medium	updateAppConfigJSON wraps errors with pkg/errors instead of fmt.Errorf %w	server/datastore/mysql/migrations/tables/migration.go:230
18	🔴 30 low — review closely	Migration progress goroutine can send after outputComplete close causing garbled/late progress writes	server/datastore/mysql/migrations/tables/migration.go:66
19	🟡 80 medium	reconcile_devices.go swallows unenrolled-marking error without wrapping context	server/mdm/android/service/reconcile_devices.go:88
20	🟢 90 high	Redundant/dead switch case in reconcile loop: case !ok: after case ok: on a boolean switch	server/mdm/android/service/reconcile_devices.go:75
21	🟡 75 medium	Best-effort auth secret set silently ignores ds.GetAllMDMConfigAssetsByName error	server/mdm/android/service/reconcile_devices.go:33
22	🟢 95 high	loadtest.go swallows errors via log.Fatalf with no wrapping context beyond format string, and ignores return errors of ApiClient calls inconsistently	tools/mdm/apple/loadtest/loadtest.go:161
23	🟢 95 high	Discarded error message on profile delete/upload failure in loadtest.go	tools/mdm/apple/loadtest/loadtest.go:169
24	🟢 95 high	signingRoundTripper wraps error with %#v verb instead of %w, breaking wrapping and readability	ee/orbit/pkg/httpsigproxy/httpsigproxy.go:226
25	🟢 90 high	NewProxy leaks the listener if handler creation fails after listener is bound	ee/orbit/pkg/httpsigproxy/httpsigproxy.go:111
26	🟢 90 high	errors.New used instead of wrapped error with context in userIdFromEmail	server/service/client_users.go:73
27	🟡 85 medium	Me() ignores decode error and may return nil User with nil error	server/service/client_users.go:92
28	🟢 90 high	Bare error return without %w wrapping in redirectStderr	orbit/pkg/go-paniclog/paniclog_windows.go:58
29	🟢 90 high	Bare error return without wrapping in dupFD's caller path (Windows paniclog)	orbit/pkg/go-paniclog/paniclog_windows.go:64
30	🟢 90 high	jamfClient.getBearerToken and do() propagate raw errors without wrapping context	tools/mdm/migration/jamf/main.go:140
31	🟡 60 medium	jamf migration webserver has no ReadTimeout/WriteTimeout beyond ReadHeaderTimeout, and echo tool similarly minimal	tools/mdm/migration/jamf/main.go:80
32	🟡 85 medium	toIUpdate error returns are not wrapped with call-boundary context	orbit/pkg/windows/windowsupdate/iupdate.go:128
33	🟡 70 medium	toIUpdatesIdentities allocates a slice sized to Count but skips filling entries when identityDisp is nil, leaving nil holes silently	orbit/pkg/windows/windowsupdate/iupdate.go:102
34	🟢 90 high	kafkarest.go checkResponse builds an error with fmt.Errorf but no %w wrapping and no context prefix	server/logging/kafkarest.go:81
35	🔴 55 low — review closely	kafkaRESTProducer.checkTopic ignores response body without draining/closing safely on error paths	server/logging/kafkarest.go:81
36	🟢 95 high	decodeEncryptedKeyPEM ignores nil block from pem.Decode	server/mdm/nanodep/cmd/deptokens/main.go:84
37	🟢 95 high	dbus.SessionBus() error is logged but not returned, code proceeds with nil conn	orbit/cmd/desktop/desktop_linux.go:55
38	🟢 92 high	DisassociateAssets no longer accepts a context, breaking timeout/cancellation propagation	server/mdm/apple/vpp/api.go:236
39	🟢 95 high	Bare error return without wrapping in table extension.go Execute()	orbit/pkg/table/extension.go:131
40	🟢 95 high	bare err return in updateHostIdentityCertHostIDBySerial loses call context	server/datastore/mysql/host_identity_scep.go:43
41	🟢 95 high	TeamID() error returned unwrapped without call-site context	server/datastore/mysql/scheduled_queries.go:367
42	🟢 95 high	Unwrapped error return in NewDB when sqlx.Open fails	server/platform/mysql/common.go:92
43	🟢 90 high	randomEnrollSecret silently swallows crypto/rand failure and returns a fixed fallback secret	tools/dibble/pkg/seed/enroll_secrets.go:42
44	🟢 95 high	log.Println used with flag.Usage() but no error wrapping/context on missing required flag	tools/bump-migration/main.go:31
45	🟢 95 high	Bare error propagation without wrapping in orbit/pkg/dataflatten JSON/plist/JSONL readers	orbit/pkg/dataflatten/json.go:10
46	🟢 95 high	Bare error propagation without wrapping in jsonl.go file opener	orbit/pkg/dataflatten/jsonl.go:11
47	🟢 95 high	Bare error propagation without wrapping in plist.go file reader	orbit/pkg/dataflatten/plist.go:11
48	🟢 92 high	InviteMailer.Message returns underlying errors unwrapped, losing call-site context	server/mail/invite.go:22
49	🟡 70 medium	Bare error propagation without contextual wrapping in customcve validation and match paths	server/vulnerabilities/customcve/matching_rules.go:99
50	🟡 65 medium	os.Remove/os.WriteFile errors in TransformHeat wrapped correctly, but os.Remove precedes overwrite risk without recovery	orbit/pkg/packaging/wix/transform.go:67
51	🟢 95 high	commonFailingStore.Cleanup returns nil error unconditionally rather than reporting misconfiguration	server/datastore/failing/common_store.go:29
52	🟢 95 high	getCalendarEventByID error not wrapped with call context before propagating	server/datastore/mysql/calendar_events.go:97
53	🟢 95 high	NewQuery in queries.go swallows result.LastInsertId() error	server/datastore/mysql/queries.go:336
54	🟢 90 high	encodeTranslatedPayload never returns an error but callers still handle it as fallible	server/service/client_hosts.go:92
55	🟢 95 high	Bare io.Copy error return without context wrapping in UploadIcon	server/service/client_software.go:211
56	🟡 85 medium	UpdateEvent stub returns unwrapped errors.New without call-site context	ee/server/calendar/google_calendar_load.go:128
57	🟡 85 medium	Interrupt() silently discards a nil cancel func without wrapping/logging	orbit/pkg/osquery/osquery.go:184
58	🟢 98 high	buildNFPM bare return without wrapping for architecture validation error	orbit/pkg/packaging/linux_shared.go:59
59	🟢 92 high	UpsertMaintainedApp software_titles/software UPDATE errors are wrapped, but LastInsertId error is silently discarded	server/datastore/mysql/maintained_apps.go:43
60	🟢 97 high	Bare error return without wrapping loses context in FMA name-sync migration	server/datastore/mysql/migrations/tables/20260326210603_UpdateSoftwareTitleNamesToFMANames.go:9
61	🟢 92 high	getOperatingSystemDB and listOperatingSystemsDB return raw sqlx errors without ctxerr wrapping	server/datastore/mysql/operating_systems.go:145
62	🟡 70 medium	logger.WarnContext error re-log without wrapping context in ReconcileAppleDeclarationsBatched cursor read	server/service/apple_mdm_declarations_batched.go:45

What 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-c7cadffe76de

Merging 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)

[flamingo]

🦩 What this fix changed, finding by finding

62 finding(s) fixed in this draft — 62 explained inline on the diff; 5 low-confidence hunk(s) need close review before merging.

[flamingo]

🦩 🟠 writeAPIKeys error message says 'finding home dir' when the actual failing operation is MkdirAll

Changed the error message in writeAPIKeys (macos_rcodesign.go) for the secure.MkdirAll failure from "finding home dir" to "creating private keys dir", correctly describing the failing operation.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 74, review and complete this code-review fix: writeAPIKeys error message says 'finding home dir' when the actual failing operation is MkdirAll.
What the draft fix changed: Changed the error message in writeAPIKeys (macos_rcodesign.go) for the secure.MkdirAll failure from "finding home dir" to "creating private keys dir", correctly describing the failing operation.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 secure.MkdirAll called with file-permission mode 0o600 instead of a directory mode

Changed the mode argument passed to secure.MkdirAll in writeAPIKeys from 0o600 to 0o700, giving the owner execute/traversal permission needed for a usable directory.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 74, review and complete this code-review fix: secure.MkdirAll called with file-permission mode 0o600 instead of a directory mode.
What the draft fix changed: Changed the mode argument passed to secure.MkdirAll in writeAPIKeys from 0o600 to 0o700, giving the owner execute/traversal permission needed for a usable directory.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 rSign swallows underlying error detail by printing to stdout instead of wrapping into returned error

In rSign, removed the fmt.Println(outBuf.String()) call and instead embedded outBuf.String() into the returned wrapped error ("rcodesign: %w: %s"), so the rcodesign output is available to callers/error handling.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 32, review and complete this code-review fix: rSign swallows underlying error detail by printing to stdout instead of wrapping into returned error.
What the draft fix changed: In rSign, removed the fmt.Println(outBuf.String()) call and instead embedded outBuf.String() into the returned wrapped error ("rcodesign: %w: %s"), so the rcodesign output is available to callers/error handling.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 rNotarizeStaple swallows rcodesign notarize output instead of returning it in the error

In rNotarizeStaple, removed the fmt.Println(outBuf.String()) call and instead embedded outBuf.String() into the returned wrapped error ("rcodesign notarize: %w: %s"), preserving diagnostic output in the error itself.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 58, review and complete this code-review fix: rNotarizeStaple swallows rcodesign notarize output instead of returning it in the error.
What the draft fix changed: In rNotarizeStaple, removed the fmt.Println(outBuf.String()) call and instead embedded outBuf.String() into the returned wrapped error ("rcodesign notarize: %w: %s"), preserving diagnostic output in the error itself.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 Private key material written to a predictable path in the shared OS temp directory

In rSign, replaced the fixed-name os.TempDir()/cert.pem file write with os.CreateTemp("", "cert-*.pem") to get a uniquely named temp file, explicitly chmod'd to 0o600 before writing the cert contents, reducing the predictable-path/TOCTOU race window. This changes control flow slightly (explicit Chmod + WriteString + Close with error handling); behavior should be equivalent but the additional error paths introduced are unverified against existing tests, and true hardening would also want O_EXCL semantics guaranteed by CreateTemp (which it already provides) plus possibly restricting the temp dir itself.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 14, review and complete this code-review fix: Private key material written to a predictable path in the shared OS temp directory.
What the draft fix changed: In rSign, replaced the fixed-name os.TempDir()/cert.pem file write with os.CreateTemp("", "cert-*.pem") to get a uniquely named temp file, explicitly chmod'd to 0o600 before writing the cert contents, reducing the predictable-path/TOCTOU race window. This changes control flow slightly (explicit Chmod + WriteString + Close with error handling); behavior should be equivalent but the additional error paths introduced are unverified against existing tests, and true hardening would also want O_EXCL semantics guaranteed by CreateTemp (which it already provides) plus possibly restricting the temp dir itself.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

[flamingo]

🦩 🟠 buildNFPM bare return without wrapping for architecture validation error

In buildNFPM (orbit/pkg/packaging/linux_shared.go), changed the error string from "Invalid architecture: %s" to "invalid architecture: %s" to conform to Go error string conventions (lowercase, no punctuation) consistent with other errors in the file. Purely a string literal change, no behavior change.

🤖 Prompt for AI agents

In orbit/pkg/packaging/linux_shared.go around line 59, review and complete this code-review fix: buildNFPM bare return without wrapping for architecture validation error.
What the draft fix changed: In buildNFPM (orbit/pkg/packaging/linux_shared.go), changed the error string from "Invalid architecture: %s" to "invalid architecture: %s" to conform to Go error string conventions (lowercase, no punctuation) consistent with other errors in the file. Purely a string literal change, no behavior change.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 UpsertMaintainedApp software_titles/software UPDATE errors are wrapped, but LastInsertId error is silently discarded

In UpsertMaintainedApp (server/datastore/mysql/maintained_apps.go), changed id, _ := res.LastInsertId() to id, err := res.LastInsertId() followed by an explicit if err != nil { return ctxerr.Wrap(ctx, err, "get last insert id for maintained app") }, so a failure to retrieve the last insert id now aborts the transaction and is propagated to the caller instead of silently leaving appID as 0. res.RowsAffected() error is left discarded, since it was not part of this finding.

🤖 Prompt for AI agents

In server/datastore/mysql/maintained_apps.go around line 43, review and complete this code-review fix: UpsertMaintainedApp software_titles/software UPDATE errors are wrapped, but LastInsertId error is silently discarded.
What the draft fix changed: In `UpsertMaintainedApp` (server/datastore/mysql/maintained_apps.go), changed `id, _ := res.LastInsertId()` to `id, err := res.LastInsertId()` followed by an explicit `if err != nil { return ctxerr.Wrap(ctx, err, "get last insert id for maintained app") }`, so a failure to retrieve the last insert id now aborts the transaction and is propagated to the caller instead of silently leaving `appID` as 0. `res.RowsAffected()` error is left discarded, since it was not part of this finding.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 Bare error return without wrapping loses context in FMA name-sync migration

In Up_20260326210603, the second tx.Exec error (software table update) is now wrapped with fmt.Errorf("updating software names to FMA names: %w", err) instead of being returned bare. Also wrapped the first tx.Exec error (software_titles update) with fmt.Errorf("updating software_titles names to FMA names: %w", err) for consistency, added the fmt import, and changed the final return to return nil after explicit error check.

🤖 Prompt for AI agents

In server/datastore/mysql/migrations/tables/20260326210603_UpdateSoftwareTitleNamesToFMANames.go around line 9, review and complete this code-review fix: Bare error return without wrapping loses context in FMA name-sync migration.
What the draft fix changed: In `Up_20260326210603`, the second `tx.Exec` error (software table update) is now wrapped with `fmt.Errorf("updating software names to FMA names: %w", err)` instead of being returned bare. Also wrapped the first `tx.Exec` error (software_titles update) with `fmt.Errorf("updating software_titles names to FMA names: %w", err)` for consistency, added the `fmt` import, and changed the final return to `return nil` after explicit error check.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 getOperatingSystemDB and listOperatingSystemsDB return raw sqlx errors without ctxerr wrapping

Wrapped the raw sqlx errors in getOperatingSystemDB and listOperatingSystemsDB (server/datastore/mysql/operating_systems.go) with ctxerr.Wrap(ctx, err, "get operating system") and ctxerr.Wrap(ctx, err, "list operating systems") respectively. Since ctxerr.Wrap preserves the underlying error chain for errors.Is/errors.Unwrap, the existing errors.Is(err, sql.ErrNoRows) check in getOrGenerateOperatingSystemDB and newOperatingSystemDB continues to work correctly against the wrapped error returned from getOperatingSystemDB.

🤖 Prompt for AI agents

In server/datastore/mysql/operating_systems.go around line 145, review and complete this code-review fix: getOperatingSystemDB and listOperatingSystemsDB return raw sqlx errors without ctxerr wrapping.
What the draft fix changed: Wrapped the raw sqlx errors in `getOperatingSystemDB` and `listOperatingSystemsDB` (server/datastore/mysql/operating_systems.go) with `ctxerr.Wrap(ctx, err, "get operating system")` and `ctxerr.Wrap(ctx, err, "list operating systems")` respectively. Since `ctxerr.Wrap` preserves the underlying error chain for `errors.Is`/`errors.Unwrap`, the existing `errors.Is(err, sql.ErrNoRows)` check in `getOrGenerateOperatingSystemDB` and `newOperatingSystemDB` continues to work correctly against the wrapped error returned from `getOperatingSystemDB`.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 logger.WarnContext error re-log without wrapping context in ReconcileAppleDeclarationsBatched cursor read

In ReconcileAppleDeclarationsBatched, the cursor-read error from ds.GetMDMAppleDeclarationReconcileCursor is now wrapped with fmt.Errorf("reading apple MDM declaration reconcile cursor: %w", err) before being passed to logger.WarnContext, so the context boundary wrapping required by FLEETMDM-002-2 is preserved in the log output. The function still intentionally continues with an empty cursor (existing recovery behavior unchanged), so the wrapped error is logged rather than returned; this matches the finding's specific complaint (missing %w wrapping at the log call) without altering the fallback control flow, which was outside the finding's scope.

🤖 Prompt for AI agents

In server/service/apple_mdm_declarations_batched.go around line 45, review and complete this code-review fix: logger.WarnContext error re-log without wrapping context in ReconcileAppleDeclarationsBatched cursor read.
What the draft fix changed: In ReconcileAppleDeclarationsBatched, the cursor-read error from ds.GetMDMAppleDeclarationReconcileCursor is now wrapped with fmt.Errorf("reading apple MDM declaration reconcile cursor: %w", err) before being passed to logger.WarnContext, so the context boundary wrapping required by FLEETMDM-002-2 is preserved in the log output. The function still intentionally continues with an empty cursor (existing recovery behavior unchanged), so the wrapped error is logged rather than returned; this matches the finding's specific complaint (missing %w wrapping at the log call) without altering the fallback control flow, which was outside the finding's scope.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 What this fix changed, finding by finding

62 finding(s) fixed in this draft — 62 explained inline on the diff; 5 low-confidence hunk(s) need close review before merging.

[flamingo]

🦩 🟠 writeAPIKeys error message says 'finding home dir' when the actual failing operation is MkdirAll

Changed the error message in writeAPIKeys (macos_rcodesign.go) for the secure.MkdirAll failure from "finding home dir" to "creating private keys dir", correctly describing the failing operation.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 74, review and complete this code-review fix: writeAPIKeys error message says 'finding home dir' when the actual failing operation is MkdirAll.
What the draft fix changed: Changed the error message in writeAPIKeys (macos_rcodesign.go) for the secure.MkdirAll failure from "finding home dir" to "creating private keys dir", correctly describing the failing operation.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 secure.MkdirAll called with file-permission mode 0o600 instead of a directory mode

Changed the mode argument passed to secure.MkdirAll in writeAPIKeys from 0o600 to 0o700, giving the owner execute/traversal permission needed for a usable directory.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 74, review and complete this code-review fix: secure.MkdirAll called with file-permission mode 0o600 instead of a directory mode.
What the draft fix changed: Changed the mode argument passed to secure.MkdirAll in writeAPIKeys from 0o600 to 0o700, giving the owner execute/traversal permission needed for a usable directory.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 rSign swallows underlying error detail by printing to stdout instead of wrapping into returned error

In rSign, removed the fmt.Println(outBuf.String()) call and instead embedded outBuf.String() into the returned wrapped error ("rcodesign: %w: %s"), so the rcodesign output is available to callers/error handling.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 32, review and complete this code-review fix: rSign swallows underlying error detail by printing to stdout instead of wrapping into returned error.
What the draft fix changed: In rSign, removed the fmt.Println(outBuf.String()) call and instead embedded outBuf.String() into the returned wrapped error ("rcodesign: %w: %s"), so the rcodesign output is available to callers/error handling.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 rNotarizeStaple swallows rcodesign notarize output instead of returning it in the error

In rNotarizeStaple, removed the fmt.Println(outBuf.String()) call and instead embedded outBuf.String() into the returned wrapped error ("rcodesign notarize: %w: %s"), preserving diagnostic output in the error itself.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 58, review and complete this code-review fix: rNotarizeStaple swallows rcodesign notarize output instead of returning it in the error.
What the draft fix changed: In rNotarizeStaple, removed the fmt.Println(outBuf.String()) call and instead embedded outBuf.String() into the returned wrapped error ("rcodesign notarize: %w: %s"), preserving diagnostic output in the error itself.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 Private key material written to a predictable path in the shared OS temp directory

In rSign, replaced the fixed-name os.TempDir()/cert.pem file write with os.CreateTemp("", "cert-*.pem") to get a uniquely named temp file, explicitly chmod'd to 0o600 before writing the cert contents, reducing the predictable-path/TOCTOU race window. This changes control flow slightly (explicit Chmod + WriteString + Close with error handling); behavior should be equivalent but the additional error paths introduced are unverified against existing tests, and true hardening would also want O_EXCL semantics guaranteed by CreateTemp (which it already provides) plus possibly restricting the temp dir itself.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 14, review and complete this code-review fix: Private key material written to a predictable path in the shared OS temp directory.
What the draft fix changed: In rSign, replaced the fixed-name os.TempDir()/cert.pem file write with os.CreateTemp("", "cert-*.pem") to get a uniquely named temp file, explicitly chmod'd to 0o600 before writing the cert contents, reducing the predictable-path/TOCTOU race window. This changes control flow slightly (explicit Chmod + WriteString + Close with error handling); behavior should be equivalent but the additional error paths introduced are unverified against existing tests, and true hardening would also want O_EXCL semantics guaranteed by CreateTemp (which it already provides) plus possibly restricting the temp dir itself.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

[flamingo]

🦩 🟠 buildNFPM bare return without wrapping for architecture validation error

In buildNFPM (orbit/pkg/packaging/linux_shared.go), changed the error string from "Invalid architecture: %s" to "invalid architecture: %s" to conform to Go error string conventions (lowercase, no punctuation) consistent with other errors in the file. Purely a string literal change, no behavior change.

🤖 Prompt for AI agents

In orbit/pkg/packaging/linux_shared.go around line 59, review and complete this code-review fix: buildNFPM bare return without wrapping for architecture validation error.
What the draft fix changed: In buildNFPM (orbit/pkg/packaging/linux_shared.go), changed the error string from "Invalid architecture: %s" to "invalid architecture: %s" to conform to Go error string conventions (lowercase, no punctuation) consistent with other errors in the file. Purely a string literal change, no behavior change.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 UpsertMaintainedApp software_titles/software UPDATE errors are wrapped, but LastInsertId error is silently discarded

In UpsertMaintainedApp (server/datastore/mysql/maintained_apps.go), changed id, _ := res.LastInsertId() to id, err := res.LastInsertId() followed by an explicit if err != nil { return ctxerr.Wrap(ctx, err, "get last insert id for maintained app") }, so a failure to retrieve the last insert id now aborts the transaction and is propagated to the caller instead of silently leaving appID as 0. res.RowsAffected() error is left discarded, since it was not part of this finding.

🤖 Prompt for AI agents

In server/datastore/mysql/maintained_apps.go around line 43, review and complete this code-review fix: UpsertMaintainedApp software_titles/software UPDATE errors are wrapped, but LastInsertId error is silently discarded.
What the draft fix changed: In `UpsertMaintainedApp` (server/datastore/mysql/maintained_apps.go), changed `id, _ := res.LastInsertId()` to `id, err := res.LastInsertId()` followed by an explicit `if err != nil { return ctxerr.Wrap(ctx, err, "get last insert id for maintained app") }`, so a failure to retrieve the last insert id now aborts the transaction and is propagated to the caller instead of silently leaving `appID` as 0. `res.RowsAffected()` error is left discarded, since it was not part of this finding.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 Bare error return without wrapping loses context in FMA name-sync migration

In Up_20260326210603, the second tx.Exec error (software table update) is now wrapped with fmt.Errorf("updating software names to FMA names: %w", err) instead of being returned bare. Also wrapped the first tx.Exec error (software_titles update) with fmt.Errorf("updating software_titles names to FMA names: %w", err) for consistency, added the fmt import, and changed the final return to return nil after explicit error check.

🤖 Prompt for AI agents

In server/datastore/mysql/migrations/tables/20260326210603_UpdateSoftwareTitleNamesToFMANames.go around line 9, review and complete this code-review fix: Bare error return without wrapping loses context in FMA name-sync migration.
What the draft fix changed: In `Up_20260326210603`, the second `tx.Exec` error (software table update) is now wrapped with `fmt.Errorf("updating software names to FMA names: %w", err)` instead of being returned bare. Also wrapped the first `tx.Exec` error (software_titles update) with `fmt.Errorf("updating software_titles names to FMA names: %w", err)` for consistency, added the `fmt` import, and changed the final return to `return nil` after explicit error check.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 getOperatingSystemDB and listOperatingSystemsDB return raw sqlx errors without ctxerr wrapping

Wrapped the raw sqlx errors in getOperatingSystemDB and listOperatingSystemsDB (server/datastore/mysql/operating_systems.go) with ctxerr.Wrap(ctx, err, "get operating system") and ctxerr.Wrap(ctx, err, "list operating systems") respectively. Since ctxerr.Wrap preserves the underlying error chain for errors.Is/errors.Unwrap, the existing errors.Is(err, sql.ErrNoRows) check in getOrGenerateOperatingSystemDB and newOperatingSystemDB continues to work correctly against the wrapped error returned from getOperatingSystemDB.

🤖 Prompt for AI agents

In server/datastore/mysql/operating_systems.go around line 145, review and complete this code-review fix: getOperatingSystemDB and listOperatingSystemsDB return raw sqlx errors without ctxerr wrapping.
What the draft fix changed: Wrapped the raw sqlx errors in `getOperatingSystemDB` and `listOperatingSystemsDB` (server/datastore/mysql/operating_systems.go) with `ctxerr.Wrap(ctx, err, "get operating system")` and `ctxerr.Wrap(ctx, err, "list operating systems")` respectively. Since `ctxerr.Wrap` preserves the underlying error chain for `errors.Is`/`errors.Unwrap`, the existing `errors.Is(err, sql.ErrNoRows)` check in `getOrGenerateOperatingSystemDB` and `newOperatingSystemDB` continues to work correctly against the wrapped error returned from `getOperatingSystemDB`.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 logger.WarnContext error re-log without wrapping context in ReconcileAppleDeclarationsBatched cursor read

In ReconcileAppleDeclarationsBatched, the cursor-read error from ds.GetMDMAppleDeclarationReconcileCursor is now wrapped with fmt.Errorf("reading apple MDM declaration reconcile cursor: %w", err) before being passed to logger.WarnContext, so the context boundary wrapping required by FLEETMDM-002-2 is preserved in the log output. The function still intentionally continues with an empty cursor (existing recovery behavior unchanged), so the wrapped error is logged rather than returned; this matches the finding's specific complaint (missing %w wrapping at the log call) without altering the fallback control flow, which was outside the finding's scope.

🤖 Prompt for AI agents

In server/service/apple_mdm_declarations_batched.go around line 45, review and complete this code-review fix: logger.WarnContext error re-log without wrapping context in ReconcileAppleDeclarationsBatched cursor read.
What the draft fix changed: In ReconcileAppleDeclarationsBatched, the cursor-read error from ds.GetMDMAppleDeclarationReconcileCursor is now wrapped with fmt.Errorf("reading apple MDM declaration reconcile cursor: %w", err) before being passed to logger.WarnContext, so the context boundary wrapping required by FLEETMDM-002-2 is preserved in the log output. The function still intentionally continues with an empty cursor (existing recovery behavior unchanged), so the wrapped error is logged rather than returned; this matches the finding's specific complaint (missing %w wrapping at the log call) without altering the fallback control flow, which was outside the finding's scope.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 What this fix changed, finding by finding

62 finding(s) fixed in this draft — 62 explained inline on the diff; 5 low-confidence hunk(s) need close review before merging.

[flamingo]

🦩 🟠 writeAPIKeys error message says 'finding home dir' when the actual failing operation is MkdirAll

Changed the error message in writeAPIKeys (macos_rcodesign.go) for the secure.MkdirAll failure from "finding home dir" to "creating private keys dir", correctly describing the failing operation.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 74, review and complete this code-review fix: writeAPIKeys error message says 'finding home dir' when the actual failing operation is MkdirAll.
What the draft fix changed: Changed the error message in writeAPIKeys (macos_rcodesign.go) for the secure.MkdirAll failure from "finding home dir" to "creating private keys dir", correctly describing the failing operation.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 secure.MkdirAll called with file-permission mode 0o600 instead of a directory mode

Changed the mode argument passed to secure.MkdirAll in writeAPIKeys from 0o600 to 0o700, giving the owner execute/traversal permission needed for a usable directory.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 74, review and complete this code-review fix: secure.MkdirAll called with file-permission mode 0o600 instead of a directory mode.
What the draft fix changed: Changed the mode argument passed to secure.MkdirAll in writeAPIKeys from 0o600 to 0o700, giving the owner execute/traversal permission needed for a usable directory.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 rSign swallows underlying error detail by printing to stdout instead of wrapping into returned error

In rSign, removed the fmt.Println(outBuf.String()) call and instead embedded outBuf.String() into the returned wrapped error ("rcodesign: %w: %s"), so the rcodesign output is available to callers/error handling.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 32, review and complete this code-review fix: rSign swallows underlying error detail by printing to stdout instead of wrapping into returned error.
What the draft fix changed: In rSign, removed the fmt.Println(outBuf.String()) call and instead embedded outBuf.String() into the returned wrapped error ("rcodesign: %w: %s"), so the rcodesign output is available to callers/error handling.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 rNotarizeStaple swallows rcodesign notarize output instead of returning it in the error

In rNotarizeStaple, removed the fmt.Println(outBuf.String()) call and instead embedded outBuf.String() into the returned wrapped error ("rcodesign notarize: %w: %s"), preserving diagnostic output in the error itself.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 58, review and complete this code-review fix: rNotarizeStaple swallows rcodesign notarize output instead of returning it in the error.
What the draft fix changed: In rNotarizeStaple, removed the fmt.Println(outBuf.String()) call and instead embedded outBuf.String() into the returned wrapped error ("rcodesign notarize: %w: %s"), preserving diagnostic output in the error itself.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 Private key material written to a predictable path in the shared OS temp directory

In rSign, replaced the fixed-name os.TempDir()/cert.pem file write with os.CreateTemp("", "cert-*.pem") to get a uniquely named temp file, explicitly chmod'd to 0o600 before writing the cert contents, reducing the predictable-path/TOCTOU race window. This changes control flow slightly (explicit Chmod + WriteString + Close with error handling); behavior should be equivalent but the additional error paths introduced are unverified against existing tests, and true hardening would also want O_EXCL semantics guaranteed by CreateTemp (which it already provides) plus possibly restricting the temp dir itself.

🤖 Prompt for AI agents

In orbit/pkg/packaging/macos_rcodesign.go around line 14, review and complete this code-review fix: Private key material written to a predictable path in the shared OS temp directory.
What the draft fix changed: In rSign, replaced the fixed-name os.TempDir()/cert.pem file write with os.CreateTemp("", "cert-*.pem") to get a uniquely named temp file, explicitly chmod'd to 0o600 before writing the cert contents, reducing the predictable-path/TOCTOU race window. This changes control flow slightly (explicit Chmod + WriteString + Close with error handling); behavior should be equivalent but the additional error paths introduced are unverified against existing tests, and true hardening would also want O_EXCL semantics guaranteed by CreateTemp (which it already provides) plus possibly restricting the temp dir itself.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

[flamingo]

🦩 🟠 buildNFPM bare return without wrapping for architecture validation error

In buildNFPM (orbit/pkg/packaging/linux_shared.go), changed the error string from "Invalid architecture: %s" to "invalid architecture: %s" to conform to Go error string conventions (lowercase, no punctuation) consistent with other errors in the file. Purely a string literal change, no behavior change.

🤖 Prompt for AI agents

In orbit/pkg/packaging/linux_shared.go around line 59, review and complete this code-review fix: buildNFPM bare return without wrapping for architecture validation error.
What the draft fix changed: In buildNFPM (orbit/pkg/packaging/linux_shared.go), changed the error string from "Invalid architecture: %s" to "invalid architecture: %s" to conform to Go error string conventions (lowercase, no punctuation) consistent with other errors in the file. Purely a string literal change, no behavior change.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 UpsertMaintainedApp software_titles/software UPDATE errors are wrapped, but LastInsertId error is silently discarded

In UpsertMaintainedApp (server/datastore/mysql/maintained_apps.go), changed id, _ := res.LastInsertId() to id, err := res.LastInsertId() followed by an explicit if err != nil { return ctxerr.Wrap(ctx, err, "get last insert id for maintained app") }, so a failure to retrieve the last insert id now aborts the transaction and is propagated to the caller instead of silently leaving appID as 0. res.RowsAffected() error is left discarded, since it was not part of this finding.

🤖 Prompt for AI agents

In server/datastore/mysql/maintained_apps.go around line 43, review and complete this code-review fix: UpsertMaintainedApp software_titles/software UPDATE errors are wrapped, but LastInsertId error is silently discarded.
What the draft fix changed: In `UpsertMaintainedApp` (server/datastore/mysql/maintained_apps.go), changed `id, _ := res.LastInsertId()` to `id, err := res.LastInsertId()` followed by an explicit `if err != nil { return ctxerr.Wrap(ctx, err, "get last insert id for maintained app") }`, so a failure to retrieve the last insert id now aborts the transaction and is propagated to the caller instead of silently leaving `appID` as 0. `res.RowsAffected()` error is left discarded, since it was not part of this finding.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 Bare error return without wrapping loses context in FMA name-sync migration

In Up_20260326210603, the second tx.Exec error (software table update) is now wrapped with fmt.Errorf("updating software names to FMA names: %w", err) instead of being returned bare. Also wrapped the first tx.Exec error (software_titles update) with fmt.Errorf("updating software_titles names to FMA names: %w", err) for consistency, added the fmt import, and changed the final return to return nil after explicit error check.

🤖 Prompt for AI agents

In server/datastore/mysql/migrations/tables/20260326210603_UpdateSoftwareTitleNamesToFMANames.go around line 9, review and complete this code-review fix: Bare error return without wrapping loses context in FMA name-sync migration.
What the draft fix changed: In `Up_20260326210603`, the second `tx.Exec` error (software table update) is now wrapped with `fmt.Errorf("updating software names to FMA names: %w", err)` instead of being returned bare. Also wrapped the first `tx.Exec` error (software_titles update) with `fmt.Errorf("updating software_titles names to FMA names: %w", err)` for consistency, added the `fmt` import, and changed the final return to `return nil` after explicit error check.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 getOperatingSystemDB and listOperatingSystemsDB return raw sqlx errors without ctxerr wrapping

Wrapped the raw sqlx errors in getOperatingSystemDB and listOperatingSystemsDB (server/datastore/mysql/operating_systems.go) with ctxerr.Wrap(ctx, err, "get operating system") and ctxerr.Wrap(ctx, err, "list operating systems") respectively. Since ctxerr.Wrap preserves the underlying error chain for errors.Is/errors.Unwrap, the existing errors.Is(err, sql.ErrNoRows) check in getOrGenerateOperatingSystemDB and newOperatingSystemDB continues to work correctly against the wrapped error returned from getOperatingSystemDB.

🤖 Prompt for AI agents

In server/datastore/mysql/operating_systems.go around line 145, review and complete this code-review fix: getOperatingSystemDB and listOperatingSystemsDB return raw sqlx errors without ctxerr wrapping.
What the draft fix changed: Wrapped the raw sqlx errors in `getOperatingSystemDB` and `listOperatingSystemsDB` (server/datastore/mysql/operating_systems.go) with `ctxerr.Wrap(ctx, err, "get operating system")` and `ctxerr.Wrap(ctx, err, "list operating systems")` respectively. Since `ctxerr.Wrap` preserves the underlying error chain for `errors.Is`/`errors.Unwrap`, the existing `errors.Is(err, sql.ErrNoRows)` check in `getOrGenerateOperatingSystemDB` and `newOperatingSystemDB` continues to work correctly against the wrapped error returned from `getOperatingSystemDB`.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 logger.WarnContext error re-log without wrapping context in ReconcileAppleDeclarationsBatched cursor read

In ReconcileAppleDeclarationsBatched, the cursor-read error from ds.GetMDMAppleDeclarationReconcileCursor is now wrapped with fmt.Errorf("reading apple MDM declaration reconcile cursor: %w", err) before being passed to logger.WarnContext, so the context boundary wrapping required by FLEETMDM-002-2 is preserved in the log output. The function still intentionally continues with an empty cursor (existing recovery behavior unchanged), so the wrapped error is logged rather than returned; this matches the finding's specific complaint (missing %w wrapping at the log call) without altering the fallback control flow, which was outside the finding's scope.

🤖 Prompt for AI agents

In server/service/apple_mdm_declarations_batched.go around line 45, review and complete this code-review fix: logger.WarnContext error re-log without wrapping context in ReconcileAppleDeclarationsBatched cursor read.
What the draft fix changed: In ReconcileAppleDeclarationsBatched, the cursor-read error from ds.GetMDMAppleDeclarationReconcileCursor is now wrapped with fmt.Errorf("reading apple MDM declaration reconcile cursor: %w", err) before being passed to logger.WarnContext, so the context boundary wrapping required by FLEETMDM-002-2 is preserved in the log output. The function still intentionally continues with an empty cursor (existing recovery behavior unchanged), so the wrapped error is logged rather than returned; this matches the finding's specific complaint (missing %w wrapping at the log call) without altering the fallback control flow, which was outside the finding's scope.
Verify the change is correct and complete; do not refactor unrelated code.

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