refactor: decompose prepareWorkDirectories into two focused sub-functions

src/workdir-setup.test.ts

@@ -11,7 +11,7 @@ jest.mock('./host-env', () => require('./test-helpers/fs-mock-factory.test-utils
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 jest.mock('./host-identity', () => require('./test-helpers/fs-mock-factory.test-utils').hostIdentityMockFactory());
 
-import { prepareWorkDirectories } from './workdir-setup';
+import { prepareWorkDirectories, workdirSetupTestHelpers } from './workdir-setup';
 import { resolveLogPaths } from './log-paths';
 import { getRealUserHome } from './host-identity';
 
@@ -277,3 +277,100 @@ describe('prepareWorkDirectories', () => {
     });
   });
 });
+
+describe('prepareLogDirectories (sub-function)', () => {
+  let tempDir: string;
+
+  const buildConfig = (overrides: Record<string, unknown> = {}) => ({
+    workDir: tempDir,
+    sslBump: false,
+    allowedDomains: [] as string[],
+    agentCommand: 'echo test',
+    logLevel: 'info' as const,
+    keepContainers: false,
+    buildLocal: false,
+    imageRegistry: 'ghcr.io/github/gh-aw-firewall',
+    imageTag: 'latest',
+    ...overrides,
+  });
+
+  beforeEach(() => {
+    tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'workdir-setup-test-'));
+    jest.clearAllMocks();
+    (fs.chownSync as unknown as jest.Mock).mockImplementation(() => undefined);
+    (getRealUserHome as jest.Mock).mockReturnValue(tempDir);
+  });
+
+  afterEach(() => {
+    fs.rmSync(tempDir, { recursive: true, force: true });
+  });
+
+  it('creates all log directories without touching chroot home', () => {
+    const config = buildConfig();
+    const logPaths = resolveLogPaths(config);
+    workdirSetupTestHelpers.prepareLogDirectories(logPaths);
+
+    expect(fs.existsSync(logPaths.agentLogs)).toBe(true);
+    expect(fs.existsSync(logPaths.sessionState)).toBe(true);
+    expect(fs.existsSync(logPaths.squidLogs)).toBe(true);
+    expect(fs.existsSync(logPaths.apiProxyLogs)).toBe(true);
+    expect(fs.existsSync(logPaths.cliProxyLogs)).toBe(true);
+    // chroot home must NOT have been created
+    expect(fs.existsSync(`${tempDir}-chroot-home`)).toBe(false);
+  });
+});
+
+describe('prepareChrootHomeMounts (sub-function)', () => {
+  let tempDir: string;
+
+  const buildConfig = (overrides: Record<string, unknown> = {}) => ({
+    workDir: tempDir,
+    sslBump: false,
+    allowedDomains: [] as string[],
+    agentCommand: 'echo test',
+    logLevel: 'info' as const,
+    keepContainers: false,
+    buildLocal: false,
+    imageRegistry: 'ghcr.io/github/gh-aw-firewall',
+    imageTag: 'latest',
+    ...overrides,
+  });
+
+  beforeEach(() => {
+    tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'workdir-setup-test-'));
+    jest.clearAllMocks();
+    (fs.chownSync as unknown as jest.Mock).mockImplementation(() => undefined);
+    (getRealUserHome as jest.Mock).mockReturnValue(tempDir);
+  });
+
+  afterEach(() => {
+    fs.rmSync(tempDir, { recursive: true, force: true });
+    fs.rmSync(`${tempDir}-chroot-home`, { recursive: true, force: true });
+  });
+
+  it('creates chroot home directory without touching log directories', () => {
+    const config = buildConfig();
+    const logPaths = resolveLogPaths(config);
+    workdirSetupTestHelpers.prepareChrootHomeMounts(config);
+
+    expect(fs.existsSync(`${tempDir}-chroot-home`)).toBe(true);
+    // log directories must NOT have been created
+    expect(fs.existsSync(logPaths.agentLogs)).toBe(false);
+    expect(fs.existsSync(logPaths.sessionState)).toBe(false);
+    expect(fs.existsSync(logPaths.squidLogs)).toBe(false);
+    expect(fs.existsSync(logPaths.apiProxyLogs)).toBe(false);
+    expect(fs.existsSync(logPaths.cliProxyLogs)).toBe(false);
+  });
+
+  it('creates .gemini directory only when geminiApiKey is provided', () => {
+    const geminiDir = path.join(tempDir, '.gemini');
+
+    workdirSetupTestHelpers.prepareChrootHomeMounts(buildConfig({ geminiApiKey: 'key' }));
+    expect(fs.existsSync(geminiDir)).toBe(true);
+
+    fs.rmSync(geminiDir, { recursive: true, force: true });
+
+    workdirSetupTestHelpers.prepareChrootHomeMounts(buildConfig());
+    expect(fs.existsSync(geminiDir)).toBe(false);
+  });
+});

