fix(OSQUERY-003): CU-86akhf8u2 5 review findings across 2 files

[flamingo]

Closes 5 review findings across 2 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	🟢 92 high	getDatabaseValue/setDatabaseValue/deleteDatabase* throw std::runtime_error instead of returning Status	osquery/database/database.cpp:301
2	🟢 92 high	setDatabaseBatch throws std::runtime_error instead of returning a failure Status	osquery/database/database.cpp:343
3	🟡 85 medium	getDatabaseValue/setDatabaseBatch/deleteDatabaseValue dereference a possibly-null plugin without checking	osquery/database/database.cpp:301
4	🟡 65 medium	OpenframeTokenRefresher constructor throws std::runtime_error instead of returning Status	openframe/openframe_token_refresher.cpp:6
5	🟢 95 high	Missing <osquery/logger/logger.h> include despite using LOG() macros	openframe/openframe_token_refresher.cpp:1

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

5 finding(s) fixed in this draft — 5 explained inline on the diff.

[flamingo]

🦩 🔴 getDatabaseValue/setDatabaseValue/deleteDatabase throw std::runtime_error instead of returning Status*

In getDatabaseValue(domain, key, value) (the std::string overload), replaced throw std::runtime_error("Cannot get database value: " + key); with return Status(1, "Cannot get database value: " + key);, matching the file's Status-based error convention.

🤖 Prompt for AI agents

In osquery/database/database.cpp around line 301, review and complete this code-review fix: getDatabaseValue/setDatabaseValue/deleteDatabase* throw std::runtime_error instead of returning Status.
What the draft fix changed: In getDatabaseValue(domain, key, value) (the std::string overload), replaced `throw std::runtime_error("Cannot get database value: " + key);` with `return Status(1, "Cannot get database value: " + key);`, matching the file's Status-based error convention.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔴 setDatabaseBatch throws std::runtime_error instead of returning a failure Status

In setDatabaseBatch, replaced throw std::runtime_error("Cannot set database values"); with return Status(1, "Cannot set database values");. Also applied the same throw-to-Status conversion for consistency in deleteDatabaseValue, deleteDatabaseRange, and scanDatabaseKeys, which had the identical pattern and are named directly in the same finding group.

🤖 Prompt for AI agents

In osquery/database/database.cpp around line 343, review and complete this code-review fix: setDatabaseBatch throws std::runtime_error instead of returning a failure Status.
What the draft fix changed: In setDatabaseBatch, replaced `throw std::runtime_error("Cannot set database values");` with `return Status(1, "Cannot set database values");`. Also applied the same throw-to-Status conversion for consistency in deleteDatabaseValue, deleteDatabaseRange, and scanDatabaseKeys, which had the identical pattern and are named directly in the same finding group.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔴 getDatabaseValue/setDatabaseBatch/deleteDatabaseValue dereference a possibly-null plugin without checking

Added if (plugin == nullptr) { return Status(1, ...); } null checks after every getDatabasePlugin() call in getDatabaseValue, setDatabaseBatch, deleteDatabaseValue, deleteDatabaseRange, and scanDatabaseKeys, preventing a null-pointer dereference when the registry lookup fails (rf.exists(...) returns false), before invoking plugin->get/putBatch/remove/removeRange/scan.

🤖 Prompt for AI agents

In osquery/database/database.cpp around line 301, review and complete this code-review fix: getDatabaseValue/setDatabaseBatch/deleteDatabaseValue dereference a possibly-null plugin without checking.
What the draft fix changed: Added `if (plugin == nullptr) { return Status(1, ...); }` null checks after every `getDatabasePlugin()` call in getDatabaseValue, setDatabaseBatch, deleteDatabaseValue, deleteDatabaseRange, and scanDatabaseKeys, preventing a null-pointer dereference when the registry lookup fails (rf.exists(...) returns false), before invoking plugin->get/putBatch/remove/removeRange/scan.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔴 OpenframeTokenRefresher constructor throws std::runtime_error instead of returning Status

In OpenframeTokenRefresher::OpenframeTokenRefresher (constructor), replaced the throw std::runtime_error(...) with a LOG(ERROR) and no throw, so a null extractor no longer crashes the daemon. Added a guard in start() that checks extractor_ and logs+returns instead of starting a thread that would dereference a null pointer, and an equivalent guard in process() as defense in depth. This does not add a Status-returning factory function (none exists in the header shown, and adding one would require editing the header file which is out of scope for a single-file fix), so the finding is only partially resolved toward the ideal OSQUERY-003 pattern — a complete fix would also add a static Status OpenframeTokenRefresher::create(...) factory in the header/other file.

🤖 Prompt for AI agents

In openframe/openframe_token_refresher.cpp around line 6, review and complete this code-review fix: OpenframeTokenRefresher constructor throws std::runtime_error instead of returning Status.
What the draft fix changed: In OpenframeTokenRefresher::OpenframeTokenRefresher (constructor), replaced the `throw std::runtime_error(...)` with a `LOG(ERROR)` and no throw, so a null extractor no longer crashes the daemon. Added a guard in `start()` that checks `extractor_` and logs+returns instead of starting a thread that would dereference a null pointer, and an equivalent guard in `process()` as defense in depth. This does not add a Status-returning factory function (none exists in the header shown, and adding one would require editing the header file which is out of scope for a single-file fix), so the finding is only partially resolved toward the ideal OSQUERY-003 pattern — a complete fix would also add a static `Status OpenframeTokenRefresher::create(...)` factory in the header/other file.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 Missing <osquery/logger/logger.h> include despite using LOG() macros

Added #include <osquery/logger/logger.h> after the existing includes at the top of the file, exactly as suggested, ensuring LOG() macros are explicitly declared rather than relying on transitive includes.

🤖 Prompt for AI agents

In openframe/openframe_token_refresher.cpp around line 1, review and complete this code-review fix: Missing <osquery/logger/logger.h> include despite using LOG() macros.
What the draft fix changed: Added `#include <osquery/logger/logger.h>` after the existing includes at the top of the file, exactly as suggested, ensuring LOG() macros are explicitly declared rather than relying on transitive includes.
Verify the change is correct and complete; do not refactor unrelated code.

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