Skip to content

fix(adhoc-sweep-fixes): CU-86akhf8u3 15 review findings across 10 files #122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

fix(adhoc-sweep-fixes): CU-86akhf8u3 15 review findings across 10 files #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3eb4107
fix(adhoc-sweep-fixes): 15 review findings across 10 files
flamingo[bot] Sep 14, 2026
9094d22
fix(adhoc-sweep-fixes): 15 review findings across 10 files
flamingo[bot] Sep 14, 2026
4d480ca
fix(adhoc-sweep-fixes): 15 review findings across 10 files
flamingo[bot] Sep 14, 2026
4f07439
fix(adhoc-sweep-fixes): 15 review findings across 10 files
flamingo[bot] Sep 14, 2026
9e17da5
fix(adhoc-sweep-fixes): 15 review findings across 10 files
flamingo[bot] Sep 14, 2026
59e9821
fix(adhoc-sweep-fixes): 15 review findings across 10 files
flamingo[bot] Sep 14, 2026
4e2d6ef
fix(adhoc-sweep-fixes): 15 review findings across 10 files
flamingo[bot] Sep 14, 2026
30fa2a0
fix(adhoc-sweep-fixes): 15 review findings across 10 files
flamingo[bot] Sep 14, 2026
e552c87
fix(adhoc-sweep-fixes): 15 review findings across 10 files
flamingo[bot] Sep 14, 2026
12ebf0b
fix(adhoc-sweep-fixes): 15 review findings across 10 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
12 changes: 11 additions & 1 deletion .github/workflows/build-openssl-windows.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,18 @@ jobs:
VERSION="${{ github.event.inputs.openssl_version || '3.5.4' }}"
BUILD_DIR="openssl-build-${{ matrix.arch.name }}-${{ matrix.config.suffix }}"
curl -L "https://github.com/openssl/openssl/releases/download/openssl-$VERSION/openssl-$VERSION.tar.gz" -o openssl.tar.gz
curl -L "https://github.com/openssl/openssl/releases/download/openssl-$VERSION/openssl-$VERSION.tar.gz.sha256" -o openssl.tar.gz.sha256
EXPECTED_SHA256="$(cat openssl.tar.gz.sha256 | tr -d '[:space:]')"
ACTUAL_SHA256="$(sha256sum openssl.tar.gz | awk '{print $1}')"
if [ -z "$EXPECTED_SHA256" ] || [ "$EXPECTED_SHA256" != "$ACTUAL_SHA256" ]; then
echo "Checksum verification failed for openssl-$VERSION.tar.gz"
echo "Expected: $EXPECTED_SHA256"
echo "Actual: $ACTUAL_SHA256"
exit 1
fi
mkdir -p "$BUILD_DIR"
tar -xzf openssl.tar.gz -C "$BUILD_DIR" --strip-components=1
rm openssl.tar.gz
rm openssl.tar.gz openssl.tar.gz.sha256

- name: Configure OpenSSL
shell: cmd
Comment on lines 50 to 67

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🟠 build-openssl-windows.yml downloads and builds unpinned OpenSSL source via HTTPS with no checksum verification

In the "Download OpenSSL source" step of the build-windows job (.github/workflows/build-openssl-windows.yml), added a download of the corresponding .sha256 checksum file published alongside each OpenSSL GitHub release tarball, computed the actual SHA-256 of the downloaded tarball with sha256sum, and added a comparison that fails the step (exit 1) if the expected and actual checksums don't match or the expected checksum is empty, before extraction proceeds. This mitigates a corrupted/MITM'd download producing poisoned libs. Unverified: I have not confirmed that OpenSSL's GitHub release assets actually include a .sha256 file at that exact URL path for every historical/future release (OpenSSL publishes SHA256 sums on openssl.org's download page and in some release notes, but the exact asset filename convention on GitHub Releases was not confirmed against the live release assets); if that asset does not exist for a given version, this step will fail closed (safe, but may block otherwise-valid builds) rather than silently skip verification. A more complete fix would pin the hash value explicitly per version (e.g., in a checked-in mapping) or verify a GPG signature against OpenSSL's published signing key rather than relying on a co-located hash file fetched over the same channel.

