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

The Node half of the four items left open in the response to the Agent Skills
security design review. Three are documentation, one is tests; no behavior
changes, and `safe-fs.ts`'s implementation is deliberately untouched.

**Privilege separation** (row 9 docs half, and the agreed counter-proposal for
row 26). The recommended deployment runs the reconcile as a different identity
than the agent, which is the whole reason the `0644`/`0755` modes deny
anything: the agent reads its instructions and cannot rewrite them, or the
manifest. That is the mitigation for AZ-1, a prompt-injected agent editing its
own skills. Write access to the manifest is the worse half — it is what tells
the *next* reconcile which paths the SDK may delete — which is why the prune
path re-validates every entry from scratch rather than trusting it.

It matters more here than in Python, because this runtime's residual race
*requires* write permission on the managed root, and privilege separation is
what denies it. The README's security-posture note now points at the new
section instead of ending on a bare "keep the root writable only by the SDK".

The README also hands the operator the check to run, because the SDK cannot
run it: it knows only its own identity, which trivially has write access,
having just written there. So `ReconcileReport` grows no writability field —
the review asked for one and we declined, since any check the SDK could make
would answer a different question than the one asked and manufacture false
confidence exactly where caution is wanted. `agents.md` records that reasoning
so the field is not added later by someone reading its absence as an oversight.

**Three hostile-manifest prune tests** (row 12 remainder): a well-formed
manifest listing `/etc/passwd`, `../../../etc/passwd`, and a path under a
parent that has since become a symlink. The prune path already refuses all
three, so these turn asserted into verified, and they mirror the Python suite
case for case. Two things make them worth more than their line count. They are
deliberately *well-formed* — the corrupt-manifest block above them proves
nothing here, because a corrupt manifest suppresses every destructive action
wholesale, whereas these manifests give the implementation everything it needs
to prune. And "deleted nothing" is asserted through an `fsOps.unlink` spy
rather than by checking that `/etc/passwd` still exists: the test process
cannot delete that file anyway, so the obvious assertion would pass against an
implementation with no path check at all.

**One sentence on** `'*'` (row 16 remainder). It materializes the whole
project library, so every skill's `description` enters the agent's context —
including skills no AI Config references and skills belonging to other teams.

**The platform bound is now explicit** (row 2 residual), in `safe-fs.ts`,
`agents.md` and the README — and it is a sharper bound here than in Python.
Node exposes no `*at()` family on *any* platform, so the racy per-component
`lstat` floor is not the Windows fallback it is in Python; it is the only
implementation, Linux included. The README previously said this of the rename
alone; it is true of every destructive step. Windows reparse-point checks
(`GetFileAttributesW` / `FILE_FLAG_OPEN_REPARSE_POINT`) are not implemented,
by decision: Windows is not a supported or tested platform for this release,
neither repository has a Windows CI runner, and Node offers no primitive that
would make such checks meaningful here anyway. This retroactively lowers the
priority of the row 25 reserved-device-name work, noted where that code lives.

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

Author: XieX

packages/client/README.md

@@ -311,6 +311,12 @@ const report = await writeSkills('*', '.claude/skills', {
 });
 ```
 
+**Know what `'*'` asks for.** It materializes the **whole project library**, which puts every
+skill's `description` into the agent's context — including skills no AI Config references and
+skills belonging to other teams. `writeSkills(skillRefs(config), root)`, the form used in the
+example above, materializes only what the resolved variation actually asked for. Reach for
+`'*'` when you genuinely want the entire library on disk.
+
 | Export | Description |
 |---|---|
 | `skillRefs(config)` | Project a config's `skills` array into typed `SkillReference[]`. Pure — no client, no store, no telemetry. `[]` when absent. |
@@ -330,7 +336,7 @@ const report = await writeSkills('*', '.claude/skills', {
 
 `ReconcileReport` exposes `actions`, `ok` (true iff no action is an `error`), and `errors` (the error actions, in order), so callers never re-derive the filter. Each `ReconcileAction` carries `key`, `action` (`written` | `updated` | `skipped_current` | `removed` | `error`), and nullable `version` / `path` / `error`. A failure belonging to the whole run rather than one skill — a corrupt manifest, for instance — carries the **empty string** in `key`.
 
-**Security posture.** `writeSkills` is writing LaunchDarkly-delivered content to your disk, so it fails closed: skill keys are re-validated locally, content is hash-verified again immediately before writing, writes go through a temp file in the target's own directory and an atomic rename at mode `0644`, symlinked roots/directories/targets are refused, a target that is not a regular file (a FIFO, a device node) is refused rather than read, and a corrupt manifest suppresses every destructive action. One limitation is worth stating plainly: Node exposes no `renameat`/`unlinkat`, so the final rename cannot be performed relative to a pinned directory descriptor. An attacker who already has **write permission on the managed root** can therefore still win a race to redirect a write or a delete outside it. Keep the managed root writable only by the process running the SDK.
+**Security posture.** `writeSkills` is writing LaunchDarkly-delivered content to your disk, so it fails closed: skill keys are re-validated locally, content is hash-verified again immediately before writing, writes go through a temp file in the target's own directory and an atomic rename at mode `0644`, symlinked roots/directories/targets are refused, a target that is not a regular file (a FIFO, a device node) is refused rather than read, and a corrupt manifest suppresses every destructive action. One limitation is worth stating plainly, and it is a platform bound rather than a detail: Node exposes no `renameat`/`unlinkat`/`openat`, so no destructive step can be performed relative to a pinned directory descriptor. Every check here is therefore a per-component `lstat` taken immediately before the path-based operation — a check-then-use race, not a closed window — and unlike the Python SDK, which closes that window on POSIX by holding a descriptor opened `O_RDONLY|O_DIRECTORY|O_NOFOLLOW` for the whole reconcile, **this floor is what runs on every platform Node supports, Linux included.** An attacker who already has **write permission on the managed root** can therefore win a race to redirect a write or a delete outside it. Windows adds nothing worse and nothing better: reparse-point checks (`GetFileAttributesW` / `FILE_FLAG_OPEN_REPARSE_POINT`) are deliberately not implemented in this release, and Windows is not a tested platform for it — neither SDK repository has a Windows CI runner. So write permission on the managed root is *the* security boundary here. Keep it writable only by the identity running the reconcile — see [privilege separation](#privilege-separation-the-agent-must-not-be-able-to-rewrite-its-own-skills).
 
 **Two constraints on skill keys, imposed here rather than by the data model.** A key becomes a single directory name, so `writeSkills` rejects — as a reported `error` action, on every platform — a key over 255 bytes and the 22 Windows reserved device names (`con`, `prn`, `aux`, `nul`, `com1`–`com9`, `lpt1`–`lpt9`). Both are checked in the filesystem layer only: a key like `aux` remains valid everywhere else, so an AI Config referencing it still parses and its other skills still materialize. One residual the SDK cannot check for you: the 255-byte bound is per *path component*, not on the total path, so `<root>/<key>/SKILL.md` can still exceed Windows' 260-character `MAX_PATH` if the root is deep and the key is long. The root is yours, so budget for it there.
 
@@ -425,6 +431,31 @@ These five tokens are the whole vocabulary, and the Python SDK publishes the sam
 
 **`hash_mismatch` deserves a page, not a dashboard.** The other codes are consistent with a malformed store, a bad deployment, or a truncated response. `hash_mismatch` means content and its declared digest disagree, which is the shape of active tampering with skill delivery — in transit, in a cache, or in whatever backs your `SkillStore`. If you serve skill content only from LaunchDarkly, `over_size_cap` and `not_utf8` warrant alerts on the same reasoning: neither should ever occur.
 
+#### Privilege separation: the agent must not be able to rewrite its own skills
+
+**The recommended deployment runs `writeSkills` as a different identity than the agent.** Reconcile as one user, run the agent as another. Everything the reconcile puts on disk is owner-write-only, and set explicitly rather than inherited from the process umask: skill files and the manifest at `0644` (applied to the open file handle, so it cannot be redirected), the per-skill `<root>/<key>/` directories at `0755`, and the execute bit never set on anything. Those modes are only a defense if the two identities actually differ — under a single identity they describe a directory the agent can freely rewrite.
+
+**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: confirm the agent's identity has no write access to
+
+- the managed root itself,
+- the per-skill directories `<root>/<key>/` and the files `<root>/<key>/SKILL.md`,
+- the manifest at `<root>/.launchdarkly-skills.json`.
+
+```bash
+# Run as the agent's user. Every line should print DENIED.
+root=.claude/skills
+for target in "$root" "$root/.launchdarkly-skills.json" "$root"/*/ "$root"/*/SKILL.md; do
+  [ -e "$target" ] || continue
+  if [ -w "$target" ]; then echo "WRITABLE — fix this: $target"; else echo "DENIED: $target"; fi
+done
+```
+
+The managed root's own mode is **yours, not the SDK's**: `writeSkills` creates only that one leaf directory, and does so with the process umask, precisely because the root is a path you chose. Own it — `chown reconcile-user:agent-group` plus `chmod 0755` on the root is the shape that makes the rest of the tree's modes mean something. It is also the mitigation for the race described under *Security posture* above: that race requires write permission on the managed root, which privilege separation is what denies.
+
+**Why this is the mitigation that matters.** A `SKILL.md` is agent *instructions*. An agent that can write its own skills directory can rewrite its own instructions, and an agent processing untrusted input is exactly the thing that might be induced to do so. Write access to the manifest is worse than write access to a skill, because the manifest is what tells the *next* reconcile which paths the SDK owns and may delete: an agent that can edit it can keep a skill LaunchDarkly has revoked, or aim the SDK's own delete path at something it should not touch. `writeSkills` re-validates every manifest entry from scratch for exactly that reason — it treats that file as untrusted input, never as authorization — but an agent that cannot edit it at all is the stronger position, and only your deployment can provide that.
+
+**The SDK deliberately does not report whether the root is writable.** There is no such field on `ReconcileReport`, and its absence is a decision rather than an oversight. The SDK knows only its own identity, which trivially has write access — it just wrote there. It cannot know which identity will later run the agent, so any check it could make would answer a different question than the one that matters, and would read as reassurance exactly where caution is wanted. You know both identities; the SDK knows one.
+
 ---
 
 ### Utility Helpers

packages/client/agents.md

@@ -265,6 +265,13 @@ Three specifics inside those two functions that a later contributor is most like
 
 `safe-fs.ts` routes the final rename and the managed-file unlink through the `fsOps` record so tests can intercept exactly those two operations. The orphaned-temp sweep goes through `unlinkNoFollow` for the same reason, and derives its filename pattern from `tempNamePattern` in `safe-fs.ts` rather than carrying a copy: that sweep is only entitled to unlink a file because the *name* identifies it as one this SDK created, so two spellings of the naming rule would eventually let it either miss orphans or remove something it did not write. Calling `fs.rename`/`fs.unlink` directly makes the operation invisible to the atomicity and "no operation was attempted" assertions, which then pass vacuously. Note also the limitation those tests document: Node exposes no `renameat`/`unlinkat`, so `SUPPORTS_DIR_FD` is `false` and the TOCTOU swap-race tests are skipped — the residual exposure is real and recorded, not fixed.
 
+### 7. "Fixing" the platform bound, or adding a writability field to `ReconcileReport`
+
+Two decisions here look like unfinished work and are not. Do not reverse either without reopening the security review.
+
+- **Windows reparse-point checks (`GetFileAttributesW`, `FILE_FLAG_OPEN_REPARSE_POINT`) are deliberately not implemented.** Windows is not a supported or tested platform for this release, neither repository has a Windows CI runner, and Node gives this module no `*at()` primitive that would make such checks meaningful anyway — the racy per-component `lstat` floor is universal here, Linux included, not a Windows-only fallback as it is in Python. The bound is documented in `safe-fs.ts` and the README instead. It also retroactively lowers the priority of the reserved-device-name work in `skills-fs.ts`: keep that code, since it keeps a root written on Linux usable when read from Windows, but do not read it as evidence that Windows is hardened. If Windows becomes supported, add the CI runner first and revisit both together.
+- **`ReconcileReport` must not grow a "managed root is writable" field.** The security review asked for one; we declined, and the reasoning is load-bearing. The SDK knows only its *own* identity, which trivially has write access — it just wrote there — and cannot know which identity will later run the agent. Any check it could perform would answer a different question than the one asked and would manufacture false confidence exactly where caution is wanted. The real mitigation is deployment-side: run the reconcile as a different identity than the agent, so the `0644`/`0755` modes deny something and a prompt-injected agent cannot rewrite its own instructions or the manifest. The operator's verification steps live in the README.
+
 ---
 
 ## Adding a New Export

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

@@ -1626,6 +1626,113 @@ describe('writeSkills corrupt manifest', () => {
   });
 });
 
+// ─── A well-formed manifest naming a path the SDK could not have written ─────
+
+/**
+ * The three literal cases the security review names for the prune path.
+ *
+ * The distinction from the corrupt-manifest block above is the whole point: a
+ * corrupt manifest suppresses every destructive action wholesale, so those tests
+ * say nothing about these. Each manifest here is *well-formed* — parseable, a
+ * `manifestVersion` this release understands, a real `entries` map, and an entry
+ * whose `key` is a perfectly valid skill key genuinely absent from the requested
+ * set. The implementation has every input it needs to prune, and must refuse
+ * anyway, because the recorded *path* is not one this SDK could have written.
+ *
+ * The manifest is untrusted input: a plain file on the customer's disk that
+ * anything with write access to the managed root can edit, and `prune` is the
+ * one code path in the SDK that deletes. So a recorded path never authorizes its
+ * own removal — it must match `<key>/SKILL.md` for a re-validated key, and the
+ * target is recomputed from the *current* managed root rather than read back out
+ * of the entry.
+ */
+const HOSTILE_RECORDED_PATHS: string[] = [
+  // Absolute: the classic. A recorded path read back and unlinked as-is is a
+  // delete of an attacker-chosen file with the reconcile's own privileges.
+  '/etc/passwd',
+  // Traversing: the same attack against an implementation that rejects a
+  // leading slash and then joins the rest onto the managed root.
+  '../../../etc/passwd',
+];
+
+/**
+ * Records every prune unlink without performing it.
+ *
+ * Asserting only that `/etc/passwd` still exists proves nothing: the test
+ * process cannot delete it anyway, so that assertion passes against an
+ * implementation with no path check at all — permissions would be doing the
+ * work. What has teeth is that the removal is never *attempted*: the refusal
+ * happens above the syscall, on a path the SDK recomputes rather than trusts.
+ * `fsOps.unlink` is the single call site the prune deletes through.
+ */
+function recordUnlinks(): string[] {
+  const targets: string[] = [];
+  vi.spyOn(fsOps, 'unlink').mockImplementation(async (target: string) => {
+    targets.push(target);
+  });
+  return targets;
+}
+
+describe('writeSkills hostile manifest prune', () => {
+  it.each(HOSTILE_RECORDED_PATHS)('refuses a recorded path outside the root: %s', async (recorded) => {
+    const unlinked = recordUnlinks();
+    await writeManifest(root, {
+      manifestVersion: 1,
+      entries: { [recorded]: manifestEntry('a', 1, SKILL_BODY) },
+    });
+
+    const report = await writeSkills([], root);
+
+    expect(report.ok).toBe(false);
+    const action = actionsByKey(report).a;
+    expect(action.action).toBe('error');
+    // The refusal is about ownership of the path, not about the file's state.
+    expect(action.error).toContain('could own');
+    expect(report.actions.filter((a) => a.action === 'removed')).toEqual([]);
+    // Nothing was even attempted, let alone completed.
+    expect(unlinked).toEqual([]);
+    expect(await exists('/etc/passwd')).toBe(true);
+    // Left in place rather than tidied away: dropping the entry would let a
+    // single hostile edit erase the SDK's own record of what it manages.
+    expect(await readManifest(root)).toHaveProperty(['entries', recorded]);
+  });
+
+  it('refuses an entry under a parent that has since become a symlink', async () => {
+    // The recorded path is the SDK's own, and is still not enough. The entry is
+    // exactly what a legitimate reconcile writes — `a/SKILL.md` under key `a` —
+    // so the shape check that catches the two cases above passes here. What
+    // changed is the disk underneath it. This is the case a validate-then-act
+    // implementation fails: the manifest and the entry are both entirely
+    // legitimate, and only the current state of the parent is not.
+    const elsewhere = path.join(scratch, 'elsewhere');
+    await mkdir(elsewhere);
+    const victim = path.join(elsewhere, SKILL_MD);
+    await writeFile(victim, 'victim content\n', 'utf-8');
+
+    // Managed legitimately first, so the manifest entry is one this SDK really
+    // did write...
+    const managed = await placeManaged(root, 'a', SKILL_BODY);
+    // ...then the parent directory is swapped for a link out of the root.
+    await rm(managed);
+    await rm(path.join(root, 'a'), { recursive: true });
+    await symlink(elsewhere, path.join(root, 'a'), 'dir');
+
+    const unlinked = recordUnlinks();
+    const report = await writeSkills([], root);
+
+    expect(report.ok).toBe(false);
+    const action = actionsByKey(report).a;
+    expect(action.action).toBe('error');
+    expect(action.error).toContain('symlink');
+    expect(report.actions.filter((a) => a.action === 'removed')).toEqual([]);
+    expect(unlinked).toEqual([]);
+    // The file the symlink pointed at is untouched, and so is the link.
+    expect(await readFile(victim, 'utf-8')).toBe('victim content\n');
+    expect((await lstat(path.join(root, 'a'))).isSymbolicLink()).toBe(true);
+    expect(await readManifest(root)).toHaveProperty(['entries', `a/${SKILL_MD}`]);
+  });
+});
+
 // ─── Telemetry seam (write half) ───────────────────────────────────────
 
 describe('writeSkills telemetry', () => {

packages/client/src/safe-fs.ts

@@ -13,6 +13,26 @@
  * opened with `O_NOFOLLOW`, every temp file is created exclusively in the
  * target's own directory, and the pinned directory's identity is re-checked
  * immediately before each destructive step.
+ *
+ * **Platform bound — the floor is what runs everywhere, deliberately.** The
+ * Python SDK closes the swap window on POSIX with a descriptor walk; this module
+ * cannot, on any platform, because Node exposes no `*at()` family at all (see
+ * {@link SUPPORTS_DIR_FD}). So unlike Python, where the racy floor is the
+ * Windows-only fallback, here it is the *only* implementation — Linux included.
+ * Windows is additionally not a supported or tested platform for this release:
+ * reparse-point checks (`GetFileAttributesW`, or opening with
+ * `FILE_FLAG_OPEN_REPARSE_POINT`) are **not implemented, by decision rather than
+ * oversight**, since neither SDK repository has a Windows CI runner and Node
+ * gives this module no primitive that would make them meaningful.
+ *
+ * The consequence is a single sentence, and it belongs in every deployment
+ * review: write permission on the managed root is *the* security boundary for
+ * skills materialization, so the privilege-separated deployment the README
+ * documents — reconcile identity separate from agent identity — is not advice but
+ * the mitigation. Relatedly, this bound retroactively lowers the priority of the
+ * Windows reserved-device-name work in `skills-fs.ts`: that code stays, because
+ * it keeps a managed root written on Linux usable when read from Windows, but it
+ * is not evidence that Windows is a hardened target. It is not.
  */
 
 import { randomBytes } from 'node:crypto';