fix(client): validate skills in skill_refs, not in the config parser

packages/client/README.md

@@ -425,11 +425,12 @@ project library**, which puts every skill's `description` into the agent's conte
 skills no AI Config references and skills belonging to other teams. `write_skills(skill_refs(...), root)`,
 as above, writes only what the resolved variation asked for.
 
-**`skills` is a validated field.** Config parsing fails closed on a `skills` value that is not
-a list of `{key, version}` objects (key matching `^[a-z0-9][a-z0-9-]*$`, version an integer
-≥ 1): the whole variation is rejected, `inspect_config` returns `config: None`, and
-`extract_variation` raises. If your variations carry a custom `skills` field of a different
-shape, rename it before upgrading.
+**`skill_refs` validates the `skills` field.** It raises `ValueError` when `skills` is present
+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
+`config().invoke()` or other core calls.
 
 **Integrity is not optional.** Content is returned only when its sha256 (lowercase hex, over
 the verbatim UTF-8 bytes) matches the delivered `contentHash`, its key and version revalidate,
@@ -722,7 +723,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. A `skills` field that is present but not a list (including `null`) fails the config parse instead, 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 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". |
 | `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

@@ -205,8 +205,10 @@ Three layers, in increasing order of blast radius:
 
 1. **Reference discovery** — `skill_refs(config)` projects the config's `skills` array into
    typed `SkillReference` values. Pure: no network, no client, no store, no telemetry.
-   Validation of the array itself lives in `parse_ai_config` and is **fail closed** — one
-   malformed reference fails the whole config parse.
+   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`,
+   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
    none configured the accessors raise
