From 207f0d58cd52b3e0cf3d9e0dacf94f1f229530bc Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Fri, 4 Sep 2026 15:56:14 -0400 Subject: [PATCH 1/3] fix(daemon): extract shared memory-setting range validators (lr-553d27) Pure validateMemAvailableThresholdMB / validateTokensPerMbHeadroom functions, extracted so the web/WS setter path and the raw IPC socket path validate against one contract instead of two independently-maintained copies. --- lib/daemon.js | 74 ++++-- lib/memory-setting-validate.js | 37 +++ ...ipc-memory-setter-parity-lr-553d27.test.js | 249 ++++++++++++++++++ .../memory-setting-validate-lr-553d27.test.js | 78 ++++++ 4 files changed, 410 insertions(+), 28 deletions(-) create mode 100644 lib/memory-setting-validate.js create mode 100644 test/daemon-ipc-memory-setter-parity-lr-553d27.test.js create mode 100644 test/memory-setting-validate-lr-553d27.test.js diff --git a/lib/daemon.js b/lib/daemon.js index 75864293..e579c151 100644 --- a/lib/daemon.js +++ b/lib/daemon.js @@ -32,6 +32,7 @@ var { createWorktree, removeWorktree, isWorktree } = require("./worktree"); var { isWorktreeSlug, scanAndRegisterWorktrees, rescanWorktrees, cleanupWorktreesForParent, getFilteredRemovedProjects, registerWorktreeSlug, unregisterWorktreeSlug } = require("./daemon-projects"); var { validateCloneUrl, buildCloneArgs } = require("./clone-validate"); var { DEFAULT_MEM_AVAILABLE_MIN_MB, DEFAULT_TOKENS_PER_MB_HEADROOM, getActiveLiveCount, buildActivityDiagnosticsResponse } = require("./sdk-bridge"); +var { validateMemAvailableThresholdMB, validateTokensPerMbHeadroom } = require("./memory-setting-validate"); var { startMemoryHighWatcher, checkAppliedMemoryCeiling } = require("./memory-limits"); var { createDrain } = require("./drain"); var { shedMemory } = require("./memory-shed"); @@ -1225,34 +1226,36 @@ var relay = createServer({ return { ok: true, autoContinueOnRateLimit: want }; }, onSetMemAvailableThreshold: function (mb) { - var val = parseInt(mb, 10); // lr-93e3c8 (item 6): out-of-range used to silently reset to the // default and still report ok:true -- a UX trap on the exact knob an // operator reaches for while already blocked by this same guard. // Reject and report instead of silently substituting a value the - // caller didn't ask for. - if (isNaN(val) || val < 0) { - console.warn("[daemon] MemAvailable threshold rejected: " + mb + " (must be a number >= 0)"); - return { ok: false, error: "Value must be a number >= 0", memAvailableMinMB: config.memAvailableMinMB !== undefined ? config.memAvailableMinMB : DEFAULT_MEM_AVAILABLE_MIN_MB }; - } - config.memAvailableMinMB = val; + // caller didn't ask for. Validation lives in memory-setting-validate.js + // so this contract can't drift from the raw IPC path (lr-553d27). + var result = validateMemAvailableThresholdMB(mb); + if (!result.ok) { + console.warn("[daemon] MemAvailable threshold rejected: " + mb + " (" + result.error + ")"); + return { ok: false, error: result.error, memAvailableMinMB: config.memAvailableMinMB !== undefined ? config.memAvailableMinMB : DEFAULT_MEM_AVAILABLE_MIN_MB }; + } + config.memAvailableMinMB = result.value; saveConfig(config); - console.log("[daemon] MemAvailable threshold:", val, "MB (web)"); - return { ok: true, memAvailableMinMB: val }; + console.log("[daemon] MemAvailable threshold:", result.value, "MB (web)"); + return { ok: true, memAvailableMinMB: result.value }; }, onSetTokensPerMbHeadroom: function (tpm) { - var val = parseInt(tpm, 10); // lr-93e3c8 (item 6): valid range 10-500 tokens/MB. Out-of-range used to // silently clamp to the default and still report ok:true -- see comment - // on onSetMemAvailableThreshold above; same trap, same fix. - if (isNaN(val) || val < 10 || val > 500) { - console.warn("[daemon] Tokens per MB headroom rejected: " + tpm + " (must be 10-500)"); - return { ok: false, error: "Value must be 10-500", tokensPerMbHeadroom: config.tokensPerMbHeadroom !== undefined ? config.tokensPerMbHeadroom : DEFAULT_TOKENS_PER_MB_HEADROOM }; - } - config.tokensPerMbHeadroom = val; + // on onSetMemAvailableThreshold above; same trap, same fix. Validation + // lives in memory-setting-validate.js (lr-553d27). + var result = validateTokensPerMbHeadroom(tpm); + if (!result.ok) { + console.warn("[daemon] Tokens per MB headroom rejected: " + tpm + " (" + result.error + ")"); + return { ok: false, error: result.error, tokensPerMbHeadroom: config.tokensPerMbHeadroom !== undefined ? config.tokensPerMbHeadroom : DEFAULT_TOKENS_PER_MB_HEADROOM }; + } + config.tokensPerMbHeadroom = result.value; saveConfig(config); - console.log("[daemon] Tokens per MB headroom:", val, "(web)"); - return { ok: true, tokensPerMbHeadroom: val }; + console.log("[daemon] Tokens per MB headroom:", result.value, "(web)"); + return { ok: true, tokensPerMbHeadroom: result.value }; }, onGetToolPalettes: function () { return config.toolPalettes || {}; @@ -1726,22 +1729,37 @@ var ipc = createIPCServer(socketPath(), function (msg) { return { ok: true }; } + // lr-553d27: this case used to silently clamp an out-of-range value to + // the default and still report ok:true, diverging from the WS/web path + // (onSetMemAvailableThreshold above), which lr-93e3c8 already fixed to + // reject-and-report. This is the documented operator escape hatch used + // when the UI can't save (see architecture.md "CLI <-> Daemon"), so a + // lying ok:true here is worse than no escape hatch at all -- brought to + // parity via the SAME validator both paths now share + // (memory-setting-validate.js), instead of carrying a second, + // independently-maintained copy of the range check. case "set_mem_available_threshold": { - var memMB = parseInt(msg.value, 10); - if (isNaN(memMB) || memMB < 0) memMB = DEFAULT_MEM_AVAILABLE_MIN_MB; - config.memAvailableMinMB = memMB; + var memResult = validateMemAvailableThresholdMB(msg.value); + if (!memResult.ok) { + console.warn("[daemon] MemAvailable threshold rejected: " + msg.value + " (" + memResult.error + ") (cli)"); + return { ok: false, error: memResult.error, memAvailableMinMB: config.memAvailableMinMB !== undefined ? config.memAvailableMinMB : DEFAULT_MEM_AVAILABLE_MIN_MB }; + } + config.memAvailableMinMB = memResult.value; saveConfig(config); - console.log("[daemon] MemAvailable threshold:", memMB, "MB (cli)"); - return { ok: true, memAvailableMinMB: memMB }; + console.log("[daemon] MemAvailable threshold:", memResult.value, "MB (cli)"); + return { ok: true, memAvailableMinMB: memResult.value }; } case "set_tokens_per_mb_headroom": { - var tpmVal = parseInt(msg.value, 10); - if (isNaN(tpmVal) || tpmVal < 10 || tpmVal > 500) tpmVal = DEFAULT_TOKENS_PER_MB_HEADROOM; - config.tokensPerMbHeadroom = tpmVal; + var tpmResult = validateTokensPerMbHeadroom(msg.value); + if (!tpmResult.ok) { + console.warn("[daemon] Tokens per MB headroom rejected: " + msg.value + " (" + tpmResult.error + ") (cli)"); + return { ok: false, error: tpmResult.error, tokensPerMbHeadroom: config.tokensPerMbHeadroom !== undefined ? config.tokensPerMbHeadroom : DEFAULT_TOKENS_PER_MB_HEADROOM }; + } + config.tokensPerMbHeadroom = tpmResult.value; saveConfig(config); - console.log("[daemon] Tokens per MB headroom:", tpmVal, "(cli)"); - return { ok: true, tokensPerMbHeadroom: tpmVal }; + console.log("[daemon] Tokens per MB headroom:", tpmResult.value, "(cli)"); + return { ok: true, tokensPerMbHeadroom: tpmResult.value }; } case "enable_recovery": { diff --git a/lib/memory-setting-validate.js b/lib/memory-setting-validate.js new file mode 100644 index 00000000..b4c2e2ce --- /dev/null +++ b/lib/memory-setting-validate.js @@ -0,0 +1,37 @@ +// memory-setting-validate.js — shared range validation for the memory-guard +// config keys (lr-553d27). +// +// Extracted so the web/WS path (lib/daemon.js's onSetMemAvailableThreshold / +// onSetTokensPerMbHeadroom) and the raw IPC socket path (lib/daemon.js's +// set_mem_available_threshold / set_tokens_per_mb_headroom cases) validate +// against ONE contract instead of two independently-maintained copies. The +// divergence between those two copies is what let the raw IPC path silently +// clamp an out-of-range value to the default and report ok:true (the web +// path was fixed for this under lr-93e3c8; the raw IPC path was not) — see +// lr-553d27. +// +// Both functions return { ok: true, value } on success or +// { ok: false, error } on rejection; the error message always names the +// valid band so a caller (including the raw-socket escape-hatch documented +// to operators) can tell exactly what would be accepted. + +function validateMemAvailableThresholdMB(rawValue) { + var val = parseInt(rawValue, 10); + if (isNaN(val) || val < 0) { + return { ok: false, error: "Value must be a number >= 0" }; + } + return { ok: true, value: val }; +} + +function validateTokensPerMbHeadroom(rawValue) { + var val = parseInt(rawValue, 10); + if (isNaN(val) || val < 10 || val > 500) { + return { ok: false, error: "Value must be 10-500" }; + } + return { ok: true, value: val }; +} + +module.exports = { + validateMemAvailableThresholdMB: validateMemAvailableThresholdMB, + validateTokensPerMbHeadroom: validateTokensPerMbHeadroom, +}; diff --git a/test/daemon-ipc-memory-setter-parity-lr-553d27.test.js b/test/daemon-ipc-memory-setter-parity-lr-553d27.test.js new file mode 100644 index 00000000..432ac59f --- /dev/null +++ b/test/daemon-ipc-memory-setter-parity-lr-553d27.test.js @@ -0,0 +1,249 @@ +// daemon-ipc-memory-setter-parity-lr-553d27.test.js +// +// Regression tests for lr-553d27: the raw IPC socket cases for +// set_mem_available_threshold / set_tokens_per_mb_headroom used to silently +// CLAMP an out-of-range value to the default and report ok:true -- the same +// defect lr-93e3c8 already fixed on the WS/web path +// (onSetMemAvailableThreshold / onSetTokensPerMbHeadroom). The raw socket +// path is the documented operator escape hatch used when the UI can't save +// (see docs/guides/architecture.md "CLI <-> Daemon"), so a lying ok:true +// there is worse than no escape hatch at all -- BOBBIE judged this a +// genuine integrity issue, not cosmetic (see task comment #1). +// +// TEST DISCIPLINE (tome #845 -- "reports success while nothing happened" is +// this repo's dominant failure mode, and the FIX for each prior instance +// introduced the next): plumbing coverage ("the validator function exists +// and returns the right shape") is NOT reachability coverage ("a caller +// sending a real message over the real raw IPC socket actually receives +// that shape, and nothing was silently persisted instead"). Per repo +// convention, lib/daemon.js has no module.exports and cannot be required +// in-process (it binds real sockets/HTTP servers as a side effect of being +// loaded) -- see test/activity-diagnostics-retrieval-lr-8b476f.test.js's +// header comment for the established precedent. Unlike that probe (whose +// response-building logic already lived in a separate, requirable module), +// this defect is specifically about the CASE BODY in daemon.js's IPC +// switch, not just the validator it calls -- so genuine reachability +// coverage here means driving a REAL lib/ipc.js Unix socket server (the +// exact transport daemon.js binds) with a handler function that reproduces +// daemon.js's own case bodies verbatim (same validator import, same +// config-mutation shape, same response shape), then asserting over the +// real socket. Test 3 below then source-checks that daemon.js's actual case +// bodies match this reproduction, so the two cannot silently drift apart. + +"use strict"; + +var test = require("node:test"); +var assert = require("node:assert/strict"); +var fs = require("fs"); +var path = require("path"); +var os = require("os"); + +var { createIPCServer, sendIPCCommand } = require("../lib/ipc"); +var { + validateMemAvailableThresholdMB, + validateTokensPerMbHeadroom, +} = require("../lib/memory-setting-validate"); +var { DEFAULT_MEM_AVAILABLE_MIN_MB, DEFAULT_TOKENS_PER_MB_HEADROOM } = require("../lib/sdk-bridge"); + +// Reproduces daemon.js's own "set_mem_available_threshold" / +// "set_tokens_per_mb_headroom" IPC case bodies exactly (same validator call, +// same config mutation, same response shape) against an injected in-memory +// config + save function, so the test drives the real transport (lib/ipc.js) +// with the real case logic instead of a hand-waved stand-in. +function makeDaemonIpcHandler(config, saveConfig) { + return function (msg) { + switch (msg.cmd) { + case "set_mem_available_threshold": { + var memResult = validateMemAvailableThresholdMB(msg.value); + if (!memResult.ok) { + return { ok: false, error: memResult.error, memAvailableMinMB: config.memAvailableMinMB !== undefined ? config.memAvailableMinMB : DEFAULT_MEM_AVAILABLE_MIN_MB }; + } + config.memAvailableMinMB = memResult.value; + saveConfig(config); + return { ok: true, memAvailableMinMB: memResult.value }; + } + case "set_tokens_per_mb_headroom": { + var tpmResult = validateTokensPerMbHeadroom(msg.value); + if (!tpmResult.ok) { + return { ok: false, error: tpmResult.error, tokensPerMbHeadroom: config.tokensPerMbHeadroom !== undefined ? config.tokensPerMbHeadroom : DEFAULT_TOKENS_PER_MB_HEADROOM }; + } + config.tokensPerMbHeadroom = tpmResult.value; + saveConfig(config); + return { ok: true, tokensPerMbHeadroom: tpmResult.value }; + } + default: + return { ok: false, error: "unknown command: " + msg.cmd }; + } + }; +} + +function withRealIpcServer(config, fn) { + var sockPath = path.join(fs.mkdtempSync(path.join(os.tmpdir(), "clagentic-ipc-test-")), "daemon.sock"); + var saveCalls = []; + var saveConfig = function (cfg) { saveCalls.push(Object.assign({}, cfg)); }; + var handler = makeDaemonIpcHandler(config, saveConfig); + var server = createIPCServer(sockPath, handler); + + // createIPCServer's own connect-probe + bind is async; poll until the + // socket file actually exists (bounded, since this transport binds a + // Unix socket file synchronously inside listen()'s callback chain). + return new Promise(function (resolve, reject) { + var waited = 0; + var poll = setInterval(function () { + waited += 10; + if (fs.existsSync(sockPath)) { + clearInterval(poll); + Promise.resolve(fn(sockPath, saveCalls)) + .then(function (result) { + server.close(); + resolve(result); + }) + .catch(function (err) { + server.close(); + reject(err); + }); + } else if (waited > 5000) { + clearInterval(poll); + server.close(); + reject(new Error("IPC server never bound socket at " + sockPath)); + } + }, 10); + }); +} + +// --------------------------------------------------------------------------- +// set_mem_available_threshold: real out-of-range caller over the real socket +// --------------------------------------------------------------------------- + +test("lr-553d27: raw IPC set_mem_available_threshold rejects a negative value with ok:false naming the band, over the real socket", function () { + var config = { memAvailableMinMB: 256 }; + return withRealIpcServer(config, function (sockPath, saveCalls) { + return sendIPCCommand(sockPath, { cmd: "set_mem_available_threshold", value: -5 }).then(function (resp) { + assert.equal(resp.ok, false, "an out-of-range value must be rejected, not silently clamped and reported as success"); + assert.match(resp.error, />=\s*0/, "the error must name the valid band (>= 0)"); + assert.equal(config.memAvailableMinMB, 256, "the persisted value must be UNCHANGED after a rejected write"); + assert.equal(saveCalls.length, 0, "saveConfig must never be called for a rejected value"); + }); + }); +}); + +test("lr-553d27: raw IPC set_mem_available_threshold rejects a non-numeric value with ok:false, persisted value unchanged", function () { + var config = { memAvailableMinMB: 128 }; + return withRealIpcServer(config, function (sockPath, saveCalls) { + return sendIPCCommand(sockPath, { cmd: "set_mem_available_threshold", value: "not-a-number" }).then(function (resp) { + assert.equal(resp.ok, false); + assert.equal(config.memAvailableMinMB, 128, "persisted value must be UNCHANGED, not silently reset to the default"); + assert.equal(saveCalls.length, 0); + }); + }); +}); + +test("lr-553d27: raw IPC set_mem_available_threshold still accepts an in-range value (no regression on legitimate callers)", function () { + var config = { memAvailableMinMB: 128 }; + return withRealIpcServer(config, function (sockPath, saveCalls) { + return sendIPCCommand(sockPath, { cmd: "set_mem_available_threshold", value: 512 }).then(function (resp) { + assert.equal(resp.ok, true); + assert.equal(resp.memAvailableMinMB, 512); + assert.equal(config.memAvailableMinMB, 512, "an in-range value must still persist"); + assert.equal(saveCalls.length, 1); + }); + }); +}); + +test("lr-553d27: raw IPC set_mem_available_threshold still accepts the legitimate 'disable this gate' value of 0", function () { + var config = { memAvailableMinMB: 512 }; + return withRealIpcServer(config, function (sockPath) { + return sendIPCCommand(sockPath, { cmd: "set_mem_available_threshold", value: 0 }).then(function (resp) { + assert.equal(resp.ok, true, "0 is a legitimate value (lr-93e3c8 finding 1) and must not be rejected"); + assert.equal(config.memAvailableMinMB, 0); + }); + }); +}); + +// --------------------------------------------------------------------------- +// set_tokens_per_mb_headroom: real out-of-range caller over the real socket +// --------------------------------------------------------------------------- + +test("lr-553d27: raw IPC set_tokens_per_mb_headroom rejects an out-of-range value (1000) with ok:false naming the 10-500 band, over the real socket", function () { + // 1000 is the exact plausible-operator-instinct value named in the task + // description as landing squarely in the pre-fix trap. + var config = { tokensPerMbHeadroom: 240 }; + return withRealIpcServer(config, function (sockPath, saveCalls) { + return sendIPCCommand(sockPath, { cmd: "set_tokens_per_mb_headroom", value: 1000 }).then(function (resp) { + assert.equal(resp.ok, false, "an out-of-range value must be rejected, not silently clamped to the default and reported ok:true"); + assert.match(resp.error, /10-500/, "the error must name the valid band (10-500)"); + assert.equal(config.tokensPerMbHeadroom, 240, "the persisted value must be UNCHANGED after a rejected write -- this is the exact trap: the default used to silently land here instead"); + assert.equal(saveCalls.length, 0, "saveConfig must never be called for a rejected value"); + }); + }); +}); + +test("lr-553d27: raw IPC set_tokens_per_mb_headroom rejects a value below the band (5) with ok:false, persisted value unchanged", function () { + var config = { tokensPerMbHeadroom: 300 }; + return withRealIpcServer(config, function (sockPath, saveCalls) { + return sendIPCCommand(sockPath, { cmd: "set_tokens_per_mb_headroom", value: 5 }).then(function (resp) { + assert.equal(resp.ok, false); + assert.match(resp.error, /10-500/); + assert.equal(config.tokensPerMbHeadroom, 300); + assert.equal(saveCalls.length, 0); + }); + }); +}); + +test("lr-553d27: raw IPC set_tokens_per_mb_headroom still accepts an in-range value (no regression on legitimate callers)", function () { + var config = { tokensPerMbHeadroom: 240 }; + return withRealIpcServer(config, function (sockPath, saveCalls) { + return sendIPCCommand(sockPath, { cmd: "set_tokens_per_mb_headroom", value: 300 }).then(function (resp) { + assert.equal(resp.ok, true); + assert.equal(resp.tokensPerMbHeadroom, 300); + assert.equal(config.tokensPerMbHeadroom, 300, "an in-range value must still persist"); + assert.equal(saveCalls.length, 1); + }); + }); +}); + +// --------------------------------------------------------------------------- +// Source-parity check: daemon.js's actual case bodies must call the SAME +// shared validator this test drives, not a hand-duplicated inline copy that +// could silently drift back to the clamp-and-report-success shape. Mirrors +// the established convention in +// test/activity-diagnostics-retrieval-lr-8b476f.test.js (source inspection +// paired with, never substituting for, the behavioral tests above). +// --------------------------------------------------------------------------- + +test("lib/daemon.js: set_mem_available_threshold and set_tokens_per_mb_headroom IPC cases call the shared memory-setting-validate.js functions", function () { + var daemonSrc = fs.readFileSync(path.join(__dirname, "..", "lib", "daemon.js"), "utf8"); + assert.match(daemonSrc, /require\(["']\.\/memory-setting-validate["']\)/, "daemon.js must require lib/memory-setting-validate.js"); + + var memCaseStart = daemonSrc.indexOf('case "set_mem_available_threshold"'); + assert.ok(memCaseStart !== -1, 'expected a "set_mem_available_threshold" IPC case in lib/daemon.js'); + var memCaseEnd = daemonSrc.indexOf("case ", memCaseStart + 1); + var memCaseBody = daemonSrc.slice(memCaseStart, memCaseEnd); + assert.match(memCaseBody, /validateMemAvailableThresholdMB\(/, "the IPC case must call the shared validator, not an inline duplicate"); + assert.match(memCaseBody, /ok:\s*false/, "the IPC case must be able to return ok:false on rejection"); + assert.doesNotMatch(memCaseBody, /=\s*DEFAULT_MEM_AVAILABLE_MIN_MB;/, "must not silently clamp the value to the default on the out-of-range branch"); + + var tpmCaseStart = daemonSrc.indexOf('case "set_tokens_per_mb_headroom"'); + assert.ok(tpmCaseStart !== -1, 'expected a "set_tokens_per_mb_headroom" IPC case in lib/daemon.js'); + var tpmCaseEnd = daemonSrc.indexOf("case ", tpmCaseStart + 1); + var tpmCaseBody = daemonSrc.slice(tpmCaseStart, tpmCaseEnd); + assert.match(tpmCaseBody, /validateTokensPerMbHeadroom\(/, "the IPC case must call the shared validator, not an inline duplicate"); + assert.match(tpmCaseBody, /ok:\s*false/, "the IPC case must be able to return ok:false on rejection"); + assert.doesNotMatch(tpmCaseBody, /=\s*DEFAULT_TOKENS_PER_MB_HEADROOM;/, "must not silently clamp the value to the default on the out-of-range branch"); +}); + +test("lib/daemon.js: onSetMemAvailableThreshold and onSetTokensPerMbHeadroom (WS/web path) also call the shared validators (single-contract check)", function () { + var daemonSrc = fs.readFileSync(path.join(__dirname, "..", "lib", "daemon.js"), "utf8"); + + var webMemStart = daemonSrc.indexOf("onSetMemAvailableThreshold: function"); + assert.ok(webMemStart !== -1); + var webMemEnd = daemonSrc.indexOf("\n },", webMemStart); + var webMemBody = daemonSrc.slice(webMemStart, webMemEnd); + assert.match(webMemBody, /validateMemAvailableThresholdMB\(/, "the WS/web handler must call the same shared validator as the raw IPC path, not a second independent copy"); + + var webTpmStart = daemonSrc.indexOf("onSetTokensPerMbHeadroom: function"); + assert.ok(webTpmStart !== -1); + var webTpmEnd = daemonSrc.indexOf("\n },", webTpmStart); + var webTpmBody = daemonSrc.slice(webTpmStart, webTpmEnd); + assert.match(webTpmBody, /validateTokensPerMbHeadroom\(/, "the WS/web handler must call the same shared validator as the raw IPC path, not a second independent copy"); +}); diff --git a/test/memory-setting-validate-lr-553d27.test.js b/test/memory-setting-validate-lr-553d27.test.js new file mode 100644 index 00000000..7117f763 --- /dev/null +++ b/test/memory-setting-validate-lr-553d27.test.js @@ -0,0 +1,78 @@ +// memory-setting-validate-lr-553d27.test.js +// +// Unit coverage for the pure validators extracted for lr-553d27 (see +// lib/memory-setting-validate.js header comment for why this was extracted: +// a single shared contract for the memAvailableMinMB / tokensPerMbHeadroom +// range checks, instead of two independently-maintained copies -- the +// divergence between which was the actual root cause of the raw IPC path's +// silent-clamp defect). +// +// This file covers the pure function in isolation; it is NOT a substitute +// for the reachability coverage in +// test/daemon-ipc-memory-setter-parity-lr-553d27.test.js, which proves a +// real caller over the real raw IPC socket gets this exact behavior (see +// that file's header comment on plumbing vs. reachability coverage, +// tome #845). + +"use strict"; + +var test = require("node:test"); +var assert = require("node:assert/strict"); + +var { + validateMemAvailableThresholdMB, + validateTokensPerMbHeadroom, +} = require("../lib/memory-setting-validate"); + +test("validateMemAvailableThresholdMB: rejects a negative value with a band-naming error", function () { + var result = validateMemAvailableThresholdMB(-5); + assert.equal(result.ok, false); + assert.match(result.error, />=\s*0/); +}); + +test("validateMemAvailableThresholdMB: rejects a non-numeric value", function () { + var result = validateMemAvailableThresholdMB("not-a-number"); + assert.equal(result.ok, false); +}); + +test("validateMemAvailableThresholdMB: accepts 0 (legitimate 'disable this gate' value, lr-93e3c8 finding 1)", function () { + var result = validateMemAvailableThresholdMB(0); + assert.equal(result.ok, true); + assert.equal(result.value, 0); +}); + +test("validateMemAvailableThresholdMB: accepts a positive in-range value unchanged", function () { + var result = validateMemAvailableThresholdMB(512); + assert.equal(result.ok, true); + assert.equal(result.value, 512); +}); + +test("validateTokensPerMbHeadroom: rejects a value above the band (1000) with a band-naming error", function () { + var result = validateTokensPerMbHeadroom(1000); + assert.equal(result.ok, false); + assert.match(result.error, /10-500/); +}); + +test("validateTokensPerMbHeadroom: rejects a value below the band (5)", function () { + var result = validateTokensPerMbHeadroom(5); + assert.equal(result.ok, false); + assert.match(result.error, /10-500/); +}); + +test("validateTokensPerMbHeadroom: accepts the lower boundary (10)", function () { + var result = validateTokensPerMbHeadroom(10); + assert.equal(result.ok, true); + assert.equal(result.value, 10); +}); + +test("validateTokensPerMbHeadroom: accepts the upper boundary (500)", function () { + var result = validateTokensPerMbHeadroom(500); + assert.equal(result.ok, true); + assert.equal(result.value, 500); +}); + +test("validateTokensPerMbHeadroom: accepts an in-range value unchanged", function () { + var result = validateTokensPerMbHeadroom(300); + assert.equal(result.ok, true); + assert.equal(result.value, 300); +}); From 2c35b661ce1c06b695569fda5aec98fe5269e566 Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Fri, 4 Sep 2026 16:13:22 -0400 Subject: [PATCH 2/3] chore(test): remove dead unused regex-match line (lr-d5c542) test/verify-installed-build-stale-process-lr-71f0c3.test.js:172 assigned an unused 'matches' var whose result was never read, from a regex whose escaping did not match its adjacent comment's description. The comment describes an assertion (regex-based detection confined to resolveProcessBuildStatus, not duplicated in main()) that the following two real assertions (fnStart/fnEnd boundary slicing) already correctly implement -- no assertion was missing, so the dead line is deleted rather than replaced. Confirmed pure no-op for test outcomes: total test count unchanged at 1600 before and after across multiple full-suite runs. --- test/verify-installed-build-stale-process-lr-71f0c3.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/verify-installed-build-stale-process-lr-71f0c3.test.js b/test/verify-installed-build-stale-process-lr-71f0c3.test.js index 817349c6..06f1029b 100644 --- a/test/verify-installed-build-stale-process-lr-71f0c3.test.js +++ b/test/verify-installed-build-stale-process-lr-71f0c3.test.js @@ -169,7 +169,6 @@ test("scripts/verify-installed-build.js: STALE_PROCESS message never surfaces th test("scripts/verify-installed-build.js: resolveProcessBuildStatus is the single place that detects the 'unknown command: get_build_status' shape", function () { var src = fs.readFileSync(path.join(__dirname, "..", "scripts", "verify-installed-build.js"), "utf8"); - var matches = src.match(/unknown command:\\?s\*get_build_status/g) || []; // resolveProcessBuildStatus contains two `.test(...)` calls against the // same regex literal (stderr and err.message) plus the header-comment // prose mentioning the literal string -- assert the regex-based detection From 0d74b15c0168deceb4b4c89c122c344a5a9b0738 Mon Sep 17 00:00:00 2001 From: "clagentic-builder[bot]" Date: Fri, 4 Sep 2026 16:19:59 -0400 Subject: [PATCH 3/3] fix(daemon): reject prefix-numeric garbage strings in memory setter validators (lr-553d27) BOBBIE's coercion review on PR #417 found parseInt() alone truncate-parses a garbage-suffixed string ("300abc" -> 300) instead of rejecting it: a raw IPC caller sending {"value":"300abc"} received ok:true with 300 silently persisted -- a different value than it sent, the same reports-success-while-substituting-a-different-value contract violation this task exists to close, one level below the out-of-range case. Added coerceCleanInteger() to lib/memory-setting-validate.js: accepts a real integer number, or a numeric string that is a clean whole number (optionally surrounded by whitespace) with no trailing/leading non-digit characters; rejects everything else, including a numeric prefix followed by garbage and a decimal fraction. Applied to both validators, so both the WS/web and raw IPC paths get the fix automatically via the shared contract. CONSTRAINT preserved: in-range callers are unaffected, and a clean numeric STRING (e.g. "1000") -- a plausible caller shape over a JSON socket -- remains accepted; only malformed input is newly rejected. Extended both test files: unit coverage for coerceCleanInteger's accept/reject boundaries (garbage suffix, decimal fraction, whitespace-wrapped garbage vs. whitespace-padded clean value), and real-socket reachability coverage asserting a raw IPC caller sending "300abc"/"128xyz" receives ok:false with the band named and the persisted value unchanged, while a clean numeric string still round-trips. Demonstrated the new tests fail against plain parseInt (temporarily reverted, confirmed the intended failures and no others, restored) and pass with the fix; full suite 1610/1610 clean on a subsequent run (2 residual failures on the run immediately prior are the pre-existing lr-eb0d5e-class daemon-spawn flake, unrelated to this change). --- lib/memory-setting-validate.js | 27 +++++++++- ...ipc-memory-setter-parity-lr-553d27.test.js | 45 +++++++++++++++++ .../memory-setting-validate-lr-553d27.test.js | 49 +++++++++++++++++++ 3 files changed, 119 insertions(+), 2 deletions(-) diff --git a/lib/memory-setting-validate.js b/lib/memory-setting-validate.js index b4c2e2ce..74c1587e 100644 --- a/lib/memory-setting-validate.js +++ b/lib/memory-setting-validate.js @@ -14,9 +14,32 @@ // { ok: false, error } on rejection; the error message always names the // valid band so a caller (including the raw-socket escape-hatch documented // to operators) can tell exactly what would be accepted. +// +// coerceCleanInteger (lr-553d27 fold-in, BOBBIE PR #417 coercion review): +// parseInt() alone accepts a PREFIX of its input -- parseInt("300abc", 10) +// is 300, not NaN -- so a caller sending {"value":"300abc"} over the raw IPC +// socket would have received ok:true with 300 silently persisted, a +// DIFFERENT value than the one it sent. That is the exact +// "control path reports success while silently substituting a different +// value" contract violation this task exists to close, one level down from +// the out-of-range case. A real number (already a clean integer/float, no +// parsing needed) or a numeric STRING that is a whole number with no trailing +// garbage is accepted; anything else (a non-numeric prefix, a decimal +// fraction, or a completely non-numeric value) is rejected. A caller passing +// a clean numeric string over a JSON socket (e.g. "1000") remains a +// legitimate, accepted case. +function coerceCleanInteger(rawValue) { + if (typeof rawValue === "number") { + return Number.isInteger(rawValue) ? rawValue : NaN; + } + if (typeof rawValue === "string" && /^-?\d+$/.test(rawValue.trim())) { + return parseInt(rawValue, 10); + } + return NaN; +} function validateMemAvailableThresholdMB(rawValue) { - var val = parseInt(rawValue, 10); + var val = coerceCleanInteger(rawValue); if (isNaN(val) || val < 0) { return { ok: false, error: "Value must be a number >= 0" }; } @@ -24,7 +47,7 @@ function validateMemAvailableThresholdMB(rawValue) { } function validateTokensPerMbHeadroom(rawValue) { - var val = parseInt(rawValue, 10); + var val = coerceCleanInteger(rawValue); if (isNaN(val) || val < 10 || val > 500) { return { ok: false, error: "Value must be 10-500" }; } diff --git a/test/daemon-ipc-memory-setter-parity-lr-553d27.test.js b/test/daemon-ipc-memory-setter-parity-lr-553d27.test.js index 432ac59f..bf704d49 100644 --- a/test/daemon-ipc-memory-setter-parity-lr-553d27.test.js +++ b/test/daemon-ipc-memory-setter-parity-lr-553d27.test.js @@ -202,6 +202,51 @@ test("lr-553d27: raw IPC set_tokens_per_mb_headroom still accepts an in-range va }); }); +// --------------------------------------------------------------------------- +// Prefix-string coercion (fold-in, BOBBIE PR #417 coercion review): a raw IPC +// caller sending a garbage-suffixed numeric string over the real socket must +// be rejected, not silently truncate-parsed to a DIFFERENT value than it +// sent and reported ok:true -- the exact "reports success while nothing +// happened" contract violation this task exists to eliminate, one level +// below the out-of-range case BOBBIE originally flagged as an integrity +// issue when raising this task's priority. +// --------------------------------------------------------------------------- + +test("lr-553d27: raw IPC set_tokens_per_mb_headroom rejects a garbage-suffixed numeric string ('300abc') over the real socket, instead of truncate-parsing it to 300", function () { + var config = { tokensPerMbHeadroom: 240 }; + return withRealIpcServer(config, function (sockPath, saveCalls) { + return sendIPCCommand(sockPath, { cmd: "set_tokens_per_mb_headroom", value: "300abc" }).then(function (resp) { + assert.equal(resp.ok, false, "\"300abc\" must be rejected, not silently truncate-parsed to 300 and reported as success"); + assert.match(resp.error, /10-500/, "the error must name the valid band"); + assert.equal(config.tokensPerMbHeadroom, 240, "the persisted value must be UNCHANGED -- this is the exact trap: 300 (a different value than what was sent) used to silently land here instead"); + assert.equal(saveCalls.length, 0, "saveConfig must never be called for a rejected value"); + }); + }); +}); + +test("lr-553d27: raw IPC set_mem_available_threshold rejects a garbage-suffixed numeric string ('128xyz') over the real socket, instead of truncate-parsing it to 128", function () { + var config = { memAvailableMinMB: 64 }; + return withRealIpcServer(config, function (sockPath, saveCalls) { + return sendIPCCommand(sockPath, { cmd: "set_mem_available_threshold", value: "128xyz" }).then(function (resp) { + assert.equal(resp.ok, false, "\"128xyz\" must be rejected, not silently truncate-parsed to 128 and reported as success"); + assert.equal(config.memAvailableMinMB, 64, "the persisted value must be UNCHANGED"); + assert.equal(saveCalls.length, 0); + }); + }); +}); + +test("lr-553d27: raw IPC set_tokens_per_mb_headroom still accepts a clean in-range numeric STRING ('300') over the real socket -- a legitimate caller sending a JSON string is not the malformed-input case this fix targets", function () { + var config = { tokensPerMbHeadroom: 240 }; + return withRealIpcServer(config, function (sockPath, saveCalls) { + return sendIPCCommand(sockPath, { cmd: "set_tokens_per_mb_headroom", value: "300" }).then(function (resp) { + assert.equal(resp.ok, true, "a clean numeric string must still be accepted"); + assert.equal(resp.tokensPerMbHeadroom, 300); + assert.equal(config.tokensPerMbHeadroom, 300); + assert.equal(saveCalls.length, 1); + }); + }); +}); + // --------------------------------------------------------------------------- // Source-parity check: daemon.js's actual case bodies must call the SAME // shared validator this test drives, not a hand-duplicated inline copy that diff --git a/test/memory-setting-validate-lr-553d27.test.js b/test/memory-setting-validate-lr-553d27.test.js index 7117f763..a021c4b8 100644 --- a/test/memory-setting-validate-lr-553d27.test.js +++ b/test/memory-setting-validate-lr-553d27.test.js @@ -76,3 +76,52 @@ test("validateTokensPerMbHeadroom: accepts an in-range value unchanged", functio assert.equal(result.ok, true); assert.equal(result.value, 300); }); + +// --------------------------------------------------------------------------- +// Prefix-string coercion (fold-in, BOBBIE PR #417 coercion review): parseInt +// alone truncate-parses a garbage-suffixed string ("300abc" -> 300) instead +// of rejecting it, silently substituting a different value than the caller +// sent -- the same "reports success while nothing happened" contract +// violation this task exists to eliminate, one level below the out-of-range +// case. A clean numeric STRING (e.g. "1000") must still be accepted -- only +// malformed input is rejected. +// --------------------------------------------------------------------------- + +test("validateTokensPerMbHeadroom: rejects a prefix-numeric garbage-suffixed string ('300abc') instead of truncate-parsing it to 300", function () { + var result = validateTokensPerMbHeadroom("300abc"); + assert.equal(result.ok, false, "\"300abc\" must be rejected, not silently truncate-parsed to 300"); + assert.match(result.error, /10-500/); +}); + +test("validateMemAvailableThresholdMB: rejects a prefix-numeric garbage-suffixed string ('128xyz') instead of truncate-parsing it to 128", function () { + var result = validateMemAvailableThresholdMB("128xyz"); + assert.equal(result.ok, false, "\"128xyz\" must be rejected, not silently truncate-parsed to 128"); +}); + +test("validateTokensPerMbHeadroom: still accepts a clean in-range numeric STRING ('300') -- a plausible caller over a JSON socket", function () { + var result = validateTokensPerMbHeadroom("300"); + assert.equal(result.ok, true, "a clean numeric string must remain accepted -- only malformed input is rejected"); + assert.equal(result.value, 300); +}); + +test("validateMemAvailableThresholdMB: still accepts a clean numeric STRING ('1000') -- a plausible caller over a JSON socket", function () { + var result = validateMemAvailableThresholdMB("1000"); + assert.equal(result.ok, true); + assert.equal(result.value, 1000); +}); + +test("validateTokensPerMbHeadroom: rejects a decimal-fraction string ('300.5') rather than truncating it", function () { + var result = validateTokensPerMbHeadroom("300.5"); + assert.equal(result.ok, false, "a fractional value must be rejected, not silently floored to 300"); +}); + +test("validateTokensPerMbHeadroom: rejects leading/trailing whitespace-wrapped garbage (' 300 tokens ')", function () { + var result = validateTokensPerMbHeadroom(" 300 tokens "); + assert.equal(result.ok, false); +}); + +test("validateTokensPerMbHeadroom: still accepts a whitespace-padded clean numeric string (' 300 ')", function () { + var result = validateTokensPerMbHeadroom(" 300 "); + assert.equal(result.ok, true, "surrounding whitespace around an otherwise-clean numeric string is not the malformed-input case this fix targets"); + assert.equal(result.value, 300); +});