Merge branch 'main' into cleanup/mount-allowlist-readonly-and-cache

Fix the agent-to-agent in_reply_to stamp (cross-process no-op)

.claude/skills/manage-mounts/SKILL.md

@@ -13,18 +13,18 @@ Configure which host directories NanoClaw agent containers can access. The mount
 cat ~/.config/nanoclaw/mount-allowlist.json 2>/dev/null || echo "No mount allowlist configured"
 ```
 
-Show the current config to the user in a readable format: which directories are allowed, whether non-main agents are read-only.
+Show the current config to the user in a readable format: which directories are allowed, and whether each is read-only or read-write.
 
 ## Add Directories
 
 Ask which directories the user wants agents to access. For each path:
 - Validate the path exists
-- Ask if it should be read-only for non-main agents (default: yes)
+- Ask if it should be read-write (`allowReadWrite: true`) or read-only (`allowReadWrite: false`, the safer default)
 
 Build the JSON config and write it:
 
 ```bash
-pnpm exec tsx setup/index.ts --step mounts --force -- --json '{"allowedRoots":[{"path":"/path/to/dir","readOnly":false}],"blockedPatterns":[],"nonMainReadOnly":true}'
+pnpm exec tsx setup/index.ts --step mounts --force -- --json '{"allowedRoots":[{"path":"/path/to/dir","allowReadWrite":true}],"blockedPatterns":[]}'
 ```
 
 Use `--force` to overwrite the existing config.
@@ -34,7 +34,7 @@ Use `--force` to overwrite the existing config.
 Read the current config, show it, ask which entry to remove, then write the updated config through the same write path (build the trimmed JSON and pass it to `--step mounts --force -- --json`):
 
 ```bash
-pnpm exec tsx setup/index.ts --step mounts --force -- --json '{"allowedRoots":[],"blockedPatterns":[],"nonMainReadOnly":true}'
+pnpm exec tsx setup/index.ts --step mounts --force -- --json '{"allowedRoots":[],"blockedPatterns":[]}'
 ```
 
 ## Reset to Empty
@@ -45,12 +45,10 @@ pnpm exec tsx setup/index.ts --step mounts --force -- --empty
 
 ## After Changes
 
-Restart the service so containers pick up the new config (the unit/label names are per-install — see `setup/lib/install-slug.sh`).
+The allowlist is read fresh when a container is spawned, so new mounts apply to newly spawned containers automatically — no service restart needed.
 
-Run from your NanoClaw project root:
+To apply the new config to a group that already has a running container, restart just that group:
 
 ```bash