@@ -527,8 +529,8 @@ Store data is **untrusted input**; the transport is not part of the trust bounda
 - **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` deliberately admit an over-long
   or reserved key, because:
-  - `parse_ai_config` fails closed on a bad `skills` entry, so a grammar rejection would
-    invalidate the *entire* AI Config for a Linux customer over a Windows-only constraint;
+  - `skill_refs` fails closed on a bad `skills` entry, so a grammar rejection would
+    reject *every* skill reference for a Linux customer over a Windows-only constraint;
   - it would also shrink `skill_refs`, which authorizes a prune, turning "fails to write on
     Windows" into "deleted on Linux".
 

packages/client/src/launchdarkly_ai_server/skills.py

@@ -28,11 +28,7 @@
     verify_raw_skill,
 )
 from .types import AiConfigRep, Skill, SkillOutcome, SkillReference
-from .types_validation import (
-    is_valid_skill_key,
-    is_valid_skill_version,
-    skill_key_rejection_reason,
-)
+from .types_validation import is_valid_skill_version, skills_field_rejection_reason
 
 logger = logging.getLogger(__name__)
 
@@ -208,46 +204,28 @@ 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. Typical use: ``await get_skills(skill_refs(config))``.
+    ``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))``.
 
-    Invalid entries (possible only in a hand-built dict; ``parse_ai_config``
-    rejects them) are dropped with a warning, because ``write_skills`` with
-    ``prune=True`` would delete a dropped skill's files.
-    """
-    if not isinstance(config, dict):
-        return []
+    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.
 
-    raw = config.get("skills")
-    if not isinstance(raw, list):
+    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``.
+    """
+    if not isinstance(config, dict) or "skills" not in config:
         return []
 
-    refs: list[SkillReference] = []
-    for index, entry in enumerate(raw):
-        if not isinstance(entry, dict):
-            logger.warning(
-                "skills[%d] is not a {key, version} object; it was dropped "
-                "from the projection",
-                index,
-            )
-            continue
-        key = entry.get("key")
-        version = entry.get("version")
-        # Branch on the TypeGuard so ``key`` narrows to ``str``.
-        if not is_valid_skill_key(key):
-            logger.warning(
-                "skills[%d].key %s; it was dropped from the projection",
-                index,
-                skill_key_rejection_reason(key),
-            )
-        elif not is_valid_skill_version(version):
-            logger.warning(
-                "skills[%d].version must be an integer >= 1; it was dropped "
-                "from the projection",
-                index,
-            )
-        else:
-            refs.append(SkillReference(key=key, version=version))
-    return refs
+    raw = config["skills"]
+    rejection = skills_field_rejection_reason(raw)
+    if rejection is not None:
+        raise ValueError(f"Invalid skills field in AI Config: {rejection}")
+    return [SkillReference(key=entry["key"], version=entry["version"]) for entry in raw]
 
 
 # ---------------------------------------------------------------------------

packages/client/src/launchdarkly_ai_server/types.py

@@ -93,8 +93,9 @@ class Message:
 """
 Raw AI config dict as returned by ``parse_ai_config``. Fields include
 ``model``, ``provider``, at least one of ``instructions`` / ``messages``, and an
-optional ``skills`` array of ``{key, version}`` references (see
-``launchdarkly_ai_server.experimental.skills.skill_refs``).
+optional ``skills`` array of ``{key, version}`` references. Parsing does not
+validate ``skills``; read it with
+``launchdarkly_ai_server.experimental.skills.skill_refs``, which does.
 """
 
 VariationMeta = dict[str, Any]

packages/client/src/launchdarkly_ai_server/types_validation.py

@@ -63,12 +63,13 @@ def _parse_tool(raw: Any, key: str) -> str | None:
     return None
 
 
-def _parse_skills(raw: Any) -> str | None:
+def skills_field_rejection_reason(raw: Any) -> str | None:
     """
-    Validates the optional ``skills`` array. Returns an error message or ``None``.
+    Why a present ``skills`` field is malformed, or ``None`` when it is valid.
 
-    Fails closed: one malformed reference fails the whole config, rather than
-    silently materializing a partial skill set.
+    Used by ``skill_refs``, not by ``parse_ai_config``: Agent Skills is
+    experimental, and a malformed field must not fail a core config call.
+    One malformed reference rejects the whole field, never a partial list.
     """
     if not isinstance(raw, list):
         return "skills must be an array of {key, version} objects"
@@ -148,11 +149,8 @@ def parse_ai_config(raw: Any) -> ParseResult:
             error={"message": "outputFormat must be an object (JSON Schema)"},
         )
 
-    # ``in``, not ``is not None``: ``skills: null`` must fail the parse. Read as
-    # "no skills", a ``prune=True`` reconcile would delete skill files on disk.
-    if "skills" in raw:
-        err = _parse_skills(raw["skills"])
-        if err:
-            return ParseFailure(success=False, error={"message": err})
+    # ``skills`` is passed through unvalidated. Agent Skills is experimental, so
+    # a malformed field must not fail a core config call (TESTING.md §0.3);
+    # ``skill_refs`` rejects it where the references are used.
 
     return ParseSuccess(success=True, data=raw)

packages/client/tests/test_lifecycle.py

@@ -785,6 +785,33 @@ async def test_returns_enabled_true_and_config_when_variation_is_enabled(
         assert result["config"]["model"]["name"] == "claude-3-5"  # type: ignore[index]
         assert result["meta"] is not None
 
+    async def test_a_malformed_skills_field_does_not_fail_core_calls(self) -> None:
+        """Agent Skills is experimental, so its field cannot break a core call
+        (TESTING.md §0.3). ``skill_refs`` rejects it instead."""
+        stub = _make_stub_client()
+        stub.variation = AsyncMock(
+            return_value={
+                "_ldMeta": {"enabled": True, "variationKey": "v1", "version": 1},
+                "model": {"name": "claude-3-5"},
+                "provider": {"name": "Anthropic"},
+                "instructions": "You are helpful.",
+                "skills": [{"key": "My_Skill", "version": 0}],
+            }
+        )
+        with patch.object(lifecycle_module, "_setup_telemetry", return_value=None):
+            await init_client(client=stub)
+        ctx = {"kind": "user", "key": "user-1"}
+        with patch(
+            "launchdarkly_ai_server.utils.to_ld_context",
+            side_effect=lambda _c, ctx: ctx,
+        ):
+            result = await inspect_config("my-flag", ctx)
+            extracted = await lifecycle_module.extract_variation("my-flag", ctx)
+
+        assert result["enabled"] is True
+        assert result["config"] is not None
+        assert extracted["config"]["model"]["name"] == "claude-3-5"
+
     async def test_preserves_model_key_and_version_on_meta(self) -> None:
         stub = _make_stub_client()
         stub.variation = AsyncMock(

packages/client/tests/test_schema.py

@@ -86,7 +86,11 @@ def test_output_format_accepted(self) -> None:
 
 class TestParseAiConfigSkills:
     """
-    Fail-closed validation of the optional ``skills`` array.
+    ``skills`` is passed through unvalidated.
+
+    Agent Skills is experimental, so a malformed ``skills`` field must not fail
+    a core config call (TESTING.md §0.3). ``skill_refs`` validates the field
+    where the references are used; see ``TestSkillRefs`` in test_skills.py.
     """
 
     def _base(self, **extra: Any) -> dict[str, Any]:
@@ -101,105 +105,31 @@ def _base(self, **extra: Any) -> dict[str, Any]:
     def test_absent_skills_is_valid(self) -> None:
         assert parse_ai_config(self._base()).success is True
 
-    def test_empty_skills_is_valid(self) -> None:
-        assert parse_ai_config(self._base(skills=[])).success is True
-
-    def test_valid_entries_accepted(self) -> None:
+    def test_valid_entries_pass_through(self) -> None:
         raw = self._base(skills=[{"key": "pdf-extraction", "version": 2}])
         result = parse_ai_config(raw)
         assert result.success is True
         assert result.data["skills"] == [{"key": "pdf-extraction", "version": 2}]
 
-    def test_multiple_valid_entries_accepted(self) -> None:
-        raw = self._base(
-            skills=[{"key": "a", "version": 1}, {"key": "b-2", "version": 10}]
-        )
-        assert parse_ai_config(raw).success is True
-
-    def test_key_at_length_bound_accepted(self) -> None:
-        raw = self._base(skills=[{"key": "a" * 256, "version": 1}])
-        assert parse_ai_config(raw).success is True
-
-    @pytest.mark.parametrize("bad_skills", ["pdf", {"key": "a"}, 3, True])
-    def test_non_array_skills_fails(self, bad_skills: Any) -> None:
-        assert parse_ai_config(self._base(skills=bad_skills)).success is False
-
-    def test_an_explicit_null_skills_fails_rather_than_reading_as_absent(
-        self,
-    ) -> None:
-        """``skills: null`` is the non-array that reads as "no skills".
-
-        It must not be treated that way. Read as absent it makes ``skill_refs``
-        return ``[]``, and a ``prune=True`` reconcile then *deletes* previously
-        materialized skill files on the strength of a field the SDK could not
-        parse — the same hazard the store path refuses, where reading a
-        malformed object as absent would let prune delete the last known-good
-        copy on disk. Failing the whole parse is the louder and safer outcome,
-        and it is why this case is pinned apart from the other non-arrays.
-        """
-        result = parse_ai_config(self._base(skills=None))
-        assert result.success is False
-        assert "skills" in result.error["message"]
-
-    def test_an_absent_skills_key_is_still_valid(self) -> None:
-        """The other half of the bullet above: *absent* is not *null*.
-
-        Rejecting ``null`` must not cost backward compatibility for the
-        configs that simply have no ``skills`` field.
-        """
-        raw = self._base()
-        assert "skills" not in raw
-        assert parse_ai_config(raw).success is True
-
-    @pytest.mark.parametrize("entry", ["pdf-extraction", 1, None, ["a", 1]])
-    def test_non_object_entry_fails(self, entry: Any) -> None:
-        assert parse_ai_config(self._base(skills=[entry])).success is False
-
-    @pytest.mark.parametrize("bad_key", [None, 1, True, {"a": 1}, ["a"]])
-    def test_missing_or_non_string_key_fails(self, bad_key: Any) -> None:
-        raw = self._base(skills=[{"key": bad_key, "version": 1}])
-        assert parse_ai_config(raw).success is False
-
-    def test_absent_key_fails(self) -> None:
-        assert parse_ai_config(self._base(skills=[{"version": 1}])).success is False
-
     @pytest.mark.parametrize(
-        "bad_key",
+        "malformed",
         [
-            "",
-            "Evil",
-            "-leading-dash",
-            ".hidden",
-            "_underscore",
-            "has space",
-            "a/b",
-            "a\\b",
-            "../escape",
-            "trailing-space ",
-            "under_score",
-            "a" * 257,
+            None,
+            "pdf",
+            {"key": "a"},
+            3,
+            ["pdf-extraction"],
+            [{"version": 1}],
+            [{"key": "Evil", "version": 1}],
+            [{"key": "../escape", "version": 1}],
+            [{"key": "a", "version": 0}],
+            [{"key": "a"}],
+            [{"key": "good", "version": 1}, {"key": "My_Skill", "version": 1}],
         ],
     )
-    def test_pattern_and_length_violations_fail(self, bad_key: str) -> None:
-        raw = self._base(skills=[{"key": bad_key, "version": 1}])
-        assert parse_ai_config(raw).success is False
-
-    @pytest.mark.parametrize("bad_version", [0, -1, 2.5, "2", None, True, [1]])
-    def test_invalid_version_fails(self, bad_version: Any) -> None:
-        raw = self._base(skills=[{"key": "a", "version": bad_version}])
-        assert parse_ai_config(raw).success is False
-
-    def test_absent_version_fails(self) -> None:
-        assert parse_ai_config(self._base(skills=[{"key": "a"}])).success is False
-
-    def test_one_bad_entry_fails_the_whole_config(self) -> None:
-        raw = self._base(
-            skills=[{"key": "good", "version": 1}, {"key": "../bad", "version": 1}]
-        )
-        assert parse_ai_config(raw).success is False
-
-    def test_error_message_mentions_skills(self) -> None:
-        raw = self._base(skills=[{"key": "../bad", "version": 1}])
-        result = parse_ai_config(raw)
-        assert result.success is False
-        assert "skills" in result.error["message"]
+    def test_a_malformed_skills_field_does_not_fail_the_parse(
+        self, malformed: Any
+    ) -> None:
+        result = parse_ai_config(self._base(skills=malformed))
+        assert result.success is True
+        assert result.data["skills"] == malformed