🤖 Prompt for AI agents 
In .github/workflows/build-openssl-windows.yml around line 47, review and complete this code-review fix: build-openssl-windows.yml downloads and builds unpinned OpenSSL source via HTTPS with no checksum verification.
What the draft fix changed: In the "Download OpenSSL source" step of the `build-windows` job (`.github/workflows/build-openssl-windows.yml`), added a download of the corresponding `.sha256` checksum file published alongside each OpenSSL GitHub release tarball, computed the actual SHA-256 of the downloaded tarball with `sha256sum`, and added a comparison that fails the step (`exit 1`) if the expected and actual checksums don't match or the expected checksum is empty, before extraction proceeds. This mitigates a corrupted/MITM'd download producing poisoned libs. Unverified: I have not confirmed that OpenSSL's GitHub release assets actually include a `.sha256` file at that exact URL path for every historical/future release (OpenSSL publishes SHA256 sums on openssl.org's download page and in some release notes, but the exact asset filename convention on GitHub Releases was not confirmed against the live release assets); if that asset does not exist for a given version, this step will fail closed (safe, but may block otherwise-valid builds) rather than silently skip verification. A more complete fix would pin the hash value explicitly per version (e.g., in a checked-in mapping) or verify a GPG signature against OpenSSL's published signing key rather than relying on a co-located hash file fetched over the same channel.
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

Expand Down Expand Up @@ -185,3 +194,4 @@ jobs:
name: build-report
path: BUILD_REPORT.md
retention-days: 90

7 changes: 4 additions & 3 deletions meshcore/KVM/MacOS/mac_tile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
*
*/

#ifndef LINUX_TILE_H_

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🟠 Include guard macro name mismatches actual file (LINUX_TILE_H_ in mac_tile.h)

Changed the include guard macro in meshcore/KVM/MacOS/mac_tile.h from LINUX_TILE_H_ (both the #ifndef/#define pair at the top and the trailing #endif comment) to MAC_TILE_H_, matching the actual filename and eliminating the guard collision risk with linux_tile.h.

🤖 Prompt for AI agents 
In meshcore/KVM/MacOS/mac_tile.h around line 10, review and complete this code-review fix: Include guard macro name mismatches actual file (LINUX_TILE_H_ in mac_tile.h).
What the draft fix changed: Changed the include guard macro in meshcore/KVM/MacOS/mac_tile.h from `LINUX_TILE_H_` (both the `#ifndef`/`#define` pair at the top and the trailing `#endif` comment) to `MAC_TILE_H_`, matching the actual filename and eliminating the guard collision risk with linux_tile.h.
Verify the change is correct and complete; do not refactor unrelated code.

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

#define LINUX_TILE_H_
#ifndef MAC_TILE_H_
#define MAC_TILE_H_

#include <stdio.h>
#include <stdlib.h>
Expand Down Expand Up @@ -37,5 +37,6 @@ extern int getScreenBuffer(unsigned char **desktop, long long *desktopsize, CGIm
extern void set_tile_compression(int type, int level);


#endif /* LINUX_TILE_H_ */
#endif /* MAC_TILE_H_ */


3 changes: 2 additions & 1 deletion microstack/ILibMulticastSocket.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ void ILibMulticastSocket_BroadcastUdpPacketV4(struct ILibMulticastSocket_StateMo
#ifndef NACL
if (module->UDPServers[i] != NULL)
{
socket = ILibAsyncUDPSocket_GetSocket(module->UDPServer);
socket = ILibAsyncUDPSocket_GetSocket(module->UDPServers[i]);
setsockopt(socket, IPPROTO_IP, IP_MULTICAST_IF, (const char*)&(module->AddressListV4[i].sin_addr), sizeof(struct in_addr));
setsockopt(socket, IPPROTO_IP, IP_MULTICAST_TTL, (const char*)&(module->TTL), sizeof(int));
for (j = 0; j < count; j++) sendto(socket, data, datalen, 0, (struct sockaddr*)addr, sizeof(struct sockaddr_in));
Comment on lines 214 to 220

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🟠 ILibMulticastSocket_BroadcastUdpPacketV4 uses module->UDPServer instead of the per-interface module->UDPServers[i] socket

In ILibMulticastSocket_BroadcastUdpPacketV4, inside the for(i = 0; i < module->AddressListLengthV4; ++i) loop, changed socket = ILibAsyncUDPSocket_GetSocket(module->UDPServer); to socket = ILibAsyncUDPSocket_GetSocket(module->UDPServers[i]); so the per-interface socket (already null-checked via module->UDPServers[i] != NULL) is used for setting IP_MULTICAST_IF/IP_MULTICAST_TTL and for sendto, matching the suggested fix exactly.

🤖 Prompt for AI agents 
In microstack/ILibMulticastSocket.c around line 213, review and complete this code-review fix: ILibMulticastSocket_BroadcastUdpPacketV4 uses module->UDPServer instead of the per-interface module->UDPServers[i] socket.
What the draft fix changed: In `ILibMulticastSocket_BroadcastUdpPacketV4`, inside the `for(i = 0; i < module->AddressListLengthV4; ++i)` loop, changed `socket = ILibAsyncUDPSocket_GetSocket(module->UDPServer);` to `socket = ILibAsyncUDPSocket_GetSocket(module->UDPServers[i]);` so the per-interface socket (already null-checked via `module->UDPServers[i] != NULL`) is used for setting `IP_MULTICAST_IF`/`IP_MULTICAST_TTL` and for `sendto`, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down Expand Up @@ -397,3 +397,4 @@ void ILibMulticastSocket_WakeOnLan(void *module, char* mac)
ILibMulticastSocket_Broadcast((struct ILibMulticastSocket_StateModule*)module, ILibScratchPad, 102, 1);
}
}

6 changes: 5 additions & 1 deletion modules/RecoveryCore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ function getOpenFrameMachineId() {
? '/Library/Application Support/OpenFrame/machine_id'
: '/var/lib/openframe/machine_id');
openframeMachineId = fs.readFileSync(machineIdPath).toString().trim();
} catch (ex) { openframeMachineId = null; }
} catch (ex) {
openframeMachineId = null;
try { sendConsoleText('getOpenFrameMachineId error: ' + ex); } catch (ex2) { console.log('getOpenFrameMachineId error: ' + ex); }
}
return openframeMachineId;
}

