fix(adhoc-sweep-fixes): 22 review findings across 22 files - #124
fix(adhoc-sweep-fixes): 22 review findings across 22 files#124flamingo[bot] wants to merge 22 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -51,7 +51,10 @@ func loadOrMakeCSR(path string, opts *csrOptions) (*x509.CertificateRequest, err | |||
| template.ChallengePassword = opts.challenge | |||
There was a problem hiding this comment.
🦩 🔵 loadOrMakeCSR discards the error from x509util.CreateCertificateRequest
In loadOrMakeCSR, changed derBytes, _ := x509util.CreateCertificateRequest(...) to derBytes, err := x509util.CreateCertificateRequest(...) followed by an if err != nil { return nil, err } check, matching the existing err variable already in scope from os.OpenFile. This surfaces the real error instead of discarding it and letting downstream pem.Encode/ParseCertificateRequest calls fail confusingly.
🤖 Prompt for AI agents
In server/mdm/scep/cmd/scepclient/csr.go around line 51, review and complete this code-review fix: loadOrMakeCSR discards the error from x509util.CreateCertificateRequest.
What the draft fix changed: In loadOrMakeCSR, changed `derBytes, _ := x509util.CreateCertificateRequest(...)` to `derBytes, err := x509util.CreateCertificateRequest(...)` followed by an `if err != nil { return nil, err }` check, matching the existing `err` variable already in scope from `os.OpenFile`. This surfaces the real error instead of discarding it and letting downstream pem.Encode/ParseCertificateRequest calls fail confusingly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| flags+="--disable-open-folder" | ||
|
|
||
| #Read flags | ||
| while getopts s:p:u:f:d:o:x flag |
There was a problem hiding this comment.
🦩 🔵 team-builder script has duplicate -f flag mapped to two different purposes
In run(), resolved the duplicate -f flag collision in the getopts spec s:p:u:f:d:o:x by renaming the source-file-path option to -s and the additional-flags option to -a, updating the spec string to s:p:u:f:a:d:o:x and the corresponding case blocks (s) for the team-names file, a) for additional fleetctl package flags), so both options are now independently reachable. Risk: this changes the public CLI interface (flag letters) — any existing callers/docs invoking -f for the source file path will break and need to be updated to -s; a complete fix should also update any README/usage documentation referencing these flags, which is outside this file.
🤖 Prompt for AI agents
In tools/team-builder/build_teams.sh around line 12, review and complete this code-review fix: team-builder script has duplicate -f flag mapped to two different purposes.
What the draft fix changed: In `run()`, resolved the duplicate `-f` flag collision in the getopts spec `s:p:u:f:d:o:x` by renaming the source-file-path option to `-s` and the additional-flags option to `-a`, updating the spec string to `s:p:u:f:a:d:o:x` and the corresponding `case` blocks (`s)` for the team-names file, `a)` for additional `fleetctl package` flags), so both options are now independently reachable. Risk: this changes the public CLI interface (flag letters) — any existing callers/docs invoking `-f` for the source file path will break and need to be updated to `-s`; a complete fix should also update any README/usage documentation referencing these flags, which is outside this file.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| ) | ||
|
|
||
| func init() { | ||
| MigrationClient.AddMigration(Up_20220510110838, Down_20220510110838) | ||
| MigrationClient.AddMigration(Up_20220526123328, Down_20220526123328) | ||
| } | ||
|
|
||
| func Up_20220510110838(tx *sql.Tx) error { | ||
| func Up_20220526123328(tx *sql.Tx) error { | ||
| // Idempotent migration. | ||
| if !indexExistsTx(tx, "hosts", "hosts_platform_idx") { | ||
| stm := "CREATE INDEX hosts_platform_idx ON hosts (platform);" |
There was a problem hiding this comment.
🦩 🔵 Duplicate migration timestamp/function name mismatch in AddIdxOnHostsPlatform.go
In server/datastore/mysql/migrations/tables/20220526123328_AddIdxOnHostsPlatform.go, renamed Up_20220510110838/Down_20220510110838 to Up_20220526123328/Down_20220526123328 (function definitions and the MigrationClient.AddMigration call in init()) so the registered migration version string matches the filename's timestamp, eliminating the mismatch and potential collision with another migration named 20220510110838.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20220526123328_AddIdxOnHostsPlatform.go around line 1, review and complete this code-review fix: Duplicate migration timestamp/function name mismatch in AddIdxOnHostsPlatform.go.
What the draft fix changed: In `server/datastore/mysql/migrations/tables/20220526123328_AddIdxOnHostsPlatform.go`, renamed `Up_20220510110838`/`Down_20220510110838` to `Up_20220526123328`/`Down_20220526123328` (function definitions and the `MigrationClient.AddMigration` call in `init()`) so the registered migration version string matches the filename's timestamp, eliminating the mismatch and potential collision with another migration named `20220510110838`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| // | ||
| let QUERIES_TO_GET_CRITICAL_SOFTWARE = [ | ||
| {query: 'Safari.app', type: 'safari'},// Safari on macOS | ||
| {query: 'Firefox.app', type: 'firefox'},// Safari on macOS | ||
| {query: 'Google Chrome', type: 'chrome'},// Safari on macOS | ||
| {query: 'Google Chrome.app', type: 'chrome'},// Safari on macOS | ||
| {query: 'Firefox.app', type: 'firefox'},// Firefox on macOS | ||
| {query: 'Google Chrome', type: 'chrome'},// Chrome on Linux/Windows | ||
| {query: 'Google Chrome.app', type: 'chrome'},// Chrome on macOS | ||
| {query: 'Firefox', type: 'firefox'},// Firefox on Linux | ||
| {query: 'Mozilla Firefox', type: 'firefox'},// Firefox on Windows? | ||
| {query: 'Flash player.app', type: 'flash'}, // Flash on macOS |
There was a problem hiding this comment.
🦩 🔵 Duplicate comment labels ('Safari on macOS') mislabel Firefox, Chrome, and Flash query entries
In the QUERIES_TO_GET_CRITICAL_SOFTWARE array inside fn, corrected the trailing comments for the Firefox.app, Google Chrome, and Google Chrome.app entries from the copy-pasted // Safari on macOS to accurate labels: // Firefox on macOS, // Chrome on Linux/Windows, and // Chrome on macOS respectively. No logic, query strings, or types were changed.
🤖 Prompt for AI agents
In ee/vulnerability-dashboard/scripts/update-critical-software.js around line 131, review and complete this code-review fix: Duplicate comment labels ('Safari on macOS') mislabel Firefox, Chrome, and Flash query entries.
What the draft fix changed: In the `QUERIES_TO_GET_CRITICAL_SOFTWARE` array inside `fn`, corrected the trailing comments for the `Firefox.app`, `Google Chrome`, and `Google Chrome.app` entries from the copy-pasted `// Safari on macOS` to accurate labels: `// Firefox on macOS`, `// Chrome on Linux/Windows`, and `// Chrome on macOS` respectively. No logic, query strings, or types were changed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| mdmEnrollmentStatus === "Off" || mdmEnrollmentStatus === null; | ||
| const mdmUnenrolled = isMdmUnenrolled(mdmEnrollmentStatus); | ||
|
|
||
| const mdmEnabledAndConnected = mdmEnabledAndConfigured && connectedToFleetMdm; |
There was a problem hiding this comment.
🦩 🔵 isMdmUnenrolled / isNewMdmEnrollment logic duplicated between DeviceUserBanners and HostDetailsBanners
In DeviceUserBanners, replaced the locally re-implemented isMdmUnenrolled/isNewMdmEnrollment derivations (which used addHours/isPast from date-fns) with calls to shared helpers isMdmUnenrolled and isNewMdmEnrollment imported from pages/hosts/details/HostDetailsPage/helpers, removing the now-unused date-fns import. This change assumes such a helpers module exists (or will be created) exporting these two functions with matching signatures (isMdmUnenrolled(status) and isNewMdmEnrollment(isUnenrolled, lastMdmEnrolledAt)); since I cannot see or create HostDetailsBanners.tsx/a new helpers file in this task (only this one file is in scope), this fix is INCOMPLETE on its own — a companion change is required in HostDetailsBanners.tsx to define/export the helpers and consume them there too, otherwise this file will fail to compile due to a missing module. Risk: build breakage until the helper module is added.
🤖 Prompt for AI agents
In frontend/pages/hosts/details/DeviceUserPage/components/DeviceUserBanners/DeviceUserBanners.tsx around line 42, review and complete this code-review fix: isMdmUnenrolled / isNewMdmEnrollment logic duplicated between DeviceUserBanners and HostDetailsBanners.
What the draft fix changed: In `DeviceUserBanners`, replaced the locally re-implemented `isMdmUnenrolled`/`isNewMdmEnrollment` derivations (which used `addHours`/`isPast` from date-fns) with calls to shared helpers `isMdmUnenrolled` and `isNewMdmEnrollment` imported from `pages/hosts/details/HostDetailsPage/helpers`, removing the now-unused `date-fns` import. This change assumes such a `helpers` module exists (or will be created) exporting these two functions with matching signatures (`isMdmUnenrolled(status)` and `isNewMdmEnrollment(isUnenrolled, lastMdmEnrolledAt)`); since I cannot see or create `HostDetailsBanners.tsx`/a new helpers file in this task (only this one file is in scope), this fix is INCOMPLETE on its own — a companion change is required in `HostDetailsBanners.tsx` to define/export the helpers and consume them there too, otherwise this file will fail to compile due to a missing module. Risk: build breakage until the helper module is added.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 45 low — review closely — react 👍/👎 to teach the reviewer
|
|
||
| // reconcileSnapshot mirrors Datastore.recordSnapshot in | ||
| // server/chart/internal/mysql/data.go. | ||
| // | ||
| // IMPORTANT: this is a standalone-process duplicate of that production SCD | ||
| // reconciliation logic (open-row lookup, close/upsert semantics, batching). | ||
| // There is no shared package or compile-time coupling enforcing consistency | ||
| // between the two implementations. If server/chart/internal/mysql/data.go | ||
| // changes the SCD encoding or reconciliation semantics (e.g. valid_to | ||
| // sentinel handling, batching size/behavior, or the close/upsert decision | ||
| // logic), this function must be updated to match or chart data written by | ||
| // this cron tool will silently drift from what the server itself would | ||
| // produce. Ideally this logic would be extracted into a shared, tested | ||
| // package imported by both server/chart/internal/mysql and this tool. | ||
| func reconcileSnapshot(db *sql.DB, dataset string, entityBitmaps map[string]*roaring.Bitmap, bucketStart time.Time) error { | ||
| rows, err := db.Query( | ||
| `SELECT entity_id, host_bitmap, encoding_type, valid_from |
There was a problem hiding this comment.
🦩 🔵 reconcileSnapshot duplicates production SCD reconciliation logic from server/chart/internal/mysql/data.go with only a code comment linking them
Expanded the doc comment directly above reconcileSnapshot (tools/charts-collect/main.go) to explicitly call out that this is a standalone-process duplicate of the production SCD reconciliation logic, enumerate the specific pieces that must stay in sync (valid_to sentinel handling, batching, close/upsert decision logic), and flag the risk of silent drift, recommending extraction into a shared package. This is a documentation-only change — it does not eliminate the duplication itself, since actually removing it would require creating a shared package importable by both server/chart/internal/mysql and this tool, which is an architectural change spanning files outside this one. A complete fix would extract the shared logic into a common internal package with tests enforcing parity, which cannot be done safely within this single file.
🤖 Prompt for AI agents
In tools/charts-collect/main.go around line 227, review and complete this code-review fix: reconcileSnapshot duplicates production SCD reconciliation logic from server/chart/internal/mysql/data.go with only a code comment linking them.
What the draft fix changed: Expanded the doc comment directly above `reconcileSnapshot` (tools/charts-collect/main.go) to explicitly call out that this is a standalone-process duplicate of the production SCD reconciliation logic, enumerate the specific pieces that must stay in sync (valid_to sentinel handling, batching, close/upsert decision logic), and flag the risk of silent drift, recommending extraction into a shared package. This is a documentation-only change — it does not eliminate the duplication itself, since actually removing it would require creating a shared package importable by both server/chart/internal/mysql and this tool, which is an architectural change spanning files outside this one. A complete fix would extract the shared logic into a common internal package with tests enforcing parity, which cannot be done safely within this single file.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 35 low — review closely — react 👍/👎 to teach the reviewer
| @@ -32,40 +32,6 @@ func newSimpleClient(apiToken string) *simpleClient { | |||
| } | |||
| } | |||
There was a problem hiding this comment.
🦩 🔵 Migration tool ships with a fully commented-out, unfinished serial-lookup implementation
Removed the large commented-out, unfinished getDeviceIDBySerial function (with its stray TODOs) from the file, and replaced the informal "TODO: Use getDeviceIDBySerial..." / "For now, we just use..." comments inside the main function's webhook handler with a single clear NOTE explaining the current functional limitation (hardcoded device ID from --device-id) and what a complete fix would require (parsing the webhook payload's serial and looking up the device ID via the SimpleMDM API). This removes the dead code as flagged while preserving a clear, non-code trace of the known gap for tracking purposes. Risk: this does not implement serial-based lookup (that is a real feature/architectural addition beyond a "minimal fix"); a complete resolution would still require implementing and wiring up the lookup call, which is out of scope for a dead-code removal fix.
🤖 Prompt for AI agents
In tools/mdm/migration/simplemdm/main.go around line 33, review and complete this code-review fix: Migration tool ships with a fully commented-out, unfinished serial-lookup implementation.
What the draft fix changed: Removed the large commented-out, unfinished `getDeviceIDBySerial` function (with its stray TODOs) from the file, and replaced the informal "TODO: Use getDeviceIDBySerial..." / "For now, we just use..." comments inside the `main` function's webhook handler with a single clear NOTE explaining the current functional limitation (hardcoded device ID from `--device-id`) and what a complete fix would require (parsing the webhook payload's serial and looking up the device ID via the SimpleMDM API). This removes the dead code as flagged while preserving a clear, non-code trace of the known gap for tracking purposes. Risk: this does not implement serial-based lookup (that is a real feature/architectural addition beyond a "minimal fix"); a complete resolution would still require implementing and wiring up the lookup call, which is out of scope for a dead-code removal fix.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer
| fmt.Sprintf("Failed to update agent options: %s", err))) | ||
| return | ||
| } | ||
| } else { | ||
| resp.Diagnostics.Append(diag.NewWarningDiagnostic( | ||
| "Empty agent options ignored", | ||
| "agent_options changed to an empty string, but this is not a valid "+ | ||
| "value and the update was skipped. The previous agent options "+ | ||
| "will be retained in state.")) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🔵 Empty agent_options string silently skips the update without informing the user
In Update(), added an else branch to the if ao != "" check that appends a warning diagnostic informing the user that an empty agent_options value was ignored, and added a guard if upTeam == nil { return } after both update blocks to prevent a nil pointer dereference in the subsequent teamModelToTF(ctx, upTeam, &state) call when no actual API update occurred (e.g., only an empty agent_options change was requested with no name/description change). This directly resolves the panic risk described in the finding while also surfacing the silently-skipped update to the user via a warning.
🤖 Prompt for AI agents
In tools/terraform/provider/teams_resource.go around line 222, review and complete this code-review fix: Empty agent_options string silently skips the update without informing the user.
What the draft fix changed: In `Update()`, added an `else` branch to the `if ao != ""` check that appends a warning diagnostic informing the user that an empty `agent_options` value was ignored, and added a guard `if upTeam == nil { return }` after both update blocks to prevent a nil pointer dereference in the subsequent `teamModelToTF(ctx, upTeam, &state)` call when no actual API update occurred (e.g., only an empty agent_options change was requested with no name/description change). This directly resolves the panic risk described in the finding while also surfacing the silently-skipped update to the user via a warning.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
|
|
||
| let informationAboutThisTenant = await MicrosoftComplianceTenant.findOne({id: complianceTenantRecordId}); | ||
| if(!informationAboutThisTenant) { | ||
| return new Error(`No matching tenant record could be found with the specified ID. (${complianceTenantRecordId}`); | ||
| return new Error(`No matching tenant record could be found with the specified ID. (${complianceTenantRecordId})`); | ||
| } | ||
|
|
||
| // Get a graph access token for this tenant |
There was a problem hiding this comment.
🦩 🔵 Unclosed template literal parenthesis in tenant-not-found error message
In fn, fixed the malformed template literal in the tenant-not-found error message by adding the missing closing parenthesis after ${complianceTenantRecordId}, changing (${complianceTenantRecordId} to (${complianceTenantRecordId}).
🤖 Prompt for AI agents
In website/api/helpers/microsoft-proxy/get-access-token-and-api-urls.js around line 27, review and complete this code-review fix: Unclosed template literal parenthesis in tenant-not-found error message.
What the draft fix changed: In `fn`, fixed the malformed template literal in the tenant-not-found error message by adding the missing closing parenthesis after `${complianceTenantRecordId}`, changing `(${complianceTenantRecordId}` to `(${complianceTenantRecordId})`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 98 high — react 👍/👎 to teach the reviewer
| let generatedMySQLRootPassword = await sails.helpers.strings.uuid(); | ||
|
|
||
| let createMySQLResponse = await sails.helpers.http.post.with({ | ||
| // url: 'https://api.render.com/v1/servicess',// Intentionally causing an error to test error handling in this script. | ||
| url: 'https://api.render.com/v1/services', | ||
| data: { | ||
| ownerId: sails.config.custom.renderOwnerId, |
There was a problem hiding this comment.
🦩 🔵 Commented-out 'intentionally causing an error' debug line left in production provisioning script
Removed the commented-out leftover debug line // url: 'https://api.render.com/v1/servicess',// Intentionally causing an error to test error handling in this script. from the MySQL service creation block inside the fn function (within the simultaneouslyForEach(renderInstancesToCreate, ...) callback). The active line below it (url: 'https://api.render.com/v1/services',) was left unchanged, so behavior is unaffected — only the stray comment was deleted.
🤖 Prompt for AI agents
In website/scripts/manage-fleet-premium-trial-instances.js around line 266, review and complete this code-review fix: Commented-out 'intentionally causing an error' debug line left in production provisioning script.
What the draft fix changed: Removed the commented-out leftover debug line `// url: 'https://api.render.com/v1/servicess',// Intentionally causing an error to test error handling in this script.` from the MySQL service creation block inside the `fn` function (within the `simultaneouslyForEach(renderInstancesToCreate, ...)` callback). The active line below it (`url: 'https://api.render.com/v1/services',`) was left unchanged, so behavior is unaffected — only the stray comment was deleted.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
Closes 22 review findings across 22 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
server/mdm/scep/cmd/scepclient/csr.go:51tools/team-builder/build_teams.sh:12server/datastore/mysql/migrations/tables/20220526123328_AddIdxOnHostsPlatform.go:1ee/vulnerability-dashboard/scripts/update-critical-software.js:131frontend/pages/hosts/details/DeviceUserPage/components/DeviceUserBanners/DeviceUserBanners.tsx:42frontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tsx:108handbook/marketing/marketing.rituals.yml:17schema/tables/apfs_volumes.yml:8schema/tables/puppet_state.yml:9server/datastore/mysql/migrations/tables/20241025112748_AddSetupExperienceResultsTable.go:90server/datastore/mysql/migrations/tables/20250213104005_AddAndroidEnterpriseTable.go:18server/datastore/mysql/migrations/tables/20250502222222_AddMdmEnrollTables_test.go:39server/datastore/mysql/migrations/tables/20260528201143_AddMDMAndroidCommands.go:12server/fleet/scripts.go:143server/mdm/nanomdm/docs/openapi.yaml:174server/platform/endpointer/json_key_rewriter.go:107server/service/teams.go:88tools/charts-collect/main.go:227tools/mdm/migration/simplemdm/main.go:33tools/terraform/provider/teams_resource.go:222website/api/helpers/microsoft-proxy/get-access-token-and-api-urls.js:27website/scripts/manage-fleet-premium-trial-instances.js:266What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
f6d23861-693f-45a1-b5b3-570aa3bc9ea7Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.