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

packages/client/README.md

@@ -356,8 +356,9 @@ LaunchDarkly's AI SDKs for the same input.
 |---|---|
 | `event` | Always `ld.skills.integrity_failure`. |
 | `action` | Always `withheld` — the content was not returned to your code. |
-| `skill_key` | The skill key, or `<invalid-key>` when the delivered key was itself malformed. |
-| `version` | The delivered version. Omitted when it was not a valid version. |
+| `skill_key` | The skill key **requested**, or `<invalid-key>` when the key was itself malformed. |
+| `served_key` | Only on `key_mismatch`: the key the store actually answered under. Same redaction as `skill_key`. Omitted on every other failure mode. |
+| `version` | The delivered version. Omitted when it was not a valid version, and on `key_mismatch`. |
 | `expected_hash` | The delivered `contentHash`, or `<not-a-sha256-digest>` when it was not one. Omitted when none was delivered. |
 | `observed_hash` | The sha256 the SDK computed. Omitted when the failure happened before anything was hashed. |
 | `reason_code` | A stable token naming the failure mode — see below. |
@@ -378,12 +379,22 @@ could carry it, never appears in the record; neither does any filesystem path.
 | `not_utf8` | The content string had no UTF-8 encoding, so there are no bytes that could have been hashed. |
 | `over_size_cap` | The content exceeded the SDK's local size cap. |
 | `hash_mismatch` | The computed sha256 did not match the delivered `contentHash`. |
+| `key_mismatch` | The store answered under a different key than the one requested. Carries an extra `served_key` field naming the key it answered under, and — uniquely — records **no** `AgentControl Skill Integrity Failure` signal. |
 
-**`hash_mismatch` is the one worth paging on.** The other seven describe a malformed or
+**`hash_mismatch` is the one worth paging on.** The other eight describe a malformed or
 truncated payload; a mismatch means content was delivered whose bytes are not the bytes
 LaunchDarkly hashed, which is a possible **active-tampering** signal. Alert on it, and
 treat `expected_hash` / `observed_hash` as the evidence pair.
 
+**`key_mismatch` is the one code that reaches this record without the product signal.** It
+is decided after verification has passed, and its usual cause is a bug in a custom
+`SkillStore` adapter — a stale cache entry, a colliding key, a wrong index lookup — rather
+than tampering, so it does not inflate LaunchDarkly's own integrity counter. It still
+reaches this record, because a store substituting one skill for another is worth seeing,
+and a rule on `ld.skills.integrity_failure` catches it without modification. Treat it like
+`hash_mismatch` if `FDv2SkillStore` is your only store; behind a custom adapter, suspect
+the adapter first.
+
 #### Failing closed on tampering
 
 The log record above is the operator's surface. `get_skill_result` is the application's:
@@ -538,10 +549,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
@@ -554,6 +576,14 @@ outage the store keeps serving the last content it received and `write_skills`'
 `on_unavailable="keep"` leaves managed files alone — an outage must not read as "everything
 was revoked".
 
+**Without the watcher, the revocation bound is process lifetime.** A deployment that calls
+`write_skills` once at boot and never runs `watch_skills` reconciles exactly once, so a skill
+revoked in LaunchDarkly after boot stays on disk — and in the agent's context — until the
+process reconciles again. For such a deployment, a restart (or an explicit re-run of
+`write_skills`) is the incident-response action when a skill must be pulled immediately.
+Neither path closes the already-loaded window: content an agent has already read stays in
+that conversation regardless, and no layer of this SDK can recall it.
+
 **One network timeout, and its default depends on the mode.** `read_timeout` bounds every
 socket operation of a request, connecting included. In `mode="poll"` it bounds the whole
 request and defaults to 10 seconds; in `mode="stream"` it bounds each wait for the next bytes
@@ -582,16 +612,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

@@ -548,9 +548,11 @@ Each of those choices is load-bearing; do not undo one as a simplification.
 - **`reason_code` is in the record only.** The signal's property set is the allowlist above
   and does not grow; the local record is where the detection vocabulary lives.
 
-`reason_code` is a **closed vocabulary of exactly eight tokens** — `IntegrityReasonCode`, a
-`Literal`, so a typo at a call site is a type error — one per `record_integrity_failure`
-call site, and the same eight in every language implementation:
+`reason_code` is a **closed vocabulary of exactly nine tokens** — `IntegrityReasonCode`, a
+`Literal`, so a typo at a call site is a type error — and the same nine in every language
+implementation. Eight are one per `record_integrity_failure` call site; the ninth,
+`key_mismatch`, comes from `record_key_mismatch` and is the only one that fires the log
+record **without** the product signal:
 
 | `reason_code` | Call site |
 |---|---|
@@ -562,8 +564,17 @@ call site, and the same eight in every language implementation:
 | `not_utf8` | `verified_bytes` — `UnicodeEncodeError` on encode (wire-`str` path only; a `Skill` already holds bytes) |
 | `over_size_cap` | `verified_bytes` — over `MAX_SKILL_CONTENT_BYTES` |
 | `hash_mismatch` | `verified_bytes` — observed sha256 != `contentHash` |
+| `key_mismatch` | `resolve_from_store` — the served object's own `key` is not the key requested. **Log record only, no signal**, and carries a `served_key` field no other record has |
 
-Adding a ninth failure mode means widening `IntegrityReasonCode`, adding a case to
+`key_mismatch` cannot join `REASON_CODE_CASES`: that table is driven uniformly through
+`all_skills`, and this code is decided at the retrieval boundary after `verify_raw_skill`
+has passed, so a listing cannot reach it. It is unioned into the exhaustiveness assertion
+instead, and covered by `test_key_mismatch_records_the_log_but_not_the_signal`. The
+record-without-signal split is deliberate — a mismatch is usually a broken store adapter
+rather than an attacker, and LaunchDarkly's counter must not fill with customers' adapter
+bugs — and tests pin both directions. Do not "fix" it by emitting the signal.
+
+Adding a tenth failure mode means widening `IntegrityReasonCode`, adding a case to
 `REASON_CODE_CASES` in `test_skills.py` (whose exhaustiveness assertion fails otherwise),
 documenting it in the README table, **and** doing the same in the other language SDKs. A
 token added on one side only is a drift bug: a customer's detection rule stops matching
@@ -673,11 +684,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
@@ -869,6 +894,13 @@ key out of the requested set, so prune deletes the last known-good copy on disk
 a routine `removed` with `report.ok` still true. Tampered content must never be able to
 trigger deletion.
 
+### 6. Expecting revocation to reach a boot-only `write_skills` deployment
+
+Without `watch_skills`, the revocation bound is process lifetime: a skill revoked after boot
+stays on disk until the process reconciles again, so a restart (or an explicit re-run of
+`write_skills`) is the incident-response action — and content an agent has already read into
+a conversation is out of reach at this layer either way.
+
 ---
 
 ## Adding a New Export

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: