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

packages/client/README.md

@@ -272,7 +272,12 @@ async def main():
 asyncio.run(main())
 ```
 
-Pass `"*"` instead of a reference list to materialize every skill the store holds.
+Pass `"*"` instead of a reference list to materialize every skill the store holds — but know
+what you are asking for. `"*"` 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. `write_skills(skill_refs(...), root)` is the form used above
+because it materializes only what the resolved variation actually asked for; reach for `"*"`
+when you genuinely want the whole library on disk.
 
 **`skills` is now a validated field.** Config parsing fails closed on a `skills` value that
 is not a list of `{key, version}` objects (key matching `^[a-z0-9][a-z0-9-]*$`, version an
@@ -406,6 +411,19 @@ never writes through a symlink; writes are atomic (temp file, `fsync`, rename) a
 `0644`; and if the manifest is unreadable it performs no destructive action at all. Removing
 a skill from a variation is how revocation works — the next reconcile prunes it.
 
+**Platform bound: the descriptor-pinned guarantee is POSIX-only.** On POSIX every destructive
+step — the open, the rename, the unlink — runs relative to a directory descriptor opened
+`O_RDONLY|O_DIRECTORY|O_NOFOLLOW` and held for the whole reconcile, so a directory swapped for
+a symlink *after* its checks cannot redirect a write or a delete: the descriptor names the
+inode that was checked, which closes the swap window rather than narrowing it. Windows has no
+`*at()` syscall family, so there `write_skills` falls back to a per-component `lstat` check
+taken immediately before each step. That floor is a check-then-use race rather than a closed
+window: an attacker who already holds **write permission on the managed root** can still win
+it. Windows 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. Treat write permission on the managed root as
+the security boundary on every platform, and on Windows as the *only* one.
+
 **One exception, and it is what makes a crashed reconcile recoverable.** A file at a managed
 path whose bytes are *already byte-identical* to the content LaunchDarkly resolved is
 adopted — recorded in the manifest and reported `skipped_current` — rather than refused.
@@ -477,6 +495,54 @@ to key its own map however the transport underneath does.
 > what was hashed. The SDK never parses or interprets them; if you want the frontmatter,
 > decode and parse the content on your side.
 
+#### Privilege separation: the agent must not be able to rewrite its own skills
+
+**The recommended deployment runs `write_skills` 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 your umask: skill files and
+the manifest at `0644` (via `fchmod` on the descriptor, 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
+```
+
+Note that the managed root's own mode is **yours, not the SDK's**: `write_skills` creates only
+that one leaf directory and does so with your umask, precisely because the root is a path you
+chose. Own it — `chown reconcile-user:agent-group` and `chmod 0755` on the root is the shape
+that makes the rest of the tree's modes mean something.
+
+**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. `write_skills` 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

@@ -510,6 +510,31 @@ resolving to the skill directory's `(st_dev, st_ino)`) instead of by comparing p
 A spy must `fstat` the descriptor **inside** the intercepted call — the implementation closes
 it as soon as the write returns.
 
+**The platform bound is POSIX-only, and that is a decision — do not quietly "fix" it.**
+Windows reparse-point checks (`GetFileAttributesW`, `FILE_FLAG_OPEN_REPARSE_POINT`) are not
+implemented because Windows is not a supported or tested platform for this release: there is
+no Windows CI runner in either repository, so the checks would ship unverified, and the
+TypeScript SDK could not match them at all — Node exposes no `*at()` family on *any*
+platform, so its racy floor is universal rather than Windows-only. Implementing them in
+Python alone would break cross-language parity and trade a documented bound for an unverified
+one. Two follow-on facts: on Windows write permission on the managed root is the only
+boundary, which is why the privilege-separated deployment is documented as the mitigation
+rather than as advice; and this bound retroactively lowers the priority of the reserved-device-name
+work above — keep that code, but do not read it as evidence that Windows is hardened. If
+Windows becomes a supported platform, revisit both together, and add the CI runner first.
+
+**Privilege separation is the deployment-side half of this, and `ReconcileReport` must not
+grow a writability field.** The recommended deployment runs the reconcile as a different
+identity than the agent, so the `0644`/`0755` modes above actually deny something: the agent
+reads its instructions and cannot rewrite them or the manifest. That is the mitigation for a
+prompt-injected agent editing its own skills. The security review asked for the report to
+surface whether the managed root is writable; we declined, and the reasoning is load-bearing
+rather than a preference. 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 create
+false confidence exactly where caution is wanted. The operator's verification steps live in
+the README instead. Do not add the field.
+
 ### Deferred: bounded retries
 
 `timeout` is implemented — a monotonic deadline, checked before each retrieval, before

packages/client/src/launchdarkly_ai_server/safe_fs.py

@@ -11,6 +11,29 @@
 re-resolving a name — which is what closes the swap window rather than merely
 narrowing it. Where the platform has no ``*at()`` syscall family (Windows) the
 identical sequence runs against full paths, the per-component ``lstat`` floor.
