fix(client): Agent Skills pre-release must-fix findings

packages/client/README.md

@@ -459,8 +459,9 @@ async def main():
     # 1. Which skills does this config reference? Pure projection — no I/O.
     info = await inspect_config("doc-agent", {"kind": "user", "key": "user-123"})
     if info["config"] is None:
-        # The config could not be resolved. Stop here: an empty reference list
-        # passed to write_skills would prune every skill it manages.
+        # The config could not be resolved. skill_refs raises ValueError on
+        # None rather than return [], which write_skills would read as "prune
+        # every skill it manages". Stop here and keep what is on disk.
         return
     refs = skill_refs(info["config"])          # [SkillReference(key='pdf-extraction', version=2)]
 
@@ -488,7 +489,9 @@ as above, writes only what the resolved variation asked for.
 but is not a list of `{key, version}` objects (key matching `^[a-z0-9][a-z0-9-]*$`, version an
 integer ≥ 1), including `skills: null`. One bad entry rejects the whole field, so
 `write_skills` never receives a partial list that would prune skills the config still
-references. Config parsing does not check `skills`, so a malformed field never fails
+references. It also raises when the config itself is not a dict, including the `None` a failed
+`inspect_config` returns, so an outage cannot reach `write_skills` as an empty list and prune
+every managed skill. A config with no `skills` field returns `[]`. Config parsing does not check `skills`, so a malformed field never fails
 `config().invoke()` or other core calls.
 
 **Integrity is not optional.** Content is returned only when its sha256 (lowercase hex, over
@@ -566,9 +569,11 @@ any handler. The same mapping is attached as `extra["ld_skills"]` for structured
 ERROR ld.skills.integrity_failure {"action":"withheld","event":"ld.skills.integrity_failure","expected_hash":"0000…0000","language":"python","observed_hash":"5fc8…6ec0","reason":"content hash mismatch","reason_code":"hash_mismatch","skill_key":"pdf-extraction","version":2}
 ```
 
-**`ld.skills.integrity_failure` is a stability commitment.** Match on it; it will not be
-renamed. JSON keys are sorted, so the line is byte-identical across LaunchDarkly's AI SDKs for
-the same input.
+**Match on `ld.skills.integrity_failure`.** It is the name LaunchDarkly's AI SDKs share for this
+record. While Agent Skills is experimental, the event name, its fields, and its `reason_code`
+values may change in a minor release; any such change gets a changelog entry under
+**Experimental**, so review it before upgrading. JSON keys are sorted, so the line is byte-identical across LaunchDarkly's
+AI SDKs for the same input.
 
 | Field | Description |
 |---|---|
@@ -782,7 +787,7 @@ that skips verification.
 
 | Export | Description |
 |---|---|
-| `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 or the config is not a dict. Raises `ValueError` when the field is present but malformed (including `null`), so an unreadable field never reaches a pruning reconcile as "no skills". |
+| `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. Raises `ValueError` when the config is not a dict (including the `None` a failed `inspect_config` returns) or the field is present but malformed (including `null`), so neither 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`. 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. |

packages/client/agents.md

@@ -207,7 +207,8 @@ Three layers, in increasing order of blast radius:
    typed `SkillReference` values. Pure: no network, no client, no store, no telemetry.
    It also validates the array, and **fails closed**: a present but malformed field
    (including `null`, or one bad entry) raises `ValueError` rather than returning a partial
-   list that would authorize a prune. `parse_ai_config` deliberately does not check `skills`,
+   list that would authorize a prune. So does a config that is not a dict, including the
+   `None` a failed `inspect_config` returns; only a dict with no `skills` key yields `[]`. `parse_ai_config` deliberately does not check `skills`,
    so an experimental field cannot fail a core config call (TESTING.md §0.3).
 2. **Content accessors** — `get_skill`, `get_skill_result`, `get_skills`, `all_skills` read
    through the `SkillStore` seam. Configure a store with `set_skill_store(store)`; with
@@ -299,6 +300,38 @@ resumes in place once the cause is fixed, clearing the terminal reason through
   answer) surfaces as an `HTTPError` that `_classify_status` maps to a fatal, non-retried
   failure.
 
+**A known stream event whose data is not JSON drops the connection; it is never skipped.**
+`_iter_sse` logs a WARNING and raises `_RecoverableTransportError`, as the base SDK's FDv2
+stream interrupts on a `JSONDecodeError`, so the in-flight payload is abandoned and the
+reconnect sends the last committed basis. Skipping the event would let the
+`payload-transferred` after it commit without it and advance the basis: a lost
+`delete-object` would then never be retransmitted, and a lost `put-object` in an `xfer-full`
+would revoke that skill by omission. The warning is logged in `_iter_sse` because `_run` logs
+a disconnect after a completed exchange at debug. Both cases are asserted by
+`test_a_malformed_event_abandons_its_transfer_and_keeps_the_basis`. Only the events in
+`_EVENTS_WITH_DATA` (the ones `handle` reads) are parsed: a `heart-beat` or an unknown event
+is yielded with `None` data, so its data cannot drop the connection, as `ldclient` parses only
+inside its known-event branches. Empty data on a known event is not JSON either, and drops the
+connection the same way: read as `None`, a `delete-object` would be ignored and a
+`payload-transferred` would commit with no selector.
+
+**A put or delete that is valid JSON but has no usable `key` is warned and ignored, for the
+experimental stage.** Its transfer still commits, so a keyless `delete-object` loses its
+revocation and a keyless `put-object` in an `xfer-full` revokes that skill by omission (and
+`write_skills("*")` prunes it). `ldclient` interrupts the stream instead. Interrupting is not
+strictly safer, because a server that keeps resending the object would then block every later
+change, so the choice is to be revisited before 1.0 (TESTING.md §3.25). Pinned by
+`test_a_delete_with_no_usable_key_is_ignored_and_its_transfer_commits` and
+`test_a_put_with_no_usable_key_in_an_xfer_full_revokes_by_omission`; changing the rule should fail
+them.
+
+**A `goodbye` with `catastrophe: true` is a recoverable, counted disconnect, not a fatal.**
+The Python base SDK does not read the flag and the Go SDK only logs it, so stopping delivery
+on it would leave this store the only LaunchDarkly SDK that needs `start()` after a server
+incident. `_goodbye` logs it at ERROR and returns a disconnect without `recycled`, so the
+delivery loop counts it even after a completed exchange. Asserted by
+`test_a_catastrophic_goodbye_reconnects_and_is_counted`.
+
 **Reads are memory-bounded.** `_read_bounded` (poll bodies) and
 `_iter_stream_lines`/`_iter_sse` (each line and each event) enforce `MAX_RESPONSE_BYTES`
 (64 MiB). Crossing it raises `_ResponseTooLargeError`, a fatal error: nothing from that
@@ -588,7 +621,9 @@ Do not undo any of these as a simplification:
   discriminate (`resolve_from_store` and `list_raw_objects` also log ERROR for a raising
   store), and the stdlib's default formatter drops `extra`, so an `extra`-only record is
   invisible under a plain `logging.basicConfig()`.
-- **`ld.skills.integrity_failure` is documented for customers to match on.** Never rename it.
+- **`ld.skills.integrity_failure` is documented for customers to match on.** Do not rename it
+  casually. The experimental stage allows a rename in a minor release, but only in every SDK
+  at once and with a changelog entry, because a rename silently breaks customers' alerts.
 - **`sort_keys=True` makes the line byte-identical across SDKs** (modulo `language`), since
   the other implementations build the object in alphabetical key order.
 - **Optional fields are omitted, never nulled**, so a SIEM field-existence check means
@@ -940,6 +975,6 @@ a conversation is out of reach at this layer either way.
 - `Skill.content` is opaque `bytes`. Do not add anything that parses or interprets it — no YAML library in this package's dependencies at any tier, and no accessor that decodes content.
 - Do not route skills telemetry through `client.track()`, and do not introduce an LD context anywhere in the skills path. Signals go through the `skills_core.py` emitter seam, whose default is a no-op, and only via its `record_*` functions.
 - Do not add a signal name outside the three in the Agent Skills table above — the list is an allowlist. `AgentControl Skill SDK Reference Returned` and `AgentControl Skill Content Retrieved` were considered and deliberately excluded from SDK emission.
