fix(client): Agent Skills — transport, lifecycle and spec fixes from the TypeScript review

packages/client/README.md

@@ -538,10 +538,21 @@ objects through the `SkillStore` interface and cannot tell which store produced
 customer-confidential. A mobile key (`mob-…`) or a client-side environment ID raises from the
 constructor.
 
-**The SDK key goes only where you pointed it.** `base_uri` must be `https://` (plain `http://`
-is refused, except to a loopback host for a local test double), and redirects are never
-followed, so a 3xx from a proxy or a misconfigured private instance stops delivery rather than
-forwarding the key to whatever host the `Location` header names.
+**The SDK key goes only where you pointed it.** `base_uri` and `stream_uri` must each be
+`https://` (plain `http://` is refused, except to a loopback host for a local test double),
+and redirects are never followed, so a 3xx from a proxy or a misconfigured private instance
+stops delivery rather than forwarding the key to whatever host the `Location` header names.
+
+**Polling and streaming have separate hosts.** LaunchDarkly serves `/sdk/poll` from
+`https://sdk.launchdarkly.com` and `/sdk/stream` from `https://stream.launchdarkly.com`, so
+the defaults are a pair. Pass `base_uri` on its own and it applies to both — what a relay or a
+private instance serving both endpoints from one host needs — or pass `stream_uri` as well to
+override them independently.
+
+**`close()` is final.** A closed store still answers from the content it received, but
+delivery cannot be resumed: `start()` afterwards raises. That is what gives `close()` a
+postcondition you can rely on — delivery has stopped — even when its join times out.
+Construct a new store to resume.
 
 **Reads are memory-bounded.** No poll body or streamed event is held past `MAX_RESPONSE_BYTES`
 (64 MiB, far above any real payload); one that crosses it is dropped without being applied, the
@@ -582,16 +593,16 @@ 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. |
+| `skill_refs(config)` | Project a config's `skills` array into `list[SkillReference]`. Pure — no client, store, or network needed. Returns `[]` when the field is absent. A `skills` field that is present but not an array — including an explicit `null` — fails the config parse instead, so a field the SDK could not read never reaches a pruning reconcile as "no skills". |
 | `get_skill(key, *, version=None)` | One verified skill, or `None`. `version=None` means newest available; a specific `version` matches exactly. Raises only when no store is configured. |
 | `get_skill_result(key, *, version=None)` | The same retrieval, reporting **why**: a frozen `SkillOutcome` with `.skill`, `.reason` (`ok` / `absent` / `integrity_failure` / `store_unavailable` / `wrong_version`), and `.detail`. Use it to fail closed on tampering — see *Failing closed on tampering* above. Raises only when no store is configured. |
 | `get_skills(refs)` | Batch form. Accepts `SkillReference` values and bare key strings (string = latest). Results follow input order; missing or unverifiable entries are omitted. |
 | `all_skills()` | Every verified skill the store holds, one per key at its newest version. |
 | `write_skills(skills, root, *, prune=True, timeout=10.0, on_unavailable="keep")` | Materialize skills under `root`, returning a `ReconcileReport`. `prune` removes formerly-managed skills no longer requested. `on_unavailable="raise"` raises instead of reporting when content cannot be retrieved. Raises `ValueError` for an unusable root, a negative `timeout`, or an unrecognised `on_unavailable`. **Performs synchronous filesystem I/O — see the note below.** |
-| `SkillStore` | The structural interface content arrives through: `get_object(kind, key, version=None)`, `all_objects(kind)`, optional `is_initialized()`, `add_listener(kind, fn)` / `remove_listener(kind, fn)`. A store without `is_initialized()` is treated as initialized. |
+| `SkillStore` | The structural interface content arrives through: `get_object(kind, key, version=None)`, `all_objects(kind)`, optional `is_initialized()`, `add_listener(kind, fn)` / `remove_listener(kind, fn)`. A store without `is_initialized()` is treated as initialized. Both shipped stores deliver only the skill kind, so `add_listener` on any other kind raises rather than being recorded and silently never firing. |
 | `InMemorySkillStore(objects=None)` | A dict-backed store with `put(raw)`, for local development and testing. Holds several versions of a key. |
