docs(client): Agent Skills — close out the security review's SDK-side remainder

packages/client/src/__tests__/lifecycle.test.ts

@@ -247,6 +247,23 @@ describe('lifecycle', () => {
       expect(mockTracerProviderRegister).toHaveBeenCalled();
     });
 
+    it('passes the BYOC overload options through to telemetry setup', async () => {
+      // The second argument is the same options bag as the other overload, so
+      // `otlpEndpoint` must reach the exporter rather than being dropped on the
+      // floor while only `skillStore` is read off it.
+      const byocClient = {
+        variation: vi.fn(),
+        track: vi.fn(),
+        flush: vi.fn().mockResolvedValue(undefined),
+        close: vi.fn().mockResolvedValue(undefined),
+      };
+      const { initClient } = await import('../lifecycle.js');
+      await initClient(byocClient, { otlpEndpoint: 'https://otlp.example.test/' });
+      expect(mockOTLPTraceExporter).toHaveBeenCalledWith(
+        expect.objectContaining({ url: 'https://otlp.example.test/v1/traces' }),
+      );
+    });
+
     it('is idempotent — calls init only once when called twice', async () => {
       const mockClient = makeMockClient();
       mockLdInit.mockReturnValue(mockClient);

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

@@ -36,9 +36,15 @@ vi.mock('node:fs/promises', async () => {
   };
 });
 
-const { atomicWrite, atomicWriteIn, openDirectoryNoFollow, openOrCreateDirectory, unlinkNoFollow } = await import(
-  '../safe-fs.js'
-);
+const {
+  atomicWrite,
+  directoryAddress,
+  isDescriptorAddressed,
+  openDirectoryNoFollow,
+  openOrCreateDirectory,
+  SUPPORTS_PROC_FD,
+  unlinkNoFollow,
+} = await import('../safe-fs.js');
 
 let scratch: string;
 
@@ -158,26 +164,28 @@ describe('atomicWrite', () => {
     }
     expect(await readdir(dir)).toEqual(['SKILL.md']);
   });
