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..74c1587e --- /dev/null +++ b/lib/memory-setting-validate.js @@ -0,0 +1,60 @@ +// 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. +// +// 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 = coerceCleanInteger(rawValue); + 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 = coerceCleanInteger(rawValue); + 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/scripts/verify-installed-build.js b/scripts/verify-installed-build.js index 37b06adc..f06d3d50 100644 --- a/scripts/verify-installed-build.js +++ b/scripts/verify-installed-build.js @@ -31,6 +31,42 @@ // service), which is different from a daemon that IS running but did not // pick up the new build. // +// STALE_PROCESS (lr-71f0c3, new): a FOURTH outcome, distinct from all of the +// above. A daemon that predates the get_build_status IPC case entirely (i.e. +// predates lr-dc9a3b itself) cannot answer this query at all — it replies +// {ok:false, error:"unknown command: get_build_status"} over the socket, +// which the CLI surfaces as `Failed: unknown command: get_build_status` on +// stderr with a non-zero exit. That raw transport string reads as a +// MISSING-CODE defect (the subcommand "isn't wired up") when it is nothing +// of the kind — the handler IS present (lib/daemon.js:~1640, see +// test/verify-installed-build-lr-dc9a3b.test.js's tests 7-9), the process +// just started before the handler existed. This condition recurred three +// times (PR #411, #412, #414 — see lr-71f0c3): twice it misled a crew agent +// into treating it as missing code; the third time it aborted a P1 merge's +// post_merge_steps chain outright, since the raw error was indistinguishable +// from a genuine tooling failure and the step was on_failure: fail. +// +// This is structurally unavoidable, not an edge case: a daemon predating the +// handler cannot manifest as a SHA mismatch (PROCESS_MISMATCH above) since it +// cannot answer the SHA question at all. "unknown command" is the ONLY shape +// this specific staleness can take, and it recurs on every merge until an +// operator restarts the daemon onto a build that has the handler. Detected +// here by matching the "unknown command: get_build_status" error text the +// daemon's default IPC case emits (see lib/daemon.js's default branch) — +// distinguished from a generic/unexpected failure the same way "no running +// daemon" already is, one level up in resolveProcessBuildStatus(). +// +// Treated as NON-FATAL (exit 0), same posture as ARTIFACT_VERIFIED_NO_PROCESS +// — the primary assertion (artifact matches merged HEAD) already passed by +// the time this leg runs, and this condition self-resolves the moment the +// daemon restarts (no operator action this script could gate on would change +// the outcome faster than a restart already would). Aborting on_failure:fail +// on it, as happened on PR #414, halts an otherwise-sound merge's post-merge +// chain on a known, self-resolving, non-code condition. It is NOT read as +// "verified" — the message says plainly the process build status is UNKNOWN +// pending a restart, mirroring ARTIFACT_VERIFIED_NO_PROCESS's own "not a +// failure, but do not read this as verified" framing exactly. +// // Every reported outcome names EXACTLY ONE of "artifact" or "process" (or // both) so "verified" is never ambiguous about which one it means — that // ambiguity is what let the artifact-only PART 1 result get relayed @@ -55,6 +91,12 @@ // the failure mode that would have caught 2026-08-25 at 01:48 — see // test/verify-installed-build-lr-dc9a3b.test.js for the // demonstrated-failure-before-fix simulation. +// — STALE_PROCESS (lr-71f0c3): the artifact matches, but the running +// daemon predates the get_build_status handler itself and cannot +// answer the query at all. NOT a failure (exit 0, see PART 2's +// header comment above for the fatal/non-fatal reasoning) — reported +// plainly with the remedy (restart the daemon), never as the raw +// 'unknown command' transport string. // All failures are reported on stderr with the exact SHAs/flags involved // so the drift is visible in loadout-merge's captured step output. // @@ -105,13 +147,17 @@ function resolveInstalledSha() { // `clagentic-console` on PATH instead of a real daemon socket. // // Returns: -// { running: false } — no daemon running -// { running: true, loadedBuildSha, staleInodes, pid } — daemon answered +// { running: false } — no daemon +// { running: true, stale: true } — daemon +// running but predates the get_build_status handler (lr-71f0c3) — see +// this function's STALE_PROCESS handling below +// { running: true, loadedBuildSha, staleInodes, pid } — daemon +// answered normally // // Throws only on a genuinely unexpected failure (the binary isn't on PATH, // or it returned output this parser can't make sense of at all) — a daemon -// that is simply not running is NOT a throw, since that's an expected, -// non-failing state (nothing to compare against yet). +// that is simply not running, or one that is running but predates this +// check, is NOT a throw, since both are expected, non-failing states. function resolveProcessBuildStatus(cliBin) { const bin = cliBin || 'clagentic-console'; let raw; @@ -132,8 +178,27 @@ function resolveProcessBuildStatus(cliBin) { return { running: false }; } } catch (_parseErr) { - // fall through to the generic failure below + // fall through below } + + // lr-71f0c3: a daemon that predates the get_build_status IPC case + // (i.e. predates lr-dc9a3b's own introduction) does not hit the + // {ok:false, error:"no running daemon"} shape above at all — it IS + // running, it just has no case for this command, so lib/daemon.js's + // default IPC branch replies {ok:false, error:"unknown command: + // get_build_status"}, and handleProcessBuildStatus (lib/cli/ + // ipc-subcommands.js) surfaces that on stderr as + // "Failed: unknown command: get_build_status" with exit 1. + // execFileSync's err.message embeds that stderr text, so match on it + // directly here (stdout carries no JSON in this path, unlike the "no + // running daemon" shape above -- the failure is on stderr, from a + // console.error, not a stdout console.log). + const stderrText = err.stderr ? err.stderr.toString() : ''; + if (/unknown command:\s*get_build_status/.test(stderrText) || + /unknown command:\s*get_build_status/.test(err.message || '')) { + return { running: true, stale: true }; + } + throw new Error(`--process-build-status failed: ${err.message}`); } const parsed = JSON.parse(raw); @@ -197,6 +262,25 @@ function main() { return; } + if (processStatus.stale) { + // lr-71f0c3: the running daemon predates the get_build_status handler + // itself, so it cannot answer this query at all -- structurally + // unavoidable until it restarts (see this script's header comment for + // why this is non-fatal, not a code defect, and not surfaced as the raw + // transport error). This is NOT read as "verified": the build status of + // the running process is explicitly UNKNOWN pending a restart. + console.log( + '[verify-installed-build] STALE_PROCESS: artifact matches merged HEAD ' + + `(${headSha}); the running daemon does not recognize the get_build_status query, ` + + 'meaning it predates that handler and has not picked up any build since. The PROCESS build ' + + 'status is UNKNOWN, NOT VERIFIED (PROCESS check inconclusive) -- restart the daemon to ' + + 'clear this: systemctl restart clagentic-console (NOT done automatically by this check). ' + + 'Not a failure: this is expected on first contact with a build that adds a new ' + + 'PROCESS-status query and self-resolves on restart.' + ); + return; + } + const processMismatch = processStatus.loadedBuildSha !== headSha; if (processMismatch || processStatus.staleInodes) { console.error( 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..bf704d49 --- /dev/null +++ b/test/daemon-ipc-memory-setter-parity-lr-553d27.test.js @@ -0,0 +1,294 @@ +// 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); + }); + }); +}); + +// --------------------------------------------------------------------------- +// 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 +// 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..a021c4b8 --- /dev/null +++ b/test/memory-setting-validate-lr-553d27.test.js @@ -0,0 +1,127 @@ +// 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); +}); + +// --------------------------------------------------------------------------- +// 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); +}); diff --git a/test/verify-installed-build-stale-process-lr-71f0c3.test.js b/test/verify-installed-build-stale-process-lr-71f0c3.test.js new file mode 100644 index 00000000..06f1029b --- /dev/null +++ b/test/verify-installed-build-stale-process-lr-71f0c3.test.js @@ -0,0 +1,185 @@ +// Regression tests for lr-71f0c3 — a daemon that predates the +// get_build_status IPC case (i.e. predates lr-dc9a3b itself) cannot answer +// scripts/verify-installed-build.js's process-status query at all. It +// replies {ok:false, error:"unknown command: get_build_status"}, which +// lib/cli/ipc-subcommands.js's handleProcessBuildStatus surfaces on stderr +// as "Failed: unknown command: get_build_status" with a non-zero exit. +// +// Surfaced three times (PR #411, #412, #414 — see lr-71f0c3's description +// and comment #1): the raw transport string reads as a MISSING-CODE defect +// ("the subcommand needs wiring") when the handler IS present +// (lib/daemon.js's "get_build_status" case, lib/daemon.js:~1640) — the +// process just started before the handler existed. Twice this misled a +// crew agent pre-dispatch; the third time it aborted a sound P1 merge's +// post_merge_steps chain outright (on_failure: fail on a script that never +// distinguished this from a genuine tooling failure). +// +// These tests pin the four discriminated process-leg outcomes so they +// cannot collapse into each other again: +// 1. STALE_PROCESS is detected as its own state, not conflated with the +// generic-failure throw path or the "no running daemon" path. +// 2. main() treats STALE_PROCESS as non-fatal (exit 0 / return, not +// process.exit(1)) — see the script's header comment for why this is +// a deliberate, justified choice, not an oversight. +// 3. STALE_PROCESS never surfaces the raw "unknown command" string in +// main()'s own output. +// 4. The STALE_PROCESS message states plainly that the process build +// status is UNKNOWN (never reads as "verified"), matching lr-dc9a3b +// requirement 3's artifact/process discipline exactly. + +"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 { execFileSync } = require("child_process"); + +var { resolveProcessBuildStatus } = require("../scripts/verify-installed-build"); + +// Builds a throwaway executable shim that stands in for a PRE-lr-dc9a3b +// `clagentic-console --process-build-status` — i.e. a running daemon that +// has no get_build_status IPC case, mirroring lib/daemon.js's default +// branch replying {ok:false, error:"unknown command: get_build_status"} +// and handleProcessBuildStatus surfacing that on stderr with exit 1. +function makeStaleCli() { + var dir = fs.mkdtempSync(path.join(os.tmpdir(), "lr-71f0c3-stalecli-")); + var binPath = path.join(dir, "fake-clagentic-console-stale"); + var script = + "#!/usr/bin/env node\n" + + "process.stderr.write('Failed: unknown command: get_build_status\\n');\n" + + "process.exit(1);\n"; + fs.writeFileSync(binPath, script, { mode: 0o755 }); + return binPath; +} + +// A genuine, unrelated CLI failure (not the "no running daemon" shape, not +// the "unknown command: get_build_status" shape) must still throw — this +// new detection must not swallow real failures the way "no running daemon" +// deliberately doesn't, but also must not over-match unrelated errors. +function makeGenericBrokenCli() { + var dir = fs.mkdtempSync(path.join(os.tmpdir(), "lr-71f0c3-brokencli-")); + var binPath = path.join(dir, "fake-clagentic-console-broken"); + var script = + "#!/usr/bin/env node\n" + + "process.stderr.write('Failed: something else entirely broke\\n');\n" + + "process.exit(1);\n"; + fs.writeFileSync(binPath, script, { mode: 0o755 }); + return binPath; +} + +// --------------------------------------------------------------------------- +// 1. resolveProcessBuildStatus reports a distinct {running:true, stale:true} +// state for a daemon that predates the handler — not a throw, not +// conflated with {running:false}. +// --------------------------------------------------------------------------- + +test("lr-71f0c3: resolveProcessBuildStatus reports {running:true, stale:true} when the daemon replies 'unknown command: get_build_status'", function () { + var fakeBin = makeStaleCli(); + + var status = resolveProcessBuildStatus(fakeBin); + + assert.equal(status.running, true, "the daemon IS running -- it just predates the handler"); + assert.equal(status.stale, true); + assert.equal(typeof status.loadedBuildSha, "undefined", + "a stale-process result must not carry a loadedBuildSha -- the daemon could not report one"); +}); + +// --------------------------------------------------------------------------- +// 2. A genuinely unrelated failure must still throw -- the new detection is +// narrow (matches only the specific "unknown command: get_build_status" +// shape), not a catch-all that swallows every non-zero exit. +// --------------------------------------------------------------------------- + +test("lr-71f0c3: resolveProcessBuildStatus still throws on an unrelated failure, not just any non-zero exit", function () { + var fakeBin = makeGenericBrokenCli(); + + assert.throws(function () { + resolveProcessBuildStatus(fakeBin); + }, /--process-build-status failed/); +}); + +// --------------------------------------------------------------------------- +// 3. Source-level check: main()'s STALE_PROCESS branch does not call +// process.exit(1) -- pins the deliberate non-fatal decision (see the +// script's header comment for the reasoning) so a future edit cannot +// silently flip this back to fatal without touching this test. +// --------------------------------------------------------------------------- + +test("scripts/verify-installed-build.js: main()'s STALE_PROCESS branch is non-fatal (no process.exit(1))", function () { + var src = fs.readFileSync(path.join(__dirname, "..", "scripts", "verify-installed-build.js"), "utf8"); + + assert.match(src, /processStatus\.stale/, "main() must check processStatus.stale"); + assert.match(src, /STALE_PROCESS:/, "a distinct STALE_PROCESS outcome must be reported"); + + var staleIdx = src.indexOf("if (processStatus.stale)"); + assert.ok(staleIdx !== -1, "expected an `if (processStatus.stale)` branch in main()"); + // The branch runs until the next top-level statement in main() -- the + // processMismatch check immediately follows it in source order. + var nextIdx = src.indexOf("const processMismatch", staleIdx); + var branchBody = src.slice(staleIdx, nextIdx === -1 ? staleIdx + 900 : nextIdx); + + assert.doesNotMatch(branchBody, /process\.exit\(1\)/, + "STALE_PROCESS must not exit non-zero -- it is a deliberate non-fatal outcome, not a failure"); + assert.match(branchBody, /return;/, "STALE_PROCESS must return (implicit exit 0), mirroring ARTIFACT_VERIFIED_NO_PROCESS"); +}); + +// --------------------------------------------------------------------------- +// 4. The STALE_PROCESS message must never leak the raw transport string, +// and must say plainly that the process build status is UNKNOWN -- +// never reads as "verified" (lr-dc9a3b requirement 3, restated for this +// fourth outcome by lr-71f0c3). +// --------------------------------------------------------------------------- + +test("scripts/verify-installed-build.js: STALE_PROCESS message never surfaces the raw 'unknown command' string and states the status is UNKNOWN", function () { + var src = fs.readFileSync(path.join(__dirname, "..", "scripts", "verify-installed-build.js"), "utf8"); + + var staleIdx = src.indexOf("if (processStatus.stale)"); + assert.ok(staleIdx !== -1); + var nextIdx = src.indexOf("const processMismatch", staleIdx); + var branchBody = src.slice(staleIdx, nextIdx === -1 ? staleIdx + 900 : nextIdx); + + // The console.log call itself (not this file's own header-comment prose, + // which legitimately discusses the raw string for documentation purposes) + // must not emit "unknown command" verbatim to the operator. + var logIdx = branchBody.indexOf("console.log("); + assert.ok(logIdx !== -1, "expected a console.log call in the STALE_PROCESS branch"); + // branchBody.indexOf(");", logIdx) would match the FIRST "..." occurrence, + // including one inside a template literal like `(${headSha});` -- that is + // not the call's actual closing paren. The call's own `return;` statement + // immediately follows its closing `);` in source order, so anchor on that + // instead of the first (wrong) `);` substring match. + var returnIdx = branchBody.indexOf("return;", logIdx); + assert.ok(returnIdx !== -1, "expected a `return;` statement after the console.log call"); + var logCall = branchBody.slice(logIdx, returnIdx); + + assert.doesNotMatch(logCall, /unknown command/i, + "the operator-facing STALE_PROCESS message must not leak the raw 'unknown command' transport string"); + assert.match(logCall, /UNKNOWN/, "the message must state the process build status is UNKNOWN"); + assert.match(logCall, /restart/i, "the message must state the remedy is a daemon restart"); +}); + +// --------------------------------------------------------------------------- +// 5. Detection lives in resolveProcessBuildStatus (the transport layer), +// not duplicated ad hoc in main() -- keeps the "what shape does the +// daemon's reply take" knowledge in one place. +// --------------------------------------------------------------------------- + +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"); + + // 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 + // itself is confined to resolveProcessBuildStatus's body, not repeated + // inside main(). + var fnStart = src.indexOf("function resolveProcessBuildStatus"); + var fnEnd = src.indexOf("\nfunction main", fnStart); + var mainBody = src.slice(src.indexOf("function main(")); + + assert.match(src.slice(fnStart, fnEnd), /unknown command/, + "resolveProcessBuildStatus must contain the detection"); + assert.doesNotMatch(mainBody, /\/unknown command/, + "main() must not duplicate the regex-based detection -- it should only branch on processStatus.stale"); +});