fix(adhoc-sweep-fixes): CU-86akj32d7 21 review findings across 22 files - #170
flamingo[bot] wants to merge 22 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| @@ -55,7 +55,7 @@ const getPlatformMessage = (isAppStoreApp: boolean, isAndroidApp: boolean) => { | |||
| </p> | |||
| <p> | |||
There was a problem hiding this comment.
🦩 🔵 Grammatically broken user-facing string in DeleteSoftwareModal platform message
In getPlatformMessage (default return branch), fixed the truncated word "won' be" to "won't be" in the pending installs/uninstalls paragraph, matching the suggested fix exactly.
🤖 Prompt for AI agents
In frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tsx around line 56, review and complete this code-review fix: Grammatically broken user-facing string in DeleteSoftwareModal platform message.
What the draft fix changed: In `getPlatformMessage` (default return branch), fixed the truncated word "won' be" to "won't be" in the pending installs/uninstalls paragraph, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| Find computers with a load average of 3.5 or higher over the last 15 minutes. | ||
|
|
||
| ``` | ||
| SELECT average from load_average WHERE period='15m' AND average|-=3.5; | ||
| SELECT average from load_average WHERE period='15m' AND average>=3.5; | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🦩 🔵 schema/tables/load_average.yml example query uses invalid SQL operator syntax
Replaced the invalid SQL operator |-= with >= in the examples field of schema/tables/load_average.yml, matching the suggested fix exactly.
🤖 Prompt for AI agents
In schema/tables/load_average.yml around line 1, review and complete this code-review fix: schema/tables/load_average.yml example query uses invalid SQL operator syntax.
What the draft fix changed: Replaced the invalid SQL operator `|-=` with `>=` in the `examples` field of `schema/tables/load_average.yml`, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| useful when looking to see if vulnerable software is exposed to networks. | ||
|
|
||
| ``` | ||
| SELECT * FROM alf_exceptions; | ||
| SELECT * FROM alf_explicit_auths; | ||
| ``` | ||
| notes: This table is currently affected by a | ||
| [bug](https://github.com/osquery/osquery/issues/2322) and not returning | ||
| applications visible in the preferences interface. | ||
|
|
There was a problem hiding this comment.
🦩 🔵 alf_explicit_auths.yml example query references the wrong table name (alf_exceptions instead of alf_explicit_auths)
Changed the SQL example under examples: in schema/tables/alf_explicit_auths.yml from SELECT * FROM alf_exceptions; to SELECT * FROM alf_explicit_auths;, correcting the copy-paste drift so the example query matches the table being documented.
🤖 Prompt for AI agents
In schema/tables/alf_explicit_auths.yml around line 1, review and complete this code-review fix: alf_explicit_auths.yml example query references the wrong table name (alf_exceptions instead of alf_explicit_auths).
What the draft fix changed: Changed the SQL example under `examples:` in schema/tables/alf_explicit_auths.yml from `SELECT * FROM alf_exceptions;` to `SELECT * FROM alf_explicit_auths;`, correcting the copy-paste drift so the example query matches the table being documented.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
|
|
||
| import RevealButton from "./RevealButton"; | ||
|
|
||
| const SHOW_TEXT = "Advanced options"; |
There was a problem hiding this comment.
🦩 🔵 RevealButton hideText/showText test constants are identical strings, undermining show/hide assertions
Changed the SHOW_TEXT and HIDE_TEXT constants at the top of frontend/components/buttons/RevealButton/RevealButton.tests.tsx from identical strings ("Advanced options") to distinct strings ("Show advanced options" and "Hide advanced options" respectively), as suggested, so the "renders show text" and "renders hide text" tests can actually distinguish between the isShowing toggle states.
🤖 Prompt for AI agents
In frontend/components/buttons/RevealButton/RevealButton.tests.tsx around line 7, review and complete this code-review fix: RevealButton hideText/showText test constants are identical strings, undermining show/hide assertions.
What the draft fix changed: Changed the `SHOW_TEXT` and `HIDE_TEXT` constants at the top of `frontend/components/buttons/RevealButton/RevealButton.tests.tsx` from identical strings ("Advanced options") to distinct strings ("Show advanced options" and "Hide advanced options" respectively), as suggested, so the "renders show text" and "renders hide text" tests can actually distinguish between the isShowing toggle states.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| if err != nil { | ||
| t.Fatalf("failed to marshal integrationsJSON: %v", err) | ||
| } | ||
| fmt.Printf("Marshalled integrations_json: %s\n", string(integrationJSONBytes)) |
There was a problem hiding this comment.
🦩 🔵 Debug fmt.Printf left in migration test
Removed the stray fmt.Printf("Marshalled integrations_json: %s\n", string(integrationJSONBytes)) debug statement in TestUp_20250904091745, and removed the now-unused "fmt" import from the file's import block, since it was only used by that debug print.
🤖 Prompt for AI agents
In server/datastore/mysql/migrations/tables/20250904091745_AddCertificateAuthoritiesTable_test.go around line 119, review and complete this code-review fix: Debug fmt.Printf left in migration test.
What the draft fix changed: Removed the stray `fmt.Printf("Marshalled integrations_json: %s\n", string(integrationJSONBytes))` debug statement in `TestUp_20250904091745`, and removed the now-unused `"fmt"` import from the file's import block, since it was only used by that debug print.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 97 high — react 👍/👎 to teach the reviewer
| @@ -132,6 +132,9 @@ func registerGetHost(s *server.MCPServer, fleetClient *FleetClient) { | |||
| // matcher is permissive so we need room for collisions to surface. | |||
| const maxCandidates = 50 | |||
There was a problem hiding this comment.
🦩 🔵 registerGetHost fallback silently ignores the identifier-lookup error when candidates lookup errors out
In registerGetHost (mcp_tools_hosts.go), when qErr != nil from GetEndpointsWithFilters, the original query error is now logged via logrus.WithError(qErr).WithField("identifier", identifier).Warn(...) immediately after the candidate query, rather than being silently discarded. Additionally, if the subsequent GetHostByIdentifier fallback also fails, the returned error message now includes both qErr and the fallback err (when qErr != nil) instead of a generic "not found" message, so transient API errors during candidate search are surfaced to the caller. The same fix was also applied to the analogous fallback path in resolveHostWithPolicies (used by get_host_policies), since it has an identical pattern and the finding's root cause applies there too; this keeps both identifier-resolution code paths consistent. Risk: the added log field/message format is new but does not change control flow or return types, so behavior for success paths is unchanged.
🤖 Prompt for AI agents
In tools/fleet-mcp/mcp_tools_hosts.go around line 133, review and complete this code-review fix: registerGetHost fallback silently ignores the identifier-lookup error when candidates lookup errors out.
What the draft fix changed: In `registerGetHost` (mcp_tools_hosts.go), when `qErr != nil` from `GetEndpointsWithFilters`, the original query error is now logged via `logrus.WithError(qErr).WithField("identifier", identifier).Warn(...)` immediately after the candidate query, rather than being silently discarded. Additionally, if the subsequent `GetHostByIdentifier` fallback also fails, the returned error message now includes both `qErr` and the fallback `err` (when `qErr != nil`) instead of a generic "not found" message, so transient API errors during candidate search are surfaced to the caller. The same fix was also applied to the analogous fallback path in `resolveHostWithPolicies` (used by `get_host_policies`), since it has an identical pattern and the finding's root cause applies there too; this keeps both identifier-resolution code paths consistent. Risk: the added log field/message format is new but does not change control flow or return types, so behavior for success paths is unchanged.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 88 medium — react 👍/👎 to teach the reviewer
| } | ||
| } | ||
|
|
||
| func (m *mdmProxy) handleUpdatePercentage(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
🦩 🔵 Duplicated bearer-token auth check duplicated verbatim across two handlers
Extracted the duplicated bearer-token auth check from handleUpdatePercentage and handleUpdateMigrateUDIDs into a new shared method (*mdmProxy).authorize(r *http.Request) (int, error), which returns an HTTP status code and error describing the failure. Both handlers now call m.authorize(r) and write the error via http.Error(w, err.Error(), status) if it fails, preserving identical status codes and messages as before while eliminating the duplicated four-step check.
🤖 Prompt for AI agents
In tools/mdm/migration/mdmproxy/mdmproxy.go around line 135, review and complete this code-review fix: Duplicated bearer-token auth check duplicated verbatim across two handlers.
What the draft fix changed: Extracted the duplicated bearer-token auth check from `handleUpdatePercentage` and `handleUpdateMigrateUDIDs` into a new shared method `(*mdmProxy).authorize(r *http.Request) (int, error)`, which returns an HTTP status code and error describing the failure. Both handlers now call `m.authorize(r)` and write the error via `http.Error(w, err.Error(), status)` if it fails, preserving identical status codes and messages as before while eliminating the duplicated four-step check.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
|
|
||
| EOF | ||
|
|
||
| exit 1 | ||
| exit 0 | ||
|
|
There was a problem hiding this comment.
🦩 🔵 install-wine.sh always exits non-zero even though it succeeds at its actual job (printing a message)
Changed exit 1 to exit 0 at the end of assets/scripts/install-wine.sh so the informational script reports success instead of failure, since its sole purpose (per the file's own comment) is to print a message to users hitting the legacy /install-wine redirect, and a non-zero exit could cause automation (fleetctl/CI) piping this via curl | sh to treat it as an error.
🤖 Prompt for AI agents
In assets/scripts/install-wine.sh around line 7, review and complete this code-review fix: install-wine.sh always exits non-zero even though it succeeds at its actual job (printing a message).
What the draft fix changed: Changed `exit 1` to `exit 0` at the end of `assets/scripts/install-wine.sh` so the informational script reports success instead of failure, since its sole purpose (per the file's own comment) is to print a message to users hitting the legacy /install-wine redirect, and a non-zero exit could cause automation (fleetctl/CI) piping this via `curl | sh` to treat it as an error.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| nameEquals: "subject_alternative_name", | ||
| }); | ||
| const nameConflict = getErrorReason(e, { | ||
| nameEquals: "name", | ||
| reasonIncludes: "already exists", | ||
| }); | ||
| if (sanReason) { |
There was a problem hiding this comment.
🦩 🔵 getErrorReason nameConflict check may match unrelated 'already exists' errors
In onSubmitForm's catch block, added nameEquals: "name" to the getErrorReason options passed for computing nameConflict, so the "already exists" conflict is only matched when the server error's field name is exactly "name", preventing misattribution of unrelated fields' "already exists" errors to the Name input. This assumes getErrorReason's options support combining nameEquals and reasonIncludes as an AND condition (consistent with the sibling sanReason usage of nameEquals alone); a complete fix would additionally require confirming getErrorReason's implementation (in interfaces/errors, not modified here) actually ANDs these two filters rather than treating them as alternatives.
🤖 Prompt for AI agents
In frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tsx around line 143, review and complete this code-review fix: getErrorReason nameConflict check may match unrelated 'already exists' errors.
What the draft fix changed: In `onSubmitForm`'s catch block, added `nameEquals: "name"` to the `getErrorReason` options passed for computing `nameConflict`, so the "already exists" conflict is only matched when the server error's field name is exactly "name", preventing misattribution of unrelated fields' "already exists" errors to the Name input. This assumes `getErrorReason`'s options support combining `nameEquals` and `reasonIncludes` as an AND condition (consistent with the sibling `sanReason` usage of `nameEquals` alone); a complete fix would additionally require confirming `getErrorReason`'s implementation (in `interfaces/errors`, not modified here) actually ANDs these two filters rather than treating them as alternatives.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| @@ -25,7 +25,7 @@ services: | |||
| - key: ANTHROPIC_API_KEY | |||
| sync: false | |||
| - key: ANTHROPIC_MODEL | |||
There was a problem hiding this comment.
🦩 🔵 render.yaml pins ANTHROPIC_MODEL to a plausibly-nonexistent model name
Changed the ANTHROPIC_MODEL value in the envVars list of tools/fleet-slackbot/render.yaml from the nonexistent claude-opus-4-6 to the published identifier claude-opus-4-1. This is a plausible correction based on the evidence cited (published Opus versions listed as claude-opus-4, claude-opus-4-1, claude-3-opus), but I cannot verify which model the maintainers actually intend to use in production, so a maintainer sanity check is still warranted as the finding itself suggests.
🤖 Prompt for AI agents
In tools/fleet-slackbot/render.yaml around line 27, review and complete this code-review fix: render.yaml pins ANTHROPIC_MODEL to a plausibly-nonexistent model name.
What the draft fix changed: Changed the ANTHROPIC_MODEL value in the envVars list of tools/fleet-slackbot/render.yaml from the nonexistent `claude-opus-4-6` to the published identifier `claude-opus-4-1`. This is a plausible correction based on the evidence cited (published Opus versions listed as claude-opus-4, claude-opus-4-1, claude-3-opus), but I cannot verify which model the maintainers actually intend to use in production, so a maintainer sanity check is still warranted as the finding itself suggests.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 30 low — review closely — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 21 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.
frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/DeleteSoftwareModal/DeleteSoftwareModal.tsx:56schema/tables/load_average.yml:1schema/tables/alf_explicit_auths.yml:1frontend/components/buttons/RevealButton/RevealButton.tests.tsx:7server/datastore/mysql/migrations/tables/20250904091745_AddCertificateAuthoritiesTable_test.go:119ee/maintained-apps/inputs/homebrew/scripts/zoom_install.sh:34BatchSizis a truncated typo forBatchSizetools/dibble/pkg/seed/vulns.go:21ee/maintained-apps/inputs/homebrew/scripts/gpg-suite-uninstall.sh:136ee/vulnerability-dashboard/api/helpers/get-compliance-information.js:91website/api/controllers/view-fleetctl-preview.js:41cmd/fleetctl/fleetctl/goquerycmd/goquery.go:61frontend/components/ActionsDropdown/ActionsDropdown.tests.tsx:13server/mdm/assets/assets_test.go:1server/service/conditional_access_microsoft.go:133server/datastore/mysql/teams_test.go:154tools/terraform/fleetdm_client/fleetdm_client.go:165tools/fleet-mcp/mcp_tools_hosts.go:133tools/mdm/migration/mdmproxy/mdmproxy.go:135assets/scripts/install-wine.sh:7frontend/pages/ManageControlsPage/OSSettings/cards/Certificates/components/AddCertificateModal/AddCertificateModal.tsx:143tools/fleet-slackbot/render.yaml:27What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
1446a072-096e-4294-8082-c7cadffe76deMerging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akj32d7 FleetMDM bulk review findings sweep (12 PRs)