fix(adhoc-sweep-fixes): CU-86akhf8u2 27 review findings across 21 files

[flamingo]

Closes 27 review findings across 21 files.

Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.

#	Fix confidence	Finding	Location
1	🟢 95 high	Missing comma in aggregate initializer list breaks compilation	tests/integration/tables/azure_instance_metadata.cpp:31
2	🟢 95 high	Missing closing brace for TEST_F body and unbalanced namespace closures	tests/integration/tables/azure_instance_metadata.cpp:22
3	🟢 90 high	OpenframeEncryptionService class defined outside the osquery namespace	openframe/openframe_encryption_service.h:11
4	🔴 55 low — review closely	decrypt() documented to throw std::runtime_error instead of returning osquery::Status	openframe/openframe_encryption_service.h:16
5	🟢 95 high	test_windows_service.py uses Python 2-only except syntax	tools/tests/test_windows_service.py:106
6	🟡 75 medium	test_windows_service.py uses Python 2 string/bytes semantics incompatible with Python 3	tools/tests/test_windows_service.py:248
7	🟡 85 medium	openframe_mode unconditionally disables TLS peer verification for every request	osquery/remote/transports/tls.cpp:115
8	🟡 60 medium	Bearer token and machine-id header are attached to every TLS request regardless of destination host	osquery/remote/transports/tls.cpp:96
9	🔴 40 low — review closely	openframe_authorization_manager.h missing standard osquery copyright/SPDX header	openframe/openframe_authorization_manager.h:10
10	🟡 60 medium	Registration macro invoked outside the osquery namespace closing brace mismatch risk	plugins/logger/filesystem_logger.h:58
11	🟡 85 medium	ETWProcessEventsTests reinterpret_casts a stack local into a shared_ptr, causing UB and a leaked/undefined subscriber pointer	osquery/tables/events/tests/windows/etw_process_events_tests.cpp:51
12	🟡 85 medium	Same unsafe reinterpret_cast pattern repeated for the publisher check	osquery/tables/events/tests/windows/etw_process_events_tests.cpp:60
13	🟡 75 medium	getSensorId parses hex string without validating conversion success	osquery/tables/applications/posix/carbon_black.cpp:34
14	🔴 55 low — review closely	getSensorId substr(11, 16) reads past end of a 16-byte string	osquery/tables/applications/posix/carbon_black.cpp:45
15	🟢 98 high	pidsFromContext lambda inserts procs into itself instead of proc	osquery/tables/system/system_utils.cpp:34
16	🟢 95 high	getBluetoothSharingStatus() checks the wrong map's key against the value, mismatching containers	osquery/tables/system/darwin/sharing_preferences.cpp:174
17	🟢 95 high	EphemeralDatabasePlugin::removeRange erases from db_ while iterating over db_.at(domain), risking dangling iterators	osquery/database/ephemeral.cpp:168
18	🟢 95 high	cosineSimilarity divides by buffer_size without checking for empty buffer	osquery/events/windows/windowseventlogpublisher.cpp:208
19	🟢 92 high	extensionPathActive swallows client construction/ping exceptions and treats them as success path fallthrough	osquery/extensions/extensions.cpp:180
20	🟢 95 high	CFRelease(details) called even though IORegistryEntryCreateCFProperties result is unchecked and details is uninitialized on failure	osquery/tables/system/darwin/iokit_registry.cpp:59
21	🟢 95 high	populateAllKeysRecursive calls populateSubkeys with wrong argument count (missing replaceKeys param) — likely a compile error	osquery/tables/system/windows/registry.cpp:473
22	🟡 85 medium	getManufacturer() misuses WmiRequest::CreateWmiRequest's Expected<> return without checking isError()	osquery/tables/system/windows/wmi_bios_info.cpp:67
23	🟡 85 medium	archive() ignores archive_write_header/archive_write_data return codes, can silently produce truncated/corrupt tar archives	osquery/filesystem/file_compression.cpp:184
24	🟡 85 medium	File-scope constant kEtwDNSPublisherName is fine, but etwDefaultKernelID/etwDNSStartID/etwDNSStopID break kPascalCase and are unused static member constants	osquery/events/windows/etw/etw_publisher_dns.h:49
25	🟢 90 high	getAt() double-manages ownership of pEntry via two RAII wrappers with no clear owner	osquery/tables/system/windows/windows_update_history.cpp:288
26	🟡 75 medium	FilesystemLoggerPlugin::impl constructor throws std::runtime_error on a config validation failure that should already be unreachable	plugins/logger/filesystem_logger.cpp:175
27	🟢 90 high	numTasks (long) compared against unsigned size_t loop index causes signed/unsigned mismatch risk	osquery/tables/system/windows/scheduled_tasks.cpp:78

What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.

---

Run: https://product-hub.flamingo.so/admin/code-review
Run id: 46e37628-c6c9-49a4-aa29-9b30b2c8be3f

Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.

ClickUp task: CU-86akhf8u2 Osquery review findings sweep (15 PRs)

[flamingo]

🦩 What this fix changed, finding by finding

27 finding(s) fixed in this draft — 27 explained inline on the diff; 3 low-confidence hunk(s) need close review before merging.

[flamingo]

🦩 🔴 Missing comma in aggregate initializer list breaks compilation

Fixed the missing comma between {"sku", NormalType} and {"version", NormalType} in the ValidationMap initializer list inside TEST_F(azureInstanceMetadata, test_sanity), now {"sku", NormalType}, followed by {"version", NormalType}, on separate lines as suggested.

🤖 Prompt for AI agents

In tests/integration/tables/azure_instance_metadata.cpp around line 31, review and complete this code-review fix: Missing comma in aggregate initializer list breaks compilation.
What the draft fix changed: Fixed the missing comma between `{"sku", NormalType}` and `{"version", NormalType}` in the ValidationMap initializer list inside TEST_F(azureInstanceMetadata, test_sanity), now `{"sku", NormalType},` followed by `{"version", NormalType},` on separate lines as suggested.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔴 Missing closing brace for TEST_F body and unbalanced namespace closures

Added the missing closing brace } for the TEST_F function body after the if (!data.empty()) { ... } block, and corrected the two trailing brace-closing comment lines so the file now ends with } // namespace table_tests followed by } // namespace osquery, matching sibling test files' structure.

🤖 Prompt for AI agents

In tests/integration/tables/azure_instance_metadata.cpp around line 22, review and complete this code-review fix: Missing closing brace for TEST_F body and unbalanced namespace closures.
What the draft fix changed: Added the missing closing brace `}` for the TEST_F function body after the `if (!data.empty()) { ... }` block, and corrected the two trailing brace-closing comment lines so the file now ends with `} // namespace table_tests` followed by `} // namespace osquery`, matching sibling test files' structure.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔴 OpenframeEncryptionService class defined outside the osquery namespace

Wrapped the OpenframeEncryptionService class declaration in namespace osquery { ... } in openframe/openframe_encryption_service.h, matching the sibling OpenframeTokenRefresher convention. Added closing brace with // namespace osquery comment.

🤖 Prompt for AI agents

In openframe/openframe_encryption_service.h around line 11, review and complete this code-review fix: OpenframeEncryptionService class defined outside the osquery namespace.
What the draft fix changed: Wrapped the OpenframeEncryptionService class declaration in `namespace osquery { ... }` in openframe/openframe_encryption_service.h, matching the sibling OpenframeTokenRefresher convention. Added closing brace with `// namespace osquery` comment.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔴 decrypt() documented to throw std::runtime_error instead of returning osquery::Status

Changed decrypt()'s signature from std::string decrypt(const std::string& data) (documented to throw std::runtime_error) to Status decrypt(const std::string& data, std::string& decrypted), updated the doc comment to describe the Status-based contract, and added #include "osquery/core/status.h". This header-only change is consistent with the repo's Status convention, but the corresponding .cpp implementation (not shown/provided) must be updated to match this new signature and populate the output parameter instead of throwing/returning a string, or the build will fail to link/compile; that implementation change is outside this file and is not verified here.

🤖 Prompt for AI agents

In openframe/openframe_encryption_service.h around line 16, review and complete this code-review fix: decrypt() documented to throw std::runtime_error instead of returning osquery::Status.
What the draft fix changed: Changed decrypt()'s signature from `std::string decrypt(const std::string& data)` (documented to throw std::runtime_error) to `Status decrypt(const std::string& data, std::string& decrypted)`, updated the doc comment to describe the Status-based contract, and added `#include "osquery/core/status.h"`. This header-only change is consistent with the repo's Status convention, but the corresponding .cpp implementation (not shown/provided) must be updated to match this new signature and populate the output parameter instead of throwing/returning a string, or the build will fail to link/compile; that implementation change is outside this file and is not verified here.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 test_windows_service.py uses Python 2-only except syntax

In sc(), changed except subprocess.CalledProcessError, err: to except subprocess.CalledProcessError as err:, the standard Python 3 exception-binding syntax, resolving the SyntaxError that prevented the module from parsing at all.

🤖 Prompt for AI agents

In tools/tests/test_windows_service.py around line 106, review and complete this code-review fix: test_windows_service.py uses Python 2-only except syntax.
What the draft fix changed: In `sc()`, changed `except subprocess.CalledProcessError, err:` to `except subprocess.CalledProcessError as err:`, the standard Python 3 exception-binding syntax, resolving the SyntaxError that prevented the module from parsing at all.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 archive() ignores archive_write_header/archive_write_data return codes, can silently produce truncated/corrupt tar archives

In archive(), the return values of archive_write_header() and archive_write_data() are now checked. archive_write_header()'s result is checked for ARCHIVE_FATAL/ARCHIVE_WARN, and archive_write_data()'s result is checked for a negative value or a byte count less than the requested block size; on either failure the entry/archive handles are freed and a failure Status with the libarchive error string (via archive_error_string()) is returned instead of silently continuing and returning Status::success().

🤖 Prompt for AI agents

In osquery/filesystem/file_compression.cpp around line 184, review and complete this code-review fix: archive() ignores archive_write_header/archive_write_data return codes, can silently produce truncated/corrupt tar archives.
What the draft fix changed: In archive(), the return values of archive_write_header() and archive_write_data() are now checked. archive_write_header()'s result is checked for ARCHIVE_FATAL/ARCHIVE_WARN, and archive_write_data()'s result is checked for a negative value or a byte count less than the requested block size; on either failure the entry/archive handles are freed and a failure Status with the libarchive error string (via archive_error_string()) is returned instead of silently continuing and returning Status::success().
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 File-scope constant kEtwDNSPublisherName is fine, but etwDefaultKernelID/etwDNSStartID/etwDNSStopID break kPascalCase and are unused static member constants

Renamed the static class constants in EtwPublisherDNS (etw_publisher_dns.h) from etwDefaultKernelID, etwDNSStartID, etwDNSStopID to kEtwDefaultKernelId, kEtwDnsStartId, kEtwDnsStopId to follow kPascalCase per OSQUERY-008, matching the existing kEtwDNSPublisherName convention. Since these are declared but unused within this header (per the finding), no other in-file references needed updating; any external usage in the corresponding .cpp file (not provided) would need matching updates, which is unverified here.

🤖 Prompt for AI agents

In osquery/events/windows/etw/etw_publisher_dns.h around line 49, review and complete this code-review fix: File-scope constant kEtwDNSPublisherName is fine, but etwDefaultKernelID/etwDNSStartID/etwDNSStopID break kPascalCase and are unused static member constants.
What the draft fix changed: Renamed the static class constants in EtwPublisherDNS (etw_publisher_dns.h) from `etwDefaultKernelID`, `etwDNSStartID`, `etwDNSStopID` to `kEtwDefaultKernelId`, `kEtwDnsStartId`, `kEtwDnsStopId` to follow kPascalCase per OSQUERY-008, matching the existing `kEtwDNSPublisherName` convention. Since these are declared but unused within this header (per the finding), no other in-file references needed updating; any external usage in the corresponding .cpp file (not provided) would need matching updates, which is unverified here.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 getAt() double-manages ownership of pEntry via two RAII wrappers with no clear owner

In getAt (osquery/tables/system/windows/windows_update_history.cpp), changed the call populateWindowsUpdateHistoryEntry(pEntry) to populateWindowsUpdateHistoryEntry(entry.get()), making the RAII wrapper entry the sole formally-referenced owner of the interface pointer used past its construction, removing the ambiguity about which object owns the lifetime and eliminating the appearance of an unused variable while preserving identical runtime behavior (same pointer value passed through, released once when entry goes out of scope).

🤖 Prompt for AI agents

In osquery/tables/system/windows/windows_update_history.cpp around line 288, review and complete this code-review fix: getAt() double-manages ownership of pEntry via two RAII wrappers with no clear owner.
What the draft fix changed: In `getAt` (osquery/tables/system/windows/windows_update_history.cpp), changed the call `populateWindowsUpdateHistoryEntry(pEntry)` to `populateWindowsUpdateHistoryEntry(entry.get())`, making the RAII wrapper `entry` the sole formally-referenced owner of the interface pointer used past its construction, removing the ambiguity about which object owns the lifetime and eliminating the appearance of an unused variable while preserving identical runtime behavior (same pointer value passed through, released once when `entry` goes out of scope).
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 FilesystemLoggerPlugin::impl constructor throws std::runtime_error on a config validation failure that should already be unreachable

In FilesystemLoggerPlugin::impl constructor (struct FilesystemLoggerPlugin::impl in plugins/logger/filesystem_logger.cpp), replaced the throw std::runtime_error(...) on tryTo<std::int32_t> conversion failure with a LOG(ERROR) call plus a safe fallback assignment to a new kDefaultLoggerModeOctal constant (0640, matching the CLI flag's own default). This removes the possibility of an unhandled C++ exception propagating out of plugin construction during setUp/registration, while preserving behavior in the normal case (validated flag converts successfully). Risk: this changes runtime behavior in the previously-unreachable error path from "crash" to "silently continue with default permissions"; a more thorough fix per the finding (returning Status from setUp() and deferring impl construction) would be more invasive and touch the header (filesystem_logger.h), which was not given, so it was not attempted.

🤖 Prompt for AI agents

In plugins/logger/filesystem_logger.cpp around line 175, review and complete this code-review fix: FilesystemLoggerPlugin::impl constructor throws std::runtime_error on a config validation failure that should already be unreachable.
What the draft fix changed: In `FilesystemLoggerPlugin::impl` constructor (struct `FilesystemLoggerPlugin::impl` in `plugins/logger/filesystem_logger.cpp`), replaced the `throw std::runtime_error(...)` on `tryTo<std::int32_t>` conversion failure with a `LOG(ERROR)` call plus a safe fallback assignment to a new `kDefaultLoggerModeOctal` constant (0640, matching the CLI flag's own default). This removes the possibility of an unhandled C++ exception propagating out of plugin construction during `setUp`/registration, while preserving behavior in the normal case (validated flag converts successfully). Risk: this changes runtime behavior in the previously-unreachable error path from "crash" to "silently continue with default permissions"; a more thorough fix per the finding (returning `Status` from `setUp()` and deferring `impl` construction) would be more invasive and touch the header (`filesystem_logger.h`), which was not given, so it was not attempted.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 numTasks (long) compared against unsigned size_t loop index causes signed/unsigned mismatch risk

In enumerateTasksForFolder, the get_Count return value (HRESULT) is now checked with FAILED(ret), and a negative numTasks is also rejected before the loop runs, returning early (after releasing pTaskCollection) on failure. The loop variable i was changed from size_t to long so the comparison i < numTasks is a signed/signed comparison with no implicit unsigned wraparound, eliminating the out-of-bounds indexing risk via get_Item(_variant_t(i + 1), ...).

🤖 Prompt for AI agents

In osquery/tables/system/windows/scheduled_tasks.cpp around line 78, review and complete this code-review fix: numTasks (long) compared against unsigned size_t loop index causes signed/unsigned mismatch risk.
What the draft fix changed: In `enumerateTasksForFolder`, the `get_Count` return value (HRESULT) is now checked with `FAILED(ret)`, and a negative `numTasks` is also rejected before the loop runs, returning early (after releasing `pTaskCollection`) on failure. The loop variable `i` was changed from `size_t` to `long` so the comparison `i < numTasks` is a signed/signed comparison with no implicit unsigned wraparound, eliminating the out-of-bounds indexing risk via `get_Item(_variant_t(i + 1), ...)`.
Verify the change is correct and complete; do not refactor unrelated code.

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