Comment on lines 17 to 26

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🟠 getOpenFrameMachineId silently swallows all errors including unexpected exceptions

In getOpenFrameMachineId() (modules/RecoveryCore.js), the catch block now logs the exception before discarding it, via sendConsoleText (with a console.log fallback in case sendConsoleText itself throws, e.g. if MeshAgent isn't ready yet), so unexpected exceptions surface for diagnosis instead of being silently swallowed, while still degrading openframeMachineId to null so existing "file not found" behavior is preserved.

🤖 Prompt for AI agents 
In modules/RecoveryCore.js around line 10, review and complete this code-review fix: getOpenFrameMachineId silently swallows all errors including unexpected exceptions.
What the draft fix changed: In getOpenFrameMachineId() (modules/RecoveryCore.js), the catch block now logs the exception before discarding it, via sendConsoleText (with a console.log fallback in case sendConsoleText itself throws, e.g. if MeshAgent isn't ready yet), so unexpected exceptions surface for diagnosis instead of being silently swallowed, while still degrading openframeMachineId to null so existing "file not found" behavior is preserved.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down Expand Up @@ -502,3 +505,4 @@ function deleteFolderRecursive(path, rec) {
fs.unlinkSync(path);
}
};

37 changes: 1 addition & 36 deletions modules/amt-xml.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,42 +70,6 @@ function _ParseWsmanRec(node) {
return r;
}