src/workdir-setup.ts

@@ -111,19 +111,10 @@ function prepareChrootHomeMountpoint(emptyHomeDir: string, relativeMountPath: st
 }
 
 /**
- * Prepares all working directories required before container startup.
- *
- * Concern 1 — log/state directories: creates and chowns agent-logs,
- * session-state, squid-logs, api-proxy-logs, cli-proxy-logs, and the host
- * MCP logs directory.
- *
- * Concern 2 — chroot home bind-mount preparation: creates the emptyHomeDir
- * placeholder, all whitelisted ~/.* subdirectories on the host, and any
- * runner tool-cache mountpoints so Docker does not create them as root-owned.
+ * Creates all log and session-state directories required before container
+ * startup, setting ownership and permissions for the respective service users.
  */
-export function prepareWorkDirectories(config: WrapperConfig, logPaths: LogPaths): void {
-  // ── Concern 1: log / state directory setup ──────────────────────────────
-
+function prepareLogDirectories(logPaths: LogPaths): void {
   // Create agent logs directory for persistence
   // Chown to host user so Copilot CLI can write logs (AWF runs as root, agent runs as host user)
   ensureDirectory(logPaths.agentLogs, {
@@ -207,78 +198,98 @@ export function prepareWorkDirectories(config: WrapperConfig, logPaths: LogPaths
     fs.chmodSync(mcpLogsDir, 0o777);
     logger.debug(`MCP logs directory permissions fixed at: ${mcpLogsDir}`);
   }
+}
 
-  // ── Concern 2: chroot home bind-mount preparation ────────────────────────
-
+/**
+ * Creates the empty chroot home directory placeholder, all whitelisted ~/.
+ * subdirectories on the host, and any runner tool-cache mountpoints so Docker
+ * does not create them as root-owned before bind mounts are established.
+ *
+ * Security note: this enforces correct UID/GID ownership on chroot home paths
+ * before Docker bind-mounts overwrite the placeholders at container start.
+ */
+function prepareChrootHomeMounts(config: WrapperConfig): void {
   // Ensure chroot home subdirectories exist with correct ownership before Docker
   // bind-mounts them. If a source directory doesn't exist, Docker creates it as
   // root:root, making it inaccessible to the agent user (e.g., UID 1001).
   // Also create an empty writable home directory that gets mounted as $HOME
   // in the chroot, giving tools a writable home without exposing credentials.
-  {
-    const effectiveHome = getRealUserHome();
-    const uid = parseInt(getSafeHostUid(), 10);
-    const gid = parseInt(getSafeHostGid(), 10);
+  const effectiveHome = getRealUserHome();
+  const uid = parseInt(getSafeHostUid(), 10);
+  const gid = parseInt(getSafeHostGid(), 10);
 
-    // Create empty writable home directory for the chroot
-    // This is mounted as $HOME inside the container so tools can write to it
-    // NOTE: Must be outside workDir to avoid being hidden by the tmpfs overlay
-    const emptyHomeDir = `${config.workDir}-chroot-home`;
-    if (!fs.existsSync(emptyHomeDir)) {
-      fs.mkdirSync(emptyHomeDir, { recursive: true });
-    }
-    fs.chownSync(emptyHomeDir, uid, gid);
-    logger.debug(`Created chroot home directory: ${emptyHomeDir} (${uid}:${gid})`);
+  // Create empty writable home directory for the chroot
+  // This is mounted as $HOME inside the container so tools can write to it
+  // NOTE: Must be outside workDir to avoid being hidden by the tmpfs overlay
+  const emptyHomeDir = `${config.workDir}-chroot-home`;
+  if (!fs.existsSync(emptyHomeDir)) {
+    fs.mkdirSync(emptyHomeDir, { recursive: true });
+  }
+  fs.chownSync(emptyHomeDir, uid, gid);
+  logger.debug(`Created chroot home directory: ${emptyHomeDir} (${uid}:${gid})`);
 
-    // Ensure source directories for home subdirectory mounts exist with correct ownership.
-    const hostHomeMountSourceDirs = [
-      '.copilot', '.cache', '.config', '.local',
-      '.anthropic', '.claude', '.cargo', '.rustup', '.npm', '.nvm',
-      ...(config.geminiApiKey ? ['.gemini'] : []),
-    ];
-    for (const dir of hostHomeMountSourceDirs) {
-      const dirPath = path.join(effectiveHome, dir);
-      if (!fs.existsSync(dirPath)) {
-        fs.mkdirSync(dirPath, { recursive: true });
-        fs.chownSync(dirPath, uid, gid);
-        logger.debug(`Created host home subdirectory: ${dirPath} (${uid}:${gid})`);
-      }
+  // Ensure source directories for home subdirectory mounts exist with correct ownership.
+  const hostHomeMountSourceDirs = [
+    '.copilot', '.cache', '.config', '.local',
+    '.anthropic', '.claude', '.cargo', '.rustup', '.npm', '.nvm',
+    ...(config.geminiApiKey ? ['.gemini'] : []),
+  ];
+  for (const dir of hostHomeMountSourceDirs) {
+    const dirPath = path.join(effectiveHome, dir);
+    if (!fs.existsSync(dirPath)) {
+      fs.mkdirSync(dirPath, { recursive: true });
+      fs.chownSync(dirPath, uid, gid);
+      logger.debug(`Created host home subdirectory: ${dirPath} (${uid}:${gid})`);
     }
+  }
 
-    // Source-side prep: this only applies when the config file explicitly names
-    // a runner tool-cache source path that does not exist yet. Create that host
-    // source so Docker has something real to bind-mount later.
-    if (config.runnerToolCachePath && !fs.existsSync(config.runnerToolCachePath)) {
-      const relToHome = path.relative(effectiveHome, config.runnerToolCachePath);
-      const isUnderHome = relToHome && !relToHome.startsWith('..') && !path.isAbsolute(relToHome);
+  // Source-side prep: this only applies when the config file explicitly names
+  // a runner tool-cache source path that does not exist yet. Create that host
+  // source so Docker has something real to bind-mount later.
+  if (config.runnerToolCachePath && !fs.existsSync(config.runnerToolCachePath)) {
+    const relToHome = path.relative(effectiveHome, config.runnerToolCachePath);
+    const isUnderHome = relToHome && !relToHome.startsWith('..') && !path.isAbsolute(relToHome);
 
-      if (isUnderHome) {
-        createMissingOwnedDirectorySegments(config.runnerToolCachePath, uid, gid);
-        logger.debug(`Created runner tool cache directory: ${config.runnerToolCachePath} (${uid}:${gid})`);
-      } else {
-        logger.warn(`Runner tool cache path does not exist; refusing to create outside effective home (${effectiveHome}): ${config.runnerToolCachePath}`);
-      }
+    if (isUnderHome) {
+      createMissingOwnedDirectorySegments(config.runnerToolCachePath, uid, gid);
+      logger.debug(`Created runner tool cache directory: ${config.runnerToolCachePath} (${uid}:${gid})`);
+    } else {
+      logger.warn(`Runner tool cache path does not exist; refusing to create outside effective home (${effectiveHome}): ${config.runnerToolCachePath}`);
     }
+  }
 
-    // Destination-side prep: resolve the same source path that home-strategy.ts
-    // will mount. If that source is nested under the empty chroot home, prepare
-    // the placeholder mountpoint there so Docker does not create parents as root.
-    const runnerToolCachePath = resolveRunnerToolCachePath(config, effectiveHome);
-    if (runnerToolCachePath) {
-      const relativeToolCachePath = path.relative(effectiveHome, runnerToolCachePath);
-      if (relativeToolCachePath && !relativeToolCachePath.startsWith('..') && !path.isAbsolute(relativeToolCachePath)) {
-        const chrootToolCachePath = prepareChrootHomeMountpoint(emptyHomeDir, relativeToolCachePath, uid, gid);
-        logger.debug(`Prepared chroot runner tool cache mountpoint: ${chrootToolCachePath} (${uid}:${gid})`);
-      }
+  // Destination-side prep: resolve the same source path that home-strategy.ts
+  // will mount. If that source is nested under the empty chroot home, prepare
+  // the placeholder mountpoint there so Docker does not create parents as root.
+  const runnerToolCachePath = resolveRunnerToolCachePath(config, effectiveHome);
+  if (runnerToolCachePath) {
+    const relativeToolCachePath = path.relative(effectiveHome, runnerToolCachePath);
+    if (relativeToolCachePath && !relativeToolCachePath.startsWith('..') && !path.isAbsolute(relativeToolCachePath)) {
+      const chrootToolCachePath = prepareChrootHomeMountpoint(emptyHomeDir, relativeToolCachePath, uid, gid);
+      logger.debug(`Prepared chroot runner tool cache mountpoint: ${chrootToolCachePath} (${uid}:${gid})`);
     }
   }
 }
 
+/**
+ * Prepares all working directories required before container startup.
+ *
+ * Delegates to two focused sub-functions:
+ * - {@link prepareLogDirectories} — log/state directory setup
+ * - {@link prepareChrootHomeMounts} — chroot home bind-mount preparation
+ */
+export function prepareWorkDirectories(config: WrapperConfig, logPaths: LogPaths): void {
+  prepareLogDirectories(logPaths);
+  prepareChrootHomeMounts(config);
+}
+
 /** @internal Exposed only for unit tests — not part of the public API. */
 // ts-prune-ignore-next
 export const workdirSetupTestHelpers = {
   ensureDirectory,
   assertRealDirectory,
   createMissingOwnedDirectorySegments,
   prepareChrootHomeMountpoint,
+  prepareLogDirectories,
+  prepareChrootHomeMounts,
 };