-
-  it('atomicWriteIn refuses a symlinked directory', async () => {
-    const outside = path.join(scratch, 'outside');
-    await mkdir(outside);
-    const link = path.join(scratch, 'link');
-    await symlink(outside, link, 'dir');
-
-    await expect(atomicWriteIn(link, 'SKILL.md', Buffer.from('body\n', 'utf-8'))).rejects.toThrow();
-    expect(await readdir(outside)).toEqual([]);
-  });
 });
 
 /**
  * On Node the identity re-check is the *whole* defense against a directory swapped
  * between validation and the destructive call: there is no `*at()` family, so the
  * rename and the unlink resolve their directory by path.
  *
- * The swap is staged directly: pin the handle, replace the directory, then invoke
- * the primitive. Both primitives re-check independently, so both halves are
- * required.
+ * Off `SUPPORTS_PROC_FD` it is the *whole* defense against a directory swapped
+ * between validation and the destructive call: Node exposes no `*at()` family, so
+ * the rename and the unlink resolve their directory by path. The swap-race cases
+ * in `skills-fs.test.ts` cannot reach it — they fire the swap from the
+ * rename/unlink hook, which by construction runs *after* the check. Verified by
+ * mutation: with the check neutered, the entire suite stays green.
+ *
+ * So the swap is staged here instead: pin the handle, then replace the directory,
+ * then invoke the primitive. Both primitives re-check independently, so both
+ * halves are required.
+ *
+ * These call the primitives with a plain path, which is what keeps them
+ * meaningful on Linux too: the check is skipped only for a directory addressed
+ * through `directoryAddress`, so passing a path exercises the floor on every
+ * platform rather than testing nothing wherever the fast path exists.
  */
 describe('pinned-directory identity re-check', () => {
   /** Moves `dir` aside and leaves a symlink to `outside` in its place. */
@@ -199,11 +207,11 @@ describe('pinned-directory identity re-check', () => {
       await swapForSymlink(dir, outside);
 
       await expect(atomicWrite(dir, 'SKILL.md', Buffer.from('body\n', 'utf-8'), handle)).rejects.toThrow(/replaced/i);
-      // The temp file *is* created through the swapped link before the check fires
-      // (only the final component gets O_NOFOLLOW), so this proves the rename is
-      // refused and the temp cleaned up — not that the outside directory was never
-      // touched at all.
+      // The identity is re-checked *before* the temp file is opened as well as
+      // before the rename, so nothing — not even a temp file — is written
+      // through the swapped link. The outside directory was never touched.
       expect(await readdir(outside)).toEqual([]);
+      expect(tempOpens()).toEqual([]);
     } finally {
       await handle.close();
     }
@@ -237,6 +245,149 @@ describe('pinned-directory identity re-check', () => {
   });
 });
 
+/**
+ * The Linux fast path's own positive control.
+ *
+ * The block above tests the *floor* — deliberately, by passing a plain path, so it
+ * exercises the identity re-check on every platform. That leaves the fast path
+ * itself unproven by anything: on Linux the defense lives in how the call is
+ * *addressed* rather than in a check a test can observe failing, and the §3.23.2
+ * swap races in `skills-fs.test.ts` and `skills-fs-root-swap.test.ts` go through
+ * the materialization layer, where the shared path check and the primitive's own
+ * `O_NOFOLLOW` both stand in the way — so they pass when either layer alone works
+ * and prove the pair rather than either member.
+ *
+ * So assert the property directly and at this layer: nothing else establishes that
+ * `directoryAddress()` resolves from the inode the handle is pinned to rather than
+ * from whatever the directory's *name* resolves to when the call is made. This is
+ * the positive mirror of the floor tests — there the contract is a refusal, here it
+ * is that the write lands in the right place despite the swap.
+ *
+ * Gated on `SUPPORTS_PROC_FD` because it tests that capability, not the floor;
+ * that makes it **Linux-only**, and a green macOS run is no evidence about it.
+ */
+describe.skipIf(!SUPPORTS_PROC_FD)('descriptor addressing on the /proc/self/fd fast path', () => {
+  it('atomicWrite lands in the pinned inode after the directory is swapped for a symlink', async () => {
+    const dir = path.join(scratch, 'skill');
+    const outside = path.join(scratch, 'outside');
+    await mkdir(dir);
+    await mkdir(outside);
+
+    const handle = await openDirectoryNoFollow(dir);
+    try {
+      // Built *before* the swap, which is the point: the address holds the
+      // descriptor, so it keeps naming this inode no matter what happens to the
+      // name `dir`.
+      const address = directoryAddress(handle, dir);
+      expect(address).not.toBe(dir);
+
+      const movedTo = `${dir}.real`;
+      await rename(dir, movedTo);
+      await symlink(outside, dir, 'dir');
+      // The swap really is in place: `dir` now resolves outside.
+      expect((await lstat(dir)).isSymbolicLink()).toBe(true);
+
+      await atomicWrite(address, 'SKILL.md', Buffer.from('body\n', 'utf-8'), handle);
+
+      // Landed in the directory that was pinned, which is now `movedTo`.
+      expect(await readFile(path.join(movedTo, 'SKILL.md'), 'utf-8')).toBe('body\n');
+      // And nothing at all reached the attacker's directory — not even a temp
+      // file, unlike the floor, where the temp is created through the link before
+      // the check fires.
+      expect(await readdir(outside)).toEqual([]);
+    } finally {
+      await handle.close();
+    }
+  });
+
+  it('unlinkNoFollow removes from the pinned inode after the directory is swapped for a symlink', async () => {
+    // The destructive half. `unlink` never follows a trailing symlink, but it does
+    // resolve the directory above it — so a path-addressed unlink through a
+    // swapped directory removes the outside file. The victim is a real file with
+    // the managed name, so only the addressing can be what saves it.
+    const dir = path.join(scratch, 'skill');
+    const outside = path.join(scratch, 'outside');
+    await mkdir(dir);
+    await mkdir(outside);
+    await writeFile(path.join(dir, 'SKILL.md'), 'managed\n', 'utf-8');
+    const victim = path.join(outside, 'SKILL.md');
+    await writeFile(victim, 'victim content\n', 'utf-8');
+
+    const handle = await openDirectoryNoFollow(dir);
+    try {
+      const address = directoryAddress(handle, dir);
+      const movedTo = `${dir}.real`;
+      await rename(dir, movedTo);
+      await symlink(outside, dir, 'dir');
+
+      await unlinkNoFollow(address, 'SKILL.md', handle);
+
+      // The managed file — the one that was pinned — is the one that went.
+      expect(await readdir(movedTo)).toEqual([]);
+      expect(await readFile(victim, 'utf-8')).toBe('victim content\n');
+    } finally {
+      await handle.close();
+    }
+  });
+});
+
+describe('isDescriptorAddressed', () => {
+  // Pure string logic, ungated: it decides whether the identity re-check is
+  // skipped, so a lookalike must never be mistaken for the descriptor address.
+  it('accepts exactly the address built for this handle, and nothing else on procfs', async () => {
+    const handle = await openDirectoryNoFollow(scratch);
+    try {
+      expect(isDescriptorAddressed(`/proc/self/fd/${handle.fd}`, handle)).toBe(true);
+      expect(isDescriptorAddressed(scratch, handle)).toBe(false);
+      expect(isDescriptorAddressed('/tmp/proc/self/fd/3', handle)).toBe(false);
+      for (const shape of [
+        '/proc/self/fd',
+        '/proc/self/fd/',
+        `/proc/self/fd/${handle.fd}/`,
+        `/proc/self/fd/${handle.fd}/child`,
+        `/proc/self/fd/${handle.fd + 1}`,
+        '/proc/self/fd/../../etc',
+      ]) {
+        expect(() => isDescriptorAddressed(shape, handle), shape).toThrow(/descriptor/i);
+      }
+    } finally {
+      await handle.close();
+    }
+  });
+});
+
+describe('single path component names', () => {
+  // `name` is the one argument that becomes part of a path this module writes
+  // or unlinks; a caller passing `../x` would be addressing outside the pinned
+  // directory. The check is structural rather than trusting every caller.
+  const badNames = ['', '.', '..', 'a/b', '../SKILL.md', 'SKILL.md/', '/SKILL.md'];
+
+  it.each(badNames)('atomicWrite refuses name %j', async (name) => {
+    const dir = path.join(scratch, 'skill');
+    await mkdir(dir);
+    const handle = await openDirectoryNoFollow(dir);
+    try {
+      await expect(atomicWrite(dir, name, Buffer.from('body\n'), handle)).rejects.toThrow(/single path component/);
+    } finally {
+      await handle.close();
+    }
+    expect(tempOpens()).toEqual([]);
+  });
+
+  it.each(badNames)('unlinkNoFollow refuses name %j', async (name) => {
+    const dir = path.join(scratch, 'skill');
+    await mkdir(dir);
+    await writeFile(path.join(dir, 'SKILL.md'), 'body\n', 'utf-8');
+    const handle = await openDirectoryNoFollow(dir);
+    try {
+      await expect(unlinkNoFollow(dir, name, handle)).rejects.toThrow(/single path component/);
+    } finally {
+      await handle.close();
+    }
+    expect(await readdir(dir)).toEqual(['SKILL.md']);
+  });
+});
+
 describe('unlinkNoFollow', () => {
   it('removes a real file', async () => {
     const dir = path.join(scratch, 'skill');