test(client): Agent Skills — the filesystem abuse matrix (5/5)

packages/client/src/launchdarkly_ai_server/__init__.py

@@ -41,17 +41,20 @@
     InMemorySkillStore,
     all_skills,
     get_skill,
+    get_skill_result,
     get_skills,
     skill_refs,
 )
 from .skills_core import SkillStore
+from .skills_fdv2 import FDv2SkillStore, StoreDiagnostics
 from .skills_fs import (
     MANIFEST_FILENAME,
     MANIFEST_VERSION,
     SKILL_FILENAME,
     OnUnavailable,
     write_skills,
 )
+from .skills_watch import SkillWatcher, watch_skills
 from .tracking import execute_and_stream, execute_and_track, wrap_tool_handlers
 from .types import (
     NATIVE_TOOL_KEY,
@@ -85,6 +88,8 @@
     ReconcileActionKind,
     ReconcileReport,
     Skill,
+    SkillOutcome,
+    SkillOutcomeReason,
     SkillReference,
     StreamChunkEvent,
     StreamDoneEvent,
@@ -150,6 +155,7 @@
     "ReconcileActionKind",
     "ReconcileReport",
     "Skill",
+    "SkillOutcome",
     "SkillReference",
     "StreamChunkEvent",
     "StreamDoneEvent",
@@ -227,14 +233,21 @@
     # skills
     "skill_refs",
     "get_skill",
+    "get_skill_result",
     "get_skills",
     "all_skills",
     "write_skills",
     "SkillStore",
     "InMemorySkillStore",
-    # skills — the two closed-set unions a typed consumer needs to name
+    # skills — the FDv2 delivery transport, and the eager re-reconcile it enables
+    "FDv2SkillStore",
+    "StoreDiagnostics",
+    "watch_skills",
+    "SkillWatcher",
+    # skills — the three closed-set unions a typed consumer needs to name
     "ReconcileActionKind",
     "OnUnavailable",
+    "SkillOutcomeReason",
     # skills — on-disk constants, identical across languages
     "SKILL_FILENAME",
     "MANIFEST_FILENAME",

packages/client/src/launchdarkly_ai_server/safe_fs.py

@@ -11,12 +11,36 @@
 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
 
 import errno
 import os
+import re
 import secrets
 import stat
 import tempfile
@@ -191,6 +215,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 +273,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 +305,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 +314,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:

packages/client/src/launchdarkly_ai_server/skills.py

@@ -37,7 +37,7 @@
     resolve_from_store,
     verify_raw_skill,
 )