-source setup/lib/install-slug.sh
-launchctl kickstart -k gui/$(id -u)/$(launchd_label)  # macOS
-systemctl --user restart $(systemd_unit)              # Linux
+ncl groups restart --id <group-id>
 ```

package.json

@@ -1,6 +1,6 @@
 {
   "name": "nanoclaw",
-  "version": "2.1.34",
+  "version": "2.1.35",
   "description": "Personal Claude assistant. Lightweight, secure, customizable.",
   "type": "module",
   "packageManager": "pnpm@10.33.0",

repo-tokens/badge.svg

@@ -1,5 +1,5 @@
-<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="90" height="20" role="img" aria-label="207k tokens, 104% of context window">
-  <title>207k tokens, 104% of context window</title>
+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="90" height="20" role="img" aria-label="208k tokens, 104% of context window">
+  <title>208k tokens, 104% of context window</title>
   <linearGradient id="s" x2="0" y2="100%">
     <stop offset="0" stop-color="#bbb" stop-opacity=".1"/>
     <stop offset="1" stop-opacity=".1"/>
@@ -15,8 +15,8 @@
       <g fill="#fff" text-anchor="middle" font-family="Verdana,Geneva,DejaVu Sans,sans-serif" font-size="11">
         <text aria-hidden="true" x="26" y="15" fill="#010101" fill-opacity=".3">tokens</text>
         <text x="26" y="14">tokens</text>
-        <text aria-hidden="true" x="71" y="15" fill="#010101" fill-opacity=".3">207k</text>
-        <text x="71" y="14">207k</text>
+        <text aria-hidden="true" x="71" y="15" fill="#010101" fill-opacity=".3">208k</text>
+        <text x="71" y="14">208k</text>
       </g>
     </g>
   </a>

src/modules/mount-security/index.test.ts

@@ -0,0 +1,119 @@
+/**
+ * Tests for the mount allowlist loader/validator.
+ *
+ * Covers the two cleanups:
+ *  - The loader honors the per-root `readOnly` key (translating it to
+ *    `allowReadWrite`) and tolerates the top-level `nonMainReadOnly` key that
+ *    setup writes into every fresh install.
+ *  - The allowlist is read per call (mtime-keyed cache), so a parse error is
+ *    never cached permanently — a fixed file is picked up without a restart.
+ */
+import fs from 'fs';
+import os from 'os';
+import path from 'path';
+
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+
+// The config path is a module-level const in production; point it at a
+// per-test temp file via a getter so each test is isolated from the cache.
+const mockState = vi.hoisted(() => ({ allowlistPath: '' }));
+
+vi.mock('../../config.js', async () => {
+  const actual = await vi.importActual<Record<string, unknown>>('../../config.js');
+  return {
+    ...actual,
+    get MOUNT_ALLOWLIST_PATH() {
+      return mockState.allowlistPath;
+    },
+  };
+});
+
+import { loadMountAllowlist, validateMount } from './index.js';
+
+let tmpDir: string;
+let configFile: string;
+let projectsDir: string;
+let repoDir: string;
+
+beforeEach(() => {
+  tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'mnt-sec-'));
+  configFile = path.join(tmpDir, 'mount-allowlist.json');
+  mockState.allowlistPath = configFile;
+
+  projectsDir = path.join(tmpDir, 'projects');
+  repoDir = path.join(projectsDir, 'repo');
+  fs.mkdirSync(repoDir, { recursive: true });
+});
+
+afterEach(() => {
+  fs.rmSync(tmpDir, { recursive: true, force: true });
+});
+
+function writeAllowlist(obj: unknown): void {
+  fs.writeFileSync(configFile, JSON.stringify(obj, null, 2) + '\n');
+}
+
+describe('loadMountAllowlist', () => {
+  it('translates per-root readOnly:false into a read-write grant', () => {
+    writeAllowlist({
+      allowedRoots: [{ path: projectsDir, readOnly: false }],
+      blockedPatterns: [],
+    });
+
+    const allowlist = loadMountAllowlist();
+    expect(allowlist).not.toBeNull();
+    expect(allowlist!.allowedRoots[0].allowReadWrite).toBe(true);
+
+    // ...and a mount that requests read-write actually gets it.
+    const result = validateMount({ hostPath: repoDir, readonly: false });
+    expect(result.allowed).toBe(true);
+    expect(result.effectiveReadonly).toBe(false);
+  });
+
+  it('keeps readOnly:true as a read-only grant', () => {
+    writeAllowlist({
+      allowedRoots: [{ path: projectsDir, readOnly: true }],
+      blockedPatterns: [],
+    });
+
+    const allowlist = loadMountAllowlist();
+    expect(allowlist!.allowedRoots[0].allowReadWrite).toBe(false);
+
+    const result = validateMount({ hostPath: repoDir, readonly: false });
+    expect(result.allowed).toBe(true);
+    expect(result.effectiveReadonly).toBe(true);
+  });
+
+  it('tolerates an unknown top-level nonMainReadOnly key', () => {
+    writeAllowlist({
+      allowedRoots: [{ path: projectsDir, allowReadWrite: true }],
+      blockedPatterns: [],
+      nonMainReadOnly: true,
+    });
+
+    const allowlist = loadMountAllowlist();
+    expect(allowlist).not.toBeNull();
+    expect(allowlist!.allowedRoots).toHaveLength(1);
+    expect(allowlist!.allowedRoots[0].allowReadWrite).toBe(true);
+  });
+
+  it('picks up a fixed file without a restart (parse errors are not cached)', () => {
+    // A broken edit blocks all mounts...
+    fs.writeFileSync(configFile, 'not valid json {');
+    expect(loadMountAllowlist()).toBeNull();
+
+    // ...but fixing the file recovers on the very next call — no restart.
+    writeAllowlist({
+      allowedRoots: [{ path: projectsDir, allowReadWrite: true }],
+      blockedPatterns: [],
+    });
+    const allowlist = loadMountAllowlist();
+    expect(allowlist).not.toBeNull();
+    expect(allowlist!.allowedRoots).toHaveLength(1);
+  });
+
+  it('returns null when the allowlist file is missing', () => {
+    // No file written.
+    expect(loadMountAllowlist()).toBeNull();
+  });
+});

src/modules/mount-security/index.ts

@@ -29,9 +29,11 @@ export interface AllowedRoot {
   description?: string;
 }
 
-// Cache the allowlist in memory - only reloads on process restart
-let cachedAllowlist: MountAllowlist | null = null;
-let allowlistLoadError: string | null = null;
+// Cache the last successfully-parsed allowlist, keyed on the file's path +
+// mtime. A changed or fixed file is picked up on the next call (no restart),
+// and a parse error is never cached permanently — one bad edit blocks mounts
+// only until the file is fixed.
+let cache: { path: string; mtimeMs: number; allowlist: MountAllowlist } | null = null;
 
 /**
  * Default blocked patterns - paths that should never be mounted
@@ -57,60 +59,108 @@ const DEFAULT_BLOCKED_PATTERNS = [
 ];
 
 /**
- * Load the mount allowlist from the external config location.
- * Returns null if the file doesn't exist or is invalid.
- * Result is cached in memory for the lifetime of the process.
+ * Normalize a raw allowed-root entry into an {@link AllowedRoot}.
+ *
+ * The read-only decision is per-root. Historically this validator only read
+ * `allowReadWrite`, but the /manage-mounts skill and setup write `readOnly`
+ * instead — so a `readOnly: false` grant was silently forced read-only.
+ * Translate `readOnly` → `allowReadWrite = !readOnly` (with a warning) unless an
+ * explicit `allowReadWrite` is already present. With neither key, default to
+ * read-only (fail safe).
  */
-export function loadMountAllowlist(): MountAllowlist | null {
-  if (cachedAllowlist !== null) {
-    return cachedAllowlist;
+function normalizeRoot(root: Record<string, unknown>): AllowedRoot {
+  const rootPath = typeof root.path === 'string' ? root.path : '';
+  const description = typeof root.description === 'string' ? root.description : undefined;
+
+  let allowReadWrite: boolean;
+  if (typeof root.allowReadWrite === 'boolean') {
+    allowReadWrite = root.allowReadWrite;
+  } else if (typeof root.readOnly === 'boolean') {
+    allowReadWrite = !root.readOnly;
+    log.warn('Mount allowlist root uses "readOnly" — translating to allowReadWrite', {
+      root: rootPath,
+      readOnly: root.readOnly,
+    });
+  } else {
+    allowReadWrite = false;
   }
 
-  if (allowlistLoadError !== null) {
-    // Already tried and failed, don't spam logs
+  return { path: rootPath, allowReadWrite, description };
+}
+
+/**
+ * Load the mount allowlist from the external config location.
+ * Returns null if the file doesn't exist or is invalid.
+ * Re-reads on every call, but serves from an in-memory cache while the file's
+ * mtime is unchanged. A parse error is never cached — fix the file and the next
+ * call recovers without a service restart.
+ */
+export function loadMountAllowlist(): MountAllowlist | null {
+  // Missing-file behavior: warn and block additional mounts, but do NOT cache
+  // the miss — the file may be created later without a restart.
+  let stat: fs.Stats;
+  try {
+    stat = fs.statSync(MOUNT_ALLOWLIST_PATH);
+  } catch {
+    log.warn(
+      'Mount allowlist not found - additional mounts will be BLOCKED. Create the file to enable additional mounts.',
+      { path: MOUNT_ALLOWLIST_PATH },
+    );
     return null;
   }
 
-  try {
-    if (!fs.existsSync(MOUNT_ALLOWLIST_PATH)) {
-      // Do NOT cache this as an error — file may be created later without restart.
-      // Only parse/structural errors are permanently cached.
-      log.warn(
-        'Mount allowlist not found - additional mounts will be BLOCKED. Create the file to enable additional mounts.',
-        { path: MOUNT_ALLOWLIST_PATH },
-      );
-      return null;
-    }
+  // Serve from cache only while the same file is unchanged since the last
+  // successful load. Any edit (including fixing a previously broken file) bumps
+  // the mtime and is picked up on the next call.
+  if (cache !== null && cache.path === MOUNT_ALLOWLIST_PATH && cache.mtimeMs === stat.mtimeMs) {
+    return cache.allowlist;
+  }
 
+  try {
     const content = fs.readFileSync(MOUNT_ALLOWLIST_PATH, 'utf-8');
-    const allowlist = JSON.parse(content) as MountAllowlist;
+    const raw = JSON.parse(content) as Record<string, unknown>;
 
     // Validate structure
-    if (!Array.isArray(allowlist.allowedRoots)) {
+    if (!Array.isArray(raw.allowedRoots)) {
       throw new Error('allowedRoots must be an array');
     }
 
-    if (!Array.isArray(allowlist.blockedPatterns)) {
+    if (!Array.isArray(raw.blockedPatterns)) {
       throw new Error('blockedPatterns must be an array');
     }
 
-    // Merge with default blocked patterns
-    const mergedBlockedPatterns = [...new Set([...DEFAULT_BLOCKED_PATTERNS, ...allowlist.blockedPatterns])];
-    allowlist.blockedPatterns = mergedBlockedPatterns;
+    // Warn-and-ignore the top-level `nonMainReadOnly` key. Setup writes it into
+    // every fresh install, but this validator has no concept of a "main" agent —
+    // read-only is decided per-root. Do NOT throw: a hard reject would fail
+    // closed and brick all mounts on a standard install.
+    if ('nonMainReadOnly' in raw) {
+      log.warn('Mount allowlist has unsupported top-level "nonMainReadOnly" key — ignoring (read-only is per-root)', {
+        path: MOUNT_ALLOWLIST_PATH,
+      });
+    }
 
-    cachedAllowlist = allowlist;
+    const allowedRoots = (raw.allowedRoots as Array<Record<string, unknown>>).map(normalizeRoot);
+
+    // Merge with default blocked patterns
+    const blockedPatterns = [...new Set([...DEFAULT_BLOCKED_PATTERNS, ...(raw.blockedPatterns as string[])])];
+
+    const allowlist: MountAllowlist = { allowedRoots, blockedPatterns };
+
+    cache = { path: MOUNT_ALLOWLIST_PATH, mtimeMs: stat.mtimeMs, allowlist };
     log.info('Mount allowlist loaded successfully', {
       path: MOUNT_ALLOWLIST_PATH,
       allowedRoots: allowlist.allowedRoots.length,
       blockedPatterns: allowlist.blockedPatterns.length,
     });
 
-    return cachedAllowlist;
+    return allowlist;
   } catch (err) {
-    allowlistLoadError = err instanceof Error ? err.message : String(err);
+    // Do NOT poison the cache — a corrupt edit blocks mounts only until it's
+    // fixed, then the next call re-reads and recovers.
+    cache = null;
     log.error('Failed to load mount allowlist - additional mounts will be BLOCKED', {
       path: MOUNT_ALLOWLIST_PATH,
-      error: allowlistLoadError,
+      error: err instanceof Error ? err.message : String(err),
     });
     return null;
   }