fix(adhoc-sweep-fixes): 57 review findings across 40 files - #120
fix(adhoc-sweep-fixes): 57 review findings across 40 files#120flamingo[bot] wants to merge 40 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -22,7 +21,7 @@ CREATE TABLE IF NOT EXISTS windows_updates ( | |||
| KEY idx_update_date (host_id, date_epoch) | |||
There was a problem hiding this comment.
🦩 🟠 Wrong table name in error message: 'operating_systems table' referenced while creating windows_updates
In Up_20220831100151, changed the error message text from "create operating_systems table" to "create windows_updates table" to correctly reflect the table being created.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20220831100151_AddWindowsUpdatesTable.go around line 22, review and complete this code-review fix: Wrong table name in error message: 'operating_systems table' referenced while creating windows_updates.
What the draft fix changed: In Up_20220831100151, changed the error message text from "create operating_systems table" to "create windows_updates table" to correctly reflect the table being created.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
|
|
||
| import ( | ||
| "database/sql" | ||
|
|
||
| "github.com/pkg/errors" | ||
| "fmt" | ||
| ) | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
🦩 🟠 errors.Wrapf from pkg/errors used instead of fmt.Errorf(%w) for windows_updates table creation
In Up_20220831100151, replaced errors.Wrapf(err, "...") from github.com/pkg/errors with fmt.Errorf("create windows_updates table: %w", err), and updated the import block to remove github.com/pkg/errors and add fmt, matching the pattern used elsewhere in the migrations package.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20220831100151_AddWindowsUpdatesTable.go around line 14, review and complete this code-review fix: errors.Wrapf from pkg/errors used instead of fmt.Errorf(%w) for windows_updates table creation.
What the draft fix changed: In Up_20220831100151, replaced `errors.Wrapf(err, "...")` from `github.com/pkg/errors` with `fmt.Errorf("create windows_updates table: %w", err)`, and updated the import block to remove `github.com/pkg/errors` and add `fmt`, matching the pattern used elsewhere in the migrations package.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| var args redigo.Args | ||
| args = args.Add(activeQueriesKey) | ||
| args = args.AddFlat(names) | ||
| _, err := conn.Do("SADD", args...) | ||
| return err | ||
| if _, err := conn.Do("SADD", args...); err != nil { | ||
| return fmt.Errorf("sadd query names: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (r *redisLiveQuery) removeQueryInfo(name string) error { |
There was a problem hiding this comment.
🦩 🟠 Cache read inside collectBatchQueriesForHost can race with async loadCache goroutine cleanup
In loadCache(), changed go func() { err = r.removeQueryNames(names...); ... }() to go func() { if err := r.removeQueryNames(names...); err != nil { ... } }(), using a new locally-scoped err instead of closing over the outer function-scoped err variable. This eliminates the data race on the shared err and matches the suggested fix exactly.
🤖 Prompt for AI agents
In server/live_query/redis_live_query.go around line 296, review and complete this code-review fix: Cache read inside collectBatchQueriesForHost can race with async loadCache goroutine cleanup.
What the draft fix changed: In `loadCache()`, changed `go func() { err = r.removeQueryNames(names...); ... }()` to `go func() { if err := r.removeQueryNames(names...); err != nil { ... } }()`, using a new locally-scoped `err` instead of closing over the outer function-scoped `err` variable. This eliminates the data race on the shared `err` and matches the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| var args redigo.Args | ||
| args = args.Add(activeQueriesKey) | ||
| args = args.AddFlat(names) | ||
| _, err := conn.Do("SADD", args...) | ||
| return err | ||
| if _, err := conn.Do("SADD", args...); err != nil { | ||
| return fmt.Errorf("sadd query names: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (r *redisLiveQuery) removeQueryInfo(name string) error { |
There was a problem hiding this comment.
🦩 🟠 removeInactiveQueries and storeQueryNames/removeQueryNames return bare errors without wrapping
Wrapped the bare error returns in storeQueryNames (SADD) and removeQueryNames (SREM) with fmt.Errorf("sadd query names: %w", err) and fmt.Errorf("srem query names: %w", err) respectively, bringing them in line with the file's error-wrapping convention used elsewhere (e.g. storeQueryInfo/removeQueryInfo). Note: the finding also mentions removeInactiveQueries, but that function already wraps its error via ctxerr.Wrap(ctx, err, "remove inactive campaign IDs"), so no change was needed there; only the two functions shown in the evidence lacked wrapping and were fixed.
(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/live_query/redis_live_query.go around line 220, review and complete this code-review fix: removeInactiveQueries and storeQueryNames/removeQueryNames return bare errors without wrapping.
What the draft fix changed: Wrapped the bare error returns in `storeQueryNames` (SADD) and `removeQueryNames` (SREM) with `fmt.Errorf("sadd query names: %w", err)` and `fmt.Errorf("srem query names: %w", err)` respectively, bringing them in line with the file's error-wrapping convention used elsewhere (e.g. storeQueryInfo/removeQueryInfo). Note: the finding also mentions `removeInactiveQueries`, but that function already wraps its error via `ctxerr.Wrap(ctx, err, "remove inactive campaign IDs")`, so no change was needed there; only the two functions shown in the evidence lacked wrapping and were fixed.
_(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
| return fmt.Errorf("call webhook: %w", err) | ||
| } | ||
| defer httpResp.Body.Close() | ||
| if httpResp.StatusCode != http.StatusOK { |
There was a problem hiding this comment.
🦩 🟠 Webhook.CallWebhook does not close the HTTP response body, leaking connections
Added defer httpResp.Body.Close() immediately after the successful w.client.Do(req) call in Webhook.CallWebhook, ensuring the response body is always closed and the underlying connection can be reused/released.
🤖 Prompt for AI agents
In server/mdm/nanodep/cmd/depsyncer/webhook.go around line 70, review and complete this code-review fix: Webhook.CallWebhook does not close the HTTP response body, leaking connections.
What the draft fix changed: Added `defer httpResp.Body.Close()` immediately after the successful `w.client.Do(req)` call in `Webhook.CallWebhook`, ensuring the response body is always closed and the underlying connection can be reused/released.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -20,7 +20,7 @@ module.exports = { | |||
| // Default to 'it-major-mdm' for users with an mdm primaryBuyingSituation. | |||
There was a problem hiding this comment.
🦩 🟠 migrate-old-primary-buying-situation-values.js uses !== {} which is always true and never skips the intended branch
In the fn handler's mdm stream callback (website/scripts/migrate-old-primary-buying-situation-values.js), changed the always-true if(thisUser.getStartedQuestionnaireAnswers !== {}) reference-comparison guard to if(thisUser.getStartedQuestionnaireAnswers && Object.keys(thisUser.getStartedQuestionnaireAnswers).length > 0), matching the suggested fix. This prevents the TypeError from indexing into getStartedQuestionnaireAnswers when it is null/undefined and correctly skips users with an empty answers object.
🤖 Prompt for AI agents
In website/scripts/migrate-old-primary-buying-situation-values.js around line 20, review and complete this code-review fix: migrate-old-primary-buying-situation-values.js uses `!== {}` which is always true and never skips the intended branch.
What the draft fix changed: In the `fn` handler's `mdm` stream callback (`website/scripts/migrate-old-primary-buying-situation-values.js`), changed the always-true `if(thisUser.getStartedQuestionnaireAnswers !== {})` reference-comparison guard to `if(thisUser.getStartedQuestionnaireAnswers && Object.keys(thisUser.getStartedQuestionnaireAnswers).length > 0)`, matching the suggested fix. This prevents the TypeError from indexing into `getStartedQuestionnaireAnswers` when it is null/undefined and correctly skips users with an empty answers object.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| data={(isSoftwareEnabled && software?.software) || []} | ||
| isLoading={isSoftwareFetching} | ||
| pageIndex={softwarePageIndex} | ||
| defaultSortHeader={SOFTWARE_DEFAULT_SORT_DIRECTION} | ||
| defaultSortHeader={SOFTWARE_DEFAULT_SORT_HEADER} | ||
| defaultSortDirection={SOFTWARE_DEFAULT_SORT_DIRECTION} | ||
| resultsTitle="software" | ||
| emptyComponent={() => <EmptySoftwareTable />} |
There was a problem hiding this comment.
🦩 🟠 Software card 'All' tab sorts by hosts_count instead of name despite constant naming
In the 'All' tab's TableContainer (first TabPanel) in the Software component, changed defaultSortHeader={SOFTWARE_DEFAULT_SORT_DIRECTION} to defaultSortHeader={SOFTWARE_DEFAULT_SORT_HEADER}, matching the correct constant used for sort column and consistent with the 'Vulnerable' tab below.
🤖 Prompt for AI agents
In frontend/pages/DashboardPage/cards/Software/Software.tsx around line 96, review and complete this code-review fix: Software card 'All' tab sorts by hosts_count instead of name despite constant naming.
What the draft fix changed: In the 'All' tab's TableContainer (first TabPanel) in the Software component, changed `defaultSortHeader={SOFTWARE_DEFAULT_SORT_DIRECTION}` to `defaultSortHeader={SOFTWARE_DEFAULT_SORT_HEADER}`, matching the correct constant used for sort column and consistent with the 'Vulnerable' tab below.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -4,9 +4,10 @@ import ( | |||
| maintained_apps "github.com/fleetdm/fleet/v4/ee/maintained-apps" | |||
| ) | |||
There was a problem hiding this comment.
🦩 🟠 CiscoJabberVersionTransformer doc comment states wrong version value
Updated the doc comment on CiscoJabberVersionTransformer (line 5) to say "15.2.1" instead of "15.2.0", matching the actual assigned value app.Version = "15.2.1" in the function body. No behavioral code was changed.
🤖 Prompt for AI agents
In ee/maintained-apps/ingesters/homebrew/external_refs/cisco_jabber_version_transformer.go around line 5, review and complete this code-review fix: CiscoJabberVersionTransformer doc comment states wrong version value.
What the draft fix changed: Updated the doc comment on CiscoJabberVersionTransformer (line 5) to say "15.2.1" instead of "15.2.0", matching the actual assigned value `app.Version = "15.2.1"` in the function body. No behavioral code was changed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| func (s *googleCalendarIntegrationTestSuite) SetupSuite() { | ||
| dbFile, err := os.CreateTemp("", "calendar.db") | ||
| s.Require().NoError(err) | ||
| s.dbFile = dbFile | ||
| handler, err := calendartest.Configure(dbFile.Name()) | ||
| s.Require().NoError(err) | ||
| server := httptest.NewUnstartedServer(handler) |
There was a problem hiding this comment.
🦩 🟠 Test suite leaves dbFile nil, TearDownSuite guard is dead code
In SetupSuite, added s.dbFile = dbFile immediately after os.CreateTemp succeeds, so the struct field is populated and TearDownSuite's if s.dbFile != nil guard now correctly closes and removes the temp file on cleanup.
🤖 Prompt for AI agents
In ee/server/calendar/google_calendar_integration_test.go around line 21, review and complete this code-review fix: Test suite leaves dbFile nil, TearDownSuite guard is dead code.
What the draft fix changed: In `SetupSuite`, added `s.dbFile = dbFile` immediately after `os.CreateTemp` succeeds, so the struct field is populated and `TearDownSuite`'s `if s.dbFile != nil` guard now correctly closes and removes the temp file on cleanup.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| await CriticalInstall.update({softwareType: complianceType}).set({isCompliant: false}); | ||
| let numberOfTheseInstalls = await CriticalInstall.count({softwareType: complianceType}); | ||
| newCompliantVersions = await CriticalInstall.update({fleetApid: {in: compliantVersions}}).set({isCompliant: true}).fetch(); | ||
| newPatchProgress = Math.floor(newCompliantInstalls.length /numberOfTheseInstalls * 100); | ||
| newPatchProgress = Math.floor(newCompliantVersions.length /numberOfTheseInstalls * 100); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
🦩 🟠 set-compliant-versions.js divides by newCompliantInstalls.length before it is populated, causing NaN/Infinity progress
In the final else branch of the fn function in set-compliant-versions.js, changed newPatchProgress = Math.floor(newCompliantInstalls.length /numberOfTheseInstalls * 100); to newPatchProgress = Math.floor(newCompliantVersions.length /numberOfTheseInstalls * 100);, using the populated newCompliantVersions array (from .fetch()) instead of the always-empty module-level newCompliantInstalls array, matching the suggested fix exactly.
(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)
🤖 Prompt for AI agents
In ee/vulnerability-dashboard/api/controllers/set-compliant-versions.js around line 52, review and complete this code-review fix: set-compliant-versions.js divides by newCompliantInstalls.length before it is populated, causing NaN/Infinity progress.
What the draft fix changed: In the final `else` branch of the `fn` function in `set-compliant-versions.js`, changed `newPatchProgress = Math.floor(newCompliantInstalls.length /numberOfTheseInstalls * 100);` to `newPatchProgress = Math.floor(newCompliantVersions.length /numberOfTheseInstalls * 100);`, using the populated `newCompliantVersions` array (from `.fetch()`) instead of the always-empty module-level `newCompliantInstalls` array, matching the suggested fix exactly.
_(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
| @@ -22,7 +21,7 @@ CREATE TABLE IF NOT EXISTS windows_updates ( | |||
| KEY idx_update_date (host_id, date_epoch) | |||
There was a problem hiding this comment.
🦩 🟠 Wrong table name in error message: 'operating_systems table' referenced while creating windows_updates
In Up_20220831100151, changed the error message text from "create operating_systems table" to "create windows_updates table" to correctly reflect the table being created.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20220831100151_AddWindowsUpdatesTable.go around line 22, review and complete this code-review fix: Wrong table name in error message: 'operating_systems table' referenced while creating windows_updates.
What the draft fix changed: In Up_20220831100151, changed the error message text from "create operating_systems table" to "create windows_updates table" to correctly reflect the table being created.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
|
|
||
| import ( | ||
| "database/sql" | ||
|
|
||
| "github.com/pkg/errors" | ||
| "fmt" | ||
| ) | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
🦩 🟠 errors.Wrapf from pkg/errors used instead of fmt.Errorf(%w) for windows_updates table creation
In Up_20220831100151, replaced errors.Wrapf(err, "...") from github.com/pkg/errors with fmt.Errorf("create windows_updates table: %w", err), and updated the import block to remove github.com/pkg/errors and add fmt, matching the pattern used elsewhere in the migrations package.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20220831100151_AddWindowsUpdatesTable.go around line 14, review and complete this code-review fix: errors.Wrapf from pkg/errors used instead of fmt.Errorf(%w) for windows_updates table creation.
What the draft fix changed: In Up_20220831100151, replaced `errors.Wrapf(err, "...")` from `github.com/pkg/errors` with `fmt.Errorf("create windows_updates table: %w", err)`, and updated the import block to remove `github.com/pkg/errors` and add `fmt`, matching the pattern used elsewhere in the migrations package.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| var args redigo.Args | ||
| args = args.Add(activeQueriesKey) | ||
| args = args.AddFlat(names) | ||
| _, err := conn.Do("SADD", args...) | ||
| return err | ||
| if _, err := conn.Do("SADD", args...); err != nil { | ||
| return fmt.Errorf("sadd query names: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (r *redisLiveQuery) removeQueryInfo(name string) error { |
There was a problem hiding this comment.
🦩 🟠 Cache read inside collectBatchQueriesForHost can race with async loadCache goroutine cleanup
In loadCache(), changed go func() { err = r.removeQueryNames(names...); ... }() to go func() { if err := r.removeQueryNames(names...); err != nil { ... } }(), using a new locally-scoped err instead of closing over the outer function-scoped err variable. This eliminates the data race on the shared err and matches the suggested fix exactly.
🤖 Prompt for AI agents
In server/live_query/redis_live_query.go around line 296, review and complete this code-review fix: Cache read inside collectBatchQueriesForHost can race with async loadCache goroutine cleanup.
What the draft fix changed: In `loadCache()`, changed `go func() { err = r.removeQueryNames(names...); ... }()` to `go func() { if err := r.removeQueryNames(names...); err != nil { ... } }()`, using a new locally-scoped `err` instead of closing over the outer function-scoped `err` variable. This eliminates the data race on the shared `err` and matches the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| var args redigo.Args | ||
| args = args.Add(activeQueriesKey) | ||
| args = args.AddFlat(names) | ||
| _, err := conn.Do("SADD", args...) | ||
| return err | ||
| if _, err := conn.Do("SADD", args...); err != nil { | ||
| return fmt.Errorf("sadd query names: %w", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (r *redisLiveQuery) removeQueryInfo(name string) error { |
There was a problem hiding this comment.
🦩 🟠 removeInactiveQueries and storeQueryNames/removeQueryNames return bare errors without wrapping
Wrapped the bare error returns in storeQueryNames (SADD) and removeQueryNames (SREM) with fmt.Errorf("sadd query names: %w", err) and fmt.Errorf("srem query names: %w", err) respectively, bringing them in line with the file's error-wrapping convention used elsewhere (e.g. storeQueryInfo/removeQueryInfo). Note: the finding also mentions removeInactiveQueries, but that function already wraps its error via ctxerr.Wrap(ctx, err, "remove inactive campaign IDs"), so no change was needed there; only the two functions shown in the evidence lacked wrapping and were fixed.
(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/live_query/redis_live_query.go around line 220, review and complete this code-review fix: removeInactiveQueries and storeQueryNames/removeQueryNames return bare errors without wrapping.
What the draft fix changed: Wrapped the bare error returns in `storeQueryNames` (SADD) and `removeQueryNames` (SREM) with `fmt.Errorf("sadd query names: %w", err)` and `fmt.Errorf("srem query names: %w", err)` respectively, bringing them in line with the file's error-wrapping convention used elsewhere (e.g. storeQueryInfo/removeQueryInfo). Note: the finding also mentions `removeInactiveQueries`, but that function already wraps its error via `ctxerr.Wrap(ctx, err, "remove inactive campaign IDs")`, so no change was needed there; only the two functions shown in the evidence lacked wrapping and were fixed.
_(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
| return fmt.Errorf("call webhook: %w", err) | ||
| } | ||
| defer httpResp.Body.Close() | ||
| if httpResp.StatusCode != http.StatusOK { |
There was a problem hiding this comment.
🦩 🟠 Webhook.CallWebhook does not close the HTTP response body, leaking connections
Added defer httpResp.Body.Close() immediately after the successful w.client.Do(req) call in Webhook.CallWebhook, ensuring the response body is always closed and the underlying connection can be reused/released.
🤖 Prompt for AI agents
In server/mdm/nanodep/cmd/depsyncer/webhook.go around line 70, review and complete this code-review fix: Webhook.CallWebhook does not close the HTTP response body, leaking connections.
What the draft fix changed: Added `defer httpResp.Body.Close()` immediately after the successful `w.client.Do(req)` call in `Webhook.CallWebhook`, ensuring the response body is always closed and the underlying connection can be reused/released.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -20,7 +20,7 @@ module.exports = { | |||
| // Default to 'it-major-mdm' for users with an mdm primaryBuyingSituation. | |||
There was a problem hiding this comment.
🦩 🟠 migrate-old-primary-buying-situation-values.js uses !== {} which is always true and never skips the intended branch
In the fn handler's mdm stream callback (website/scripts/migrate-old-primary-buying-situation-values.js), changed the always-true if(thisUser.getStartedQuestionnaireAnswers !== {}) reference-comparison guard to if(thisUser.getStartedQuestionnaireAnswers && Object.keys(thisUser.getStartedQuestionnaireAnswers).length > 0), matching the suggested fix. This prevents the TypeError from indexing into getStartedQuestionnaireAnswers when it is null/undefined and correctly skips users with an empty answers object.
🤖 Prompt for AI agents
In website/scripts/migrate-old-primary-buying-situation-values.js around line 20, review and complete this code-review fix: migrate-old-primary-buying-situation-values.js uses `!== {}` which is always true and never skips the intended branch.
What the draft fix changed: In the `fn` handler's `mdm` stream callback (`website/scripts/migrate-old-primary-buying-situation-values.js`), changed the always-true `if(thisUser.getStartedQuestionnaireAnswers !== {})` reference-comparison guard to `if(thisUser.getStartedQuestionnaireAnswers && Object.keys(thisUser.getStartedQuestionnaireAnswers).length > 0)`, matching the suggested fix. This prevents the TypeError from indexing into `getStartedQuestionnaireAnswers` when it is null/undefined and correctly skips users with an empty answers object.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| data={(isSoftwareEnabled && software?.software) || []} | ||
| isLoading={isSoftwareFetching} | ||
| pageIndex={softwarePageIndex} | ||
| defaultSortHeader={SOFTWARE_DEFAULT_SORT_DIRECTION} | ||
| defaultSortHeader={SOFTWARE_DEFAULT_SORT_HEADER} | ||
| defaultSortDirection={SOFTWARE_DEFAULT_SORT_DIRECTION} | ||
| resultsTitle="software" | ||
| emptyComponent={() => <EmptySoftwareTable />} |
There was a problem hiding this comment.
🦩 🟠 Software card 'All' tab sorts by hosts_count instead of name despite constant naming
In the 'All' tab's TableContainer (first TabPanel) in the Software component, changed defaultSortHeader={SOFTWARE_DEFAULT_SORT_DIRECTION} to defaultSortHeader={SOFTWARE_DEFAULT_SORT_HEADER}, matching the correct constant used for sort column and consistent with the 'Vulnerable' tab below.
🤖 Prompt for AI agents
In frontend/pages/DashboardPage/cards/Software/Software.tsx around line 96, review and complete this code-review fix: Software card 'All' tab sorts by hosts_count instead of name despite constant naming.
What the draft fix changed: In the 'All' tab's TableContainer (first TabPanel) in the Software component, changed `defaultSortHeader={SOFTWARE_DEFAULT_SORT_DIRECTION}` to `defaultSortHeader={SOFTWARE_DEFAULT_SORT_HEADER}`, matching the correct constant used for sort column and consistent with the 'Vulnerable' tab below.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -4,9 +4,10 @@ import ( | |||
| maintained_apps "github.com/fleetdm/fleet/v4/ee/maintained-apps" | |||
| ) | |||
There was a problem hiding this comment.
🦩 🟠 CiscoJabberVersionTransformer doc comment states wrong version value
Updated the doc comment on CiscoJabberVersionTransformer (line 5) to say "15.2.1" instead of "15.2.0", matching the actual assigned value app.Version = "15.2.1" in the function body. No behavioral code was changed.
🤖 Prompt for AI agents
In ee/maintained-apps/ingesters/homebrew/external_refs/cisco_jabber_version_transformer.go around line 5, review and complete this code-review fix: CiscoJabberVersionTransformer doc comment states wrong version value.
What the draft fix changed: Updated the doc comment on CiscoJabberVersionTransformer (line 5) to say "15.2.1" instead of "15.2.0", matching the actual assigned value `app.Version = "15.2.1"` in the function body. No behavioral code was changed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| func (s *googleCalendarIntegrationTestSuite) SetupSuite() { | ||
| dbFile, err := os.CreateTemp("", "calendar.db") | ||
| s.Require().NoError(err) | ||
| s.dbFile = dbFile | ||
| handler, err := calendartest.Configure(dbFile.Name()) | ||
| s.Require().NoError(err) | ||
| server := httptest.NewUnstartedServer(handler) |
There was a problem hiding this comment.
🦩 🟠 Test suite leaves dbFile nil, TearDownSuite guard is dead code
In SetupSuite, added s.dbFile = dbFile immediately after os.CreateTemp succeeds, so the struct field is populated and TearDownSuite's if s.dbFile != nil guard now correctly closes and removes the temp file on cleanup.
🤖 Prompt for AI agents
In ee/server/calendar/google_calendar_integration_test.go around line 21, review and complete this code-review fix: Test suite leaves dbFile nil, TearDownSuite guard is dead code.
What the draft fix changed: In `SetupSuite`, added `s.dbFile = dbFile` immediately after `os.CreateTemp` succeeds, so the struct field is populated and `TearDownSuite`'s `if s.dbFile != nil` guard now correctly closes and removes the temp file on cleanup.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| await CriticalInstall.update({softwareType: complianceType}).set({isCompliant: false}); | ||
| let numberOfTheseInstalls = await CriticalInstall.count({softwareType: complianceType}); | ||
| newCompliantVersions = await CriticalInstall.update({fleetApid: {in: compliantVersions}}).set({isCompliant: true}).fetch(); | ||
| newPatchProgress = Math.floor(newCompliantInstalls.length /numberOfTheseInstalls * 100); | ||
| newPatchProgress = Math.floor(newCompliantVersions.length /numberOfTheseInstalls * 100); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
🦩 🟠 set-compliant-versions.js divides by newCompliantInstalls.length before it is populated, causing NaN/Infinity progress
In the final else branch of the fn function in set-compliant-versions.js, changed newPatchProgress = Math.floor(newCompliantInstalls.length /numberOfTheseInstalls * 100); to newPatchProgress = Math.floor(newCompliantVersions.length /numberOfTheseInstalls * 100);, using the populated newCompliantVersions array (from .fetch()) instead of the always-empty module-level newCompliantInstalls array, matching the suggested fix exactly.
(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)
🤖 Prompt for AI agents
In ee/vulnerability-dashboard/api/controllers/set-compliant-versions.js around line 52, review and complete this code-review fix: set-compliant-versions.js divides by newCompliantInstalls.length before it is populated, causing NaN/Infinity progress.
What the draft fix changed: In the final `else` branch of the `fn` function in `set-compliant-versions.js`, changed `newPatchProgress = Math.floor(newCompliantInstalls.length /numberOfTheseInstalls * 100);` to `newPatchProgress = Math.floor(newCompliantVersions.length /numberOfTheseInstalls * 100);`, using the populated `newCompliantVersions` array (from `.fetch()`) instead of the always-empty module-level `newCompliantInstalls` array, matching the suggested fix exactly.
_(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
🦩 What this fix changed, finding by finding57 finding(s) fixed in this draft. (Inline placement was rejected by GitHub for this PR.) 🟠 1. Wrong table name in error message: 'operating_systems table' referenced while creating windows_updates — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 2. errors.Wrapf from pkg/errors used instead of fmt.Errorf(%w) for windows_updates table creation — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 3. Cache read inside collectBatchQueriesForHost can race with async loadCache goroutine cleanup — 🤖 Prompt for AI agentsfix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer 🟠 4. removeInactiveQueries and storeQueryNames/removeQueryNames return bare errors without wrapping — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🟠 5. Webhook.CallWebhook does not close the HTTP response body, leaking connections — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 6. Bare error returns without context wrapping in webhook.go — 🤖 Prompt for AI agentsfix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer 🟠 7. Windows binary name check uses wrong comparison value in fleetctl-npm run.js — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 8. Unreachable/incorrect error message interpolates function reference instead of platform value — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 9. migrate-users-with-no-trial-key.js uses Array.includes on a query result of full record objects, so the membership check never matches — 🤖 Prompt for AI agentsfix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer 🟠 10. migrate-users-with-no-trial-key.js sends a placeholder/wrong subject line 'Whoops' for the trial-license email — 🤖 Prompt for AI agentsfix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer 🟠 11. signingRoundTripper.RoundTrip formats signer error with %#v instead of %w — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 12. Proxy.Serve() wraps a nil-possible error from http.Server.Serve without distinguishing ErrServerClosed — 🤖 Prompt for AI agentsfix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer 🟠 13. Assertions placed outside 🤖 Prompt for AI agentsfix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer 🟠 14. Assertions placed outside 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 15. getInstallErrorMessage produces malformed double-period message for fleetd-installed error — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 16. getUninstallErrorMessage's 'No uninstall script exists' branch also has a doubled-period — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 17. DisassociateAssets missing context parameter breaks caller cancellation/timeout propagation — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🟠 18. DisassociateAssets propagates bare http.NewRequest error without context wrapping in a shared package — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🟠 19. makeAndroidAppUnavailable in software_worker.go swallows detailed context on RemoveAppsFromAndroidPolicy failure — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 20. makeAndroidAppAvailablePerHost hardcodes admin GlobalRole via undefined-looking helper 'new' — 🤖 Prompt for AI agentsfix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer 🟠 21. santa_status.yml has duplicate/swapped descriptions for file_logging hash columns — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 22. santa_status.yml metrics_server column typed as integer but described as an address string — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 23. runMigration calls log.Fatal on migration transaction failure, killing the whole process instead of returning an error — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 24. log.Fatal used inside FinalizeMigration failure path after fn(tx) fails, obscuring the returned error — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 25. Duplicate GenerateSingleSSIDTestWLANXMLProfiles call in TestIsWLANXML produces two identical variables — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 26. TestEqual compares wrong variable pairs for 'hex only' vs 'name only' single-SSID cases — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 27. Check-in handler continues to write response body after error, causing double header write — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 28. CommandAndReportResultsHandler falls through to Write after http.Error on failure — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 29. resolveBranch has unreachable duplicate return statement after the nil-lastErr branch — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🟠 30. Bare error return without wrapping context in resolveBranch — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🟠 31. Test uses non-existent ptr.String(fleet.RoleObserver) call shape inconsistently with rest of file — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 32. TeamID pointer constructed with new(uint(1)) is invalid Go syntax — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 33. server.ListenAndServe error in mdmproxy main() is only printed, not treated as fatal — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🔵 34. Duplicated auth-token validation logic between handleUpdatePercentage and handleUpdateMigrateUDIDs — 🤖 Prompt for AI agentsfix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer 🟠 35. INSUFFICIENT_PERMISSIONS_ERROR constant referenced but not returned in NDESForm getErrorMessage — 🤖 Prompt for AI agentsfix confidence: 🟢 97 high — react 👍/👎 to teach the reviewer 🟠 36. Duplicated useEffect comment header in LivePolicyPage.tsx — 🤖 Prompt for AI agentsfix confidence: 🟢 100 high — react 👍/👎 to teach the reviewer 🟠 37. Invalid Go syntax 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 38. Misspelled field-doc comment 'BlcokSize' in CarveMetadata — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 39. RetrieveCursor stats the wrong file (profileFilename instead of cursorFilename) — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 40. EncodeSCEPRequest POST branch ignores error from http.NewRequest before using rr — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 41. SSOMFAConflict message has a duplicated 'is' typo — 🤖 Prompt for AI agentsfix confidence: 🟢 98 high — react 👍/👎 to teach the reviewer 🟠 42. Test bug: RemediatedBy set on wrong variable (cve3 instead of cve4) — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 43. LoadHostConditionalAccessStatus ignores sql.ErrNoRows path and falls through to use zero-valued struct — 🤖 Prompt for AI agentsfix confidence: 🟢 97 high — react 👍/👎 to teach the reviewer 🟠 44. Migration comment references wrong migration timestamp (copy-paste error) — 🤖 Prompt for AI agentsfix confidence: 🟢 97 high — react 👍/👎 to teach the reviewer 🟠 45. isMac() compares a function reference to a boolean instead of calling isIPad() — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 46. isIosLockedWithLocationAvail compares hostGeolocation to null with strict inequality, which is always true for optional undefined prop — |
Closes 57 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.
server/datastore/mysql/migrations/tables/20220831100151_AddWindowsUpdatesTable.go:22server/datastore/mysql/migrations/tables/20220831100151_AddWindowsUpdatesTable.go:14server/live_query/redis_live_query.go:296server/live_query/redis_live_query.go:220server/mdm/nanodep/cmd/depsyncer/webhook.go:70server/mdm/nanodep/cmd/depsyncer/webhook.go:60tools/fleetctl-npm/run.js:39tools/fleetctl-npm/run.js:27website/scripts/migrate-users-with-no-trial-key.js:18website/scripts/migrate-users-with-no-trial-key.js:56ee/orbit/pkg/httpsigproxy/httpsigproxy.go:210ee/orbit/pkg/httpsigproxy/httpsigproxy.go:138it()block execute at describe-time, not as part of any testfrontend/pages/admin/IntegrationsPage/cards/IdentityProviders/components/EndUserAuthSection/helpers.tests.ts:79it()block in newFormDataIdp describe block never run as part of a testfrontend/pages/admin/IntegrationsPage/cards/IdentityProviders/components/EndUserAuthSection/helpers.tests.ts:231frontend/pages/hosts/details/cards/HostSoftwareLibrary/helpers.tsx:58frontend/pages/hosts/details/cards/HostSoftwareLibrary/helpers.tsx:92server/mdm/apple/vpp/api.go:200server/mdm/apple/vpp/api.go:205server/worker/software_worker.go:279server/worker/software_worker.go:213schema/tables/santa_status.yml:40schema/tables/santa_status.yml:121server/goose/migration.go:46server/goose/migration.go:58server/mdm/microsoft/wlanxml/wlanxml_test.go:82server/mdm/microsoft/wlanxml/wlanxml_test.go:133server/mdm/nanomdm/http/mdm/mdm.go:48server/mdm/nanomdm/http/mdm/mdm.go:82tools/migration-cleanup/main.go:248tools/migration-cleanup/main.go:236server/service/targets_test.go:59server/service/targets_test.go:68tools/mdm/migration/mdmproxy/mdmproxy.go:366tools/mdm/migration/mdmproxy/mdmproxy.go:140frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/NDESForm/helpers.ts:141frontend/pages/policies/live/LivePolicyPage/LivePolicyPage.tsx:78new(time.Unix(1, 0))in host_cache_writes_test.goserver/datastore/mysqlredis/host_cache_writes_test.go:79server/fleet/carves.go:18server/mdm/nanodep/storage/file/file.go:149server/mdm/scep/server/transport.go:107server/service/invites.go:40server/vulnerabilities/msrc/parsed/security_bulletin_test.go:53server/datastore/mysql/conditional_access_microsoft.go:85server/datastore/mysql/migrations/tables/20260528211626_AddClearPasscodeRefToHostMDMActions.go:13frontend/pages/hosts/details/DeviceUserPage/helpers.ts:66frontend/pages/hosts/details/modals/LocationModal/LocationModal.tsx:137frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/AddPatchPolicyModal/AddPatchPolicyModal.tsx:39orbit/pkg/table/mdm_bridge/mdm_bridge.go:178server/datastore/mysql/labels_openframe_test.go:66server/datastore/mysql/migrations/tables/20210819131107_AddCascadeToHostSoftware.go:39server/datastore/mysql/users_test.go:396server/mdm/scep/depot/cacert.go:116!== {}which is always true and never skips the intended branchwebsite/scripts/migrate-old-primary-buying-situation-values.js:20frontend/pages/DashboardPage/cards/Software/Software.tsx:96ee/maintained-apps/ingesters/homebrew/external_refs/cisco_jabber_version_transformer.go:5ee/server/calendar/google_calendar_integration_test.go:21ee/vulnerability-dashboard/api/controllers/set-compliant-versions.js:52What 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.