fix: keep the provisioning marker tied to the run that wrote it

install.sh

@@ -71,25 +71,88 @@ CLAWBOX_HOME="/home/clawbox"
 # exits non-zero, and a machine-readable marker is left for the flash host.
 PROVISION_FAILURES=()
 PROVISION_STATUS_FILE="${CLAWBOX_PROVISION_STATUS_FILE:-/etc/clawbox/provision-status}"
+# Identifies THIS run. Stamped into the marker and printed on stdout, so a
+# reader holding both can tell whose verdict it is looking at.
+PROVISION_RUN_ID="$(date -u +%Y%m%dT%H%M%SZ 2>/dev/null || echo unknown)-$$"
+# Set when the marker channel could not be made to describe this run. The final
+# verdict folds this in: a verdict we cannot publish is not a success.
+PROVISION_STATUS_UNPUBLISHED=0
 
 record_provision_failure() {
   PROVISION_FAILURES+=("$1")
 }
 
+# ── The marker must never speak for a run other than this one ────────────────
+# The flash host reads $PROVISION_STATUS_FILE INSTEAD of parsing stdout, so the
+# file has to satisfy two properties, neither of which "write it at the end and
+# hope" provides:
+#
+#   1. Never stale. A run that cannot write the marker (read-only /etc, a file
+#      owned by another user, a full disk) used to leave the PREVIOUS run's
+#      STATUS=ok sitting there, and the flash host read it as this run's verdict
+#      — the same false-healthy result this whole block exists to prevent. So
+#      the marker is DELETED before provisioning starts: if the file exists at
+#      the end, this run wrote it.
+#   2. Never half-written. Temp file + rename in the same directory, so a reader
+#      sees the whole old marker or the whole new one, and a truncated write
+#      cannot leave "STATUS=ok" with the rest of the record missing.
+#
+# When either cannot be guaranteed, that is itself a reason not to ship the box:
+# the run says so on stdout and its verdict becomes "incomplete". Staying quiet
+# is what produced the false "ok".
+
+# Drop any marker left behind by an earlier run. Called once, before the first
+# provisioning step of a full install.
+invalidate_provision_status() {
+  rm -f "$PROVISION_STATUS_FILE" 2>/dev/null || true
+  # `rm -f` reports success for an already-absent file and failure for one it
+  # could not remove, so test the outcome rather than its exit status.
+  if [ -e "$PROVISION_STATUS_FILE" ]; then
+    PROVISION_STATUS_UNPUBLISHED=1
+    echo "  WARNING: could not clear the previous provisioning marker"
+    echo "           $PROVISION_STATUS_FILE — its contents describe an EARLIER"
+    echo "           run and must not be read as this one's verdict."
+    return 1
+  fi
+  return 0
+}
+
 # Persist the final provisioning verdict where the flash host (or an operator,
 # or the next update) can read it without re-parsing install.sh's stdout.
 write_provision_status() {
   local status="$1"; shift
-  local dir
+  local dir tmp failed=0
   dir="$(dirname "$PROVISION_STATUS_FILE")"
+  tmp="$PROVISION_STATUS_FILE.tmp.$$"
   mkdir -p "$dir" 2>/dev/null || true
-  {
+  if ! {
     echo "# Written by install.sh at the end of a full install. Machine-readable."
+    echo "# One marker per run: the previous one is removed before provisioning"
+    echo "# starts, so this file always describes the run named by RUN_ID."
+    echo "RUN_ID=$PROVISION_RUN_ID"
     echo "STATUS=$status"
     echo "FAILED_STEPS=$*"
     echo "TIMESTAMP=$(date -u +%Y-%m-%dT%H:%M:%SZ 2>/dev/null || true)"
-  } > "$PROVISION_STATUS_FILE" 2>/dev/null || true
-  chmod 644 "$PROVISION_STATUS_FILE" 2>/dev/null || true
+  } > "$tmp" 2>/dev/null; then
+    failed=1
+  else
+    chmod 644 "$tmp" 2>/dev/null || true
+    # Rename last: until this succeeds the live path holds nothing (it was
+    # cleared at the start), never a partial record.
+    mv -f "$tmp" "$PROVISION_STATUS_FILE" 2>/dev/null || failed=1
+  fi
+  # Either half failing means the same thing to the caller and wants the same
+  # answer, so there is one branch for both rather than two that must be kept
+  # saying the same thing.
+  if [ "$failed" -ne 0 ]; then
+    rm -f "$tmp" 2>/dev/null || true
+    PROVISION_STATUS_UNPUBLISHED=1
+    echo "  WARNING: could not publish $PROVISION_STATUS_FILE (status=$status)."
+    echo "           This run has no marker. Use install.sh's exit code or the"
+    echo "           [provision-status] line on stdout instead."
+    return 1
+  fi
+  return 0
 }
 
 # ── Edition (single-harness lock) ────────────────────────────────────────────
