fix(clone): route EVERY settings writer through one safe-write gate

Round 2 of the #251 review: settingsWriteBlocker covered only
writeHooksConfig and updateCaseModel, while applyStatusLineConfig,
stripCaseEnvKeys, updateCaseEnvVars, refreshStaleCodemanHooks and
ensureCodemanHooks still wrote the same repository-controlled path
unguarded (applyStatusLineConfig was demonstrated writing through a
symlinked settings.local.json).

All seven writers now go through withSafeSettingsWrite(), which runs
the blocker check INSIDE the per-path settings lock and then hands the
writer its claudeDir/settingsPath; none of them touch the settings path
directly anymore. Test pins all seven against a symlinked
settings.local.json at once (link target must stay byte-identical).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

src/hooks-config.ts

@@ -287,6 +287,64 @@ function withSettingsLock<T>(path: string, fn: () => Promise<T>): Promise<T> {
   return run;
 }
 
+/**
+ * Why writing into `<casePath>/.claude/settings.local.json` must NOT proceed,
+ * or null when it is safe.
+ *
+ * Case contents can be FOREIGN (a freshly cloned repository, an imported
+ * tree): `.claude` or the settings file itself can arrive as a symlink
+ * pointing anywhere on this machine, and `writeFile` follows links, so a
+ * scaffold write would land outside the case, up to and including replacing
+ * the user's own `~/.claude/settings.json` (#251 review). Any symlink in the
+ * chain, or a `.claude` that resolves outside the case, refuses the write.
+ * A missing `.claude` is fine (the writer creates it).
+ */
+export async function settingsWriteBlocker(casePath: string): Promise<string | null> {
+  const claudeDir = join(casePath, '.claude');
+  try {
+    const dirStat = await lstat(claudeDir).catch(() => null);
+    if (dirStat?.isSymbolicLink()) return 'its .claude is a symlink';
+    if (dirStat && !dirStat.isDirectory()) return 'its .claude is a file, not a directory';
+    if (dirStat && (await realpath(claudeDir)) !== join(await realpath(casePath), '.claude')) {
+      return 'its .claude directory resolves outside the case';
+    }
+    const settingsStat = await lstat(join(claudeDir, 'settings.local.json')).catch(() => null);
+    if (settingsStat?.isSymbolicLink()) return 'its .claude/settings.local.json is a symlink';
+  } catch (err) {
+    return `its .claude paths could not be verified (${String(err)})`;
+  }
+  return null;
+}
+
+/**
+ * The ONE gate for writing `<casePath>/.claude/settings.local.json`.
+ *
+ * Serializes writers per path (withSettingsLock) and, INSIDE the lock, refuses
+ * the write when `settingsWriteBlocker` reports the target unsafe. Every
+ * settings writer in this module must go through here rather than calling
+ * `writeFile` on the settings path itself, so a repository-controlled symlink
+ * can never redirect ANY of them outside the case (#251 review: the guard
+ * originally covered only two writers, and applyStatusLineConfig was shown
+ * writing through a symlinked settings file). Refusal is a console.warn, not
+ * a throw: hooks/statusline degrade gracefully and the session still runs.
+ */
+async function withSafeSettingsWrite(
+  casePath: string,
+  purpose: string,
+  fn: (claudeDir: string, settingsPath: string) => Promise<void>
+): Promise<void> {
+  const claudeDir = join(casePath, '.claude');
+  const settingsPath = join(claudeDir, 'settings.local.json');
+  await withSettingsLock(settingsPath, async () => {
+    const blocker = await settingsWriteBlocker(casePath);
+    if (blocker) {
+      console.warn(`[hooks-config] Refusing to write ${purpose} for ${casePath}: ${blocker}`);
+      return;
+    }
+    await fn(claudeDir, settingsPath);
+  });
+}
+
 /**
  * Generates the hooks section for .claude/settings.local.json
  *
@@ -460,8 +518,7 @@ function mergeCodemanHooks(existingValue: unknown, generated: Record<string, unk
 export async function stripCaseEnvKeys(casePath: string, keysToRemove: readonly string[]): Promise<void> {
   if (keysToRemove.length === 0) return;
 
-  const settingsPath = join(casePath, '.claude', 'settings.local.json');
+  await withSafeSettingsWrite(casePath, 'env-key removal', async (_claudeDir, settingsPath) => {
-  await withSettingsLock(settingsPath, async () => {
     if (!existsSync(settingsPath)) return;
 
     let existing: Record<string, unknown>;
@@ -493,9 +550,7 @@ export async function stripCaseEnvKeys(casePath: string, keysToRemove: readonly
  * Merges with existing env field; removes vars set to empty string.
  */
 export async function updateCaseEnvVars(casePath: string, envVars: Record<string, string>): Promise<void> {
-  const claudeDir = join(casePath, '.claude');
+  await withSafeSettingsWrite(casePath, 'env vars', async (claudeDir, settingsPath) => {
-  const settingsPath = join(claudeDir, 'settings.local.json');
-  await withSettingsLock(settingsPath, async () => {
     if (!existsSync(claudeDir)) {
       await mkdir(claudeDir, { recursive: true });
     }
@@ -526,16 +581,7 @@ export async function updateCaseEnvVars(casePath: string, envVars: Record<string
  * Pass a non-empty string to set, or empty/null to remove.
  */
 export async function updateCaseModel(casePath: string, model: string | null): Promise<void> {
-  // Same symlink refusal as writeHooksConfig: this writer runs for cloned
+  await withSafeSettingsWrite(casePath, 'model', async (claudeDir, settingsPath) => {
-  // cases too (modelOverride at quick-start), against foreign tree contents.
-  const blocker = await settingsWriteBlocker(casePath);
-  if (blocker) {
-    console.warn(`[hooks-config] Refusing to write model for ${casePath}: ${blocker}`);
-    return;
-  }
-  const claudeDir = join(casePath, '.claude');
-  const settingsPath = join(claudeDir, 'settings.local.json');
-  await withSettingsLock(settingsPath, async () => {
     if (!existsSync(claudeDir)) {
       await mkdir(claudeDir, { recursive: true });
     }
@@ -557,35 +603,6 @@ export async function updateCaseModel(casePath: string, model: string | null): P
   });
 }
 
-/**
- * Why writing into `<casePath>/.claude/settings.local.json` must NOT proceed,
- * or null when it is safe.
- *
- * Case contents can be FOREIGN (a freshly cloned repository, an imported
- * tree): `.claude` or the settings file itself can arrive as a symlink
- * pointing anywhere on this machine, and `writeFile` follows links, so a
- * scaffold write would land outside the case, up to and including replacing
- * the user's own `~/.claude/settings.json` (#251 review). Any symlink in the
- * chain, or a `.claude` that resolves outside the case, refuses the write.
- * A missing `.claude` is fine (the writer creates it).
- */
-export async function settingsWriteBlocker(casePath: string): Promise<string | null> {
-  const claudeDir = join(casePath, '.claude');
-  try {
-    const dirStat = await lstat(claudeDir).catch(() => null);
-    if (dirStat?.isSymbolicLink()) return 'its .claude is a symlink';
-    if (dirStat && !dirStat.isDirectory()) return 'its .claude is a file, not a directory';
-    if (dirStat && (await realpath(claudeDir)) !== join(await realpath(casePath), '.claude')) {
-      return 'its .claude directory resolves outside the case';
-    }
-    const settingsStat = await lstat(join(claudeDir, 'settings.local.json')).catch(() => null);
-    if (settingsStat?.isSymbolicLink()) return 'its .claude/settings.local.json is a symlink';
-  } catch (err) {
-    return `its .claude paths could not be verified (${String(err)})`;
-  }
-  return null;
-}
-
 /**
  * Writes hooks config to .claude/settings.local.json in the given case path.
  * Merges with existing file content, only touching the `hooks` key.
@@ -593,14 +610,7 @@ export async function settingsWriteBlocker(casePath: string): Promise<string | n
  * idle detection) when `settingsWriteBlocker` reports the target unsafe.
  */
 export async function writeHooksConfig(casePath: string): Promise<void> {
-  const blocker = await settingsWriteBlocker(casePath);
+  await withSafeSettingsWrite(casePath, 'hooks', async (claudeDir, settingsPath) => {
-  if (blocker) {
-    console.warn(`[hooks-config] Refusing to write hooks for ${casePath}: ${blocker}`);
-    return;
-  }
-  const claudeDir = join(casePath, '.claude');
-  const settingsPath = join(claudeDir, 'settings.local.json');
-  await withSettingsLock(settingsPath, async () => {
     if (!existsSync(claudeDir)) {
       await mkdir(claudeDir, { recursive: true });
     }
@@ -642,9 +652,7 @@ export async function writeHooksConfig(casePath: string): Promise<void> {
  * block they have never had), so that call is left to the owner rather than made here.
  */
 export async function ensureCodemanHooks(casePath: string): Promise<void> {
-  const claudeDir = join(casePath, '.claude');
+  await withSafeSettingsWrite(casePath, 'hooks (ensure)', async (claudeDir, settingsPath) => {
-  const settingsPath = join(claudeDir, 'settings.local.json');
-  await withSettingsLock(settingsPath, async () => {
     if (!existsSync(claudeDir)) {
       await mkdir(claudeDir, { recursive: true });
     }
@@ -685,9 +693,8 @@ export async function ensureCodemanHooks(casePath: string): Promise<void> {
  * when the hooks aren't ours, so it is cheap enough to call on every Claude spawn.
  */
 export async function refreshStaleCodemanHooks(casePath: string): Promise<void> {
-  const settingsPath = join(casePath, '.claude', 'settings.local.json');
+  if (!existsSync(join(casePath, '.claude', 'settings.local.json'))) return;
-  if (!existsSync(settingsPath)) return;
+  await withSafeSettingsWrite(casePath, 'hooks (refresh)', async (_claudeDir, settingsPath) => {
-  await withSettingsLock(settingsPath, async () => {
     let existing: Record<string, unknown>;
     try {
       existing = JSON.parse(await readFile(settingsPath, 'utf-8'));
@@ -749,10 +756,7 @@ export function generateStatusLineCommand(): string {
  * Claude mode. Merges, preserving all other keys (hooks, env, model).
  */
 export async function applyStatusLineConfig(casePath: string, enabled: boolean): Promise<void> {
-  const claudeDir = join(casePath, '.claude');
+  await withSafeSettingsWrite(casePath, 'statusLine', async (claudeDir, settingsPath) => {
-  const settingsPath = join(claudeDir, 'settings.local.json');
-
-  await withSettingsLock(settingsPath, async () => {
     let existing: Record<string, unknown> = {};
     if (existsSync(settingsPath)) {
       try {

test/hooks-config.test.ts

@@ -11,12 +11,16 @@ import { join } from 'node:path';
 import { tmpdir } from 'node:os';
 import { spawn } from 'node:child_process';
 import {
+  applyStatusLineConfig,
   ensureCodemanHooks,
   generateBackgroundWakeScript,
   generateHooksConfig,
   generateSubagentStopGuardScript,
   refreshStaleCodemanHooks,
   settingsWriteBlocker,
+  stripCaseEnvKeys,
+  updateCaseEnvVars,
+  updateCaseModel,
   writeHooksConfig,
 } from '../src/hooks-config.js';
 
@@ -230,6 +234,31 @@ describe('writeHooksConfig', () => {
     expect(await settingsWriteBlocker(caseDir)).toBeNull();
   });
 
+  it('EVERY settings writer refuses a symlinked settings.local.json (#251 review round 2)', async () => {
+    // Round 1 guarded only writeHooksConfig/updateCaseModel; the reviewer
+    // demonstrated applyStatusLineConfig writing through the link. All
+    // writers now share one safe-write gate, so pin all of them at once.
+    const outsideFile = join(testDir, 'victim-all-writers.json');
+    const precious =
+      '{"env":{"CLAUDE_CODE_KEEP":"me"},"hooks":{"Stop":[{"hooks":[{"command":"curl /api/hook-event"}]}]}}\n';
+    writeFileSync(outsideFile, precious);
+    const caseDir = join(testDir, 'case-writers');
+    mkdirSync(join(caseDir, '.claude'), { recursive: true });
+    symlinkSync(outsideFile, join(caseDir, '.claude', 'settings.local.json'));
+
+    await writeHooksConfig(caseDir);
+    await ensureCodemanHooks(caseDir);
+    await refreshStaleCodemanHooks(caseDir);
+    await updateCaseModel(caseDir, 'opus');
+    await updateCaseEnvVars(caseDir, { CLAUDE_CODE_NEW: 'value' });
+    await stripCaseEnvKeys(caseDir, ['CLAUDE_CODE_KEEP']);
+    await applyStatusLineConfig(caseDir, true);
+    await applyStatusLineConfig(caseDir, false);
+
+    // The link target is byte-identical: none of the writers went through it.
+    expect(readFileSync(outsideFile, 'utf-8')).toBe(precious);
+  });
+
   it('should merge with existing settings.local.json', async () => {
     const claudeDir = join(testDir, '.claude');
     mkdirSync(claudeDir, { recursive: true });