fix(MESHCENT-008-2): CU-86akn96pm child.stdout.str accumulation pattern duplicated dozens of times inline instead of a shared helper being consistently used - #153
Conversation
… dozens of times inline instead of a shared helper being consistently used
| identifiers['board_name'] = require('fs').readFileSync('/sys/firmware/devicetree/base/model').toString().trim(); | ||
| identifiers['board_serial'] = require('fs').readFileSync('/sys/firmware/devicetree/base/serial-number').toString().trim(); | ||
| const memorySlots = []; | ||
| var child = require('child_process').execFile('/bin/sh', ['sh']); |
There was a problem hiding this comment.
🦩 🟠 child.stdout.str accumulation pattern duplicated dozens of times inline instead of a shared helper being consistently used
Added a shared spawnShell(captureStderr) helper (near dataHandler) that encapsulates require('child_process').execFile('/bin/sh', ['sh']) plus the child.stdout.str = ''; child.stdout.on('data', dataHandler); (and optional stderr) boilerplate. Replaced all /bin/sh shell-spawn call sites in linux_identifiers, macos_identifiers, module.exports.isDocker, and module.exports.isBatteryPowered (darwin case) with calls to spawnShell(false) or spawnShell(true), removing the duplicated accumulation setup at each site while keeping subsequent child.stdin.write(...)/child.waitExit() logic unchanged. Sites using anonymous inline closures instead of the shared dataHandler (e.g. windows_volumes, win_chassisType, the uptime/sysctl boot-time blocks, and PowerShell child spawns) were left untouched since they are not /bin/sh shell spawns using dataHandler and converting them risks altering this binding subtleties not requested by the finding; a complete fix could further unify those but that would expand scope beyond the flagged duplication pattern.
🤖 Prompt for AI agents
In modules/identifiers.js around line 79, review and complete this code-review fix: child.stdout.str accumulation pattern duplicated dozens of times inline instead of a shared helper being consistently used.
What the draft fix changed: Added a shared `spawnShell(captureStderr)` helper (near `dataHandler`) that encapsulates `require('child_process').execFile('/bin/sh', ['sh'])` plus the `child.stdout.str = ''; child.stdout.on('data', dataHandler);` (and optional stderr) boilerplate. Replaced all `/bin/sh` shell-spawn call sites in `linux_identifiers`, `macos_identifiers`, `module.exports.isDocker`, and `module.exports.isBatteryPowered` (darwin case) with calls to `spawnShell(false)` or `spawnShell(true)`, removing the duplicated accumulation setup at each site while keeping subsequent `child.stdin.write(...)`/`child.waitExit()` logic unchanged. Sites using anonymous inline closures instead of the shared `dataHandler` (e.g. `windows_volumes`, `win_chassisType`, the uptime/sysctl boot-time blocks, and PowerShell child spawns) were left untouched since they are not `/bin/sh` shell spawns using `dataHandler` and converting them risks altering `this` binding subtleties not requested by the finding; a complete fix could further unify those but that would expand scope beyond the flagged duplication pattern.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
Closes findings from rule MESHCENT-008-2 — child.stdout.str accumulation pattern duplicated dozens of times inline instead of a shared helper being consistently used.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
modules/identifiers.js:79What 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:
06986371-2b8e-4388-aee4-5d1dc04f8838Merging 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-86akn96pm MeshAgent review findings sweep (12 PRs)