-| `FDv2SkillStore(sdk_key, *, base_uri=…, mode="stream", …)` | The delivery transport: a store fed by LaunchDarkly over the SDK-facing FDv2 channel. `start()`, `wait_for_skills(timeout)`, `is_initialized()`, `close()`, `diagnostics`, `failed`; also a context manager. **Server-side only** — a mobile key or client-side environment ID raises. See *Receiving skills from LaunchDarkly* above. |
-| `watch_skills(skills, root, …)` | `write_skills` plus a re-reconcile on every delivery change. Returns `(initial report, SkillWatcher)`; close the watcher when done. Revocation then takes effect within `debounce` of arriving rather than at the next restart. |
+| `FDv2SkillStore(sdk_key, *, base_uri=…, stream_uri=…, mode="stream", …)` | The delivery transport: a store fed by LaunchDarkly over the SDK-facing FDv2 channel. `start()`, `wait_for_skills(timeout)`, `is_initialized()`, `close()`, `diagnostics`, `failed`; also a context manager. `base_uri` and `stream_uri` are separate hosts, defaulting to LaunchDarkly's polling and streaming origins; `base_uri` alone covers both. `close()` is **final** — `start()` afterwards raises. **Server-side only** — a mobile key or client-side environment ID raises. See *Receiving skills from LaunchDarkly* above. |
+| `watch_skills(skills, root, *, debounce=0.5, on_reconcile=None, …)` | `write_skills` plus a re-reconcile on every delivery change. Returns `(initial report, SkillWatcher)`; close the watcher when done. Revocation then takes effect within `debounce` of arriving rather than at the next restart. `debounce` is in **seconds** and must be non-negative and finite; `on_reconcile` is called with each *subsequent* report, the initial one being returned directly. One watcher per root. |
 | `StoreDiagnostics` | What the transport has seen: `payloads_transferred`, `skill_objects_received`, `objects_ignored`, `objects_revoked`, `hashless_objects`, `connection_failures`, `last_error`. |
 
 Configure the store with `init_client(options={"skillStore": store})`. With none configured,

packages/client/agents.md

@@ -673,11 +673,25 @@ 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
+no Windows CI runner in either repository, so the checks would ship unverified, and there is
+no second implementation to check them against — the TypeScript SDK has no Windows story
+either. Implementing them in Python alone would trade a documented bound for an unverified
+one.
+
+The parity argument used to be stronger than that, and the correction matters because the
+old wording is now wrong. It read: Node exposes no `*at()` family on *any* platform, so its
+racy floor is universal rather than Windows-only. The first half is still true and the
+second is not. `*at()` is not the only way to address a child relative to a pinned inode:
+TypeScript commit `0a15b10` added a `SUPPORTS_PROC_FD` probe and `/proc/self/fd/<fd>/<name>`
+addressing, which the Linux kernel resolves from the inode the descriptor holds rather than
+from the name it was opened under. That **closes** the swap window on Linux exactly as
+`*at()` does here, so TypeScript's `lstat` floor now applies on macOS and Windows only —
+the same shape as this side's, not a universal one.
+
+None of which reopens the decision above. It never rested on TypeScript being equally
+exposed; it rests on there being no Windows CI runner to verify the checks against, which is
+still the case in both repositories. 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

packages/client/src/launchdarkly_ai_server/skills.py

@@ -153,10 +153,21 @@ def add_listener(self, kind: str, fn: Callable[[dict[str, Any]], Any]) -> None:
         """
         Registers *fn* to be called with each raw object ``put`` under *kind*.
 
-        Only ``kind == SKILL_OBJECT_KIND`` is ever notified, because ``put`` only
-        accepts skill objects; a listener registered under any other kind is
-        recorded and never fires.
+        Only ``kind == SKILL_OBJECT_KIND`` is ever notified, because ``put``
+        only accepts skill objects — so a registration for any other kind
+        **raises** rather than being recorded and silently never firing. This is
+        the reason ``watch_skills`` refuses a store with no ``add_listener`` at
+        all: a listener that never fires looks exactly like one whose objects
+        never changed, and a store that accepted the registration has promised
+        something it cannot keep. ``FDv2SkillStore.add_listener`` refuses the
+        same way.
         """
