fix(skills): set per-skill directory mode to 0755 through the pinned handle

packages/client/README.md

@@ -548,7 +548,7 @@ switch (outcome.reason) {
 
 #### Privilege separation: the agent must not be able to rewrite its own skills
 
-**Run `writeSkills` as a different identity than the agent.** Reconcile as one user, run the agent as another. The reconcile sets modes explicitly rather than from the process umask: skill files and the manifest at `0644` (applied to the open file handle, so it cannot be redirected), per-skill `<root>/<key>/` directories at `0755`, and never the execute bit. Those modes only protect anything if the two identities differ.
+**Run `writeSkills` as a different identity than the agent.** Reconcile as one user, run the agent as another. The reconcile sets modes explicitly rather than from the process umask: skill files and the manifest at `0644` (applied to the open file handle, so it cannot be redirected), per-skill `<root>/<key>/` directories at `0755` (keeping a setgid bit inherited from a setgid root, so a shared group still propagates), and never the execute bit. Those modes only protect anything if the two identities differ.
 
 **What to verify, as the identity that will run the agent.** The SDK cannot check this for you (see below), so make it a deployment step. The agent's identity must have no write access to:
 

packages/client/src/__tests__/safe-fs.test.ts

@@ -7,7 +7,20 @@
  */
 
 import { constants as fsConstants } from 'node:fs';
-import { lstat, mkdir, mkdtemp, readdir, readFile, realpath, rename, rm, symlink, writeFile } from 'node:fs/promises';
+import {
+  chmod,
+  lstat,
+  mkdir,
+  mkdtemp,
+  readdir,
+  readFile,
+  realpath,
+  rename,
+  rm,
+  stat,
+  symlink,
+  writeFile,
+} from 'node:fs/promises';
 import { tmpdir } from 'node:os';
 import path from 'node:path';
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
@@ -73,6 +86,17 @@ describe('openOrCreateDirectory', () => {
     }
   });
 
+  it.skipIf(process.platform === 'win32')('leaves the mode of an existing directory alone', async () => {
+    // The explicit 0755 is for directories this call creates; one that was
+    // already there keeps the mode its owner chose.
+    const dir = path.join(scratch, 'existing');
+    await mkdir(dir);
+    await chmod(dir, 0o700);
+    const handle = await openOrCreateDirectory(dir);
+    await handle.close();
+    expect((await stat(dir)).mode & 0o777).toBe(0o700);
+  });
+
   it('refuses an existing symlink-to-directory', async () => {
     // `mkdir(..., { recursive: true })` would treat this as "already there". The
     // plain mkdir plus an lstat on the EEXIST path is what refuses it.

packages/client/src/__tests__/skills-fs.test.ts

@@ -18,6 +18,7 @@ import {
   readFile,
   realpath,
   rm,
+  rmdir,
   stat,
   symlink,
   writeFile,
@@ -692,6 +693,47 @@ describe('writeSkills atomicity and permissions', () => {
     expect(mode & fsConstants.S_IXOTH).toBe(0);
   });
 
+  it.skipIf(process.platform === 'win32')(
+    'creates per-skill directories as 0755 even under a restrictive umask',
+    async () => {
+      // mkdir's mode is masked by the umask, so without an explicit chmod a 0077
+      // umask leaves <root>/<key>/ at 0700 and a separate agent identity cannot
+      // read the SKILL.md inside it.
+      const previous = process.umask(0o077);
+      let report: Awaited<ReturnType<typeof writeSkills>>;
+      try {
+        report = await writeSkills([skill('a')], root);
+      } finally {
+        process.umask(previous);
+      }
+      expect(report.ok).toBe(true);
+      expect((await stat(path.join(root, 'a'))).mode & 0o777).toBe(0o755);
+      expect((await stat(path.join(root, 'a', SKILL_MD))).mode & 0o777).toBe(0o644);
+    },
+  );
+
+  it.skipIf(process.platform === 'win32')('keeps a setgid bit the new skill directory inherited', async (ctx) => {
+    // Linux copies a setgid parent's bit onto a new directory so files inside
+    // inherit the shared group. The explicit 0755 must not clear it; fchmod
+    // sets exactly the bits it is given.
+    await chmod(root, 0o2755);
+    const probe = path.join(root, 'probe');
+    await mkdir(probe);
+    const inherits = ((await stat(probe)).mode & 0o2000) !== 0;
+    await rmdir(probe);
+    if (!inherits) ctx.skip();
+
+    const previous = process.umask(0o077);
+    let report: Awaited<ReturnType<typeof writeSkills>>;
+    try {
+      report = await writeSkills([skill('a')], root);
+    } finally {
+      process.umask(previous);
+    }
+    expect(report.ok).toBe(true);
+    expect((await stat(path.join(root, 'a'))).mode & 0o7777).toBe(0o2755);
+  });
+
   it('goes through a single atomic rename with the temp file in the target directory', async () => {
     // Positive control for the interception hook. Without this,
     // the `calls === []` assertions in the failure tests below and in the

packages/client/src/safe-fs.ts

@@ -31,6 +31,16 @@ import path from 'node:path';
 /** Mode set explicitly on every written file: never from the umask, never executable. */
 const FILE_MODE = 0o644;
 
+/**
+ * Permission bits set explicitly on every directory this module creates, because `mkdir`'s
+ * mode argument is masked by the umask: under `0077` a separate agent identity
+ * could not traverse the directory to read what is inside.
+ */
+const DIRECTORY_MODE = 0o755;
+
+/** The setgid bit. Not in `fs.constants`, which exposes no special mode bits. */
+const S_ISGID = 0o2000;
+
 /** Bound on the `O_EXCL` retry loop; with 64-bit random names it only keeps the loop finite. */
 const TEMP_NAME_ATTEMPTS = 5;
 
@@ -180,15 +190,33 @@ export async function openDirectoryNoFollow(directory: string): Promise<FileHand
  * `mkdir(..., { recursive: true })` accepts an existing symlink-to-directory.
  */
 export async function openOrCreateDirectory(directory: string): Promise<FileHandle> {
+  let created = false;
   try {
-    await mkdir(directory, 0o755);
+    await mkdir(directory, DIRECTORY_MODE);
+    created = true;
   } catch (error) {
     if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error;
     const info = await lstat(directory);
     if (info.isSymbolicLink()) throw new Error('the directory is a symlink');
     if (!info.isDirectory()) throw new Error('the path is not a directory');
   }
-  return openDirectoryNoFollow(directory);
+  const handle = await openDirectoryNoFollow(directory);
+  // Only a directory this call created: an existing one keeps the mode its
+  // owner gave it. Set on the handle, never chmod on the path, which a swap
+  // between mkdir and open could redirect.
+  if (created) {
+    try {
+      // Keep the setgid bit Linux copies from a setgid parent: fchmod sets
+      // exactly the bits given, so a bare 0755 would clear it and files
+      // written inside would stop inheriting the shared group.
+      const inherited = (await handle.stat()).mode & S_ISGID;
+      await handle.chmod(inherited | DIRECTORY_MODE);
+    } catch (error) {
+      await handle.close().catch(() => undefined);
+      throw error;
+    }
+  }
+  return handle;
 }
 
 /**