feat(client): Agent Skills — self-healing reconciles, and keys no filesystem can hold

packages/client/README.md

@@ -357,6 +357,32 @@ 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.
 
+**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.
+Without that, a process killed after a skill file lands but before the manifest is rewritten
+leaves that file managed-but-unrecorded, which is indistinguishable from a file you wrote
+yourself, so every later reconcile would refuse it and the skill would stay wedged until
+someone intervened. Adoption cannot weaken the guarantee above, because bytes that differ in
+any way are still refused and left untouched. Note that an adopted file becomes prunable
+like any other managed file — which is the same outcome the crash pre-empted.
+
+**A few keys are legal to an AI Config but not to a filesystem.** A key becomes a single
+directory name, so `write_skills` applies bounds of its own on top of the key grammar: no
+mainstream filesystem allows a 256-byte path component, and Windows reserves 22 MS-DOS
+device names (`con`, `prn`, `aux`, `nul`, `com1`–`com9`, `lpt1`–`lpt9`) that cannot be
+directory names there. Either one is a reported `error` action for that skill, and the
+rejection is unconditional rather than platform-gated — a managed root written from a Linux
+container is routinely read from a Windows host, so the on-disk result must not depend on
+which OS ran the write. The keys stay valid everywhere else: an AI Config referencing a skill
+named `aux` parses, and its other fields are unaffected. If you have a skill named for a
+device, rename it.
+
+**Total path length is yours to bound, not the SDK's.** The 255-byte bound above is per
+*component*; the root is your path, so `<root>` + `<key>` + `/SKILL.md` can still exceed
+Windows' 260-character `MAX_PATH` with a perfectly legal key. Choose a short managed root on
+Windows.
+
 | Export | Description |
 |---|---|
 | `skill_refs(config)` | Project a config's `skills` array into `list[SkillReference]`. Pure — no client, store, or network needed. Returns `[]` when absent. |

packages/client/agents.md

@@ -250,7 +250,26 @@ Store data is **untrusted input**; the transport is not part of the trust bounda
 - **Never write through a symlink**, in either the skill directory or the target file, on
   the write path *and* the prune path.
 - **Destructive operations only on manifest-listed paths whose `key` matches.** A file at a
-  managed path with no matching manifest entry is reported as `error` and left alone.
+  managed path with no matching manifest entry is reported as `error` and left alone —
+  *unless its bytes already are the resolved content*, in which case it is adopted (manifest
+  entry recorded, reported `skipped_current`). That single exception is what makes a
+  reconcile killed between the content writes and the final manifest rewrite recoverable
+  instead of permanently wedged, and it cannot be widened: the comparison is over the
+  verbatim bytes against the resolved `contentHash`, a read that fails is a refusal and
+  never an overwrite, and the read is bounded at `len(content) + 1` bytes so a file that
+  merely *begins* with the resolved content is refused too. Do not relax it to a prefix, a
+  length, an mtime, or the manifest's own recorded `sha256` — that field is untrusted and is
+  never a decision input. `skipped_current` is reused deliberately rather than adding an
+  `adopted` action kind; `ReconcileActionKind` is a public closed set.
+- **Temp files are swept, within the same bounds as everything else.** `atomic_write` unlinks
+  its own temp file on any exception, but a `SIGKILL` leaves one behind that no manifest
+  entry records, and a non-empty directory defeats `_prune_one`'s `rmdir` — so one orphan
+  pins a skill directory forever. The sweep is the only place this SDK removes a file the
+  manifest does not list, and it is bounded on every axis: inside `<root>/<key>/` only, for a
+  key that passes `_key_rejection_reason`; only names `safe_fs.is_temp_name` recognizes,
+  anchored at both ends and asked of `safe_fs` rather than re-spelled (a copy would drift
+  from the writer); only regular files, with the type read off the descriptor; unlinked
+  through the pinned descriptor. It never raises and never aborts a run.
 - **A corrupt manifest fails closed**: unreadable, unparseable, not an object, malformed
   `entries`, or a `manifestVersion` this release cannot read means no overwrites and no
   prunes, brand-new paths may still be written, an `error` action names the manifest, and
@@ -265,10 +284,28 @@ Store data is **untrusted input**; the transport is not part of the trust bounda
   "Descriptor-pinned filesystem access" below. Re-resolving `<root>/<key>` from its path at
   write or unlink time reopens a swap window that the checks above cannot cover.
 - **A key valid to the data model may still be unrepresentable on disk.** The model allows