function _PutObjToBodyXml(resuri, putObj) {

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🟠 _PutObjToBodyXml / _ObjectToXmlAttributes duplicated verbatim between amt-wsman.js and amt-xml.js

Removed the dead/broken _PutObjToBodyXml and _ObjectToXmlAttributes functions from modules/amt-xml.js. _ObjectToXmlAttributes was not shown in the provided evidence/current file content (only _PutObjToBodyXml was present in the file body), so only _PutObjToBodyXml existed to remove; it referenced the undefined obj variable causing a ReferenceError, was never called by this module's exported ParseWsman API, and duplicated the same-named function in modules/amt-wsman.js. It has been deleted entirely; no other code in this file referenced it, so nothing else needed to change.

🤖 Prompt for AI agents 
In modules/amt-xml.js around line 73, review and complete this code-review fix: _PutObjToBodyXml / _ObjectToXmlAttributes duplicated verbatim between amt-wsman.js and amt-xml.js.
What the draft fix changed: Removed the dead/broken `_PutObjToBodyXml` and `_ObjectToXmlAttributes` functions from `modules/amt-xml.js`. `_ObjectToXmlAttributes` was not shown in the provided evidence/current file content (only `_PutObjToBodyXml` was present in the file body), so only `_PutObjToBodyXml` existed to remove; it referenced the undefined `obj` variable causing a ReferenceError, was never called by this module's exported `ParseWsman` API, and duplicated the same-named function in `modules/amt-wsman.js`. It has been deleted entirely; no other code in this file referenced it, so nothing else needed to change.
Verify the change is correct and complete; do not refactor unrelated code.

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

if (!resuri || putObj == null) return '';
var objname = obj.GetNameFromUrl(resuri);
var result = '<r:' + objname + ' xmlns:r="' + resuri + '">';

for (var prop in putObj) {
if (!putObj.hasOwnProperty(prop) || prop.indexOf('__') === 0 || prop.indexOf('@') === 0) continue;
if (putObj[prop] == null || typeof putObj[prop] === 'function') continue;
if (typeof putObj[prop] === 'object' && putObj[prop]['ReferenceParameters']) {
result += '<r:' + prop + '><a:Address>' + putObj[prop].Address + '</a:Address><a:ReferenceParameters><w:ResourceURI>' + putObj[prop]['ReferenceParameters']["ResourceURI"] + '</w:ResourceURI><w:SelectorSet>';
var selectorArray = putObj[prop]['ReferenceParameters']['SelectorSet']['Selector'];
if (Array.isArray(selectorArray)) {
for (var i = 0; i < selectorArray.length; i++) {
result += '<w:Selector' + _ObjectToXmlAttributes(selectorArray[i]) + '>' + selectorArray[i]['Value'] + '</w:Selector>';
}
}
else {
result += '<w:Selector' + _ObjectToXmlAttributes(selectorArray) + '>' + selectorArray['Value'] + '</w:Selector>';
}
result += '</w:SelectorSet></a:ReferenceParameters></r:' + prop + '>';
}
else {
if (Array.isArray(putObj[prop])) {
for (var i = 0; i < putObj[prop].length; i++) {
result += '<r:' + prop + '>' + putObj[prop][i].toString() + '</r:' + prop + '>';
}
} else {
result += '<r:' + prop + '>' + putObj[prop].toString() + '</r:' + prop + '>';
}
}
}

result += '</r:' + objname + '>';
return result;
}

// This is a drop-in replacement to _turnToXml() that works without xml parser dependency.
function _treeBuilder() {
this.tree = [];
Expand Down Expand Up @@ -187,3 +151,4 @@ function _turnToXmlRec(text) {
} catch (ex) { return null; }
return lastElement;
}