@@ -3241,6 +3304,12 @@ log() {
 
 echo "=== ClawBox Installer ==="
 
+# Clear the previous run's verdict BEFORE provisioning anything. From here until
+# the summary at the bottom there is deliberately no marker on disk, so a run
+# that dies mid-way (or cannot write its own marker at the end) leaves the flash
+# host with "no verdict" rather than with the last run's "ok".
+invalidate_provision_status || true
+
 log "Ensuring clawbox user exists..."
 step_ensure_user
 
@@ -3382,7 +3451,9 @@ echo ""
 # success by the caller (the flash host's "Setup: N/N succeeded"). The marker
 # file carries the same verdict for a caller that reads a file instead of the
 # exit code, and the sentinel line ([provision-status] ...) for one that greps
-# stdout. Keep all three in agreement.
+# stdout. Keep all three in agreement — including when the marker cannot be
+# written at all, in which case the other two must report "incomplete" rather
+# than a success no reader of the file can see.
 FINAL_RC=0
 if [ "${#PROVISION_FAILURES[@]}" -gt 0 ] || [ "${VALIDATE_RC:-0}" -ne 0 ]; then
   FINAL_RC=1
@@ -3400,11 +3471,29 @@ if [ "$FINAL_RC" -ne 0 ]; then
     echo "  # Service validation FAILED (see the checks listed above)."
   fi
   echo "  ############################################################"
-  write_provision_status incomplete "${PROVISION_FAILURES[*]:-}"
+  write_provision_status incomplete "${PROVISION_FAILURES[*]:-}" || true
+  # The sentinel lines below are a stdout contract with the flash host: keep the
+  # prefix and the verdict word byte-identical. The run id goes on its own line.
   echo "[provision-status] INCOMPLETE${PROVISION_FAILURES[*]:+ (${PROVISION_FAILURES[*]})}"
+  echo "[provision-run] $PROVISION_RUN_ID"
 else
-  write_provision_status ok ""
-  echo "[provision-status] OK"
+  write_provision_status ok "" || true
+  if [ "$PROVISION_STATUS_UNPUBLISHED" -ne 0 ]; then
+    # Every step passed, but the channel the flash host reads cannot be made to
+    # say so for THIS run. Reporting success here is how a stale marker gets
+    # read as a fresh verdict, so downgrade instead: an install whose result
+    # cannot be published is not an install anyone should ship.
+    FINAL_RC=1
+    echo "  ############################################################"
+    echo "  # PROVISIONING INCOMPLETE — every step passed, but this run"
+    echo "  # could not publish its verdict to $PROVISION_STATUS_FILE."
+    echo "  # Do NOT ship this box as healthy; fix the path and re-run."
+    echo "  ############################################################"
+    echo "[provision-status] INCOMPLETE (marker unwritable)"
+  else
+    echo "[provision-status] OK"
+  fi
+  echo "[provision-run] $PROVISION_RUN_ID"
 fi
 
 exit "$FINAL_RC"

scripts/setup-hermes-dashboard-auth.sh

@@ -287,8 +287,32 @@ PY
   done
 
   # Store the plaintext password for the proxy ONLY (clawbox-owned, 0600).
-  ( umask 077; printf '%s' "$pw" > "$PWFILE" )
-  chmod 600 "$PWFILE"
+  #
+  # Same temp-file-and-rename as the config write above, for the same reason:
+  # `> "$PWFILE"` truncates in place, so the proxy — which re-reads this file on
+  # every session renewal — can observe an empty or partial password and answer
+  # 401, and a crash between the truncate and the write leaves it empty for good
+  # (classify_creds then calls that PW_MISSING). The temp file lives in the same
+  # directory so the rename is atomic, and is created 0600 by umask so the
+  # plaintext is never briefly world-readable.
+  local pwtmp="$PWFILE.tmp.$$"
+  if ! ( umask 077; printf '%s' "$pw" > "$pwtmp" ); then
+    rm -f "$pwtmp" 2>/dev/null || true
+    log "ERROR: failed to write $pwtmp" >&2
+    return 1
+  fi
+  # Checked separately from the write above: folding both into one subshell
+  # would let a successful chmod mask a partial printf.
+  if ! chmod 600 "$pwtmp"; then
+    rm -f "$pwtmp" 2>/dev/null || true
+    log "ERROR: failed to set mode 600 on $pwtmp" >&2
+    return 1
+  fi
+  if ! mv -f "$pwtmp" "$PWFILE"; then
+    rm -f "$pwtmp" 2>/dev/null || true
+    log "ERROR: failed to install $PWFILE" >&2
+    return 1
+  fi
   return 0
 }
 