-  256 characters; `NAME_MAX` is 255 bytes. `write_skills` rejects an over-long key before
-  any filesystem call, and every per-skill filesystem failure is caught at the loop so it
-  becomes an `error` action — aborting the loop would skip the manifest rewrite and orphan
-  files already written in that run.
+  256 characters; `NAME_MAX` is 255 bytes. Windows additionally reserves 22 MS-DOS device
+  names, none of which can be a directory name there: `con`, `prn`, `aux`, `nul`,
+  `com1`–`com9`, `lpt1`–`lpt9` (`com0` and `lpt0` are *not* reserved; do not add them).
+  `write_skills` rejects both before any filesystem call, and every per-skill filesystem
+  failure is caught at the loop so it becomes an `error` action — aborting the loop would
+  skip the manifest rewrite and orphan files already written in that run.
+- **Those two bounds live in `_key_rejection_reason`, not in the key grammar, and must not
+  move.** `is_valid_skill_key` / `skill_key_rejection_reason` keep admitting an over-long or
+  reserved key on purpose. `parse_ai_config` fails closed on a bad `skills` entry, so a
+  grammar-level rejection would invalidate the *entire* AI Config — model, provider,
+  instructions, tools — for a Linux customer over a Windows-only constraint; and it would
+  silently shrink `skill_refs`, which is what authorizes a prune, converting "this skill
+  fails to write on Windows" into "this skill gets deleted on Linux". `_key_rejection_reason`
+  is shared by the write and prune paths, so one edit covers both destructive paths.
+  The reserved-name check is unconditional rather than `os.name == "nt"`-gated: a root
+  written from a Linux container is routinely read from a Windows host, and neither
+  repository has a Windows CI runner (every matrix job is `ubuntu-latest`), so a gated branch
+  would be untestable — the exact condition that produced the gap. No suffix stripping and no
+  case folding are needed, because the grammar admits no `.` and no `$` (so `con.txt` and
+  `CONIN$` are unreachable) and is lowercase-only. The residual the SDK cannot check is total
+  path length: the 255-byte bound is per *component*, and the root belongs to the customer,
+  so `MAX_PATH` overflow is a README note rather than a check.
 - **A key is untrusted input everywhere it appears.** `skill_key_rejection_reason` is the
   single canonical explanation, so the config parser and the reference projection reject a
   key for the same stated reason — and so does every layer added later. A silently

packages/client/src/launchdarkly_ai_server/safe_fs.py

@@ -17,6 +17,7 @@
 
 import errno
 import os
+import re
 import secrets
 import stat
 import tempfile
@@ -191,6 +192,53 @@ def unlink_file(directory: Path, name: str, *, dir_fd: int | None) -> None:
     os.unlink(name, dir_fd=dir_fd)
 
 
+_TEMP_SUFFIX = ".tmp"
+"""Suffix on every temp file this module creates."""
+
+_TEMP_TOKEN_BYTES = 8
+"""Bytes of randomness in a temp name, as ``secrets.token_hex`` takes them."""
+
+_TEMP_TOKEN_PATTERN = re.compile(
+    # Two producers, one recognizer. The descriptor path below names its temp
+    # file with ``secrets.token_hex(_TEMP_TOKEN_BYTES)`` — twice that many
+    # lowercase hex characters. The fallback path hands naming to
+    # ``tempfile.mkstemp``, whose sequence is eight characters drawn from
+    # ``[a-z0-9_]``. Matched with ``fullmatch``, which anchors both branches at
+    # both ends, so nothing longer or otherwise-shaped is ever recognized.
+    rf"[0-9a-f]{{{_TEMP_TOKEN_BYTES * 2}}}|[a-z0-9_]{{8}}"
+)
+
+
+def temp_name_prefix(name: str) -> str:
+    """
+    The prefix every temp file for *name* is created under.
+
+    Spelled once because two callers need to agree on it: ``atomic_write``
+    creates the name, and a caller sweeping orphaned temp files left by a crash
+    has to recognize it. A copy of the format string in the sweeper would be a
+    copy that can drift out of step with the writer.
+    """
+    return f".{name}."
+
+
+def is_temp_name(candidate: str, name: str) -> bool:
+    """
+    Whether *candidate* is a name this module could have created for *name*.
+
+    The recognizer for the orphan sweep: ``atomic_write`` unlinks its temp file
+    on any exception, but a ``SIGKILL`` between the create and the rename leaves
+    it behind, and nothing else on disk records that it exists. Deliberately
+    narrow — prefix, random token, and suffix must all match, with nothing
+    before or after — because the only thing a caller does with a ``True`` here
+    is delete the file.
+    """
+    prefix = temp_name_prefix(name)
+    if not candidate.startswith(prefix) or not candidate.endswith(_TEMP_SUFFIX):
+        return False
+    token = candidate[len(prefix) : -len(_TEMP_SUFFIX)]
+    return _TEMP_TOKEN_PATTERN.fullmatch(token) is not None
+
+
 def _mkstemp_at(dir_fd: int, prefix: str) -> tuple[int, str]:
     """
     ``tempfile.mkstemp`` for a directory descriptor.
@@ -202,7 +250,7 @@ def _mkstemp_at(dir_fd: int, prefix: str) -> tuple[int, str]:
     """
     flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, "O_NOFOLLOW", 0)
     for _ in range(tempfile.TMP_MAX):
-        name = f"{prefix}{secrets.token_hex(8)}.tmp"
+        name = f"{prefix}{secrets.token_hex(_TEMP_TOKEN_BYTES)}{_TEMP_SUFFIX}"
         try:
             return os.open(name, flags, 0o600, dir_fd=dir_fd), name
         except FileExistsError:
@@ -234,7 +282,7 @@ def atomic_write(
     semantics on Windows).
     """
     at_fd = dir_fd if dir_fd is not None and SUPPORTS_DIR_FD else None
-    prefix = f".{name}."
+    prefix = temp_name_prefix(name)
     target: str | Path
 
     if at_fd is not None:
@@ -243,7 +291,7 @@ def atomic_write(
     else:
         # mkstemp opens with O_CREAT|O_EXCL, so an existing temp path is never
         # reused.
-        fd, temp = tempfile.mkstemp(dir=directory, prefix=prefix, suffix=".tmp")
+        fd, temp = tempfile.mkstemp(dir=directory, prefix=prefix, suffix=_TEMP_SUFFIX)
         target = directory / name
 
     try: