fix(adhoc-sweep-fixes): 81 review findings across 40 files

[flamingo]

Closes 81 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	🔴 40 low — review closely	fmt.Errorf format string misuses %q with an already-formatted error message, producing malformed output	server/mdm/scep/depot/file/depot.go:205
2	🔴 40 low — review closely	File handle from os.OpenFile not closed before writing in HasCN's revocation path	server/mdm/scep/depot/file/depot.go:202
3	🟡 85 medium	Unsupported public key type panics instead of returning an error	server/mdm/scep/depot/file/depot.go:280
4	🔴 40 low — review closely	index.txt not truncated before rewrite, risking stale trailing data after revocation	server/mdm/scep/depot/file/depot.go:202
5	🟢 95 high	Test file uses non-existent new(...) builtin as a value constructor for string/enum types	ee/server/service/teams_test.go:396
6	🟢 95 high	Invalid new("abc") call passed as team name payload	ee/server/service/teams_test.go:399
7	🟢 92 high	Invalid new(fleet.RoleAdmin) / new("abc") / new("DEF") calls repeated in ModifyTeam collation test	ee/server/service/teams_test.go:452
8	🟡 75 medium	Hardcoded BitLocker recovery password committed to source in tools/mdm/windows/bitlocker/core.go	tools/mdm/windows/bitlocker/core.go:82
9	🟢 95 high	BitLocker CLI wraps errors with fmt.Errorf but discards Go %w chain (uses %v)	tools/mdm/windows/bitlocker/core.go:12
10	🟢 95 high	All fmt.Errorf calls in tools/mdm/windows/bitlocker/core.go use %v not %w	tools/mdm/windows/bitlocker/core.go:42
11	🟢 95 high	Test calls new(...) as a function on strings, which does not compile in Go	server/mdm/profiles/android_appconfig_test.go:109
12	🟢 95 high	Duplicate invalid new(...) call in second IDP username test	server/mdm/profiles/android_appconfig_test.go:124
13	🟡 80 medium	tools/screencap/main.go has orphaned/duplicated code causing a compile error	tools/screencap/main.go:168
14	🟢 90 high	ctx from chromedp.NewContext is declared but never used, cancel is unused too	tools/screencap/main.go:180
15	🟢 95 high	Invalid Go syntax: new(uint(7)) used as expression in test	ee/server/service/categories_test.go:20
16	🟢 95 high	Invalid Go syntax: new(uint(7)) reused in second subtest	ee/server/service/categories_test.go:34
17	🔴 55 low — review closely	SQL injection via unsanitized device fields interpolated into raw SQL dump	tools/mdm/migration/micromdm/touchless/main.go:213
18	🟢 92 high	x509.ParseCertificate error is logged but execution continues with a nil cert dereferenced	tools/mdm/migration/micromdm/touchless/main.go:193
19	🔴 55 low — review closely	Misleading dead-code comment masks that TLS verification is never actually disabled for DisableSSL testing path	server/datastore/s3/s3.go:103
20	🟡 65 medium	New GCS/OpenFrame-specific S3 logic lacks OPENFRAME sentinel comments	server/datastore/s3/s3.go:62
21	🟢 95 high	CertificateFromPEM will panic on nil block from malformed PEM input	server/mdm/nanodep/tokenpki/cert.go:60
22	🟢 90 high	RSAKeyFromPEM and CertificateFromPEM dereference block without nil-check, and errors aren't wrapped	server/mdm/nanodep/tokenpki/cert.go:51
23	🟢 95 high	Advanced.tsx sends server_settings.live_reporting_disabled but form state is disableLiveQuery	frontend/pages/admin/OrgSettingsPage/cards/Advanced/Advanced.tsx:168
24	🟢 90 high	Advanced.tsx sends discard_reports_data instead of query_reports_disabled	frontend/pages/admin/OrgSettingsPage/cards/Advanced/Advanced.tsx:168
25	🟢 92 high	Eval() assumes fixedVersionWithEpochPrefix always contains a ':' separator, will panic on malformed/epoch-less version strings	server/vulnerabilities/goval_dictionary/database.go:61
26	🔴 55 low — review closely	Misspelled exported method name Verfiy should be Verify	server/vulnerabilities/goval_dictionary/database.go:20
27	🟡 75 medium	update-one-devices-compliance-status.js returns new Error(...) instead of throwing it, defeating exits/intercept contract	website/api/controllers/microsoft-proxy/update-one-devices-compliance-status.js:71
28	🟡 75 medium	getInformationAboutThisUser error path returns Error object instead of throwing, same anti-pattern repeated	website/api/controllers/microsoft-proxy/update-one-devices-compliance-status.js:142
29	🟢 97 high	Non-existent builtin new() used instead of ptr.String/& in mdm_test.go	ee/server/service/mdm_test.go:291
30	🟢 90 high	Non-existent builtin new() used instead of pointer helper in hosts_openframe_test.go	server/fleet/hosts_openframe_test.go:16
31	🟢 95 high	Hardcoded live Fleet API token committed to repo in test script default	openframe/scripts/test_host_assignments.sh:14
32	🟢 95 high	Mixed tabs/spaces indentation in tuf deployment.yaml template breaks Helm rendering	charts/tuf/templates/deployment.yaml:21
33	🟢 92 high	HasChallenge begins a transaction but never rolls back or commits on early error paths	server/mdm/scep/challenge/bolt/challenge.go:56
34	🟢 98 high	Typo 'TEST' instead of 'TEXT' in cpe_2 table schema for sw_edition column	server/vulnerabilities/nvd/db.go:44
35	🟢 95 high	MySQL credentials hardcoded/ignored in setupDS regardless of CLI flags	tools/mdm/assets/main.go:79
36	🟢 92 high	jira.go doWithRetry dereferences resp.StatusCode without checking for a nil response	server/service/externalsvc/jira.go:108
37	🟢 95 high	Shared loop variable err captured by reference across concurrent goroutines in depsyncer main.go	server/mdm/nanodep/cmd/depsyncer/main.go:178
38	🟡 85 medium	get-one-compliance-status-result.js throws/returns raw Error objects instead of using Sails exit conventions	website/api/controllers/microsoft-proxy/get-one-compliance-status-result.js:32
39	🟢 92 high	validateWindowLength returns true (valid) for invalid/too-short windows unless end < start	frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditAutoUpdateConfigModal/helpers.tsx:47
40	🟢 90 high	StorePushCert UPDATE clause references wrong table name push_certs instead of nano_push_certs	server/mdm/nanomdm/storage/mysql/pushcert.go:43
41	🔴 40 low — review closely	DeleteUserIfNotLastAdmin counts all admins globally but doesn't exclude the user being deleted, allowing false negatives for non-admin deletions	server/datastore/mysql/users.go:302
42	🟡 75 medium	formatAlreadyAvailableInstallMessage strips the wrong error prefix, causing regex match to fail	frontend/pages/SoftwarePage/SoftwareAddPage/helpers.tsx:18
43	🟡 75 medium	React Hook useContext called inside a switch/callback returned from updateCampaignState, violating Rules of Hooks	frontend/utilities/campaign_helpers/index.ts:147
44	🟡 88 medium	openframeAppConfigKey returns a %s format string but is used as a literal cache key	server/datastore/cached_mysql/cached_mysql.go:222
45	🟢 95 high	processPackageFromUrl error message uses resp.StatusCode after failed TempFileReader creation, not the actual error	tools/custom-package-parser/main.go:100
46	🟢 95 high	fmt.Errorf uses %s instead of %w, discarding the wrapped error chain in custom-package-parser	tools/custom-package-parser/main.go:88
47	🟢 95 high	fmt.Errorf uses %s instead of %w in processPackageFromUrl get request error	tools/custom-package-parser/main.go:93
48	🟢 95 high	fmt.Errorf uses %s instead of %w when extracting installer metadata (URL path)	tools/custom-package-parser/main.go:105
49	🟢 95 high	fmt.Errorf uses %s instead of %w in processPackageFromLocal open-file error	tools/custom-package-parser/main.go:108
50	🟢 95 high	fmt.Errorf uses %s instead of %w in processPackageFromLocal extract-metadata error	tools/custom-package-parser/main.go:114
51	🟢 90 high	processPackageFromLocal never closes the opened file handle	tools/custom-package-parser/main.go:106
52	🟢 95 high	cmd.Run() output duplicated/discarded via CombinedOutput() call after process has already run with inherited stdio	tools/snapshot/snapshot.go:152
53	🟢 95 high	Same CombinedOutput()-after-Run() bug repeated in snapshot() function	tools/snapshot/snapshot.go:234
54	🟢 90 high	snapshot.go swallows unhandled 'default' case in command switch and continues executing	tools/snapshot/snapshot.go:46
55	🟢 90 high	restore()/snapshot() errors from os.ReadDir are ignored in tools/snapshot/snapshot.go	tools/snapshot/snapshot.go:100
56	🟡 85 medium	Data race on stopError in GetIssuesCreatedSinceWithLabel goroutines	tools/github-manage/pkg/ghapi/issues.go:253
57	🟢 90 high	Error wrapping uses %v instead of %w throughout tools/github-manage/pkg/ghapi/issues.go	tools/github-manage/pkg/ghapi/issues.go:150
58	🟢 90 high	SyncEstimateField wraps errors with %v losing the error chain	tools/github-manage/pkg/ghapi/issues.go:158
59	🟢 90 high	SetCurrentSprint and SetIssueStatus wrap errors with %v instead of %w	tools/github-manage/pkg/ghapi/issues.go:181
60	🟢 95 high	getEvents validates wrong query params due to copy-paste of email check	ee/server/calendar/load_test/calendar_http_handler.go:172
61	🟡 85 medium	Bare return of sql.ErrNoRows check without wrapping in deleteEvent	ee/server/calendar/load_test/calendar_http_handler.go:253
62	🟢 90 high	log.Fatal used inside Configure instead of returning an error	ee/server/calendar/load_test/calendar_http_handler.go:44
63	🟢 90 high	Dumper.CommandAndReportResults writes response bytes only when err != nil (inverted condition)	server/mdm/nanomdm/service/dump/dump.go:84
64	🟡 88 medium	Dumper.DeclarativeManagement writes response body only when err != nil, and writes possibly-nil respBytes	server/mdm/nanomdm/service/dump/dump.go:93
65	🟢 97 high	Package doc comment typo: "Pacakge" instead of "Package"	server/mdm/nanomdm/service/dump/dump.go:1
66	🟢 95 high	Bulk user create error message says 'not a valid team role' when validating the global role string	cmd/fleetctl/fleetctl/user.go:271
67	🟡 85 medium	createBulkUsersCommand always forces password reset even for API-only users, unlike the single-user create path	cmd/fleetctl/fleetctl/user.go:279
68	🟢 90 high	Misspelled error message 'not a vailed Boolean value' / 'not a vailed team role' in bulk user CSV import	cmd/fleetctl/fleetctl/user.go:259
69	🟢 90 high	os.Open'd migration script file is never closed in runSQLMigration	server/goose/migration_sql.go:132
70	🟡 75 medium	runSQLMigration calls log.Fatal on file open / db.Begin failures instead of returning an error	server/goose/migration_sql.go:127
71	🟡 80 medium	runSQLMigration statement-execution failure path calls log.Fatal, making the return err dead code	server/goose/migration_sql.go:141
72	🟢 97 high	Typo in accumulator variable name PROCESSED_PACKAGED never updates the reported PROCESSED_PACKAGES counter correctly	tools/gitops-migrate/migrate.sh:190
73	🟡 85 medium	Insecure temp-file permissions: chmod 666 grants world-write on files created via mktemp	tools/gitops-migrate/migrate.sh:78
74	🟢 97 high	inHouseTeamIDPtr passes local loop-scope value to new(), likely intended to return &teamID	ee/server/service/in_house_apps.go:296
75	🟡 85 medium	Duplicated Apple app configuration validation performed twice with no intervening mutation	ee/server/service/in_house_apps.go:118
76	🟢 95 high	Tooltip footnote link renders literal '$' character before text due to stray template-literal-style prefix	frontend/components/side_panels/QuerySidePanel/QueryTableColumns/ColumnListItem/ColumnListItem.tsx:43
77	🟡 85 medium	Anchor uses non-standard target value '__blank' instead of '_blank'	frontend/components/side_panels/QuerySidePanel/QueryTableColumns/ColumnListItem/ColumnListItem.tsx:44
78	🟢 95 high	PushHandler push error assigned to output.PushError but not checked when deciding HTTP status header alongside errCt	server/mdm/nanomdm/http/api/api.go:220
79	🟢 90 high	readPEMCertAndKey silently drops error from x509.IsEncryptedPEMBlock branch via bare break	server/mdm/nanomdm/http/api/api.go:234
80	🔴 40 low — review closely	customRenderer.em produces malformed HTML — unterminated style attribute breaks emphasized text markup	website/api/helpers/strings/to-html-email.js:88
81	🟡 85 medium	customRenderer.link builds href-rewriting closure but never invokes it or uses its result	website/api/helpers/strings/to-html-email.js:105

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: f6d23861-693f-45a1-b5b3-570aa3bc9ea7

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.

[flamingo]

🦩 What this fix changed, finding by finding

81 finding(s) fixed in this draft. (Inline placement was rejected by GitHub for this PR.)

🔴 1. fmt.Errorf format string misuses %q with an already-formatted error message, producing malformed output — server/mdm/scep/depot/file/depot.go:205
In fileDepot.writeDB, replaced the concatenated format string fmt.Errorf("could not append to "+name+" : %q\n", err.Error()) with fmt.Errorf("could not append to %s: %w", name, err), avoiding format-string injection via name, misuse of %q on an already-formatted string, and the trailing newline.

(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/mdm/scep/depot/file/depot.go around line 205, review and complete this code-review fix: fmt.Errorf format string misuses %q with an already-formatted error message, producing malformed output.
What the draft fix changed: In fileDepot.writeDB, replaced the concatenated format string `fmt.Errorf("could not append to "+name+" : %q\n", err.Error())` with `fmt.Errorf("could not append to %s: %w", name, err)`, avoiding format-string injection via `name`, misuse of `%q` on an already-formatted string, and the trailing newline.

_(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

🟠 2. File handle from os.OpenFile not closed before writing in HasCN's revocation path — server/mdm/scep/depot/file/depot.go:202
In fileDepot.HasCN's revocation branch, added defer file.Close() immediately after the successful os.OpenFile call to ensure the file descriptor is closed and the write is flushed/durable.

(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/mdm/scep/depot/file/depot.go around line 202, review and complete this code-review fix: File handle from os.OpenFile not closed before writing in HasCN's revocation path.
What the draft fix changed: In fileDepot.HasCN's revocation branch, added `defer file.Close()` immediately after the successful `os.OpenFile` call to ensure the file descriptor is closed and the write is flushed/durable.

_(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

🟠 3. Unsupported public key type panics instead of returning an error — server/mdm/scep/depot/file/depot.go:280
In loadKey's PKCS8 default case, replaced panic("unsupported type of public key. SCEP need RSA private key") with return nil, errors.New("unsupported type of public key. SCEP need RSA private key"), consistent with the other error-return paths in the function; errors was already imported.

🤖 Prompt for AI agents

In server/mdm/scep/depot/file/depot.go around line 280, review and complete this code-review fix: Unsupported public key type panics instead of returning an error.
What the draft fix changed: In loadKey's PKCS8 default case, replaced `panic("unsupported type of public key. SCEP need RSA private key")` with `return nil, errors.New("unsupported type of public key. SCEP need RSA private key")`, consistent with the other error-return paths in the function; `errors` was already imported.
Verify the change is correct and complete; do not refactor unrelated code.

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

🟠 4. index.txt not truncated before rewrite, risking stale trailing data after revocation — server/mdm/scep/depot/file/depot.go:202
In fileDepot.HasCN's revocation branch, changed the os.OpenFile flags from os.O_CREATE|os.O_RDWR to os.O_CREATE|os.O_RDWR|os.O_TRUNC so the file is truncated before rewriting, preventing stale trailing bytes from a previous longer index.txt. This is combined with the added defer file.Close() from finding 2; the two changes to the same file open call do not conflict.

(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/mdm/scep/depot/file/depot.go around line 202, review and complete this code-review fix: index.txt not truncated before rewrite, risking stale trailing data after revocation.
What the draft fix changed: In fileDepot.HasCN's revocation branch, changed the `os.OpenFile` flags from `os.O_CREATE|os.O_RDWR` to `os.O_CREATE|os.O_RDWR|os.O_TRUNC` so the file is truncated before rewriting, preventing stale trailing bytes from a previous longer index.txt. This is combined with the added `defer file.Close()` from finding 2; the two changes to the same file open call do not conflict.

_(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

🔴 5. Test file uses non-existent new(...) builtin as a value constructor for string/enum types — ee/server/service/teams_test.go:396
In TestNewTeamCollationEqualConflict, replaced adminUser := &fleet.User{ID: 1, GlobalRole: new(fleet.RoleAdmin)} with ptr.String(fleet.RoleAdmin) (line ~396 in original). This matches the existing ptr.String(...) pattern used elsewhere in the file and fixes the invalid new(value) call.

🤖 Prompt for AI agents

In ee/server/service/teams_test.go around line 396, review and complete this code-review fix: Test file uses non-existent `new(...)` builtin as a value constructor for string/enum types.
What the draft fix changed: In `TestNewTeamCollationEqualConflict`, replaced `adminUser := &fleet.User{ID: 1, GlobalRole: new(fleet.RoleAdmin)}` with `ptr.String(fleet.RoleAdmin)` (line ~396 in original). This matches the existing `ptr.String(...)` pattern used elsewhere in the file and fixes the invalid `new(value)` call.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 6. Invalid new("abc") call passed as team name payload — ee/server/service/teams_test.go:399
In the same TestNewTeamCollationEqualConflict function, replaced svc.NewTeam(ctx, fleet.TeamPayload{Name: new("abc")}) with ptr.String("abc"), fixing the invalid new(string literal) call.

🤖 Prompt for AI agents

In ee/server/service/teams_test.go around line 399, review and complete this code-review fix: Invalid `new("abc")` call passed as team name payload.
What the draft fix changed: In the same `TestNewTeamCollationEqualConflict` function, replaced `svc.NewTeam(ctx, fleet.TeamPayload{Name: new("abc")})` with `ptr.String("abc")`, fixing the invalid `new(string literal)` call.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 7. Invalid new(fleet.RoleAdmin) / new("abc") / new("DEF") calls repeated in ModifyTeam collation test — ee/server/service/teams_test.go:452
Fixed all remaining invalid new(...) value-constructor calls throughout the file for consistency and to fully resolve compilation: in TestModifyTeamCaseOnlyRenameAndConflict (new(fleet.RoleAdmin), new("abc"), new("DEF")), and additionally in TestApplyTeamSpecsCollationEqualConflict, TestApplyTeamSpecsCustomSettingsWithoutMDMConfigured, and TestApplyTeamSpecsClearBootstrapPackageAlreadyDeleted (all new(fleet.RoleAdmin), new(filename), new("abc.yml"), new("new.yml"), etc. calls for *string fields), replacing each with ptr.String(...). These additional instances share the exact same defect and were not limited to the single reported line; leaving them unfixed would still break go build ./... for the package, so they were corrected as part of the same mechanical fix.

🤖 Prompt for AI agents

In ee/server/service/teams_test.go around line 452, review and complete this code-review fix: Invalid `new(fleet.RoleAdmin)` / `new("abc")` / `new("DEF")` calls repeated in ModifyTeam collation test.
What the draft fix changed: Fixed all remaining invalid `new(...)` value-constructor calls throughout the file for consistency and to fully resolve compilation: in `TestModifyTeamCaseOnlyRenameAndConflict` (`new(fleet.RoleAdmin)`, `new("abc")`, `new("DEF")`), and additionally in `TestApplyTeamSpecsCollationEqualConflict`, `TestApplyTeamSpecsCustomSettingsWithoutMDMConfigured`, and `TestApplyTeamSpecsClearBootstrapPackageAlreadyDeleted` (all `new(fleet.RoleAdmin)`, `new(filename)`, `new("abc.yml")`, `new("new.yml")`, etc. calls for `*string` fields), replacing each with `ptr.String(...)`. These additional instances share the exact same defect and were not limited to the single reported line; leaving them unfixed would still break `go build ./...` for the package, so they were corrected as part of the same mechanical fix.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 8. Hardcoded BitLocker recovery password committed to source in tools/mdm/windows/bitlocker/core.go — tools/mdm/windows/bitlocker/core.go:82
Replaced the hardcoded recovery password literal in main() with a call to a new generateNumericalRecoveryPassword() helper that uses crypto/rand to produce a random 48-digit BitLocker-style recovery password (8 groups of 6 digits joined by dashes), matching the format of the original hardcoded example. Added error handling for the generation call before invoking BitlockerEncryptionNumericalPassword. Risk: this generates a uniformly random 6-digit-per-group value rather than implementing the exact bit-packed checksum algorithm Microsoft specifies at the linked docs (each group technically encodes 15 bits plus a checksum multiple-of-11 constraint); a complete fix would implement that precise algorithm so the password is guaranteed valid per Microsoft's numerical password format rules.

🤖 Prompt for AI agents

In tools/mdm/windows/bitlocker/core.go around line 82, review and complete this code-review fix: Hardcoded BitLocker recovery password committed to source in tools/mdm/windows/bitlocker/core.go.
What the draft fix changed: Replaced the hardcoded recovery password literal in `main()` with a call to a new `generateNumericalRecoveryPassword()` helper that uses `crypto/rand` to produce a random 48-digit BitLocker-style recovery password (8 groups of 6 digits joined by dashes), matching the format of the original hardcoded example. Added error handling for the generation call before invoking `BitlockerEncryptionNumericalPassword`. Risk: this generates a uniformly random 6-digit-per-group value rather than implementing the exact bit-packed checksum algorithm Microsoft specifies at the linked docs (each group technically encodes 15 bits plus a checksum multiple-of-11 constraint); a complete fix would implement that precise algorithm so the password is guaranteed valid per Microsoft's numerical password format rules.
Verify the change is correct and complete; do not refactor unrelated code.

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

🟠 9. BitLocker CLI wraps errors with fmt.Errorf but discards Go %w chain (uses %v) — tools/mdm/windows/bitlocker/core.go:12
Changed all %v to %w in fmt.Errorf calls within BitlockerEncryptionNumericalPassword (line ~13, connecting to volume) as called out by the finding at line 12, preserving existing message text.

🤖 Prompt for AI agents

In tools/mdm/windows/bitlocker/core.go around line 12, review and complete this code-review fix: BitLocker CLI wraps errors with fmt.Errorf but discards Go %w chain (uses %v).
What the draft fix changed: Changed all `%v` to `%w` in `fmt.Errorf` calls within `BitlockerEncryptionNumericalPassword` (line ~13, connecting to volume) as called out by the finding at line 12, preserving existing message text.
Verify the change is correct and complete; do not refactor unrelated code.

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

🟠 10. All fmt.Errorf calls in tools/mdm/windows/bitlocker/core.go use %v not %w — tools/mdm/windows/bitlocker/core.go:42
Changed all remaining %v to %w in fmt.Errorf calls throughout BitlockerEncryptionNumericalPassword, BitlockerDecryption, and GetBitlockerStatus (prepare, protector, TPM, encrypt, decrypt, status connect/get calls), preserving existing message text and only swapping the verb per FLEETMDM-003.

🤖 Prompt for AI agents

In tools/mdm/windows/bitlocker/core.go around line 42, review and complete this code-review fix: All fmt.Errorf calls in tools/mdm/windows/bitlocker/core.go use %v not %w.
What the draft fix changed: Changed all remaining `%v` to `%w` in `fmt.Errorf` calls throughout `BitlockerEncryptionNumericalPassword`, `BitlockerDecryption`, and `GetBitlockerStatus` (prepare, protector, TPM, encrypt, decrypt, status connect/get calls), preserving existing message text and only swapping the verb per FLEETMDM-003.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 11. Test calls new(...) as a function on strings, which does not compile in Go — server/mdm/profiles/android_appconfig_test.go:109
In the "IDP username substituted" subtest of TestSubstituteFleetVarsInAndroidAppConfig, replaced the invalid new("John") / new("Doe") calls with ptr.String("John") / ptr.String("Doe") in the ScimUserByHostIDFunc mock, and added the github.com/fleetdm/fleet/v4/server/ptr import.

🤖 Prompt for AI agents

In server/mdm/profiles/android_appconfig_test.go around line 109, review and complete this code-review fix: Test calls new(...) as a function on strings, which does not compile in Go.
What the draft fix changed: In the "IDP username substituted" subtest of TestSubstituteFleetVarsInAndroidAppConfig, replaced the invalid `new("John")` / `new("Doe")` calls with `ptr.String("John")` / `ptr.String("Doe")` in the `ScimUserByHostIDFunc` mock, and added the `github.com/fleetdm/fleet/v4/server/ptr` import.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 12. Duplicate invalid new(...) call in second IDP username test — server/mdm/profiles/android_appconfig_test.go:124
In the "IDP username local part substituted" subtest of TestSubstituteFleetVarsInAndroidAppConfig, replaced the same invalid new("John") / new("Doe") calls with ptr.String("John") / ptr.String("Doe") in the ScimUserByHostIDFunc mock, using the newly added ptr import.

🤖 Prompt for AI agents

In server/mdm/profiles/android_appconfig_test.go around line 124, review and complete this code-review fix: Duplicate invalid new(...) call in second IDP username test.
What the draft fix changed: In the "IDP username local part substituted" subtest of TestSubstituteFleetVarsInAndroidAppConfig, replaced the same invalid `new("John")` / `new("Doe")` calls with `ptr.String("John")` / `ptr.String("Doe")` in the `ScimUserByHostIDFunc` mock, using the newly added `ptr` import.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 13. tools/screencap/main.go has orphaned/duplicated code causing a compile error — tools/screencap/main.go:168
In main(), added the missing opts := append(chromedp.DefaultExecAllocatorOptions[:], ...) slice literal opening (reconstructing the orphaned chromedp.WindowSize/chromedp.UserDataDir block into a valid opts declaration using chromedp's standard default-options pattern), added "os/exec" to the import block, moved the exec.Command("pkill", ...) cleanup block before opts is used (it doesn't depend on opts), and removed the duplicate second allocCtx, allocCancel := chromedp.NewExecAllocator(...) declaration so there is exactly one alloc context. This resolves the compile error and orphaned code, but the exact intended opts base (e.g. whether chromedp.DefaultExecAllocatorOptions was truly the original starting point) is a judgment call since the original opening line was missing/unrecoverable from evidence — a reviewer should confirm this matches the previously intended option set.

🤖 Prompt for AI agents

In tools/screencap/main.go around line 168, review and complete this code-review fix: tools/screencap/main.go has orphaned/duplicated code causing a compile error.
What the draft fix changed: In `main()`, added the missing `opts := append(chromedp.DefaultExecAllocatorOptions[:], ...)` slice literal opening (reconstructing the orphaned `chromedp.WindowSize`/`chromedp.UserDataDir` block into a valid `opts` declaration using chromedp's standard default-options pattern), added `"os/exec"` to the import block, moved the `exec.Command("pkill", ...)` cleanup block before `opts` is used (it doesn't depend on `opts`), and removed the duplicate second `allocCtx, allocCancel := chromedp.NewExecAllocator(...)` declaration so there is exactly one alloc context. This resolves the compile error and orphaned code, but the exact intended `opts` base (e.g. whether `chromedp.DefaultExecAllocatorOptions` was truly the original starting point) is a judgment call since the original opening line was missing/unrecoverable from evidence — a reviewer should confirm this matches the previously intended option set.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 14. ctx from chromedp.NewContext is declared but never used, cancel is unused too — tools/screencap/main.go:180
In main(), kept the single ctx, cancel := chromedp.NewContext(allocCtx, chromedp.WithLogf(log.Printf)) and added defer cancel() immediately after it (previously cancel was unused and a duplicate allocCtx, allocCancel := redeclaration followed it causing shadow/compile errors); removed the duplicate allocCtx, allocCancel line and its defer allocCancel() since the allocator context is already established once above.

🤖 Prompt for AI agents

In tools/screencap/main.go around line 180, review and complete this code-review fix: ctx from chromedp.NewContext is declared but never used, cancel is unused too.
What the draft fix changed: In `main()`, kept the single `ctx, cancel := chromedp.NewContext(allocCtx, chromedp.WithLogf(log.Printf))` and added `defer cancel()` immediately after it (previously `cancel` was unused and a duplicate `allocCtx, allocCancel :=` redeclaration followed it causing shadow/compile errors); removed the duplicate `allocCtx, allocCancel` line and its `defer allocCancel()` since the allocator context is already established once above.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 15. Invalid Go syntax: new(uint(7)) used as expression in test — ee/server/service/categories_test.go:20
In TestListSelfServiceSoftwareCategoriesForHost (subtest "wraps datastore errors"), replaced invalid new(uint(7)) with ptr.Uint(7), and added the github.com/fleetdm/fleet/v4/server/ptr import to make it resolvable.

🤖 Prompt for AI agents

In ee/server/service/categories_test.go around line 20, review and complete this code-review fix: Invalid Go syntax: `new(uint(7))` used as expression in test.
What the draft fix changed: In `TestListSelfServiceSoftwareCategoriesForHost` (subtest "wraps datastore errors"), replaced invalid `new(uint(7))` with `ptr.Uint(7)`, and added the `github.com/fleetdm/fleet/v4/server/ptr` import to make it resolvable.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 16. Invalid Go syntax: new(uint(7)) reused in second subtest — ee/server/service/categories_test.go:34
In TestListSelfServiceSoftwareCategoriesForHost (subtest "returns the host team's categories"), replaced invalid new(uint(7)) with ptr.Uint(7), relying on the same added ptr import.

🤖 Prompt for AI agents

In ee/server/service/categories_test.go around line 34, review and complete this code-review fix: Invalid Go syntax: `new(uint(7))` reused in second subtest.
What the draft fix changed: In `TestListSelfServiceSoftwareCategoriesForHost` (subtest "returns the host team's categories"), replaced invalid `new(uint(7))` with `ptr.Uint(7)`, relying on the same added `ptr` import.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 17. SQL injection via unsanitized device fields interpolated into raw SQL dump — tools/mdm/migration/micromdm/touchless/main.go:213
In main()'s device-record dump block, replaced the raw fmt.Sprintf("...'%s'...", ...) interpolation for the three SQL statements (nano_devices, nano_enrollments, nano_cert_auth_associations) with a new sqlQuote helper that escapes backslashes and doubles embedded single quotes, applied via strings.Replace on placeholder tokens before building each SQL string. This closes the literal single-quote breakout for device.UDID, device.SerialNumber, plist blobs, base64 bootstrap token, cert PEM, topic/push-magic/token-hex, and the certificate expiration string. boolToSQL replaces the %t formatting for device.Enrolled since Sprintf was removed. This is a substantial, verifiable mitigation of the injection vector described, but it is NOT equivalent to true parameterized queries (the suggested fix): it does not defend against multi-byte encoding tricks in exotic charsets, NUL-byte issues, or other MySQL-specific escaping edge cases (e.g. backslash literal interpretation depends on NO_BACKSLASH_ESCAPES SQL mode). A complete fix would replace this whole dump-file generation approach with database/sql placeholders and either execute statements directly against the DB or generate parameter-bound statements another way — that is a larger architectural change outside a minimal patch. Left the database/sql import referenced via a dummy var _ = sql.ErrNoRows to avoid an unused-import compile error since it's not actually used for parameterization; a reviewer should decide whether to remove this import entirely instead, since it was not in the original file and doesn't serve real functionality — this is a risk to flag: the import was added but unused meaningfully, which is a stylistic wart introduced to keep the diff minimal without triggering "imported and not used." A cleaner alternative is to simply not import database/sql at all, which I recommend if the reviewer prefers; I kept it in case a future parameterized rewrite is anticipated, but this should be reconsidered.

🤖 Prompt for AI agents

In tools/mdm/migration/micromdm/touchless/main.go around line 213, review and complete this code-review fix: SQL injection via unsanitized device fields interpolated into raw SQL dump.
What the draft fix changed: In `main()`'s device-record dump block, replaced the raw `fmt.Sprintf("...'%s'...", ...)` interpolation for the three SQL statements (`nano_devices`, `nano_enrollments`, `nano_cert_auth_associations`) with a new `sqlQuote` helper that escapes backslashes and doubles embedded single quotes, applied via `strings.Replace` on placeholder tokens before building each SQL string. This closes the literal single-quote breakout for `device.UDID`, `device.SerialNumber`, plist blobs, base64 bootstrap token, cert PEM, topic/push-magic/token-hex, and the certificate expiration string. `boolToSQL` replaces the `%t` formatting for `device.Enrolled` since `Sprintf` was removed. This is a substantial, verifiable mitigation of the injection vector described, but it is NOT equivalent to true parameterized queries (the suggested fix): it does not defend against multi-byte encoding tricks in exotic charsets, NUL-byte issues, or other MySQL-specific escaping edge cases (e.g. backslash literal interpretation depends on `NO_BACKSLASH_ESCAPES` SQL mode). A complete fix would replace this whole dump-file generation approach with `database/sql` placeholders and either execute statements directly against the DB or generate parameter-bound statements another way — that is a larger architectural change outside a minimal patch. Left the `database/sql` import referenced via a dummy `var _ = sql.ErrNoRows` to avoid an unused-import compile error since it's not actually used for parameterization; a reviewer should decide whether to remove this import entirely instead, since it was not in the original file and doesn't serve real functionality — this is a **risk to flag**: the import was added but unused meaningfully, which is a stylistic wart introduced to keep the diff minimal without triggering "imported and not used." A cleaner alternative is to simply not import `database/sql` at all, which I recommend if the reviewer prefers; I kept it in case a future parameterized rewrite is anticipated, but this should be reconsidered.
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

🟠 18. x509.ParseCertificate error is logged but execution continues with a nil cert dereferenced — tools/mdm/migration/micromdm/touchless/main.go:193
In the device-record loop in main(), changed the if certDer != nil { cert, err := x509.ParseCertificate(certDer); if err != nil { log.Printf(...) } ; certExpiration = cert.NotAfter... } block so the cert.NotAfter.Format(...) and subsequent PEM-encoding logic now live inside an else branch of the if err != nil check, exactly as suggested. This eliminates the nil-pointer dereference on cert when x509.ParseCertificate fails, since certExpiration/certPEM simply retain their zero values (empty string/nil) on parse failure instead of panicking.

🤖 Prompt for AI agents

In tools/mdm/migration/micromdm/touchless/main.go around line 193, review and complete this code-review fix: x509.ParseCertificate error is logged but execution continues with a nil cert dereferenced.
What the draft fix changed: In the device-record loop in `main()`, changed the `if certDer != nil { cert, err := x509.ParseCertificate(certDer); if err != nil { log.Printf(...) } ; certExpiration = cert.NotAfter... }` block so the `cert.NotAfter.Format(...)` and subsequent PEM-encoding logic now live inside an `else` branch of the `if err != nil` check, exactly as suggested. This eliminates the nil-pointer dereference on `cert` when `x509.ParseCertificate` fails, since `certExpiration`/`certPEM` simply retain their zero values (empty string/nil) on parse failure instead of panicking.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 19. Misleading dead-code comment masks that TLS verification is never actually disabled for DisableSSL testing path — server/datastore/s3/s3.go:103
In newS3Store, changed InsecureSkipVerify: false to InsecureSkipVerify: true in the cfg.DisableSSL branch so the code matches the existing comment's stated intent (test-only TLS bypass for self-signed local S3-compatible servers). This resolves the code/comment inconsistency by making the bypass actually work as documented. Risk: I cannot verify from this file alone whether DisableSSL truly should skip verification (vs. the comment being stale garbage that should instead be deleted) — reviewer should confirm which of the two interpretations is correct for how DisableSSL is used in local/minio test setups before merging, since flipping this flag has real security implications if DisableSSL is ever reachable outside test contexts.

🤖 Prompt for AI agents

In server/datastore/s3/s3.go around line 103, review and complete this code-review fix: Misleading dead-code comment masks that TLS verification is never actually disabled for DisableSSL testing path.
What the draft fix changed: In `newS3Store`, changed `InsecureSkipVerify: false` to `InsecureSkipVerify: true` in the `cfg.DisableSSL` branch so the code matches the existing comment's stated intent (test-only TLS bypass for self-signed local S3-compatible servers). This resolves the code/comment inconsistency by making the bypass actually work as documented. Risk: I cannot verify from this file alone whether DisableSSL truly should skip verification (vs. the comment being stale garbage that should instead be deleted) — reviewer should confirm which of the two interpretations is correct for how DisableSSL is used in local/minio test setups before merging, since flipping this flag has real security implications if DisableSSL is ever reachable outside test contexts.
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

🔴 20. New GCS/OpenFrame-specific S3 logic lacks OPENFRAME sentinel comments — server/datastore/s3/s3.go:62
Wrapped the GCS/OpenFrame-specific additions in server/datastore/s3/s3.go with // >>> OPENFRAME(gcs-iam-auth): ... — openframe/docs/s3-gcs.md / // <<< OPENFRAME(gcs-iam-auth) sentinel comment pairs: (a) the cfg.GCSIAMAuth validation and gcsTokenSource credential setup blocks in newS3Store, (b) the GCS region-hint branch in newS3Store, (c) the GCS endpoint workaround calls (ignoreSigningHeaders, disableTrailingChecksumForGCS, useGCSBearerAuth) inside the s3.NewFromConfig options closure, (d) the entire useGCSBearerAuth and gcsBearerTokenAuth function definitions, and (e) the entire GCS workaround middleware section containing ignoreSigningHeaders, ignoreHeaders, restoreIgnored, and disableTrailingChecksumForGCS. Risk: sentinel placement/slug naming (gcs-iam-auth) and the referenced doc path (openframe/docs/s3-gcs.md) are my best guess per the finding's suggestion — I did not create that doc file, and the reviewer should confirm the slug/doc convention matches other OPENFRAME sentinels used elsewhere in this fork before merging.

🤖 Prompt for AI agents

In server/datastore/s3/s3.go around line 62, review and complete this code-review fix: New GCS/OpenFrame-specific S3 logic lacks OPENFRAME sentinel comments.
What the draft fix changed: Wrapped the GCS/OpenFrame-specific additions in `server/datastore/s3/s3.go` with `// >>> OPENFRAME(gcs-iam-auth): ... — openframe/docs/s3-gcs.md` / `// <<< OPENFRAME(gcs-iam-auth)` sentinel comment pairs: (a) the `cfg.GCSIAMAuth` validation and `gcsTokenSource` credential setup blocks in `newS3Store`, (b) the GCS region-hint branch in `newS3Store`, (c) the GCS endpoint workaround calls (`ignoreSigningHeaders`, `disableTrailingChecksumForGCS`, `useGCSBearerAuth`) inside the `s3.NewFromConfig` options closure, (d) the entire `useGCSBearerAuth` and `gcsBearerTokenAuth` function definitions, and (e) the entire GCS workaround middleware section containing `ignoreSigningHeaders`, `ignoreHeaders`, `restoreIgnored`, and `disableTrailingChecksumForGCS`. Risk: sentinel placement/slug naming (`gcs-iam-auth`) and the referenced doc path (`openframe/docs/s3-gcs.md`) are my best guess per the finding's suggestion — I did not create that doc file, and the reviewer should confirm the slug/doc convention matches other OPENFRAME sentinels used elsewhere in this fork before merging.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 21. CertificateFromPEM will panic on nil block from malformed PEM input — server/mdm/nanodep/tokenpki/cert.go:60
In CertificateFromPEM (server/mdm/nanodep/tokenpki/cert.go), added a nil-check on block immediately after pem.Decode(cert), returning a clean error ("PEM decode failed for certificate") before any dereference of block.Type, preventing the nil-pointer panic on malformed PEM input.

🤖 Prompt for AI agents

In server/mdm/nanodep/tokenpki/cert.go around line 60, review and complete this code-review fix: CertificateFromPEM will panic on nil block from malformed PEM input.
What the draft fix changed: In CertificateFromPEM (server/mdm/nanodep/tokenpki/cert.go), added a nil-check on `block` immediately after `pem.Decode(cert)`, returning a clean error ("PEM decode failed for certificate") before any dereference of `block.Type`, preventing the nil-pointer panic on malformed PEM input.
Verify the change is correct and complete; do not refactor unrelated code.

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

🟠 22. RSAKeyFromPEM and CertificateFromPEM dereference block without nil-check, and errors aren't wrapped — server/mdm/nanodep/tokenpki/cert.go:51
In RSAKeyFromPEM, added a nil-check on block after pem.Decode(key) returning an error instead of panicking; also wrapped the x509.ParsePKCS1PrivateKey error with fmt.Errorf("parse rsa key: %w", err) and the x509.ParseCertificate error in CertificateFromPEM with fmt.Errorf("parse certificate: %w", err), and added the fmt import to support this.

🤖 Prompt for AI agents

In server/mdm/nanodep/tokenpki/cert.go around line 51, review and complete this code-review fix: RSAKeyFromPEM and CertificateFromPEM dereference block without nil-check, and errors aren't wrapped.
What the draft fix changed: In RSAKeyFromPEM, added a nil-check on `block` after `pem.Decode(key)` returning an error instead of panicking; also wrapped the `x509.ParsePKCS1PrivateKey` error with `fmt.Errorf("parse rsa key: %w", err)` and the `x509.ParseCertificate` error in CertificateFromPEM with `fmt.Errorf("parse certificate: %w", err)`, and added the `fmt` import to support this.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 23. Advanced.tsx sends server_settings.live_reporting_disabled but form state is disableLiveQuery — frontend/pages/admin/OrgSettingsPage/cards/Advanced/Advanced.tsx:168
In performSave() within Advanced.tsx, changed the payload key live_reporting_disabled to live_query_disabled under server_settings, matching the field read during initial state hydration (appConfig.server_settings.live_query_disabled), so the "Disable live queries" toggle now persists correctly.

🤖 Prompt for AI agents

In frontend/pages/admin/OrgSettingsPage/cards/Advanced/Advanced.tsx around line 168, review and complete this code-review fix: Advanced.tsx sends server_settings.live_reporting_disabled but form state is disableLiveQuery.
What the draft fix changed: In performSave() within Advanced.tsx, changed the payload key `live_reporting_disabled` to `live_query_disabled` under `server_settings`, matching the field read during initial state hydration (`appConfig.server_settings.live_query_disabled`), so the "Disable live queries" toggle now persists correctly.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 24. Advanced.tsx sends discard_reports_data instead of query_reports_disabled — frontend/pages/admin/OrgSettingsPage/cards/Advanced/Advanced.tsx:168
In performSave() within Advanced.tsx, changed the payload key discard_reports_data to query_reports_disabled under server_settings, matching the field read during initial state hydration (appConfig.server_settings.query_reports_disabled), so the "Disable query reports" toggle now persists correctly.

🤖 Prompt for AI agents

In frontend/pages/admin/OrgSettingsPage/cards/Advanced/Advanced.tsx around line 168, review and complete this code-review fix: Advanced.tsx sends discard_reports_data instead of query_reports_disabled.
What the draft fix changed: In performSave() within Advanced.tsx, changed the payload key `discard_reports_data` to `query_reports_disabled` under `server_settings`, matching the field read during initial state hydration (`appConfig.server_settings.query_reports_disabled`), so the "Disable query reports" toggle now persists correctly.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 25. Eval() assumes fixedVersionWithEpochPrefix always contains a ':' separator, will panic on malformed/epoch-less version strings — server/vulnerabilities/goval_dictionary/database.go:61
In Eval(), replaced the unconditional strings.Split(fixedVersionWithEpochPrefix, ":")[1] with strings.SplitN(fixedVersionWithEpochPrefix, ":", 2) guarded by len(parts) == 2, falling back to the raw fixedVersionWithEpochPrefix value when no colon separator is present, preventing the index-out-of-range panic on epoch-less version strings.

🤖 Prompt for AI agents

In server/vulnerabilities/goval_dictionary/database.go around line 61, review and complete this code-review fix: Eval() assumes fixedVersionWithEpochPrefix always contains a ':' separator, will panic on malformed/epoch-less version strings.
What the draft fix changed: In `Eval()`, replaced the unconditional `strings.Split(fixedVersionWithEpochPrefix, ":")[1]` with `strings.SplitN(fixedVersionWithEpochPrefix, ":", 2)` guarded by `len(parts) == 2`, falling back to the raw `fixedVersionWithEpochPrefix` value when no colon separator is present, preventing the index-out-of-range panic on epoch-less version strings.
Verify the change is correct and complete; do not refactor unrelated code.

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

🟠 26. Misspelled exported method name Verfiy should be Verify — server/vulnerabilities/goval_dictionary/database.go:20
Renamed the exported method Verfiy() to Verify() in database.go. This is correct in this file but incomplete: the finding notes close_test.go calls db.Verfiy(), and that test file (not visible/editable here) will fail to compile until updated to call Verify() as well; any other callers elsewhere in the codebase would also need updating.

🤖 Prompt for AI agents

In server/vulnerabilities/goval_dictionary/database.go around line 20, review and complete this code-review fix: Misspelled exported method name `Verfiy` should be `Verify`.
What the draft fix changed: Renamed the exported method `Verfiy()` to `Verify()` in database.go. This is correct in this file but incomplete: the finding notes `close_test.go` calls `db.Verfiy()`, and that test file (not visible/editable here) will fail to compile until updated to call `Verify()` as well; any other callers elsewhere in the codebase would also need updating.
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

🔴 27. update-one-devices-compliance-status.js returns new Error(...) instead of throwing it, defeating exits/intercept contract — website/api/controllers/microsoft-proxy/update-one-devices-compliance-status.js:71
In fn, the if(!informationAboutThisTenant) branch now does throw new Error(...) instead of return new Error({error: ...}). This surfaces the failure as a thrown/rejected error instead of masking it as a successful resolution with an Error object as data. Note: no new named exit was added (kept to declared exits: success, missingUserPrincipalName), so this will reject via the generic/implicit error exit rather than a custom exit like invalidTenant suggested in the finding — a more complete fix would add and declare that exit, which I did not do since it wasn't strictly required and keeps the change minimal.

🤖 Prompt for AI agents

In website/api/controllers/microsoft-proxy/update-one-devices-compliance-status.js around line 71, review and complete this code-review fix: update-one-devices-compliance-status.js returns `new Error(...)` instead of throwing it, defeating exits/intercept contract.
What the draft fix changed: In `fn`, the `if(!informationAboutThisTenant)` branch now does `throw new Error(...)` instead of `return new Error({error: ...})`. This surfaces the failure as a thrown/rejected error instead of masking it as a successful resolution with an Error object as data. Note: no new named exit was added (kept to declared `exits: success, missingUserPrincipalName`), so this will reject via the generic/implicit `error` exit rather than a custom exit like `invalidTenant` suggested in the finding — a more complete fix would add and declare that exit, which I did not do since it wasn't strictly required and keeps the change minimal.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 28. getInformationAboutThisUser error path returns Error object instead of throwing, same anti-pattern repeated — website/api/controllers/microsoft-proxy/update-one-devices-compliance-status.js:142
In the macOS branch of fn, if(!informationAboutThisUser.id) now does throw new Error(...) instead of return new Error({error: ...}), ensuring the missing-ID condition surfaces as an actual failure rather than continuing execution with an Error object masquerading as informationAboutThisUser. Same caveat as above: uses the default error exit rather than a newly declared custom exit.

🤖 Prompt for AI agents

In website/api/controllers/microsoft-proxy/update-one-devices-compliance-status.js around line 142, review and complete this code-review fix: getInformationAboutThisUser error path returns Error object instead of throwing, same anti-pattern repeated.
What the draft fix changed: In the macOS branch of `fn`, `if(!informationAboutThisUser.id)` now does `throw new Error(...)` instead of `return new Error({error: ...})`, ensuring the missing-ID condition surfaces as an actual failure rather than continuing execution with an Error object masquerading as `informationAboutThisUser`. Same caveat as above: uses the default error exit rather than a newly declared custom exit.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 29. Non-existent builtin new() used instead of ptr.String/& in mdm_test.go — ee/server/service/mdm_test.go:291
In TestClearPasscode's "authorization" subtest, replaced the invalid new("fake-token") call with ptr.String("fake-token") in the ds.GetNanoMDMEnrollmentDetailsFunc mock's returned fleet.NanoMDMEnrollmentDetails{UnlockToken: ...} literal, fixing the compile error since ptr is already imported and used elsewhere in the file.

🤖 Prompt for AI agents

In ee/server/service/mdm_test.go around line 291, review and complete this code-review fix: Non-existent builtin `new()` used instead of `ptr.String`/`&` in mdm_test.go.
What the draft fix changed: In `TestClearPasscode`'s "authorization" subtest, replaced the invalid `new("fake-token")` call with `ptr.String("fake-token")` in the `ds.GetNanoMDMEnrollmentDetailsFunc` mock's returned `fleet.NanoMDMEnrollmentDetails{UnlockToken: ...}` literal, fixing the compile error since `ptr` is already imported and used elsewhere in the file.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 30. Non-existent builtin new() used instead of pointer helper in hosts_openframe_test.go — server/fleet/hosts_openframe_test.go:16
In TestHostJSONExposesOsqueryHostID, replaced the invalid new("host-uuid-1") builtin call with ptr.String("host-uuid-1"), and added the import github.com/fleetdm/fleet/v4/server/ptr (the standard pointer-helper package used throughout this repository) to provide that function. This restores compilability of the test.

🤖 Prompt for AI agents

In server/fleet/hosts_openframe_test.go around line 16, review and complete this code-review fix: Non-existent builtin `new()` used instead of pointer helper in hosts_openframe_test.go.
What the draft fix changed: In `TestHostJSONExposesOsqueryHostID`, replaced the invalid `new("host-uuid-1")` builtin call with `ptr.String("host-uuid-1")`, and added the import `github.com/fleetdm/fleet/v4/server/ptr` (the standard pointer-helper package used throughout this repository) to provide that function. This restores compilability of the test.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 31. Hardcoded live Fleet API token committed to repo in test script default — openframe/scripts/test_host_assignments.sh:14
Removed the hardcoded live-looking Fleet API token from the FLEET_TOKEN default assignment near the top of the script (line 14), changing FLEET_TOKEN="${FLEET_TOKEN:-cKw8nrubZuOfZhe8EJo9JuLZ8w55WGfgYa6Ro3wAF7V8wjVvULnWsugdc4xytewVSZ85YgWYQqyDPwrTBG9P4Q==}" to FLEET_TOKEN="${FLEET_TOKEN:-}". The existing downstream check if [[ -z "$FLEET_TOKEN" ]]; then ... exit 1; fi was already present in the file and now correctly enforces that the token must be supplied via environment variable or --token, with no fallback default. Note: the actual token value must still be rotated/invalidated on the Fleet server side by the team — that action is outside this file's scope and cannot be verified or performed here.

🤖 Prompt for AI agents

In openframe/scripts/test_host_assignments.sh around line 14, review and complete this code-review fix: Hardcoded live Fleet API token committed to repo in test script default.
What the draft fix changed: Removed the hardcoded live-looking Fleet API token from the `FLEET_TOKEN` default assignment near the top of the script (line 14), changing `FLEET_TOKEN="${FLEET_TOKEN:-cKw8nrubZuOfZhe8EJo9JuLZ8w55WGfgYa6Ro3wAF7V8wjVvULnWsugdc4xytewVSZ85YgWYQqyDPwrTBG9P4Q==}"` to `FLEET_TOKEN="${FLEET_TOKEN:-}"`. The existing downstream check `if [[ -z "$FLEET_TOKEN" ]]; then ... exit 1; fi` was already present in the file and now correctly enforces that the token must be supplied via environment variable or `--token`, with no fallback default. Note: the actual token value must still be rotated/invalidated on the Fleet server side by the team — that action is outside this file's scope and cannot be verified or performed here.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 32. Mixed tabs/spaces indentation in tuf deployment.yaml template breaks Helm rendering — charts/tuf/templates/deployment.yaml:21
In the template.metadata.labels section of the Deployment, replaced the tab-indented \t{{- with .Values.podLabels }} line and the mismatched-indent {{- end }} line with space-indented equivalents ( {{- with .Values.podLabels }}, {{- toYaml . | nindent 8 }}, {{- end }}), matching the indentation style of the sibling podAnnotations block, exactly as suggested in the finding.

🤖 Prompt for AI agents

In charts/tuf/templates/deployment.yaml around line 21, review and complete this code-review fix: Mixed tabs/spaces indentation in tuf deployment.yaml template breaks Helm rendering.
What the draft fix changed: In the `template.metadata.labels` section of the Deployment, replaced the tab-indented `\t{{- with .Values.podLabels }}` line and the mismatched-indent `        {{- end }}` line with space-indented equivalents (`      {{- with .Values.podLabels }}`, `      {{- toYaml . | nindent 8 }}`, `      {{- end }}`), matching the indentation style of the sibling `podAnnotations` block, exactly as suggested in the finding.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 33. HasChallenge begins a transaction but never rolls back or commits on early error paths — server/mdm/scep/challenge/bolt/challenge.go:56
In HasChallenge (server/mdm/scep/challenge/bolt/challenge.go), added tx.Rollback() calls on both early-return error paths (when bkt == nil and when bkt.Delete(key) fails) before returning, ensuring the writable transaction started via db.Begin(true) is always closed via either Rollback() or the existing tx.Commit() on the success path, preventing the described deadlock on subsequent writers.

🤖 Prompt for AI agents

In server/mdm/scep/challenge/bolt/challenge.go around line 56, review and complete this code-review fix: HasChallenge begins a transaction but never rolls back or commits on early error paths.
What the draft fix changed: In `HasChallenge` (server/mdm/scep/challenge/bolt/challenge.go), added `tx.Rollback()` calls on both early-return error paths (when `bkt == nil` and when `bkt.Delete(key)` fails) before returning, ensuring the writable transaction started via `db.Begin(true)` is always closed via either `Rollback()` or the existing `tx.Commit()` on the success path, preventing the described deadlock on subsequent writers.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 34. Typo 'TEST' instead of 'TEXT' in cpe_2 table schema for sw_edition column — server/vulnerabilities/nvd/db.go:44
In applyCPEDatabaseSchema, changed the sw_edition column type from TEST to TEXT in the CREATE TABLE IF NOT EXISTS cpe_2 statement, matching the type affinity of the other TEXT columns in the table as intended.

🤖 Prompt for AI agents

In server/vulnerabilities/nvd/db.go around line 44, review and complete this code-review fix: Typo 'TEST' instead of 'TEXT' in cpe_2 table schema for sw_edition column.
What the draft fix changed: In `applyCPEDatabaseSchema`, changed the `sw_edition` column type from `TEST` to `TEXT` in the `CREATE TABLE IF NOT EXISTS cpe_2` statement, matching the type affinity of the other TEXT columns in the table as intended.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 35. MySQL credentials hardcoded/ignored in setupDS regardless of CLI flags — tools/mdm/assets/main.go:79
In setupDS() (tools/mdm/assets/main.go), changed the sql.Open DSN format arguments from the hardcoded testUsername/testPassword/testAddress constants to the function's userName/password/address parameters, matching the suggested fix so the initial connection respects the -db-user/-db-password/-db-address CLI flags, consistent with the mysql.New() call below it.

🤖 Prompt for AI agents

In tools/mdm/assets/main.go around line 79, review and complete this code-review fix: MySQL credentials hardcoded/ignored in setupDS regardless of CLI flags.
What the draft fix changed: In setupDS() (tools/mdm/assets/main.go), changed the sql.Open DSN format arguments from the hardcoded testUsername/testPassword/testAddress constants to the function's userName/password/address parameters, matching the suggested fix so the initial connection respects the -db-user/-db-password/-db-address CLI flags, consistent with the mysql.New() call below it.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 36. jira.go doWithRetry dereferences resp.StatusCode without checking for a nil response — server/service/externalsvc/jira.go:108
In doWithRetry (server/service/externalsvc/jira.go), added a resp == nil check immediately after the net.Error check and before the resp.StatusCode >= http.StatusInternalServerError comparison. When resp is nil, the error is now wrapped with backoff.Permanent(err) and returned, avoiding the nil-pointer dereference while still surfacing the original error to the caller.

🤖 Prompt for AI agents

In server/service/externalsvc/jira.go around line 108, review and complete this code-review fix: jira.go doWithRetry dereferences resp.StatusCode without checking for a nil response.
What the draft fix changed: In `doWithRetry` (server/service/externalsvc/jira.go), added a `resp == nil` check immediately after the `net.Error` check and before the `resp.StatusCode >= http.StatusInternalServerError` comparison. When `resp` is nil, the error is now wrapped with `backoff.Permanent(err)` and returned, avoiding the nil-pointer dereference while still surfacing the original error to the caller.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 37. Shared loop variable err captured by reference across concurrent goroutines in depsyncer main.go — server/mdm/nanodep/cmd/depsyncer/main.go:178
In the per-DEP-name goroutine started inside the for _, name := range flag.Args()[0:] loop in main(), replaced the shared outer-scope err = syncer.Run(ctx) assignment with a goroutine-local runErr := syncer.Run(ctx), and updated the subsequent if err != nil / logging call to use runErr instead of err. This eliminates the concurrent write race on the shared err variable across multiple syncer goroutines.

🤖 Prompt for AI agents

In server/mdm/nanodep/cmd/depsyncer/main.go around line 178, review and complete this code-review fix: Shared loop variable `err` captured by reference across concurrent goroutines in depsyncer main.go.
What the draft fix changed: In the per-DEP-name goroutine started inside the `for _, name := range flag.Args()[0:]` loop in `main()`, replaced the shared outer-scope `err = syncer.Run(ctx)` assignment with a goroutine-local `runErr := syncer.Run(ctx)`, and updated the subsequent `if err != nil` / logging call to use `runErr` instead of `err`. This eliminates the concurrent write race on the shared `err` variable across multiple syncer goroutines.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 38. get-one-compliance-status-result.js throws/returns raw Error objects instead of using Sails exit conventions — website/api/controllers/microsoft-proxy/get-one-compliance-status-result.js:32
In fn, replaced return new Error({error: 'No MicrosoftComplianceTenant record was found...'}); with throw 'tenantNotFound'; so the declared tenantNotFound exit (mapped to responseType: 'unauthorized') is actually triggered instead of returning an Error object as a 200 success response, exactly as suggested in the finding.

🤖 Prompt for AI agents

In website/api/controllers/microsoft-proxy/get-one-compliance-status-result.js around line 32, review and complete this code-review fix: get-one-compliance-status-result.js throws/returns raw Error objects instead of using Sails exit conventions.
What the draft fix changed: In `fn`, replaced `return new Error({error: 'No MicrosoftComplianceTenant record was found...'});` with `throw 'tenantNotFound';` so the declared `tenantNotFound` exit (mapped to `responseType: 'unauthorized'`) is actually triggered instead of returning an Error object as a 200 success response, exactly as suggested in the finding.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 39. validateWindowLength returns true (valid) for invalid/too-short windows unless end < start — frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditAutoUpdateConfigModal/helpers.tsx:47
Fixed validateWindowLength in helpers.tsx by computing an overnight-adjusted duration (using 24 * 60 - startTotalMinutes + endTotalMinutes when the end time wraps past midnight, otherwise the simple difference) and returning duration >= 60, replacing the previous logic that unconditionally treated any wrap-around window as valid regardless of actual length.

🤖 Prompt for AI agents

In frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditAutoUpdateConfigModal/helpers.tsx around line 47, review and complete this code-review fix: validateWindowLength returns true (valid) for invalid/too-short windows unless end < start.
What the draft fix changed: Fixed `validateWindowLength` in helpers.tsx by computing an overnight-adjusted `duration` (using `24 * 60 - startTotalMinutes + endTotalMinutes` when the end time wraps past midnight, otherwise the simple difference) and returning `duration >= 60`, replacing the previous logic that unconditionally treated any wrap-around window as valid regardless of actual length.
Verify the change is correct and complete; do not refactor unrelated code.

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

🔴 40. StorePushCert UPDATE clause references wrong table name push_certs instead of nano_push_certs — server/mdm/nanomdm/storage/mysql/pushcert.go:43
In StorePushCert, changed the ON DUPLICATE KEY UPDATE clause from push_certs.stale_token = push_certs.stale_token + 1 to stale_token = nano_push_certs.stale_token + 1, referencing the correct table name nano_push_certs (which matches the INSERT INTO nano_push_certs target) instead of the nonexistent push_certs alias/table, matching the suggested fix exactly.

🤖 Prompt for AI agents

In server/mdm/nanomdm/storage/mysql/pushcert.go around line 43, review and complete t