Skip to content

fix(MESHCENT-002-2): CU-86akhf8u3 3 review findings in kvm-helper.js - #124

Draft
flamingo[bot] wants to merge 1 commit into
masterfrom
ai-fix/meshcent-002-2-67048e44-bc788a01
Draft

flamingo[bot] wants to merge 1 commit into
masterfrom
ai-fix/meshcent-002-2-67048e44-bc788a01

Conversation

@flamingo

@flamingo flamingo Bot commented Sep 14, 2026

Copy link
Copy Markdown

Closes 3 review findings in modules/kvm-helper.js.

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 🟡 75 medium child.stdin.write() calls not wrapped in try/catch, unlike ws.send() convention elsewhere modules/kvm-helper.js:130
2 🟢 90 high getUsers() references undeclared/leaked global 'i' variable via for..in without var modules/kvm-helper.js:8
3 🟢 90 high loginUids() also leaks undeclared global loop variable 'i' modules/kvm-helper.js:111

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: bc788a01-3d0c-4679-816f-282037bb5004

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-86akhf8u3 MeshAgent review findings sweep (13 PRs)

@flamingo flamingo Bot left a comment

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.

🦩 What this fix changed, finding by finding

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

Comment thread modules/kvm-helper.js
@@ -129,25 +134,30 @@ if (process.platform == 'linux')
{
var child = require('child_process').execFile('/bin/sh', ['sh']);

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.

🦩 🔴 child.stdin.write() calls not wrapped in try/catch, unlike ws.send() convention elsewhere

Wrapped every child.stdin.write(...) / terminal.stdin.write(...) call sequence in try/catch blocks across spawnVirtualSession, waylandStatus (both gdm/gdm3 branches), disableWayland (both branches), and enableWayland (both branches) in modules/kvm-helper.js. Each block of consecutive writes to a given child process is wrapped together (rather than each call individually) to keep the change minimal while ensuring an exception from a closed/broken stdin pipe is caught rather than propagating unhandled; child.waitExit() is still called afterward in all cases to preserve existing control flow.

🤖 Prompt for AI agents
In modules/kvm-helper.js around line 130, review and complete this code-review fix: child.stdin.write() calls not wrapped in try/catch, unlike ws.send() convention elsewhere.
What the draft fix changed: Wrapped every `child.stdin.write(...)` / `terminal.stdin.write(...)` call sequence in try/catch blocks across `spawnVirtualSession`, `waylandStatus` (both gdm/gdm3 branches), `disableWayland` (both branches), and `enableWayland` (both branches) in `modules/kvm-helper.js`. Each block of consecutive writes to a given child process is wrapped together (rather than each call individually) to keep the change minimal while ensuring an exception from a closed/broken stdin pipe is caught rather than propagating unhandled; `child.waitExit()` is still called afterward in all cases to preserve existing control flow.
Verify the change is correct and complete; do not refactor unrelated code.

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

Comment thread modules/kvm-helper.js
var res, i, uu = {};
require('user-sessions').Current(function (u) { res = u; });

for (i in res)

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.

🦩 🟠 getUsers() references undeclared/leaked global 'i' variable via for..in without var

In getUsers(), changed both for (i in res) and for (i in spawnable) to for (var i in res) and for (var i in spawnable) respectively, declaring the loop variable locally instead of leaking a global i.

🤖 Prompt for AI agents
In modules/kvm-helper.js around line 8, review and complete this code-review fix: getUsers() references undeclared/leaked global 'i' variable via for..in without var.
What the draft fix changed: In `getUsers()`, changed both `for (i in res)` and `for (i in spawnable)` to `for (var i in res)` and `for (var i in spawnable)` respectively, declaring the loop variable locally instead of leaking a global `i`.
Verify the change is correct and complete; do not refactor unrelated code.

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

Comment thread modules/kvm-helper.js
{
var lids = module.exports.allowed
var uu = {};
for (i in lids)

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.

🦩 🟠 loginUids() also leaks undeclared global loop variable 'i'

In loginUids(), changed for (i in lids) to for (var i in lids), declaring the loop variable locally to match the fix applied to getUsers().

🤖 Prompt for AI agents
In modules/kvm-helper.js around line 111, review and complete this code-review fix: loginUids() also leaks undeclared global loop variable 'i'.
What the draft fix changed: In `loginUids()`, changed `for (i in lids)` to `for (var i in lids)`, declaring the loop variable locally to match the fix applied to `getUsers()`.
Verify the change is correct and complete; do not refactor unrelated code.

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

@flamingo flamingo Bot changed the title fix(MESHCENT-002-2): 3 review findings in kvm-helper.js fix(MESHCENT-002-2): CU-86akhf8u3 3 review findings in kvm-helper.js Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants