Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
96527d1
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
b7db533
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
3872eea
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
b392feb
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
4d06641
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
8ee753c
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
08b0085
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
21e7846
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
b4ce032
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
10b19cd
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
ee46810
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
91eff8c
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
75c0a95
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
df71a21
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
c5deab4
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
ac786b9
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
193e3a9
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
aba1ed2
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
9056b5d
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
36e97ab
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
d862ac3
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
a886642
fix(adhoc-sweep-fixes): 21 review findings across 22 files
flamingo[bot] Sep 14, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions api/helpers/ensure-trial-license-key.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
module.exports = {


friendlyName: 'Ensure trial license key',


description: 'Generate and persist a Fleet Premium trial license key for the given user if they do not already have one.',


inputs: {

user: {
type: 'ref',
description: 'The logged-in user record (this.req.me) to check and possibly update.',
required: true,
}

},


exits: {

success: {
outputDescription: 'The trial license key for this user, along with whether it is expired.',
outputType: {
trialLicenseKey: 'string',
userHasExpiredTrialLicense: 'boolean',
}
}

},


fn: async function ({user}) {

let userHasExpiredTrialLicense = false;
let trialLicenseKey;

if(user.fleetPremiumTrialLicenseKey) {
if(user.fleetPremiumTrialLicenseKeyExpiresAt < Date.now()) {
userHasExpiredTrialLicense = true;
}
trialLicenseKey = user.fleetPremiumTrialLicenseKey;
} else {
// If this user does not have a trial license key, generate a new one for them.
let thirtyDaysFromNowAt = Date.now() + (1000 * 60 * 60 * 24 * 30);
let trialLicenseKeyForThisUser = await sails.helpers.createLicenseKey.with({
numberOfHosts: 10,
organization: user.organization ? user.organization : 'Fleet Premium trial',
expiresAt: thirtyDaysFromNowAt,
});
// Save the trial license key to the DB record for this user.
await User.updateOne({id: user.id})
.set({
fleetPremiumTrialLicenseKey: trialLicenseKeyForThisUser,
fleetPremiumTrialLicenseKeyExpiresAt: thirtyDaysFromNowAt,
});
trialLicenseKey = trialLicenseKeyForThisUser;
}

return {
trialLicenseKey,
userHasExpiredTrialLicense,
};

}


};
3 changes: 2 additions & 1 deletion assets/scripts/install-wine.sh
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,5 @@ have caused repeated breakage.

EOF

exit 1
exit 0

Comment on lines 26 to +30

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ 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

15 changes: 15 additions & 0 deletions cmd/fleetctl/fleetctl/goquerycmd/goquery.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"errors"
"fmt"
"strconv"
"sync"