+        if kind != SKILL_OBJECT_KIND:
+            raise ValueError(
+                f"InMemorySkillStore notifies only {SKILL_OBJECT_KIND!r} "
+                f"changes, so a listener on {kind!r} would never fire. Register "
+                f"it on {SKILL_OBJECT_KIND!r}."
+            )
         self._listeners.setdefault(kind, []).append(fn)
 
     def remove_listener(self, kind: str, fn: Callable[[dict[str, Any]], Any]) -> None:

packages/client/src/launchdarkly_ai_server/skills_core.py

@@ -421,7 +421,15 @@ def verified_bytes(
     key: str, content: str | bytes, expected_hash: str, version: int
 ) -> VerifiedContent | VerificationFailure:
     """
-    The whole content half of integrity verification: encode, size, hash.
+    The whole content half of integrity verification: size, encoding, hash.
+
+    The order is fixed, so content failing two classes reports one determined
+    code: **size** before **encoding** before **hash**. Size first is what makes
+    over-cap content carrying a lone surrogate report ``over_size_cap`` rather
+    than ``not_utf8`` — without a fixed order that input's code is whichever
+    check the implementation happens to reach first, and §3.21's "one code per
+    failure class" rule cannot be tested against it. The shape checks that
+    precede all three are in ``verify_raw_skill``.
 
     Accepts either shape content arrives in. A wire-shaped ``str`` is UTF-8
     encoded here, once — the only place that encode happens. ``bytes`` is an
@@ -439,6 +447,7 @@ def verified_bytes(
     first one's verdict forward: that puts a "trust the value computed upstream"
     branch inside the one function whose job is not to.
     """
+    encodable = True
     if isinstance(content, bytes):
         encoded = content
     else:
@@ -447,17 +456,18 @@ def verified_bytes(
         except UnicodeEncodeError:
             # json.loads turns a "\ud800" escape into an unpaired surrogate,
             # which has no UTF-8 encoding — so there are no bytes the server
-            # could have hashed. Never reach for errors="surrogatepass": it
-            # would fabricate bytes that could satisfy the hash comparison.
-            reason = "content is not encodable as UTF-8"
-            record_integrity_failure(
-                key,
-                reason,
-                reason_code="not_utf8",
-                version=version,
-                expected_hash=expected_hash,
-            )
-            return VerificationFailure(reason)
+            # could have hashed.
+            #
+            # These replacement bytes exist only to measure the content against
+            # the size cap, so that the *reported* failure follows the fixed
+            # order above. They are never hashed and never returned: the
+            # ``not_utf8`` branch below returns before the hash comparison, and
+            # nothing else reads ``encoded`` on this path. That is the whole
+            # reason ``errors="replace"`` is safe here and
+            # ``errors="surrogatepass"`` would not be anywhere — fabricated
+            # bytes that reached the comparison could satisfy it.
+            encodable = False
+            encoded = content.encode("utf-8", errors="replace")
 
     if len(encoded) > MAX_SKILL_CONTENT_BYTES:
         reason = (
@@ -473,6 +483,17 @@ def verified_bytes(
         )
         return VerificationFailure(reason)
 
+    if not encodable:
+        reason = "content is not encodable as UTF-8"
+        record_integrity_failure(
+            key,
+            reason,
+            reason_code="not_utf8",
+            version=version,
+            expected_hash=expected_hash,
+        )
+        return VerificationFailure(reason)
+
     # sha256, lowercase hex, over the verbatim bytes — no canonicalization and
     # no content parsing of any kind anywhere in the integrity path.
     observed_hash = hashlib.sha256(encoded).hexdigest()