+
+**Platform bound — this guarantee is POSIX-only, deliberately.** On POSIX the
+descriptor walk closes the swap window. On Windows it does not exist: there is no
+``*at()`` family, so the ``lstat`` floor is all that runs, and a floor is a
+check-then-use race rather than a closed window. The remedy would be
+reparse-point checks (``GetFileAttributesW``, or opening with
+``FILE_FLAG_OPEN_REPARSE_POINT``) and it is **not implemented, by decision rather
+than by oversight**: Windows is not a supported or tested platform for this
+release, and neither SDK repository has a Windows CI runner, so the checks would
+ship untested — and the TypeScript SDK could not match them in any case, because
+Node exposes no ``*at()`` family on *any* platform. Shipping them in Python alone
+would break the cross-language parity the two SDKs are held to and would trade a
+documented bound for an unverified one.
+
+Two consequences worth stating plainly rather than discovering later. First, on
+Windows write permission on the managed root is the *only* boundary, so the
+privilege-separated deployment the README documents is not advice there but the
+mitigation. Second, this bound retroactively lowers the priority of the Windows
+reserved-device-name work in ``skills_fs.py`` (``_WINDOWS_RESERVED_NAMES``): that
+code stays, because it is cheap and it keeps a managed root written on Linux
+usable when read from Windows, but it should not be read as evidence that Windows
+is a hardened target. It is not. Revisit both together if Windows becomes
+supported.
 """
 
 from __future__ import annotations

packages/client/tests/test_skills_fs.py

@@ -1394,6 +1394,136 @@ async def test_corrupt_manifest_file_is_not_destroyed(self, root: Path) -> None:
         assert _manifest_path(root).read_text(encoding="utf-8") == "{not json at all"
 
 
+# The three literal cases the security review names for the prune path.
+#
+# The distinction from ``TestCorruptManifest`` 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 that is 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.
+HOSTILE_RECORDED_PATHS: list[str] = [
+    # Absolute: the classic. A recorded path read back and unlinked as-is is a
+    # delete of an attacker-chosen file with the reconcile's privileges.
+    "/etc/passwd",
+    # Traversing: the same attack for an implementation that rejects a leading
+    # slash and then joins the rest onto the root.
+    "../../../etc/passwd",
+]
+
+
+class _UnlinkSpy:
+    """Records every ``os.unlink`` while delegating to the real one.
+
+    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.
+    """
+
+    def __init__(self) -> None:
+        self.targets: list[str] = []
+
+    def install(self, monkeypatch: pytest.MonkeyPatch) -> _UnlinkSpy:
+        real = os.unlink
+
+        def spy(path: Any, *args: Any, **kwargs: Any) -> None:
+            self.targets.append(os.fsdecode(path))
+            real(path, *args, **kwargs)
+
+        # ``safe_fs_module.os`` *is* the ``os`` module, so this covers both the
+        # descriptor-relative ``os.unlink(name, dir_fd=...)`` and the
+        # ``Path.unlink`` used on the no-``*at()`` floor.
+        monkeypatch.setattr(safe_fs_module.os, "unlink", spy)
+        return self
+
+
+class TestHostileManifestPrune:
+    """A well-formed manifest naming a path this SDK could not have written.
+
+    The manifest is untrusted input. It is 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 instead of being read back out of the entry.
+    """
+
+    @pytest.mark.parametrize("recorded", HOSTILE_RECORDED_PATHS)
+    async def test_recorded_path_outside_the_root_is_refused(
+        self, root: Path, recorded: str, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        spy = _UnlinkSpy().install(monkeypatch)
+        _write_manifest(
+            root,
+            {
+                "manifestVersion": 1,
+                "entries": {recorded: _entry("a", 1, SKILL_BODY)},
+            },
+        )
+
+        report = await write_skills([], root)
+
+        assert report.ok is False
+        action = _actions_by_key(report)["a"]
+        assert action.action == "error"
+        assert action.error is not None
+        # The refusal is about ownership of the path, not about the file's state.
+        assert "could own" in action.error
+        assert [a for a in report.actions if a.action == "removed"] == []
+        # Nothing was even attempted, let alone completed.
+        assert spy.targets == []
+        assert Path("/etc/passwd").exists()
+        # 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.
+        assert recorded in _read_manifest(root)["entries"]
+
+    async def test_entry_under_a_since_symlinked_parent_is_refused(
+        self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch
+    ) -> None:
+        """The recorded path is the SDK's own, and is still not enough.
+
+        Here 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. What changed is the disk underneath it: ``<root>/a``
+        is now a symlink to somewhere else. This is the case a validate-then-act
+        implementation fails, because the manifest and the entry are both
+        entirely legitimate; only the current state of the parent is not.
+        """
+        root = tmp_path / "skills"
+        root.mkdir()
+        elsewhere = tmp_path / "elsewhere"
+        elsewhere.mkdir()
+        victim = elsewhere / "SKILL.md"
+        victim.write_text("victim content\n", encoding="utf-8")
+
+        # Managed legitimately first, so the manifest entry is one this SDK
+        # really did write...
+        managed = _place_managed(root, "a", SKILL_BODY)
+        # ...then the parent directory is swapped for a link out of the root.
+        managed.unlink()
+        (root / "a").rmdir()
+        (root / "a").symlink_to(elsewhere, target_is_directory=True)
+
+        spy = _UnlinkSpy().install(monkeypatch)
+        report = await write_skills([], root)
+
+        assert report.ok is False
+        action = _actions_by_key(report)["a"]
+        assert action.action == "error"
+        assert action.error is not None
+        assert "symlink" in action.error
+        assert [a for a in report.actions if a.action == "removed"] == []
+        assert spy.targets == []
+        # The file the symlink pointed at is untouched, and so is the link.
+        assert victim.read_text(encoding="utf-8") == "victim content\n"
+        assert (root / "a").is_symlink()
+        assert "a/SKILL.md" in _read_manifest(root)["entries"]
+
+
 class TestWriteSkillsTelemetry:
     """Materialized / revoked signals from write_skills."""