fix(daemon): raw IPC memory setters reject out-of-range input (lr-553d27)

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": {

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,
+};

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(