-from .types import AiConfigRep, Skill, SkillReference
+from .types import AiConfigRep, Skill, SkillOutcome, SkillReference
 from .types_validation import (
     is_valid_skill_key,
     is_valid_skill_version,
@@ -167,6 +167,22 @@ def add_listener(self, kind: str, fn: Callable[[dict[str, Any]], Any]) -> None:
         """
         self._listeners.setdefault(kind, []).append(fn)
 
+    def remove_listener(self, kind: str, fn: Callable[[dict[str, Any]], Any]) -> None:
+        """
+        Unregisters *fn* from *kind*, so a subsequent ``put`` no longer calls it.
+
+        Removes one occurrence: a callable registered twice must be removed twice.
+        Removing a callable that is not registered is a no-op, not an error, so a
+        consumer that detaches on close can do so unconditionally.
+        """
+        listeners = self._listeners.get(kind)
+        if listeners is None:
+            return
+        try:
+            listeners.remove(fn)
+        except ValueError:
+            return
+
 
 # ---------------------------------------------------------------------------
 # Reference discovery
@@ -247,6 +263,40 @@ async def get_skill(key: str, *, version: int | None = None) -> Skill | None:
     return resolve_from_store(require_store(), key, version).skill
 
 
+async def get_skill_result(key: str, *, version: int | None = None) -> SkillOutcome:
+    """
+    Retrieves one verified skill, reporting *why* when there is none.
+
+    Same retrieval, same verification, same telemetry as ``get_skill`` — the two
+    differ only in what they report. ``get_skill`` collapses "no such skill",
+    "the store raised", "that is not the version held", and "the content failed
+    integrity verification" to one ``None``; this returns a ``SkillOutcome``
+    whose ``reason`` names which of them happened, so a caller can fail closed on
+    suspected tampering while tolerating a merely-absent skill:
+
+    ```python
+    outcome = await get_skill_result("pdf-extraction")
+    if outcome.reason == "integrity_failure":
+        raise SystemExit(f"refusing to run: {outcome.detail}")
+    if outcome.skill is not None:
+        print(outcome.skill.content)
+    ```
+
+    ``detail`` is human-readable and safe to surface — it names the key and the
+    failure mode, never any skill content or filesystem path. Branch on
+    ``reason``, not on ``detail``.
+
+    Emits nothing of its own: an integrity failure has already recorded its log
+    record and its signal inside verification, and recording a second here would
+    double-count one failure. Raises ``RuntimeError`` only when no skill store is
+    configured, exactly as ``get_skill`` does.
+    """
+    resolved = resolve_from_store(require_store(), key, version)
+    return SkillOutcome(
+        skill=resolved.skill, reason=resolved.reason, detail=resolved.error
+    )
+
+
 async def get_skills(refs: Sequence[SkillReference | str]) -> list[Skill]:
     """
     Retrieves a batch of verified skills.

packages/client/src/launchdarkly_ai_server/skills_core.py

@@ -44,7 +44,7 @@
 from dataclasses import dataclass
 from typing import Any, Literal, Protocol, get_args
 
-from .types import Skill, SkillReference
+from .types import Skill, SkillOutcomeReason, SkillReference
 from .types_validation import is_valid_skill_key, is_valid_skill_version
 
 logger = logging.getLogger(__name__)
@@ -56,9 +56,9 @@
 An **internal seam value**, deliberately not exported from the package root. It
 is the string ``skills.py`` and ``skills_fs.py`` pass to ``SkillStore.get_object``
 and ``SkillStore.all_objects``, and a store adapter is free to map it onto
-whatever the transport underneath actually uses — a delivery payload may well
-carry skills under a broader kind with a narrower category, in which case
-translating that pair to this one value is the adapter's job.
+whatever the transport underneath actually uses — the value happens to match
+the kind LaunchDarkly's delivery channel uses today, but a transport that spelt
+it differently would translate, and that translation is the adapter's job.
 
 Exporting it would publish an SDK-side seam string as though it were the wire
 contract, which is a claim this side cannot make and would be hard to walk back
@@ -134,9 +134,18 @@
 
 NO_STORE_MESSAGE = (
     "No skill store is configured, so skill content cannot be retrieved. Configure "
-    'one with init_client(options={"skillStore": store}) — InMemorySkillStore is '
-    "available for local development and testing."
+    'one with init_client(options={"skillStore": store}) — FDv2SkillStore receives '
+    "content from LaunchDarkly, and InMemorySkillStore is available for local "
+    "development and testing."
 )
+"""
+The first thing a user sees when no store is configured, so it names both stores.
+
+``FDv2SkillStore`` comes first because it is the answer in production, and a
+message that offered only ``InMemorySkillStore`` would point a deployment at the
+development store. Callers match on "skill store"; keep that phrase if the
+wording changes.
+"""
 
 
 # ---------------------------------------------------------------------------
@@ -151,11 +160,17 @@ class SkillStore(Protocol):
     Duck-typed on purpose, mirroring how the LaunchDarkly client interface works
     in this package: pass any object carrying these methods.
 
-    ``add_listener(kind, fn)`` is part of the seam but
-    **optional**, which is why it is deliberately not declared here: a Protocol
-    member is required for structural compatibility, so declaring it would reject
-    every store that does not implement it. Nothing in this module calls it — it
-    exists for the delivery transport to push updates through.
+    ``add_listener(kind, fn)`` and ``remove_listener(kind, fn)`` are part of the
+    interface but **optional**, which is why they are deliberately not declared
+    here: a Protocol member is required for structural compatibility, so declaring
+    them would reject every store that does not implement them. Nothing in this
+    module calls either — they exist for the delivery transport to push updates
+    through, and for a consumer such as ``watch_skills`` to stop receiving them.
+    A store that implements ``add_listener`` should implement ``remove_listener``
+    too; consumers probe for it and skip detaching when it is absent, so an
+    older store keeps working at the cost of a listener that lives as long as
+    the store does. ``remove_listener`` removes one occurrence of *fn* under
+    *kind* and is a no-op when *fn* is not registered.
 
     The raw objects a store serves are wire-shaped, with camelCase field names
     identical across language implementations::
@@ -672,6 +687,26 @@ def newest_by_key(objects: dict[str, dict[str, Any]]) -> list[tuple[str, Any]]:
 class Resolution:
     """One key resolved against a store: the skill, or why there is none."""
 
+    reason: SkillOutcomeReason
+    """
+    Which of the five public outcomes this resolution is.
+
+    Declared first and **without a default**, so every construction site has to
+    state it. A default would be the wrong shape twice over: a contributor
+    adding a sixth internal outcome would inherit whichever token happened to be
+    the default rather than deciding which public token it maps to, and if that
+    default were ``"ok"`` a failure would publish ``ok`` with no skill attached.
+
+    Carried as a token rather than derived from ``error`` on the way out:
+    ``get_skill_result`` publishes this value, and pattern-matching prose to
+    recover a decision a caller fails closed on is exactly the fragility the
+    typed outcome exists to remove. A reviewer can read the mapping here.
+
+    Distinct from ``unavailable`` on purpose — that flag answers one question
+    (may prune run?) and this token answers a different one (what does the
+    caller learn?) — but the two can only disagree by a bug: ``unavailable`` is
+    ``True`` in exactly the ``store_unavailable`` case.
+    """
     skill: Skill | None = None
     error: str | None = None
     unavailable: bool = False
@@ -702,26 +737,33 @@ def resolve_from_store(
         raw = store.get_object(SKILL_OBJECT_KIND, key, wanted_version)
     except Exception as exc:
         logger.error("Skill store raised while retrieving '%s'", key, exc_info=True)
-        return Resolution(error=store_raised(exc), unavailable=True)
+        return Resolution(
+            reason="store_unavailable",
+            error=store_raised(exc),
+            unavailable=True,
+        )
 
     if not isinstance(raw, dict):
         return Resolution(
-            error=f"skill '{key}' is not available from the configured skill store"
+            reason="absent",
+            error=f"skill '{key}' is not available from the configured skill store",
         )
 
     skill = verify_raw_skill(raw)
     if skill is None:
         return Resolution(
-            error=f"skill '{key}' failed integrity verification and was withheld"
+            reason="integrity_failure",
+            error=f"skill '{key}' failed integrity verification and was withheld",
         )
     if wanted_version is not None and skill.version != wanted_version:
         return Resolution(
+            reason="wrong_version",
             error=(
                 f"skill '{key}' version {wanted_version} is not available "
                 f"(the store holds version {skill.version})"
-            )
+            ),
         )
-    return Resolution(skill=skill)
+    return Resolution(reason="ok", skill=skill)
 
 
 def reference_target(item: SkillReference | str) -> tuple[str, int | None]: