fix(DUP-001): CU-86akhf8u2 9 review findings across 4 files - #100
flamingo[bot] wants to merge 4 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| * constrained ids, and the child node to populate a Row which is appended to | ||
| * results. | ||
| */ | ||
| QueryData genContainerMounts(QueryContext& context) { |
There was a problem hiding this comment.
🦩 🟠 osquery/tables/applications/posix/docker#osquery::tables::genContainerMounts duplicates a near-identical definition
genContainerMounts (formerly at line 530) was rewritten to call a new shared helper getContainerChildRows() (added above genContainerMounts), passing "Mounts" as the child path and a lambda that builds the mount-specific Row fields. This removes the near-duplicate loop/try/catch structure it shared with genContainerPorts. Needed #include <functional> transitively via existing headers (std::function used); if not already transitively included, this could fail to compile — worth a build check, hence not marked 90+.
🤖 Prompt for AI agents
In osquery/tables/applications/posix/docker.cpp around line 530, review and complete this code-review fix: osquery/tables/applications/posix/docker#osquery::tables::genContainerMounts duplicates a near-identical definition.
What the draft fix changed: genContainerMounts (formerly at line 530) was rewritten to call a new shared helper `getContainerChildRows()` (added above genContainerMounts), passing "Mounts" as the child path and a lambda that builds the mount-specific Row fields. This removes the near-duplicate loop/try/catch structure it shared with genContainerPorts. Needed `#include <functional>` transitively via existing headers (std::function used); if not already transitively included, this could fail to compile — worth a build check, hence not marked 90+.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| @@ -605,33 +638,20 @@ QueryData genContainerNetworks(QueryContext& context) { | |||
| * @brief Entry point for docker_container_ports table. | |||
| */ | |||
| QueryData genContainerPorts(QueryContext& context) { | |||
There was a problem hiding this comment.
🦩 🟠 osquery/tables/applications/posix/docker#osquery::tables::genContainerPorts duplicates a near-identical definition
genContainerPorts (formerly at line 607) was rewritten to call the same new shared helper getContainerChildRows(), passing "Ports" as the child path and a lambda that builds the port-specific Row fields, eliminating the duplicated iterate/try/catch structure previously copy-pasted from genContainerMounts.
🤖 Prompt for AI agents
In osquery/tables/applications/posix/docker.cpp around line 607, review and complete this code-review fix: osquery/tables/applications/posix/docker#osquery::tables::genContainerPorts duplicates a near-identical definition.
What the draft fix changed: genContainerPorts (formerly at line 607) was rewritten to call the same new shared helper `getContainerChildRows()`, passing "Ports" as the child path and a lambda that builds the port-specific Row fields, eliminating the duplicated iterate/try/catch structure previously copy-pasted from genContainerMounts.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| /** | ||
| * @brief Calls layer extractor for all images for docker_image_layers table | ||
| */ | ||
| void getImageLayersAll(QueryData& results) { |
There was a problem hiding this comment.
🦩 🟠 osquery/tables/applications/posix/docker#osquery::tables::getImageLayersAll duplicates an existing definition
getImageLayersAll (formerly at line 1086) was removed and replaced by a call to a new shared helper getImageDetailsAll() (added just above genImageLayers) which fetches "/images/json", iterates entries, extracts the sha256-stripped image id, and invokes a caller-supplied per-image extractor function pointer (getImageLayers). genImageLayers now calls getImageDetailsAll(results, "image details", getImageLayers) in its else branch instead of the removed getImageLayersAll.
🤖 Prompt for AI agents
In osquery/tables/applications/posix/docker.cpp around line 1086, review and complete this code-review fix: osquery/tables/applications/posix/docker#osquery::tables::getImageLayersAll duplicates an existing definition.
What the draft fix changed: getImageLayersAll (formerly at line 1086) was removed and replaced by a call to a new shared helper `getImageDetailsAll()` (added just above genImageLayers) which fetches "/images/json", iterates entries, extracts the sha256-stripped image id, and invokes a caller-supplied per-image extractor function pointer (getImageLayers). genImageLayers now calls `getImageDetailsAll(results, "image details", getImageLayers)` in its else branch instead of the removed getImageLayersAll.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| * @param extractor Per-image extraction function (getImageLayers or | ||
| * getImageHistory). | ||
| */ | ||
| void getImageHistoryAll(QueryData& results) { |
There was a problem hiding this comment.
🦩 🟠 osquery/tables/applications/posix/docker#osquery::tables::getImageHistoryAll duplicates an existing definition
getImageHistoryAll (formerly at line 1168) was removed and replaced by a call to the same new shared helper getImageDetailsAll(), passing the getImageHistory function as the per-image extractor. genImageHistory now calls getImageDetailsAll(results, "image history", getImageHistory) in its else branch instead of the removed getImageHistoryAll, eliminating the duplicated iterate/try/catch loop over "/images/json".
🤖 Prompt for AI agents
In osquery/tables/applications/posix/docker.cpp around line 1168, review and complete this code-review fix: osquery/tables/applications/posix/docker#osquery::tables::getImageHistoryAll duplicates an existing definition.
What the draft fix changed: getImageHistoryAll (formerly at line 1168) was removed and replaced by a call to the same new shared helper `getImageDetailsAll()`, passing the getImageHistory function as the per-image extractor. genImageHistory now calls `getImageDetailsAll(results, "image history", getImageHistory)` in its else branch instead of the removed getImageHistoryAll, eliminating the duplicated iterate/try/catch loop over "/images/json".
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| @@ -242,35 +262,13 @@ Status readFile(const fs::path& path, std::string& content, bool shouldLog) { | |||
| } | |||
|
|
|||
| Status isWritable(const fs::path& path, bool effective) { | |||
There was a problem hiding this comment.
🦩 🟠 osquery/filesystem/filesystem#isWritable duplicates an existing definition
isWritable (line ~245) now delegates to a new private helper checkPathAccess (defined in the anonymous namespace alongside checkFileReadLimit), passing PF_WRITE/W_OK and the "not writable" message, eliminating the duplicated body it shared with isReadable.
🤖 Prompt for AI agents
In osquery/filesystem/filesystem.cpp around line 244, review and complete this code-review fix: osquery/filesystem/filesystem#isWritable duplicates an existing definition.
What the draft fix changed: `isWritable` (line ~245) now delegates to a new private helper `checkPathAccess` (defined in the anonymous namespace alongside `checkFileReadLimit`), passing `PF_WRITE`/`W_OK` and the "not writable" message, eliminating the duplicated body it shared with `isReadable`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| path, effective, PF_WRITE, W_OK, "Path is not writable: "); | ||
| } | ||
|
|
||
| Status isReadable(const fs::path& path, bool effective) { |
There was a problem hiding this comment.
🦩 🟠 osquery/filesystem/filesystem#isReadable duplicates an existing definition
isReadable (line ~250) now likewise delegates to the same checkPathAccess helper, passing PF_READ/R_OK and the "not readable" message, so both functions share one implementation instead of duplicating the pathExists/PlatformFile/platformAccess logic.
🤖 Prompt for AI agents
In osquery/filesystem/filesystem.cpp around line 260, review and complete this code-review fix: osquery/filesystem/filesystem#isReadable duplicates an existing definition.
What the draft fix changed: `isReadable` (line ~250) now likewise delegates to the same `checkPathAccess` helper, passing `PF_READ`/`R_OK` and the "not readable" message, so both functions share one implementation instead of duplicating the pathExists/PlatformFile/platformAccess logic.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| }; | ||
| #endif | ||
|
|
||
| std::set<std::string> getPathsFromConstraints(const QueryContext& context) { |
There was a problem hiding this comment.
🦩 🟠 osquery/tables/utility/file#osquery::tables::getPathsFromConstraints duplicates an existing definition
In osquery/tables/utility/file.cpp, getPathsFromConstraints (anonymous namespace) was rewritten to be a thin wrapper delegating to a new shared local helper getPathsOrDirsFromConstraints(context, column_name, glob_flags), which factors out the common constraint-expansion logic previously duplicated with getDirsFromConstraints. getPathsFromConstraints now just calls the helper with "path" and GLOB_ALL | GLOB_NO_CANON.
🤖 Prompt for AI agents
In osquery/tables/utility/file.cpp around line 210, review and complete this code-review fix: osquery/tables/utility/file#osquery::tables::getPathsFromConstraints duplicates an existing definition.
What the draft fix changed: In `osquery/tables/utility/file.cpp`, `getPathsFromConstraints` (anonymous namespace) was rewritten to be a thin wrapper delegating to a new shared local helper `getPathsOrDirsFromConstraints(context, column_name, glob_flags)`, which factors out the common constraint-expansion logic previously duplicated with `getDirsFromConstraints`. `getPathsFromConstraints` now just calls the helper with `"path"` and `GLOB_ALL | GLOB_NO_CANON`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| return results; | ||
| } | ||
|
|
||
| std::set<std::string> getDirsFromConstraints(const QueryContext& context) { |
There was a problem hiding this comment.
🦩 🟠 osquery/tables/utility/file#osquery::tables::getDirsFromConstraints duplicates an existing definition
In the same file, getDirsFromConstraints was likewise rewritten to delegate to getPathsOrDirsFromConstraints with "directory" and GLOB_FOLDERS | GLOB_NO_CANON, eliminating the duplicated structure flagged against getPathsFromConstraints. Both functions retain their original external signatures/behavior; only the shared logic was extracted into one local static helper within the same anonymous namespace (no new file/module needed since the helper is private to this translation unit).
🤖 Prompt for AI agents
In osquery/tables/utility/file.cpp around line 237, review and complete this code-review fix: osquery/tables/utility/file#osquery::tables::getDirsFromConstraints duplicates an existing definition.
What the draft fix changed: In the same file, `getDirsFromConstraints` was likewise rewritten to delegate to `getPathsOrDirsFromConstraints` with `"directory"` and `GLOB_FOLDERS | GLOB_NO_CANON`, eliminating the duplicated structure flagged against `getPathsFromConstraints`. Both functions retain their original external signatures/behavior; only the shared logic was extracted into one local static helper within the same anonymous namespace (no new file/module needed since the helper is private to this translation unit).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| return s; | ||
| } | ||
|
|
||
| Status RocksDBDatabasePlugin::put(const std::string& domain, |
There was a problem hiding this comment.
🦩 🟠 plugins/database/rocksdb#osquery::RocksDBDatabasePlugin::put duplicates an existing definition
In RocksDBDatabasePlugin::put(const std::string&, const std::string&, int) (the int-overload), replaced the duplicated body return putBatch(domain, {std::make_pair(key, std::to_string(value))}); with a delegation to the existing string overload return put(domain, key, std::to_string(value));. This mirrors the pattern flagged as duplicating SQLiteDatabasePlugin::put, removing the structural duplication of building a single-entry batch inline while keeping identical externally-observable behavior (same putBatch call chain, same WAL/sync semantics, same error handling in the string put -> putBatch path).
🤖 Prompt for AI agents
In plugins/database/rocksdb.cpp around line 334, review and complete this code-review fix: plugins/database/rocksdb#osquery::RocksDBDatabasePlugin::put duplicates an existing definition.
What the draft fix changed: In `RocksDBDatabasePlugin::put(const std::string&, const std::string&, int)` (the int-overload), replaced the duplicated body `return putBatch(domain, {std::make_pair(key, std::to_string(value))});` with a delegation to the existing string overload `return put(domain, key, std::to_string(value));`. This mirrors the pattern flagged as duplicating `SQLiteDatabasePlugin::put`, removing the structural duplication of building a single-entry batch inline while keeping identical externally-observable behavior (same putBatch call chain, same WAL/sync semantics, same error handling in the string `put` -> `putBatch` path).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 9 review findings across 4 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
osquery/tables/applications/posix/docker.cpp:530osquery/tables/applications/posix/docker.cpp:607osquery/tables/applications/posix/docker.cpp:1086osquery/tables/applications/posix/docker.cpp:1168osquery/filesystem/filesystem.cpp:244osquery/filesystem/filesystem.cpp:260osquery/tables/utility/file.cpp:210osquery/tables/utility/file.cpp:237plugins/database/rocksdb.cpp:334What 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-9b30b2c8be3fMerging 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)