-- Do not rename `ld.skills.integrity_failure`, and do not add an eleventh `reason_code` in one language only — both are documented compatibility surfaces. See "The integrity-failure log record" above.
+- Do not rename `ld.skills.integrity_failure` in one language only or without a changelog entry, and do not add an eleventh `reason_code` in one language only — both are documented compatibility surfaces, though the experimental stage lets either change in a minor release. See "The integrity-failure log record" above.
 - Do not relax any of the `write_skills` filesystem defenses (local key re-validation, symlink refusal, manifest-authorized destruction, corrupt-manifest fail-closed, atomic `0644` writes). Each is a deliberate security property with abuse-case tests attached.
 - Do not make `SkillStore` lookups key-only. Version is part of the lookup identity because a payload holds several versions of one key; a key-only seam cannot express a version-pinned reference.

packages/client/src/launchdarkly_ai_server/skills.py

@@ -204,21 +204,30 @@ def skill_refs(config: AiConfigRep | None) -> list[SkillReference]:
     Returns the skill references attached to a resolved AI Config.
 
     Pure: no network, store, or telemetry. Returns ``[]`` when the config has no
-    ``skills`` field, or when *config* is not a dict (for example ``None`` from a
-    failed ``inspect_config``). Typical use: ``await get_skills(skill_refs(config))``.
+    ``skills`` field. Typical use: ``await get_skills(skill_refs(config))``.
 
     The config parser does not validate ``skills``, so a malformed field does
     not fail core config calls. It is validated here instead, and rejected
     whole: ``write_skills`` with ``prune=True`` would delete the files of any
     skill missing from the list, so a partial or empty list is never returned
-    for a field that is present.
+    for a field that is present. A *config* that is not a dict, such as the
+    ``None`` a failed ``inspect_config`` returns, is rejected for the same
+    reason: read as "no skills", it would prune every managed skill.
 
     Raises:
-        ValueError: If ``skills`` is present but is not a list of ``{key,
-            version}`` objects with a valid key and an integer version >= 1.
-            This includes ``skills: null``.
+        ValueError: If *config* is not a dict (including ``None``), or if
+            ``skills`` is present but is not a list of ``{key, version}``
+            objects with a valid key and an integer version >= 1. This
+            includes ``skills: null``.
     """
-    if not isinstance(config, dict) or "skills" not in config:
+    if not isinstance(config, dict):
+        raise ValueError(
+            f"skill_refs was given {type(config).__name__}, not an AI Config. "
+            "A failed inspect_config returns None as its config; handle that "
+            "before deriving skill references, because an empty list passed to "
+            "write_skills would prune every skill it manages."
+        )
+    if "skills" not in config:
         return []
 
     raw = config["skills"]

packages/client/src/launchdarkly_ai_server/skills_fdv2.py

@@ -108,6 +108,20 @@
 _EVENT_GOODBYE = "goodbye"
 _EVENT_ERROR = "error"
 
+# The events whose data ``_FDv2Reader.handle`` reads. Only these are parsed, so
+# a ``heart-beat`` or an unknown event with data that is not JSON is ignored,
+# as ``ldclient``'s FDv2 stream ignores it.
+_EVENTS_WITH_DATA = frozenset(
+    (
+        _EVENT_SERVER_INTENT,
+        _EVENT_PUT_OBJECT,
+        _EVENT_DELETE_OBJECT,
+        _EVENT_PAYLOAD_TRANSFERRED,
+        _EVENT_GOODBYE,
+        _EVENT_ERROR,
+    )
+)
+
 _INTENT_TRANSFER_FULL = "xfer-full"
 _INTENT_TRANSFER_CHANGES = "xfer-changes"
 _INTENT_TRANSFER_NONE = "none"
@@ -482,7 +496,6 @@ class _TransferOutcome:
     committed: bool = False
     changes: list[dict[str, Any]] = field(default_factory=list)
     basis: str | None = None
-    fatal: str | None = None
     disconnect: str | None = None
     up_to_date: bool = False
     """A ``none`` intent: the content held is current. Counts as a healthy
@@ -738,14 +751,23 @@ def _goodbye(self, data: Any) -> _TransferOutcome:
         catastrophe = bool(data.get("catastrophe")) if isinstance(data, dict) else False
         silent = bool(data.get("silent")) if isinstance(data, dict) else False
         self._abandon_in_flight()