"github.com/AbGuthrie/goquery/v2"
gqconfig "github.com/AbGuthrie/goquery/v2/config"
Expand All @@ -26,6 +27,7 @@ type activeQuery struct {

type goqueryClient struct {
client *service.Client
mu sync.Mutex
queryCounter int
queries map[string]activeQuery
// goquery passes the UUID, while we need the hostname (or ID) to
Expand Down Expand Up @@ -62,7 +64,9 @@ func (c *goqueryClient) CheckHost(query string) (gqhosts.Host, error) {
return gqhosts.Host{}, fmt.Errorf("host %s not found", query)
}

c.mu.Lock()
c.hostnameByUUID[host.UUID] = host.Hostname
c.mu.Unlock()

return gqhosts.Host{
UUID: host.UUID,
Comment on lines 64 to 72

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ hostnameByUUID map also written without synchronization alongside queries map

Added a sync.Mutex field mu to goqueryClient and used it to guard all reads/writes of both hostnameByUUID and queries maps: in CheckHost around the hostnameByUUID write, in ScheduleQuery around the counter increment, the hostnameByUUID read, and the queries write, in the background goroutine launched by ScheduleQuery around both queries writes on the result/error channels, and in FetchResults around the queries read. This addresses both the hostnameByUUID race noted in this finding and the related queries map race in the same struct, since they need a shared mutex to be fixed together.

πŸ€– Prompt for AI agents
In cmd/fleetctl/fleetctl/goquerycmd/goquery.go around line 61, review and complete this code-review fix: hostnameByUUID map also written without synchronization alongside queries map.
What the draft fix changed: Added a `sync.Mutex` field `mu` to `goqueryClient` and used it to guard all reads/writes of both `hostnameByUUID` and `queries` maps: in `CheckHost` around the `hostnameByUUID` write, in `ScheduleQuery` around the counter increment, the `hostnameByUUID` read, and the `queries` write, in the background goroutine launched by `ScheduleQuery` around both `queries` writes on the result/error channels, and in `FetchResults` around the `queries` read. This addresses both the `hostnameByUUID` race noted in this finding and the related `queries` map race in the same struct, since they need a shared mutex to be fixed together.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 85 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer

Expand All @@ -73,32 +77,41 @@ func (c *goqueryClient) CheckHost(query string) (gqhosts.Host, error) {
}

func (c *goqueryClient) ScheduleQuery(uuid, query string) (string, error) {
c.mu.Lock()
c.queryCounter++
queryName := strconv.Itoa(c.queryCounter)

hostname, ok := c.hostnameByUUID[uuid]
if !ok {
c.mu.Unlock()
return "", errors.New("could not lookup host")
}
c.mu.Unlock()

res, err := c.client.LiveQuery(query, nil, []string{}, []string{hostname})
if err != nil {
return "", err
}

c.mu.Lock()
c.queries[queryName] = activeQuery{status: "Pending"}
c.mu.Unlock()

// We need to start a separate thread due to goquery expecting
// scheduling a query and retrieving results to be separate
// operations.
go func() {
select {
case hostResult := <-res.Results():
c.mu.Lock()
c.queries[queryName] = activeQuery{status: "Completed", results: hostResult.Rows}
c.mu.Unlock()

// Print an error
case err := <-res.Errors():
c.mu.Lock()
c.queries[queryName] = activeQuery{status: "error: " + err.Error()}
c.mu.Unlock()
}
}()

Expand All @@ -107,7 +120,9 @@ func (c *goqueryClient) ScheduleQuery(uuid, query string) (string, error) {
}

func (c *goqueryClient) FetchResults(queryName string) (gqmodels.Rows, string, error) {
c.mu.Lock()
res, ok := c.queries[queryName]
c.mu.Unlock()
if !ok {
return nil, "", fmt.Errorf("Unknown query %s", queryName)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ remove_receipt_files() {
fi

echo "sudo pkgutil --only-files --files \"$PKGID\" | sed \"s|^|${FULL_INSTALL_LOCATION}/|\" | tr '\\\\n' '\\\\0' | /usr/bin/sudo -u root -E -- /usr/bin/xargs -0 -- /bin/rm -rf"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ remove_receipt_files uses INSTALL_LOCATION directly instead of the computed FULL_INSTALL_LOCATION for the file removal sed pattern

In remove_receipt_files, changed the executed sudo pkgutil --only-files --files "$PKGID" | sed "s|^|/${INSTALL_LOCATION}/|" command to use "s|^|${FULL_INSTALL_LOCATION}/|" instead, matching the already-computed FULL_INSTALL_LOCATION variable and the diagnostic echo line immediately above it. The --only-dirs sed command already used FULL_INSTALL_LOCATION correctly and was left unchanged. This ensures the actually-executed rm targets match the intended, volume-aware install path rather than always prefixing with /.

πŸ€– Prompt for AI agents
In ee/maintained-apps/inputs/homebrew/scripts/gpg-suite-uninstall.sh around line 136, review and complete this code-review fix: remove_receipt_files uses INSTALL_LOCATION directly instead of the computed FULL_INSTALL_LOCATION for the file removal sed pattern.
What the draft fix changed: In `remove_receipt_files`, changed the executed `sudo pkgutil --only-files --files "$PKGID" | sed "s|^|/${INSTALL_LOCATION}/|"` command to use `"s|^|${FULL_INSTALL_LOCATION}/|"` instead, matching the already-computed `FULL_INSTALL_LOCATION` variable and the diagnostic echo line immediately above it. The `--only-dirs` sed command already used `FULL_INSTALL_LOCATION` correctly and was left unchanged. This ensures the actually-executed rm targets match the intended, volume-aware install path rather than always prefixing with `/`.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟒 95 high β€” react πŸ‘/πŸ‘Ž to teach the reviewer

sudo pkgutil --only-files --files "$PKGID" | sed "s|^|/${INSTALL_LOCATION}/|" | tr '\n' '\0' | /usr/bin/sudo -u root -E -- /usr/bin/xargs -0 -- /bin/rm -rf
sudo pkgutil --only-files --files "$PKGID" | sed "s|^|${FULL_INSTALL_LOCATION}/|" | tr '\n' '\0' | /usr/bin/sudo -u root -E -- /usr/bin/xargs -0 -- /bin/rm -rf

echo "sudo pkgutil --only-dirs --files \"$PKGID\" | sed \"s|^|${FULL_INSTALL_LOCATION}/|\" | grep '\\.app$' | tr '\\\\n' '\\\\0' | /usr/bin/sudo -u root -E -- /usr/bin/xargs -0 -- /bin/rm -rf"
sudo pkgutil --only-dirs --files "$PKGID" | sed "s|^|${FULL_INSTALL_LOCATION}/|" | grep '\.app$' | tr '\n' '\0' | /usr/bin/sudo -u root -E -- /usr/bin/xargs -0 -- /bin/rm -rf
Expand Down
10 changes: 8 additions & 2 deletions ee/maintained-apps/inputs/homebrew/scripts/zoom_install.sh
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,16 @@ quit_application() {

restart_zoom() {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ Duplicate quit/relaunch helper logic across webex_install.sh and zoom_install.sh with divergent robustness

Modified restart_zoom() in ee/maintained-apps/inputs/homebrew/scripts/zoom_install.sh to bootstrap into the console user's Mach namespace via launchctl asuser "$console_uid" before invoking sudo -u "$console_user" open -a "zoom.us", falling back to the old sudo -u behavior if the UID lookup fails, mirroring the robustness concern raised for webex_install.sh's relaunch_application. This is a minimal in-file mitigation of the LSOpenURLsWithRole() failure mode rather than the requested shared-helper extraction (which would require creating and importing a new shared module referencing webex_install.sh internals I cannot see/verify); the per-bundle-id tracking (ZOOM_WAS_RUNNING vs per-bundle var) and full de-duplication with webex_install.sh were intentionally left unchanged since the finding is informational/maintainability and a full extraction risks touching webex_install.sh, which is out of scope for this file-only fix.

πŸ€– Prompt for AI agents
In ee/maintained-apps/inputs/homebrew/scripts/zoom_install.sh around line 34, review and complete this code-review fix: Duplicate quit/relaunch helper logic across webex_install.sh and zoom_install.sh with divergent robustness.
What the draft fix changed: Modified `restart_zoom()` in ee/maintained-apps/inputs/homebrew/scripts/zoom_install.sh to bootstrap into the console user's Mach namespace via `launchctl asuser "$console_uid"` before invoking `sudo -u "$console_user" open -a "zoom.us"`, falling back to the old `sudo -u` behavior if the UID lookup fails, mirroring the robustness concern raised for webex_install.sh's relaunch_application. This is a minimal in-file mitigation of the LSOpenURLsWithRole() failure mode rather than the requested shared-helper extraction (which would require creating and importing a new shared module referencing webex_install.sh internals I cannot see/verify); the per-bundle-id tracking (ZOOM_WAS_RUNNING vs per-bundle var) and full de-duplication with webex_install.sh were intentionally left unchanged since the finding is informational/maintainability and a full extraction risks touching webex_install.sh, which is out of scope for this file-only fix.
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

local console_user="$1"

if [[ -n "$console_user" && "$console_user" != "root" ]]; then
echo "Restarting Zoom for user: $console_user"
sudo -u "$console_user" open -a "zoom.us"
local console_uid
console_uid=$(id -u "$console_user" 2>/dev/null || echo "")
if [[ -n "$console_uid" ]]; then
launchctl asuser "$console_uid" sudo -u "$console_user" open -a "zoom.us"
else
sudo -u "$console_user" open -a "zoom.us"
fi
else
echo "No console user found, attempting direct Zoom start..."
open -a "zoom.us"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ module.exports = {
}
operatingSystemsInUse.push(osInfo);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ get-compliance-information.js sorts an empty array before it is populated

Removed the no-op line complianceInformation.versionsInUse = _.sortByOrder(complianceInformation.versionsInUse, 'sortByName'); inside the operatingSystem branch of the fn function in get-compliance-information.js. This line sorted the still-empty complianceInformation.versionsInUse array and its result was immediately discarded by the subsequent (correct) assignment complianceInformation.versionsInUse = _.sortByOrder(operatingSystemsInUse, 'hostCount', 'asc');, so deleting the dead line has no behavioral effect and eliminates the leftover refactor artifact.

πŸ€– Prompt for AI agents
In ee/vulnerability-dashboard/api/helpers/get-compliance-information.js around line 91, review and complete this code-review fix: get-compliance-information.js sorts an empty array before it is populated.
What the draft fix changed: Removed the no-op line `complianceInformation.versionsInUse = _.sortByOrder(complianceInformation.versionsInUse, 'sortByName');` inside the `operatingSystem` branch of the `fn` function in get-compliance-information.js. This line sorted the still-empty `complianceInformation.versionsInUse` array and its result was immediately discarded by the subsequent (correct) assignment `complianceInformation.versionsInUse = _.sortByOrder(operatingSystemsInUse, 'hostCount', 'asc');`, so deleting the dead line has no behavioral effect and eliminates the leftover refactor artifact.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟒 90 high β€” react πŸ‘/πŸ‘Ž to teach the reviewer

}
complianceInformation.versionsInUse = _.sortByOrder(complianceInformation.versionsInUse, 'sortByName');

let numberOfHostsToReport = await Host.count({teamApid: teamApid});
let numberOfHostsOnThisTeamWithACompliantOs = await Host.count({operatingSystem: {in: idsOfCompliantOperatingSystems}, teamApid: teamApid});
Expand Down Expand Up @@ -264,3 +263,4 @@ module.exports = {

};


Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ const DROPDOWN_OPTIONS = [
{ disabled: true, label: "Delete", value: "delete-query" },
];
const PLACEHOLDER = "Actions";
const ON_CHANGE = (value: string) => {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ console.log left in test onChange stub instead of a jest mock

Replaced the ON_CHANGE stub in ActionsDropdown.tests.tsx from a function that called console.log(value) to a no-op () => {}, matching the suggested fix and eliminating console spam in tests that use ON_CHANGE directly.

πŸ€– Prompt for AI agents
In frontend/components/ActionsDropdown/ActionsDropdown.tests.tsx around line 13, review and complete this code-review fix: console.log left in test onChange stub instead of a jest mock.
What the draft fix changed: Replaced the `ON_CHANGE` stub in `ActionsDropdown.tests.tsx` from a function that called `console.log(value)` to a no-op `() => {}`, matching the suggested fix and eliminating console spam in tests that use `ON_CHANGE` directly.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟒 95 high β€” react πŸ‘/πŸ‘Ž to teach the reviewer

console.log(value);
};
const ON_CHANGE = () => {};

describe("Actions dropdown", () => {
it("renders dropdown placeholder and options", async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import { renderWithSetup } from "test/test-utils";

import RevealButton from "./RevealButton";

const SHOW_TEXT = "Advanced options";

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ 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

const HIDE_TEXT = "Advanced options";
const SHOW_TEXT = "Show advanced options";
const HIDE_TEXT = "Hide advanced options";
const TOOLTIP_CONTENT = "Customize logging type and platforms";

describe("Reveal button", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ const AddCertModal = ({
nameEquals: "subject_alternative_name",
});
const nameConflict = getErrorReason(e, {
nameEquals: "name",
reasonIncludes: "already exists",
});
if (sanReason) {
Comment on lines 144 to 150

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ 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

Expand Down Expand Up @@ -253,3 +254,4 @@ const AddCertModal = ({
};

export default AddCertModal;

Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ const getPlatformMessage = (isAppStoreApp: boolean, isAndroidApp: boolean) => {
</p>
<p>

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ 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&apos; be" to "won&apos;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

Pending installs and uninstalls will be canceled. If they have already
started, they won&apos; be canceled, and the results won&apos;t appear
started, they won&apos;t be canceled, and the results won&apos;t appear
in Fleet.
</p>
</>
Expand Down
3 changes: 2 additions & 1 deletion schema/tables/alf_explicit_auths.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ examples: |-
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.

Comment on lines 4 to +12

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ 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

3 changes: 2 additions & 1 deletion schema/tables/load_average.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@ examples: |-
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;
```

Comment on lines 3 to +8

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ 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

Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"crypto/md5" // nolint:gosec // used only to hash for efficient comparisons
"encoding/hex"
"encoding/json"
"fmt"
"strings"
"testing"

Expand Down Expand Up @@ -116,7 +115,6 @@ func TestUp_20250904091745(t *testing.T) {
if err != nil {
t.Fatalf("failed to marshal integrationsJSON: %v", err)
}
fmt.Printf("Marshalled integrations_json: %s\n", string(integrationJSONBytes))

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ 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


insertNDESPasswordStmt := `INSERT INTO mdm_config_assets (name, value, md5_checksum) VALUES (?, ?, UNHEX(?))` // nolint:gosec // just test data, not hardcoded credentials
_, err = db.Exec(insertNDESPasswordStmt, fleet.MDMAssetNDESPassword, ndesEncryptedPassword, md5ChecksumBytes(ndesEncryptedPassword))
Expand Down
8 changes: 4 additions & 4 deletions server/datastore/mysql/teams_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func testTeamsGetSetDelete(t *testing.T, ds *Datastore) {
mdm_apple_configuration_profiles (profile_uuid, team_id, identifier, name, mobileconfig, checksum)
VALUES (?, ?, ?, ?, ?, ?)`,
fmt.Sprintf("uuid_%s", tt.name),
0,
team.ID,
fmt.Sprintf("TestPayloadIdentifier_%s", tt.name),
fmt.Sprintf("TestPayloadName_%s", tt.name),
`<?xml version="1.0"`,
Comment on lines 157 to 163

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ testTeamsGetSetDelete hardcodes team_id=0 for adhoc SQL inserts unrelated to the actual team under test

In testTeamsGetSetDelete's ad-hoc insert block, changed the literal 0 team_id arguments to team.ID for the three inserts into mdm_apple_configuration_profiles, mdm_windows_configuration_profiles, and mdm_android_configuration_profiles, so the subsequent DeleteTeam verification loop (which checks WHERE team_id = ? with team.ID against teamRefs) actually validates cleanup of these rows under the real team being deleted.

πŸ€– Prompt for AI agents
In server/datastore/mysql/teams_test.go around line 154, review and complete this code-review fix: testTeamsGetSetDelete hardcodes team_id=0 for adhoc SQL inserts unrelated to the actual team under test.
What the draft fix changed: In testTeamsGetSetDelete's ad-hoc insert block, changed the literal `0` team_id arguments to `team.ID` for the three inserts into mdm_apple_configuration_profiles, mdm_windows_configuration_profiles, and mdm_android_configuration_profiles, so the subsequent DeleteTeam verification loop (which checks `WHERE team_id = ?` with `team.ID` against `teamRefs`) actually validates cleanup of these rows under the real team being deleted.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟒 95 high β€” react πŸ‘/πŸ‘Ž to teach the reviewer

Expand All @@ -169,14 +169,14 @@ func testTeamsGetSetDelete(t *testing.T, ds *Datastore) {
_, err = q.ExecContext(context.Background(), `
INSERT INTO
mdm_windows_configuration_profiles (team_id, name, syncml, profile_uuid)
VALUES (?, ?, ?, ?)`, 0, fmt.Sprintf("TestPayloadName_%s", tt.name), `<?xml version="1.0"`, fmt.Sprintf("uuid_%s", tt.name))
VALUES (?, ?, ?, ?)`, team.ID, fmt.Sprintf("TestPayloadName_%s", tt.name), `<?xml version="1.0"`, fmt.Sprintf("uuid_%s", tt.name))
if err != nil {
return err
}
_, err = q.ExecContext(context.Background(), `
INSERT INTO
mdm_android_configuration_profiles (profile_uuid, team_id, name, raw_json)
VALUES (?, ?, ?, ?)`, fmt.Sprintf("uuid_%s", tt.name), 0, fmt.Sprintf("TestPayloadName_%s", tt.name), `{"foo": "bar"}`)
VALUES (?, ?, ?, ?)`, fmt.Sprintf("uuid_%s", tt.name), team.ID, fmt.Sprintf("TestPayloadName_%s", tt.name), `{"foo": "bar"}`)
if err != nil {
return err
}
Expand Down Expand Up @@ -970,7 +970,7 @@ func testTeamConflictsWithName(t *testing.T, ds *Datastore) {
// e + combining acute accent).
reneeCombined, err := ds.NewTeam(ctx, &fleet.Team{Name: "RenΓ©e"})
require.NoError(t, err)
reneeDecomposed := "Renée" // e + U+0301 COMBINING ACUTE ACCENT
reneeDecomposed := "RenΓ©e" // e + U+0301 COMBINING ACUTE ACCENT
conflict, err = ds.TeamConflictsWithName(ctx, reneeDecomposed, 0)
require.NoError(t, err)
require.Equal(t, reneeCombined.ID, conflict.ID)
Expand Down
1 change: 1 addition & 0 deletions server/mdm/assets/assets_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -225,3 +225,4 @@ func TestABMToken(t *testing.T) {
require.True(t, ds.GetAllMDMConfigAssetsByNameFuncInvoked)
require.True(t, ds.GetABMTokenByOrgNameFuncInvoked)
}

Comment on lines 225 to +228

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ No test coverage added for new CACertsAndKeyForDecryption / CADecryptRetriever logic

No test was added for CACertsAndKeyForDecryption / CADecryptRetriever in server/mdm/assets/assets_test.go. I was given only the test file and its existing imports/usages (CAKeyPair, APNSKeyPair, X509Cert, APNSTopic, ABMToken); the actual signatures, field names, and error types for CACertsAndKeyForDecryption and CADecryptRetriever (e.g., which fleet.MDMAssetName constants represent "historical" CA certs/keys, the exact returned struct shape, and the specific error value/message for the "not a crypto.Signer" path) are not present anywhere in the provided material. Per the hard rules I must not invent identifiers, struct fields, asset-name constants, or error strings that I cannot see, since a wrong guess would fail to compile or silently test the wrong thing. A complete fix requires viewing server/mdm/assets/assets.go (or wherever these symbols are defined) to get the real function signature, the CADecryptRetriever interface method set, and the historical-cert asset names, then adding table-driven tests for: (a) multiple historical certs where one matches the public key, (b) no historical certs matching (expect a "not found"/no-match error), and (c) a stored key that is not a crypto.Signer producing the documented error. I left the file otherwise unchanged rather than risk a non-compiling or semantically incorrect test.

πŸ€– Prompt for AI agents
In server/mdm/assets/assets_test.go around line 1, review and complete this code-review fix: No test coverage added for new CACertsAndKeyForDecryption / CADecryptRetriever logic.
What the draft fix changed: No test was added for `CACertsAndKeyForDecryption` / `CADecryptRetriever` in `server/mdm/assets/assets_test.go`. I was given only the test file and its existing imports/usages (CAKeyPair, APNSKeyPair, X509Cert, APNSTopic, ABMToken); the actual signatures, field names, and error types for `CACertsAndKeyForDecryption` and `CADecryptRetriever` (e.g., which `fleet.MDMAssetName` constants represent "historical" CA certs/keys, the exact returned struct shape, and the specific error value/message for the "not a crypto.Signer" path) are not present anywhere in the provided material. Per the hard rules I must not invent identifiers, struct fields, asset-name constants, or error strings that I cannot see, since a wrong guess would fail to compile or silently test the wrong thing. A complete fix requires viewing `server/mdm/assets/assets.go` (or wherever these symbols are defined) to get the real function signature, the `CADecryptRetriever` interface method set, and the historical-cert asset names, then adding table-driven tests for: (a) multiple historical certs where one matches the public key, (b) no historical certs matching (expect a "not found"/no-match error), and (c) a stored key that is not a `crypto.Signer` producing the documented error. I left the file otherwise unchanged rather than risk a non-compiling or semantically incorrect test.
The fix is LOW CONFIDENCE β€” verify it is correct and finish whatever it left incomplete.

fix confidence: πŸ”΄ 15 low β€” review closely β€” react πŸ‘/πŸ‘Ž to teach the reviewer

3 changes: 1 addition & 2 deletions server/service/conditional_access_microsoft.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,7 @@ func (svc *Service) ConditionalAccessMicrosoftConfirm(ctx context.Context) (conf

getResponse, err := svc.conditionalAccessMicrosoftProxy.Get(ctx, integration.TenantID, integration.ProxyServerSecret)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ conditionalAccessMicrosoftConfirmResponse silently returns false/empty on proxy Get failure instead of propagating the error

In ConditionalAccessMicrosoftConfirm, when svc.conditionalAccessMicrosoftProxy.Get fails, the function now returns false, "", ctxerr.Wrap(ctx, err, "failed to get integration settings from proxy") instead of swallowing the error and returning (false, "", nil). The redundant svc.logger.ErrorContext call for this failure path was removed since the error is now propagated (and will be logged/handled by the standard error-handling path via ctxerr.Wrap), causing the endpoint (conditionalAccessMicrosoftConfirmEndpoint) to surface a non-200 error response instead of a misleading configuration_completed: false. Risk: this changes the HTTP contract for this specific failure mode (previously 200 OK, now an error response) β€” callers/frontend polling logic relying on the old always-200 behavior to keep polling on proxy outages will need to handle the new error response instead.

πŸ€– Prompt for AI agents
In server/service/conditional_access_microsoft.go around line 133, review and complete this code-review fix: conditionalAccessMicrosoftConfirmResponse silently returns false/empty on proxy Get failure instead of propagating the error.
What the draft fix changed: In `ConditionalAccessMicrosoftConfirm`, when `svc.conditionalAccessMicrosoftProxy.Get` fails, the function now returns `false, "", ctxerr.Wrap(ctx, err, "failed to get integration settings from proxy")` instead of swallowing the error and returning `(false, "", nil)`. The redundant `svc.logger.ErrorContext` call for this failure path was removed since the error is now propagated (and will be logged/handled by the standard error-handling path via `ctxerr.Wrap`), causing the endpoint (`conditionalAccessMicrosoftConfirmEndpoint`) to surface a non-200 error response instead of a misleading `configuration_completed: false`. Risk: this changes the HTTP contract for this specific failure mode (previously 200 OK, now an error response) β€” callers/frontend polling logic relying on the old always-200 behavior to keep polling on proxy outages will need to handle the new error response instead.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟑 75 medium β€” react πŸ‘/πŸ‘Ž to teach the reviewer

if err != nil {
svc.logger.ErrorContext(ctx, "failed to get integration settings from proxy", "err", err)
return false, "", nil
return false, "", ctxerr.Wrap(ctx, err, "failed to get integration settings from proxy")
}

if !getResponse.SetupDone {
Expand Down
16 changes: 8 additions & 8 deletions tools/dibble/pkg/seed/vulns.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,11 @@ var vulnCSVs embed.FS
// VulnsOptions configures the vuln seeder. Counts are per-platform; pass 0
// to skip a platform. DSN is a MySQL connection string.
type VulnsOptions struct {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 πŸ”΅ VulnsOptions field name BatchSiz is a truncated typo for BatchSize

Renamed the BatchSiz field to BatchSize in the VulnsOptions struct definition, and updated all references (opt.BatchSiz in Vulns) to opt.BatchSize, and struct field alignment/formatting adjusted accordingly. No other behavior changed.

πŸ€– Prompt for AI agents
In tools/dibble/pkg/seed/vulns.go around line 21, review and complete this code-review fix: VulnsOptions field name `BatchSiz` is a truncated typo for `BatchSize`.
What the draft fix changed: Renamed the `BatchSiz` field to `BatchSize` in the `VulnsOptions` struct definition, and updated all references (`opt.BatchSiz` in `Vulns`) to `opt.BatchSize`, and struct field alignment/formatting adjusted accordingly. No other behavior changed.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟒 90 high β€” react πŸ‘/πŸ‘Ž to teach the reviewer

DSN string
MacOS int
Ubuntu int
Windows int
BatchSiz int
DSN string
MacOS int
Ubuntu int
Windows int
BatchSize int
}

// Vulns writes plausible-looking software rows directly to MySQL so the
Expand All @@ -41,8 +41,8 @@ type VulnsOptions struct {
// from these rows on their own.
func Vulns(ctx context.Context, log Logger, opt VulnsOptions) Result {
res := Result{Entity: "vulns"}
if opt.BatchSiz <= 0 {
opt.BatchSiz = 500
if opt.BatchSize <= 0 {
opt.BatchSize = 500
}

dsn, err := mysqlDSN(opt.DSN, true)
Expand Down Expand Up @@ -80,7 +80,7 @@ func Vulns(ctx context.Context, log Logger, opt VulnsOptions) Result {
res.Errors = append(res.Errors, fmt.Errorf("read %s: %w", p.file, err))
continue
}
if err := insertSoftware(ctx, db, p.platform, rows, p.count, opt.BatchSiz); err != nil {
if err := insertSoftware(ctx, db, p.platform, rows, p.count, opt.BatchSize); err != nil {
res.Errors = append(res.Errors, fmt.Errorf("insert %s: %w", p.platform, err))
continue
}
Expand Down
Loading