@@ -385,6 +409,12 @@ while :; do
     "$CREDS_NOT_CONFIGURED"|"$CREDS_PW_MISSING")
       if [ "$attempt" -lt "$max_attempts" ]; then
         log "WARNING: the dashboard auth block / password file we just wrote to $HERMES_CONFIG is already gone — a concurrent process rewrote the file. The credentials generated were correct; the write was lost. Retrying under $CONFIG_LOCK (attempt $attempt/$max_attempts)." >&2
+        # Back off before re-minting. This branch only runs when the lock did
+        # NOT hold us apart, i.e. exactly while the other writer is still inside
+        # its own read-modify-write window — retrying immediately loses the same
+        # race again, and all three attempts can finish before it lands its
+        # os.replace. Grows with the attempt so a slow writer still gets a turn.
+        sleep "$attempt"
         continue
       fi
       log "ERROR: after $attempt attempts our dashboard auth block did not survive in $HERMES_CONFIG — another process keeps rewriting the file and is not honouring $CONFIG_LOCK. This is a write/serialisation fault, NOT a credential mismatch: the password and hash were correct on every attempt." >&2

src/tests/unit/hermes-config-lock.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, it } from "vitest";
-import { spawnSync } from "node:child_process";
+import { spawn, spawnSync } from "node:child_process";
 import fs from "node:fs";
 import os from "node:os";
 import path from "node:path";
@@ -43,11 +43,27 @@ describe("both writers share ONE lock file", () => {
     // its PyYAML reconcile AND holds it across the `hermes tools disable` CLI
     // call (which does its own wide load->save_config on the same file).
     expect(AUTH_SRC).toMatch(/acquire_config_lock[\s\S]*mint_credentials/);
-    const regTail = REGISTER_SRC.slice(REGISTER_SRC.indexOf("acquire_config_lock\n\nexport"));
-    expect(regTail).toContain("acquire_config_lock");
-    expect(REGISTER_SRC.indexOf("acquire_config_lock\n\nexport")).toBeLessThan(
-      REGISTER_SRC.indexOf("tools disable browser"),
-    );
+
+    // Locate the registrar's CALL SITE with an anchored regex: a bare
+    // `acquire_config_lock` line at column 0. That cannot match the DEFINITION
+    // (`acquire_config_lock() {`) and does not depend on what happens to follow
+    // it, unlike the old "acquire_config_lock\n\nexport" formatting marker.
+    const call = REGISTER_SRC.search(/^acquire_config_lock[ \t]*$/m);
+    // The registrar's two writes of config.yaml: the PyYAML reconcile, and the
+    // Hermes CLI call that does its own load->save_config. The lock has to come
+    // before BOTH — "before the CLI call" alone was satisfied by taking it one
+    // line above, leaving the reconcile unprotected.
+    const reconcile = REGISTER_SRC.search(/^export CLAWBOX_MCP_HERMES_CONFIG=/m);
+    const cliCall = REGISTER_SRC.search(/^if "\$HERMES_BIN" tools disable browser/m);
+    // Every marker must have been FOUND before their order means anything: a
+    // `search` miss returns -1, and -1 < anything, so an ordering assertion over
+    // a moved marker passes while checking nothing.
+    expect(call, "register-mcp.sh: no top-level acquire_config_lock call").toBeGreaterThan(-1);
+    expect(reconcile, "register-mcp.sh: no PyYAML reconcile block").toBeGreaterThan(-1);
+    expect(cliCall, "register-mcp.sh: no `hermes tools disable browser` call").toBeGreaterThan(-1);
+    expect(call).toBeLessThan(reconcile);
+    expect(call).toBeLessThan(cliCall);
+
     expect(AUTH_SRC).toContain("flock -w 120 9");
     expect(REGISTER_SRC).toContain("flock -w 120 9");
   });
@@ -78,10 +94,38 @@ describe.runIf(RUNNABLE)("the lock is really taken at runtime", () => {
   });
 
   it.runIf(FLOCK)("waits for a held lock instead of racing through it", () => {
-    // Hold the shared lock for ~800ms in the background, then run the auth
-    // script. If it honours the lock it blocks until release (elapsed ≳ hold);
-    // if it ignored it, it would finish in well under 300ms. This is the mutual
-    // exclusion that stops the lost update.
+    // Timed in THIS process, not in the shell: `date +%s.%N` is a GNU coreutils
+    // extension, and on BSD/macOS date `%N` is emitted literally, so the old
+    // driver parsed "1770000000.N" and failed for a reason that had nothing to
+    // do with the lock. The suite gates only on platform !== "win32", so it runs
+    // there. And only the auth script's OWN duration is measured — timing the
+    // whole driver, including `wait` on the holder, would report ≈ the hold
+    // time whether or not the script honoured the lock.
+    const HOLD_S = 1.5;
+
+    // Calibrate: what one UNCONTENDED run of this script costs on this machine.
+    // The assertion below is "honouring the lock adds most of the hold ON TOP of
+    // that", so it cannot be satisfied by a slow interpreter. Twice, keeping the
+    // faster: the first run in a fresh process pays cold-start costs (bash,
+    // python, page cache) that the contended run no longer pays, and a cold
+    // baseline would eat the margin and fail for the wrong reason.
+    const timeUncontendedRun = () => {
+      const baseRoot = fs.mkdtempSync(path.join(os.tmpdir(), "clawbox-lockbase-"));
+      const baseConfig = path.join(baseRoot, "hermes", "config.yaml");
+      fs.mkdirSync(path.dirname(baseConfig), { recursive: true });
+      fs.writeFileSync(baseConfig, "mcp_servers:\n  clawbox:\n    enabled: true\n");
+      const baseStart = Date.now();
+      const baseProc = spawnSync("bash", [AUTH], {
+        encoding: "utf-8",
+        timeout: 20000,
+        env: { ...process.env, CLAWBOX_ROOT: baseRoot, HERMES_CONFIG: baseConfig },
+      });
+      const ms = Date.now() - baseStart;
+      expect(baseProc.status, baseProc.stderr).toBe(0);
+      return ms;
+    };
+    const uncontendedMs = Math.min(timeUncontendedRun(), timeUncontendedRun());
+
     const root = fs.mkdtempSync(path.join(os.tmpdir(), "clawbox-lockwait-"));
     const configPath = path.join(root, "hermes", "config.yaml");
     fs.mkdirSync(path.dirname(configPath), { recursive: true });
@@ -90,25 +134,43 @@ describe.runIf(RUNNABLE)("the lock is really taken at runtime", () => {
     fs.writeFileSync(configPath, "mcp_servers:\n  clawbox:\n    enabled: true\n");
     const lockFile = `${configPath}.lock`;
 
-    // One bash driver so the holder and the auth script actually overlap: launch
-    // a background holder that takes the lock for ~0.8s, wait 0.15s so it wins
-    // the lock first, then run the auth script and time how long it blocks.
-    const script = `
-      set -e
-      LOCK="${lockFile}"
-      ( exec 9>"$LOCK"; flock 9; sleep 0.8 ) &
-      hold=$!
-      sleep 0.15                       # ensure the holder has the lock first
-      start=$(date +%s.%N)
-      CLAWBOX_ROOT="${root}" HERMES_CONFIG="${configPath}" bash "${AUTH}" >/dev/null 2>&1
-      end=$(date +%s.%N)
-      wait $hold
-      awk -v s="$start" -v e="$end" 'BEGIN{printf "%.3f", e - s}'
-    `;
-    const proc = spawnSync("bash", ["-c", script], { encoding: "utf-8", timeout: 20000 });
-    const elapsed = parseFloat(proc.stdout.trim() || "0");
-    // Held ~0.8s, started ~0.15s in, so the auth script should wait ≳0.5s.
-    expect(elapsed).toBeGreaterThan(0.5);
+    // Background holder: takes the shared lock and keeps it for HOLD_S.
+    const holder = spawn("bash", ["-c", `exec 9>"${lockFile}"; flock 9; sleep ${HOLD_S}`], {
+      stdio: "ignore",
+    });
+    holder.on("error", () => {});
+    try {
+      // Do not GUESS that the holder has the lock — prove it, by probing with a
+      // non-blocking flock until the probe is refused. A sleep-and-hope here is
+      // how this test would start measuring nothing on a loaded machine.
+      const deadline = Date.now() + 5000;
+      let held = false;
+      while (Date.now() < deadline) {
+        const probe = spawnSync("bash", ["-c", `exec 9>"${lockFile}"; flock -n 9`], {
+          encoding: "utf-8",
+        });
+        if (probe.status !== 0) {
+          held = true;
+          break;
+        }
+      }
+      expect(held, "the background holder never took the lock").toBe(true);
+
+      const start = Date.now();
+      const proc = spawnSync("bash", [AUTH], {
+        encoding: "utf-8",
+        timeout: 20000,
+        env: { ...process.env, CLAWBOX_ROOT: root, HERMES_CONFIG: configPath },
+      });
+      const contendedMs = Date.now() - start;
+      expect(proc.status, proc.stderr).toBe(0);
+      // Held 1.5s and the script started while it was held, so honouring the
+      // lock costs ≳1.4s more than the uncontended run. Ignoring it would cost
+      // about the same as the uncontended run. 500ms separates those cleanly.
+      expect(contendedMs - uncontendedMs).toBeGreaterThan(500);
+    } finally {
+      holder.kill();
+    }
 
     // And the foreign writer's key survived alongside the new dashboard block.
     const config = fs.readFileSync(configPath, "utf-8");