+        if catastrophe:
+            # Recoverable, as in the base SDKs: Python's does not read the flag
+            # and Go's only logs it. Not ``recycled``, so it is counted even
+            # after a completed exchange, and logged here at ERROR because the
+            # delivery loop logs a disconnect after one at debug.
+            logger.error(
+                "The FDv2 server reported a catastrophic failure (%s); "
+                "reconnecting with backoff",
+                reason,
+            )
+            return _TransferOutcome(
+                disconnect=f"server sent a catastrophic goodbye: {reason}"
+            )
         if not silent:
             # Debug only: a goodbye after a completed exchange is a routine
             # recycle, and the delivery loop warns when one is not.
             logger.debug("FDv2 connection closing: %s", reason)
-        if catastrophe:
-            return _TransferOutcome(
-                fatal=f"server sent a catastrophic goodbye: {reason}"
-            )
         return _TransferOutcome(
             disconnect=f"server said goodbye: {reason}", recycled=True
         )
@@ -1235,6 +1257,17 @@ def _iter_sse(response: Any) -> Any:
     newlines, blank line dispatches, ``:`` comments skipped. An event over
     ``MAX_RESPONSE_BYTES`` raises a fatal error and the in-flight payload is
     abandoned.
+
+    A known event whose data is not JSON, empty data included, is logged at
+    WARNING and raises ``_RecoverableTransportError``, as the base SDK's FDv2
+    stream does (``json.loads("")`` raises there too): the
+    in-flight payload is abandoned and the reconnect resumes from the last
+    committed basis. Skipping the event instead would let the
+    ``payload-transferred`` after it commit the transfer without it and advance
+    the basis past it, so a lost ``delete-object`` would never be retransmitted.
+    The warning is logged here because the delivery loop logs a disconnect
+    after a completed exchange at debug. Any other event is yielded with
+    ``None`` data, unparsed.
     """
     limit = MAX_RESPONSE_BYTES
     try:
@@ -1246,15 +1279,19 @@ def _iter_sse(response: Any) -> Any:
             if line == "":
                 if name is not None:
                     payload = "\n".join(data_lines)
-                    try:
-                        parsed = json.loads(payload) if payload else None
-                    except json.JSONDecodeError:
-                        logger.warning(
-                            "Discarding FDv2 '%s' event whose data was not JSON", name
-                        )
-                        parsed = None
-                    else:
-                        yield name, parsed
+                    parsed = None
+                    if name in _EVENTS_WITH_DATA:
+                        try:
+                            parsed = json.loads(payload)
+                        except json.JSONDecodeError as exc:
+                            message = (
+                                f"an FDv2 '{name}' event's data was not JSON "
+                                f"({exc}); the connection was dropped and "
+                                "nothing from the in-flight payload was applied"
+                            )
+                            logger.warning("%s; reconnecting", message)
+                            raise _RecoverableTransportError(message) from exc
+                    yield name, parsed
                 name = None
                 data_lines = []
                 event_bytes = 0
@@ -1796,8 +1833,6 @@ def _apply(self, name: str, data: Any) -> bool:
             self._publish_first_payload()
             if outcome.changes:
                 self._notify(outcome.changes)
-        if outcome.fatal:
-            raise _FatalTransportError(outcome.fatal)
         if outcome.disconnect:
             raise _RecoverableTransportError(
                 outcome.disconnect, recycled=outcome.recycled

packages/client/tests/test_skills.py

@@ -287,8 +287,23 @@ def test_emits_no_telemetry(self, recording_emitter: Any) -> None:
         skill_refs(self._config(skills=[{"key": "a", "version": 1}]))
         assert recording_emitter.records == []
 
-    def test_a_non_dict_config_returns_empty_list(self) -> None:
-        assert skill_refs(None) == []
+    @pytest.mark.parametrize(
+        "config",
+        [
+            pytest.param(None, id="none"),
+            pytest.param([], id="list"),
+            pytest.param("doc-agent", id="string"),
+        ],
+    )
+    def test_a_config_that_is_not_a_dict_raises(self, config: Any) -> None:
+        """``None`` is what a failed ``inspect_config`` returns as its config.
+
+        Read as "no skills", it would let ``write_skills(skill_refs(config),
+        root)``, which prunes by default, delete every managed skill during an
+        outage.
+        """
+        with pytest.raises(ValueError, match="not an AI Config"):
+            skill_refs(config)
 
     @pytest.mark.parametrize(
         "malformed",