fix(adhoc-sweep-fixes): CU-86akdypw4 56 review findings across 31 files - #106
fix(adhoc-sweep-fixes): CU-86akdypw4 56 review findings across 31 files#106flamingo[bot] wants to merge 31 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| fn.apply(this, opt); | ||
| }, callback, optional); | ||
| } | ||
| this.startConfiguration = function () { |
There was a problem hiding this comment.
🦩 🔴 startConfiguration/stopConfiguration/openUserInitiatedConnection reference undefined data and callback variables
In startConfiguration, stopConfiguration, openUserInitiatedConnection, closeUserInitiatedConnection, and getRemoteAccessConnectionStatus, changed the function signatures from function () to function (mode, callback) and added var data = new Buffer(4); data.writeUInt32LE(mode, 0); before the sendCommand call, mirroring the pattern in unprovision() just above. This defines the previously-undeclared data and callback identifiers used in the sendCommand(...) calls. The optional-args loop (for (var i = 2; ...)) already assumed a (mode, callback, ...optional) signature, so this aligns the declared parameters with that assumption. Risk: I inferred that all five commands take a single 4-byte mode argument written as UInt32LE, based on unprovision's pattern and the loop starting at index 2; if any of these five HECI commands actually expects a different payload (e.g. no payload, or a different structure), this would need further correction — this cannot be fully verified without the HECI protocol spec for opcodes 0x29, 0x5E, 0x44, 0x45, 0x46.
🤖 Prompt for AI agents
In modules/amt_heci.js around line 265, review and complete this code-review fix: startConfiguration/stopConfiguration/openUserInitiatedConnection reference undefined `data` and `callback` variables.
What the draft fix changed: In startConfiguration, stopConfiguration, openUserInitiatedConnection, closeUserInitiatedConnection, and getRemoteAccessConnectionStatus, changed the function signatures from `function ()` to `function (mode, callback)` and added `var data = new Buffer(4); data.writeUInt32LE(mode, 0);` before the `sendCommand` call, mirroring the pattern in `unprovision()` just above. This defines the previously-undeclared `data` and `callback` identifiers used in the `sendCommand(...)` calls. The optional-args loop (`for (var i = 2; ...)`) already assumed a `(mode, callback, ...optional)` signature, so this aligns the declared parameters with that assumption. Risk: I inferred that all five commands take a single 4-byte `mode` argument written as UInt32LE, based on `unprovision`'s pattern and the loop starting at index 2; if any of these five HECI commands actually expects a different payload (e.g. no payload, or a different structure), this would need further correction — this cannot be fully verified without the HECI protocol spec for opcodes 0x29, 0x5E, 0x44, 0x45, 0x46.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 72 medium — react 👍/👎 to teach the reviewer
| @@ -297,7 +307,7 @@ function amt_heci() { | |||
| } | |||
| this.getProtocolVersion = function (callback) { | |||
There was a problem hiding this comment.
🦩 🔴 getProtocolVersion pushes to undeclared opt instead of optional
In getProtocolVersion, changed opt.push(arguments[i]); to optional.push(arguments[i]); in the optional-arguments collection loop, matching the exact suggested fix and the pattern used by all sibling methods.
🤖 Prompt for AI agents
In modules/amt_heci.js around line 298, review and complete this code-review fix: getProtocolVersion pushes to undeclared `opt` instead of `optional`.
What the draft fix changed: In getProtocolVersion, changed `opt.push(arguments[i]);` to `optional.push(arguments[i]);` in the optional-arguments collection loop, matching the exact suggested fix and the pattern used by all sibling methods.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 97 high — react 👍/👎 to teach the reviewer
| for (var i = 2; i < arguments.length; ++i) { optional.push(arguments[i]); } | ||
| var data = new Buffer(4); | ||
| data.writeUInt32LE(mode, 0); | ||
| this.sendCommand(0x46, data, function (header, fn, opt) { |
There was a problem hiding this comment.
🦩 🟠 getRemoteAccessConnectionStatus references undeclared v for hostname slicing
In the getRemoteAccessConnectionStatus sendCommand callback, changed var hostname = v.slice(14, header.Data.readUInt16LE(12) + 14).toString() to var hostname = header.Data.slice(14, header.Data.readUInt16LE(12) + 14).toString();, replacing the undeclared v with header.Data as suggested, and added the missing trailing semicolon for consistency with surrounding style.
🤖 Prompt for AI agents
In modules/amt_heci.js around line 288, review and complete this code-review fix: getRemoteAccessConnectionStatus references undeclared `v` for hostname slicing.
What the draft fix changed: In the getRemoteAccessConnectionStatus sendCommand callback, changed `var hostname = v.slice(14, header.Data.readUInt16LE(12) + 14).toString()` to `var hostname = header.Data.slice(14, header.Data.readUInt16LE(12) + 14).toString();`, replacing the undeclared `v` with `header.Data` as suggested, and added the missing trailing semicolon for consistency with surrounding style.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -141,7 +141,7 @@ function windows_registry() | |||
| retVal = this._AdvApi.RegQueryInfoKeyW(h.Deref(), achClass, achClassSize, 0, | |||
There was a problem hiding this comment.
🦩 🔴 windows_registry.QueryKey leaks registry handle when RegQueryInfoKeyW fails during subkey enumeration
In QueryKey, in the subkey/value enumeration branch, added this._AdvApi.RegCloseKey(h.Deref()); before the throw on the RegQueryInfoKeyW error path (if (retVal.Val != 0) { ... }), matching the pattern used by other error paths in the function so the HKEY handle acquired by RegOpenKeyExW is released before throwing.
🤖 Prompt for AI agents
In modules/win-registry.js around line 141, review and complete this code-review fix: windows_registry.QueryKey leaks registry handle when RegQueryInfoKeyW fails during subkey enumeration.
What the draft fix changed: In `QueryKey`, in the subkey/value enumeration branch, added `this._AdvApi.RegCloseKey(h.Deref());` before the `throw` on the `RegQueryInfoKeyW` error path (`if (retVal.Val != 0) { ... }`), matching the pattern used by other error paths in the function so the HKEY handle acquired by `RegOpenKeyExW` is released before throwing.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -203,11 +203,12 @@ function windows_registry() | |||
| v = this._AdvApi.RegQueryInfoKeyW(h.Deref(), achClass, achClassSize, 0, | |||
There was a problem hiding this comment.
🦩 🔴 QueryKeyLastModified leaks registry handle on RegQueryInfoKeyW or FileTimeToSystemTime failure
In QueryKeyLastModified, added this._AdvApi.RegCloseKey(h.Deref()); before the throw on both the RegQueryInfoKeyW failure (v.Val != 0) and the FileTimeToSystemTime failure, and added a RegCloseKey call before the final successful return, so the handle opened via RegOpenKeyExW is closed on every exit path of the function.
🤖 Prompt for AI agents
In modules/win-registry.js around line 203, review and complete this code-review fix: QueryKeyLastModified leaks registry handle on RegQueryInfoKeyW or FileTimeToSystemTime failure.
What the draft fix changed: In `QueryKeyLastModified`, added `this._AdvApi.RegCloseKey(h.Deref());` before the `throw` on both the `RegQueryInfoKeyW` failure (`v.Val != 0`) and the `FileTimeToSystemTime` failure, and added a `RegCloseKey` call before the final successful `return`, so the handle opened via `RegOpenKeyExW` is closed on every exit path of the function.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 88 medium — react 👍/👎 to teach the reviewer
| @@ -21,6 +21,8 @@ function parse(exePath) | |||
| { | |||
| var retVal = {}; | |||
There was a problem hiding this comment.
🦩 🟠 File descriptor opened with fs.openSync in PE_Parser.js parse() is never closed on thrown-error paths
In parse() (modules/PE_Parser.js), wrapped the entire function body after fs.openSync(exePath, 'rb') in a try/finally block, moving fs.closeSync(fd) into the finally clause and removing the duplicate closeSync call from the success path (replaced with a plain return inside try). This guarantees the file descriptor is closed on all thrown-error paths ('unrecognized binary format', 'not a PE file', 'Unknown Value found for Optional Magic') as well as on the normal success path. Indentation of the body was left unchanged aside from the added try/finally wrapper lines to minimize diff noise.
🤖 Prompt for AI agents
In modules/PE_Parser.js around line 22, review and complete this code-review fix: File descriptor opened with fs.openSync in PE_Parser.js parse() is never closed on thrown-error paths.
What the draft fix changed: In parse() (modules/PE_Parser.js), wrapped the entire function body after fs.openSync(exePath, 'rb') in a try/finally block, moving fs.closeSync(fd) into the finally clause and removing the duplicate closeSync call from the success path (replaced with a plain return inside try). This guarantees the file descriptor is closed on all thrown-error paths ('unrecognized binary format', 'not a PE file', 'Unknown Value found for Optional Magic') as well as on the normal success path. Indentation of the body was left unchanged aside from the added try/finally wrapper lines to minimize diff noise.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| MeshAgentHost_BatteryInfo_LOW = 2, | ||
| MeshAgentHost_BatteryInfo_CRITICAL = 4, | ||
| MeshAgentHost_BatteryInfo_CHARGING = 8, | ||
| MeshAgentHost_BatteryInfo_NONE = 3, | ||
| MeshAgentHost_BatteryInfo_NONE = 0x10, | ||
| MeshAgentHost_BatteryInfo_UNKNOWN = 0, | ||
| }MeshAgentHost_BatteryInfo; | ||
|
|
There was a problem hiding this comment.
🦩 🟠 MeshAgentHost_BatteryInfo enum has duplicate/overlapping bit values breaking bitmask semantics
Changed MeshAgentHost_BatteryInfo_NONE from 3 (which overlapped with HIGH|LOW bits) to 0x10, a distinct non-overlapping bit value, in the MeshAgentHost_BatteryInfo enum definition in meshcore/agentcore.h. This preserves NONE as a distinct flag usable in bitmask checks without colliding with HIGH(1)/LOW(2)/CRITICAL(4)/CHARGING(8), while leaving UNKNOWN(0) unchanged for the "no bits set" case. Risk: this is a header-only enum value change; any code elsewhere (not visible in this file) comparing against the literal value 3 for NONE, or relying on MeshAgentHost_BatteryInfo_STRINGS array indexing by these enum values, could break if it assumed NONE occupied a specific position/value — a complete fix would require auditing all usages of MeshAgentHost_BatteryInfo_NONE and the MeshAgentHost_BatteryInfo_STRINGS array across the codebase.
🤖 Prompt for AI agents
In meshcore/agentcore.h around line 161, review and complete this code-review fix: MeshAgentHost_BatteryInfo enum has duplicate/overlapping bit values breaking bitmask semantics.
What the draft fix changed: Changed `MeshAgentHost_BatteryInfo_NONE` from `3` (which overlapped with HIGH|LOW bits) to `0x10`, a distinct non-overlapping bit value, in the `MeshAgentHost_BatteryInfo` enum definition in `meshcore/agentcore.h`. This preserves NONE as a distinct flag usable in bitmask checks without colliding with HIGH(1)/LOW(2)/CRITICAL(4)/CHARGING(8), while leaving UNKNOWN(0) unchanged for the "no bits set" case. Risk: this is a header-only enum value change; any code elsewhere (not visible in this file) comparing against the literal value `3` for NONE, or relying on `MeshAgentHost_BatteryInfo_STRINGS` array indexing by these enum values, could break if it assumed NONE occupied a specific position/value — a complete fix would require auditing all usages of `MeshAgentHost_BatteryInfo_NONE` and the `MeshAgentHost_BatteryInfo_STRINGS` array across the codebase.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| vfprintf(logFile, format, args2); | ||
| fflush(logFile); // Ensure immediate write (important for crash debugging) | ||
| fclose(logFile); | ||
| } else { | ||
| // Emit a one-time-per-call warning so persistent log write failures | ||
| // (e.g. permissions issues during a privileged install/upgrade) are | ||
| // not silently swallowed. | ||
| fprintf(stderr, "mesh_log_message: failed to open log file '%s' for writing: %s\n", | ||
| MESH_LOG_FILE, strerror(errno)); | ||
| } | ||
| va_end(args2); | ||
| } | ||
|
|
||
| FILE>>> | ||
| <<<NOTES | ||
| 1. CONFIDENCE: 70 - In `mesh_log_message` (meshcore/MacOS/mac_logging_utils.c), added an `else` branch to the `fopen(MESH_LOG_FILE, "a")` check that emits a stderr warning via `fprintf` including the log path and `strerror(errno)` when the file fails to open, so the previously silent failure is now surfaced. This requires `<string.h>` (for `strerror`) and `<errno.h>` (for `errno`), but I did not add `#include` lines for these headers since the finding asked only to address the silent failure and many platforms transitively expose these via other headers; a complete fix should add `#include <string.h>` and `#include <errno.h>` explicitly to guarantee portability/compilation correctness. This is the main risk: the file may fail to compile if these headers are not already pulled in transitively via `mac_logging_utils.h` or `stdio.h`/`stdarg.h`. |
There was a problem hiding this comment.
🦩 🟠 mesh_log_message silently drops log file open failures
In mesh_log_message (meshcore/MacOS/mac_logging_utils.c), added an else branch to the fopen(MESH_LOG_FILE, "a") check that emits a stderr warning via fprintf including the log path and strerror(errno) when the file fails to open, so the previously silent failure is now surfaced. This requires <string.h> (for strerror) and <errno.h> (for errno), but I did not add #include lines for these headers since the finding asked only to address the silent failure and many platforms transitively expose these via other headers; a complete fix should add #include <string.h> and #include <errno.h> explicitly to guarantee portability/compilation correctness. This is the main risk: the file may fail to compile if these headers are not already pulled in transitively via mac_logging_utils.h or stdio.h/stdarg.h.
FILE>>>
🤖 Prompt for AI agents
In meshcore/MacOS/mac_logging_utils.c around line 28, review and complete this code-review fix: mesh_log_message silently drops log file open failures.
What the draft fix changed: In `mesh_log_message` (meshcore/MacOS/mac_logging_utils.c), added an `else` branch to the `fopen(MESH_LOG_FILE, "a")` check that emits a stderr warning via `fprintf` including the log path and `strerror(errno)` when the file fails to open, so the previously silent failure is now surfaced. This requires `<string.h>` (for `strerror`) and `<errno.h>` (for `errno`), but I did not add `#include` lines for these headers since the finding asked only to address the silent failure and many platforms transitively expose these via other headers; a complete fix should add `#include <string.h>` and `#include <errno.h>` explicitly to guarantee portability/compilation correctness. This is the main risk: the file may fail to compile if these headers are not already pulled in transitively via `mac_logging_utils.h` or `stdio.h`/`stdarg.h`.
FILE>>>
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer
| @@ -89,10 +89,10 @@ function linux_identifiers() | |||
| } | |||
| } catch (xx) { } | |||
| } else { | |||
There was a problem hiding this comment.
🦩 🟠 linux_identifiers() throws bare strings instead of Error objects, breaking stack traces and catch(Error) handling upstream
In linux_identifiers(), changed throw('Unknown board'); to throw new Error('Unknown board'); and throw ('this platform does not have DMI statistics'); to throw new Error('this platform does not have DMI statistics');. Both raw string throws are now proper Error objects with stack traces, matching the finding's suggested fix exactly. No other throws (e.g. the unrelated default-platform throw ('Unsupported Platform') outside this function) were touched, per the instruction to change only what the finding requires.
🤖 Prompt for AI agents
In modules/identifiers.js around line 91, review and complete this code-review fix: linux_identifiers() throws bare strings instead of Error objects, breaking stack traces and catch(Error) handling upstream.
What the draft fix changed: In `linux_identifiers()`, changed `throw('Unknown board');` to `throw new Error('Unknown board');` and `throw ('this platform does not have DMI statistics');` to `throw new Error('this platform does not have DMI statistics');`. Both raw string throws are now proper Error objects with stack traces, matching the finding's suggested fix exactly. No other throws (e.g. the unrelated default-platform `throw ('Unsupported Platform')` outside this function) were touched, per the instruction to change only what the finding requires.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| var info = JSON.parse(child.stdout.str.trim()); | ||
| return (info); | ||
| } catch (e) { | ||
| // Fallback: return default values if parsing fails | ||
| return({ ac: 1, level: 100 }); | ||
| // Parsing failed; surface the error instead of fabricating plausible-looking data | ||
| this.emit('error', new Error('power-monitor: failed to parse pmset battery output: ' + e.message + ' (stdout=' + JSON.stringify(child.stdout.str.trim()) + ', stderr=' + JSON.stringify(child.stderr.str.trim()) + ')')); | ||
| return (null); | ||
| } | ||
| }; | ||
| this._batteryLevelCheck = function _batteryLevelCheck() | ||
| { | ||
| var newLevel = this._getBatteryLevel(); | ||
| if (newLevel == null) { return; } | ||
| if (newLevel.ac != this._ACState) | ||
| { | ||
| this._ACState = newLevel.ac; |
There was a problem hiding this comment.
🦩 🟠 power-monitor.js macOS battery-level JSON parse silently masks failures with fabricated defaults
In _getBatteryLevel() (darwin branch), the catch (e) block no longer returns the fabricated { ac: 1, level: 100 } default; instead it calls this.emit('error', new Error(...)) with diagnostic details (parse error message, raw stdout/stderr) and returns null. Callers were updated accordingly: _batteryLevelCheck() now checks if (newLevel == null) { return; } before using the result, and the initial var tmp = this._getBatteryLevel(); call at construction time now guards with if (tmp != null) { ... } before assigning _ACState/_BatteryLevel, leaving them at their prior defaults (1 / -1) on parse failure rather than silently reporting a fully-charged AC state. Unverified: there is no formal 'error' event consumer/listener requirement enforced elsewhere in the codebase, so if no listener is attached, Node's default EventEmitter behavior for an unhandled 'error' event (throwing) could crash the process — a complete fix might instead use a dedicated diagnostic/log call (e.g. ILibRemoteLogging as suggested) instead of 'error', or ensure a default no-op error listener exists somewhere in this module.
🤖 Prompt for AI agents
In modules/power-monitor.js around line 145, review and complete this code-review fix: power-monitor.js macOS battery-level JSON parse silently masks failures with fabricated defaults.
What the draft fix changed: In `_getBatteryLevel()` (darwin branch), the `catch (e)` block no longer returns the fabricated `{ ac: 1, level: 100 }` default; instead it calls `this.emit('error', new Error(...))` with diagnostic details (parse error message, raw stdout/stderr) and returns `null`. Callers were updated accordingly: `_batteryLevelCheck()` now checks `if (newLevel == null) { return; }` before using the result, and the initial `var tmp = this._getBatteryLevel();` call at construction time now guards with `if (tmp != null) { ... }` before assigning `_ACState`/`_BatteryLevel`, leaving them at their prior defaults (1 / -1) on parse failure rather than silently reporting a fully-charged AC state. Unverified: there is no formal 'error' event consumer/listener requirement enforced elsewhere in the codebase, so if no listener is attached, Node's default EventEmitter behavior for an unhandled 'error' event (throwing) could crash the process — a complete fix might instead use a dedicated diagnostic/log call (e.g. ILibRemoteLogging as suggested) instead of 'error', or ensure a default no-op error listener exists somewhere in this module.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 56 review findings across 31 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
Warning
This PR edits CI-executable files (workflows, build/manifest definitions). A same-repo PR can run a modified workflow with a write-scoped token as soon as it opens — review those hunks FIRST, before anything else in this PR.
dataandcallbackvariablesmodules/amt_heci.js:265optinstead ofoptionalmodules/amt_heci.js:298vfor hostname slicingmodules/amt_heci.js:288modules/win-registry.js:141modules/win-registry.js:203modules/win-volumes.js:51modules/win-volumes.js:63meshcore/KVM/Linux/linux_compression.c:121meshcore/KVM/Linux/linux_compression.c:31modules/code-utils.js:422modules/code-utils.js:406modules/code-utils.js:278meshconsole/main.c:335meshconsole/main.c:363meshconsole/main.c:448xinfoleaks into global scopemodules/clipboard.js:152xinfobug as dispatchReadmodules/clipboard.js:212modules/clipboard.js:382modules/win-console.js:124modules/win-console.js:131modules/win-console.js:124.github/workflows/build-openssl-bsd.yml:147.github/workflows/build-openssl-bsd.yml:72meshcore/signcheck.c:38meshcore/signcheck.c:90modules/amt-xml.js:73modules/amt-xml.js:73samples/webrtc/C# Sample/SimpleRendezvousServer.cs:149samples/webrtc/C# Sample/SimpleRendezvousServer.cs:123modules/lme_heci.js:143modules/lme_heci.js:2modules/zip-reader.js:45modules/zip-reader.js:76this.bufferinstead of the MemoryStream's accumulated buffermodules/wifi-scanner.js:81tokens(missing var)modules/wifi-scanner.js:84.github/workflows/build-openssl-linux.yml:265.github/workflows/build-openssl-linux.yml:249.github/workflows/build-openssl-windows.yml:50.github/workflows/build-openssl-windows.yml:128modules/win-deskutils.js:65modules/win-deskutils.js:86microscript/ILibDuktape_EncryptionStream.c:52microscript/ILibDuktape_EncryptionStream.c:42modules/win-com.js:45modules/win-com.js:77modules/amt-wsman-duk.js:33modules/amt-wsman.js:32modules/upnp.js:270Kernel32leaks into global scope inside service-host.js finalizermodules/service-host.js:111modules/utils/win-kblayout.js:96modules/AgentHashTool.js:56modules/PE_Parser.js:22meshcore/agentcore.h:161meshcore/MacOS/mac_logging_utils.c:28modules/identifiers.js:91modules/power-monitor.js:145What 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:
3f8dcc8a-490a-435f-9b5e-47e003ea63b7Merging 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-86akdypw4 Ad hoc sweep fixes across services (14 PRs)