16 changes: 8 additions & 8 deletions modules/amt_heci.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,32 +262,32 @@ function amt_heci() {
fn.apply(this, opt);
}, callback, optional);
}
this.startConfiguration = function () {

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🔴 amt_heci.js startConfiguration/stopConfiguration/openUserInitiatedConnection/closeUserInitiatedConnection reference undeclared data and callback

Added data, callback parameters to this.startConfiguration = function (data, callback) {...} so the this.sendCommand(0x29, data, ..., callback, optional) call resolves both identifiers from declared parameters instead of the (nonexistent) enclosing scope.

🤖 Prompt for AI agents 
In modules/amt_heci.js around line 265, review and complete this code-review fix: amt_heci.js startConfiguration/stopConfiguration/openUserInitiatedConnection/closeUserInitiatedConnection reference undeclared `data` and `callback`.
What the draft fix changed: Added `data, callback` parameters to `this.startConfiguration = function (data, callback) {...}` so the `this.sendCommand(0x29, data, ..., callback, optional)` call resolves both identifiers from declared parameters instead of the (nonexistent) enclosing scope.
Verify the change is correct and complete; do not refactor unrelated code.

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

this.startConfiguration = function (data, callback) {
var optional = [];
for (var i = 2; i < arguments.length; ++i) { optional.push(arguments[i]); }
this.sendCommand(0x29, data, function (header, fn, opt) { opt.unshift(header.Status); fn.apply(this, opt); }, callback, optional);
}
this.stopConfiguration = function () {

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🔴 amt_heci.js stopConfiguration references undeclared data and callback

Added data, callback parameters to this.stopConfiguration = function (data, callback) {...}, fixing the same undeclared-variable ReferenceError for the sendCommand(0x5E, ...) call.

🤖 Prompt for AI agents 
In modules/amt_heci.js around line 270, review and complete this code-review fix: amt_heci.js stopConfiguration references undeclared `data` and `callback`.
What the draft fix changed: Added `data, callback` parameters to `this.stopConfiguration = function (data, callback) {...}`, fixing the same undeclared-variable ReferenceError for the `sendCommand(0x5E, ...)` call.
Verify the change is correct and complete; do not refactor unrelated code.

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

this.stopConfiguration = function (data, callback) {
var optional = [];
for (var i = 2; i < arguments.length; ++i) { optional.push(arguments[i]); }
this.sendCommand(0x5E, data, function (header, fn, opt) { opt.unshift(header.Status); fn.apply(this, opt); }, callback, optional);
}
this.openUserInitiatedConnection = function () {

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🔴 amt_heci.js openUserInitiatedConnection references undeclared data and callback

Added data, callback parameters to this.openUserInitiatedConnection = function (data, callback) {...}, fixing the same undeclared-variable ReferenceError for the sendCommand(0x44, ...) call.

🤖 Prompt for AI agents 
In modules/amt_heci.js around line 275, review and complete this code-review fix: amt_heci.js openUserInitiatedConnection references undeclared `data` and `callback`.
What the draft fix changed: Added `data, callback` parameters to `this.openUserInitiatedConnection = function (data, callback) {...}`, fixing the same undeclared-variable ReferenceError for the `sendCommand(0x44, ...)` call.
Verify the change is correct and complete; do not refactor unrelated code.

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

this.openUserInitiatedConnection = function (data, callback) {
var optional = [];
for (var i = 2; i < arguments.length; ++i) { optional.push(arguments[i]); }
this.sendCommand(0x44, data, function (header, fn, opt) { opt.unshift(header.Status); fn.apply(this, opt); }, callback, optional);
}
this.closeUserInitiatedConnection = function () {

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🔴 amt_heci.js closeUserInitiatedConnection references undeclared data and callback

Added data, callback parameters to this.closeUserInitiatedConnection = function (data, callback) {...}, fixing the same undeclared-variable ReferenceError for the sendCommand(0x45, ...) call.

🤖 Prompt for AI agents 
In modules/amt_heci.js around line 280, review and complete this code-review fix: amt_heci.js closeUserInitiatedConnection references undeclared `data` and `callback`.
What the draft fix changed: Added `data, callback` parameters to `this.closeUserInitiatedConnection = function (data, callback) {...}`, fixing the same undeclared-variable ReferenceError for the `sendCommand(0x45, ...)` call.
Verify the change is correct and complete; do not refactor unrelated code.

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

this.closeUserInitiatedConnection = function (data, callback) {
var optional = [];
for (var i = 2; i < arguments.length; ++i) { optional.push(arguments[i]); }
this.sendCommand(0x45, data, function (header, fn, opt) { opt.unshift(header.Status); fn.apply(this, opt); }, callback, optional);
}
this.getRemoteAccessConnectionStatus = function () {

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🔴 amt_heci.js getRemoteAccessConnectionStatus references undeclared data, callback, and v

In this.getRemoteAccessConnectionStatus, added data, callback parameters to the outer function signature (fixing the same sendCommand(0x46, data, ..., callback, optional) bug as above), and replaced v.slice(14, ...) with header.Data.slice(14, ...) inside the response callback, since v was never defined in that closure and header.Data is the buffer being parsed elsewhere in the same callback (matches the field offsets used for networkStatus/remoteAccessStatus/remoteAccessTrigger a few lines below). This assumes header.Data is the correct source buffer for the hostname bytes, consistent with how it's used for all other fields in that same object literal.

🤖 Prompt for AI agents 
In modules/amt_heci.js around line 285, review and complete this code-review fix: amt_heci.js getRemoteAccessConnectionStatus references undeclared `data`, `callback`, and `v`.
What the draft fix changed: In `this.getRemoteAccessConnectionStatus`, added `data, callback` parameters to the outer function signature (fixing the same `sendCommand(0x46, data, ..., callback, optional)` bug as above), and replaced `v.slice(14, ...)` with `header.Data.slice(14, ...)` inside the response callback, since `v` was never defined in that closure and `header.Data` is the buffer being parsed elsewhere in the same callback (matches the field offsets used for `networkStatus`/`remoteAccessStatus`/`remoteAccessTrigger` a few lines below). This assumes `header.Data` is the correct source buffer for the hostname bytes, consistent with how it's used for all other fields in that same object literal.
Verify the change is correct and complete; do not refactor unrelated code.

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

this.getRemoteAccessConnectionStatus = function (data, callback) {
var optional = [];
for (var i = 2; i < arguments.length; ++i) { optional.push(arguments[i]); }
this.sendCommand(0x46, data, function (header, fn, opt) {
if (header.Status == 0) {
var hostname = v.slice(14, header.Data.readUInt16LE(12) + 14).toString()
var hostname = header.Data.slice(14, header.Data.readUInt16LE(12) + 14).toString()
opt.unshift({ status: header.Status, networkStatus: header.Data.readUInt32LE(0), remoteAccessStatus: header.Data.readUInt32LE(4), remoteAccessTrigger: header.Data.readUInt32LE(8), mpsHostname: hostname, raw: header.Data });
} else {
opt.unshift({ status: header.Status });
Expand All @@ -297,7 +297,7 @@ function amt_heci() {
}
this.getProtocolVersion = function (callback) {

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🔴 amt_heci.js getProtocolVersion references undeclared opt instead of optional

In this.getProtocolVersion, changed opt.push(arguments[i]) to optional.push(arguments[i]) in the arguments-collecting loop so it populates the actually-declared optional array instead of the undeclared opt.

🤖 Prompt for AI agents 
In modules/amt_heci.js around line 298, review and complete this code-review fix: amt_heci.js getProtocolVersion references undeclared `opt` instead of `optional`.
What the draft fix changed: In `this.getProtocolVersion`, changed `opt.push(arguments[i])` to `optional.push(arguments[i])` in the arguments-collecting loop so it populates the actually-declared `optional` array instead of the undeclared `opt`.
Verify the change is correct and complete; do not refactor unrelated code.

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

var optional = [];
for (var i = 1; i < arguments.length; ++i) { opt.push(arguments[i]); }
for (var i = 1; i < arguments.length; ++i) { optional.push(arguments[i]); }

heci.doIoctl(heci.IOCTL.HECI_VERSION, Buffer.alloc(5), Buffer.alloc(5), function (status, buffer, self, fn, opt) {
if (status == 0) {
Expand All @@ -313,4 +313,4 @@ function amt_heci() {
}
}

module.exports = amt_heci;
module.exports = amt_heci;
3 changes: 2 additions & 1 deletion modules/lme_heci.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ function lme_heci()
break;
case APF_SERVICE_REQUEST:
var nameLen = chunk.readUInt32BE(1);

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🟠 Buffer comparison with == instead of string equality check may never match

In the APF_SERVICE_REQUEST case of lme_heci()'s data handler, changed var name = chunk.slice(5, nameLen + 5); to var name = chunk.slice(5, nameLen + 5).toString();, matching the pattern already used in the APF_GLOBAL_REQUEST handler. This makes name a string so the subsequent == comparisons against 'pfwd@amt.intel.com' and 'auth@amt.intel.com' work correctly, restoring APF_SERVICE_ACCEPT negotiation.

🤖 Prompt for AI agents 
In modules/lme_heci.js around line 143, review and complete this code-review fix: Buffer comparison with == instead of string equality check may never match.
What the draft fix changed: In the APF_SERVICE_REQUEST case of lme_heci()'s data handler, changed `var name = chunk.slice(5, nameLen + 5);` to `var name = chunk.slice(5, nameLen + 5).toString();`, matching the pattern already used in the APF_GLOBAL_REQUEST handler. This makes `name` a string so the subsequent `==` comparisons against `'pfwd@amt.intel.com'` and `'auth@amt.intel.com'` work correctly, restoring APF_SERVICE_ACCEPT negotiation.
Verify the change is correct and complete; do not refactor unrelated code.

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

var name = chunk.slice(5, nameLen + 5);
var name = chunk.slice(5, nameLen + 5).toString();
//console.log("Service Request for: " + name);
if (name == 'pfwd@amt.intel.com' || name == 'auth@amt.intel.com')
{
Expand Down Expand Up @@ -353,3 +353,4 @@ function lme_heci()
}

module.exports = lme_heci;

29 changes: 27 additions & 2 deletions modules/process-manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,21 @@ function processManager() {
p.waitExit();
}

var J = JSON.parse(p.stdout.str);

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🟠 getProcesses() on linux does not check p.stdout.str for empty/parse errors before JSON.parse, risking an uncaught exception on malformed awk output

In getProcesses(), both the linux branch (primary/fallback awk pipeline output) and the darwin/freebsd branch now wrap JSON.parse(p.stdout.str) in a try/catch. On parse failure, instead of letting the exception propagate, the code builds p.args from the extra arguments (matching the existing pattern used elsewhere in this function) and invokes callback with null as the first argument (in place of the parsed process map), then breaks out of the switch case rather than continuing to process a partially-undefined J. This routes the error to the caller via the same callback-based convention already used in this function, instead of throwing synchronously. Risk: callers of getProcesses/enumerateProcesses that don't currently check for a falsy first argument will need to handle null being passed instead of a process map — this is a behavior change from "crash" to "callback(null)", which is safer but callers should be audited; I did not have visibility into every call site.

🤖 Prompt for AI agents 
In modules/process-manager.js around line 169, review and complete this code-review fix: getProcesses() on linux does not check p.stdout.str for empty/parse errors before JSON.parse, risking an uncaught exception on malformed awk output.
What the draft fix changed: In `getProcesses()`, both the linux branch (primary/fallback awk pipeline output) and the darwin/freebsd branch now wrap `JSON.parse(p.stdout.str)` in a try/catch. On parse failure, instead of letting the exception propagate, the code builds `p.args` from the extra arguments (matching the existing pattern used elsewhere in this function) and invokes `callback` with `null` as the first argument (in place of the parsed process map), then breaks out of the switch case rather than continuing to process a partially-undefined `J`. This routes the error to the caller via the same callback-based convention already used in this function, instead of throwing synchronously. Risk: callers of `getProcesses`/`enumerateProcesses` that don't currently check for a falsy first argument will need to handle `null` being passed instead of a process map — this is a behavior change from "crash" to "callback(null)", which is safer but callers should be audited; I did not have visibility into every call site.
Verify the change is correct and complete; do not refactor unrelated code.

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

try
{
var J = JSON.parse(p.stdout.str);
}
catch (parseErr)
{
if (callback)
{
p.args = [];
for (var i = 1; i < arguments.length; ++i) { p.args.push(arguments[i]); }
p.args.unshift(null);
callback.apply(this, p.args);
}
break;
}
if (callback)
{
p.args = [];
Expand Down Expand Up @@ -209,7 +223,18 @@ function processManager() {

if (callback)
{
var J = JSON.parse(p.stdout.str);
try
{
var J = JSON.parse(p.stdout.str);
}
catch (parseErr)
{
p.args = [];
for (var i = 1; i < arguments.length; ++i) { p.args.push(arguments[i]); }
p.args.unshift(null);
callback.apply(this, p.args);
break;
}
p.args = [];
for (var i = 1; i < arguments.length; ++i) { p.args.push(arguments[i]); }
if (process.platform == 'freebsd')
Expand Down
24 changes: 22 additions & 2 deletions modules/win-virtual-terminal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,29 @@ function vt()
}
else
{
k32.ClosePseudoConsole(ret._h.Deref());
k32.CloseHandle(ret._input.Deref());
k32.CloseHandle(ret._output.Deref());
k32.CloseHandle(ret._consoleInput.Deref());
k32.CloseHandle(ret._consoleOutput.Deref());
}
}

else
{
k32.ClosePseudoConsole(ret._h.Deref());
k32.CloseHandle(ret._input.Deref());
k32.CloseHandle(ret._output.Deref());
k32.CloseHandle(ret._consoleInput.Deref());
k32.CloseHandle(ret._consoleOutput.Deref());
}
}
else
{
k32.ClosePseudoConsole(ret._h.Deref());
k32.CloseHandle(ret._input.Deref());
k32.CloseHandle(ret._output.Deref());
k32.CloseHandle(ret._consoleInput.Deref());
k32.CloseHandle(ret._consoleOutput.Deref());
}
throw ('Internal Error');
}
Comment on lines 185 to 213

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🟠 CreatePseudoConsole handle and pipes leaked when CreateProcessW / UpdateProcThreadAttribute fails

In vt.Create (modules/win-virtual-terminal.js), added cleanup blocks in the else branches for the CreateProcessW failure, the UpdateProcThreadAttribute failure, and the InitializeProcThreadAttributeList failure. Each new else now calls k32.ClosePseudoConsole(ret._h.Deref()) and k32.CloseHandle() on ret._input, ret._output, ret._consoleInput, and ret._consoleOutput before falling through to throw ('Internal Error'). This addresses the leaked pseudoconsole handle and four pipe handles on every failure path. Not freed: the attrList and startupinfoex GM.CreateVariable-backed native memory blocks — these are garbage-collected buffers wrapped by the marshal layer rather than raw HeapAlloc pointers explicitly freed elsewhere in this file (no HeapFree call exists in the original code either), so no change was made there since inventing a free call risks diverging from the module's existing memory-management pattern; a reviewer familiar with the _GenericMarshal lifecycle should confirm whether HeapFree of the attrList (obtained via HeapAlloc-style semantics if any) is actually needed.

(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)

🤖 Prompt for AI agents 
In modules/win-virtual-terminal.js around line 96, review and complete this code-review fix: CreatePseudoConsole handle and pipes leaked when CreateProcessW / UpdateProcThreadAttribute fails.
What the draft fix changed: In vt.Create (modules/win-virtual-terminal.js), added cleanup blocks in the `else` branches for the CreateProcessW failure, the UpdateProcThreadAttribute failure, and the InitializeProcThreadAttributeList failure. Each new `else` now calls `k32.ClosePseudoConsole(ret._h.Deref())` and `k32.CloseHandle()` on `ret._input`, `ret._output`, `ret._consoleInput`, and `ret._consoleOutput` before falling through to `throw ('Internal Error')`. This addresses the leaked pseudoconsole handle and four pipe handles on every failure path. Not freed: the `attrList` and `startupinfoex` GM.CreateVariable-backed native memory blocks — these are garbage-collected buffers wrapped by the marshal layer rather than raw HeapAlloc pointers explicitly freed elsewhere in this file (no HeapFree call exists in the original code either), so no change was made there since inventing a free call risks diverging from the module's existing memory-management pattern; a reviewer familiar with the _GenericMarshal lifecycle should confirm whether HeapFree of the attrList (obtained via HeapAlloc-style semantics if any) is actually needed.

_(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)_
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

Expand Down Expand Up @@ -235,4 +255,4 @@ function vt()
if (process.platform == 'win32')
{
module.exports = new vt();
}
}
3 changes: 3 additions & 0 deletions modules/win-wmi.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,8 @@ function enumerateProperties(j, fields)
{
properties.push(nn.Deref().increment(i * GM.PointerSize).Deref().Wide2UTF8);
}

OleAut32.SafeArrayUnaccessData(nme.Deref());
}

// Now we need to introspect the Array Fields
Comment on lines 254 to 261

@flamingo flamingo Bot Sep 14, 2026

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.

🦩 🟠 SafeArrayAccessData success path never calls SafeArrayUnaccessData on the outer array in win-wmi.js enumerateProperties

In enumerateProperties, added OleAut32.SafeArrayUnaccessData(nme.Deref()) immediately after the loop that consumes the property-names SAFEARRAY obtained via OleAut32.SafeArrayAccessData(nme.Deref(), nn), pairing the Access/Unaccess calls as required by the SafeArray API and matching the pattern already used for the array-value branch further down in the same function.

🤖 Prompt for AI agents 
In modules/win-wmi.js around line 246, review and complete this code-review fix: SafeArrayAccessData success path never calls SafeArrayUnaccessData on the outer array in win-wmi.js enumerateProperties.
What the draft fix changed: In `enumerateProperties`, added `OleAut32.SafeArrayUnaccessData(nme.Deref())` immediately after the loop that consumes the property-names SAFEARRAY obtained via `OleAut32.SafeArrayAccessData(nme.Deref(), nn)`, pairing the Access/Unaccess calls as required by the SafeArray API and matching the pattern already used for the array-value branch further down in the same function.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down Expand Up @@ -456,3 +458,4 @@ function query(resourceString, queryString, fields)
}

module.exports = { query: query, queryAsync: queryAsync };